Skip to content

Commit ef6a240

Browse files
hamishknighthamishknight
committed
[CS] Fix key path dynamic member IUOs
Previously we were only handling IUO unwrapping in visitKeyPathExpr, but not for callers that were building their own property and subscript components such as dynamic member lookup. Move the IUO unwrapping logic into buildKeyPath[Property/Subscript]Component, and adjust their signatures to allow them to append multiple components. Also add buildKeyPathOptionalForceComponent to allow more convenient adding of an optional force component. Resolves SR-11893.
1 parent c1849ba commit ef6a240

File tree

2 files changed

+93
-74
lines changed

2 files changed

+93
-74
lines changed

‎lib/Sema/CSApply.cpp

Lines changed: 64 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -1524,7 +1524,7 @@ namespace {
15241524
auto &ctx = cs.getASTContext();
15251525
auto *anchor = memberLoc->getAnchor();
15261526

1527-
KeyPathExpr::Component component;
1527+
SmallVector<KeyPathExpr::Component, 2> components;
15281528

15291529
// Let's create a KeyPath expression and fill in "parsed path"
15301530
// after component is built.
@@ -1546,10 +1546,10 @@ namespace {
15461546
// calls necessary to resolve a member reference.
15471547
if (overload.choice.getKind() ==
15481548
OverloadChoiceKind::KeyPathDynamicMemberLookup) {
1549-
keyPath->resolveComponents(ctx,
1550-
buildKeyPathSubscriptComponent(
1551-
overload, dotLoc, /*indexExpr=*/nullptr,
1552-
ctx.Id_dynamicMember, componentLoc));
1549+
buildKeyPathSubscriptComponent(overload, dotLoc, /*indexExpr=*/nullptr,
1550+
ctx.Id_dynamicMember, componentLoc,
1551+
components);
1552+
keyPath->resolveComponents(ctx, components);
15531553
return keyPath;
15541554
}
15551555

@@ -1599,8 +1599,8 @@ namespace {
15991599
UDE->getNameLoc(),
16001600
/*Implicit=*/true);
16011601

1602-
component = buildKeyPathPropertyComponent(overload, UDE->getLoc(),
1603-
componentLoc);
1602+
buildKeyPathPropertyComponent(overload, UDE->getLoc(), componentLoc,
1603+
components);
16041604
} else if (auto *SE = dyn_cast<SubscriptExpr>(anchor)) {
16051605
componentExpr = SE;
16061606
// If this is not for a keypath component, we have to copy
@@ -1628,9 +1628,9 @@ namespace {
16281628
/*implicit=*/true, SE->getAccessSemantics());
16291629
}
16301630

1631-
component = buildKeyPathSubscriptComponent(
1632-
overload, SE->getLoc(), SE->getIndex(), SE->getArgumentLabels(),
1633-
componentLoc);
1631+
buildKeyPathSubscriptComponent(overload, SE->getLoc(), SE->getIndex(),
1632+
SE->getArgumentLabels(), componentLoc,
1633+
components);
16341634
} else {
16351635
return nullptr;
16361636
}
@@ -1643,7 +1643,7 @@ namespace {
16431643
cs.setType(keyPath, 0, ty);
16441644

16451645
keyPath->setParsedPath(componentExpr);
1646-
keyPath->resolveComponents(ctx, {component});
1646+
keyPath->resolveComponents(ctx, components);
16471647
return keyPath;
16481648
}
16491649

@@ -4190,24 +4190,6 @@ namespace {
41904190
resolvedComponents.push_back(origComponent);
41914191
continue;
41924192
}
4193-
4194-
auto getObjectType = [](Type optionalTy) -> Type {
4195-
Type objectTy;
4196-
if (auto lvalue = optionalTy->getAs<LValueType>()) {
4197-
objectTy = lvalue->getObjectType()->getOptionalObjectType();
4198-
if (optionalTy->hasUnresolvedType() && !objectTy) {
4199-
objectTy = optionalTy;
4200-
}
4201-
objectTy = LValueType::get(objectTy);
4202-
} else {
4203-
objectTy = optionalTy->getOptionalObjectType();
4204-
if (optionalTy->hasUnresolvedType() && !objectTy) {
4205-
objectTy = optionalTy;
4206-
}
4207-
}
4208-
assert(objectTy);
4209-
return objectTy;
4210-
};
42114193

42124194
auto kind = origComponent.getKind();
42134195
Optional<SelectedOverload> foundDecl;
@@ -4249,17 +4231,8 @@ namespace {
42494231
break;
42504232
}
42514233

