Merge pull request #31872 from hamishknight/implementation-details

Author: hamishknight

include/swift/AST/TypeCheckRequests.h

@@ -2430,6 +2430,32 @@ class ResolveTypeRequest
 void simple_display(llvm::raw_ostream &out, const TypeResolution *resolution);
 SourceLoc extractNearestSourceLoc(const TypeRepr *repr);
 
+/// Checks to see if any of the imports in a module use `@_implementationOnly`
+/// in one file and not in another.
+///
+/// Like redeclaration checking, but for imports.
+///
+/// This is a request purely to ensure that we don't need to perform the same
+/// checking for each file we resolve imports for.
+/// FIXME: Once import resolution operates at module-level, this checking can
+/// integrated into it.
+class CheckInconsistentImplementationOnlyImportsRequest
+    : public SimpleRequest<CheckInconsistentImplementationOnlyImportsRequest,
+                           evaluator::SideEffect(ModuleDecl *),
+                           RequestFlags::Cached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  evaluator::SideEffect evaluate(Evaluator &evaluator, ModuleDecl *mod) const;
+
+public:
+  // Cached.
+  bool isCached() const { return true; }
+};
+
 // Allow AnyValue to compare two Type values, even though Type doesn't
 // support ==.
 template<>

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -29,6 +29,8 @@ SWIFT_REQUEST(TypeChecker, AttachedPropertyWrappersRequest,
               NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CallerSideDefaultArgExprRequest,
               Expr *(DefaultArgumentExpr *), SeparatelyCached, NoLocationInfo)
+SWIFT_REQUEST(TypeChecker, CheckInconsistentImplementationOnlyImportsRequest,
+              evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckRedeclarationRequest,
               evaluator::SideEffect(ValueDecl *),
               Uncached, NoLocationInfo)

include/swift/Subsystems.h

@@ -152,14 +152,6 @@ namespace swift {
   /// emitted.
   void performWholeModuleTypeChecking(SourceFile &SF);
 
-  /// Checks to see if any of the imports in \p M use `@_implementationOnly` in
-  /// one file and not in another.
-  ///
-  /// Like redeclaration checking, but for imports. This isn't part of
-  /// swift::performWholeModuleTypeChecking because it's linear in the number
-  /// of declarations in the module.
-  void checkInconsistentImplementationOnlyImports(ModuleDecl *M);
-
   /// Recursively validate the specified type.
   ///
   /// This is used when dealing with partial source files (e.g. SIL parsing,

lib/Frontend/Frontend.cpp

@@ -897,8 +897,6 @@ void CompilerInstance::finishTypeChecking() {
       performWholeModuleTypeChecking(SF);
     });
   }
-
-  checkInconsistentImplementationOnlyImports(MainModule);
 }
 
 SourceFile *CompilerInstance::createSourceFileForMainModule(

lib/Sema/ImportResolution.cpp

@@ -674,6 +674,87 @@ void UnboundImport::diagnoseInvalidAttr(DeclAttrKind attrKind,
   attr->setInvalid();
 }
 
+evaluator::SideEffect
+CheckInconsistentImplementationOnlyImportsRequest::evaluate(
+    Evaluator &evaluator, ModuleDecl *mod) const {
+  bool hasAnyImplementationOnlyImports =
+      llvm::any_of(mod->getFiles(), [](const FileUnit *F) -> bool {
+        auto *SF = dyn_cast<SourceFile>(F);
+        return SF && SF->hasImplementationOnlyImports();
+      });
+  if (!hasAnyImplementationOnlyImports)
+    return {};
+
+  auto diagnose = [mod](const ImportDecl *normalImport,
+                        const ImportDecl *implementationOnlyImport) {
+    auto &diags = mod->getDiags();
+    {
+      InFlightDiagnostic warning =
+          diags.diagnose(normalImport, diag::warn_implementation_only_conflict,
+                         normalImport->getModule()->getName());
+      if (normalImport->getAttrs().isEmpty()) {
+        // Only try to add a fix-it if there's no other annotations on the
+        // import to avoid creating things like
+        // `@_implementationOnly @_exported import Foo`. The developer can
+        // resolve those manually.
+        warning.fixItInsert(normalImport->getStartLoc(),
+                            "@_implementationOnly ");
+      }
+    }
+    diags.diagnose(implementationOnlyImport,
+                   diag::implementation_only_conflict_here);
+  };
+
+  llvm::DenseMap<ModuleDecl *, std::vector<const ImportDecl *>> normalImports;
+  llvm::DenseMap<ModuleDecl *, const ImportDecl *> implementationOnlyImports;
+
+  for (const FileUnit *file : mod->getFiles()) {
+    auto *SF = dyn_cast<SourceFile>(file);
+    if (!SF)
+      continue;
+
+    for (auto *topLevelDecl : SF->getTopLevelDecls()) {
+      auto *nextImport = dyn_cast<ImportDecl>(topLevelDecl);
+      if (!nextImport)
+        continue;
+
+      ModuleDecl *module = nextImport->getModule();
+      if (!module)
+        continue;
+
+      if (nextImport->getAttrs().hasAttribute<ImplementationOnlyAttr>()) {
+        // We saw an implementation-only import.
+        bool isNew =
+            implementationOnlyImports.insert({module, nextImport}).second;
+        if (!isNew)
+          continue;
+
+        auto seenNormalImportPosition = normalImports.find(module);
+        if (seenNormalImportPosition != normalImports.end()) {
+          for (auto *seenNormalImport : seenNormalImportPosition->getSecond())
+            diagnose(seenNormalImport, nextImport);
+
+          // We're done with these; keep the map small if possible.
+          normalImports.erase(seenNormalImportPosition);
+        }
+        continue;
+      }
+
+      // We saw a non-implementation-only import. Is that in conflict with what
+      // we've seen?
+      if (auto *seenImplementationOnlyImport =
+            implementationOnlyImports.lookup(module)) {
+        diagnose(nextImport, seenImplementationOnlyImport);
+        continue;
+      }
+
+      // Otherwise, record it for later.
+      normalImports[module].push_back(nextImport);
+    }
+  }
+  return {};
+}
+
 //===----------------------------------------------------------------------===//
 // MARK: Scoped imports
 //===----------------------------------------------------------------------===//

lib/Sema/TypeChecker.cpp

@@ -351,6 +351,12 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
     typeCheckDelayedFunctions(*SF);
   }
 
