Merge pull request #68831 from xymus/report-superfluously-public-imports

Sema: warn on imports that are too public and remark source of entities in API

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -1126,6 +1126,20 @@ REMARK(module_loaded,none,
        "; source: '%2', loaded: '%3'",
        (Identifier, unsigned, StringRef, StringRef))
 
+REMARK(module_api_import,none,
+       "%select{|implicitly used }4"
+       "%kind0 is imported via %1"
+       "%select{, which reexports definition from %2|}3",
+       (const Decl *, ModuleDecl *, ModuleDecl *, bool, bool))
+REMARK(module_api_import_conformance,none,
+       "conformance of %0 to %kind1 used here is imported via %2"
+       "%select{ which reexports the definition from %3|}4",
+       (const Type, const Decl *, ModuleDecl *, ModuleDecl *, bool))
+REMARK(module_api_import_aliases,none,
+       "typealias underlying type %kind0 is imported via %1"
+       "%select{, which reexports definition from %2|}3",
+       (const Decl *, ModuleDecl *, ModuleDecl *, bool))
+
 REMARK(macro_loaded,none,
        "loaded macro implementation module %0 from "
        "%select{shared library '%2'|executable '%2'|"
@@ -2454,6 +2468,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/Import.h

@@ -591,19 +591,19 @@ struct AttributedImport {
 
   /// Location of the attribute that defined \c accessLevel. Also indicates
   /// if the access level was implicit or explicit.
-  SourceLoc accessLevelLoc;
+  SourceRange accessLevelRange;
 
   AttributedImport(ModuleInfo module, SourceLoc importLoc = SourceLoc(),
                    ImportOptions options = ImportOptions(),
                    StringRef filename = {}, ArrayRef<Identifier> spiGroups = {},
                    SourceRange preconcurrencyRange = {},
                    llvm::Optional<AccessLevel> docVisibility = llvm::None,
                    AccessLevel accessLevel = AccessLevel::Public,
-                   SourceLoc accessLevelLoc = SourceLoc())
+                   SourceRange accessLevelRange = SourceRange())
       : module(module), importLoc(importLoc), options(options),
         sourceFileArg(filename), spiGroups(spiGroups),
         preconcurrencyRange(preconcurrencyRange), docVisibility(docVisibility),
-        accessLevel(accessLevel), accessLevelLoc(accessLevelLoc) {
+        accessLevel(accessLevel), accessLevelRange(accessLevelRange) {
     assert(!(options.contains(ImportFlags::Exported) &&
              options.contains(ImportFlags::ImplementationOnly)) ||
            options.contains(ImportFlags::Reserved));
@@ -614,7 +614,7 @@ struct AttributedImport {
     : AttributedImport(module, other.importLoc, other.options,
                        other.sourceFileArg, other.spiGroups,
                        other.preconcurrencyRange, other.docVisibility,
-                       other.accessLevel, other.accessLevelLoc) { }
+                       other.accessLevel, other.accessLevelRange) { }
 
   friend bool operator==(const AttributedImport<ModuleInfo> &lhs,
                          const AttributedImport<ModuleInfo> &rhs) {
@@ -624,7 +624,7 @@ struct AttributedImport {
            lhs.spiGroups == rhs.spiGroups &&
            lhs.docVisibility == rhs.docVisibility &&
            lhs.accessLevel == rhs.accessLevel &&
-           lhs.accessLevelLoc == rhs.accessLevelLoc;
+           lhs.accessLevelRange == rhs.accessLevelRange;
   }
 
   AttributedImport<ImportedModule> getLoaded(ModuleDecl *loadedModule) const {
@@ -802,7 +802,7 @@ struct DenseMapInfo<swift::AttributedImport<ModuleInfo>> {
            a.spiGroups == b.spiGroups &&
            a.docVisibility == b.docVisibility &&
            a.accessLevel == b.accessLevel &&
-           a.accessLevelLoc == b.accessLevelLoc;
+           a.accessLevelRange == b.accessLevelRange;
   }
 };
 }

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,

include/swift/Basic/LangOptions.h

@@ -246,6 +246,9 @@ namespace swift {
     /// Emit remarks about contextual inconsistencies in loaded modules.
     bool EnableModuleRecoveryRemarks = false;
 
+    /// Emit remarks about the source of each element exposed by the module API.
+    bool EnableModuleApiImportRemarks = false;
+
      /// Emit a remark after loading a macro implementation.
     bool EnableMacroLoadingRemarks = false;
 

include/swift/Option/Options.td

@@ -397,6 +397,9 @@ def remark_loading_module : Flag<["-"], "Rmodule-loading">,
 def remark_module_recovery : Flag<["-"], "Rmodule-recovery">,
   Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
   HelpText<"Emit remarks about contextual inconsistencies in loaded modules">;
+def remark_module_api_import : Flag<["-"], "Rmodule-api-import">,
+  Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
+  HelpText<"Emit remarks about the import briging in each element composing the API">;
 
 def remark_macro_loading : Flag<["-"], "Rmacro-loading">,
   Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,

lib/AST/Decl.cpp

@@ -1481,7 +1481,11 @@ ArrayRef<ValueDecl *> ImportDecl::getDecls() const {
 
 AccessLevel ImportDecl::getAccessLevel() const {
   if (auto attr = getAttrs().getAttribute<AccessControlAttr>()) {
-    return attr->getAccess();
+    if (attr->getAccess() == AccessLevel::Open) {
+      // Use a conservative import if the level given is invalid.
+      return AccessLevel::Internal;
+    } else
+      return attr->getAccess();
   }
 
   auto &LangOpts = getASTContext().LangOpts;

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/Frontend/CompilerInvocation.cpp

@@ -1021,6 +1021,7 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
 
   Opts.EnableModuleLoadingRemarks = Args.hasArg(OPT_remark_loading_module);
   Opts.EnableModuleRecoveryRemarks = Args.hasArg(OPT_remark_module_recovery);
+  Opts.EnableModuleApiImportRemarks = Args.hasArg(OPT_remark_module_api_import);
   Opts.EnableMacroLoadingRemarks = Args.hasArg(OPT_remark_macro_loading);
   Opts.EnableIndexingSystemModuleRemarks = Args.hasArg(OPT_remark_indexing_system_module);
 

lib/Sema/ImportResolution.cpp

@@ -570,7 +570,7 @@ UnboundImport::UnboundImport(ImportDecl *ID)
 
   import.accessLevel = ID->getAccessLevel();
   if (auto attr = ID->getAttrs().getAttribute<AccessControlAttr>()) {
-    import.accessLevelLoc = attr->getLocation();
+    import.accessLevelRange = attr->getLocation();
   }
 
   if (ID->getAttrs().hasAttribute<SPIOnlyAttr>())
@@ -822,9 +822,9 @@ void UnboundImport::validateResilience(NullablePtr<ModuleDecl> topLevelModule,
                                      SF.getParentModule()->getName());
 
   if (ctx.LangOpts.hasFeature(Feature::AccessLevelOnImport)) {
-    SourceLoc attrLoc = import.accessLevelLoc;
-    if (attrLoc.isValid())
-      inFlight.fixItReplace(attrLoc, "internal");
+    SourceRange attrRange = import.accessLevelRange;
+    if (attrRange.isValid())
+      inFlight.fixItReplace(attrRange, "internal");
     else
       inFlight.fixItInsert(import.importLoc, "internal ");
   } else {

lib/Sema/ResilienceDiagnostics.cpp

@@ -60,8 +60,27 @@ 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);
+
+    if (Context.LangOpts.EnableModuleApiImportRemarks) {
+      ModuleDecl *importedVia = problematicImport->module.importedModule,
+                 *sourceModule = D->getModuleContext();
+      Context.Diags.diagnose(loc, diag::module_api_import,
+                             D, importedVia, sourceModule,
+                             importedVia == sourceModule,
+                             /*isImplicit*/false);
+    }
+  }
+
+  // General check on access-level of the decl.
   auto declAccessScope =
       D->getFormalAccessScope(/*useDC=*/DC,
                               /*allowUsableFromInline=*/true);
@@ -72,8 +91,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,10 +129,9 @@ 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->accessLevelLoc,
+    Context.Diags.diagnose(problematicImport->importLoc,
                            diag::decl_import_via_here, D,
                            problematicImport->accessLevel,
                            problematicImport->module.importedModule);
@@ -133,15 +149,32 @@ 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);
+
+    if (ctx.LangOpts.EnableModuleApiImportRemarks) {
+      ModuleDecl *importedVia = problematicImport->module.importedModule,
+                 *sourceModule = D->getModuleContext();
+      ctx.Diags.diagnose(loc, diag::module_api_import_aliases,
+                             D, importedVia, sourceModule,
+                             importedVia == sourceModule);
+    }
+  }
+
   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 +217,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 +321,30 @@ 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);
+
+    if (ctx.LangOpts.EnableModuleApiImportRemarks) {
+      ModuleDecl *importedVia = problematicImport->module.importedModule,
+                 *sourceModule = ext->getModuleContext();
+      ctx.Diags.diagnose(loc, diag::module_api_import_conformance,
+                         rootConf->getType(), rootConf->getProtocol(),
+                         importedVia, sourceModule,
+                         importedVia == sourceModule);
+    }
+  }
+
   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 +373,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);