Merge pull request #10870 from jckarter/key-path-failure-diagnosis-with-resolved-components

Sema: Allow KeyPath and KeyPathApplicationExprs to re-type-check during failure diagnosis.

Author: jckarter

lib/Sema/CSApply.cpp

@@ -3581,7 +3581,9 @@ namespace {
     }
 
     Expr *visitKeyPathApplicationExpr(KeyPathApplicationExpr *expr){
-      llvm_unreachable("Already type-checked");
+      // This should already be type-checked, but we may have had to re-
+      // check it for failure diagnosis.
+      return simplifyExprType(expr);
     }
 
     Expr *visitEnumIsCaseExpr(EnumIsCaseExpr *expr) {

lib/Sema/CSGen.cpp

@@ -2713,7 +2713,16 @@ namespace {
       llvm_unreachable("Already type-checked");
     }
     Type visitKeyPathApplicationExpr(KeyPathApplicationExpr *expr) {
-      llvm_unreachable("Already type-checked");
+      // This should only appear in already-type-checked solutions, but we may
+      // need to re-check for failure diagnosis.
+      auto locator = CS.getConstraintLocator(expr);
+      auto projectedTy = CS.createTypeVariable(locator,
+                                               TVO_CanBindToLValue);
+      CS.addKeyPathApplicationConstraint(expr->getKeyPath()->getType(),
+                                         expr->getBase()->getType(),
+                                         projectedTy,
+                                         locator);
+      return projectedTy;
     }
 
     Type visitEnumIsCaseExpr(EnumIsCaseExpr *expr) {
@@ -2797,19 +2806,6 @@ namespace {
                          locator);
       }
 
-      // If a component is already resolved, then all of them should be
-      // resolved, and we can let the expression be. This might happen when
-      // re-checking a failed system for diagnostics.
-      if (E->getComponents().front().isResolved()) {
-        assert([&]{
-          for (auto &c : E->getComponents())
-            if (!c.isResolved())
-              return false;
-          return true;
-        }());
-        return E->getType();
-      }
-      
       bool didOptionalChain = false;
       // We start optimistically from an lvalue base.
       Type base = LValueType::get(root);
@@ -2820,16 +2816,23 @@ namespace {
         case KeyPathExpr::Component::Kind::Invalid:
           break;
 
-        case KeyPathExpr::Component::Kind::UnresolvedProperty: {
+        case KeyPathExpr::Component::Kind::UnresolvedProperty:
+        // This should only appear in resolved ASTs, but we may need to
+        // re-type-check the constraints during failure diagnosis.
+        case KeyPathExpr::Component::Kind::Property: {
           auto memberTy = CS.createTypeVariable(locator,
                                                 TVO_CanBindToLValue |
                                                 TVO_CanBindToInOut);
-          auto refKind = component.getUnresolvedDeclName().isSimpleName()
+          auto lookupName = kind == KeyPathExpr::Component::Kind::UnresolvedProperty
+            ? component.getUnresolvedDeclName()
+            : component.getDeclRef().getDecl()->getFullName();
+          
+          auto refKind = lookupName.isSimpleName()
             ? FunctionRefKind::Unapplied
             : FunctionRefKind::Compound;
           auto memberLocator = CS.getConstraintLocator(E,
                         ConstraintLocator::PathElement::getKeyPathComponent(i));
-          CS.addValueMemberConstraint(base, component.getUnresolvedDeclName(),
+          CS.addValueMemberConstraint(base, lookupName,
                                       memberTy,
                                       CurDC,
                                       refKind,
@@ -2838,7 +2841,11 @@ namespace {
           break;
         }
 
-        case KeyPathExpr::Component::Kind::UnresolvedSubscript: {
+        case KeyPathExpr::Component::Kind::UnresolvedSubscript:
+        // Subscript should only appear in resolved ASTs, but we may need to
+        // re-type-check the constraints during failure diagnosis.
+        case KeyPathExpr::Component::Kind::Subscript: {
+
           auto memberLocator = CS.getConstraintLocator(E,
                         ConstraintLocator::PathElement::getKeyPathComponent(i));
           base = addSubscriptConstraints(E, base, component.getIndexExpr(),
@@ -2869,11 +2876,13 @@ namespace {
           break;
         }
 
-        case KeyPathExpr::Component::Kind::Property:
-        case KeyPathExpr::Component::Kind::Subscript:
-        case KeyPathExpr::Component::Kind::OptionalWrap:
-          llvm_unreachable("already resolved");
-        }        
+        case KeyPathExpr::Component::Kind::OptionalWrap: {
+          // This should only appear in resolved ASTs, but we may need to
+          // re-type-check the constraints during failure diagnosis.
+          base = OptionalType::get(base);
+          break;
+        }
+        }
       }
 
       // If there was an optional chaining component, the end result must be

lib/Sema/CSSimplify.cpp

@@ -3905,6 +3905,8 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
     case KeyPathExpr::Component::Kind::Invalid:
       break;
 
+    case KeyPathExpr::Component::Kind::Property:
+    case KeyPathExpr::Component::Kind::Subscript:
     case KeyPathExpr::Component::Kind::UnresolvedProperty:
     case KeyPathExpr::Component::Kind::UnresolvedSubscript: {
       // If no choice was made, leave the constraint unsolved.
@@ -3949,10 +3951,11 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
       // Forcing an optional preserves its lvalue-ness.
       break;
 
-    case KeyPathExpr::Component::Kind::Property:
-    case KeyPathExpr::Component::Kind::Subscript:
     case KeyPathExpr::Component::Kind::OptionalWrap:
-      llvm_unreachable("already resolved");
+      // An optional chain should already have forced the entire key path to
+      // be read-only.
+      assert(capability == ReadOnly);
+      break;
     }
   }
 done:

test/expr/unary/keypath/salvage-with-other-type-errors.swift

@@ -27,5 +27,32 @@ struct A {
 }
 
 extension A: K {
-    static let j = S(\A.id + "id")
+    static let j = S(\A.id + "id") // expected-error {{'+' cannot be applied}} expected-note {{}}
+}
+
+// SR-5034
+
+struct B {
+    let v: String
+    func f1<T, E>(block: (T) -> E) -> B {
+        return self
+    }
+
+    func f2<T, E: Equatable>(keyPath: KeyPath<T, E>) {
+    }
+}
+func f3() {
+    B(v: "").f1(block: { _ in }).f2(keyPath: \B.v) // expected-error{{}}
+}
+
+// SR-5375
+
+protocol Bindable: class { }
+
+extension Bindable {
+  func test<Value>(to targetKeyPath: ReferenceWritableKeyPath<Self, Value>, change: Value?) {
+    if self[keyPath:targetKeyPath] != change {  // expected-error{{}}
+      self[keyPath: targetKeyPath] = change!
+    }
+  }
 }