Skip to content

[TypeChecker] Improve contextual mismatch diagnostics for key path #24308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
7 changes: 7 additions & 0 deletions lib/Sema/CSFix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
25 changes: 25 additions & 0 deletions lib/Sema/CSFix.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.:
Expand Down
14 changes: 12 additions & 2 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions lib/Sema/ConstraintSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() ||
Expand Down
4 changes: 4 additions & 0 deletions lib/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions test/Constraints/keypath_swift_5.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Expand Down
9 changes: 5 additions & 4 deletions test/expr/unary/keypath/keypath.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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>'}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this test file puts the assertions on the same line or the line before; I think putting these on the line after might confuse future maintainers.

(This applies to the other changes in this file, too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's going to be a big problem especially considering that most of the tests (if not all) have expected checks on the same or line after the one checked. These TODOs where more of an exception to the rule.


// 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?
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down