Warn when using '@_implementationOnly' inconsistently in a module (swiftlang#24800)

I thought it would be useful to allow some uses of a module to be
'@_implementationOnly' and others to not be in case someone wanted to
change from one to the other gradually, but it turns out that if
you're trying to /make/ an import implementation-only, you want to
know everywhere you used it.

rdar://problem/50748157

Author: jrose-apple

include/swift/AST/DiagnosticsSema.def

@@ -2447,6 +2447,12 @@ ERROR(assoc_conformance_from_implementation_only_module,none,
       "%4); %2 has been imported as implementation-only",
       (Type, DeclName, Identifier, Type, Type))
 
+WARNING(warn_implementation_only_conflict,none,
+        "%0 inconsistently imported as implementation-only",
+        (Identifier))
+NOTE(implementation_only_conflict_here,none,
+     "imported as implementation-only here", ())
+
 // Derived conformances
 ERROR(cannot_synthesize_init_in_extension_of_nonfinal,none,
       "implementation of %0 for non-final class cannot be automatically "

include/swift/Subsystems.h

@@ -216,6 +216,14 @@ 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);
+
   /// Incrementally type-check only added external definitions.
   void typeCheckExternalDefinitions(SourceFile &SF);
 

lib/Frontend/Frontend.cpp

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

lib/Sema/TypeChecker.cpp

@@ -573,6 +573,81 @@ void swift::performWholeModuleTypeChecking(SourceFile &SF) {
 #endif
 }
 
+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->Decls) {
+      auto *nextImport = dyn_cast<ImportDecl>(topLevelDecl);
+      if (!nextImport)
+        continue;
+
+      ModuleDecl *module = nextImport->getModule();
+      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) {

test/ParseableInterface/Inputs/imports-other.swift

@@ -2,5 +2,5 @@ import A
 import B.B3
 import D
 
-import NotSoSecret
-@_implementationOnly import NotSoSecret2
+import NotSoSecret // expected-warning {{'NotSoSecret' inconsistently imported as implementation-only}}
+@_implementationOnly import NotSoSecret2 // expected-note {{imported as implementation-only here}}

test/ParseableInterface/imports.swift

@@ -9,8 +9,8 @@ import func C.c // expected-warning {{scoped imports are not yet supported in mo
 import D
 @_implementationOnly import Secret_BAD
 
-@_implementationOnly import NotSoSecret
-import NotSoSecret2
+@_implementationOnly import NotSoSecret // expected-note {{imported as implementation-only here}}
+import NotSoSecret2 // expected-warning {{'NotSoSecret2' inconsistently imported as implementation-only}}
 
 // CHECK-NOT: import
 // CHECK: {{^}}import A{{$}}

test/decl/import/Inputs/inconsistent-implementation-only/2.swift

@@ -0,0 +1,8 @@
+import NotSoSecret // expected-warning {{'NotSoSecret' inconsistently imported as implementation-only}}
+@_implementationOnly import NotSoSecret2 // expected-note 2 {{imported as implementation-only here}}
+import NotSoSecret3 // expected-warning {{'NotSoSecret3' inconsistently imported as implementation-only}}
+
+@_implementationOnly import ActuallySecret // no-warning
+import ActuallyOkay // no-warning
+
+@_implementationOnly import Contradictory // expected-note {{imported as implementation-only here}}

test/decl/import/Inputs/inconsistent-implementation-only/3.swift

@@ -0,0 +1,6 @@
+@_implementationOnly import NotSoSecret
+import NotSoSecret2 // expected-warning {{'NotSoSecret2' inconsistently imported as implementation-only}}
+@_implementationOnly import NotSoSecret3 // expected-note 2 {{imported as implementation-only here}}
+
+@_implementationOnly import ActuallySecret // no-warning
+import ActuallyOkay // no-warning

test/decl/import/Inputs/inconsistent-implementation-only/module.modulemap

@@ -0,0 +1,8 @@
+module NotSoSecret {}
+module NotSoSecret2 {}
+module NotSoSecret3 {}
+
+module ActuallySecret {}
+module ActuallyOkay {}
+
+module Contradictory {}

test/decl/import/inconsistent-implementation-only.swift

@@ -0,0 +1,15 @@
+// Check that the diagnostics are produced regardless of what primary file we're using.
+// RUN: %target-swift-frontend -typecheck -primary-file %s %S/Inputs/inconsistent-implementation-only/2.swift %S/Inputs/inconsistent-implementation-only/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+// RUN: %target-swift-frontend -typecheck %s -primary-file %S/Inputs/inconsistent-implementation-only/2.swift %S/Inputs/inconsistent-implementation-only/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+// RUN: %target-swift-frontend -typecheck %s %S/Inputs/inconsistent-implementation-only/2.swift -primary-file %S/Inputs/inconsistent-implementation-only/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+// RUN: %target-swift-frontend -typecheck -primary-file %s -primary-file %S/Inputs/inconsistent-implementation-only/2.swift -primary-file %S/Inputs/inconsistent-implementation-only/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+// RUN: %target-swift-frontend -typecheck %s %S/Inputs/inconsistent-implementation-only/2.swift %S/Inputs/inconsistent-implementation-only/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+
+@_implementationOnly import NotSoSecret // expected-note {{imported as implementation-only here}}
+import NotSoSecret2 // expected-warning {{'NotSoSecret2' inconsistently imported as implementation-only}} {{1-1=@_implementationOnly }}
+import NotSoSecret3 // expected-warning {{'NotSoSecret3' inconsistently imported as implementation-only}} {{1-1=@_implementationOnly }}
+
+@_implementationOnly import ActuallySecret // no-warning
+import ActuallyOkay // no-warning
+
+@_exported import Contradictory // expected-warning {{'Contradictory' inconsistently imported as implementation-only}} {{none}}