Merge pull request #24308 from xedin/improve-contextual-diags-for-keypath

[TypeChecker] Improve contextual mismatch diagnostics for key path

Author: xedin

lib/Sema/CSDiagnostics.cpp

@@ -1371,6 +1371,14 @@ bool ContextualFailure::diagnoseAsError() {
     break;
   }
 
+  case ConstraintLocator::ContextualType: {
+    if (isKnownKeyPathType(FromType) && isKnownKeyPathType(ToType)) {
+      diagnostic = diag::cannot_convert_initializer_value;
+      break;
+    }
+
+    LLVM_FALLTHROUGH;
+  }
   default:
     return false;
   }

lib/Sema/CSFix.cpp

@@ -477,3 +477,10 @@ AllowInvalidRefInKeyPath::create(ConstraintSystem &cs, RefKind kind,
   return new (cs.getAllocator())
       AllowInvalidRefInKeyPath(cs, kind, member, locator);
 }
+
+KeyPathContextualMismatch *
+KeyPathContextualMismatch::create(ConstraintSystem &cs, Type lhs, Type rhs,
+                                  ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      KeyPathContextualMismatch(cs, lhs, rhs, locator);
+}

lib/Sema/CSFix.h

@@ -458,6 +458,31 @@ class ContextualMismatch : public ConstraintFix {
                                     ConstraintLocator *locator);
 };
 
+/// Detect situations where key path doesn't have capability required
+/// by the context e.g. read-only vs. writable, or either root or value
+/// types are incorrect e.g.
+///
+/// ```swift
+/// struct S { let foo: Int }
+/// let _: WritableKeyPath<S, Int> = \.foo
+/// ```
+///
+/// Here context requires a writable key path but `foo` property is
+/// read-only.
+class KeyPathContextualMismatch final : public ContextualMismatch {
+  KeyPathContextualMismatch(ConstraintSystem &cs, Type lhs, Type rhs,
+                            ConstraintLocator *locator)
+      : ContextualMismatch(cs, lhs, rhs, locator) {}
+
+public:
+  std::string getName() const override {
+    return "fix key path contextual mismatch";
+  }
+
+  static KeyPathContextualMismatch *
+  create(ConstraintSystem &cs, Type lhs, Type rhs, ConstraintLocator *locator);
+};
+
 /// Detect situations when argument of the @autoclosure parameter is itself
 /// marked as @autoclosure and is not applied. Form a fix which suggests a
 /// proper way to forward such arguments, e.g.:

lib/Sema/CSSimplify.cpp

