Merge pull request #70675 from xymus/access-level-import-same-file

xymus · web-flow · commit 8efd30ec564b · 2024-01-03T12:12:03.000-08:00
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -3675,6 +3675,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", ())
diff --git a/include/swift/AST/TypeCheckRequests.h b/include/swift/AST/TypeCheckRequests.h
@@ -3708,6 +3708,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
diff --git a/include/swift/AST/TypeCheckerTypeIDZone.def b/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,
diff --git a/lib/Sema/ImportResolution.cpp b/lib/Sema/ImportResolution.cpp
@@ -971,6 +971,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 {
diff --git a/lib/Sema/TypeChecker.cpp b/lib/Sema/TypeChecker.cpp
@@ -314,23 +314,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()}, {});
diff --git a/test/Sema/access-level-import-inconsistencies.swift b/test/Sema/access-level-import-inconsistencies.swift
@@ -24,10 +24,15 @@ public struct LibType {}
 // RUN:   -package-name package -verify
 //--- OneFile_AllExplicit.swift
 public import Lib // expected-warning {{public import of 'Lib' was not used in public declarations or inlinable code}}
+// expected-note @-1 4 {{imported 'public' here}}
 package import Lib // expected-warning {{package import of 'Lib' was not used in package declarations}}
+// expected-warning @-1 {{module 'Lib' is imported as 'public' from the same file; this 'package' access level will be ignored}}
 internal import Lib
+// expected-warning @-1 {{module 'Lib' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
 fileprivate import Lib
+// expected-warning @-1 {{module 'Lib' is imported as 'public' from the same file; this 'fileprivate' access level will be ignored}}
 private import Lib
+// expected-warning @-1 {{module 'Lib' is imported as 'public' from the same file; this 'private' access level will be ignored}}
 
 // RUN: %target-swift-frontend -typecheck %t/ManyFiles_AllExplicit_File?.swift -I %t \
 // RUN:   -package-name package -verify
diff --git a/test/Sema/access-level-import-inconsistent-same-file.swift b/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
diff --git a/test/Sema/superfluously-public-imports.swift b/test/Sema/superfluously-public-imports.swift
@@ -122,8 +122,10 @@ public import ExtendedDefinitionNonPublic // expected-warning {{public import of
 
 /// Repeat some imports to make sure we report all of them.
 public import UnusedImport // expected-warning {{public import of 'UnusedImport' was not used in public declarations or inlinable code}} {{1-8=}}
+// expected-note @-1 {{imported 'public' here}}
 public import UnusedImport // expected-warning {{public import of 'UnusedImport' was not used in public declarations or inlinable code}} {{1-8=}}
 package import UnusedImport // expected-warning {{package import of 'UnusedImport' was not used in package declarations}} {{1-9=}}
+// expected-warning @-1 {{module 'UnusedImport' is imported as 'public' from the same file; this 'package' access level will be ignored}}
 
 package import UnusedPackageImport // expected-warning {{package import of 'UnusedPackageImport' was not used in package declarations}} {{1-9=}}
 public import ImportNotUseFromAPI // expected-warning {{public import of 'ImportNotUseFromAPI' was not used in public declarations or inlinable code}} {{1-8=}}
