From 821f63fe98581f57dfcf68916eee42654e3509b4 Mon Sep 17 00:00:00 2001
From: gregomni <gregomni@gmail.com>
Date: Thu, 23 Aug 2018 20:12:30 -0700
Subject: [PATCH 1/3] Make assignments and assignment failure diagnoses
 directly in the CS. More specific diagnoses for assigning to read-only
 keypaths. 'computeAssignDestType' is dead code now. ConstraintFix
 shouldRecordFix()

---
 lib/Sema/CSApply.cpp                          | 11 +--
 lib/Sema/CSDiag.cpp                           | 82 +++++++++++--------
 lib/Sema/CSDiagnostics.cpp                    | 62 ++++++++++++--
 lib/Sema/CSDiagnostics.h                      |  7 +-
 lib/Sema/CSFix.cpp                            | 16 ++++
 lib/Sema/CSFix.h                              |  9 ++
 lib/Sema/CSGen.cpp                            | 36 ++++----
 lib/Sema/CSSimplify.cpp                       | 10 ++-
 lib/Sema/ConstraintSystem.h                   |  6 +-
 lib/Sema/TypeCheckConstraints.cpp             | 53 ------------
 ...c_redeclared_properties_incompatible.swift | 12 +--
 test/Constraints/assignment.swift             |  3 +-
 test/Constraints/dynamic_lookup.swift         |  8 +-
 test/Constraints/keypath.swift                |  2 +-
 test/Constraints/keypath_swift_5.swift        |  4 +-
 test/Parse/recovery.swift                     |  2 +-
 test/decl/func/operator.swift                 |  3 +-
 test/decl/init/constructor-kind.swift         | 10 +--
 test/expr/unary/keypath/keypath.swift         | 41 +++++-----
 19 files changed, 205 insertions(+), 172 deletions(-)

diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
index 7ce6aa2069f43..81e85c564bbde 100644
--- a/lib/Sema/CSApply.cpp
+++ b/lib/Sema/CSApply.cpp
@@ -3832,18 +3832,11 @@ namespace {
     }
 
     Expr *visitAssignExpr(AssignExpr *expr) {
-      // Compute the type to which the source must be converted to allow
-      // assignment to the destination.
-      //
-      // FIXME: This is also computed when the constraint system is set up.
-      auto destTy = cs.computeAssignDestType(expr->getDest(), expr->getLoc());
-      if (!destTy)
-        return nullptr;
-
       // Convert the source to the simplified destination type.
+      auto destTy = simplifyType(cs.getType(expr->getDest()));
       auto locator =
         ConstraintLocatorBuilder(cs.getConstraintLocator(expr->getSrc()));
-      Expr *src = coerceToType(expr->getSrc(), destTy, locator);
+      Expr *src = coerceToType(expr->getSrc(), destTy->getRValueType(), locator);
       if (!src)
         return nullptr;
 
diff --git a/lib/Sema/CSDiag.cpp b/lib/Sema/CSDiag.cpp
index 52031512b82d7..9003ebaed5702 100644
--- a/lib/Sema/CSDiag.cpp
+++ b/lib/Sema/CSDiag.cpp
@@ -689,6 +689,17 @@ resolveImmutableBase(Expr *expr, ConstraintSystem &CS) {
         return { expr, member };
     }
 
+    if (auto tupleExpr = dyn_cast<TupleExpr>(SE->getIndex())) {
+      if (tupleExpr->getNumElements() == 1 && tupleExpr->getElementName(0).str() == "keyPath") {
+        auto indexType = CS.simplifyType(CS.getType(tupleExpr->getElement(0)));
+        if (auto bgt = indexType->getAs<BoundGenericType>()) {
+          auto decl = bgt->getDecl();
+          if (decl == CS.getASTContext().getKeyPathDecl())
+            return resolveImmutableBase(tupleExpr->getElement(0), CS);
+        }
+      }
+    }
+
     // If it is settable, then the base must be the problem, recurse.
     return resolveImmutableBase(SE->getBase(), CS);
   }
@@ -744,6 +755,9 @@ resolveImmutableBase(Expr *expr, ConstraintSystem &CS) {
     if (!isa<LoadExpr>(ICE->getSubExpr()))
       return resolveImmutableBase(ICE->getSubExpr(), CS);
 
+  if (auto *SAE = dyn_cast<SelfApplyExpr>(expr))
+    return resolveImmutableBase(SAE->getFn(), CS);
+
   return { expr, nullptr };
 }
 
