Sema: Fix generics invariant violations in override checking

Override checking checks if the derived declaration's generic
signature is compatible with the base, but it does this after
doing a bunch of other checks which feed potentially invalid
type parameters to generic signature queries.

Now that the requirement machine is stricter about this kind
of this, re-organize some code to get around this.

Unfortunately this regresses a diagnostic, because we reject
candidates with mismatched generic requirements earlier in
the process.

Author: slavapestov

include/swift/AST/DiagnosticsSema.def

@@ -2708,10 +2708,6 @@ WARNING(override_swift3_objc_inference,Deprecation,
         "override of %0 %1 from extension of %2 depends on deprecated "
         "inference of '@objc'",
         (DescriptiveDeclKind, DeclName, Identifier))
-ERROR(override_method_different_generic_sig,none,
-      "overridden method %0 has generic signature %1 which is incompatible with "
-      "base method's generic signature %2; expected generic signature to be %3",
-      (DeclBaseName, StringRef, StringRef, StringRef))
 
 // Inheritance
 ERROR(duplicate_inheritance,none,

lib/AST/ASTContext.cpp

@@ -5150,11 +5150,22 @@ ASTContext::getOverrideGenericSignature(const ValueDecl *base,
 bool ASTContext::overrideGenericSignatureReqsSatisfied(
     const ValueDecl *base, const ValueDecl *derived,
     const OverrideGenericSignatureReqCheck direction) {
+  auto *baseCtx = base->getAsGenericContext();
+  auto *derivedCtx = derived->getAsGenericContext();
+
+  if (baseCtx->isGeneric() != derivedCtx->isGeneric())
+    return false;
+
+  if (baseCtx->isGeneric() &&
+      (baseCtx->getGenericParams()->size() !=
+       derivedCtx->getGenericParams()->size()))
+    return false;
+
   auto sig = getOverrideGenericSignature(base, derived);
   if (!sig)
     return true;
 
-  auto derivedSig = derived->getAsGenericContext()->getGenericSignature();
+  auto derivedSig = derivedCtx->getGenericSignature();
 
   switch (direction) {
   case OverrideGenericSignatureReqCheck::BaseReqSatisfiedByDerived:

lib/Sema/CodeSynthesis.cpp

@@ -541,8 +541,9 @@ computeDesignatedInitOverrideSignature(ASTContext &ctx,
       auto lookupConformanceFn =
           [&](CanType depTy, Type substTy,
               ProtocolDecl *proto) -> ProtocolConformanceRef {
-        if (auto conf = subMap.lookupConformance(depTy, proto))
-          return conf;
+        if (depTy->getRootGenericParam()->getDepth() < superclassDepth)
+          if (auto conf = subMap.lookupConformance(depTy, proto))
+            return conf;
 
         return ProtocolConformanceRef(proto);
       };
@@ -1125,9 +1126,7 @@ static void addImplicitInheritedConstructorsToClass(ClassDecl *decl) {
           continue;
 
         auto type = swift::getMemberTypeForComparison(ctor, nullptr);
-        auto parentType = swift::getMemberTypeForComparison(superclassCtor, ctor);
-
-        if (isOverrideBasedOnType(ctor, type, superclassCtor, parentType)) {
+        if (isOverrideBasedOnType(ctor, type, superclassCtor)) {
           alreadyDeclared = true;
           break;
         }

lib/Sema/TypeCheckDeclOverride.cpp

@@ -146,13 +146,11 @@ areAccessorsOverrideCompatible(const AbstractStorageDecl *storage,
 }
 
 bool swift::isOverrideBasedOnType(const ValueDecl *decl, Type declTy,
-                                  const ValueDecl *parentDecl,
-                                  Type parentDeclTy) {
+                                  const ValueDecl *parentDecl) {
   auto genericSig =
       decl->getInnermostDeclContext()->getGenericSignatureOfContext();
 
   auto canDeclTy = declTy->getCanonicalType(genericSig);
-  auto canParentDeclTy = parentDeclTy->getCanonicalType(genericSig);
 
   auto declIUOAttr = decl->isImplicitlyUnwrappedOptional();
   auto parentDeclIUOAttr = parentDecl->isImplicitlyUnwrappedOptional();
@@ -167,18 +165,33 @@ bool swift::isOverrideBasedOnType(const ValueDecl *decl, Type declTy,
   // We can still succeed with a subtype match later in
   // OverrideMatcher::match().
   if (decl->getDeclContext()->getSelfClassDecl()) {
-    if (auto declGenericCtx = decl->getAsGenericContext()) {
+    if (auto declCtx = decl->getAsGenericContext()) {
+      auto *parentCtx = parentDecl->getAsGenericContext();
+
+      if (declCtx->isGeneric() != parentCtx->isGeneric())
+        return false;
+
+      if (declCtx->isGeneric() &&
+          (declCtx->getGenericParams()->size() !=
+           parentCtx->getGenericParams()->size()))
+        return false;
+
       auto &ctx = decl->getASTContext();
       auto sig = ctx.getOverrideGenericSignature(parentDecl, decl);
-
       if (sig &&
-          declGenericCtx->getGenericSignature().getCanonicalSignature() !=
+          declCtx->getGenericSignature().getCanonicalSignature() !=
               sig.getCanonicalSignature()) {
         return false;
       }
     }
   }
 
+  auto parentDeclTy = getMemberTypeForComparison(parentDecl, decl);
+  if (parentDeclTy->hasError())
+    return false;
+
+  auto canParentDeclTy = parentDeclTy->getCanonicalType(genericSig);
+
   // If this is a constructor, let's compare only parameter types.
   if (isa<ConstructorDecl>(decl)) {
     // Within a protocol context, check for a failability mismatch.
@@ -285,6 +298,11 @@ static bool areOverrideCompatibleSimple(ValueDecl *decl,
     auto genParentDecl = parentDecl->getAsGenericContext();
     if (genDecl->isGeneric() != genParentDecl->isGeneric())
       return false;
+
+    if (genDecl->isGeneric() &&
+        (genDecl->getGenericParams()->size() !=
+         genParentDecl->getGenericParams()->size()))
+      return false;
   }
 
   // Factory initializers cannot be overridden.
@@ -908,13 +926,22 @@ SmallVector<OverrideMatch, 2> OverrideMatcher::match(
     (void)parentMethod;
     (void)parentStorage;
 
-    // Check whether the types are identical.
-    auto parentDeclTy = getMemberTypeForComparison(parentDecl, decl);
-    if (parentDeclTy->hasError())
-      continue;
+    // If the generic requirements don't match, don't try anything else below,
+    // because it will compute an invalid interface type by applying malformed
+    // substitutions.
+    if (isClassOverride()) {
+      using Direction = ASTContext::OverrideGenericSignatureReqCheck;
+      if (decl->getAsGenericContext()) {
+        if (!ctx.overrideGenericSignatureReqsSatisfied(
+                parentDecl, decl, Direction::DerivedReqSatisfiedByBase)) {
+          continue;
+        }
+      }
+    }
 
+    // Check whether the types are identical.
     Type declTy = getDeclComparisonType();
-    if (isOverrideBasedOnType(decl, declTy, parentDecl, parentDeclTy)) {
+    if (isOverrideBasedOnType(decl, declTy, parentDecl)) {
       matches.push_back({parentDecl, true});
       continue;
     }
@@ -944,6 +971,7 @@ SmallVector<OverrideMatch, 2> OverrideMatcher::match(
     }
 
     auto declFnTy = getDeclComparisonType()->getAs<AnyFunctionType>();
+    auto parentDeclTy = getMemberTypeForComparison(parentDecl, decl);
     auto parentDeclFnTy = parentDeclTy->getAs<AnyFunctionType>();
     if (declFnTy && parentDeclFnTy) {
       auto paramsAndResultMatch = [=]() -> bool {
@@ -1102,26 +1130,6 @@ bool OverrideMatcher::checkOverride(ValueDecl *baseDecl,
     emittedMatchError = true;
   }
 
-  if (isClassOverride()) {
-    auto baseGenericCtx = baseDecl->getAsGenericContext();
-    auto derivedGenericCtx = decl->getAsGenericContext();
-
-    using Direction = ASTContext::OverrideGenericSignatureReqCheck;
-    if (baseGenericCtx && derivedGenericCtx) {
-      if (!ctx.overrideGenericSignatureReqsSatisfied(
-              baseDecl, decl, Direction::DerivedReqSatisfiedByBase)) {
-        auto newSig = ctx.getOverrideGenericSignature(baseDecl, decl);
-        diags.diagnose(decl, diag::override_method_different_generic_sig,
-                       decl->getBaseName(),
-                       derivedGenericCtx->getGenericSignature()->getAsString(),
-                       baseGenericCtx->getGenericSignature()->getAsString(),
-                       newSig->getAsString());
-        diags.diagnose(baseDecl, diag::overridden_here);
-        emittedMatchError = true;
-      }
-    }
-  }
-
   // If we have an explicit ownership modifier and our parent doesn't,
   // complain.
   auto parentAttr =

lib/Sema/TypeChecker.h

@@ -1261,7 +1261,7 @@ Type getMemberTypeForComparison(const ValueDecl *member,
 /// Determine whether the given declaration is an override by comparing type
 /// information.
 bool isOverrideBasedOnType(const ValueDecl *decl, Type declTy,
-                           const ValueDecl *parentDecl, Type parentDeclTy);
+                           const ValueDecl *parentDecl);
 
 /// Determine whether the given declaration is an operator defined in a
 /// protocol. If \p type is not null, check specifically whether \p decl

test/attr/attr_override.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -swift-version 5
+// RUN: %target-typecheck-verify-swift -swift-version 5 -requirement-machine=verify
 
 @override // expected-error {{'override' can only be specified on class members}} {{1-11=}} expected-error {{'override' is a declaration modifier, not an attribute}} {{1-2=}}
 func virtualAttributeCanNotBeUsedInSource() {}
@@ -545,21 +545,21 @@ class SR_4206_DerivedConcrete_2<T>: SR_4206_BaseGeneric_2<T> {
 // Base class generic w/ method generic, derived class generic w/ method generic but different requirement
 
 class SR_4206_BaseGeneric_3<T> {
-  func foo<T>(arg: T) {} // expected-note {{overridden declaration is here}}
+  func foo<T>(arg: T) {}
 }
 
 class SR_4206_DerivedGeneric_3<T>: SR_4206_BaseGeneric_3<T> {
-  override func foo<T: SR_4206_Protocol_1>(arg: T) {} // expected-error {{overridden method 'foo' has generic signature <T, T where T : SR_4206_Protocol_1> which is incompatible with base method's generic signature <T, T>; expected generic signature to be <T, T>}}
+  override func foo<T: SR_4206_Protocol_1>(arg: T) {} // expected-error {{method does not override any method from its superclass}}
 }
 
 // Base class not generic w/ method generic, derived class not generic w/ method generic but different requirement
 
 class SR_4206_BaseConcrete_4 {
-  func foo<T>(arg: T) {} // expected-note {{overridden declaration is here}}
+  func foo<T>(arg: T) {}
 }
 
 class SR_4206_DerivedConcrete_4: SR_4206_BaseConcrete_4 {
-  override func foo<T: SR_4206_Protocol_1>(arg: T) {} // expected-error {{overridden method 'foo' has generic signature <T where T : SR_4206_Protocol_1> which is incompatible with base method's generic signature <T>; expected generic signature to be <T>}}
+  override func foo<T: SR_4206_Protocol_1>(arg: T) {} // expected-error {{method does not override any method from its superclass}}
 }
 
 // Base class not generic w/ method generic, derived class not generic w/ method generic but removed requirement
@@ -575,22 +575,22 @@ class SR_4206_DerivedConcrete_5: SR_4206_BaseConcrete_5 {
 // Base class not generic w/ method generic, derived class generic w/ method generic but different requirement
 
 class SR_4206_BaseConcrete_6 {
-  func foo<T: SR_4206_Protocol_2>(arg: T) {} // expected-note {{overridden declaration is here}}
+  func foo<T: SR_4206_Protocol_2>(arg: T) {}
 }
 
 class SR_4206_DerivedGeneric_6<T>: SR_4206_BaseConcrete_6 {
-  override func foo<T: SR_4206_Protocol_1>(arg: T) {} // expected-error {{overridden method 'foo' has generic signature <T, T where T : SR_4206_Protocol_1> which is incompatible with base method's generic signature <T where T : SR_4206_Protocol_2>; expected generic signature to be <T, T where T : SR_4206_Protocol_2>}}
+  override func foo<T: SR_4206_Protocol_1>(arg: T) {} // expected-error {{method does not override any method from its superclass}}
 }
 
 // Contextual where clauses on non-generic members
 
 class SR_4206_Base_7<T> {
-  func foo1() where T: SR_4206_Protocol_1 {} // expected-note {{overridden declaration is here}}
+  func foo1() where T: SR_4206_Protocol_1 {}
   func foo2() where T: SR_4206_Protocol_1 {}
 }
 
 class SR_4206_Derived_7<T>: SR_4206_Base_7<T> {
-  override func foo1() where T: SR_4206_Protocol_2 {} // expected-error {{overridden method 'foo1' has generic signature <T where T : SR_4206_Protocol_2> which is incompatible with base method's generic signature <T where T : SR_4206_Protocol_1>; expected generic signature to be <T where T : SR_4206_Protocol_1>}}
+  override func foo1() where T: SR_4206_Protocol_2 {} // expected-error {{method does not override any method from its superclass}}
 
   override func foo2() {} // OK
 }
@@ -624,10 +624,10 @@ class SR_4206_Derived_9<T>: SR_4206_Base_9<T> {
 // Override with constraint on a non-inherited generic param
 
 class SR_4206_Base_10<T> {
-  func foo() where T: SR_4206_Protocol_1 {} // expected-note {{overridden declaration is here}}
+  func foo() where T: SR_4206_Protocol_1 {}
 }
 class SR_4206_Derived_10<T, U>: SR_4206_Base_10<T> {
-  override func foo() where U: SR_4206_Protocol_1 {} // expected-error {{overridden method 'foo' has generic signature <T, U where U : SR_4206_Protocol_1> which is incompatible with base method's generic signature <T where T : SR_4206_Protocol_1>; expected generic signature to be <T, U where T : SR_4206_Protocol_1>}}
+  override func foo() where U: SR_4206_Protocol_1 {} // expected-error {{method does not override any method from its superclass}}
 }
 
 // Override with return type specialization
@@ -710,3 +710,19 @@ public extension SR_11740_Base where F: SR_11740_Q {
 extension SR_11740_Derived where F: SR_11740_P {
     public static func foo(_: A) {}
 }
+
+// Make sure we don't crash on generic requirement mismatch
+protocol P3 {}
+
+protocol P4 {
+  associatedtype T
+}
+
+class Base {
+  func method<T: P3>(_: T) {}
+  func method<T: P4>(_: T) where T.T : P3 {}
+}
+
+class Derived: Base {
+  override func method<T: P3>(_: T) {}
+}

test/decl/circularity.swift

@@ -84,8 +84,12 @@ open class G1<A> {
 class C3: G1<A>, P {
     // expected-error@-1 {{type 'C3' does not conform to protocol 'P'}}
     // expected-error@-2 {{cannot find type 'A' in scope}}
+    // expected-note@-3 {{through reference here}}
     override func run(a: A) {}
     // expected-error@-1 {{method does not override any method from its superclass}}
+    // expected-error@-2 {{circular reference}}
+    // expected-note@-3 2 {{through reference here}}
+    // expected-note@-4 {{while resolving type 'A'}}
 }
 
 // Another case that triggers circular override checking.