+  // Check to see if there's any inconsistent @_implementationOnly imports.
+  evaluateOrDefault(
+      Ctx.evaluator,
+      CheckInconsistentImplementationOnlyImportsRequest{SF->getParentModule()},
+      {});
+
   // Checking that benefits from having the whole module available.
   if (!Ctx.TypeCheckerOpts.DelayWholeModuleChecking) {
     performWholeModuleTypeChecking(*SF);
@@ -406,84 +412,6 @@ bool swift::isAdditiveArithmeticConformanceDerivationEnabled(SourceFile &SF) {
   return isDifferentiableProgrammingEnabled(SF);
 }
 
-void swift::checkInconsistentImplementationOnlyImports(ModuleDecl *MainModule) {
-  bool hasAnyImplementationOnlyImports =
-      llvm::any_of(MainModule->getFiles(), [](const FileUnit *F) -> bool {
-    auto *SF = dyn_cast<SourceFile>(F);
-    return SF && SF->hasImplementationOnlyImports();
-  });
-  if (!hasAnyImplementationOnlyImports)
-    return;
-
-  auto diagnose = [MainModule](const ImportDecl *normalImport,
-                               const ImportDecl *implementationOnlyImport) {
-    auto &diags = MainModule->getDiags();
-    {
-      InFlightDiagnostic warning =
-          diags.diagnose(normalImport, diag::warn_implementation_only_conflict,
-                         normalImport->getModule()->getName());
-      if (normalImport->getAttrs().isEmpty()) {
-        // Only try to add a fix-it if there's no other annotations on the
-        // import to avoid creating things like
-        // `@_implementationOnly @_exported import Foo`. The developer can
-        // resolve those manually.
-        warning.fixItInsert(normalImport->getStartLoc(),
-                            "@_implementationOnly ");
-      }
-    }
-    diags.diagnose(implementationOnlyImport,
-                   diag::implementation_only_conflict_here);
-  };
-
-  llvm::DenseMap<ModuleDecl *, std::vector<const ImportDecl *>> normalImports;
-  llvm::DenseMap<ModuleDecl *, const ImportDecl *> implementationOnlyImports;
-
-  for (const FileUnit *file : MainModule->getFiles()) {
-    auto *SF = dyn_cast<SourceFile>(file);
-    if (!SF)
-      continue;
-
-    for (auto *topLevelDecl : SF->getTopLevelDecls()) {
-      auto *nextImport = dyn_cast<ImportDecl>(topLevelDecl);
-      if (!nextImport)
-        continue;
-
-      ModuleDecl *module = nextImport->getModule();
-      if (!module)
-        continue;
-
-      if (nextImport->getAttrs().hasAttribute<ImplementationOnlyAttr>()) {
-        // We saw an implementation-only import.
-        bool isNew =
-            implementationOnlyImports.insert({module, nextImport}).second;
-        if (!isNew)
-          continue;
-
-        auto seenNormalImportPosition = normalImports.find(module);
-        if (seenNormalImportPosition != normalImports.end()) {
-          for (auto *seenNormalImport : seenNormalImportPosition->getSecond())
-            diagnose(seenNormalImport, nextImport);
-
-          // We're done with these; keep the map small if possible.
-          normalImports.erase(seenNormalImportPosition);
-        }
-        continue;
-      }
-
-      // We saw a non-implementation-only import. Is that in conflict with what
-      // we've seen?
-      if (auto *seenImplementationOnlyImport =
-            implementationOnlyImports.lookup(module)) {
-        diagnose(nextImport, seenImplementationOnlyImport);
-        continue;
-      }
-
-      // Otherwise, record it for later.
-      normalImports[module].push_back(nextImport);
-    }
-  }
-}
-
 bool swift::performTypeLocChecking(ASTContext &Ctx, TypeLoc &T,
                                    DeclContext *DC,
                                    bool ProduceDiagnostics) {