4252-
auto component = buildKeyPathPropertyComponent(
4253-
*foundDecl, origComponent.getLoc(), locator);
4254-
4255-
resolvedComponents.push_back(component);
4256-
4257-
if (shouldForceUnwrapResult(foundDecl->choice, locator)) {
4258-
auto objectTy = getObjectType(baseTy);
4259-
auto loc = origComponent.getLoc();
4260-
resolvedComponents.push_back(
4261-
KeyPathExpr::Component::forOptionalForce(objectTy, loc));
4262-
}
4234+
buildKeyPathPropertyComponent(*foundDecl, origComponent.getLoc(),
4235+
locator, resolvedComponents);
42634236
break;
42644237
}
42654238
case KeyPathExpr::Component::Kind::UnresolvedSubscript: {
@@ -4273,17 +4246,9 @@ namespace {
42734246
if (!isDynamicMember)
42744247
subscriptLabels = origComponent.getSubscriptLabels();
42754248

4276-
auto component = buildKeyPathSubscriptComponent(
4249+
buildKeyPathSubscriptComponent(
42774250
*foundDecl, origComponent.getLoc(), origComponent.getIndexExpr(),
4278-
subscriptLabels, locator);
4279-
resolvedComponents.push_back(component);
4280-
4281-
if (shouldForceUnwrapResult(foundDecl->choice, locator)) {
4282-
auto objectTy = getObjectType(baseTy);
4283-
auto loc = origComponent.getLoc();
4284-
resolvedComponents.push_back(
4285-
KeyPathExpr::Component::forOptionalForce(objectTy, loc));
4286-
}
4251+
subscriptLabels, locator, resolvedComponents);
42874252
break;
42884253
}
42894254
case KeyPathExpr::Component::Kind::OptionalChain: {
@@ -4301,13 +4266,9 @@ namespace {
43014266
KeyPathExpr::Component::forOptionalChain(objectTy, loc));
43024267
break;
43034268
}
4304-
case KeyPathExpr::Component::Kind::OptionalForce: {
4305-
auto objectTy = getObjectType(baseTy);
4306-
auto loc = origComponent.getLoc();
4307-
resolvedComponents.push_back(
4308-
KeyPathExpr::Component::forOptionalForce(objectTy, loc));
4269+
case KeyPathExpr::Component::Kind::OptionalForce:
4270+
buildKeyPathOptionalForceComponent(resolvedComponents);
43094271
break;
4310-
}
43114272
case KeyPathExpr::Component::Kind::Invalid: {
43124273
auto component = origComponent;
43134274
component.setComponentType(leafTy);
@@ -4466,10 +4427,37 @@ namespace {
44664427
return coerceToType(outerApply, exprType, cs.getConstraintLocator(E));
44674428
}
44684429

4469-
KeyPathExpr::Component
4470-
buildKeyPathPropertyComponent(const SelectedOverload &overload,
4471-
SourceLoc componentLoc,
4472-
ConstraintLocator *locator) {
4430+
void buildKeyPathOptionalForceComponent(
4431+
SmallVectorImpl<KeyPathExpr::Component> &components) {
4432+
assert(!components.empty());
4433+
4434+
// Unwrap the last component type, preserving @lvalue-ness.
4435+
auto optionalTy = components.back().getComponentType();
4436+
Type objectTy;
4437+
if (auto lvalue = optionalTy->getAs<LValueType>()) {
4438+
objectTy = lvalue->getObjectType()->getOptionalObjectType();
4439+
if (optionalTy->hasUnresolvedType() && !objectTy) {
4440+
objectTy = optionalTy;
4441+
}
4442+
objectTy = LValueType::get(objectTy);
4443+
} else {
4444+
objectTy = optionalTy->getOptionalObjectType();
4445+
if (optionalTy->hasUnresolvedType() && !objectTy) {
4446+
objectTy = optionalTy;
4447+
}
4448+
}
4449+
assert(objectTy);
4450+
4451+
auto loc = components.back().getLoc();
4452+
components.push_back(
4453+
KeyPathExpr::Component::forOptionalForce(objectTy, loc));
4454+
}
4455+
4456+
void buildKeyPathPropertyComponent(
4457+
const SelectedOverload &overload, SourceLoc componentLoc,
4458+
ConstraintLocator *locator,
4459+
SmallVectorImpl<KeyPathExpr::Component> &components) {
4460+
auto resolvedTy = simplifyType(overload.openedType);
44734461
if (auto *property = overload.choice.getDeclOrNull()) {
44744462
// Key paths can only refer to properties currently.
44754463
auto varDecl = cast<VarDecl>(property);
@@ -4479,26 +4467,24 @@ namespace {
44794467
// There is a fix which diagnoses such situation already.
44804468
assert(!varDecl->isStatic());
44814469

4482-
auto resolvedTy = overload.openedType;
4483-
resolvedTy = simplifyType(resolvedTy);
4484-
44854470
// Compute the concrete reference to the member.
44864471
auto ref = resolveConcreteDeclRef(property, locator);
4487-
return KeyPathExpr::Component::forProperty(ref, resolvedTy,
4488-
componentLoc);
4472+
components.push_back(
4473+
KeyPathExpr::Component::forProperty(ref, resolvedTy, componentLoc));
4474+
} else {
4475+
auto fieldIndex = overload.choice.getTupleIndex();
4476+
components.push_back(KeyPathExpr::Component::forTupleElement(
4477+
fieldIndex, resolvedTy, componentLoc));
44894478
}
44904479

4491-
auto fieldIndex = overload.choice.getTupleIndex();
4492-
auto resolvedTy = overload.openedType;
4493-
resolvedTy = simplifyType(resolvedTy);
4494-
4495-
return KeyPathExpr::Component::forTupleElement(fieldIndex, resolvedTy,
4496-
componentLoc);
4480+
if (shouldForceUnwrapResult(overload.choice, locator))
4481+
buildKeyPathOptionalForceComponent(components);
44974482
}
44984483

4499-
KeyPathExpr::Component buildKeyPathSubscriptComponent(
4484+
void buildKeyPathSubscriptComponent(
45004485
SelectedOverload &overload, SourceLoc componentLoc, Expr *indexExpr,
4501-
ArrayRef<Identifier> labels, ConstraintLocator *locator) {
4486+
ArrayRef<Identifier> labels, ConstraintLocator *locator,
4487+
SmallVectorImpl<KeyPathExpr::Component> &components) {
45024488
auto subscript = cast<SubscriptDecl>(overload.choice.getDecl());
45034489
assert(!subscript->isGetterMutating());
45044490

@@ -4568,10 +4554,14 @@ namespace {
45684554
conformances.push_back(hashableConformance);
45694555
}
45704556

4571-
return KeyPathExpr::Component::forSubscriptWithPrebuiltIndexExpr(
4557+
auto comp = KeyPathExpr::Component::forSubscriptWithPrebuiltIndexExpr(
45724558
ref, newIndexExpr, cs.getASTContext().AllocateCopy(newLabels),
45734559
resolvedTy, componentLoc,
45744560
cs.getASTContext().AllocateCopy(conformances));
4561+
components.push_back(comp);
4562+
4563+
if (shouldForceUnwrapResult(overload.choice, locator))
4564+
buildKeyPathOptionalForceComponent(components);
45754565
}
45764566

45774567
Expr *visitKeyPathDotExpr(KeyPathDotExpr *E) {

‎test/Constraints/keypath_dynamic_member_lookup.swift

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,3 +390,32 @@ func test_constrained_ext_vs_dynamic_member() {
390390
// CHECK: function_ref @$s29keypath_dynamic_member_lookup8SR_11465VAASHRzlE9hashValueSivg
391391
_ = SR_11465<Int>(rawValue: 1).hashValue // Ok, keep choice from constrained extension
392392
}
393+
394+
// SR-11893: Make sure we properly handle IUO unwraps for key path dynamic members.
395+
struct SR_11893_Base {
396+
var i: Int!
397+
subscript(_ x: Int) -> Int! { x }
398+
}
399+
400+
@dynamicMemberLookup
401+
struct SR_11893 {
402+
subscript(dynamicMember kp: KeyPath<SR_11893_Base, Int>) -> Void { () }
403+
}
404+
405+
// CHECK-LABEL: sil hidden @$s29keypath_dynamic_member_lookup13testIUOUnwrapyyAA8SR_11893VF : $@convention(thin) (SR_11893) -> ()
406+
func testIUOUnwrap(_ x: SR_11893) {
407+
// CHECK: keypath $KeyPath<SR_11893_Base, Int>, (root $SR_11893_Base; stored_property #SR_11893_Base.i : $Optional<Int>; optional_force : $Int)
408+
x.i
409+
410+
// 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)
411+
x[5]
412+
413+
// FIXME(SR-11896): We should not vend a WritableKeyPath for the "outer" key path here.
414+
// 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)
415+
// 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]])
416+
_ = \SR_11893.i
417+
418+
// 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)
419+
// 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]])
420+
_ = \SR_11893.[5]
421+
}

0 commit comments

Comments
 (0)