[Type checker] Make redeclaration checking validate fewer declarations.

Redeclaration checking was validating all declarations with the same
base name as the given declaration (and in the same general nominal
type), even when it was trivial to determine that the declarations
could not be conflicting. Separate out the easy structural checks
(based on kind, full name, instance vs. non-instance member, etc.) and
perform those first, before validation.

Fixes SR-6558, a case where redeclaration checking caused some
unnecessary recursion in the type checker.

Author: DougGregor

include/swift/AST/Decl.h

@@ -213,10 +213,6 @@ struct OverloadSignature {
   /// The full name of the declaration.
   DeclName Name;
 
-  /// The interface type of the declaration, when relevant to the
-  /// overload signature.
-  CanType InterfaceType;
-
   /// The kind of unary operator.
   UnaryOperatorKind UnaryOperator = UnaryOperatorKind::None;
 
@@ -231,7 +227,8 @@ struct OverloadSignature {
 };
 
 /// Determine whether two overload signatures conflict.
-bool conflicting(const OverloadSignature& sig1, const OverloadSignature& sig2);
+bool conflicting(const OverloadSignature& sig1, const OverloadSignature& sig2,
+                 bool skipProtocolExtensionCheck = false);
 
 
 /// Decl - Base class for all declarations in Swift.
@@ -2276,9 +2273,13 @@ class ValueDecl : public Decl {
   /// Retrieve the declaration that this declaration overrides, if any.
   ValueDecl *getOverriddenDecl() const;
 
-  /// Compute the overload signature for this declaration.
+  /// Compute the untyped overload signature for this declaration.
   OverloadSignature getOverloadSignature() const;
 
+  /// Retrieve the type used to describe this entity for the purposes of
+  /// overload resolution.
+  CanType getOverloadSignatureType() const;
+
   /// Returns true if the decl requires Objective-C interop.
   ///
   /// This can be true even if there is no 'objc' attribute on the declaration.

lib/AST/Decl.cpp

@@ -1654,27 +1654,34 @@ ValueDecl *ValueDecl::getOverriddenDecl() const {
 }
 
 bool swift::conflicting(const OverloadSignature& sig1,
-                        const OverloadSignature& sig2) {
+                        const OverloadSignature& sig2,
+                        bool skipProtocolExtensionCheck) {
   // A member of a protocol extension never conflicts with a member of a
   // protocol.
-  if (sig1.InProtocolExtension != sig2.InProtocolExtension)
+  if (!skipProtocolExtensionCheck &&
+      sig1.InProtocolExtension != sig2.InProtocolExtension)
     return false;
 
   // If the base names are different, they can't conflict.
   if (sig1.Name.getBaseName() != sig2.Name.getBaseName())
     return false;
-  
+
+  // If one is an operator and the other is not, they can't conflict.
+  if (sig1.UnaryOperator != sig2.UnaryOperator)
+    return false;
+
+  // If one is an instance and the other is not, they can't conflict.
+  if (sig1.IsInstanceMember != sig2.IsInstanceMember)
+    return false;
+
   // If one is a compound name and the other is not, they do not conflict
   // if one is a property and the other is a non-nullary function.
   if (sig1.Name.isCompoundName() != sig2.Name.isCompoundName()) {
     return !((sig1.IsProperty && sig2.Name.getArgumentNames().size() > 0) ||
              (sig2.IsProperty && sig1.Name.getArgumentNames().size() > 0));
   }
 
-  return sig1.Name == sig2.Name &&
-         sig1.InterfaceType == sig2.InterfaceType &&
-         sig1.UnaryOperator == sig2.UnaryOperator &&
-         sig1.IsInstanceMember == sig2.IsInstanceMember;
+  return sig1.Name == sig2.Name;
 }
 
 static Type mapSignatureFunctionType(ASTContext &ctx, Type type,
@@ -1791,63 +1798,68 @@ OverloadSignature ValueDecl::getOverloadSignature() const {
   signature.Name = getFullName();
   signature.InProtocolExtension
     = getDeclContext()->getAsProtocolExtensionContext();
+  signature.IsInstanceMember = isInstanceMember();
+  signature.IsProperty = isa<VarDecl>(this);
 
-  // Functions, initializers, and de-initializers include their
-  // interface types in their signatures as well as whether they are
-  // instance members.
-  if (auto afd = dyn_cast<AbstractFunctionDecl>(this)) {
-    signature.InterfaceType =
-        mapSignatureFunctionType(
-            getASTContext(), getInterfaceType(),
-            /*topLevelFunction=*/true,
-            /*isMethod=*/afd->getImplicitSelfDecl() != nullptr,
-            /*isInitializer=*/isa<ConstructorDecl>(afd),
-             afd->getNumParameterLists())->getCanonicalType();
-
-    signature.IsInstanceMember = isInstanceMember();
-    // Unary operators also include prefix/postfix.
-    if (auto func = dyn_cast<FuncDecl>(this)) {
-      if (func->isUnaryOperator()) {
-        signature.UnaryOperator = func->getAttrs().getUnaryOperatorKind();
-      }
+  if (auto func = dyn_cast<FuncDecl>(this)) {
+    if (func->isUnaryOperator()) {
+      signature.UnaryOperator = func->getAttrs().getUnaryOperatorKind();
     }
-  } else if (isa<SubscriptDecl>(this)) {
-    signature.InterfaceType = getInterfaceType()->getCanonicalType();
+  }
 
-    // If the subscript occurs within a generic extension context,
-    // consider the generic signature of the extension.
-    if (auto ext = dyn_cast<ExtensionDecl>(getDeclContext())) {
-      if (auto genericSig = ext->getGenericSignature()) {
-        if (auto funcTy = signature.InterfaceType->getAs<AnyFunctionType>()) {
-          signature.InterfaceType
-            = GenericFunctionType::get(genericSig,
-                                       funcTy->getParams(),
-                                       funcTy->getResult(),
-                                       funcTy->getExtInfo())
-                ->getCanonicalType();
-        }
-      }
-    }
-  } else if (isa<VarDecl>(this)) {
-    signature.IsProperty = true;
-    signature.IsInstanceMember = isInstanceMember();
+  return signature;
+}
 
-    // If the property occurs within a generic extension context,
+CanType ValueDecl::getOverloadSignatureType() const {
+  if (auto afd = dyn_cast<AbstractFunctionDecl>(this)) {
+    return mapSignatureFunctionType(
+                           getASTContext(), getInterfaceType(),
+                           /*topLevelFunction=*/true,
+                           /*isMethod=*/afd->getImplicitSelfDecl() != nullptr,
+                           /*isInitializer=*/isa<ConstructorDecl>(afd),
+                           afd->getNumParameterLists())->getCanonicalType();
+  }
+
+  if (isa<SubscriptDecl>(this)) {
+    CanType interfaceType = getInterfaceType()->getCanonicalType();
+
+    // If the subscript declaration occurs within a generic extension context,
     // consider the generic signature of the extension.
-    if (auto ext = dyn_cast<ExtensionDecl>(getDeclContext())) {
-      if (auto genericSig = ext->getGenericSignature()) {
-        ASTContext &ctx = getASTContext();
-        signature.InterfaceType
-          = GenericFunctionType::get(genericSig,
-                                     TupleType::getEmpty(ctx),
-                                     TupleType::getEmpty(ctx),
-                                     AnyFunctionType::ExtInfo())
+    auto ext = dyn_cast<ExtensionDecl>(getDeclContext());
+    if (!ext) return interfaceType;
+
+    auto genericSig = ext->getGenericSignature();
+    if (!genericSig) return interfaceType;
+
+    if (auto funcTy = interfaceType->getAs<AnyFunctionType>()) {
+      return GenericFunctionType::get(genericSig,
+                                      funcTy->getParams(),
+                                      funcTy->getResult(),
+                                      funcTy->getExtInfo())
               ->getCanonicalType();
-      }
     }
+
+    return interfaceType;
   }
 
-  return signature;
+  if (isa<VarDecl>(this)) {
+    // If the variable declaration occurs within a generic extension context,
+    // consider the generic signature of the extension.
+    auto ext = dyn_cast<ExtensionDecl>(getDeclContext());
+    if (!ext) return CanType();
+
+    auto genericSig = ext->getGenericSignature();
+    if (!genericSig) return CanType();
+
+    ASTContext &ctx = getASTContext();
+    return GenericFunctionType::get(genericSig,
+                                    TupleType::getEmpty(ctx),
+                                    TupleType::getEmpty(ctx),
+                                    AnyFunctionType::ExtInfo())
+             ->getCanonicalType();
+  }
+
+  return CanType();
 }
 
 void ValueDecl::setIsObjC(bool Value) {

lib/AST/LookupVisibleDecls.cpp

@@ -668,35 +668,6 @@ template <> struct DenseMapInfo<FoundDeclTy> {
 
 namespace {
 
-/// Similar to swift::conflicting, but lenient about protocol extensions which
-/// don't affect code completion's concept of overloading.
-static bool relaxedConflicting(const OverloadSignature &sig1,
-                               const OverloadSignature &sig2) {
-
-  // If the base names are different, they can't conflict.
-  if (sig1.Name.getBaseName() != sig2.Name.getBaseName())
-    return false;
-
-  // If one is a compound name and the other is not, they do not conflict
-  // if one is a property and the other is a non-nullary function.
-  if (sig1.Name.isCompoundName() != sig2.Name.isCompoundName()) {
-    return !((sig1.IsProperty && sig2.Name.getArgumentNames().size() > 0) ||
-             (sig2.IsProperty && sig1.Name.getArgumentNames().size() > 0));
-  }
-
-  // Allow null property types to match non-null ones, which only happens when
-  // one property is from a generic extension and the other is not.
-  if (sig1.InterfaceType != sig2.InterfaceType) {
-    if (!sig1.IsProperty || !sig2.IsProperty)
-      return false;
-    if (sig1.InterfaceType && sig2.InterfaceType)
-      return false;
-  }
-
-  return sig1.Name == sig2.Name && sig1.UnaryOperator == sig2.UnaryOperator &&
-         sig1.IsInstanceMember == sig2.IsInstanceMember;
-}
-
 /// Hack to guess at whether substituting into the type of a declaration will
 /// be okay.
 /// FIXME: This is awful. We should either have Type::subst() work for
@@ -803,11 +774,12 @@ class OverrideFilteringConsumer : public VisibleDeclConsumer {
     }
 
     auto FoundSignature = VD->getOverloadSignature();
-    if (FoundSignature.InterfaceType && shouldSubst &&
-        shouldSubstIntoDeclType(FoundSignature.InterfaceType)) {
+    auto FoundSignatureType = VD->getOverloadSignatureType();
+    if (FoundSignatureType && shouldSubst &&
+        shouldSubstIntoDeclType(FoundSignatureType)) {
       auto subs = BaseTy->getMemberSubstitutionMap(M, VD);
-      if (auto CT = FoundSignature.InterfaceType.subst(subs))
-        FoundSignature.InterfaceType = CT->getCanonicalType();
+      if (auto CT = FoundSignatureType.subst(subs))
+        FoundSignatureType = CT->getCanonicalType();
     }
 
     for (auto I = PossiblyConflicting.begin(), E = PossiblyConflicting.end();
@@ -820,14 +792,18 @@ class OverrideFilteringConsumer : public VisibleDeclConsumer {
       }
 
       auto OtherSignature = OtherVD->getOverloadSignature();
-      if (OtherSignature.InterfaceType && shouldSubst &&
-          shouldSubstIntoDeclType(OtherSignature.InterfaceType)) {
+      auto OtherSignatureType = OtherVD->getOverloadSignatureType();
+      if (OtherSignatureType && shouldSubst &&
+          shouldSubstIntoDeclType(OtherSignatureType)) {
         auto subs = BaseTy->getMemberSubstitutionMap(M, OtherVD);
-        if (auto CT = OtherSignature.InterfaceType.subst(subs))
-          OtherSignature.InterfaceType = CT->getCanonicalType();
+        if (auto CT = OtherSignatureType.subst(subs))
+          OtherSignatureType = CT->getCanonicalType();
       }
 
-      if (relaxedConflicting(FoundSignature, OtherSignature)) {
+      if (conflicting(FoundSignature, OtherSignature, true) &&
+          (FoundSignatureType == OtherSignatureType ||
+           FoundSignature.Name.isCompoundName() !=
+             OtherSignature.Name.isCompoundName())) {
         if (VD->getFormalAccess() > OtherVD->getFormalAccess()) {
           PossiblyConflicting.erase(I);
           PossiblyConflicting.insert(VD);

lib/AST/NameLookup.cpp

@@ -193,7 +193,7 @@ bool swift::removeShadowedDecls(SmallVectorImpl<ValueDecl*> &decls,
     // constrained extensions, so use the overload signature's
     // type. This is layering a partial fix upon a total hack.
     if (auto asd = dyn_cast<AbstractStorageDecl>(decl))
-      signature = asd->getOverloadSignature().InterfaceType;
+      signature = asd->getOverloadSignatureType();
 
     // If we've seen a declaration with this signature before, note it.
     auto &knownDecls =

lib/Sema/TypeCheckDecl.cpp

@@ -899,6 +899,7 @@ static void checkRedeclaration(TypeChecker &tc, ValueDecl *current) {
   // Compare this signature against the signature of other
   // declarations with the same name.
   OverloadSignature currentSig = current->getOverloadSignature();
+  CanType currentSigType = current->getOverloadSignatureType();
   ModuleDecl *currentModule = current->getModuleContext();
   for (auto other : otherDefinitions) {
     // Skip invalid declarations and ourselves.
@@ -914,6 +915,12 @@ static void checkRedeclaration(TypeChecker &tc, ValueDecl *current) {
     if (currentDC->isTypeContext() != other->getDeclContext()->isTypeContext())
       continue;
 
+    // Check whether the overload signatures conflict (ignoring the type for
+    // now).
+    auto otherSig = other->getOverloadSignature();
+    if (!conflicting(currentSig, otherSig))
+      continue;
+
     // Validate the declaration.
     tc.validateDecl(other);
     if (other->isInvalid() || !other->hasInterfaceType())
@@ -946,8 +953,12 @@ static void checkRedeclaration(TypeChecker &tc, ValueDecl *current) {
       break;
     }
 
+    // Get the overload signature type.
+    CanType otherSigType = other->getOverloadSignatureType();
+
     // If there is another conflict, complain.
-    if (conflicting(currentSig, other->getOverloadSignature())) {
+    if (currentSigType == otherSigType ||
+        currentSig.Name.isCompoundName() != otherSig.Name.isCompoundName()) {
       // If the two declarations occur in the same source file, make sure
       // we get the diagnostic ordering to be sensible.
       if (auto otherFile = other->getDeclContext()->getParentSourceFile()) {

test/IDE/complete_value_expr.swift

@@ -1650,24 +1650,26 @@ struct dedupS : dedupP {
 
 func testDeDuped(_ x: dedupS) {
   x#^PROTOCOL_EXT_DEDUP_1^#
-// FIXME: Should produce 3 items
-// PROTOCOL_EXT_DEDUP_1: Begin completions, 5 items
+// FIXME: Should produce 3 items (?)
+// PROTOCOL_EXT_DEDUP_1: Begin completions, 6 items
 // PROTOCOL_EXT_DEDUP_1: Decl[InstanceMethod]/CurrNominal:   .foo()[#Int#]; name=foo()
 // PROTOCOL_EXT_DEDUP_1: Decl[InstanceVar]/CurrNominal:      .bar[#Int#]; name=bar
 // PROTOCOL_EXT_DEDUP_1: Decl[Subscript]/CurrNominal:        [{#Int#}][#Int#]; name=[Int]
 // PROTOCOL_EXT_DEDUP_1: End completions
 }
 func testDeDuped2(_ x: dedupP) {
   x#^PROTOCOL_EXT_DEDUP_2^#
-// PROTOCOL_EXT_DEDUP_2: Begin completions, 3 items
+// FIXME: Should produce 3 items (?)
+// PROTOCOL_EXT_DEDUP_2: Begin completions, 4 items
 // PROTOCOL_EXT_DEDUP_2: Decl[InstanceMethod]/CurrNominal:   .foo()[#dedupP.T#]; name=foo()
 // PROTOCOL_EXT_DEDUP_2: Decl[InstanceVar]/CurrNominal:      .bar[#dedupP.T#]; name=bar
 // PROTOCOL_EXT_DEDUP_2: Decl[Subscript]/CurrNominal:        [{#Self.T#}][#Self.T#]; name=[Self.T]
 // PROTOCOL_EXT_DEDUP_2: End completions
 }
 func testDeDuped3<T : dedupP where T.T == Int>(_ x: T) {
+// FIXME: Should produce 3 items (?)
   x#^PROTOCOL_EXT_DEDUP_3^#
-// PROTOCOL_EXT_DEDUP_3: Begin completions, 3 items
+// PROTOCOL_EXT_DEDUP_3: Begin completions, 4 items
 // PROTOCOL_EXT_DEDUP_3: Decl[InstanceMethod]/Super:   .foo()[#Int#]; name=foo()
 // PROTOCOL_EXT_DEDUP_3: Decl[InstanceVar]/Super:      .bar[#Int#]; name=bar
 // PROTOCOL_EXT_DEDUP_3: Decl[Subscript]/Super:        [{#Self.T#}][#Self.T#]; name=[Self.T]

validation-test/compiler_crashers_2_fixed/0126-sr5905.swift

@@ -1,4 +1,4 @@
-// RUN: not %target-swift-frontend %s -typecheck
+// RUN: %target-swift-frontend %s -emit-ir -o /dev/null
 protocol VectorIndex {
     associatedtype Vector8 : Vector where Vector8.Index == Self, Vector8.Element == UInt8
 }

validation-test/compiler_crashers/28790-conformance-getwitness-requirement-nullptr-getdecl-already-have-a-non-optional-w.swift renamed to validation-test/compiler_crashers_fixed/28790-conformance-getwitness-requirement-nullptr-getdecl-already-have-a-non-optional-w.swift

@@ -5,6 +5,6 @@
 // See https://swift.org/LICENSE.txt for license information
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 
-// REQUIRES: asserts
-// RUN: not --crash %target-swift-frontend %s -emit-ir
+
+// RUN: not %target-swift-frontend %s -emit-ir
 class a:P{class a{}{}func a:a{}func a{}}@objc protocol P{{}func a