Skip to content

[CS] Fix key path dynamic member IUOs #28562

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.

Sign up for GitHub

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 4 commits into from
Dec 4, 2019
Merged

[CS] Fix key path dynamic member IUOs #28562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92a6302
[CS] Don't throw away unresolved key path components
hamishknight Dec 4, 2019
c1849ba
[CS] Refactor visitKeyPathExpr a little
hamishknight Dec 4, 2019
ef6a240
[CS] Fix key path dynamic member IUOs
hamishknight Dec 4, 2019
d328b1b
[CS] Correctly set key path component types
hamishknight Dec 4, 2019
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
191 changes: 83 additions & 108 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,7 @@ namespace {
auto &ctx = cs.getASTContext();
auto *anchor = memberLoc->getAnchor();

KeyPathExpr::Component component;
SmallVector<KeyPathExpr::Component, 2> components;

// Let's create a KeyPath expression and fill in "parsed path"
// after component is built.
Expand All @@ -1546,10 +1546,11 @@ namespace {
// calls necessary to resolve a member reference.
if (overload.choice.getKind() ==
OverloadChoiceKind::KeyPathDynamicMemberLookup) {
keyPath->resolveComponents(ctx,
buildKeyPathSubscriptComponent(
overload, dotLoc, /*indexExpr=*/nullptr,
ctx.Id_dynamicMember, componentLoc));
buildKeyPathSubscriptComponent(overload, dotLoc, /*indexExpr=*/nullptr,
ctx.Id_dynamicMember, componentLoc,
components);
keyPath->resolveComponents(ctx, components);
cs.cacheExprTypes(keyPath);
return keyPath;
}

Expand Down Expand Up @@ -1599,8 +1600,8 @@ namespace {
UDE->getNameLoc(),
/*Implicit=*/true);

component = buildKeyPathPropertyComponent(overload, UDE->getLoc(),
componentLoc);
buildKeyPathPropertyComponent(overload, UDE->getLoc(), componentLoc,
components);
} else if (auto *SE = dyn_cast<SubscriptExpr>(anchor)) {
componentExpr = SE;
// If this is not for a keypath component, we have to copy
Expand Down Expand Up @@ -1628,9 +1629,9 @@ namespace {
/*implicit=*/true, SE->getAccessSemantics());
}

component = buildKeyPathSubscriptComponent(
overload, SE->getLoc(), SE->getIndex(), SE->getArgumentLabels(),
componentLoc);
buildKeyPathSubscriptComponent(overload, SE->getLoc(), SE->getIndex(),
SE->getArgumentLabels(), componentLoc,
components);
} else {
return nullptr;
}
Expand All @@ -1640,10 +1641,9 @@ namespace {
componentExpr->setType(ty);
cs.cacheType(componentExpr);

cs.setType(keyPath, 0, ty);

keyPath->setParsedPath(componentExpr);
keyPath->resolveComponents(ctx, {component});
keyPath->resolveComponents(ctx, components);
cs.cacheExprTypes(keyPath);
return keyPath;
}

Expand Down Expand Up @@ -4181,13 +4181,6 @@ namespace {
leafTy = keyPathTy->getGenericArgs()[1];
}

// Updates the constraint system with the type of the last resolved
// component. We do it this way because we sometimes insert new
// components.
auto updateCSWithResolvedComponent = [&]() {
cs.setType(E, resolvedComponents.size() - 1, baseTy);
};

for (unsigned i : indices(E->getComponents())) {
auto &origComponent = E->getMutableComponents()[i];

Expand All @@ -4197,24 +4190,6 @@ namespace {
resolvedComponents.push_back(origComponent);
continue;
}

auto getObjectType = [](Type optionalTy) -> Type {
Type objectTy;
if (auto lvalue = optionalTy->getAs<LValueType>()) {
objectTy = lvalue->getObjectType()->getOptionalObjectType();
if (optionalTy->hasUnresolvedType() && !objectTy) {
objectTy = optionalTy;
}
objectTy = LValueType::get(objectTy);
} else {
objectTy = optionalTy->getOptionalObjectType();
if (optionalTy->hasUnresolvedType() && !objectTy) {
objectTy = optionalTy;
}
}
assert(objectTy);
return objectTy;
};

auto kind = origComponent.getKind();
Optional<SelectedOverload> foundDecl;
Expand Down Expand Up @@ -4248,57 +4223,32 @@ namespace {
}
}

