Pre-Request Cleanup For Implicit Constructors

Push through an easy refactoring to the way we validate and install
implicit constructors.  This patch would be NFC but for a regression
test that now must diagnose.  swiftlang#26159 changed validation order in such
a way that the code in validation-test-macosx-x86_64/compiler_crashers_2_fixed/0124-sr5825.swift
used to be accepted.  This patch once again changes validation order, so
we now reject this code, restoring the behavior seen on all prior
versions of Swift.

On its face, this test should work.  In order for it to do so, witness
matching has to be smarter about the declarations it asks for their
interface type, or it will risk these circular constructions
accidentally being accepted or rejected on a whim.

Author: CodaFi

lib/Sema/CodeSynthesis.cpp

@@ -920,17 +920,6 @@ static void addImplicitInheritedConstructorsToClass(ClassDecl *decl) {
 
   decl->setAddedImplicitInitializers();
 
-  auto &ctx = decl->getASTContext();
-  SmallVector<std::pair<ValueDecl *, Type>, 4> declaredInitializers;
-  for (auto member : decl->getMembers()) {
-    if (auto ctor = dyn_cast<ConstructorDecl>(member)) {
-      if (!ctor->isInvalid()) {
-        auto type = getMemberTypeForComparison(ctx, ctor, nullptr);
-        declaredInitializers.push_back({ctor, type});
-      }
-    }
-  }
-
   // We can only inherit initializers if we have a superclass.
   // FIXME: We should be bailing out earlier in the function, but unfortunately
   // that currently regresses associated type inference for cases like
@@ -942,6 +931,7 @@ static void addImplicitInheritedConstructorsToClass(ClassDecl *decl) {
 
   // Check whether the user has defined a designated initializer for this class,
   // and whether all of its stored properties have initial values.
+  auto &ctx = decl->getASTContext();
   bool foundDesignatedInit = hasUserDefinedDesignatedInit(ctx.evaluator, decl);
   bool defaultInitable =
       areAllStoredPropertiesDefaultInitializable(ctx.evaluator, decl);
@@ -985,14 +975,19 @@ static void addImplicitInheritedConstructorsToClass(ClassDecl *decl) {
     // A designated or required initializer has not been overridden.
 
     bool alreadyDeclared = false;
-    for (const auto &ctorAndType : declaredInitializers) {
-      auto *ctor = ctorAndType.first;
-      auto type = ctorAndType.second;
-      auto parentType = getMemberTypeForComparison(ctx, superclassCtor, ctor);
-
-      if (isOverrideBasedOnType(ctor, type, superclassCtor, parentType)) {
-        alreadyDeclared = true;
-        break;
+    for (auto *member : decl->getMembers()) {
+      if (auto ctor = dyn_cast<ConstructorDecl>(member)) {
+        // Skip any invalid constructors.
+        if (ctor->isInvalid())
+          continue;
+
+        auto type = swift::getMemberTypeForComparison(ctor, nullptr);
+        auto parentType = swift::getMemberTypeForComparison(superclassCtor, ctor);
+
+        if (isOverrideBasedOnType(ctor, type, superclassCtor, parentType)) {
+          alreadyDeclared = true;
+          break;
+        }
       }
     }
 

lib/Sema/TypeCheckDeclOverride.cpp

@@ -65,16 +65,14 @@ static Type dropResultOptionality(Type type, unsigned uncurryLevel) {
   return FunctionType::get(parameters, resultType, fnType->getExtInfo());
 }
 
-Type swift::getMemberTypeForComparison(ASTContext &ctx, ValueDecl *member,
-                                       ValueDecl *derivedDecl) {
+Type swift::getMemberTypeForComparison(const ValueDecl *member,
+                                       const ValueDecl *derivedDecl) {
   auto *method = dyn_cast<AbstractFunctionDecl>(member);
-  ConstructorDecl *ctor = nullptr;
-  if (method)
-    ctor = dyn_cast<ConstructorDecl>(method);
+  auto *ctor = dyn_cast_or_null<ConstructorDecl>(method);
 
   auto abstractStorage = dyn_cast<AbstractStorageDecl>(member);
   assert((method || abstractStorage) && "Not a method or abstractStorage?");
-  SubscriptDecl *subscript = dyn_cast_or_null<SubscriptDecl>(abstractStorage);
+  auto *subscript = dyn_cast_or_null<SubscriptDecl>(abstractStorage);
 
   auto memberType = member->getInterfaceType();
   if (memberType->is<ErrorType>())
@@ -112,8 +110,9 @@ Type swift::getMemberTypeForComparison(ASTContext &ctx, ValueDecl *member,
   return memberType;
 }
 
-static bool areAccessorsOverrideCompatible(AbstractStorageDecl *storage,
-                                           AbstractStorageDecl *parentStorage) {
+static bool
+areAccessorsOverrideCompatible(const AbstractStorageDecl *storage,
+                               const AbstractStorageDecl *parentStorage) {
   // It's okay for the storage to disagree about whether to use a getter or
   // a read accessor; we'll patch up any differences when setting overrides
   // for the accessors.  We don't want to diagnose anything involving
@@ -143,8 +142,9 @@ static bool areAccessorsOverrideCompatible(AbstractStorageDecl *storage,
   return true;
 }
 
-bool swift::isOverrideBasedOnType(ValueDecl *decl, Type declTy,
-                                  ValueDecl *parentDecl, Type parentDeclTy) {
+bool swift::isOverrideBasedOnType(const ValueDecl *decl, Type declTy,
+                                  const ValueDecl *parentDecl,
+                                  Type parentDeclTy) {
   auto genericSig =
       decl->getInnermostDeclContext()->getGenericSignatureOfContext();
 
@@ -667,7 +667,7 @@ namespace {
     /// Retrieve the type of the declaration, to be used in comparisons.
     Type getDeclComparisonType() {
       if (!cachedDeclType) {
-        cachedDeclType = getMemberTypeForComparison(ctx, decl);
+        cachedDeclType = getMemberTypeForComparison(decl);
       }
 
       return cachedDeclType;
@@ -756,7 +756,7 @@ SmallVector<OverrideMatch, 2> OverrideMatcher::match(
     (void)parentStorage;
 
     // Check whether the types are identical.
-    auto parentDeclTy = getMemberTypeForComparison(ctx, parentDecl, decl);
+    auto parentDeclTy = getMemberTypeForComparison(parentDecl, decl);
     if (parentDeclTy->hasError())
       continue;
 
@@ -931,7 +931,7 @@ static void checkOverrideAccessControl(ValueDecl *baseDecl, ValueDecl *decl,
 bool OverrideMatcher::checkOverride(ValueDecl *baseDecl,
                                     OverrideCheckingAttempt attempt) {
   auto &diags = ctx.Diags;
-  auto baseTy = getMemberTypeForComparison(ctx, baseDecl, decl);
+  auto baseTy = getMemberTypeForComparison(baseDecl, decl);
   bool emittedMatchError = false;
 
   // If the name of our match differs from the name we were looking for,

lib/Sema/TypeChecker.h

@@ -2024,13 +2024,13 @@ OverrideRequiresKeyword overrideRequiresKeyword(ValueDecl *overridden);
 
 /// Compute the type of a member that will be used for comparison when
 /// performing override checking.
-Type getMemberTypeForComparison(ASTContext &ctx, ValueDecl *member,
-                                ValueDecl *derivedDecl = nullptr);
+Type getMemberTypeForComparison(const ValueDecl *member,
+                                const ValueDecl *derivedDecl = nullptr);
 
 /// Determine whether the given declaration is an override by comparing type
 /// information.
-bool isOverrideBasedOnType(ValueDecl *decl, Type declTy,
-                           ValueDecl *parentDecl, Type parentDeclTy);
+bool isOverrideBasedOnType(const ValueDecl *decl, Type declTy,
+                           const ValueDecl *parentDecl, Type parentDeclTy);
 
 /// Determine whether the given declaration is an operator defined in a
 /// protocol. If \p type is not null, check specifically whether \p decl

validation-test/compiler_crashers_2_fixed/0124-sr5825.swift

@@ -1,4 +1,4 @@
-// RUN: %target-swift-frontend %s -emit-ir
+// RUN: %target-typecheck-verify-swift
 
 struct DefaultAssociatedType {
 }
@@ -10,7 +10,7 @@ protocol Protocol {
 
 final class Conformance: Protocol {
     private let object: AssociatedType
-    init(object: AssociatedType) {
+    init(object: AssociatedType) { // expected-error {{reference to invalid associated type 'AssociatedType' of type 'Conformance'}}
         self.object = object
     }
 }