Skip to content

[Type-checker] Improve diagnostic for keypath used without 'keyPath:' label #31716

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
May 13, 2020
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
38 changes: 32 additions & 6 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5674,6 +5674,19 @@ static bool isForKeyPathSubscript(ConstraintSystem &cs,
return false;
}

static bool isForKeyPathSubscriptWithoutLabel(ConstraintSystem &cs,
ConstraintLocator *locator) {
if (!locator || !locator->getAnchor())
return false;

if (auto *SE = getAsExpr<SubscriptExpr>(locator->getAnchor())) {
auto *indexExpr = SE->getIndex();
return isa<ParenExpr>(indexExpr) &&
isa<KeyPathExpr>(indexExpr->getSemanticsProvidingExpr());
}
return false;
}

/// Determine whether all of the given candidate overloads
/// found through conditional conformances of a given base type.
/// This is useful to figure out whether it makes sense to
Expand Down Expand Up @@ -5801,7 +5814,13 @@ performMemberLookup(ConstraintKind constraintKind, DeclNameRef memberName,
MemberLookupResult result;
result.OverallResult = MemberLookupResult::HasResults;

if (isForKeyPathSubscript(*this, memberLocator)) {
// Add key path result.
// If we are including inaccessible members, check for the use of a keypath
// subscript without a `keyPath:` label. Add it to the result so that it
// can be caught by the missing argument label checking later.
if (isForKeyPathSubscript(*this, memberLocator) ||
(isForKeyPathSubscriptWithoutLabel(*this, memberLocator)
&& includeInaccessibleMembers)) {
if (baseTy->isAnyObject()) {
result.addUnviable(
OverloadChoice(baseTy, OverloadChoiceKind::KeyPathApplication),
Expand Down Expand Up @@ -9693,14 +9712,21 @@ ConstraintSystem::addKeyPathApplicationRootConstraint(Type root, ConstraintLocat
path[0].getKind() == ConstraintLocator::SubscriptMember) ||
(path.size() == 2 &&
path[1].getKind() == ConstraintLocator::KeyPathDynamicMember));

auto indexTuple = dyn_cast<TupleExpr>(subscript->getIndex());
if (!indexTuple || indexTuple->getNumElements() != 1)
return;

auto keyPathExpr = dyn_cast<KeyPathExpr>(indexTuple->getElement(0));
auto indexParen = dyn_cast<ParenExpr>(subscript->getIndex());
// If a keypath subscript is used without the expected `keyPath:` label,
// continue with type-checking when attempting fixes so that it gets caught
// by the argument label checking. In such cases, the KeyPathExpr is contained
// in a ParenExpr, instead of a TupleExpr.
assert(((indexTuple && indexTuple->getNumElements() == 1) || indexParen) &&
"Expected KeyPathExpr to be in either TupleExpr or ParenExpr");

auto keyPathExpr = dyn_cast<KeyPathExpr>(
indexTuple ? indexTuple->getElement(0) : indexParen->getSubExpr());
if (!keyPathExpr)
return;

auto typeVar = getType(keyPathExpr)->getAs<TypeVariableType>();
if (!typeVar)
return;
Expand Down
35 changes: 35 additions & 0 deletions test/Sema/keypath_subscript_nolabel.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// RUN: %target-swift-frontend -typecheck -verify -primary-file %s
// [SR-12745]
// rdar://problem/62957095
struct S1 {
var x : Int = 0
}
var s1 = S1()
s1[\.x] = 10 // expected-error {{missing argument label 'keyPath:' in subscript}} {{4-4=keyPath: }}

struct S2 {
var x : Int = 0
subscript(_ v: Int) -> Int { 0 }
}
var s2 = S2()
s2[\.x] = 10 // expected-error {{missing argument label 'keyPath:' in subscript}} {{4-4=keyPath: }}

struct S3 {
var x : Int = 0
subscript(v v: KeyPath<S3, Int>) -> Int { get { 0 } set(newValue) {} }
}
var s3 = S3()
// TODO(diagnostics): This should actually be a diagnostic that correctly identifies that in the presence
// of a missing label, there are two options for resolution: 'keyPath' and 'v:' and to offer the user
// a choice.
// Today, the ExprTypeChecker identifies the disjunction with two of these possibilities, but
// filters out some of the terms based on label mismatch (but not implicit keypath terms, for example).
// It should probably not do that.
s3[\.x] = 10 // expected-error {{missing argument label 'keyPath:' in subscript}} {{4-4=keyPath: }}

struct S4 {
var x : Int = 0
subscript(v: KeyPath<S4, String>) -> Int { get { 0 } set(newValue) {} }
}
var s4 = S4()
s4[\.x] = 10 // expected-error {{key path value type 'Int' cannot be converted to contextual type 'String'}}