[Sema] Report imports marked as public that are not used in API

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -2451,6 +2451,12 @@ REMARK(add_predates_concurrency_import,none,
      "%select{| as warnings}0", (bool, Identifier))
 REMARK(remove_predates_concurrency_import,none,
      "'@preconcurrency' attribute on module %0 is unused", (Identifier))
+WARNING(remove_public_import,none,
+        "public import of %0 was not used in public declarations or inlinable code",
+        (const ModuleDecl *))
+WARNING(remove_package_import,none,
+        "package import of %0 was not used in package declarations",
+        (const ModuleDecl *))
 WARNING(public_decl_needs_sendable,none,
         "public %kind0 does not specify whether it is 'Sendable' or not",
         (const ValueDecl *))

include/swift/AST/SourceFile.h

@@ -115,6 +115,10 @@ class SourceFile final : public FileUnit {
   llvm::SmallDenseSet<AttributedImport<ImportedModule>>
       PreconcurrencyImportsUsed;
 
+  /// The highest access level of declarations referencing each import.
+  llvm::DenseMap<AttributedImport<ImportedModule>, 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
   /// same module.
@@ -365,6 +369,15 @@ class SourceFile final : public FileUnit {
   void setImportUsedPreconcurrency(
       AttributedImport<ImportedModule> import);
 
+  /// Return the highest access level of the declarations referencing
+  /// this import in signature or inlinable code.
+  AccessLevel
+  getMaxAccessLevelUsingImport(AttributedImport<ImportedModule> import) const;
+
+  /// Register the use of \p import from an API with \p accessLevel.
+  void registerAccessLevelUsingImport(AttributedImport<ImportedModule> import,
+                                      AccessLevel accessLevel);
+
   enum ImportQueryKind {
     /// Return the results for testable or private imports.
     TestableAndPrivate,

lib/AST/Module.cpp

@@ -3087,6 +3087,25 @@ void SourceFile::setImportUsedPreconcurrency(
   PreconcurrencyImportsUsed.insert(import);
 }
 
+AccessLevel
+SourceFile::getMaxAccessLevelUsingImport(
+    AttributedImport<ImportedModule> import) const {
+  auto known = ImportsUseAccessLevel.find(import);
+  if (known == ImportsUseAccessLevel.end())
+    return AccessLevel::Internal;
+  return known->second;
+}
+
+void SourceFile::registerAccessLevelUsingImport(
+    AttributedImport<ImportedModule> import,
+    AccessLevel accessLevel) {
+  auto known = ImportsUseAccessLevel.find(import);
+  if (known == ImportsUseAccessLevel.end())
+    ImportsUseAccessLevel[import] = accessLevel;
+  else
+    ImportsUseAccessLevel[import] = std::max(accessLevel, known->second);
+}
+
 bool HasImportsMatchingFlagRequest::evaluate(Evaluator &evaluator,
                                              SourceFile *SF,
                                              ImportFlags flag) const {

lib/Sema/ResilienceDiagnostics.cpp

@@ -60,8 +60,18 @@ bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
   if (D->getDeclContext()->isLocalContext())
     return false;
 
-  // General check on access-level of the decl.
   auto *DC = where.getDeclContext();
+  auto &Context = DC->getASTContext();
+
+  ImportAccessLevel problematicImport = D->getImportAccessFrom(DC);
+  if (problematicImport.has_value()) {
+    auto SF = DC->getParentSourceFile();
+    if (SF)
+      SF->registerAccessLevelUsingImport(problematicImport.value(),
+                                         AccessLevel::Public);
+  }
+
+  // General check on access-level of the decl.
   auto declAccessScope =
       D->getFormalAccessScope(/*useDC=*/DC,
                               /*allowUsableFromInline=*/true);
@@ -72,8 +82,6 @@ bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
   if (declAccessScope.isPublic())
     return false;
 
-  auto &Context = DC->getASTContext();
-
   // Dynamic declarations were mistakenly not checked in Swift 4.2.
   // Do enforce the restriction even in pre-Swift-5 modes if the module we're
   // building is resilient, though.
@@ -112,7 +120,6 @@ bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
 
   Context.Diags.diagnose(D, diag::resilience_decl_declared_here, D);
 
-  ImportAccessLevel problematicImport = D->getImportAccessFrom(DC);
   if (problematicImport.has_value() &&
       problematicImport->accessLevel < D->getFormalAccess()) {
     Context.Diags.diagnose(problematicImport->importLoc,
@@ -133,15 +140,24 @@ static bool diagnoseTypeAliasDeclRefExportability(SourceLoc loc,
   if (!D)
     return false;
 
+  auto exportingModule = where.getDeclContext()->getParentModule();
+  ASTContext &ctx = exportingModule->getASTContext();
+
+  ImportAccessLevel problematicImport = D->getImportAccessFrom(
+                                                       where.getDeclContext());
+  if (problematicImport.has_value()) {
+    auto SF = where.getDeclContext()->getParentSourceFile();
+    if (SF)
+      SF->registerAccessLevelUsingImport(problematicImport.value(),
+                                         AccessLevel::Public);
+  }
+
   auto ignoredDowngradeToWarning = DowngradeToWarning::No;
   auto originKind =
       getDisallowedOriginKind(D, where, ignoredDowngradeToWarning);
   if (originKind == DisallowedOriginKind::None)
     return false;
 
-  auto exportingModule = where.getDeclContext()->getParentModule();
-  ASTContext &ctx = exportingModule->getASTContext();
-
   // As an exception, if the import of the module that defines the desugared
   // decl is just missing (as opposed to imported explicitly with reduced
   // visibility) then we should only diagnose if we're building a resilient
@@ -184,7 +200,7 @@ static bool diagnoseTypeAliasDeclRefExportability(SourceLoc loc,
     assert(limitImport.has_value() &&
            limitImport->accessLevel < AccessLevel::Public &&
            "The import should still be non-public");
-    ctx.Diags.diagnose(limitImport->accessLevelLoc,
+    ctx.Diags.diagnose(limitImport->importLoc,
                        diag::decl_import_via_here, D,
                        limitImport->accessLevel,
                        limitImport->module.importedModule);
@@ -288,13 +304,21 @@ TypeChecker::diagnoseConformanceExportability(SourceLoc loc,
   if (ext->getParentModule()->isBuiltinModule())
     return false;
 
+  ModuleDecl *M = ext->getParentModule();
+  ASTContext &ctx = M->getASTContext();
+
+  ImportAccessLevel problematicImport = ext->getImportAccessFrom(where.getDeclContext());
+  if (problematicImport.has_value()) {
+    auto SF = where.getDeclContext()->getParentSourceFile();
+    if (SF)
+      SF->registerAccessLevelUsingImport(problematicImport.value(),
+                                         AccessLevel::Public);
+  }
+
   auto originKind = getDisallowedOriginKind(ext, where);
   if (originKind == DisallowedOriginKind::None)
     return false;
 
-  ModuleDecl *M = ext->getParentModule();
-  ASTContext &ctx = M->getASTContext();
-
   auto reason = where.getExportabilityReason();
   if (!reason.has_value())
     reason = ExportabilityReason::General;
@@ -323,7 +347,7 @@ TypeChecker::diagnoseConformanceExportability(SourceLoc loc,
     assert(limitImport.has_value() &&
            limitImport->accessLevel < AccessLevel::Public &&
            "The import should still be non-public");
-    ctx.Diags.diagnose(limitImport->accessLevelLoc,
+    ctx.Diags.diagnose(limitImport->importLoc,
                        diag::decl_import_via_here, ext,
                        limitImport->accessLevel,
                        limitImport->module.importedModule);

lib/Sema/TypeCheckAccess.cpp

@@ -191,6 +191,35 @@ void AccessControlCheckerBase::checkTypeAccessImpl(
       contextAccessScope.getDeclContext()->isLocalContext())
     return;
 
+  // Report where it was imported from.
+  if (contextAccessScope.isPublic() ||
+      contextAccessScope.isPackage()) {
+    auto SF = useDC->getParentSourceFile();
+    auto report = [&](const IdentTypeRepr *typeRepr, const ValueDecl *VD) {
+      ImportAccessLevel import = VD->getImportAccessFrom(useDC);
+      if (import.has_value()) {
+        if (SF) {
+          auto useLevel = contextAccessScope.isPublic() ? AccessLevel::Public
+                                                        : AccessLevel::Package;
+          SF->registerAccessLevelUsingImport(import.value(), useLevel);
+        }
+      }
+    };
+
+    if (typeRepr) {
+      typeRepr->walk(TypeReprIdentFinder([&](const IdentTypeRepr *TR) {
+        const ValueDecl *VD = TR->getBoundDecl();
+        report(TR, VD);
+        return true;
+      }));
+    } else if (type) {
+      type.walk(SimpleTypeDeclFinder([&](const ValueDecl *VD) {
+        report(/*typeRepr=*/nullptr, VD);
+        return TypeWalker::Action::Continue;
+      }));
+    }
+  };
+
   AccessScope problematicAccessScope = AccessScope::getPublic();
 
   if (type) {
@@ -2358,6 +2387,39 @@ DisallowedOriginKind swift::getDisallowedOriginKind(const Decl *decl,
   return getDisallowedOriginKind(decl, where, downgradeToWarning);
 }
 
+void swift::diagnoseUnnecessaryPublicImports(SourceFile &SF) {
+  ASTContext &ctx = SF.getASTContext();
+  if (ctx.TypeCheckerOpts.SkipFunctionBodies != FunctionBodySkipping::None)
+    return;
+
+  for (const auto &import : SF.getImports()) {
+    // Only check imports with an explicit access level above internal.
+    if (!import.accessLevelRange.isValid() ||
+        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.isSwiftVersionAtLeast(6)) {
+        // Let it default to internal.
+        inFlight.fixItRemove(import.accessLevelRange);
+      } else {
+        inFlight.fixItReplace(import.accessLevelRange, "internal");
+      }
+    }
+  }
+}
+
 void swift::checkAccessControl(Decl *D) {
   if (isa<ValueDecl>(D) || isa<PatternBindingDecl>(D)) {
     bool allowInlineable =

lib/Sema/TypeChecker.cpp

@@ -297,6 +297,7 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
   }
 
   diagnoseUnnecessaryPreconcurrencyImports(*SF);
+  diagnoseUnnecessaryPublicImports(*SF);
 
   // Check to see if there are any inconsistent imports.
   evaluateOrDefault(

lib/Sema/TypeChecker.h

@@ -1547,6 +1547,9 @@ class DiscriminatorFinder : public ASTWalker {
   }
 };
 
+/// Report imports that are marked public but are not used in API.
+void diagnoseUnnecessaryPublicImports(SourceFile &SF);
+
 } // end namespace swift
 
 #endif

test/Sema/access-level-and-non-resilient-import.swift

@@ -45,7 +45,9 @@
 
 import DefaultLib // expected-error {{module 'DefaultLib' was not compiled with library evolution support; using it means binary compatibility for 'Client_Swift5' can't be guaranteed}} {{1-1=internal }}
 public import PublicLib // expected-error {{module 'PublicLib' was not compiled with library evolution support; using it means binary compatibility for 'Client_Swift5' can't be guaranteed}} {{1-7=internal}}
+// expected-warning @-1 {{public import of 'PublicLib' was not used in public declarations or inlinable code}}
 package import PackageLib
+// expected-warning @-1 {{package import of 'PackageLib' was not used in package declarations}}
 internal import InternalLib
 fileprivate import FileprivateLib
 private import PrivateLib
@@ -54,7 +56,9 @@ private import PrivateLib
 
 import DefaultLib
 public import PublicLib // expected-error {{module 'PublicLib' was not compiled with library evolution support; using it means binary compatibility for 'Client_Swift6' can't be guaranteed}} {{1-7=internal}}
+// expected-warning @-1 {{public import of 'PublicLib' was not used in public declarations or inlinable code}}
 package import PackageLib
+// expected-warning @-1 {{package import of 'PackageLib' was not used in package declarations}}
 internal import InternalLib
 fileprivate import FileprivateLib
 private import PrivateLib

test/Sema/access-level-import-diag-priority.swift

@@ -91,7 +91,6 @@ public func publicFuncUsesPackageLevelPackageImportedType(_ a: PackageLevelPacka
 
 /// Local vs imports
 //--- LocalVsImportClient.swift
-public import PublicLib
 internal import InternalLib
 fileprivate import FileprivateLib // expected-note {{struct 'FileprivateImportType' imported as 'fileprivate' from 'FileprivateLib' here}}
 

test/Sema/access-level-import-exportability.swift

@@ -119,10 +119,10 @@ public struct PrivateLibWrapper<T> {
 
 /// Short test mostly to count the notes.
 //--- MinimalClient.swift
-public import PublicLib
-package import PackageLib
-internal import InternalLib // expected-note@:1 {{struct 'InternalImportType' imported as 'internal' from 'InternalLib' here}}
-fileprivate import FileprivateLib // expected-note@:1 {{class 'FileprivateImportClass' imported as 'fileprivate' from 'FileprivateLib' here}}
+public import PublicLib // expected-warning {{public import of 'PublicLib' was not used in public declarations or inlinable code}}
+package import PackageLib // expected-warning {{package import of 'PackageLib' was not used in package declarations}}
+internal import InternalLib // expected-note {{struct 'InternalImportType' imported as 'internal' from 'InternalLib' here}}
+fileprivate import FileprivateLib // expected-note {{class 'FileprivateImportClass' imported as 'fileprivate' from 'FileprivateLib' here}}
 private import PrivateLib
 
 public func PublicFuncUsesInternal(_: InternalImportType) { // expected-error {{function cannot be declared public because its parameter uses an internal type}}