Support module selectors in CSDiagnostics

Author: beccadax

include/swift/Sema/CSFix.h

@@ -185,6 +185,10 @@ enum class FixKind : uint8_t {
   /// no access control.
   AllowInaccessibleMember,
 
+  /// If a module selector prevented us from selecting a member, pretend that it
+  /// was not specified.
+  AllowMemberFromWrongModule,
+
   /// Allow KeyPaths to use AnyObject as root type
   AllowAnyObjectKeyPathRoot,
 
@@ -1519,6 +1523,25 @@ class AllowInaccessibleMember final : public AllowInvalidMemberRef {
                                          ConstraintLocator *locator);
 };
 
+class AllowMemberFromWrongModule final : public AllowInvalidMemberRef {
+  AllowMemberFromWrongModule(ConstraintSystem &cs, Type baseType,
+                             ValueDecl *member, DeclNameRef name,
+                             ConstraintLocator *locator)
+      : AllowInvalidMemberRef(cs, FixKind::AllowMemberFromWrongModule, baseType,
+                              member, name, locator) {}
+
+public:
+  std::string getName() const override {
+    return "allow reference to member from wrong module";
+  }
+
+  bool diagnose(const Solution &solution, bool asNote = false) const override;
+
+  static AllowMemberFromWrongModule *create(ConstraintSystem &cs, Type baseType,
+                                            ValueDecl *member, DeclNameRef name,
+                                            ConstraintLocator *locator);
+};
+
 class AllowAnyObjectKeyPathRoot final : public ConstraintFix {
 
   AllowAnyObjectKeyPathRoot(ConstraintSystem &cs, ConstraintLocator *locator)

include/swift/Sema/ConstraintSystem.h

@@ -1500,6 +1500,9 @@ struct MemberLookupResult {
     /// This is a static member being access through a protocol metatype
     /// but its result type doesn't conform to this protocol.
     UR_InvalidStaticMemberOnProtocolMetatype,
+
+    /// The module selector in the `DeclNameRef` does not match this candidate.
+    UR_WrongModule
   };
 
   /// This is a list of considered (but rejected) candidates, along with a

lib/Sema/CSDiagnostics.cpp

@@ -5316,10 +5316,7 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
 
   if (baseExpr) {
     auto *locator = getConstraintLocator(baseExpr, ConstraintLocator::Member);
-    const auto &solution = getSolution();
-    if (llvm::any_of(solution.Fixes, [&locator](const ConstraintFix *fix) {
-          return fix->getLocator() == locator;
-        }))
+    if (hasFixFor(getSolution(), locator))
       return false;
   }
 
@@ -5339,6 +5336,54 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
   return true;
 }
 
