Sema: Improve MemberImportVisibility diagnostics for cross import overlays.

When `MemberImportVisibility` is enabled and a declaration from a cross import
overlay is diagnosed because it has not been imported, suggest imports of the
declaring and bystanding modules instead of the cross import overlay module
(which is an implementation detail).

Resolves rdar://149307959.

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()
+}