Improve expression diagnostics for incorrect module selectors

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -905,8 +905,14 @@ ERROR(cannot_find_type_in_scope_did_you_mean,none,
       "cannot find type %0 in scope; did you mean to use '%1'?", (DeclNameRef, StringRef))
 ERROR(type_not_in_module,none,
       "type %0 is not imported through module %1", (DeclName, Identifier))
+ERROR(decl_not_in_module,none,
+      "declaration %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,none,
+     "did you mean the local declaration?", ())
+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 %1 '%0'?", (StringRef, StringRef))
 NOTE(note_remapped_type,none,

lib/Sema/PreCheckExpr.cpp

@@ -430,7 +430,7 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
   auto &Context = DC->getASTContext();
 
   // First, look for a local binding in scope.
-  if (Loc.isValid() && !Name.isOperator()) {
+  if (Loc.isValid() && !Name.isOperator() && !Name.hasModuleSelector()) {
     SmallVector<ValueDecl *, 2> localDecls;
     ASTScope::lookupLocalDecls(DC->getParentSourceFile(),
                                LookupName.getFullName(), Loc,
@@ -494,6 +494,50 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
       return errorResult();
     }
 
+    // Is there an incorrect module selector?
+    if (Name.hasModuleSelector()) {
+      auto anyModuleName = DeclNameRef(Name.getFullName());
+      auto anyModuleResults = TypeChecker::lookupUnqualified(DC, anyModuleName,
+                                                             Loc,
+                                                             lookupOptions);
+      if (!anyModuleResults.empty()) {
+        Context.Diags.diagnose(UDRE->getNameLoc(), diag::decl_not_in_module,
+                               Name.getFullName(), Name.getModuleSelector());
+
+        SourceLoc moduleSelectorLoc = UDRE->getNameLoc().getModuleSelectorLoc();
+
+        for (auto result : anyModuleResults) {
+          ValueDecl * decl = result.getValueDecl();
+          Identifier moduleName = decl->getModuleContext()->getName();
+
+          if (moduleName != Name.getModuleSelector()) {
+            SmallString<64> replacement;
+            if (decl->isInstanceMember())
+              replacement += "self.";
+            replacement += moduleName.str();
+
+            Context.Diags.diagnose(moduleSelectorLoc,
+                                   diag::note_change_module_selector,
+                                   moduleName)
+                .fixItReplace(moduleSelectorLoc, replacement);
+          } else {
+            // It's just something we need to pick up contextually.
+            if (decl->getDeclContext()->getLocalContext())
+              decl->diagnose(diag::note_remove_module_selector)
+                  .fixItRemove(moduleSelectorLoc);
+
+            if (decl->isInstanceMember())
+              Context.Diags.diagnose(moduleSelectorLoc,
+                  diag::note_add_explicit_self_with_module_selector)
+                  .fixItInsert(moduleSelectorLoc, "self.");
+          }
+        }
+
+        // FIXME: Can we recover by assuming the first/best result is correct?
+        return new (Context) ErrorExpr(UDRE->getSourceRange());
+      }
+    }
+
     // Try ignoring access control.
     NameLookupOptions relookupOptions = lookupOptions;
     relookupOptions |= NameLookupFlags::IgnoreAccessControl;

test/NameLookup/module_selector.swift

@@ -94,14 +94,17 @@ extension B: main::Equatable {
     // FIXME incorrect: expected-error@-3 {{variable used within its own initial value}}
 
     if main::Bool.main::random() {
-    // FIXME improve: expected-error@-1 {{use of unresolved identifier 'main::Bool'}}
+    // expected-error@-1 {{declaration 'Bool' is not imported through module 'main'}}
+    // expected-note@-2 {{did you mean module 'Swift'?}} {{8-12=Swift}}
 
       main::negate()
-      // FIXME improve, suggest adding 'self.': expected-error@-1 {{use of unresolved identifier 'main::negate'}}
+      // expected-error@-1 {{declaration '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 {{use of unresolved identifier 'main::B'}}
+      // expected-error@-1 {{declaration 'B' is not imported through module 'main'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{14-18=ModuleSelectorTestingKit}}
     }
 
     self.main::myNegate()
@@ -152,10 +155,12 @@ extension ModuleSelectorTestingKit::C: ModuleSelectorTestingKit::Equatable {
     // FIXME incorrect: expected-error@-3 {{variable used within its own initial value}}
 
     if ModuleSelectorTestingKit::Bool.ModuleSelectorTestingKit::random() {
-    // FIXME improve: expected-error@-1 {{use of unresolved identifier 'ModuleSelectorTestingKit::Bool'}}
+    // expected-error@-1 {{declaration 'Bool' is not imported through module 'ModuleSelectorTestingKit'}}
+    // expected-note@-2 {{did you mean module 'Swift'?}} {{8-31=Swift}}
 
       ModuleSelectorTestingKit::negate()
-      // FIXME improve, suggest adding 'self.': expected-error@-1 {{use of unresolved identifier 'ModuleSelectorTestingKit::negate'}}
+      // expected-error@-1 {{declaration '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)
@@ -176,7 +181,7 @@ extension Swift::D {}
 // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{11-16=ModuleSelectorTestingKit}}
 
 extension D: Swift::Equatable {
-// FIXME wat: expected-error@-1 2{{implementation of 'Equatable' cannot be automatically synthesized in an extension in a different file to the type}}
+// FIXME wat: expected-error@-1 {{implementation of 'Equatable' cannot be automatically synthesized in an extension in a different file to the type}}
 
   @_implements(Swift::Equatable, Swift::==(_:_:))
   // expected-error@-1 {{name cannot be qualified with module selector here}} {{34-41=}}
@@ -205,11 +210,13 @@ extension D: Swift::Equatable {
     // FIXME: expected-error@-2 {{variable used within its own initial value}}
     if Swift::Bool.Swift::random() {
       Swift::negate()
-      // FIXME improve: expected-error@-1 {{use of unresolved identifier 'Swift::negate'}}
+      // expected-error@-1 {{declaration 'negate' is not imported through module 'Swift'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{7-12=self.ModuleSelectorTestingKit}}
     }
     else {
       self = Swift::D(value: .ModuleSelectorTestingKit::min)
-      // FIXME improve: expected-error@-1 {{use of unresolved identifier 'Swift::D'}}
+      // expected-error@-1 {{declaration 'D' is not imported through module 'Swift'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{14-19=ModuleSelectorTestingKit}}
     }
 
     self.Swift::myNegate()
@@ -314,10 +321,10 @@ func main::decl1(
   // expected-error@-2 {{name of constant declaration cannot be qualified with module selector}}
 
   // From uses in the switch statements below:
-  // expected-note@-5 3{{did you mean 'decl1g'?}}
+  // expected-note@-5 3{{did you mean the local declaration?}}
 
   switch Optional(main::decl1g) {
-  // expected-error@-1 {{use of unresolved identifier 'main::decl1g'}}
+  // expected-error@-1 {{declaration 'decl1g' is not imported through module 'main'}}
   case Optional.some(let main::decl1i):
     // expected-error@-1 {{name of constant declaration cannot be qualified with module selector}}
     break
@@ -326,7 +333,7 @@ func main::decl1(
   }
 
   switch Optional(main::decl1g) {
-  // expected-error@-1 {{use of unresolved identifier 'main::decl1g'}}
+  // expected-error@-1 {{declaration 'decl1g' is not imported through module 'main'}}
   case let Optional.some(main::decl1j):
     // expected-error@-1 {{name of constant declaration cannot be qualified with module selector}}
     break
@@ -335,7 +342,7 @@ func main::decl1(
   }
 
   switch Optional(main::decl1g) {
- // expected-error@-1 {{use of unresolved identifier 'main::decl1g'}}
+ // expected-error@-1 {{declaration 'decl1g' is not imported through module 'main'}}
  case let main::decl1k?:
     // expected-error@-1 {{name of constant declaration cannot be qualified with module selector}}
     break