Move all shouldRecordFix logic back into CS itself keying off of FixKind.

Author: gregomni

lib/Sema/CSFix.cpp

@@ -44,18 +44,6 @@ void ConstraintFix::print(llvm::raw_ostream &Out) const {
 
 void ConstraintFix::dump() const {print(llvm::errs()); }
 
-bool ConstraintFix::constraintSystemContainsFixInExpr(Expr *expr) const {
-  llvm::SmallDenseSet<Expr *> fixExprs;
-  for (auto fix : CS.Fixes)
-    fixExprs.insert(fix->getAnchor());
-  bool found = false;
-  expr->forEachChildExpr([&](Expr *subExpr) -> Expr * {
-    found |= fixExprs.count(subExpr) > 0;
-    return subExpr;
-  });
-  return found;
-}
-
 std::string ForceDowncast::getName() const {
   llvm::SmallString<16> name;
   name += "force downcast (as! ";
@@ -117,10 +105,6 @@ AddAddressOf *AddAddressOf::create(ConstraintSystem &cs,
   return new (cs.getAllocator()) AddAddressOf(cs, locator);
 }
 
-bool TreatRValueAsLValue::shouldRecordFix() const {
-  return !constraintSystemContainsFixInExpr(getAnchor());
-}
-
 bool TreatRValueAsLValue::diagnose(Expr *root, bool asNote) const {
   RValueTreatedAsLValueFailure failure(getConstraintSystem(), getLocator());
   return failure.diagnose(asNote);

lib/Sema/CSFix.h

@@ -92,12 +92,6 @@ class ConstraintFix {
 
   virtual std::string getName() const = 0;
 
-  /// Determine from the current CS (primarily the already existing fixes)
-  /// whether this fix should be recorded for later diagnosis.
-  virtual bool shouldRecordFix() const {
-    return true;
-  }
-
   /// Diagnose a failure associated with this fix given
   /// root expression and information from constraint system.
   virtual bool diagnose(Expr *root, bool asNote = false) const = 0;
@@ -116,8 +110,6 @@ class ConstraintFix {
 
 protected:
   ConstraintSystem &getConstraintSystem() const { return CS; }
-
-  bool constraintSystemContainsFixInExpr(Expr *expr) const;
 };
 
 /// Append 'as! T' to force a downcast to the specified type.
@@ -197,7 +189,6 @@ class TreatRValueAsLValue final : public ConstraintFix {
 public:
   std::string getName() const override { return "treat rvalue as lvalue"; }
 
-  bool shouldRecordFix() const override;
   bool diagnose(Expr *root, bool asNote = false) const override;
 
   static TreatRValueAsLValue *create(ConstraintSystem &cs,

lib/Sema/CSSimplify.cpp

@@ -4987,6 +4987,15 @@ ConstraintSystem::simplifyRestrictedConstraint(
   llvm_unreachable("Unhandled SolutionKind in switch.");
 }
 
+static bool isAugmentingFix(ConstraintFix *fix) {
+  switch (fix->getKind()) {
+    case swift::constraints::FixKind::TreatRValueAsLValue:
+      return false;
+    default:
+      return true;
+  }
+}
+
 bool ConstraintSystem::recordFix(ConstraintFix *fix) {
   auto &ctx = getASTContext();
   if (ctx.LangOpts.DebugConstraintSolver) {
@@ -5005,20 +5014,30 @@ bool ConstraintSystem::recordFix(ConstraintFix *fix) {
   if (worseThanBestSolution())
     return true;
 
-  if (!fix->shouldRecordFix())
-    return false;
-
-  auto *loc = fix->getLocator();
-  auto existingFix = llvm::find_if(Fixes, [&](const ConstraintFix *e) {
-    // If we already have a fix like this recorded, let's not do it again,
-    // this situation might happen when the same fix kind is applicable to
+  if (isAugmentingFix(fix)) {
+    // Always useful, unless duplicate of exactly the same fix and location.
+    // This situation might happen when the same fix kind is applicable to
     // different overload choices.
-    return e->getKind() == fix->getKind() && e->getLocator() == loc;
-  });
-
-  if (existingFix == Fixes.end())
-    Fixes.push_back(fix);
-
+    auto *loc = fix->getLocator();
+    auto existingFix = llvm::find_if(Fixes, [&](const ConstraintFix *e) {
+      // If we already have a fix like this recorded, let's not do it again,
+      return e->getKind() == fix->getKind() && e->getLocator() == loc;
+    });
+    if (existingFix == Fixes.end())
+      Fixes.push_back(fix);
+  } else {
+    // Only useful to record if no pre-existing fix in the subexpr tree.
+    llvm::SmallDenseSet<Expr *> fixExprs;
+    for (auto fix : Fixes)
+      fixExprs.insert(fix->getAnchor());
+    bool found = false;
+    fix->getAnchor()->forEachChildExpr([&](Expr *subExpr) -> Expr * {
+      found |= fixExprs.count(subExpr) > 0;
+      return subExpr;
+    });
+    if (!found)
+      Fixes.push_back(fix);
+  }
   return false;
 }