KeyPathExpr::Component component;
switch (kind) {
case KeyPathExpr::Component::Kind::UnresolvedProperty: {
// If we couldn't resolve the component, leave it alone.
if (!foundDecl) {
component = origComponent;
resolvedComponents.push_back(origComponent);
break;
}

component = buildKeyPathPropertyComponent(
*foundDecl, origComponent.getLoc(), locator);

baseTy = component.getComponentType();
resolvedComponents.push_back(component);

if (shouldForceUnwrapResult(foundDecl->choice, locator)) {
updateCSWithResolvedComponent();
auto objectTy = getObjectType(baseTy);
auto loc = origComponent.getLoc();
component = KeyPathExpr::Component::forOptionalForce(objectTy, loc);
baseTy = component.getComponentType();
resolvedComponents.push_back(component);
}
buildKeyPathPropertyComponent(*foundDecl, origComponent.getLoc(),
locator, resolvedComponents);
break;
}
case KeyPathExpr::Component::Kind::UnresolvedSubscript: {
// Leave the component unresolved if the overload was not resolved.
if (!foundDecl) {
component = origComponent;
resolvedComponents.push_back(origComponent);
break;
}

ArrayRef<Identifier> subscriptLabels;
if (!isDynamicMember)
subscriptLabels = origComponent.getSubscriptLabels();

component = buildKeyPathSubscriptComponent(
buildKeyPathSubscriptComponent(
*foundDecl, origComponent.getLoc(), origComponent.getIndexExpr(),
subscriptLabels, locator);

baseTy = component.getComponentType();
resolvedComponents.push_back(component);

if (shouldForceUnwrapResult(foundDecl->choice, locator)) {
updateCSWithResolvedComponent();
auto objectTy = getObjectType(baseTy);
auto loc = origComponent.getLoc();
component = KeyPathExpr::Component::forOptionalForce(objectTy, loc);
baseTy = component.getComponentType();
resolvedComponents.push_back(component);
}
subscriptLabels, locator, resolvedComponents);
break;
}
case KeyPathExpr::Component::Kind::OptionalChain: {
Expand All @@ -4312,41 +4262,35 @@ namespace {
assert(objectTy);

auto loc = origComponent.getLoc();
component = KeyPathExpr::Component::forOptionalChain(objectTy, loc);

baseTy = component.getComponentType();
resolvedComponents.push_back(component);
resolvedComponents.push_back(
KeyPathExpr::Component::forOptionalChain(objectTy, loc));
break;
}
case KeyPathExpr::Component::Kind::OptionalForce: {
auto objectTy = getObjectType(baseTy);
auto loc = origComponent.getLoc();
component = KeyPathExpr::Component::forOptionalForce(objectTy, loc);
baseTy = component.getComponentType();
resolvedComponents.push_back(component);
case KeyPathExpr::Component::Kind::OptionalForce:
buildKeyPathOptionalForceComponent(resolvedComponents);
break;
}
case KeyPathExpr::Component::Kind::Invalid:
component = origComponent;
case KeyPathExpr::Component::Kind::Invalid: {
auto component = origComponent;
component.setComponentType(leafTy);

baseTy = component.getComponentType();
resolvedComponents.push_back(component);
break;
case KeyPathExpr::Component::Kind::Identity:
component = origComponent;
}
case KeyPathExpr::Component::Kind::Identity: {
auto component = origComponent;
component.setComponentType(baseTy);
resolvedComponents.push_back(component);
break;
}
case KeyPathExpr::Component::Kind::Property:
case KeyPathExpr::Component::Kind::Subscript:
case KeyPathExpr::Component::Kind::OptionalWrap:
case KeyPathExpr::Component::Kind::TupleElement:
llvm_unreachable("already resolved");
}

// By now, "baseTy" is the result type of this component.
updateCSWithResolvedComponent();
// Update "baseTy" with the result type of the last component.
assert(!resolvedComponents.empty());
baseTy = resolvedComponents.back().getComponentType();
}

// Wrap a non-optional result if there was chaining involved.
Expand All @@ -4359,10 +4303,12 @@ namespace {
auto component = KeyPathExpr::Component::forOptionalWrap(leafTy);
resolvedComponents.push_back(component);
baseTy = leafTy;
updateCSWithResolvedComponent();
}

// Set the resolved components, and cache their types.
E->resolveComponents(cs.getASTContext(), resolvedComponents);

cs.cacheExprTypes(E);