@@ -840,7 +854,11 @@ void swift::diagnoseSubElementFailure(Expr *destExpr,
     message += VD->getName().str().str();
     message += "'";
 
-    if (VD->isCaptureList())
+    auto type = CS.simplifyType(CS.getType(immInfo.first));
+    auto bgt = type ? type->getAs<BoundGenericType>() : nullptr;
+    if (bgt && bgt->getDecl() == CS.getASTContext().getKeyPathDecl())
+      message += " is a read-only key path";
+    else if (VD->isCaptureList())
       message += " is an immutable capture";
     else if (VD->isImplicit())
       message += " is immutable";
@@ -898,6 +916,14 @@ void swift::diagnoseSubElementFailure(Expr *destExpr,
     return;
   }
 
+  // If a keypath was the problem but wasn't resolved into a vardecl
+  // it is ambiguous or unable to be used for setting.
+  if (auto *KPE = dyn_cast_or_null<KeyPathExpr>(immInfo.first)) {
+    TC.diagnose(loc, diagID, "immutable key path")
+      .highlight(KPE->getSourceRange());
+    return;
+  }
+
   // If the expression is the result of a call, it is an rvalue, not a mutable
   // lvalue.
   if (auto *AE = dyn_cast<ApplyExpr>(immInfo.first)) {
@@ -3401,44 +3427,30 @@ bool FailureDiagnosis::diagnoseContextualConversionError(
 
 /// When an assignment to an expression is detected and the destination is
 /// invalid, emit a detailed error about the condition.
-void ConstraintSystem::diagnoseAssignmentFailure(Expr *dest, Type destTy,
+bool ConstraintSystem::diagnoseAssignmentFailure(Expr *dest, Type destTy,
                                                  SourceLoc equalLoc) {
   auto &TC = getTypeChecker();
 
-  // Diagnose obvious assignments to literals.
-  if (isa<LiteralExpr>(dest->getValueProvidingExpr())) {
-    TC.diagnose(equalLoc, diag::cannot_assign_to_literal);
-    return;
-  }
-
-  // Diagnose assignments to let-properties in delegating initializers.
-  if (auto *member = dyn_cast<UnresolvedDotExpr>(dest)) {
-    if (auto *ctor = dyn_cast<ConstructorDecl>(DC)) {
-      if (auto *baseRef = dyn_cast<DeclRefExpr>(member->getBase())) {
-        if (baseRef->getDecl() == ctor->getImplicitSelfDecl() &&
-            ctor->getDelegatingOrChainedInitKind(nullptr) ==
-              ConstructorDecl::BodyInitKind::Delegating) {
-          auto resolved = resolveImmutableBase(member, *this);
-          assert(resolved.first == member);
-          TC.diagnose(equalLoc, diag::assignment_let_property_delegating_init,
-                      member->getName());
-          if (resolved.second) {
-            TC.diagnose(resolved.second, diag::decl_declared_here,
-                        member->getName());
-          }
-          return;
-        }
-      }
+  // Assignments to let-properties in delegating initializers will be caught
+  // elsewhere now, so if we see them here, it isn't an assignment problem.
+  if (auto *ctor = dyn_cast<ConstructorDecl>(DC)) {
+    DeclRefExpr *baseRef = nullptr;
+    if (auto *member = dyn_cast<UnresolvedDotExpr>(dest)) {
+      baseRef = dyn_cast<DeclRefExpr>(member->getBase());
+    } else if (auto *member = dyn_cast<MemberRefExpr>(dest)) {
+      if (auto *load = dyn_cast<LoadExpr>(member->getBase()))
+        baseRef = dyn_cast<DeclRefExpr>(load->getSubExpr());
+    }
+    if (baseRef && baseRef->getDecl() == ctor->getImplicitSelfDecl() &&
+        ctor->getDelegatingOrChainedInitKind(nullptr) ==
+          ConstructorDecl::BodyInitKind::Delegating) {
+      return false;
     }
   }
 
   Diag<StringRef> diagID;
-  if (isa<ApplyExpr>(dest))
+  if (isa<ApplyExpr>(dest) || isa<SelfApplyExpr>(dest))
     diagID = diag::assignment_lhs_is_apply_expression;
-  else if (isa<DeclRefExpr>(dest))
-    diagID = diag::assignment_lhs_is_immutable_variable;
-  else if (isa<ForceValueExpr>(dest))
-    diagID = diag::assignment_bang_has_immutable_subcomponent;
   else if (isa<UnresolvedDotExpr>(dest) || isa<MemberRefExpr>(dest))
     diagID = diag::assignment_lhs_is_immutable_property;
   else if (auto sub = dyn_cast<SubscriptExpr>(dest)) {
@@ -3451,12 +3463,12 @@ void ConstraintSystem::diagnoseAssignmentFailure(Expr *dest, Type destTy,
         sub->getArgumentLabels().size() == 1 &&
         sub->getArgumentLabels().front() == TC.Context.Id_dynamicMember)
       diagID = diag::assignment_dynamic_property_has_immutable_base;
-  } else {
+  } else
     diagID = diag::assignment_lhs_is_immutable_variable;
-  }
 
   diagnoseSubElementFailure(dest, equalLoc, *this, diagID,
                             diag::assignment_lhs_not_lvalue);
+  return true;
 }
 
 //===----------------------------------------------------------------------===//
@@ -6545,8 +6557,8 @@ bool FailureDiagnosis::visitAssignExpr(AssignExpr *assignExpr) {
       if (diagnoseSubscriptErrors(subscriptExpr, /* inAssignmentDestination = */ true))
         return true;
     }
-    CS.diagnoseAssignmentFailure(destExpr, destType, assignExpr->getLoc());
-    return true;
+    if (CS.diagnoseAssignmentFailure(destExpr, destType, assignExpr->getLoc()))
+      return true;
   }
 
   auto *srcExpr = assignExpr->getSrc();
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
index 2dc883d2b1af4..07085ae695409 100644
--- a/lib/Sema/CSDiagnostics.cpp
+++ b/lib/Sema/CSDiagnostics.cpp
@@ -277,7 +277,10 @@ bool MissingForcedDowncastFailure::diagnoseAsError() {
 
   auto &TC = getTypeChecker();
 
-  auto *coerceExpr = dyn_cast<CoerceExpr>(getAnchor());
+  auto *expr = getAnchor();
+  if (auto *assignExpr = dyn_cast<AssignExpr>(expr))
+    expr = assignExpr->getSrc();
+  auto *coerceExpr = dyn_cast<CoerceExpr>(expr);
   if (!coerceExpr)
     return false;
 
@@ -331,6 +334,8 @@ bool MissingExplicitConversionFailure::diagnoseAsError() {
   auto &TC = getTypeChecker();
 
   auto *anchor = getAnchor();
+  if (auto *assign = dyn_cast<AssignExpr>(anchor))
+    anchor = assign->getSrc();
   if (auto *paren = dyn_cast<ParenExpr>(anchor))
     anchor = paren->getSubExpr();
 
@@ -538,6 +543,10 @@ bool MissingOptionalUnwrapFailure::diagnoseAsError() {
     return false;
 
   auto *anchor = getAnchor();
+
+  if (auto assignExpr = dyn_cast<AssignExpr>(anchor))
+    anchor = assignExpr->getSrc();
+  
   auto *unwrapped = anchor->getValueProvidingExpr();
   auto type = getType(anchor)->getRValueType();
 
@@ -552,9 +561,13 @@ bool MissingOptionalUnwrapFailure::diagnoseAsError() {
 
 bool RValueTreatedAsLValueFailure::diagnoseAsError() {
   Diag<StringRef> subElementDiagID;
-  Diag<Type> rvalueDiagID;
+  Diag<Type> rvalueDiagID = diag::assignment_lhs_not_lvalue;
   Expr *diagExpr = getLocator()->getAnchor();
-  SourceLoc loc;
+  SourceLoc loc = diagExpr->getLoc();
+
+  if (auto assignExpr = dyn_cast<AssignExpr>(diagExpr)) {
+    diagExpr = assignExpr->getDest();
+  }
 
   if (auto callExpr = dyn_cast<ApplyExpr>(diagExpr)) {
     Expr *argExpr = callExpr->getArg();
@@ -569,7 +582,7 @@ bool RValueTreatedAsLValueFailure::diagnoseAsError() {
       rvalueDiagID = diag::cannot_apply_lvalue_binop_to_rvalue;
       auto argTuple = dyn_cast<TupleExpr>(argExpr);
       diagExpr = argTuple->getElement(0);
-    } else {
+    } else if (getLocator()->getPath().size() > 0) {
       auto lastPathElement = getLocator()->getPath().back();
       assert(lastPathElement.getKind() ==
              ConstraintLocator::PathElementKind::ApplyArgToParam);
@@ -580,10 +593,11 @@ bool RValueTreatedAsLValueFailure::diagnoseAsError() {
         diagExpr = argTuple->getElement(lastPathElement.getValue());
       else if (auto parens = dyn_cast<ParenExpr>(argExpr))
         diagExpr = parens->getSubExpr();
+    } else {
+      subElementDiagID = diag::assignment_lhs_is_apply_expression;
     }
   } else if (auto inoutExpr = dyn_cast<InOutExpr>(diagExpr)) {
-    Type type = getConstraintSystem().getType(inoutExpr);
-    if (auto restriction = restrictionForType(type)) {
+    if (auto restriction = getRestrictionForType(getType(inoutExpr))) {
       PointerTypeKind pointerKind;
       if (restriction->second == ConversionRestrictionKind::ArrayToPointer &&
           restriction->first->getAnyPointerElementType(pointerKind) &&
@@ -601,10 +615,42 @@ bool RValueTreatedAsLValueFailure::diagnoseAsError() {
 
     subElementDiagID = diag::cannot_pass_rvalue_inout_subelement;
     rvalueDiagID = diag::cannot_pass_rvalue_inout;
-    loc = diagExpr->getLoc();
     diagExpr = inoutExpr->getSubExpr();
+  } else if (isa<DeclRefExpr>(diagExpr)) {
+    subElementDiagID = diag::assignment_lhs_is_immutable_variable;
+  } else if (isa<ForceValueExpr>(diagExpr)) {
+    subElementDiagID = diag::assignment_bang_has_immutable_subcomponent;
+  } else if (isa<MemberRefExpr>(diagExpr)) {
+    subElementDiagID = diag::assignment_lhs_is_immutable_property;
+  } else if (auto member = dyn_cast<UnresolvedDotExpr>(diagExpr)) {
+    subElementDiagID = diag::assignment_lhs_is_immutable_property;
+
+    if (auto *ctor = dyn_cast<ConstructorDecl>(getDC())) {
+      if (auto *baseRef = dyn_cast<DeclRefExpr>(member->getBase())) {
+        if (baseRef->getDecl() == ctor->getImplicitSelfDecl() &&
+            ctor->getDelegatingOrChainedInitKind(nullptr) ==
+            ConstructorDecl::BodyInitKind::Delegating) {
+          emitDiagnostic(loc, diag::assignment_let_property_delegating_init,
+                      member->getName());
+          if (auto *ref = getResolvedMemberRef(member)) {
+            emitDiagnostic(ref, diag::decl_declared_here, member->getName());
+          }
+          return true;
+        }
+      }
+    }
+  } else if (auto sub = dyn_cast<SubscriptExpr>(diagExpr)) {
+      subElementDiagID = diag::assignment_subscript_has_immutable_base;
+
+      // If the destination is a subscript with a 'dynamicLookup:' label and if
+      // the tuple is implicit, then this was actually a @dynamicMemberLookup
+      // access. Emit a more specific diagnostic.
+      if (sub->getIndex()->isImplicit() &&
+          sub->getArgumentLabels().size() == 1 &&
+          sub->getArgumentLabels().front() == getTypeChecker().Context.Id_dynamicMember)
+        subElementDiagID = diag::assignment_dynamic_property_has_immutable_base;
   } else {
-    return false;
+    subElementDiagID = diag::assignment_lhs_is_immutable_variable;
   }
 
   diagnoseSubElementFailure(diagExpr, loc, getConstraintSystem(),
diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h
index fadc55a5919b9..5ca74ddd080a7 100644
--- a/lib/Sema/CSDiagnostics.h
+++ b/lib/Sema/CSDiagnostics.h
@@ -95,7 +95,7 @@ class FailureDiagnostic {
   DeclContext *getDC() const { return CS.DC; }
 
   Optional<std::pair<Type, ConversionRestrictionKind>>
-  restrictionForType(Type type) const {
+  getRestrictionForType(Type type) const {
     for (auto &restriction : CS.ConstraintRestrictions) {
       if (std::get<0>(restriction)->isEqual(type))
         return std::pair<Type, ConversionRestrictionKind>(
@@ -104,6 +104,11 @@ class FailureDiagnostic {
     return None;
   }
 
+  ValueDecl *getResolvedMemberRef(UnresolvedDotExpr *member) {
+    auto locator = CS.getConstraintLocator(member, ConstraintLocator::Member);
+    return CS.findResolvedMemberRef(locator);
+  }
+
   Optional<SelectedOverload>
   getOverloadChoiceIfAvailable(ConstraintLocator *locator) const {
     if (auto *overload = getResolvedOverload(locator))
diff --git a/lib/Sema/CSFix.cpp b/lib/Sema/CSFix.cpp
index 311265eadb9c5..e44fe1138853d 100644
--- a/lib/Sema/CSFix.cpp
+++ b/lib/Sema/CSFix.cpp
@@ -44,6 +44,18 @@ 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! ";
@@ -105,6 +117,10 @@ 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);
diff --git a/lib/Sema/CSFix.h b/lib/Sema/CSFix.h
index 28b5318cdaee3..4b049bb84d886 100644
--- a/lib/Sema/CSFix.h
+++ b/lib/Sema/CSFix.h
@@ -92,6 +92,12 @@ 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;
@@ -110,6 +116,8 @@ class ConstraintFix {
 
 protected:
   ConstraintSystem &getConstraintSystem() const { return CS; }
+
+  bool constraintSystemContainsFixInExpr(Expr *expr) const;
 };
 
 /// Append 'as! T' to force a downcast to the specified type.
@@ -189,6 +197,7 @@ 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,
diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp
index cb47c95389440..c7259d58f532e 100644
--- a/lib/Sema/CSGen.cpp
+++ b/lib/Sema/CSGen.cpp
@@ -2745,26 +2745,30 @@ namespace {
       auto typeVar = CS.createTypeVariable(locator);
       return LValueType::get(typeVar);
     }
-    
+
+    static Type genAssignDestType(Expr *expr, ConstraintSystem &CS) {
+      if (auto *TE = dyn_cast<TupleExpr>(expr)) {
+        SmallVector<TupleTypeElt, 4> destTupleTypes;
+        for (unsigned i = 0; i !=  TE->getNumElements(); ++i) {
+          Type subType = genAssignDestType(TE->getElement(i), CS);
+          destTupleTypes.push_back(TupleTypeElt(subType, TE->getElementName(i)));
+        }
+        return TupleType::get(destTupleTypes, CS.getASTContext());
+      } else {
+        Type destTy = CS.createTypeVariable(CS.getConstraintLocator(expr));
+        CS.addConstraint(ConstraintKind::Bind, CS.getType(expr), LValueType::get(destTy),
+                         CS.getConstraintLocator(expr));
+        return destTy;
+      }
+    }
+
     Type visitAssignExpr(AssignExpr *expr) {
       // Handle invalid code.
       if (!expr->getDest() || !expr->getSrc())
         return Type();
-      
-      // Compute the type to which the source must be converted to allow
-      // assignment to the destination.
-      auto destTy = CS.computeAssignDestType(expr->getDest(), expr->getLoc());
-      if (!destTy)
-        return Type();
-      if (destTy->is<UnresolvedType>()) {
-        return CS.createTypeVariable(CS.getConstraintLocator(expr));
-      }
-      
-      // The source must be convertible to the destination.
-      CS.addConstraint(ConstraintKind::Conversion,
-                       CS.getType(expr->getSrc()), destTy,
-                       CS.getConstraintLocator(expr->getSrc()));
-
+      Type destTy = genAssignDestType(expr->getDest(), CS);
+      CS.addConstraint(ConstraintKind::Conversion, CS.getType(expr->getSrc()), destTy,
+                       CS.getConstraintLocator(expr));
       return TupleType::getEmpty(CS.getASTContext());
     }
     
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
index 0d175d34db3d5..eb584bc74f9a0 100644
--- a/lib/Sema/CSSimplify.cpp
+++ b/lib/Sema/CSSimplify.cpp
@@ -1600,8 +1600,9 @@ ConstraintSystem::matchTypesBindTypeVar(
 
   // If the left-hand type variable cannot bind to an lvalue,
   // but we still have an lvalue, fail.
-  if (!typeVar->getImpl().canBindToLValue() && type->hasLValueType())
+  if (!typeVar->getImpl().canBindToLValue() && type->hasLValueType()) {
     return getTypeMatchFailure(locator);
+  }
 
   // Disallow bindings of noescape functions to type variables that
   // represent an opened archetype. If we allowed this it would allow
@@ -2520,11 +2521,11 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
           TreatRValueAsLValue::create(*this, getConstraintLocator(locator)));
       }
     }
+  }
 
-    if (type2->is<LValueType>() && !isTypeVarOrMember1) {
+  if (attemptFixes && type2->is<LValueType>() && !isTypeVarOrMember1) {
       conversionsOrFixes.push_back(
           TreatRValueAsLValue::create(*this, getConstraintLocator(locator)));
-    }
   }
 
   if (attemptFixes)
@@ -5004,6 +5005,9 @@ 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,
diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h
index f383a32f78560..ac88edd3b3947 100644
--- a/lib/Sema/ConstraintSystem.h
+++ b/lib/Sema/ConstraintSystem.h
@@ -1774,7 +1774,7 @@ class ConstraintSystem {
 
   /// When an assignment to an expression is detected and the destination is
   /// invalid, emit a detailed error about the condition.
-  void diagnoseAssignmentFailure(Expr *dest, Type destTy, SourceLoc equalLoc);
+  bool diagnoseAssignmentFailure(Expr *dest, Type destTy, SourceLoc equalLoc);
 
   
   /// \brief Mine the active and inactive constraints in the constraint
@@ -2366,10 +2366,6 @@ class ConstraintSystem {
     TypeMatchResult(SolutionKind result) : Kind(result) {}
   };
 
-  /// \brief Compute the rvalue type of the given expression, which is the
-  /// destination of an assignment statement.
-  Type computeAssignDestType(Expr *dest, SourceLoc equalLoc);
-
   /// \brief Subroutine of \c matchTypes(), which matches up two tuple types.
   ///
   /// \returns the result of performing the tuple-to-tuple conversion.
diff --git a/lib/Sema/TypeCheckConstraints.cpp b/lib/Sema/TypeCheckConstraints.cpp
index 7d2a0194130e4..a98a71c7f5192 100644
--- a/lib/Sema/TypeCheckConstraints.cpp
+++ b/lib/Sema/TypeCheckConstraints.cpp
@@ -2668,59 +2668,6 @@ bool TypeChecker::typeCheckForEachBinding(DeclContext *dc, ForEachStmt *stmt) {
   return !resultTy;
 }
 
-/// \brief Compute the rvalue type of the given expression, which is the
-/// destination of an assignment statement.
-Type ConstraintSystem::computeAssignDestType(Expr *dest, SourceLoc equalLoc) {
-  if (auto *TE = dyn_cast<TupleExpr>(dest)) {
-    auto &ctx = getASTContext();
-    SmallVector<TupleTypeElt, 4> destTupleTypes;
-    for (unsigned i = 0; i != TE->getNumElements(); ++i) {
-      Expr *subExpr = TE->getElement(i);
-      Type elemTy = computeAssignDestType(subExpr, equalLoc);
-      if (!elemTy)
-        return Type();
-      destTupleTypes.push_back(TupleTypeElt(elemTy, TE->getElementName(i)));
-    }
-
-    return TupleType::get(destTupleTypes, ctx);
-  }
-
-  Type destTy = simplifyType(getType(dest));
-  if (destTy->hasError())
-    return Type();
-
-  // If we have already resolved a concrete lvalue destination type, return it.
-  if (LValueType *destLV = destTy->getAs<LValueType>())
-    return destLV->getObjectType();
-
-  // If the destination is a type variable, the type variable must be an
-  // lvalue type, which we enforce via an equality relationship with
-  // @lvalue T, where T is a fresh type variable that will be the object type of
-  // this particular expression type.
-  if (auto typeVar = dyn_cast<TypeVariableType>(destTy.getPointer())) {
-    // Newly allocated type should be explicitly materializable,
-    // it's invalid to use non-materializable types as assignment destination.
-    auto objectTv = createTypeVariable(getConstraintLocator(dest));
-    auto refTv = LValueType::get(objectTv);
-    addConstraint(ConstraintKind::Bind, typeVar, refTv,
-                  getConstraintLocator(dest));
-    return objectTv;
-  }
-
-  // Otherwise, the destination is erroneous.  If there are FixIt hints
-  // introduced into the system, they may well be the reason that this isn't an
-  // lvalue.  Emit them if present.
-  if (!Fixes.empty()) {
-    auto solution = finalize(FreeTypeVariableBinding::Allow);
-    if (applySolutionFixes(dest, solution))
-      return Type();
-  }
-
-  // Otherwise, it is a structural problem, diagnose that.
-  diagnoseAssignmentFailure(dest, destTy, equalLoc);
-  return Type();
-}
-
 bool TypeChecker::typeCheckCondition(Expr *&expr, DeclContext *dc) {
   // If this expression is already typechecked and has an i1 type, then it has
   // already got its conversion from Boolean back to i1.  Just re-typecheck
diff --git a/test/ClangImporter/objc_redeclared_properties_incompatible.swift b/test/ClangImporter/objc_redeclared_properties_incompatible.swift
index bdcd2c7483d5e..84c2462404f84 100644
--- a/test/ClangImporter/objc_redeclared_properties_incompatible.swift
+++ b/test/ClangImporter/objc_redeclared_properties_incompatible.swift
@@ -24,7 +24,7 @@ func testWithInitializer() {
   // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
-  // CHECK-PUBLIC: [[@LINE-1]]:23: error: cannot assign to property: 'readwriteChange' is a get-only property
+  // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
 func testWithoutInitializer(obj: PropertiesNoInit) {
@@ -38,7 +38,7 @@ func testWithoutInitializer(obj: PropertiesNoInit) {
   // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
-  // CHECK-PUBLIC: [[@LINE-1]]:23: error: cannot assign to property: 'readwriteChange' is a get-only property
+  // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
 func testGenericWithInitializer() {
@@ -54,7 +54,7 @@ func testGenericWithInitializer() {
   // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
-  // CHECK-PUBLIC: [[@LINE-1]]:23: error: cannot assign to property: 'readwriteChange' is a get-only property
+  // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
 func testGenericWithoutInitializer(obj: PropertiesNoInitGeneric<Base>) {
@@ -68,7 +68,7 @@ func testGenericWithoutInitializer(obj: PropertiesNoInitGeneric<Base>) {
   // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
-  // CHECK-PUBLIC: [[@LINE-1]]:23: error: cannot assign to property: 'readwriteChange' is a get-only property
+  // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
 func testCategoryWithInitializer() {
@@ -84,7 +84,7 @@ func testCategoryWithInitializer() {
   // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
-  // CHECK-PUBLIC: [[@LINE-1]]:23: error: cannot assign to property: 'readwriteChange' is a get-only property
+  // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
 func testCategoryWithoutInitializer(obj: PropertiesNoInitCategory) {
@@ -98,5 +98,5 @@ func testCategoryWithoutInitializer(obj: PropertiesNoInitCategory) {
   // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
-  // CHECK-PUBLIC: [[@LINE-1]]:23: error: cannot assign to property: 'readwriteChange' is a get-only property
+  // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
diff --git a/test/Constraints/assignment.swift b/test/Constraints/assignment.swift
index d1f5d6c39b8e0..7f902f05b6b94 100644
--- a/test/Constraints/assignment.swift
+++ b/test/Constraints/assignment.swift
@@ -53,8 +53,9 @@ value(_) // expected-error{{'_' can only appear in a pattern or on the left side
 // <rdar://problem/23798944> = vs. == in Swift if string character count statement causes segmentation fault
 func f23798944() {
   let s = ""
+  // FIXME: better would be: use of '=' in a boolean context, did you mean '=='?
   if s.count = 0 { // expected-error {{cannot assign to property: 'count' is a get-only property}}
   }
 }
 
-.sr_3506 = 0 // expected-error {{reference to member 'sr_3506' cannot be resolved without a contextual type}}
\ No newline at end of file
+.sr_3506 = 0 // expected-error {{reference to member 'sr_3506' cannot be resolved without a contextual type}}
diff --git a/test/Constraints/dynamic_lookup.swift b/test/Constraints/dynamic_lookup.swift
index efb6b67a0cb1a..1b6dd666599d8 100644
--- a/test/Constraints/dynamic_lookup.swift
+++ b/test/Constraints/dynamic_lookup.swift
@@ -291,9 +291,9 @@ let _: String = o[s]!
 let _: String? = o[s]
 // FIXME: These should all produce lvalues that we can write through
 o.s = s // expected-error {{cannot assign to immutable expression of type 'String?'}}
-o.s! = s // expected-error {{cannot assign to immutable expression of type 'String'}}
+o.s! = s // expected-error {{cannot assign through '!': 'o' is immutable}}
 o[s] = s // expected-error {{cannot assign to immutable expression of type 'String?'}}
-o[s]! = s // expected-error {{cannot assign to immutable expression of type 'String'}}
+o[s]! = s // expected-error {{cannot assign through '!': 'o' is immutable}}
 
 let _: String = o.t
 let _: String = o.t!
@@ -310,8 +310,8 @@ let _: DynamicIUO = o[dyn_iuo]!!
 let _: DynamicIUO? = o[dyn_iuo]
 // FIXME: These should all produce lvalues that we can write through
 o[dyn_iuo] = dyn_iuo // expected-error {{cannot assign to immutable expression of type 'DynamicIUO??'}}
-o[dyn_iuo]! = dyn_iuo // expected-error {{cannot assign to immutable expression of type 'DynamicIUO?'}}
-o[dyn_iuo]!! = dyn_iuo // expected-error {{cannot assign to immutable expression of type 'DynamicIUO'}}
+o[dyn_iuo]! = dyn_iuo // expected-error {{cannot assign through '!': 'o' is immutable}}
+o[dyn_iuo]!! = dyn_iuo // expected-error {{cannot assign through '!': 'o' is immutable}}
 
 
 // Check that we avoid picking an unavailable overload if there's an
diff --git a/test/Constraints/keypath.swift b/test/Constraints/keypath.swift
index 3e4c7590ffab1..9d050b6798752 100644
--- a/test/Constraints/keypath.swift
+++ b/test/Constraints/keypath.swift
@@ -7,7 +7,7 @@ struct S {
     let _: WritableKeyPath<S, Int> = \.i // no error for Swift 3/4
 
     S()[keyPath: \.i] = 1
-    // expected-error@-1 {{cannot assign to immutable expression}}
+    // expected-error@-1 {{cannot assign through subscript: function call returns immutable value}}
   }
 }
 
diff --git a/test/Constraints/keypath_swift_5.swift b/test/Constraints/keypath_swift_5.swift
index fb2916283e280..1bd5fca885bbf 100644
--- a/test/Constraints/keypath_swift_5.swift
+++ b/test/Constraints/keypath_swift_5.swift
@@ -7,7 +7,7 @@ struct S {
     let _: WritableKeyPath<S, Int> = \.i // expected-error {{type of expression is ambiguous without more context}}
 
     S()[keyPath: \.i] = 1
-    // expected-error@-1 {{cannot assign to immutable expression}}
+    // expected-error@-1 {{cannot assign through subscript: immutable key path}}
   }
 }
 
@@ -15,7 +15,7 @@ func test() {
   let _: WritableKeyPath<C, Int> = \.i // expected-error {{type of expression is ambiguous without more context}}
 
   C()[keyPath: \.i] = 1
-  // expected-error@-1 {{cannot assign to immutable expression}}
+  // expected-error@-1 {{cannot assign through subscript: immutable key path}}
 
   let _ = C()[keyPath: \.i] // no warning for a read
 }
diff --git a/test/Parse/recovery.swift b/test/Parse/recovery.swift
index 52b08b445efda..f29323b0761e4 100644
--- a/test/Parse/recovery.swift
+++ b/test/Parse/recovery.swift
@@ -587,7 +587,7 @@ class WrongInheritanceClause6(Int {}
 class WrongInheritanceClause7<T>(Int where T:AnyObject {}
 
 // <rdar://problem/18502220> [swift-crashes 078] parser crash on invalid cast in sequence expr
-Base=1 as Base=1  // expected-error {{cannot assign to immutable expression of type 'Base'}}
+Base=1 as Base=1  // expected-error {{cannot assign to immutable expression of type 'Base.Type'}}
 
 
 
diff --git a/test/decl/func/operator.swift b/test/decl/func/operator.swift
index 2c468fab67443..9c12f455bbddd 100644
--- a/test/decl/func/operator.swift
+++ b/test/decl/func/operator.swift
@@ -355,6 +355,7 @@ class C6 {
   static func == (lhs: C6, rhs: C6) -> Bool { return false }
 
   func test1(x: C6) {
-    if x == x && x = x { } // expected-error{{expression is not assignable: '&&' returns immutable value}}
+    // FIXME: Better would be: use of '=' in a boolean context, did you mean '=='?
+    if x == x && x = x { } // expected-error{{cannot convert value of type 'C6' to expected argument type 'Bool'}}
   }
 }
diff --git a/test/decl/init/constructor-kind.swift b/test/decl/init/constructor-kind.swift
index 2434bfbe6b6a9..28c58c9ebe761 100644
--- a/test/decl/init/constructor-kind.swift
+++ b/test/decl/init/constructor-kind.swift
@@ -51,8 +51,8 @@ struct SomeStruct {
   // Delegating initializer
   init() {
     self.init()
-    self.width = Measurement.self.init(val: width) // expected-error {{'let' property 'width' may not be initialized directly; use "self.init(...)" or "self = ..." instead}}
-    self.height = Measurement.self.init(val: height) // expected-error {{'let' property 'height' may not be initialized directly; use "self.init(...)" or "self = ..." instead}}
+    self.width = Measurement.self.init(val: width) // expected-error {{cannot convert value of type 'Measurement' to expected argument type 'Int'}}
+    self.height = Measurement.self.init(val: height) // expected-error {{cannot convert value of type 'Measurement' to expected argument type 'Int'}}
   }
 
   // Designated initializer
@@ -64,14 +64,14 @@ struct SomeStruct {
   // Delegating initializer
   init(width: Int) {
     self.init()
-    self.width = width // expected-error {{'let' property 'width' may not be initialized directly; use "self.init(...)" or "self = ..." instead}}
+    self.width = width // expected-error {{cannot assign value of type 'Int' to type 'Measurement'}}
     self.height = Measurement(val: 20) // expected-error {{'let' property 'height' may not be initialized directly; use "self.init(...)" or "self = ..." instead}}
   }
 
-  // Designated initializer
+  // Delegating initializer
   init(height: Int) {
     self.width = Measurement(val: 10) // expected-error {{'let' property 'width' may not be initialized directly; use "self.init(...)" or "self = ..." instead}}
-    self.height = height // expected-error {{'let' property 'height' may not be initialized directly; use "self.init(...)" or "self = ..." instead}}
+    self.height = height // expected-error {{cannot assign value of type 'Int' to type 'Measurement'}}
     self.init()
   }
 }
diff --git a/test/expr/unary/keypath/keypath.swift b/test/expr/unary/keypath/keypath.swift
index 9a4324cdf25b4..ae6a6bd4a27ce 100644
--- a/test/expr/unary/keypath/keypath.swift
+++ b/test/expr/unary/keypath/keypath.swift
@@ -265,9 +265,9 @@ func testKeyPathSubscript(readonly: Z, writable: inout Z,
   sink = readonly[keyPath: rkp]
   sink = writable[keyPath: rkp]
 
-  readonly[keyPath: kp] = sink // expected-error{{cannot assign to immutable}}
-  writable[keyPath: kp] = sink // expected-error{{cannot assign to immutable}}
-  readonly[keyPath: wkp] = sink // expected-error{{cannot assign to immutable}}
+  readonly[keyPath: kp] = sink // expected-error{{cannot assign through subscript: 'kp' is a read-only key path}}
+  writable[keyPath: kp] = sink // expected-error{{cannot assign through subscript: 'kp' is a read-only key path}}
+  readonly[keyPath: wkp] = sink // expected-error{{cannot assign through subscript: 'readonly' is a 'let' constant}}
   writable[keyPath: wkp] = sink
   readonly[keyPath: rkp] = sink
   writable[keyPath: rkp] = sink
@@ -279,8 +279,8 @@ func testKeyPathSubscript(readonly: Z, writable: inout Z,
   var anySink2 = writable[keyPath: pkp]
   expect(&anySink2, toHaveType: Exactly<Any>.self)
 
-  readonly[keyPath: pkp] = anySink1 // expected-error{{cannot assign to immutable}}
-  writable[keyPath: pkp] = anySink2 // expected-error{{cannot assign to immutable}}
+  readonly[keyPath: pkp] = anySink1 // expected-error{{cannot assign through subscript: 'readonly' is a 'let' constant}}
+  writable[keyPath: pkp] = anySink2 // expected-error{{cannot assign through subscript: 'writable' is immutable}}
 
   let akp: AnyKeyPath = pkp
 
@@ -289,8 +289,8 @@ func testKeyPathSubscript(readonly: Z, writable: inout Z,
   var anyqSink2 = writable[keyPath: akp]
   expect(&anyqSink2, toHaveType: Exactly<Any?>.self)
 
-  readonly[keyPath: akp] = anyqSink1 // expected-error{{cannot assign to immutable}}
-  writable[keyPath: akp] = anyqSink2 // expected-error{{cannot assign to immutable}}
+  readonly[keyPath: akp] = anyqSink1 // expected-error{{cannot assign through subscript: 'readonly' is a 'let' constant}}
+  writable[keyPath: akp] = anyqSink2 // expected-error{{cannot assign through subscript: 'writable' is immutable}}
 }
 
 struct ZwithSubscript {
@@ -314,10 +314,9 @@ func testKeyPathSubscript(readonly: ZwithSubscript, writable: inout ZwithSubscri
   sink = writable[keyPath: wkp]
   sink = readonly[keyPath: rkp]
   sink = writable[keyPath: rkp]
-
-  readonly[keyPath: kp] = sink // expected-error{{cannot assign through subscript: subscript is get-only}}
-  writable[keyPath: kp] = sink // expected-error{{cannot assign through subscript: subscript is get-only}}
-  readonly[keyPath: wkp] = sink // expected-error{{cannot assign through subscript: subscript is get-only}}
+  readonly[keyPath: kp] = sink // expected-error{{cannot assign through subscript: 'kp' is a read-only key path}}
+  writable[keyPath: kp] = sink // expected-error{{cannot assign through subscript: 'kp' is a read-only key path}}
+  readonly[keyPath: wkp] = sink // expected-error{{cannot assign through subscript: 'readonly' is a 'let' constant}}
   // FIXME: silently falls back to keypath application, which seems inconsistent
   writable[keyPath: wkp] = sink
   // FIXME: silently falls back to keypath application, which seems inconsistent
@@ -332,8 +331,8 @@ func testKeyPathSubscript(readonly: ZwithSubscript, writable: inout ZwithSubscri
   var anySink2 = writable[keyPath: pkp]
   expect(&anySink2, toHaveType: Exactly<Any>.self)
 
-  readonly[keyPath: pkp] = anySink1 // expected-error{{cannot assign through subscript: subscript is get-only}}
-  writable[keyPath: pkp] = anySink2 // expected-error{{cannot assign through subscript: subscript is get-only}}
+  readonly[keyPath: pkp] = anySink1 // expected-error{{cannot assign through subscript: 'readonly' is a 'let' constant}}
+  writable[keyPath: pkp] = anySink2 // expected-error{{cannot assign through subscript: 'writable' is immutable}}
 
   let akp: AnyKeyPath = pkp
 
@@ -343,9 +342,9 @@ func testKeyPathSubscript(readonly: ZwithSubscript, writable: inout ZwithSubscri
   expect(&anyqSink2, toHaveType: Exactly<Any?>.self)
 
   // FIXME: silently falls back to keypath application, which seems inconsistent
-  readonly[keyPath: akp] = anyqSink1 // expected-error{{cannot assign to immutable}}
+  readonly[keyPath: akp] = anyqSink1 // expected-error{{cannot assign through subscript: 'readonly' is a 'let' constant}}
   // FIXME: silently falls back to keypath application, which seems inconsistent
-  writable[keyPath: akp] = anyqSink2 // expected-error{{cannot assign to immutable}}
+  writable[keyPath: akp] = anyqSink2 // expected-error{{cannot assign through subscript: 'writable' is immutable}}
 
   _ = wrongType[keyPath: kp] // expected-error{{cannot be applied}}
   _ = wrongType[keyPath: wkp] // expected-error{{cannot be applied}}
@@ -366,9 +365,9 @@ func testKeyPathSubscriptMetatype(readonly: Z.Type, writable: inout Z.Type,
   sink = readonly[keyPath: rkp]
   sink = writable[keyPath: rkp]
 
-  readonly[keyPath: kp] = sink // expected-error{{cannot assign to immutable}}
-  writable[keyPath: kp] = sink // expected-error{{cannot assign to immutable}}
-  readonly[keyPath: wkp] = sink // expected-error{{cannot assign to immutable}}
+  readonly[keyPath: kp] = sink // expected-error{{cannot assign through subscript: 'kp' is a read-only key path}}
+  writable[keyPath: kp] = sink // expected-error{{cannot assign through subscript: 'kp' is a read-only key path}}
+  readonly[keyPath: wkp] = sink // expected-error{{cannot assign through subscript: 'readonly' is a 'let' constant}}
   writable[keyPath: wkp] = sink
   readonly[keyPath: rkp] = sink
   writable[keyPath: rkp] = sink
@@ -386,9 +385,9 @@ func testKeyPathSubscriptTuple(readonly: (Z,Z), writable: inout (Z,Z),
   sink = readonly[keyPath: rkp]
   sink = writable[keyPath: rkp]
 
-  readonly[keyPath: kp] = sink // expected-error{{cannot assign to immutable}}
-  writable[keyPath: kp] = sink // expected-error{{cannot assign to immutable}}
-  readonly[keyPath: wkp] = sink // expected-error{{cannot assign to immutable}}
+  readonly[keyPath: kp] = sink // expected-error{{cannot assign through subscript: 'kp' is a read-only key path}}
+  writable[keyPath: kp] = sink // expected-error{{cannot assign through subscript: 'kp' is a read-only key path}}
+  readonly[keyPath: wkp] = sink // expected-error{{cannot assign through subscript: 'readonly' is a 'let' constant}}
   writable[keyPath: wkp] = sink
   readonly[keyPath: rkp] = sink
   writable[keyPath: rkp] = sink

From 31ca8db0f49f496fba3dc8faa4fada4e53ea1d1f Mon Sep 17 00:00:00 2001
From: gregomni <gregomni@gmail.com>
Date: Sat, 25 Aug 2018 11:40:28 -0700
Subject: [PATCH 2/3] Move all shouldRecordFix logic back into CS itself keying
 off of FixKind.

---
 lib/Sema/CSFix.cpp      | 16 ---------------
 lib/Sema/CSFix.h        |  9 ---------
 lib/Sema/CSSimplify.cpp | 45 +++++++++++++++++++++++++++++------------
 3 files changed, 32 insertions(+), 38 deletions(-)

diff --git a/lib/Sema/CSFix.cpp b/lib/Sema/CSFix.cpp
index e44fe1138853d..311265eadb9c5 100644
--- a/lib/Sema/CSFix.cpp
+++ b/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);
diff --git a/lib/Sema/CSFix.h b/lib/Sema/CSFix.h
index 4b049bb84d886..28b5318cdaee3 100644
--- a/lib/Sema/CSFix.h
+++ b/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,
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
index eb584bc74f9a0..4f6fe6ccc96b0 100644
--- a/lib/Sema/CSSimplify.cpp
+++ b/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;
 }
 

From 6d5a69c89b451fd113442698ff7c51a8268dafd1 Mon Sep 17 00:00:00 2001
From: gregomni <gregomni@gmail.com>
Date: Sat, 25 Aug 2018 12:46:09 -0700
Subject: [PATCH 3/3] Remove excess namespacing

---
 lib/Sema/CSSimplify.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
index 4f6fe6ccc96b0..d96d9dda77736 100644
--- a/lib/Sema/CSSimplify.cpp
+++ b/lib/Sema/CSSimplify.cpp
@@ -4989,7 +4989,7 @@ ConstraintSystem::simplifyRestrictedConstraint(
 
 static bool isAugmentingFix(ConstraintFix *fix) {
   switch (fix->getKind()) {
-    case swift::constraints::FixKind::TreatRValueAsLValue:
+    case FixKind::TreatRValueAsLValue:
       return false;
     default:
       return true;