Sema: Improve the 'protocol method can't impose new requirements on Self' check

This check had two problems. First, it would assert upon encountering
a layout requirement, due to an unimplemented code path.

A more fundamental issue is that the logic wasn't fully sound, because
it would miss certain cases, for example:

    protocol P {
      associatedtype A

      func run<B: Equatable>(_: B) where B == Self.A
    }

Here, the reduced type of `Self.A` is `B`, and at first glance, the
requirement `B: Equatable` appears to be fine. However, this
is actually a new requirement on `Self`, and the protocol be rejected.

Now that we can change the reduction order by assigning weights to
generic parameters, this check can be implemented in a better way,
by building a new generic signature first, where all generic
parameters introduced by the protocol method, like 'B' above, are
assigned a non-zero weight.

With this reduction order, any type that is equivalent to
a member type of `Self` will have a reduced type rooted in `Self`,
at which point the previous syntactic check becomes sound.

Since this may cause us to reject code we accepted previously,
the type checker now performs the check once: first on the original
signature, which may miss certain cases, and then again on the new
signature built with the weighted reduction order.

If the first check fails, we diagnose an error. If the second
check fails, we only diagnose a warning.

However, this warning will become an error soon, and it really
can cause compiler crashes and miscompiles to have a malformed
protocol like this.

Fixes rdar://116938972.

Author: slavapestov

include/swift/AST/DiagnosticEngine.h

