Merge pull request swiftlang#23312 from xedin/form-complete-member-overload-sets

[ConstraintSystem] Include unviable-by-fixable choices into member overload sets

Author: xedin

lib/Sema/CSDiag.cpp

@@ -791,15 +791,19 @@ void FailureDiagnosis::diagnoseUnviableLookupResults(
   // Otherwise, we have at least one (and potentially many) viable candidates
   // sort them out.  If all of the candidates have the same problem (commonly
   // because there is exactly one candidate!) diagnose this.
-  bool sameProblem = true;
-  auto firstProblem = result.UnviableCandidates[0].second;
+  auto firstProblem = result.UnviableReasons[0];
+  bool sameProblem = llvm::all_of(
+      result.UnviableReasons,
+      [&firstProblem](const MemberLookupResult::UnviableReason &problem) {
+        return problem == firstProblem;
+      });
+
   ValueDecl *member = nullptr;
   for (auto cand : result.UnviableCandidates) {
     if (member == nullptr)
-      member = cand.first.getDecl();
-    sameProblem &= cand.second == firstProblem;
+      member = cand.getDecl();
   }
-  
+
   auto instanceTy = baseObjTy;
   if (auto *MTT = instanceTy->getAs<AnyMetatypeType>())
     instanceTy = MTT->getInstanceType();
@@ -842,7 +846,7 @@ void FailureDiagnosis::diagnoseUnviableLookupResults(
     }
 
     case MemberLookupResult::UR_Inaccessible: {
-      auto decl = result.UnviableCandidates[0].first.getDecl();
+      auto decl = result.UnviableCandidates[0].getDecl();
       // FIXME: What if the unviable candidates have different levels of access?
       //
       // If we found an inaccessible member of a protocol extension, it might
@@ -853,8 +857,8 @@ void FailureDiagnosis::diagnoseUnviableLookupResults(
       diagnose(nameLoc, diag::candidate_inaccessible, decl->getBaseName(),
                decl->getFormalAccessScope().accessLevelForDiagnostics());
       for (auto cand : result.UnviableCandidates)
-        diagnose(cand.first.getDecl(), diag::decl_declared_here, memberName);
-        
+        diagnose(cand.getDecl(), diag::decl_declared_here, memberName);
+
       return;
     }
     }
@@ -4206,7 +4210,7 @@ bool FailureDiagnosis::diagnoseMethodAttributeFailures(
 
   SmallVector<OverloadChoice, 2> choices;
   for (auto &unviable : results.UnviableCandidates)
-    choices.push_back(OverloadChoice(baseType, unviable.first.getDecl(),
+    choices.push_back(OverloadChoice(baseType, unviable.getDecl(),
                                      UDE->getFunctionRefKind()));
 
   CalleeCandidateInfo unviableCandidates(baseType, choices, hasTrailingClosure,

lib/Sema/CSGen.cpp

@@ -3749,7 +3749,7 @@ swift::resolveValueMember(DeclContext &DC, Type BaseTy, DeclName Name) {
 
   // Keep track of all the unviable members.
   for (auto Can : LookupResult.UnviableCandidates)
-    Result.Impl.AllDecls.push_back(Can.first.getDecl());
+    Result.Impl.AllDecls.push_back(Can.getDecl());
 
   // Keep track of the start of viable choices.
   Result.Impl.ViableStartIdx = Result.Impl.AllDecls.size();

lib/Sema/CSSimplify.cpp

@@ -3753,8 +3753,15 @@ performMemberLookup(ConstraintKind constraintKind, DeclName memberName,
     if (decl->isInstanceMember()) {
       if ((isa<FuncDecl>(decl) && !hasInstanceMethods) ||
           (!isa<FuncDecl>(decl) && !hasInstanceMembers)) {
-        result.addUnviable(candidate,
-                           MemberLookupResult::UR_InstanceMemberOnType);
+        // `AnyObject` has special semantics, so let's just let it be.
+        // Otherwise adjust base type and reference kind to make it
+        // look as if lookup was done on the instance, that helps
+        // with diagnostics.
+        auto choice = instanceTy->isAnyObject()
+                          ? candidate
+                          : OverloadChoice(instanceTy, decl,
+                                           FunctionRefKind::SingleApply);
+        result.addUnviable(choice, MemberLookupResult::UR_InstanceMemberOnType);
         return;
       }
 
@@ -3926,10 +3933,10 @@ performMemberLookup(ConstraintKind constraintKind, DeclName memberName,
           result.addViable(
             OverloadChoice::getDynamicMemberLookup(baseTy, decl, name));
       }
-      for (auto candidate : subscripts.UnviableCandidates) {
-        auto decl = candidate.first.getDecl();
+      for (auto index : indices(subscripts.UnviableCandidates)) {
+        auto decl = subscripts.UnviableCandidates[index].getDecl();
         auto choice = OverloadChoice::getDynamicMemberLookup(baseTy, decl,name);
-        result.addUnviable(choice, candidate.second);
+        result.addUnviable(choice, subscripts.UnviableReasons[index]);
       }
     }
   }
@@ -4203,9 +4210,9 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
     break;
   }
 
+  SmallVector<Constraint *, 4> candidates;
   // If we found viable candidates, then we're done!
   if (!result.ViableCandidates.empty()) {
-    llvm::SmallVector<Constraint *, 8> candidates;
     generateConstraints(
         candidates, memberTy, result.ViableCandidates, useDC, locator,
         result.getFavoredIndex(), /*requiresFix=*/false,
@@ -4222,11 +4229,24 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
       generateConstraints(candidates, memberTy, outerAlternatives,
                           useDC, locator);
     }
+  }
+
+  if (!result.UnviableCandidates.empty()) {
+    // Generate constraints for unvailable choices if they have a fix,
+    // and disable them by default, they'd get picked up in the "salvage" mode.
+    generateConstraints(
+        candidates, memberTy, result.UnviableCandidates, useDC, locator,
+        /*favoredChoice=*/None, /*requiresFix=*/true,
+        [&](unsigned idx, const OverloadChoice &choice) {
+          return fixMemberRef(*this, baseTy, member, choice, locator,
+                              result.UnviableReasons[idx]);
+        });
+  }
 
+  if (!candidates.empty()) {
     addOverloadSet(candidates, locator);
     return SolutionKind::Solved;
   }
-  
 
   // If the lookup found no hits at all (either viable or unviable), diagnose it
   // as such and try to recover in various ways.
@@ -4238,30 +4258,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
     if (!MissingMembers.insert(locator))
       return SolutionKind::Error;
 
-    if (!result.UnviableCandidates.empty()) {
-      SmallVector<OverloadChoice, 8> choices;
-      llvm::transform(
-          result.UnviableCandidates, std::back_inserter(choices),
-          [](const std::pair<OverloadChoice, MemberLookupResult::UnviableReason>
-                 &candidate) { return candidate.first; });
-
-      SmallVector<Constraint *, 4> candidates;
-      generateConstraints(candidates, memberTy, choices, useDC, locator,
-                          /*favoredChoice=*/None, /*requiresFix=*/true,
-                          [&](unsigned idx, const OverloadChoice &choice) {
-                            return fixMemberRef(
-                                *this, baseTy, member, choice, locator,
-                                result.UnviableCandidates[idx].second);
-                          });
-
-      // If there are any viable "fixed" candidates, let's schedule
-      // them to be attempted.
-      if (!candidates.empty()) {
-        addOverloadSet(candidates, locator);
-        return SolutionKind::Solved;
-      }
-    }
-
     if (baseObjTy->getOptionalObjectType()) {
       // If the base type was an optional, look through it.
 

lib/Sema/CSSolver.cpp

@@ -1305,8 +1305,10 @@ ConstraintSystem::filterDisjunction(
   for (unsigned constraintIdx : indices(disjunction->getNestedConstraints())) {
     auto constraint = disjunction->getNestedConstraints()[constraintIdx];
 
-    // Skip already-disabled constraints.
-    if (constraint->isDisabled())
+    // Skip already-disabled constraints. Let's treat disabled
+    // choices which have a fix as "enabled" ones here, so we can
+    // potentially infer some type information from them.
+    if (constraint->isDisabled() && !constraint->getFix())
       continue;
 
     if (pred(constraint)) {

lib/Sema/CSStep.cpp

@@ -430,7 +430,9 @@ StepResult DisjunctionStep::resume(bool prevFailed) {
 bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const {
   auto &ctx = CS.getASTContext();
 
-  if (choice.isDisabled()) {
+  bool attemptFixes = CS.shouldAttemptFixes();
+  // Enable "fixed" overload choices in "diagnostic" mode.
+  if (!(attemptFixes && choice.hasFix()) && choice.isDisabled()) {
     if (isDebugMode()) {
       auto &log = getDebugLogger();
       log << "(skipping ";
@@ -442,7 +444,7 @@ bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const {
   }
 
   // Skip unavailable overloads unless solver is in the "diagnostic" mode.
-  if (!CS.shouldAttemptFixes() && choice.isUnavailable())
+  if (!attemptFixes && choice.isUnavailable())
     return true;
 
   if (ctx.LangOpts.DisableConstraintSolverPerformanceHacks)

lib/Sema/Constraint.cpp

@@ -169,7 +169,7 @@ Constraint::Constraint(Type type, OverloadChoice choice, DeclContext *useDC,
                        ConstraintFix *fix, ConstraintLocator *locator,
                        ArrayRef<TypeVariableType *> typeVars)
     : Kind(ConstraintKind::BindOverload), TheFix(fix), HasRestriction(false),
-      IsActive(false), IsDisabled(false), RememberChoice(false),
+      IsActive(false), IsDisabled(bool(fix)), RememberChoice(false),
       IsFavored(false),
       NumTypeVariables(typeVars.size()), Overload{type, choice, useDC},
       Locator(locator) {

lib/Sema/Constraint.h

@@ -401,6 +401,7 @@ class Constraint final : public llvm::ilist_node<Constraint>,
                                  ConstraintLocator *locator);
 
   /// Create a bind overload choice with a fix.
+  /// Note: This constraint is going to be disabled by default.
   static Constraint *createFixedChoice(ConstraintSystem &cs, Type type,
                                        OverloadChoice choice,
                                        DeclContext *useDC, ConstraintFix *fix,

lib/Sema/ConstraintSystem.cpp

@@ -2525,10 +2525,9 @@ void ConstraintSystem::generateConstraints(
   }
 
   for (auto index : indices(choices)) {
-    const auto &choice = choices[index];
     if (favoredIndex && (*favoredIndex == index))
       continue;
 
-    recordChoice(constraints, index, choice);
+    recordChoice(constraints, index, choices[index]);
   }
 }

lib/Sema/ConstraintSystem.h

@@ -873,11 +873,12 @@ struct MemberLookupResult {
     /// The member is inaccessible (e.g. a private member in another file).
     UR_Inaccessible,
   };
-  
-  /// This is a list of considered (but rejected) candidates, along with a
-  /// reason for their rejection.
-  SmallVector<std::pair<OverloadChoice, UnviableReason>, 4> UnviableCandidates;
 
+  /// This is a list of considered (but rejected) candidates, along with a
+  /// reason for their rejection. Split into separate collections to make
+  /// it easier to use in conjunction with viable candidates.
+  SmallVector<OverloadChoice, 4> UnviableCandidates;
+  SmallVector<UnviableReason, 4> UnviableReasons;
 
   /// Mark this as being an already-diagnosed error and return itself.
   MemberLookupResult &markErrorAlreadyDiagnosed() {
@@ -890,7 +891,8 @@ struct MemberLookupResult {
   }
 
   void addUnviable(OverloadChoice candidate, UnviableReason reason) {
-    UnviableCandidates.push_back({candidate, reason});
+    UnviableCandidates.push_back(candidate);
+    UnviableReasons.push_back(reason);
   }
 
   Optional<unsigned> getFavoredIndex() const {
@@ -2474,7 +2476,7 @@ class ConstraintSystem {
   /// \param requiresFix Determines whether choices require a fix to
   /// be included in the result. If the fix couldn't be provided by
   /// `getFix` for any given choice, such choice would be filtered out.
-  ////
+  ///
   /// \param getFix Optional callback to determine a fix for a given
   /// choice (first argument is a position of current choice,
   /// second - the choice in question).
@@ -3678,6 +3680,10 @@ class DisjunctionChoice {
 
   bool isDisabled() const { return Choice->isDisabled(); }
 
+  bool hasFix() const {
+    return bool(Choice->getFix());
+  }
+
   bool isUnavailable() const {
     if (auto *decl = getDecl(Choice))
       return decl->getAttrs().isUnavailable(decl->getASTContext());

test/Constraints/members.swift

@@ -359,19 +359,19 @@ extension Sequence {
 }
 
 class C_25341015 {
-  static func baz(_ x: Int, _ y: Int) {} // expected-note {{'baz' declared here}}
+  static func baz(_ x: Int, _ y: Int) {}
   func baz() {}
   func qux() {
-    baz(1, 2) // expected-error {{use of 'baz' refers to instance method 'baz()' rather than static method 'baz' in class 'C_25341015'}} expected-note {{use 'C_25341015.' to reference the static method}}
+    baz(1, 2) // expected-error {{static member 'baz' cannot be used on instance of type 'C_25341015'}} {{5-5=C_25341015.}}
   }
 }
 
 struct S_25341015 {
-  static func foo(_ x: Int, y: Int) {} // expected-note {{'foo(_:y:)' declared here}}
+  static func foo(_ x: Int, y: Int) {}
 
   func foo(z: Int) {}
   func bar() {
-    foo(1, y: 2) // expected-error {{use of 'foo' refers to instance method 'foo(z:)' rather than static method 'foo(_:y:)' in struct 'S_25341015'}} expected-note {{use 'S_25341015.' to reference the static method}}
+    foo(1, y: 2) // expected-error {{static member 'foo' cannot be used on instance of type 'S_25341015'}} {{5-5=S_25341015.}}
   }
 }