Sema: report inconsistent access levels on imports of one module from one file

Warn on imports of the same target module from the same source file when
they have different access-levels. This situation can lead to unexpected
behavior as the compiler will only take into account the most permissive
import.

In the following example, the module Foundation in functionally imported
as public. The `internal` modifier will be ignored. The new diagnostic
reports it as such.

```
public import Foundation
internal import Foundation // warning: module 'Foundation' is imported
as 'public' from the same file; this 'internal' access level will be
ignored
```

This could be an easy mistake to do when using scoped imports where this
diagnostic will also help.

rdar://117855481

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -3635,6 +3635,13 @@ NOTE(inconsistent_implicit_access_level_on_import_here,none,
 FIXIT(inconsistent_implicit_access_level_on_import_fixit,
      "%select{private|fileprivate|internal|package|public|%error}0 ",
      (AccessLevel))
+WARNING(inconsistent_import_access_levels,none,
+        "module %0 is imported as "
+        "'%select{private|fileprivate|internal|package|public|%error}1' "
+        "from the same file; "
+        "this '%select{private|fileprivate|internal|package|public|%error}2' "
+        "access level will be ignored",
+      (const ModuleDecl *, AccessLevel, AccessLevel))
 
 ERROR(implementation_only_decl_non_override,none,
       "'@_implementationOnly' can only be used on overrides", ())

include/swift/AST/TypeCheckRequests.h

@@ -3676,6 +3676,25 @@ class CheckInconsistentSPIOnlyImportsRequest
   bool isCached() const { return true; }
 };
 
