Sema: Pull @_implementationOnly override checking out of ExportabilityChecker

Author: slavapestov

lib/Sema/TypeCheckAccess.cpp

@@ -1737,48 +1737,13 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     return true;
   }
 
-  void checkOverride(const ValueDecl *VD) {
-    const ValueDecl *overridden = VD->getOverriddenDecl();
-    if (!overridden)
-      return;
-
-    auto *SF = VD->getDeclContext()->getParentSourceFile();
-    assert(SF && "checking a non-source declaration?");
-
-    ModuleDecl *M = overridden->getModuleContext();
-    if (SF->isImportedImplementationOnly(M)) {
-      VD->diagnose(diag::implementation_only_override_import_without_attr,
-                   overridden->getDescriptiveKind())
-          .fixItInsert(VD->getAttributeInsertionLoc(false),
-                       "@_implementationOnly ");
-      overridden->diagnose(diag::overridden_here);
-      return;
-    }
-
-    if (overridden->getAttrs().hasAttribute<ImplementationOnlyAttr>()) {
-      VD->diagnose(diag::implementation_only_override_without_attr,
-                   overridden->getDescriptiveKind())
-          .fixItInsert(VD->getAttributeInsertionLoc(false),
-                       "@_implementationOnly ");
-      overridden->diagnose(diag::overridden_here);
-      return;
-    }
-
-    // FIXME: Check storage decls where the setter is in a separate module from
-    // the getter, which is a thing Objective-C can do. The ClangImporter
-    // doesn't make this easy, though, because it just gives the setter the same
-    // DeclContext as the property or subscript, which means we've lost the
-    // information about whether its module was implementation-only imported.
-  }
-
   void visit(Decl *D) {
     if (D->isInvalid() || D->isImplicit())
       return;
 
     if (auto *VD = dyn_cast<ValueDecl>(D)) {
       if (shouldSkipChecking(VD))
         return;
-      checkOverride(VD);
     }
 
     // Note: references to @_spi and @_implementationOnly declarations from
@@ -1828,8 +1793,6 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     if (shouldSkipChecking(theVar))
       return;
 
-    checkOverride(theVar);
-
     // Only check the type of individual variables if we didn't check an
     // enclosing TypedPattern.
     if (seenVars.count(theVar) || theVar->isInvalid())

lib/Sema/TypeCheckDecl.h

@@ -33,6 +33,7 @@ const ConstructorDecl *findNonImplicitRequiredInit(const ConstructorDecl *CD);
 
 // Implemented in TypeCheckDeclOverride.cpp
 bool checkOverrides(ValueDecl *decl);
+void checkImplementationOnlyOverride(const ValueDecl *VD);
 
 // Implemented in TypeCheckStorage.cpp
 void setBoundVarsTypeError(Pattern *pattern, ASTContext &ctx);

lib/Sema/TypeCheckDeclOverride.cpp

@@ -2156,3 +2156,53 @@ bool IsABICompatibleOverrideRequest::evaluate(Evaluator &evaluator,
   return derivedInterfaceTy->matches(overrideInterfaceTy,
                                      TypeMatchFlags::AllowABICompatible);
 }
+
+void swift::checkImplementationOnlyOverride(const ValueDecl *VD) {
+  if (VD->isImplicit())
+    return;
+
+  if (VD->getAttrs().hasAttribute<ImplementationOnlyAttr>())
+    return;
+
+  if (isa<AccessorDecl>(VD))
+    return;
+
+  // Is this part of the module's API or ABI?
+  AccessScope accessScope =
+      VD->getFormalAccessScope(nullptr,
+                               /*treatUsableFromInlineAsPublic*/true);
+  if (!accessScope.isPublic())
+    return;
+
+  const ValueDecl *overridden = VD->getOverriddenDecl();
+  if (!overridden)
+    return;
+
+  auto *SF = VD->getDeclContext()->getParentSourceFile();
+  assert(SF && "checking a non-source declaration?");
+
+  ModuleDecl *M = overridden->getModuleContext();
+  if (SF->isImportedImplementationOnly(M)) {
+    VD->diagnose(diag::implementation_only_override_import_without_attr,
+                 overridden->getDescriptiveKind())
+        .fixItInsert(VD->getAttributeInsertionLoc(false),
+                     "@_implementationOnly ");
+    overridden->diagnose(diag::overridden_here);
+    return;
+  }
+
+  if (overridden->getAttrs().hasAttribute<ImplementationOnlyAttr>()) {
+    VD->diagnose(diag::implementation_only_override_without_attr,
+                 overridden->getDescriptiveKind())
+        .fixItInsert(VD->getAttributeInsertionLoc(false),
+                     "@_implementationOnly ");
+    overridden->diagnose(diag::overridden_here);
+    return;
+  }
+
+  // FIXME: Check storage decls where the setter is in a separate module from
+  // the getter, which is a thing Objective-C can do. The ClangImporter
+  // doesn't make this easy, though, because it just gives the setter the same
+  // DeclContext as the property or subscript, which means we've lost the
+  // information about whether its module was implementation-only imported.
+}

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -1551,6 +1551,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
       }
     }
 
+    checkImplementationOnlyOverride(VD);
+
     if (VD->getDeclContext()->getSelfClassDecl()) {
       if (VD->getValueInterfaceType()->hasDynamicSelfType()) {
         if (VD->hasStorage())
@@ -1772,6 +1774,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
       }
     }
 
+    checkImplementationOnlyOverride(SD);
+
     // Compute these requests in case they emit diagnostics.
     (void) SD->isGetterMutating();
     (void) SD->isSetterMutating();
@@ -2364,6 +2368,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
       }
     }
 
+    checkImplementationOnlyOverride(FD);
+
     if (requiresDefinition(FD) && !FD->hasBody()) {
       // Complain if we should have a body.
       FD->diagnose(diag::func_decl_without_brace);
@@ -2672,6 +2678,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
       }
     }
 
+    checkImplementationOnlyOverride(CD);
+
     // If this initializer overrides a 'required' initializer, it must itself
     // be marked 'required'.
     if (!CD->getAttrs().hasAttribute<RequiredAttr>()) {