Merge pull request swiftlang#80972 from tshortli/member-import-visibility-cross-import-overlay

Sema: Improve MemberImportVisibility diagnostics for cross import overlays

Author: tshortli

include/swift/AST/DiagnosticsSema.def

@@ -169,9 +169,13 @@ ERROR(init_candidate_inaccessible,none,
 
 ERROR(candidate_from_missing_import,none,
       "%kind0 is not available due to missing import of defining module %1",
-      (const ValueDecl *, ModuleDecl *))
+      (const ValueDecl *, const ModuleDecl *))
+ERROR(candidate_from_missing_imports_2_or_more,none,
+      "%kind0 is not available due to missing imports of defining modules "
+      "%2%select{ and|, }1 %3%select{|, and others}1",
+      (const ValueDecl *, bool, const ModuleDecl *, const ModuleDecl *))
 NOTE(candidate_add_import,none,
-     "add import of module %0", (ModuleDecl *))
+     "add import of module %0", (const ModuleDecl *))
 
 ERROR(cannot_pass_rvalue_mutating_subelement,none,
       "cannot use mutating member on immutable value: %0",
@@ -6049,7 +6053,7 @@ ERROR(actor_isolation_multiple_attr_2,none,
       "%kind0 has multiple actor-isolation attributes (%1 and %2)",
       (const Decl *, DeclAttribute, DeclAttribute))
 ERROR(actor_isolation_multiple_attr_3,none,
-      "%0 %1 has multiple actor-isolation attributes (%2, %3 and %4)",
+      "%0 %1 has multiple actor-isolation attributes (%2, %3, and %4)",
       (const Decl *, DeclAttribute, DeclAttribute, DeclAttribute))
 ERROR(actor_isolation_multiple_attr_4,none,
       "%0 %1 has multiple actor-isolation attributes (%2, %3, %4, and %5)",

lib/Sema/TypeCheckNameLookup.cpp

@@ -21,6 +21,7 @@
 #include "TypoCorrection.h"
 #include "swift/AST/ConformanceLookup.h"
 #include "swift/AST/ExistentialLayout.h"
+#include "swift/AST/ImportCache.h"
 #include "swift/AST/Initializer.h"
 #include "swift/AST/NameLookup.h"
 #include "swift/AST/NameLookupRequests.h"
@@ -798,7 +799,50 @@ TypoCorrectionResults::claimUniqueCorrection() {
   return SyntacticTypoCorrection(WrittenName, Loc, uniqueCorrectedName);
 }
 
+/// Returns a sorted vector of modules that are not imported in the given
+/// `SourceFile` and must be in order to make declarations from \p owningModule
+/// visible.
+static SmallVector<ModuleDecl *, 2>
+missingImportsForDefiningModule(ModuleDecl *owningModule, SourceFile &sf) {
+  SmallVector<ModuleDecl *, 2> result;
+  auto &ctx = sf.getASTContext();
+
+  if (auto *declaringModule =
+          owningModule->getDeclaringModuleIfCrossImportOverlay()) {
+    // If the module that owns the declaration is a cross import overlay the
+    // fix-its should suggest importing the declaring and bystanding modules,
+    // not the overlay module.
+    result.push_back(declaringModule);
+
+    SmallVector<Identifier, 2> bystanders;
+    if (owningModule->getRequiredBystandersIfCrossImportOverlay(declaringModule,
+                                                                bystanders)) {
+      for (auto bystander : bystanders) {
+        if (auto bystanderModule = ctx.getModuleByIdentifier(bystander))
+          result.push_back(bystanderModule);
+      }
+    }
+
+    // Remove the modules that are already imported by the source file.
+    auto &importCache = ctx.getImportCache();
+    const DeclContext *dc = &sf;
+    llvm::erase_if(result, [&](ModuleDecl *candidate) {
+      return importCache.isImportedBy(candidate, dc);
+    });
+  } else {
+    // Just the module that owns the declaration is required.
+    result.push_back(owningModule);
+  }
+
+  std::sort(result.begin(), result.end(), [](ModuleDecl *LHS, ModuleDecl *RHS) {
+    return LHS->getNameStr() < LHS->getNameStr();
+  });
+
+  return result;
+}
+
 struct MissingImportFixItInfo {
+  const ModuleDecl *moduleToImport = nullptr;
   OptionSet<ImportFlags> flags;
   std::optional<AccessLevel> accessLevel;
 };
@@ -807,15 +851,13 @@ class MissingImportFixItCache {
   SourceFile &sf;
   llvm::DenseMap<const ModuleDecl *, MissingImportFixItInfo> infos;
 
-public:
-  MissingImportFixItCache(SourceFile &sf) : sf(sf){};
-
-  MissingImportFixItInfo getInfo(const ModuleDecl *mod) {
+  MissingImportFixItInfo getFixItInfo(ModuleDecl *mod) {
     auto existing = infos.find(mod);
     if (existing != infos.end())
       return existing->getSecond();
 
     MissingImportFixItInfo info;
+    info.moduleToImport = mod;
 
     // Find imports of the defining module in other source files and aggregate
     // the attributes and access level usage on those imports collectively. This
@@ -845,33 +887,49 @@ class MissingImportFixItCache {
     infos[mod] = info;
     return info;
   }
-};
 
-static void diagnoseMissingImportForMember(const ValueDecl *decl,
-                                           SourceFile *sf, SourceLoc loc) {
-  auto &ctx = sf->getASTContext();
-  auto definingModule = decl->getModuleContextForNameLookup();
-  ctx.Diags.diagnose(loc, diag::candidate_from_missing_import, decl,
-                     definingModule);
-}
+public:
+  MissingImportFixItCache(SourceFile &sf) : sf(sf) {};
 
-static void
-diagnoseAndFixMissingImportForMember(const ValueDecl *decl, SourceFile *sf,
-                                     SourceLoc loc,
-                                     MissingImportFixItCache &fixItCache) {
+  std::pair<SmallVector<ModuleDecl *, 2>,
+            SmallVector<MissingImportFixItInfo, 2>>
+  getModulesAndFixIts(ModuleDecl *mod) {
+    auto modulesToImport = missingImportsForDefiningModule(mod, sf);
+    SmallVector<MissingImportFixItInfo, 2> fixItInfos;
 
-  diagnoseMissingImportForMember(decl, sf, loc);
+    for (auto *mod : modulesToImport) {
+      fixItInfos.emplace_back(getFixItInfo(mod));
+    }
 
+    return {modulesToImport, fixItInfos};
+  }
+};
+
+static void
+diagnoseMissingImportsForMember(const ValueDecl *decl,
+                                SmallVectorImpl<ModuleDecl *> &modulesToImport,
+                                SourceFile *sf, SourceLoc loc) {
   auto &ctx = sf->getASTContext();
-  auto definingModule = decl->getModuleContextForNameLookup();
-  SourceLoc bestLoc = ctx.Diags.getBestAddImportFixItLoc(decl, sf);
-  if (!bestLoc.isValid())
-    return;
+  auto count = modulesToImport.size();
+  ASSERT(count > 0);
+
+  if (count > 1) {
+    ctx.Diags.diagnose(loc, diag::candidate_from_missing_imports_2_or_more,
+                       decl, bool(count > 2), modulesToImport[0],
+                       modulesToImport[1]);
+  } else {
+    ctx.Diags.diagnose(loc, diag::candidate_from_missing_import, decl,
+                       modulesToImport.front());
+  }
+}
 
+static void emitMissingImportFixIt(SourceLoc loc,
+                                   const MissingImportFixItInfo &fixItInfo,
+                                   const ValueDecl *decl) {
+  ASTContext &ctx = decl->getASTContext();
   llvm::SmallString<64> importText;
 
   // Add flags that must be used consistently on every import in every file.
-  auto fixItInfo = fixItCache.getInfo(definingModule);
   if (fixItInfo.flags.contains(ImportFlags::ImplementationOnly))
     importText += "@_implementationOnly ";
   if (fixItInfo.flags.contains(ImportFlags::WeakLinked))
@@ -905,10 +963,36 @@ diagnoseAndFixMissingImportForMember(const ValueDecl *decl, SourceFile *sf,
   }
 
   importText += "import ";
-  importText += definingModule->getName().str();
+  importText += fixItInfo.moduleToImport->getName().str();
   importText += "\n";
-  ctx.Diags.diagnose(bestLoc, diag::candidate_add_import, definingModule)
-      .fixItInsert(bestLoc, importText);
+  ctx.Diags
+      .diagnose(loc, diag::candidate_add_import, fixItInfo.moduleToImport)
+      .fixItInsert(loc, importText);
+}
+
+static void
+diagnoseAndFixMissingImportForMember(const ValueDecl *decl, SourceFile *sf,
+                                     SourceLoc loc,
+                                     MissingImportFixItCache &fixItCache) {
+
+  auto modulesAndFixits =
+      fixItCache.getModulesAndFixIts(decl->getModuleContextForNameLookup());
+  auto modulesToImport = modulesAndFixits.first;
+  auto fixItInfos = modulesAndFixits.second;
+
+  if (modulesToImport.empty())
+    return;
+
+  diagnoseMissingImportsForMember(decl, modulesToImport, sf, loc);
+
+  auto &ctx = sf->getASTContext();
+  SourceLoc bestLoc = ctx.Diags.getBestAddImportFixItLoc(decl, sf);
+  if (!bestLoc.isValid())
+    return;
+
+  for (auto &fixItInfo : fixItInfos) {
+    emitMissingImportFixIt(bestLoc, fixItInfo, decl);
+  }
 }
 
 bool swift::maybeDiagnoseMissingImportForMember(const ValueDecl *decl,
@@ -917,12 +1001,13 @@ bool swift::maybeDiagnoseMissingImportForMember(const ValueDecl *decl,
   if (dc->isDeclImported(decl))
     return false;
 
+  auto definingModule = decl->getModuleContextForNameLookup();
   if (dc->getASTContext().LangOpts.EnableCXXInterop) {
     // With Cxx interop enabled, there are some declarations that always belong
     // to the Clang header import module which should always be implicitly
     // visible. However, that module is not implicitly imported in source files
     // so we need to special case it here and avoid diagnosing.
-    if (decl->getModuleContextForNameLookup()->isClangHeaderImportModule())
+    if (definingModule->isClangHeaderImportModule())
       return false;
   }
 
@@ -935,7 +1020,11 @@ bool swift::maybeDiagnoseMissingImportForMember(const ValueDecl *decl,
   // In lazy typechecking mode just emit the diagnostic immediately without a
   // fix-it since there won't be an opportunity to emit delayed diagnostics.
   if (ctx.TypeCheckerOpts.EnableLazyTypecheck) {
-    diagnoseMissingImportForMember(decl, sf, loc);
+    auto modulesToImport = missingImportsForDefiningModule(definingModule, *sf);
+    if (modulesToImport.empty())
+      return false;
+
+    diagnoseMissingImportsForMember(decl, modulesToImport, sf, loc);
     return true;
   }
 

test/CrossImport/Inputs/lib-templates/lib/swift/BystandingLibrary.swiftinterface

@@ -3,5 +3,7 @@
 
 import Swift
 
-public struct BystandingLibraryTy {}
+public struct BystandingLibraryTy {
+  public init()
+}
 

test/CrossImport/Inputs/lib-templates/lib/swift/DeclaringLibrary.swiftinterface

@@ -3,5 +3,7 @@
 
 import Swift
 
-public struct DeclaringLibraryTy {}
+public struct DeclaringLibraryTy {
+  public init()
+}
 public struct ShadowTy {}

test/CrossImport/Inputs/lib-templates/lib/swift/_OverlayLibrary.swiftinterface

@@ -11,3 +11,11 @@ public struct OverlayLibraryTy {
 }
 
 public struct ShadowTy {}
+
+extension DeclaringLibrary.DeclaringLibraryTy {
+  public func overlayMember()
+}
+
+extension BystandingLibrary.BystandingLibraryTy {
+  public func overlayMember()
+}

test/CrossImport/member-import-visibility.swift

@@ -0,0 +1,57 @@
+// RUN: %empty-directory(%t)
+// RUN: cp -r %S/Inputs/lib-templates/* %t/
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend -typecheck -verify -enable-cross-import-overlays \
+// RUN:   %t/OnlyDeclaring.swift \
+// RUN:   %t/OnlyBystanding.swift \
+// RUN:   %t/NeitherDeclaringNorBystanding.swift \
+// RUN:   %t/BothDeclaringAndBystanding.swift \
+// RUN:   -I %t/include -I %t/lib/swift -F %t/Frameworks \
+// RUN:   -enable-upcoming-feature MemberImportVisibility
+
+// REQUIRES: swift_feature_MemberImportVisibility
+
+//--- OnlyDeclaring.swift
+
+import DeclaringLibrary
+// expected-note 2 {{add import of module 'BystandingLibrary'}}
+
+private func test() {
+  returnsDeclaringTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing import of defining module 'BystandingLibrary'}}
+  returnsBystandingTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing import of defining module 'BystandingLibrary'}}
+}
+
+//--- OnlyBystanding.swift
+
+import BystandingLibrary
+// expected-note 2 {{add import of module 'DeclaringLibrary'}}
+
+private func test() {
+  returnsDeclaringTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing import of defining module 'DeclaringLibrary'}}
+  returnsBystandingTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing import of defining module 'DeclaringLibrary'}}
+}
+
+//--- NeitherDeclaringNorBystanding.swift
+
+import Swift
+// expected-note 2 {{add import of module 'BystandingLibrary'}}
+// expected-note@-1 2 {{add import of module 'DeclaringLibrary'}}
+
+private func test() {
+  returnsDeclaringTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing imports of defining modules 'DeclaringLibrary' and 'BystandingLibrary'}}
+  returnsBystandingTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing imports of defining modules 'DeclaringLibrary' and 'BystandingLibrary'}}
+}
+
+//--- BothDeclaringAndBystanding.swift
+
+import DeclaringLibrary
+import BystandingLibrary
+
+func returnsDeclaringTy() -> DeclaringLibraryTy {
+  return DeclaringLibraryTy()
+}
+
+func returnsBystandingTy() -> BystandingLibraryTy {
+  return BystandingLibraryTy()
+}