+// Check that imports of the same module from the same file have the same
+// access-level.
+class CheckInconsistentAccessLevelOnImportSameFileRequest
+    : public SimpleRequest<CheckInconsistentAccessLevelOnImportSameFileRequest,
+                           evaluator::SideEffect(SourceFile *),
+                           RequestFlags::Cached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  evaluator::SideEffect evaluate(Evaluator &evaluator, SourceFile *mod) const;
+
+public:
+  // Cached.
+  bool isCached() const { return true; }
+};
+
 /// Report default imports if other imports of the same target from this
 /// module have an explicitly defined access level. In such a case, all imports
 /// of the target module need an explicit access level or it may be made public

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -37,6 +37,8 @@ SWIFT_REQUEST(TypeChecker, CheckInconsistentSPIOnlyImportsRequest,
               evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckInconsistentAccessLevelOnImport,
               evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
+SWIFT_REQUEST(TypeChecker, CheckInconsistentAccessLevelOnImportSameFileRequest,
+              evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckInconsistentWeakLinkedImportsRequest,
               evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckRedeclarationRequest,

lib/Sema/ImportResolution.cpp

@@ -970,6 +970,58 @@ CheckInconsistentSPIOnlyImportsRequest::evaluate(
   return {};
 }
 
+evaluator::SideEffect
+CheckInconsistentAccessLevelOnImportSameFileRequest::evaluate(
+    Evaluator &evaluator, SourceFile *SF) const {
+
+  // Gather the most permissive import decl for each imported module.
+  llvm::DenseMap<ModuleDecl *, const ImportDecl *> mostPermissiveImports;
+  for (auto *topLevelDecl : SF->getTopLevelDecls()) {
+    auto *importDecl = dyn_cast<ImportDecl>(topLevelDecl);
+    if (!importDecl)
+      continue;
+
+    ModuleDecl *importedModule = importDecl->getModule();
+    if (!importedModule)
+      continue;
+
+    auto otherImportDecl = mostPermissiveImports.find(importedModule);
+    if (otherImportDecl == mostPermissiveImports.end() ||
+        otherImportDecl->second->getAccessLevel() <
+          importDecl->getAccessLevel()) {
+      mostPermissiveImports[importedModule] = importDecl;
+    }
+  }
+
+  // Report import decls that are not the most permissive.
+  auto &diags = SF->getASTContext().Diags;
+  for (auto *topLevelDecl : SF->getTopLevelDecls()) {
+    auto *importDecl = dyn_cast<ImportDecl>(topLevelDecl);
+    if (!importDecl)
+      continue;
+
+    ModuleDecl *importedModule = importDecl->getModule();
+    if (!importedModule)
+      continue;
+
+    auto otherImportDecl = mostPermissiveImports.find(importedModule);
+    if (otherImportDecl != mostPermissiveImports.end() &&
+        otherImportDecl->second != importDecl &&
+        otherImportDecl->second->getAccessLevel() >
+          importDecl->getAccessLevel()) {
+      diags.diagnose(importDecl, diag::inconsistent_import_access_levels,
+                     importedModule,
+                     otherImportDecl->second->getAccessLevel(),
+                     importDecl->getAccessLevel());
+      diags.diagnose(otherImportDecl->second,
+                     diag::inconsistent_implicit_access_level_on_import_here,
+                     otherImportDecl->second->getAccessLevel());
+    }
+  }
+
+  return {};
+}
+
 evaluator::SideEffect
 CheckInconsistentAccessLevelOnImport::evaluate(
     Evaluator &evaluator, SourceFile *SF) const {

lib/Sema/TypeChecker.cpp

@@ -306,23 +306,34 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
   diagnoseUnnecessaryPublicImports(*SF);
 
   // Check to see if there are any inconsistent imports.
+  // Whole-module @_implementationOnly imports.
   evaluateOrDefault(
       Ctx.evaluator,
       CheckInconsistentImplementationOnlyImportsRequest{SF->getParentModule()},
       {});
 
+  // Whole-module @_spiOnly imports.
   evaluateOrDefault(
       Ctx.evaluator,
       CheckInconsistentSPIOnlyImportsRequest{SF},
       {});
 
+  // Whole-module ambiguous bare imports defaulting to public, when other
+  // imports are marked 'internal'.
   if (!Ctx.LangOpts.hasFeature(Feature::InternalImportsByDefault)) {
     evaluateOrDefault(
       Ctx.evaluator,
       CheckInconsistentAccessLevelOnImport{SF},
       {});
   }
 
+  // Per-file inconsistent access-levels on imports of the same module.
+  evaluateOrDefault(
+    Ctx.evaluator,
+    CheckInconsistentAccessLevelOnImportSameFileRequest{SF},
+    {});
+
+  // Whole-module inconsistent @_weakLinked.
   evaluateOrDefault(
       Ctx.evaluator,
       CheckInconsistentWeakLinkedImportsRequest{SF->getParentModule()}, {});

test/Sema/access-level-import-inconsistent-same-file.swift

@@ -0,0 +1,118 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+/// Build the library.
+// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib1 -o %t
+// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib2 -o %t
+// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib3 -o %t
+// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib4 -o %t
+// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib5 -o %t
+
+/// Test main cases.
+// RUN: %target-swift-frontend -typecheck -verify %t/Client.swift -I %t
+// RUN: %target-swift-frontend -typecheck -verify %t/Client_Scoped.swift -I %t
+// RUN: %target-swift-frontend -typecheck -verify %t/Client_Clang.swift -I %t
+
+/// Test language mode specific variants.
+// RUN: %target-swift-frontend -typecheck -verify %t/Client_Swift6.swift -I %t \
+// RUN:   -enable-upcoming-feature InternalImportsByDefault
+// RUN: %target-swift-frontend -typecheck -verify %t/Client_Swift5.swift -I %t \
+// RUN:   -swift-version 5
+
+//--- Lib.swift
+
+public struct Type1 {}
+public struct Type2 {}
+
+//--- Client.swift
+
+/// Simple public vs internal.
+public import Lib1 // expected-note {{imported 'public' here}}
+internal import Lib1 // expected-warning {{module 'Lib1' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
+
+/// Simple public vs internal, inverted.
+internal import Lib2 // expected-warning {{module 'Lib2' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
+public import Lib2 // expected-note {{imported 'public' here}}
+
+/// 3 different ones.
+public import Lib3 // expected-note 2 {{imported 'public' here}}
+internal import Lib3 // expected-warning {{module 'Lib3' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
+private import Lib3 // expected-warning {{module 'Lib3' is imported as 'public' from the same file; this 'private' access level will be ignored}}
+
+/// private vs fileprivate, we don't really need this warning but it may point to unintended stylistic inconsistencies.
+fileprivate import Lib4 // expected-note {{imported 'fileprivate' here}}
+private import Lib4 // expected-warning {{module 'Lib4' is imported as 'fileprivate' from the same file; this 'private' access level will be ignored}}
+
+// Don't complain about repeated imports. As far as this diagnostic
+// is concerned we may see this with scoped imports.
+internal import Lib5
+internal import Lib5
+internal import Lib5
+internal import Lib5
+
+public func dummyAPI(t1: Lib1.Type1, t2: Lib2.Type1, t3: Lib3.Type1) {}
+
+//--- Client_Swift5.swift
+
+/// Simple public vs internal, imports defaults to public.
+import Lib1 // expected-note {{imported 'public' here}}
+// expected-error @-1 {{ambiguous implicit access level for import of 'Lib1'; it is imported as 'internal' elsewhere}}
+internal import Lib1 // expected-warning {{module 'Lib1' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
+// expected-note @-1 {{imported 'internal' here}}
+
+// There's no warning about "will be ignored" for a matching implicit access level.
+public import Lib2
+// expected-note @-1 {{imported 'public' here}}
+import Lib2
+// expected-error @-1 {{ambiguous implicit access level for import of 'Lib2'; it is imported as 'public' elsewhere}}
+
+public func dummyAPI(t: Lib1.Type1, t2: Lib2.Type1) {}
+
+//--- Client_Swift6.swift
+
+/// Simple public vs internal, imports default to internal.
+public import Lib1 // expected-note {{imported 'public' here}}
+import Lib1 // expected-warning {{module 'Lib1' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
+
+// There's no warning about "will be ignored" for a matching implicit access level.
+import Lib2
+internal import Lib2
+
+public func dummyAPI(t: Lib1.Type1) {}
+
+//--- Client_Scoped.swift
+
+/// Access level on scoped imports still import the whole module.
+public import struct Lib1.Type1 // expected-note {{imported 'public' here}}
+internal import struct Lib1.Type2 // expected-warning {{module 'Lib1' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
+
+public func dummyAPI(t: Lib1.Type1) {}
+
+//--- Client_Clang.swift
+
+public import ClangLib.Sub1 // expected-note {{imported 'public' here}}
+// expected-warning @-1 {{public import of 'Sub1' was not used in public declarations or inlinable code}}
+private import ClangLib.Sub1 // expected-warning {{module 'Sub1' is imported as 'public' from the same file; this 'private' access level will be ignored}}
+internal import ClangLib.Sub2
+
+public func dummyAPI(t1: ClangType1, t2: ClangType2) {}
+
+//--- module.modulemap
+
+module ClangLib {
+  header "ClangLib1.h"
+
+  explicit module Sub1 {
+    header "ClangLib2.h"
+  }
+
+  explicit module Sub2 {
+    header "ClangLib3.h"
+  }
+}
+
+//--- ClangLib1.h
+struct ClangType1 {};
+//--- ClangLib2.h
+struct ClangType2 {};
+//--- ClangLib3.h