@@ -2066,6 +2066,14 @@ repairFailures(ConstraintSystem &cs, Type lhs, Type rhs,
   }
 
   case ConstraintLocator::ContextualType: {
+    // If both types are key path, the only differences
+    // between them are mutability and/or root, value type mismatch.
+    if (isKnownKeyPathType(lhs) && isKnownKeyPathType(rhs)) {
+      auto *fix = KeyPathContextualMismatch::create(
+          cs, lhs, rhs, cs.getConstraintLocator(locator));
+      conversionsOrFixes.push_back(fix);
+    }
+
     if (lhs->is<FunctionType>() && !rhs->is<AnyFunctionType>() &&
         isa<ClosureExpr>(anchor)) {
       auto *fix = ContextualMismatch::create(cs, lhs, rhs,
@@ -5230,8 +5238,10 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
 
   auto resolvedKPTy = BoundGenericType::get(kpDecl, nullptr,
                                             {rootTy, valueTy});
-  return matchTypes(resolvedKPTy, keyPathTy, ConstraintKind::Bind,
-                    subflags, locator);
+  // Let's check whether deduced key path type would match
+  // expected contextual one.
+  return matchTypes(resolvedKPTy, keyPathTy, ConstraintKind::Bind, subflags,
+                    locator.withPathElement(ConstraintLocator::ContextualType));
 }
 
 ConstraintSystem::SolutionKind

lib/Sema/ConstraintSystem.cpp

@@ -2709,6 +2709,12 @@ void ConstraintSystem::generateConstraints(
   }
 }
 
+bool constraints::isKnownKeyPathType(Type type) {
+  if (auto *BGT = type->getAs<BoundGenericType>())
+    return isKnownKeyPathDecl(type->getASTContext(), BGT->getDecl());
+  return false;
+}
+
 bool constraints::isKnownKeyPathDecl(ASTContext &ctx, ValueDecl *decl) {
   return decl == ctx.getKeyPathDecl() || decl == ctx.getWritableKeyPathDecl() ||
          decl == ctx.getReferenceWritableKeyPathDecl() ||

lib/Sema/ConstraintSystem.h

@@ -4011,6 +4011,10 @@ class DisjunctionChoiceProducer : public BindingProducer<DisjunctionChoice> {
   }
 };
 
+/// Determine whether given type is a known one
+/// for a key path `{Writable, ReferenceWritable}KeyPath`.
+bool isKnownKeyPathType(Type type);
+
 /// Determine whether given declaration is one for a key path
 /// `{Writable, ReferenceWritable}KeyPath`.
 bool isKnownKeyPathDecl(ASTContext &ctx, ValueDecl *decl);

test/Constraints/keypath_swift_5.swift

@@ -4,15 +4,15 @@ struct S {
   let i: Int
 
   init() {
-    let _: WritableKeyPath<S, Int> = \.i // expected-error {{type of expression is ambiguous without more context}}
+    let _: WritableKeyPath<S, Int> = \.i // expected-error {{cannot convert value of type 'KeyPath<S, Int>' to specified type 'WritableKeyPath<S, Int>'}}
 
     S()[keyPath: \.i] = 1
     // expected-error@-1 {{cannot assign through subscript: immutable key path}}
   }
 }
 
 func test() {
-  let _: WritableKeyPath<C, Int> = \.i // expected-error {{type of expression is ambiguous without more context}}
+  let _: WritableKeyPath<C, Int> = \.i // expected-error {{cannot convert value of type 'KeyPath<C, Int>' to specified type 'WritableKeyPath<C, Int>'}}
 
   C()[keyPath: \.i] = 1
   // expected-error@-1 {{cannot assign through subscript: immutable key path}}

test/expr/unary/keypath/keypath.swift

@@ -125,16 +125,16 @@ func testKeyPath(sub: Sub, optSub: OptSub,
   let _: PartialKeyPath<A> = \.property
   let _: KeyPath<A, Prop> = \.property
   let _: WritableKeyPath<A, Prop> = \.property
-  // expected-error@+1{{ambiguous}} (need to improve diagnostic)
   let _: ReferenceWritableKeyPath<A, Prop> = \.property
+  //expected-error@-1 {{cannot convert value of type 'WritableKeyPath<A, Prop>' to specified type 'ReferenceWritableKeyPath<A, Prop>'}}
 
   // FIXME: shouldn't be ambiguous
   // expected-error@+1{{ambiguous}}
   let _: PartialKeyPath<A> = \.[sub]
   let _: KeyPath<A, A> = \.[sub]
   let _: WritableKeyPath<A, A> = \.[sub]
-  // expected-error@+1{{ambiguous}} (need to improve diagnostic)
   let _: ReferenceWritableKeyPath<A, A> = \.[sub]
+  // expected-error@-1 {{cannot convert value of type 'WritableKeyPath<A, A>' to specified type 'ReferenceWritableKeyPath<A, A>}}
 
   let _: PartialKeyPath<A> = \.optProperty?
   let _: KeyPath<A, Prop?> = \.optProperty?
@@ -158,8 +158,8 @@ func testKeyPath(sub: Sub, optSub: OptSub,
   let _: PartialKeyPath<C<A>> = \.value
   let _: KeyPath<C<A>, A> = \.value
   let _: WritableKeyPath<C<A>, A> = \.value
-  // expected-error@+1{{ambiguous}} (need to improve diagnostic)
   let _: ReferenceWritableKeyPath<C<A>, A> = \.value
+  // expected-error@-1 {{cannot convert value of type 'WritableKeyPath<C<A>, A>' to specified type 'ReferenceWritableKeyPath<C<A>, A>'}}
 
   let _: PartialKeyPath<C<A>> = \C.value
   let _: KeyPath<C<A>, A> = \C.value
@@ -684,7 +684,8 @@ func testSubtypeKeypathClass(_ keyPath: ReferenceWritableKeyPath<Base, Int>) {
 }
 
 func testSubtypeKeypathProtocol(_ keyPath: ReferenceWritableKeyPath<PP, Int>) {
-  testSubtypeKeypathProtocol(\Base.i) // expected-error {{type 'PP' has no member 'i'}}
+  testSubtypeKeypathProtocol(\Base.i)
+  // expected-error@-1 {{cannot convert value of type 'ReferenceWritableKeyPath<Base, Int>' to specified type 'ReferenceWritableKeyPath<PP, Int>'}}
 }
 
 // rdar://problem/32057712