Improve module selector expr lookup diagnostics

beccadax · beccadax · commit d1e391094dd7 · 2025-10-24T16:23:49.000-07:00
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -1195,6 +1195,12 @@ ERROR(wrong_module_selector,none,
       "%0 is not imported through module %1", (DeclName, Identifier))
 NOTE(note_change_module_selector,none,
      "did you mean module %0?", (Identifier))
+NOTE(note_remove_module_selector_local_decl,none,
+     "did you mean the local declaration?", ())
+NOTE(note_remove_module_selector_outer_type,none,
+     "did you mean an outer type member?", ())
+NOTE(note_add_explicit_self_with_module_selector,none,
+     "did you mean the member of 'self'?", ())
 NOTE(note_typo_candidate_implicit_member,none,
      "did you mean the implicitly-synthesized %kindbase0?", (const ValueDecl *))
 NOTE(note_remapped_type,none,
diff --git a/lib/Sema/PreCheckTarget.cpp b/lib/Sema/PreCheckTarget.cpp
@@ -620,6 +620,18 @@ static Expr *resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC,
   }
 
   if (!Lookup) {
+    // Is there an incorrect module selector?
+    if (Name.hasModuleSelector()) {
+      auto anyModuleName = DeclNameRef(LookupName.getFullName());
+      ModuleSelectorCorrection correction(
+        TypeChecker::lookupUnqualified(DC, anyModuleName, Loc, lookupOptions));
+
+      if (correction.diagnose(Context, UDRE->getNameLoc(), LookupName)) {
+        // FIXME: Can we recover by assuming the first/best result is correct?
+        return errorResult();
+      }
+    }
+
     // For the purpose of diagnosing inaccessible results, try the lookup again
     // but ignore access control.
     NameLookupOptions relookupOptions = lookupOptions;
diff --git a/lib/Sema/TypeCheckNameLookup.cpp b/lib/Sema/TypeCheckNameLookup.cpp
@@ -1160,8 +1160,23 @@ ModuleSelectorCorrection(const LookupResult &candidates) {
   // emit a bunch of duplicates.
   for (auto result : candidates) {
     ValueDecl * decl = result.getValueDecl();
+
+    CandidateKind kind;
+    if (!result.getDeclContext()) {
+      if (decl->getDeclContext()->isLocalContext())
+        kind = CandidateKind::Local;
+      else
+        kind = CandidateKind::ContextFree;
+    } else {
+      if (result.getDeclContext()->isTypeContext())
+        kind = CandidateKind::MemberViaContext;
+      else // found through result.getDeclContext()'s implicit `self`
+        kind = CandidateKind::MemberViaSelf;
+    }
+
     auto owningModule = decl->getModuleContext();
-    candidateModules.insert(owningModule->getName());
+    candidateModules.insert(
+      { owningModule->getName(), kind });
   }
 }
 
@@ -1171,7 +1186,8 @@ ModuleSelectorCorrection(const LookupTypeResult &candidates) {
   // emit a bunch of duplicates.
   for (auto result : candidates) {
     auto owningModule = result.Member->getModuleContext();
-    candidateModules.insert(owningModule->getName());
+    candidateModules.insert(
+      { owningModule->getName(), CandidateKind::ContextFree });
   }
 }
 
@@ -1187,10 +1203,47 @@ bool ModuleSelectorCorrection::diagnose(ASTContext &ctx,
 
   SourceLoc moduleSelectorLoc = nameLoc.getModuleSelectorLoc();
 
-  for (auto moduleName : candidateModules) {
-    ctx.Diags.diagnose(moduleSelectorLoc, diag::note_change_module_selector,
-                       moduleName)
-        .fixItReplace(moduleSelectorLoc, moduleName.str());
+  for (auto pair : candidateModules) {
+    Identifier moduleName = pair.first;
+    switch (pair.second) {
+    case CandidateKind::ContextFree:
+      ctx.Diags.diagnose(moduleSelectorLoc, diag::note_change_module_selector,
+                         moduleName)
+          .fixItReplace(moduleSelectorLoc, moduleName.str());
+      break;
+
+    case CandidateKind::MemberViaSelf: {
+      SmallString<64> replacement("self.");
+      if (moduleName != originalName.getModuleSelector()) {
+        // The module selector specified the wrong module; replace it.
+        replacement += moduleName.str();
+
+        ctx.Diags.diagnose(moduleSelectorLoc, diag::note_change_module_selector,
+                           moduleName)
+            .fixItReplace(moduleSelectorLoc, replacement);
+      } else {
+        // The module selector specified the right module, but we need to
+        // make the `self.` explicit.
+        ctx.Diags.diagnose(moduleSelectorLoc,
+                           diag::note_add_explicit_self_with_module_selector)
+            .fixItInsert(moduleSelectorLoc, replacement);
+      }
+      break;
+    }
+    case CandidateKind::MemberViaContext:
+      // FIXME: If we had more info here, we could construct a reference
+      //        to the outer type in question.
+      ctx.Diags.diagnose(moduleSelectorLoc,
+                         diag::note_remove_module_selector_outer_type)
+          .fixItRemoveChars(moduleSelectorLoc, nameLoc.getBaseNameLoc());
+      break;
+
+    case CandidateKind::Local:
+      ctx.Diags.diagnose(moduleSelectorLoc,
+                         diag::note_remove_module_selector_local_decl)
+          .fixItRemoveChars(moduleSelectorLoc, nameLoc.getBaseNameLoc());
+      break;
+    }
   }
 
   return true;
diff --git a/lib/Sema/TypeChecker.h b/lib/Sema/TypeChecker.h
@@ -312,7 +312,20 @@ class CheckGenericArgumentsResult {
 /// there are any results, the \c diagnose() method will emit an error and a
 /// set of fix-it notes.
 class ModuleSelectorCorrection {
-  using CandidateModule = Identifier;
+  enum class CandidateKind {
+    /// A declaration that is found without depending on context.
+    /// Usually either a top-level type or a member with an explicit base type.
+    ContextFree,
+    /// A member declaration accessed via implicit \c self .
+    MemberViaSelf,
+    /// A member declaration not accessed via implicit \c self (usually a static
+    /// member of an enclosing type).
+    MemberViaContext,
+    /// A local declaration.
+    Local
+  };
+
+  using CandidateModule = std::pair<Identifier, CandidateKind>;
   SmallSetVector<CandidateModule, 4> candidateModules;
 
 public:
diff --git a/test/NameLookup/module_selector.swift b/test/NameLookup/module_selector.swift
@@ -87,7 +87,8 @@ extension B: @retroactive main::Equatable {
     // expected-error@-4 {{'Bool' is not imported through module 'main'}}
     // expected-note@-5 {{did you mean module 'Swift'?}} {{56-60=Swift}}
     main::fatalError()
-    // FIXME improve: expected-error@-1 {{cannot find 'main::fatalError' in scope}}
+    // expected-error@-1 {{'fatalError' is not imported through module 'main'}}
+    // expected-note@-2 {{did you mean module 'Swift'?}} {{5-9=Swift}}
   }
 
   // FIXME: Add tests with autodiff @_differentiable(jvp:vjp:) and
@@ -103,7 +104,9 @@ extension B: @retroactive main::Equatable {
     // expected-note@-3 {{did you mean module 'Swift'?}} {{25-29=Swift}}
     // expected-note@-4 {{did you mean module 'Swift'?}} {{39-43=Swift}}
       (main::+)
-      // FIXME improve: expected-error@-1 {{cannot find operator 'main::+' in scope}}
+      // expected-error@-1 {{'+' is not imported through module 'main'}}
+      // expected-note@-2 {{did you mean module 'Swift'?}} {{8-12=Swift}}
+      // expected-note@-3 {{did you mean module '_Concurrency'?}} {{8-12=_Concurrency}} FIXME: Accept and suggest 'Swift::' instead?
 
     let magnitude: Int.main::Magnitude = main::magnitude
     // expected-error@-1 {{'Magnitude' is not imported through module 'main'}}
@@ -112,15 +115,18 @@ extension B: @retroactive main::Equatable {
     _ = (fn, magnitude)
 
     if main::Bool.main::random() {
-      // FIXME improve: expected-error@-1 {{cannot find 'main::Bool' in scope}}
+      // expected-error@-1 {{'Bool' is not imported through module 'main'}}
+      // expected-note@-2 {{did you mean module 'Swift'?}} {{8-12=Swift}}
 
       main::negate()
-      // FIXME improve: expected-error@-1 {{cannot find 'main::negate' in scope}}
+      // expected-error@-1 {{'negate' is not imported through module 'main'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{7-11=self.ModuleSelectorTestingKit}}
     }
     else {
       self = main::B(value: .main::min)
-      // FIXME improve: expected-error@-1 {{cannot find 'main::B' in scope}}
-      // expected-error@-2 {{cannot infer contextual base in reference to member 'main::min'}}
+      // expected-error@-1 {{'B' is not imported through module 'main'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{14-18=ModuleSelectorTestingKit}}
+      // expected-error@-3 {{cannot infer contextual base in reference to member 'main::min'}}
 
       self = B.main::init(value: .min)
       // FIXME improve: expected-error@-1 {{'B' cannot be constructed because it has no accessible initializers}}
@@ -130,13 +136,15 @@ extension B: @retroactive main::Equatable {
     self.main::myNegate()
 
     main::fatalError()
-    // FIXME improve: expected-error@-1 {{cannot find 'main::fatalError' in scope}}
+    // expected-error@-1 {{'fatalError' is not imported through module 'main'}}
+    // expected-note@-2 {{did you mean module 'Swift'?}} {{5-9=Swift}}
 
     _ = \main::A.magnitude
     // expected-error@-1 {{'A' is not imported through module 'main'}}
     // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{10-14=ModuleSelectorTestingKit}}
     _ = \A.main::magnitude
-    // FIXME improve: expected-error@-1 {{value of type 'A' has no member 'main::magnitude'}}
+    // expected-error@-1 {{'magnitude' is not imported through module 'main'}}
+    // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{none}}
 
     _ = #main::ExprMacro
     // expected-error@-1 {{no macro named 'main::ExprMacro'}}
@@ -165,7 +173,8 @@ extension C: @retroactive ModuleSelectorTestingKit::Equatable {
     // expected-note@-2 {{did you mean module 'Swift'?}} {{96-120=Swift}}
 
     ModuleSelectorTestingKit::fatalError()
-    // FIXME improve: expected-error@-1 {{cannot find 'ModuleSelectorTestingKit::fatalError' in scope}}
+    // expected-error@-1 {{'fatalError' is not imported through module 'ModuleSelectorTestingKit'}}
+    // expected-note@-2 {{did you mean module 'Swift'?}} {{5-29=Swift}}
   }
 
   // FIXME: Add tests with autodiff @_differentiable(jvp:vjp:) and
@@ -174,26 +183,33 @@ extension C: @retroactive ModuleSelectorTestingKit::Equatable {
   @_dynamicReplacement(for: ModuleSelectorTestingKit::negate())
 
   mutating func myNegate() {
+  // expected-note@-1 {{did you mean 'myNegate'?}}
+
     let fn: (ModuleSelectorTestingKit::Int, ModuleSelectorTestingKit::Int) -> ModuleSelectorTestingKit::Int =
     // expected-error@-1 3{{'Int' is not imported through module 'ModuleSelectorTestingKit'}}
     // expected-note@-2 {{did you mean module 'Swift'?}} {{14-38=Swift}}
     // expected-note@-3 {{did you mean module 'Swift'?}} {{45-69=Swift}}
     // expected-note@-4 {{did you mean module 'Swift'?}} {{79-103=Swift}}
       (ModuleSelectorTestingKit::+)
-      // FIXME improve: expected-error@-1 {{cannot find operator 'ModuleSelectorTestingKit::+' in scope}}
+      // expected-error@-1 {{'+' is not imported through module 'ModuleSelectorTestingKit'}}
+      // expected-note@-2 {{did you mean module 'Swift'?}} {{8-32=Swift}}
+      // expected-note@-3 {{did you mean module '_Concurrency'?}} {{8-32=_Concurrency}} FIXME: Accept and suggest 'Swift::' instead?
 
     let magnitude: Int.ModuleSelectorTestingKit::Magnitude = ModuleSelectorTestingKit::magnitude
     // expected-error@-1 {{'Magnitude' is not imported through module 'ModuleSelectorTestingKit'}}
     // expected-note@-2 {{did you mean module 'Swift'?}} {{24-48=Swift}}
-    // FIXME improve: expected-error@-3 {{cannot find 'ModuleSelectorTestingKit::magnitude' in scope}}
+    // expected-error@-3 {{'magnitude' is not imported through module 'ModuleSelectorTestingKit'}}
+    // expected-note@-4 {{did you mean the local declaration?}} {{62-88=}}
 
     _ = (fn, magnitude)
 
     if ModuleSelectorTestingKit::Bool.ModuleSelectorTestingKit::random() {
-    // FIXME improve: expected-error@-1 {{cannot find 'ModuleSelectorTestingKit::Bool' in scope}}
+    // expected-error@-1 {{'Bool' is not imported through module 'ModuleSelectorTestingKit'}}
+    // expected-note@-2 {{did you mean module 'Swift'?}} {{8-32=Swift}}
 
       ModuleSelectorTestingKit::negate()
-      // expected-error@-1 {{cannot find 'ModuleSelectorTestingKit::negate' in scope}}
+      // expected-error@-1 {{'negate' is not imported through module 'ModuleSelectorTestingKit'}}
+      // expected-note@-2 {{did you mean the member of 'self'?}} {{7-7=self.}}
     }
     else {
       self = ModuleSelectorTestingKit::C(value: .ModuleSelectorTestingKit::min)
@@ -206,7 +222,8 @@ extension C: @retroactive ModuleSelectorTestingKit::Equatable {
     // FIXME improve: expected-error@-1 {{value of type 'C' has no member 'ModuleSelectorTestingKit::myNegate'}}
 
     ModuleSelectorTestingKit::fatalError()
-    // FIXME improve: expected-error@-1 {{cannot find 'ModuleSelectorTestingKit::fatalError' in scope}}
+    // expected-error@-1 {{'fatalError' is not imported through module 'ModuleSelectorTestingKit'}}
+    // expected-note@-2 {{did you mean module 'Swift'?}} {{5-29=Swift}}
 
     _ = \ModuleSelectorTestingKit::A.magnitude
     _ = \A.ModuleSelectorTestingKit::magnitude
@@ -244,24 +261,28 @@ extension D: @retroactive Swift::Equatable {
   // FIXME improve: expected-error@-1 {{replaced function 'Swift::negate()' could not be found}}
 
   mutating func myNegate() {
+    // expected-note@-1 {{did you mean 'myNegate'?}}
 
     let fn: (Swift::Int, Swift::Int) -> Swift::Int =
       (Swift::+)
 
     let magnitude: Int.Swift::Magnitude = Swift::magnitude
-    // expected-error@-1 {{cannot find 'Swift::magnitude' in scope}}
+    // expected-error@-1 {{'magnitude' is not imported through module 'Swift'}}
+    // expected-note@-2 {{did you mean the local declaration?}} {{43-50=}}
 
     _ = (fn, magnitude)
 
     if Swift::Bool.Swift::random() {
 
       Swift::negate()
-      // FIXME improve: expected-error@-1 {{cannot find 'Swift::negate' in scope}}
+      // expected-error@-1 {{'negate' is not imported through module 'Swift'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{7-12=self.ModuleSelectorTestingKit}}
     }
     else {
       self = Swift::D(value: .Swift::min)
-      // FIXME improve: expected-error@-1 {{cannot find 'Swift::D' in scope}}
-      // expected-error@-2 {{cannot infer contextual base in reference to member 'Swift::min'}}
+      // expected-error@-1 {{'D' is not imported through module 'Swift'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{14-19=ModuleSelectorTestingKit}}
+      // expected-error@-3 {{cannot infer contextual base in reference to member 'Swift::min'}}
 
       self = D.Swift::init(value: .min)
       // FIXME improve: expected-error@-1 {{'D' cannot be constructed because it has no accessible initializers}}
@@ -277,7 +298,8 @@ extension D: @retroactive Swift::Equatable {
     // expected-error@-1 {{'A' is not imported through module 'Swift'}}
     // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{10-15=ModuleSelectorTestingKit}}
     _ = \A.Swift::magnitude
-    // FIXME improve: expected-error@-1 {{value of type 'A' has no member 'Swift::magnitude'}}
+    // expected-error@-1 {{'magnitude' is not imported through module 'Swift'}}
+    // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{none}}
 
     _ = #Swift::ExprMacro
     // expected-error@-1 {{no macro named 'Swift::ExprMacro'}}
@@ -293,7 +315,8 @@ let mog: Never = fatalError()
 
 func localVarsCantBeAccessedByModuleSelector() {
   let mag: Int.Swift::Magnitude = main::mag
-  // expected-error@-1 {{cannot find 'main::mag' in scope}}
+  // expected-error@-1 {{'mag' is not imported through module 'main'}}
+  // expected-note@-2 {{did you mean the local declaration?}} {{35-41=}}
 
   let mog: Never = main::mog
 }
@@ -332,23 +355,26 @@ func decl1(
   label p3: @escaping () -> A
 ) {
   switch Optional(main::p2) {
-  // expected-error@-1 {{cannot find 'main::p2' in scope}}
+  // expected-error@-1 {{'p2' is not imported through module 'main'}}
+  // expected-note@-2 {{did you mean the local declaration?}} {{19-25=}}
   case Optional.some(let decl1i):
     break
   case .none:
     break
   }
 
   switch Optional(main::p2) {
-  // expected-error@-1 {{cannot find 'main::p2' in scope}}
+  // expected-error@-1 {{'p2' is not imported through module 'main'}}
+  // expected-note@-2 {{did you mean the local declaration?}} {{19-25=}}
   case let Optional.some(decl1j):
     break
   case .none:
     break
   }
 
   switch Optional(main::p2) {
-  // expected-error@-1 {{cannot find 'main::p2' in scope}}
+  // expected-error@-1 {{'p2' is not imported through module 'main'}}
+  // expected-note@-2 {{did you mean the local declaration?}} {{19-25=}}
  case let decl1k?:
     break
   case .none:
@@ -362,7 +388,8 @@ typealias decl5 = main::Bool
 
 func badModuleNames() {
   NonexistentModule::print()
-  // expected-error@-1 {{cannot find 'NonexistentModule::print' in scope}}
+  // expected-error@-1 {{'print' is not imported through module 'NonexistentModule'}}
+  // expected-note@-2 {{did you mean module 'Swift'?}} {{3-20=Swift}}
 
   _ = "foo".NonexistentModule::count
   // FIXME improve: expected-error@-1 {{value of type 'String' has no member 'NonexistentModule::count'}}
