Merge pull request #26031 from slavapestov/storage-validation-cleanup

Storage validation cleanup

Author: slavapestov

lib/AST/ASTVerifier.cpp

@@ -2471,44 +2471,48 @@ class Verifier : public ASTWalker {
             Out << "property getter has parameters\n";
             abort();
           }
-          Type getterResultType = getter->getResultInterfaceType();
-          getterResultType =
-              var->getDeclContext()->mapTypeIntoContext(getterResultType);
-          if (!getterResultType->isEqual(typeForAccessors)) {
-            Out << "property and getter have mismatched types: '";
-            typeForAccessors.print(Out);
-            Out << "' vs. '";
-            getterResultType.print(Out);
-            Out << "'\n";
-            abort();
+          if (getter->hasInterfaceType()) {
+            Type getterResultType = getter->getResultInterfaceType();
+            getterResultType =
+                var->getDeclContext()->mapTypeIntoContext(getterResultType);
+            if (!getterResultType->isEqual(typeForAccessors)) {
+              Out << "property and getter have mismatched types: '";
+              typeForAccessors.print(Out);
+              Out << "' vs. '";
+              getterResultType.print(Out);
+              Out << "'\n";
+              abort();
+            }
           }
         }
       }
 
       if (const FuncDecl *setter = var->getSetter()) {
-        if (!setter->getResultInterfaceType()->isVoid()) {
-          Out << "property setter has non-Void result type\n";
-          abort();
-        }
-        if (setter->getParameters()->size() == 0) {
-          Out << "property setter has no parameters\n";
-          abort();
-        }
-        if (setter->getParameters()->size() != 1) {
-          Out << "property setter has 2+ parameters\n";
-          abort();
-        }
-        const ParamDecl *param = setter->getParameters()->get(0);
-        Type paramType = param->getInterfaceType();
-        if (!var->getDeclContext()->contextHasLazyGenericEnvironment()) {
-          paramType = var->getDeclContext()->mapTypeIntoContext(paramType);
-          if (!paramType->isEqual(typeForAccessors)) {
-            Out << "property and setter param have mismatched types:\n";
-            typeForAccessors.dump(Out, 2);
-            Out << "vs.\n";
-            paramType.dump(Out, 2);
+        if (setter->hasInterfaceType()) {
+          if (!setter->getResultInterfaceType()->isVoid()) {
+            Out << "property setter has non-Void result type\n";
+            abort();
+          }
+          if (setter->getParameters()->size() == 0) {
+            Out << "property setter has no parameters\n";
             abort();
           }
+          if (setter->getParameters()->size() != 1) {
+            Out << "property setter has 2+ parameters\n";
+            abort();
+          }
+          const ParamDecl *param = setter->getParameters()->get(0);
+          Type paramType = param->getInterfaceType();
+          if (!var->getDeclContext()->contextHasLazyGenericEnvironment()) {
+            paramType = var->getDeclContext()->mapTypeIntoContext(paramType);
+            if (!paramType->isEqual(typeForAccessors)) {
+              Out << "property and setter param have mismatched types:\n";
+              typeForAccessors.dump(Out, 2);
+              Out << "vs.\n";
+              paramType.dump(Out, 2);
+              abort();
+            }
+          }
         }
       }
 
@@ -3010,6 +3014,16 @@ class Verifier : public ASTWalker {
     void verifyChecked(AbstractFunctionDecl *AFD) {
       PrettyStackTraceDecl debugStack("verifying AbstractFunctionDecl", AFD);
 
+      if (!AFD->hasValidSignature()) {
+        if (isa<AccessorDecl>(AFD) && AFD->isImplicit())
+          return;
+
+        Out << "All functions except implicit accessors should be "
+               "validated by now\n";
+        AFD->dump(Out);
+        abort();
+      }
+
       // If this function is generic or is within a generic context, it should
       // have an interface type.
       if (AFD->isGenericContext() !=

lib/AST/Decl.cpp

@@ -2701,7 +2701,8 @@ bool ValueDecl::canInferObjCFromRequirement(ValueDecl *requirement) {
   // If there is already an @objc attribute with an explicit name, we
   // can't infer a name (it's already there).
   if (auto objcAttr = getAttrs().getAttribute<ObjCAttr>()) {
-    if (!objcAttr->isNameImplicit()) return false;
+    if (objcAttr->hasName() && !objcAttr->isNameImplicit())
+      return false;
   }
 
   // If the nominal type doesn't conform to the protocol at all, we

lib/SILGen/SILGenType.cpp

@@ -24,6 +24,7 @@
 #include "swift/AST/ASTMangler.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/AST/ProtocolConformance.h"
+#include "swift/AST/PrettyStackTrace.h"
 #include "swift/AST/SubstitutionMap.h"
 #include "swift/AST/TypeMemberVisitor.h"
 #include "swift/SIL/FormalLinkage.h"
@@ -410,6 +411,10 @@ class SILGenConformance : public SILGenWitnessTable<SILGenConformance> {
     if (!Conformance)
       return nullptr;
 
+    PrettyStackTraceConformance trace(SGM.getASTContext(),
+                                      "generating SIL witness table",
+                                      Conformance);
+
     auto *proto = Conformance->getProtocol();
     visitProtocolDecl(proto);
 
@@ -773,6 +778,10 @@ class SILGenSelfConformanceWitnessTable
   }
 
   void emit() {
+    PrettyStackTraceConformance trace(SGM.getASTContext(),
+                                      "generating SIL witness table",
+                                      conformance);
+
     // Add entries for all the requirements.
     visitProtocolDecl(conformance->getProtocol());
 

lib/Sema/CodeSynthesis.cpp

@@ -1404,6 +1404,10 @@ void TypeChecker::synthesizeWitnessAccessorsForStorage(
                                              AbstractStorageDecl *storage) {
   bool addedAccessor = false;
 
+  // Make sure the protocol requirement itself has the right accessors.
+  // FIXME: This should be a request kicked off by SILGen.
+  DeclsToFinalize.insert(requirement);
+
   requirement->visitExpectedOpaqueAccessors([&](AccessorKind kind) {
     // If the accessor already exists, we have nothing to do.
     if (storage->getAccessor(kind))
@@ -1708,6 +1712,10 @@ LazyStoragePropertyRequest::evaluate(Evaluator &evaluator,
   NameBuf += "$__lazy_storage_$_";
   NameBuf += VD->getName().str();
   auto StorageName = Context.getIdentifier(NameBuf);
+
+  if (!VD->hasInterfaceType())
+    Context.getLazyResolver()->resolveDeclSignature(VD);
+
   auto StorageTy = OptionalType::get(VD->getType());
   auto StorageInterfaceTy = OptionalType::get(VD->getInterfaceType());
 

lib/Sema/TypeCheckDecl.cpp

@@ -2153,35 +2153,6 @@ OpaqueReadOwnershipRequest::evaluate(Evaluator &evaluator,
           : OpaqueReadOwnership::Owned);
 }
 
-static void validateAbstractStorageDecl(TypeChecker &TC,
-                                        AbstractStorageDecl *storage) {
-  if (storage->getOpaqueResultTypeDecl()) {
-    if (auto sf = storage->getInnermostDeclContext()->getParentSourceFile()) {
-      sf->markDeclWithOpaqueResultTypeAsValidated(storage);
-    }
-  }
-
-  // Everything else about the accessors can wait until finalization.
-  // This will validate all the accessors.
-  TC.DeclsToFinalize.insert(storage);
-}
-
-static void finalizeAbstractStorageDecl(TypeChecker &TC,
-                                        AbstractStorageDecl *storage) {
-  TC.validateDecl(storage);
-
-  // Add any mandatory accessors now.
-  maybeAddAccessorsToStorage(storage);
-
-  for (auto accessor : storage->getAllAccessors()) {
-    // Are there accessors we can safely ignore here, like maybe observers?
-    TC.validateDecl(accessor);
-
-    // Finalize the accessors as well.
-    TC.DeclsToFinalize.insert(accessor);
-  }
-}
-
 /// Check the requirements in the where clause of the given \c source
 /// to ensure that they don't introduce additional 'Self' requirements.
 static void checkProtocolSelfRequirements(ProtocolDecl *proto,
@@ -2255,6 +2226,18 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     if (auto VD = dyn_cast<ValueDecl>(decl)) {
       checkRedeclaration(TC, VD);
 
+      // Force some requests, which can produce diagnostics.
+
+      // Compute access level.
+      (void) VD->getFormalAccess();
+
+      // Compute overrides.
+      (void) VD->getOverriddenDecls();
+
+      // Check whether the member is @objc or dynamic.
+      (void) VD->isObjC();
+      (void) VD->isDynamic();
+
       // Make sure we finalize this declaration.
       TC.DeclsToFinalize.insert(VD);
 
@@ -2974,6 +2957,24 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     TC.addImplicitConstructors(CD);
     CD->addImplicitDestructor();
 
+    // Compute @objc for each superclass member, to catch selector
+    // conflicts resulting from unintended overrides.
+    //
+    // FIXME: This should be a request so we can measure how much work
+    // we're doing here.
+    CD->walkSuperclasses(
+      [&](ClassDecl *superclass) {
+        for (auto *member : superclass->getMembers()) {
+          if (auto *vd = dyn_cast<ValueDecl>(member)) {
+            if (vd->isPotentiallyOverridable()) {
+              (void) vd->isObjC();
+            }
+          }
+        }
+
+        return TypeWalker::Action::Continue;
+      });
+
     if (auto superclassTy = CD->getSuperclass()) {
       ClassDecl *Super = superclassTy->getClassOrBoundGenericClass();
 
@@ -3961,8 +3962,11 @@ void TypeChecker::validateDecl(ValueDecl *D) {
     checkDeclAttributesEarly(VD);
     validateAttributes(*this, VD);
 
-    // Perform accessor-related validation.
-    validateAbstractStorageDecl(*this, VD);
+    if (VD->getOpaqueResultTypeDecl()) {
+      if (auto SF = VD->getInnermostDeclContext()->getParentSourceFile()) {
+        SF->markDeclWithOpaqueResultTypeAsValidated(VD);
+      }
+    }
 
     break;
   }
@@ -4240,7 +4244,11 @@ void TypeChecker::validateDecl(ValueDecl *D) {
     }
 
     // Perform accessor-related validation.
-    validateAbstractStorageDecl(*this, SD);
+    if (SD->getOpaqueResultTypeDecl()) {
+      if (auto SF = SD->getInnermostDeclContext()->getParentSourceFile()) {
+        SF->markDeclWithOpaqueResultTypeAsValidated(SD);
+      }
+    }
 
     break;
   }
@@ -4423,29 +4431,9 @@ void TypeChecker::requestMemberLayout(ValueDecl *member) {
   if (auto *protocolDecl = dyn_cast<ProtocolDecl>(dc))
     requestNominalLayout(protocolDecl);
 
-  if (auto ext = dyn_cast<ExtensionDecl>(dc)) {
-    if (ext->getSelfClassDecl()) {
-      // Finalize members of class extensions, to ensure we compute their
-      // @objc and dynamic state.
-      DeclsToFinalize.insert(member);
-    }
-  }
-
   // If this represents (abstract) storage, form the appropriate accessors.
-  if (auto storage = dyn_cast<AbstractStorageDecl>(member)) {
-    validateAbstractStorageDecl(*this, storage);
-
-    // Request layout of the accessors for an @objc declaration.
-    // We can't delay validation of getters and setters on @objc properties,
-    // because if they never get validated at all then conformance checkers
-    // will complain about selector mismatches.
-    if (storage->isObjC()) {
-      maybeAddAccessorsToStorage(storage);
-      for (auto accessor : storage->getAllAccessors()) {
-        requestMemberLayout(accessor);
-      }
-    }
-  }
+  if (auto storage = dyn_cast<AbstractStorageDecl>(member))
+    DeclsToFinalize.insert(storage);
 }
 
 void TypeChecker::requestNominalLayout(NominalTypeDecl *nominalDecl) {
@@ -4485,20 +4473,19 @@ static void finalizeType(TypeChecker &TC, NominalTypeDecl *nominal) {
 
     TC.DeclsToFinalize.insert(VD);
 
-    // The only thing left to do is synthesize storage for lazy variables.
+    // The only thing left to do is synthesize storage for lazy variables
+    // and property wrappers.
     auto *prop = dyn_cast<VarDecl>(D);
     if (!prop)
       continue;
 
     if (prop->getAttrs().hasAttribute<LazyAttr>() && !prop->isStatic() &&
         (!prop->getGetter() || !prop->getGetter()->hasBody())) {
-      finalizeAbstractStorageDecl(TC, prop);
       (void) prop->getLazyStorageProperty();
     }
 
     // Ensure that we create the backing variable for a wrapped property.
     if (prop->hasAttachedPropertyWrapper()) {
-      finalizeAbstractStorageDecl(TC, prop);
       (void) prop->getPropertyWrapperBackingProperty();
     }
   }
@@ -4530,12 +4517,9 @@ static void finalizeType(TypeChecker &TC, NominalTypeDecl *nominal) {
     forceConformance(TC.Context.getProtocol(KnownProtocolKind::Hashable));
   }
 
-  // validateDeclForNameLookup will not trigger an immediate full
-  // validation of protocols, but clients will assume that things
-  // like the requirement signature have been set.
   if (auto PD = dyn_cast<ProtocolDecl>(nominal)) {
-    (void)PD->getInheritedProtocols();
-    TC.validateDecl(PD);
+    for (auto *inherited : PD->getInheritedProtocols())
+      TC.requestNominalLayout(inherited);
   }
 }
 
@@ -4548,18 +4532,8 @@ void TypeChecker::finalizeDecl(ValueDecl *decl) {
   if (auto nominal = dyn_cast<NominalTypeDecl>(decl)) {
     finalizeType(*this, nominal);
   } else if (auto storage = dyn_cast<AbstractStorageDecl>(decl)) {
-    finalizeAbstractStorageDecl(*this, storage);
+    maybeAddAccessorsToStorage(storage);
   }
-
-  // Compute access level.
-  (void)decl->getFormalAccess();
-
-  // Compute overrides.
-  (void)decl->getOverriddenDecls();
-
-  // Check whether the member is @objc or dynamic.
-  (void)decl->isObjC();
-  (void)decl->isDynamic();
 }
 
 /// Determine whether this is a "pass-through" typealias, which has the

test/decl/protocol/conforms/near_miss_objc.swift

@@ -10,7 +10,7 @@
 class C1a : P1 {
   @objc func doSomething(a: Int, c: Double) { }
   // expected-warning@-1{{instance method 'doSomething(a:c:)' nearly matches optional requirement 'doSomething(a:b:)' of protocol 'P1'}}
-  // expected-note@-2{{rename to 'doSomething(a:b:)' to satisfy this requirement}}{{34-34=b }}{{8-8=(doSomethingWithA:b:)}} {{34-34=b }}
+  // expected-note@-2{{rename to 'doSomething(a:b:)' to satisfy this requirement}}{{34-34=b }}
   // expected-note@-3{{move 'doSomething(a:c:)' to an extension to silence this warning}}
   // expected-note@-4{{make 'doSomething(a:c:)' private to silence this warning}}{{9-9=private }}
 }