+bool MemberFromWrongModuleFailure::diagnoseAsError() {
+  auto anchor = getRawAnchor();
+  // Let's try to avoid over-diagnosing chains of wrong-module
+  // members e.g.:
+  //
+  // struct A {
+  //   struct B {
+  //     struct C {}
+  //   }
+  // }
+  //
+  // _ = A.Other::B.Other::C()
+  Expr *baseExpr = nullptr;
+  DeclNameLoc nameLoc;
+  if (auto *UDE = getAsExpr<UnresolvedDotExpr>(anchor)) {
+    baseExpr = UDE->getBase();
+    nameLoc = UDE->getNameLoc();
+  } else if (auto *UME = getAsExpr<UnresolvedMemberExpr>(anchor)) {
+    nameLoc = UME->getNameLoc();
+  } else if (auto *SE = getAsExpr<SubscriptExpr>(anchor)) {
+    baseExpr = SE->getBase();
+  } else if (auto *call = getAsExpr<CallExpr>(anchor)) {
+    baseExpr = call->getFn();
+  }
+
+  if (baseExpr) {
+    auto *locator = getConstraintLocator(baseExpr, ConstraintLocator::Member);
+    if (hasFixFor(getSolution(), locator))
+      return false;
+  }
+
+  auto modSelLoc = nameLoc.getModuleSelectorLoc();
+  auto loc = nameLoc.isValid() ? nameLoc.getStartLoc() : ::getLoc(anchor);
+
+  emitDiagnosticAt(loc, diag::decl_not_in_module, Member->getName(),
+                   Name.getModuleSelector());
+
+  Identifier actualModuleName = Member->getModuleContext()->getName();
+  assert(actualModuleName != Name.getModuleSelector() &&
+         "Module selector failure on member in same module?");
+  emitDiagnosticAt(loc, diag::note_change_module_selector, actualModuleName)
+      .fixItReplace(modSelLoc, actualModuleName.str());
+
+  emitDiagnosticAt(Member, diag::decl_declared_here, Member->getName());
+
+  return true;
+}
+
 SourceLoc AnyObjectKeyPathRootFailure::getLoc() const {
   auto anchor = getAnchor();
 

lib/Sema/CSDiagnostics.h

@@ -1537,6 +1537,27 @@ class InaccessibleMemberFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
+/// Diagnose an attempt to reference member from the wrong module with a module
+/// selector, e.g.
+///
+/// ```swift
+/// import Foo
+/// import Bar
+///
+/// SomeType.Bar::methodDefinedInFoo()
+/// ```
+class MemberFromWrongModuleFailure final : public FailureDiagnostic {
+  ValueDecl *Member;
+  DeclNameRef Name;
+
+public:
+  MemberFromWrongModuleFailure(const Solution &solution, DeclNameRef name,
+                               ValueDecl *member, ConstraintLocator *locator)
+      : FailureDiagnostic(solution, locator), Member(member), Name(name) {}
+
+  bool diagnoseAsError() override;
+};
+
 /// Diagnose an attempt to reference member marked as `mutating`
 /// on immutable base e.g. `let` variable:
 ///

lib/Sema/CSFix.cpp

@@ -959,6 +959,21 @@ AllowInaccessibleMember::create(ConstraintSystem &cs, Type baseType,
       AllowInaccessibleMember(cs, baseType, member, name, locator);
 }
 
+bool AllowMemberFromWrongModule::diagnose(const Solution &solution,
+                                          bool asNote) const {
+  MemberFromWrongModuleFailure failure(solution, getMemberName(), getMember(),
+                                       getLocator());
+  return failure.diagnose(asNote);
+}
+
+AllowMemberFromWrongModule *
+AllowMemberFromWrongModule::create(ConstraintSystem &cs, Type baseType,
+                                   ValueDecl *member, DeclNameRef name,
+                                   ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      AllowMemberFromWrongModule(cs, baseType, member, name, locator);
+}
+
 bool AllowAnyObjectKeyPathRoot::diagnose(const Solution &solution,
                                          bool asNote) const {
   AnyObjectKeyPathRootFailure failure(solution, getLocator());

lib/Sema/CSSimplify.cpp

@@ -7719,34 +7719,53 @@ performMemberLookup(ConstraintKind constraintKind, DeclNameRef memberName,
   }
 
   // If we have no viable or unviable candidates, and we're generating,
-  // diagnostics, rerun the query with inaccessible members included, so we can
-  // include them in the unviable candidates list.
+  // diagnostics, rerun the query with various excluded members included, so we
+  // can include them in the unviable candidates list.
   if (result.ViableCandidates.empty() && result.UnviableCandidates.empty() &&
       includeInaccessibleMembers) {
+    auto addExcludedChoices = [&](LookupResult &lookup,
+                                  MemberLookupResult::UnviableReason reason) {
+      for (auto entry : lookup) {
+        auto *cand = entry.getValueDecl();
+
+        // If the result is invalid, skip it.
+        if (cand->isInvalid()) {
+          result.markErrorAlreadyDiagnosed();
+          return false;
+        }
+
+        if (excludedDynamicMembers.count(cand))
+          continue;
+
+        result.addUnviable(getOverloadChoice(cand, /*isBridged=*/false,
+                                             /*isUnwrappedOptional=*/false),
+                           reason);
+      }
+      return true;
+    };
+
+    // Look for members that were excluded because of a module selector.
+    if (memberName.hasModuleSelector()) {
+      DeclNameRef unqualifiedMemberName{memberName.getFullName()};
+
+      NameLookupOptions lookupOptions = defaultMemberLookupOptions;
+      auto lookup =
+          TypeChecker::lookupMember(DC, instanceTy, unqualifiedMemberName,
+                                    lookupOptions);
+      if (!addExcludedChoices(lookup, MemberLookupResult::UR_WrongModule))
+        return result;
+    }
+
+    // Look for members that were excluded by access control.
     NameLookupOptions lookupOptions = defaultMemberLookupOptions;
-    
+
     // Ignore access control so we get candidates that might have been missed
     // before.
     lookupOptions |= NameLookupFlags::IgnoreAccessControl;
 
     auto lookup =
         TypeChecker::lookupMember(DC, instanceTy, memberName, lookupOptions);
-    for (auto entry : lookup) {
-      auto *cand = entry.getValueDecl();
-
-      // If the result is invalid, skip it.
-      if (cand->isInvalid()) {
-        result.markErrorAlreadyDiagnosed();
-        return result;
-      }
-
-      if (excludedDynamicMembers.count(cand))
-        continue;
-
-      result.addUnviable(getOverloadChoice(cand, /*isBridged=*/false,
-                                           /*isUnwrappedOptional=*/false),
-                         MemberLookupResult::UR_Inaccessible);
-    }
+    addExcludedChoices(lookup, MemberLookupResult::UR_Inaccessible);
   }
 
   return result;
@@ -7948,6 +7967,11 @@ fixMemberRef(ConstraintSystem &cs, Type baseTy,
                  : nullptr;
     }
 
+    case MemberLookupResult::UR_WrongModule:
+      assert(choice.isDecl());
+      return AllowMemberFromWrongModule::create(cs, baseTy, choice.getDecl(),
+                                                memberName, locator);
+
     case MemberLookupResult::UR_Inaccessible:
       assert(choice.isDecl());
       return AllowInaccessibleMember::create(cs, baseTy, choice.getDecl(),
@@ -11637,6 +11661,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::AllowInvalidInitRef:
   case FixKind::AllowClosureParameterDestructuring:
   case FixKind::AllowInaccessibleMember:
+  case FixKind::AllowMemberFromWrongModule:
   case FixKind::AllowAnyObjectKeyPathRoot:
   case FixKind::AllowMultiArgFuncKeyPathMismatch:
   case FixKind::TreatKeyPathSubscriptIndexAsHashable:

test/NameLookup/module_selector.swift

@@ -46,6 +46,7 @@ extension ModuleSelectorTestingKit::A: Swift::Equatable {
     }
     else {
       self = ModuleSelectorTestingKit::A(value: .Swift::min)
+      self = A.ModuleSelectorTestingKit::init(value: .min)
     }
 
     self.main::myNegate()
@@ -112,6 +113,9 @@ extension B: main::Equatable {
       self = main::B(value: .main::min)
       // expected-error@-1 {{declaration 'B' is not imported through module 'main'}}
       // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{14-18=ModuleSelectorTestingKit}}
+      self = B.main::init(value: .min)
+      // expected-error@-1 {{declaration 'init(value:)' is not imported through module 'main'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{16-20=ModuleSelectorTestingKit}}
     }
 
     self.main::myNegate()
@@ -146,7 +150,7 @@ extension C: ModuleSelectorTestingKit::Equatable {
 
   @_dynamicReplacement(for: ModuleSelectorTestingKit::negate())
   mutating func myNegate() {
-  // FIXME improve: expected-note@-1 {{did you mean 'myNegate'?}}
+  // expected-note@-1 {{'myNegate()' declared here}}
 
     let fn: (ModuleSelectorTestingKit::Int, ModuleSelectorTestingKit::Int) -> ModuleSelectorTestingKit::Int =
     // expected-error@-1 3{{type 'Int' is not imported through module 'ModuleSelectorTestingKit'}}
@@ -172,11 +176,14 @@ extension C: ModuleSelectorTestingKit::Equatable {
     }
     else {
       self = ModuleSelectorTestingKit::C(value: .ModuleSelectorTestingKit::min)
-      // FIXME improve: expected-error@-1 {{type 'Int' has no member 'ModuleSelectorTestingKit::min'}}
+      // expected-error@-1 {{declaration 'min' is not imported through module 'ModuleSelectorTestingKit'}}
+      // expected-note@-2 {{did you mean module 'Swift'?}} {{50-74=Swift}}
+      self = C.ModuleSelectorTestingKit::init(value: .min)
     }
 
     self.ModuleSelectorTestingKit::myNegate()
-    // FIXME improve: expected-error@-1 {{value of type 'C' has no member 'ModuleSelectorTestingKit::myNegate'}}
+    // expected-error@-1 {{declaration 'myNegate()' is not imported through module 'ModuleSelectorTestingKit'}}
+    // expected-note@-2 {{did you mean module 'main'?}} {{10-34=main}}
   }
 
   // FIXME: Can we test @convention(witness_method:)?
@@ -209,7 +216,7 @@ extension D: Swift::Equatable {
   // expected-error@-1 {{replaced function 'Swift::negate()' could not be found}}
   // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{29-34=ModuleSelectorTestingKit}}
   mutating func myNegate() {
-  // FIXME improve: expected-note@-1 {{did you mean 'myNegate'?}}
+  // expected-note@-1 {{'myNegate()' declared here}}
 
     let fn: (Swift::Int, Swift::Int) -> Swift::Int =
       (Swift::+)
@@ -229,10 +236,14 @@ extension D: Swift::Equatable {
       self = Swift::D(value: .ModuleSelectorTestingKit::min)
       // expected-error@-1 {{declaration 'D' is not imported through module 'Swift'}}
       // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{14-19=ModuleSelectorTestingKit}}
+      self = D.Swift::init(value: .min)
+      // expected-error@-1 {{declaration 'init(value:)' is not imported through module 'Swift'}}
+      // expected-note@-2 {{did you mean module 'ModuleSelectorTestingKit'?}} {{16-21=ModuleSelectorTestingKit}}
     }
 
     self.Swift::myNegate()
-    // FIXME improve: expected-error@-1 {{value of type 'D' has no member 'Swift::myNegate'}}
+    // expected-error@-1 {{declaration 'myNegate()' is not imported through module 'Swift'}}
+    // expected-note@-2 {{did you mean module 'main'?}} {{10-15=main}}
   }
 
   // FIXME: Can we test @convention(witness_method:)?
@@ -463,8 +474,8 @@ func badModuleNames() {
   // FIXME redundant: expected-note@-3  {{did you mean module 'Swift'?}}
 
   _ = "foo".NonexistentModule::count
-  // FIXME improve: expected-error@-1 {{value of type 'String' has no member 'NonexistentModule::count'}}
-  // FIXME: expected-EVENTUALLY-note@-2 {{did you mean module 'Swift'?}} {{13-30=Swift}}
+  // expected-error@-1 {{declaration 'count' is not imported through module 'NonexistentModule'}}
+  // expected-note@-2 {{did you mean module 'Swift'?}} {{13-30=Swift}}
 
   let x: NonexistentModule::MyType = NonexistentModule::MyType()
   // expected-error@-1 {{cannot find type 'NonexistentModule::MyType' in scope}}