Merge pull request #70700 from xymus/fix-submodule-import-warnings

xymus · web-flow · commit e32c05da9ee9 · 2024-01-04T14:59:55.000-08:00
Sema: Don't report public imports of submodules as being unused in API
diff --git a/include/swift/AST/SourceFile.h b/include/swift/AST/SourceFile.h
@@ -149,8 +149,7 @@ class SourceFile final : public FileUnit {
       PreconcurrencyImportsUsed;
 
   /// The highest access level of declarations referencing each import.
-  llvm::DenseMap<AttributedImport<ImportedModule>, AccessLevel>
-      ImportsUseAccessLevel;
+  llvm::DenseMap<const ModuleDecl *, AccessLevel> ImportsUseAccessLevel;
 
   /// A unique identifier representing this file; used to mark private decls
   /// within the file to keep them from conflicting with other files in the
@@ -418,7 +417,7 @@ class SourceFile final : public FileUnit {
   /// Return the highest access level of the declarations referencing
   /// this import in signature or inlinable code.
   AccessLevel
-  getMaxAccessLevelUsingImport(AttributedImport<ImportedModule> import) const;
+  getMaxAccessLevelUsingImport(const ModuleDecl *import) const;
 
   /// Register the use of \p import from an API with \p accessLevel.
   void registerAccessLevelUsingImport(AttributedImport<ImportedModule> import,
diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp
@@ -3192,8 +3192,8 @@ void SourceFile::setImportUsedPreconcurrency(
 
 AccessLevel
 SourceFile::getMaxAccessLevelUsingImport(
-    AttributedImport<ImportedModule> import) const {
-  auto known = ImportsUseAccessLevel.find(import);
+    const ModuleDecl *mod) const {
+  auto known = ImportsUseAccessLevel.find(mod);
   if (known == ImportsUseAccessLevel.end())
     return AccessLevel::Internal;
   return known->second;
@@ -3202,11 +3202,12 @@ SourceFile::getMaxAccessLevelUsingImport(
 void SourceFile::registerAccessLevelUsingImport(
     AttributedImport<ImportedModule> import,
     AccessLevel accessLevel) {
-  auto known = ImportsUseAccessLevel.find(import);
+  auto mod = import.module.importedModule;
+  auto known = ImportsUseAccessLevel.find(mod);
   if (known == ImportsUseAccessLevel.end())
-    ImportsUseAccessLevel[import] = accessLevel;
+    ImportsUseAccessLevel[mod] = accessLevel;
   else
-    ImportsUseAccessLevel[import] = std::max(accessLevel, known->second);
+    ImportsUseAccessLevel[mod] = std::max(accessLevel, known->second);
 }
 
 bool HasImportsMatchingFlagRequest::evaluate(Evaluator &evaluator,
diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp
@@ -2440,24 +2440,30 @@ void swift::diagnoseUnnecessaryPublicImports(SourceFile &SF) {
         import.accessLevel <= AccessLevel::Internal)
       continue;
 
-    AccessLevel levelUsed = SF.getMaxAccessLevelUsingImport(import);
-    if (import.accessLevel > levelUsed) {
-      auto diagId = import.accessLevel == AccessLevel::Public ?
-                                                  diag::remove_public_import :
-                                                  diag::remove_package_import;
-
-      auto inFlight = ctx.Diags.diagnose(import.importLoc,
-                                         diagId,
-                                         import.module.importedModule);
-
-      if (levelUsed == AccessLevel::Package) {
-        inFlight.fixItReplace(import.accessLevelRange, "package");
-      } else if (ctx.LangOpts.hasFeature(Feature::InternalImportsByDefault)) {
-        // Let it default to internal.
-        inFlight.fixItRemove(import.accessLevelRange);
-      } else {
-        inFlight.fixItReplace(import.accessLevelRange, "internal");
-      }
+    // Ignore submodules as we associate decls with the top module.
+    // The top module will be reported if it's not used.
+    auto importedModule = import.module.importedModule;
+    if (importedModule->getTopLevelModule() != importedModule)
+      continue;
+
+    AccessLevel levelUsed = SF.getMaxAccessLevelUsingImport(importedModule);
+    if (import.accessLevel <= levelUsed)
+      continue;
+
+    auto diagId = import.accessLevel == AccessLevel::Public ?
+                                          diag::remove_public_import :
+                                          diag::remove_package_import;
+    auto inFlight = ctx.Diags.diagnose(import.importLoc,
+                                       diagId,
+                                       importedModule);
+
+    if (levelUsed == AccessLevel::Package) {
+      inFlight.fixItReplace(import.accessLevelRange, "package");
+    } else if (ctx.LangOpts.hasFeature(Feature::InternalImportsByDefault)) {
+      // Let it default to internal.
+      inFlight.fixItRemove(import.accessLevelRange);
+    } else {
+      inFlight.fixItReplace(import.accessLevelRange, "internal");
     }
   }
 }
diff --git a/test/ModuleInterface/imports-swift6.swift b/test/ModuleInterface/imports-swift6.swift
@@ -19,9 +19,7 @@
 
 //--- main.swift
 @_exported public import resilient // expected-warning {{public import of 'resilient' was not used in public declarations or inlinable code}}
-public import B.B2 // expected-warning {{public import of 'B2' was not used in public declarations or inlinable code}}
-// expected-warning @-1 {{public import of 'B' was not used in public declarations or inlinable code}}
-// FIXME: We don't want this last warning.
+public import B.B2 // expected-warning {{public import of 'B' was not used in public declarations or inlinable code}}
 
 public import func C.c // expected-warning {{public import of 'C' was not used in public declarations or inlinable code}}
 // expected-warning @-1 {{scoped imports are not yet supported in module interfaces}}
@@ -34,8 +32,7 @@ public import NotSoSecret2 // expected-warning {{'NotSoSecret2' inconsistently i
 
 //--- main-other.swift
 public import A // expected-warning {{public import of 'A' was not used in public declarations or inlinable code}}
-public import B.B3 // expected-warning {{public import of 'B3' was not used in public declarations or inlinable code}}
-// expected-warning @-1 {{public import of 'B' was not used in public declarations or inlinable code}}
+public import B.B3 // expected-warning {{public import of 'B' was not used in public declarations or inlinable code}}
 public import D // expected-warning {{public import of 'D' was not used in public declarations or inlinable code}}
 
 public import NotSoSecret // expected-warning {{'NotSoSecret' inconsistently imported as implementation-only}}
diff --git a/test/Sema/access-level-import-inconsistent-same-file.swift b/test/Sema/access-level-import-inconsistent-same-file.swift
@@ -91,7 +91,6 @@ 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
 
diff --git a/test/Sema/superfluously-public-imports.swift b/test/Sema/superfluously-public-imports.swift
@@ -24,6 +24,8 @@
 /// Check diagnostics.
 // RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
 // RUN:   -package-name pkg -Rmodule-api-import -swift-version 6 -verify
+// RUN: %target-swift-frontend -typecheck %t/ClientOfClangModules.swift -I %t \
+// RUN:   -package-name pkg -Rmodule-api-import -swift-version 6 -verify
 // RUN: %target-swift-frontend -typecheck %t/Client_Swift5.swift -I %t \
 // RUN:   -swift-version 5 -verify
 
@@ -211,3 +213,63 @@ func implicitlyInternalFunc(a: NotAnAPIType = notAnAPIFunc()) {}
 
 // For package decls we only remark on types used in signatures, not for inlinable code.
 package func packageFunc(a: PackageType = packageFunc()) {} // expected-remark {{struct 'PackageType' is imported via 'ImportUsedInPackage'}}
+
+/// Tests for imports of clang modules.
+//--- module.modulemap
+module ClangSimpleUnused {
+    header "ClangSimpleUnused.h"
+}
+module ClangSimple {
+    header "ClangSimple.h"
+}
+
+module ClangSubmodule {
+    header "ClangSubmodule.h"
+
+    module ClangSubmoduleSubmodule {
+      header "ClangSubmoduleSubmodule.h"
+    }
+}
+
+module ClangSubmoduleUnused {
+    header "ClangSubmoduleUnused.h"
+
+    module ClangSubmoduleUnsuedSubmodule {
+      header "ClangSubmoduleUnusedSubmodule.h"
+    }
+}
+
+module ClangTopModule {
+  header "ClangTopModule.h"
+  module ClangTopModuleSubmodule {
+    header "ClangTopModuleSubmodule.h"
+  }
+}
+
+//--- ClangSimpleUnused.h
+//--- ClangSimple.h
+struct ClangSimpleType {};
+
+//--- ClangSubmodule.h
+//--- ClangSubmoduleSubmodule.h
+struct ClangSubmoduleSubmoduleType {};
+
+//--- ClangSubmoduleUnused.h
+//--- ClangSubmoduleUnusedSubmodule.h
+
+//--- ClangTopModule.h
+struct ClangTopModuleType {};
+//--- ClangTopModuleSubmodule.h
+
+//--- ClientOfClangModules.swift
+public import ClangSimple
+public import ClangSimpleUnused // expected-warning {{public import of 'ClangSimpleUnused' was not used in public declarations or inlinable code}}
+public import ClangSubmodule.ClangSubmoduleSubmodule
+public import ClangSubmoduleUnused.ClangSubmoduleUnsuedSubmodule // expected-warning {{public import of 'ClangSubmoduleUnused' was not used in public declarations or inlinable code}}
+
+// Only the top-level module is used, but we can't detect whether the submodule was used or not.
+public import ClangTopModule.ClangTopModuleSubmodule
+
+public func clangUser(a: ClangSimpleType) {} // expected-remark {{struct 'ClangSimpleType' is imported via 'ClangSimple'}}
+public func clangUser(a: ClangSubmoduleSubmoduleType) {} // expected-remark {{struct 'ClangSubmoduleSubmoduleType' is imported via 'ClangSubmodule'}}
+public func clangUser(a: ClangTopModuleType) {} // expected-remark {{struct 'ClangTopModuleType' is imported via 'ClangTopModule'}}