@@ -401,6 +401,12 @@ namespace swift {
     /// until the next major language version.
     InFlightDiagnostic &warnUntilFutureSwiftVersion();
 
+    InFlightDiagnostic &warnUntilFutureSwiftVersionIf(bool shouldLimit) {
+      if (!shouldLimit)
+        return *this;
+      return warnUntilFutureSwiftVersion();
+    }
+
     /// Limit the diagnostic behavior to warning until the specified version.
     ///
     /// This helps stage in fixes for stricter diagnostics as warnings

lib/Sema/TypeCheckGeneric.cpp

@@ -267,49 +267,157 @@ OpaqueResultTypeRequest::evaluate(Evaluator &evaluator,
   return opaqueDecl;
 }
 
-/// Determine whether the given type is \c Self, an associated type of \c Self,
-/// or a concrete type.
-static bool isSelfDerivedOrConcrete(Type protoSelf, Type type) {
-  // Check for a concrete type.
-  if (!type->hasTypeParameter())
-    return true;
+static bool checkProtocolSelfRequirementsImpl(
+    ASTContext &ctx, ProtocolDecl *proto, ValueDecl *decl,
+    GenericSignature originalSig,
+    GenericSignature effectiveSig,
+    bool downgrade,
+    bool *hasSameTypeRequirement) {
+  if (hasSameTypeRequirement)
+    *hasSameTypeRequirement = false;
+
+  for (auto req : effectiveSig.getRequirements()) {
+    if (req.getKind() == RequirementKind::SameType)
+      if (hasSameTypeRequirement)
+        *hasSameTypeRequirement = true;
+
+    // The conformance of 'Self' to the protocol is okay.
+    if (req.getKind() == RequirementKind::Conformance &&
+        req.getProtocolDecl() == proto &&
+        req.getFirstType()->isEqual(ctx.TheSelfType))
+      continue;
+
+    auto isSelfDerived = [&](Type t) -> bool {
+      return t->getRootGenericParam()->isEqual(ctx.TheSelfType);
+    };
+
+    // If the requirement's subject type is rooted in an inner generic
+    // parameter, this requirement is okay.
+    if (!isSelfDerived(req.getFirstType()))
+      continue;
+
+    Type firstType = req.getFirstType();
+    Type secondType;
+    if (req.getKind() == RequirementKind::Layout)
+      secondType = ctx.getAnyObjectConstraint();
+    else
+      secondType = req.getSecondType();
 
-  if (type->isTypeParameter() &&
-      type->getRootGenericParam()->isEqual(protoSelf))
+    // Self.A.B == <<inner generic parameter>> is OK.
+    if (req.getKind() == RequirementKind::SameType &&
+        secondType->isTypeParameter() &&
+        !isSelfDerived(secondType))
+      continue;
+
+    // Anything else is a new requirement on 'Self', which is not allowed.
+
+    // Downgrade even harder in this case, since the old logic always missed it.
+    if (secondType->hasTypeParameter() && !secondType->isTypeParameter())
+      downgrade = true;
+
+    // Re-sugar the types, since effectiveSig might be canonical.
+    firstType = originalSig->getSugaredType(firstType);
+    secondType = originalSig->getSugaredType(secondType);
+    static_assert((unsigned)RequirementKind::LAST_KIND == 4,
+                  "update %select in diagnostic!");
+
+    ctx.Diags.diagnose(decl, diag::requirement_restricts_self, decl,
+                       firstType.getString(),
+                       static_cast<unsigned>(req.getKind()),
+                       secondType.getString())
+        // FIXME: This should become an unconditional error since violating
+        // this invariant can introduce compiler and run time crashes.
+        .warnUntilFutureSwiftVersionIf(downgrade);
     return true;
+  }
 
   return false;
 }
 
 // For a generic requirement in a protocol, make sure that the requirement
 // set didn't add any requirements to Self or its associated types.
 void TypeChecker::checkProtocolSelfRequirements(ValueDecl *decl) {
-  // For a generic requirement in a protocol, make sure that the requirement
-  // set didn't add any requirements to Self or its associated types.
+  // Make sure the generic signature of a protocol method doesn't
+  // impose any new requirements on Self or one of its member types.
   if (auto *proto = dyn_cast<ProtocolDecl>(decl->getDeclContext())) {
+    auto *genCtx = decl->getAsGenericContext();
+    ASSERT(genCtx != nullptr);
+
+    // If it's not generic and it doesn't have a where clause, there is
+    // nothing to check.
+    if (!genCtx->getGenericParams() && !genCtx->getTrailingWhereClause())
+      return;
+
     auto &ctx = proto->getASTContext();
-    auto protoSelf = proto->getSelfInterfaceType();
-    auto sig = decl->getInnermostDeclContext()->getGenericSignatureOfContext();
+    auto sig = genCtx->getGenericSignature();
+
+    bool hasSameTypeRequirement = false;
+
+    // Perform the check as it was formerly implemented first, by looking at
+    // the syntactic requirements of the original generic signature.
+    if (checkProtocolSelfRequirementsImpl(ctx, proto, decl, sig, sig,
+                                          /*downgrade=*/false,
+                                          &hasSameTypeRequirement)) {
+      return;
+    }
+
+    // If the generic signature was sufficiently simple, we're done.
+    if (!hasSameTypeRequirement)
+      return;
+
+    // The quick check doesn't catch everything when same-type requirements
+    // are involved, so the correct thing is to build a new signature where
+    // the innermost generic parameters added by the protocol method now have
+    // a non-zero weight. In this signature, any type that can be made
+    // equivalent to a member type of Self will have a reduced type rooted
+    // in Self.
+    SmallVector<GenericTypeParamType *, 2> params;
+    SmallVector<Requirement, 2> reqs;
+
+    auto transformParam = [&](GenericTypeParamType *paramTy)
+        -> GenericTypeParamType * {
+      if (paramTy->getDepth() == sig->getMaxDepth()) {
+        return GenericTypeParamType::get(
+            paramTy->getParamKind(),
+            paramTy->getDepth(),
+            paramTy->getIndex(),
+            /*weight=*/1,
+            paramTy->getValueType(), ctx);
+      }
+
+      return cast<GenericTypeParamType>(paramTy->getCanonicalType());
+    };
+
+    for (auto *paramTy : sig.getGenericParams())
+      params.push_back(transformParam(paramTy));
+
+    auto transformType = [&](Type t) -> Type {
+      return t.transformRec([&](TypeBase *t) -> std::optional<Type> {
+        if (auto *paramTy = dyn_cast<GenericTypeParamType>(t))
+          return transformParam(paramTy);
+
+        return std::nullopt;
+      })->getCanonicalType();
+    };
+
     for (auto req : sig.getRequirements()) {
-      // If one of the types in the requirement is dependent on a non-Self
-      // type parameter, this requirement is okay.
-      if (!isSelfDerivedOrConcrete(protoSelf, req.getFirstType()) ||
-          !isSelfDerivedOrConcrete(protoSelf, req.getSecondType()))
-        continue;
-
-      // The conformance of 'Self' to the protocol is okay.
-      if (req.getKind() == RequirementKind::Conformance &&
-          req.getProtocolDecl() == proto &&
-          req.getFirstType()->is<GenericTypeParamType>())
-        continue;
-
-      static_assert((unsigned)RequirementKind::LAST_KIND == 4,
-                    "update %select in diagnostic!");
-      ctx.Diags.diagnose(decl, diag::requirement_restricts_self, decl,
-                         req.getFirstType().getString(),
-                         static_cast<unsigned>(req.getKind()),
-                         req.getSecondType().getString());
+      if (req.getKind() != RequirementKind::Layout) {
+        reqs.emplace_back(req.getKind(),
+                          transformType(req.getFirstType()),
+                          transformType(req.getSecondType()));
+      } else {
+        reqs.emplace_back(req.getKind(),
+                          transformType(req.getFirstType()),
+                          req.getLayoutConstraint());
+      }
     }
+
+    auto weightedSig = buildGenericSignature(
+        ctx, GenericSignature(), params, reqs, /*allowInverses=*/false);
+
+    // Repeat the check with the new signature.
+    checkProtocolSelfRequirementsImpl(ctx, proto, decl, sig, weightedSig,
+                                      /*downgrade=*/true, nullptr);
   }
 }
 

test/decl/protocol/req/unsatisfiable.swift

@@ -97,3 +97,32 @@ class C7<T>: P7 { // expected-error {{type 'C7<T>' does not conform to protocol
 // expected-note@-1 {{add stubs for conformance}}
   typealias A = T
 }
+
+// This used to just crash.
+protocol LayoutConstraint {
+  associatedtype A
+
+  func f<T>(_: T) where A: AnyObject
+  // expected-error@-1 {{instance method requirement 'f' cannot add constraint 'Self.A: AnyObject' on 'Self'}}
+}
+
+protocol Q {
+  associatedtype A3
+}
+
+// We missed these cases originally.
+protocol ComplexDerivation {
+  associatedtype A1
+  associatedtype A2: Q
+
+  func bad1<B: Equatable>(_: B) where B == Self.A1
+  // expected-warning@-1 {{instance method requirement 'bad1' cannot add constraint 'Self.A1: Equatable' on 'Self'; this will be an error in a future Swift language mode}}
+
+  func bad2<B>(_: B) where A1 == [B]
+  // expected-warning@-1 {{instance method requirement 'bad2' cannot add constraint 'Self.A1 == [B]' on 'Self'; this will be an error in a future Swift language mode}}
+
+  func good<B>(_: B) where A2 == B  // This is fine
+
+  func bad3<B, C>(_: B, _: C) where A2 == B, B.A3 == [C]
+  // expected-warning@-1 {{instance method requirement 'bad3' cannot add constraint 'Self.A2.A3 == Array<C>' on 'Self'; this will be an error in a future Swift language mode}}
+}

validation-test/compiler_crashers_2/16638651735c2c8.swift renamed to validation-test/compiler_crashers_2_fixed/16638651735c2c8.swift

@@ -1,5 +1,5 @@
 // {"kind":"typecheck","signature":"swift::TypeChecker::checkProtocolSelfRequirements(swift::ValueDecl*)"}
-// RUN: not --crash %target-swift-frontend -typecheck %s
+// RUN: not %target-swift-frontend -typecheck %s
 protocol a {
   associatedtype b
   func c () where b : AnyObject