// See whether there's an equivalent ObjC key path string we can produce
// for interop purposes.
if (cs.getASTContext().LangOpts.EnableObjCInterop) {
Expand Down Expand Up @@ -4481,10 +4427,37 @@ namespace {
return coerceToType(outerApply, exprType, cs.getConstraintLocator(E));
}

KeyPathExpr::Component
buildKeyPathPropertyComponent(const SelectedOverload &overload,
SourceLoc componentLoc,
ConstraintLocator *locator) {
void buildKeyPathOptionalForceComponent(
SmallVectorImpl<KeyPathExpr::Component> &components) {
assert(!components.empty());

// Unwrap the last component type, preserving @lvalue-ness.
auto optionalTy = components.back().getComponentType();
Type objectTy;
if (auto lvalue = optionalTy->getAs<LValueType>()) {
objectTy = lvalue->getObjectType()->getOptionalObjectType();
if (optionalTy->hasUnresolvedType() && !objectTy) {
objectTy = optionalTy;
}
objectTy = LValueType::get(objectTy);
} else {
objectTy = optionalTy->getOptionalObjectType();
if (optionalTy->hasUnresolvedType() && !objectTy) {
objectTy = optionalTy;
}
}
assert(objectTy);

auto loc = components.back().getLoc();
components.push_back(
KeyPathExpr::Component::forOptionalForce(objectTy, loc));
}

void buildKeyPathPropertyComponent(
const SelectedOverload &overload, SourceLoc componentLoc,
ConstraintLocator *locator,
SmallVectorImpl<KeyPathExpr::Component> &components) {
auto resolvedTy = simplifyType(overload.openedType);
if (auto *property = overload.choice.getDeclOrNull()) {
// Key paths can only refer to properties currently.
auto varDecl = cast<VarDecl>(property);
Expand All @@ -4494,26 +4467,24 @@ namespace {
// There is a fix which diagnoses such situation already.
assert(!varDecl->isStatic());

auto resolvedTy = overload.openedType;
resolvedTy = simplifyType(resolvedTy);

// Compute the concrete reference to the member.
auto ref = resolveConcreteDeclRef(property, locator);
return KeyPathExpr::Component::forProperty(ref, resolvedTy,
componentLoc);
components.push_back(
KeyPathExpr::Component::forProperty(ref, resolvedTy, componentLoc));
} else {
auto fieldIndex = overload.choice.getTupleIndex();
components.push_back(KeyPathExpr::Component::forTupleElement(
fieldIndex, resolvedTy, componentLoc));
}

auto fieldIndex = overload.choice.getTupleIndex();
auto resolvedTy = overload.openedType;
resolvedTy = simplifyType(resolvedTy);

return KeyPathExpr::Component::forTupleElement(fieldIndex, resolvedTy,
componentLoc);
if (shouldForceUnwrapResult(overload.choice, locator))
buildKeyPathOptionalForceComponent(components);
}

KeyPathExpr::Component buildKeyPathSubscriptComponent(
void buildKeyPathSubscriptComponent(
SelectedOverload &overload, SourceLoc componentLoc, Expr *indexExpr,
ArrayRef<Identifier> labels, ConstraintLocator *locator) {
ArrayRef<Identifier> labels, ConstraintLocator *locator,
SmallVectorImpl<KeyPathExpr::Component> &components) {
auto subscript = cast<SubscriptDecl>(overload.choice.getDecl());
assert(!subscript->isGetterMutating());

Expand Down Expand Up @@ -4583,10 +4554,14 @@ namespace {
conformances.push_back(hashableConformance);
}

return KeyPathExpr::Component::forSubscriptWithPrebuiltIndexExpr(
auto comp = KeyPathExpr::Component::forSubscriptWithPrebuiltIndexExpr(
ref, newIndexExpr, cs.getASTContext().AllocateCopy(newLabels),
resolvedTy, componentLoc,
cs.getASTContext().AllocateCopy(conformances));
components.push_back(comp);

if (shouldForceUnwrapResult(overload.choice, locator))
buildKeyPathOptionalForceComponent(components);
}

Expr *visitKeyPathDotExpr(KeyPathDotExpr *E) {
Expand Down
29 changes: 29 additions & 0 deletions test/Constraints/keypath_dynamic_member_lookup.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,3 +390,32 @@ func test_constrained_ext_vs_dynamic_member() {
// CHECK: function_ref @$s29keypath_dynamic_member_lookup8SR_11465VAASHRzlE9hashValueSivg
_ = SR_11465<Int>(rawValue: 1).hashValue // Ok, keep choice from constrained extension
}

// SR-11893: Make sure we properly handle IUO unwraps for key path dynamic members.
struct SR_11893_Base {
var i: Int!
subscript(_ x: Int) -> Int! { x }
}

@dynamicMemberLookup
struct SR_11893 {
subscript(dynamicMember kp: KeyPath<SR_11893_Base, Int>) -> Void { () }
}

// CHECK-LABEL: sil hidden @$s29keypath_dynamic_member_lookup13testIUOUnwrapyyAA8SR_11893VF : $@convention(thin) (SR_11893) -> ()
func testIUOUnwrap(_ x: SR_11893) {
// CHECK: keypath $KeyPath<SR_11893_Base, Int>, (root $SR_11893_Base; stored_property #SR_11893_Base.i : $Optional<Int>; optional_force : $Int)
x.i

// CHECK: keypath $KeyPath<SR_11893_Base, Int>, (root $SR_11893_Base; gettable_property $Optional<Int>, id @$s29keypath_dynamic_member_lookup13SR_11893_BaseVySiSgSicig : $@convention(method) (Int, SR_11893_Base) -> Optional<Int>, getter @$s29keypath_dynamic_member_lookup13SR_11893_BaseVySiSgSicipACTK : $@convention(thin) (@in_guaranteed SR_11893_Base, UnsafeRawPointer) -> @out Optional<Int>, indices [%$0 : $Int : $Int], indices_equals @$sSiTH : $@convention(thin) (UnsafeRawPointer, UnsafeRawPointer) -> Bool, indices_hash @$sSiTh : $@convention(thin) (UnsafeRawPointer) -> Int; optional_force : $Int)
x[5]

// FIXME(SR-11896): We should not vend a WritableKeyPath for the "outer" key path here.
// CHECK: [[INNER_KP:%[0-9]+]] = keypath $KeyPath<SR_11893_Base, Int>, (root $SR_11893_Base; stored_property #SR_11893_Base.i : $Optional<Int>; optional_force : $Int)
// CHECK: keypath $WritableKeyPath<SR_11893, ()>, (root $SR_11893; gettable_property $(), id @$s29keypath_dynamic_member_lookup8SR_11893V0B6Memberys7KeyPathCyAA0E11_11893_BaseVSiG_tcig : $@convention(method) (@guaranteed KeyPath<SR_11893_Base, Int>, SR_11893) -> (), getter @$s29keypath_dynamic_member_lookup8SR_11893V0B6Memberys7KeyPathCyAA0E11_11893_BaseVSiG_tcipACTK : $@convention(thin) (@in_guaranteed SR_11893, UnsafeRawPointer) -> @out (), indices [%$0 : $KeyPath<SR_11893_Base, Int> : $KeyPath<SR_11893_Base, Int>], indices_equals @$ss7KeyPathCy29keypath_dynamic_member_lookup13SR_11893_BaseVSiGTH : $@convention(thin) (UnsafeRawPointer, UnsafeRawPointer) -> Bool, indices_hash @$ss7KeyPathCy29keypath_dynamic_member_lookup13SR_11893_BaseVSiGTh : $@convention(thin) (UnsafeRawPointer) -> Int) ([[INNER_KP]])
_ = \SR_11893.i

// CHECK: [[INNER_SUB_KP:%[0-9]+]] = keypath $KeyPath<SR_11893_Base, Int>, (root $SR_11893_Base; gettable_property $Optional<Int>, id @$s29keypath_dynamic_member_lookup13SR_11893_BaseVySiSgSicig : $@convention(method) (Int, SR_11893_Base) -> Optional<Int>, getter @$s29keypath_dynamic_member_lookup13SR_11893_BaseVySiSgSicipACTK : $@convention(thin) (@in_guaranteed SR_11893_Base, UnsafeRawPointer) -> @out Optional<Int>, indices [%$0 : $Int : $Int], indices_equals @$sSiTH : $@convention(thin) (UnsafeRawPointer, UnsafeRawPointer) -> Bool, indices_hash @$sSiTh : $@convention(thin) (UnsafeRawPointer) -> Int; optional_force : $Int)
// CHECK: keypath $KeyPath<SR_11893, ()>, (root $SR_11893; gettable_property $(), id @$s29keypath_dynamic_member_lookup8SR_11893V0B6Memberys7KeyPathCyAA0E11_11893_BaseVSiG_tcig : $@convention(method) (@guaranteed KeyPath<SR_11893_Base, Int>, SR_11893) -> (), getter @$s29keypath_dynamic_member_lookup8SR_11893V0B6Memberys7KeyPathCyAA0E11_11893_BaseVSiG_tcipACTK : $@convention(thin) (@in_guaranteed SR_11893, UnsafeRawPointer) -> @out (), indices [%$0 : $KeyPath<SR_11893_Base, Int> : $KeyPath<SR_11893_Base, Int>], indices_equals @$ss7KeyPathCy29keypath_dynamic_member_lookup13SR_11893_BaseVSiGTH : $@convention(thin) (UnsafeRawPointer, UnsafeRawPointer) -> Bool, indices_hash @$ss7KeyPathCy29keypath_dynamic_member_lookup13SR_11893_BaseVSiGTh : $@convention(thin) (UnsafeRawPointer) -> Int) ([[INNER_SUB_KP]])
_ = \SR_11893.[5]
}