Lift implementation-only import checking into a request

This diagnostic logic is currently called from
`finishTypeChecking`, however it doesn't need to
be delayed until after all the source files have
been type-checked, only after they have all had
their imports resolved.

It can therefore be lifted into a request that
operates on a ModuleDecl, and be called from
TypeCheckSourceFileRequest. Being a request will
ensure that the pass is only run once across the
module.

Eventually we'll probably want to re-do import
resolution so that it operates on an entire
module. At that point, we'll want to look at
integrating this diagnostic logic into it.

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,18 +674,20 @@ void UnboundImport::diagnoseInvalidAttr(DeclAttrKind attrKind,
   attr->setInvalid();
 }
 
-void swift::checkInconsistentImplementationOnlyImports(ModuleDecl *MainModule) {
+evaluator::SideEffect
+CheckInconsistentImplementationOnlyImportsRequest::evaluate(
+    Evaluator &evaluator, ModuleDecl *mod) const {
   bool hasAnyImplementationOnlyImports =
-      llvm::any_of(MainModule->getFiles(), [](const FileUnit *F) -> bool {
-    auto *SF = dyn_cast<SourceFile>(F);
-    return SF && SF->hasImplementationOnlyImports();
-  });
+      llvm::any_of(mod->getFiles(), [](const FileUnit *F) -> bool {
+        auto *SF = dyn_cast<SourceFile>(F);
+        return SF && SF->hasImplementationOnlyImports();
+      });
   if (!hasAnyImplementationOnlyImports)
-    return;
+    return {};
 
-  auto diagnose = [MainModule](const ImportDecl *normalImport,
-                               const ImportDecl *implementationOnlyImport) {
-    auto &diags = MainModule->getDiags();
+  auto diagnose = [mod](const ImportDecl *normalImport,
+                        const ImportDecl *implementationOnlyImport) {
+    auto &diags = mod->getDiags();
     {
       InFlightDiagnostic warning =
           diags.diagnose(normalImport, diag::warn_implementation_only_conflict,
@@ -706,7 +708,7 @@ void swift::checkInconsistentImplementationOnlyImports(ModuleDecl *MainModule) {
   llvm::DenseMap<ModuleDecl *, std::vector<const ImportDecl *>> normalImports;
   llvm::DenseMap<ModuleDecl *, const ImportDecl *> implementationOnlyImports;
 
-  for (const FileUnit *file : MainModule->getFiles()) {
+  for (const FileUnit *file : mod->getFiles()) {
     auto *SF = dyn_cast<SourceFile>(file);
     if (!SF)
       continue;
@@ -750,6 +752,7 @@ void swift::checkInconsistentImplementationOnlyImports(ModuleDecl *MainModule) {
       normalImports[module].push_back(nextImport);
     }
   }
+  return {};
 }
 
 //===----------------------------------------------------------------------===//

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);