Skip to content

Commit e5ea69c

Browse files
hamishknighthamishknight
authored
[CS] Fix key path dynamic member IUOs (#28562)
[CS] Fix key path dynamic member IUOs
2 parents 380d0d9 + d328b1b commit e5ea69c

File tree

2 files changed

+112
-108
lines changed

2 files changed

+112
-108
lines changed

‎lib/Sema/CSApply.cpp

Lines changed: 83 additions & 108 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,11 @@ 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);
1553+
cs.cacheExprTypes(keyPath);
15531554
return keyPath;
15541555
}
15551556

@@ -1599,8 +1600,8 @@ namespace {
15991600
UDE->getNameLoc(),
16001601
/*Implicit=*/true);
16011602

1602-
component = buildKeyPathPropertyComponent(overload, UDE->getLoc(),
1603-
componentLoc);
1603+
buildKeyPathPropertyComponent(overload, UDE->getLoc(), componentLoc,
1604+
components);
16041605
} else if (auto *SE = dyn_cast<SubscriptExpr>(anchor)) {
16051606
componentExpr = SE;
16061607
// If this is not for a keypath component, we have to copy
@@ -1628,9 +1629,9 @@ namespace {
16281629
/*implicit=*/true, SE->getAccessSemantics());
16291630
}
16301631

1631-
component = buildKeyPathSubscriptComponent(
1632-
overload, SE->getLoc(), SE->getIndex(), SE->getArgumentLabels(),
1633-
componentLoc);
1632+
buildKeyPathSubscriptComponent(overload, SE->getLoc(), SE->getIndex(),
1633+
SE->getArgumentLabels(), componentLoc,
1634+
components);
16341635
} else {
16351636
return nullptr;
16361637
}
@@ -1640,10 +1641,9 @@ namespace {
16401641
componentExpr->setType(ty);
16411642
cs.cacheType(componentExpr);
16421643

1643-
cs.setType(keyPath, 0, ty);
1644-
16451644
keyPath->setParsedPath(componentExpr);
1646-
keyPath->resolveComponents(ctx, {component});
1645+
keyPath->resolveComponents(ctx, components);
1646+
cs.cacheExprTypes(keyPath);
16471647
return keyPath;
16481648
}
16491649

@@ -4181,13 +4181,6 @@ namespace {
41814181
leafTy = keyPathTy->getGenericArgs()[1];
41824182
}
41834183

4184-
// Updates the constraint system with the type of the last resolved
4185-
// component. We do it this way because we sometimes insert new
4186-
// components.
4187-
auto updateCSWithResolvedComponent = [&]() {
4188-
cs.setType(E, resolvedComponents.size() - 1, baseTy);
4189-
};
4190-
41914184
for (unsigned i : indices(E->getComponents())) {
41924185
auto &origComponent = E->getMutableComponents()[i];
41934186

@@ -4197,24 +4190,6 @@ namespace {
41974190
resolvedComponents.push_back(origComponent);
41984191
continue;
41994192
}
4200-
4201-
auto getObjectType = [](Type optionalTy) -> Type {
4202-
Type objectTy;
4203-
if (auto lvalue = optionalTy->getAs<LValueType>()) {
4204-
objectTy = lvalue->getObjectType()->getOptionalObjectType();
4205-
if (optionalTy->hasUnresolvedType() && !objectTy) {
4206-
objectTy = optionalTy;
4207-
}
4208-
objectTy = LValueType::get(objectTy);
4209-
} else {
4210-
objectTy = optionalTy->getOptionalObjectType();
4211-
if (optionalTy->hasUnresolvedType() && !objectTy) {
4212-
objectTy = optionalTy;
4213-
}
4214-
}
4215-
assert(objectTy);
4216-
return objectTy;
4217-
};
42184193

42194194
auto kind = origComponent.getKind();
42204195
Optional<SelectedOverload> foundDecl;
@@ -4248,57 +4223,32 @@ namespace {
42484223
}
42494224
}
42504225

4251-
KeyPathExpr::Component component;
42524226
switch (kind) {
42534227
case KeyPathExpr::Component::Kind::UnresolvedProperty: {
42544228
// If we couldn't resolve the component, leave it alone.
42554229
if (!foundDecl) {
4256-
component = origComponent;
4230+
resolvedComponents.push_back(origComponent);
42574231
break;
42584232
}
42594233

4260-
component = buildKeyPathPropertyComponent(
4261-
*foundDecl, origComponent.getLoc(), locator);
4262-
4263-
baseTy = component.getComponentType();
4264-
resolvedComponents.push_back(component);
4265-
4266-
if (shouldForceUnwrapResult(foundDecl->choice, locator)) {
4267-
updateCSWithResolvedComponent();
4268-
auto objectTy = getObjectType(baseTy);
4269-
auto loc = origComponent.getLoc();
4270-
component = KeyPathExpr::Component::forOptionalForce(objectTy, loc);
4271-
baseTy = component.getComponentType();
4272-
resolvedComponents.push_back(component);
4273-
}
4234+
buildKeyPathPropertyComponent(*foundDecl, origComponent.getLoc(),
4235+
locator, resolvedComponents);
42744236
break;
42754237
}
42764238
case KeyPathExpr::Component::Kind::UnresolvedSubscript: {
42774239
// Leave the component unresolved if the overload was not resolved.
42784240
if (!foundDecl) {
4279-
component = origComponent;
4241+
resolvedComponents.push_back(origComponent);
42804242
break;
42814243
}
42824244

42834245
ArrayRef<Identifier> subscriptLabels;
42844246
if (!isDynamicMember)
42854247
subscriptLabels = origComponent.getSubscriptLabels();
42864248

4287-
component = buildKeyPathSubscriptComponent(
4249+
buildKeyPathSubscriptComponent(
42884250
*foundDecl, origComponent.getLoc(), origComponent.getIndexExpr(),
4289-
subscriptLabels, locator);
4290-
4291-
baseTy = component.getComponentType();
4292-
resolvedComponents.push_back(component);
4293-
4294-
if (shouldForceUnwrapResult(foundDecl->choice, locator)) {
4295-
updateCSWithResolvedComponent();
4296-
auto objectTy = getObjectType(baseTy);
4297-
auto loc = origComponent.getLoc();
4298-
component = KeyPathExpr::Component::forOptionalForce(objectTy, loc);
4299-
baseTy = component.getComponentType();
4300-
resolvedComponents.push_back(component);
4301-
}
4251+
subscriptLabels, locator, resolvedComponents);
43024252
break;
43034253
}
43044254
case KeyPathExpr::Component::Kind::OptionalChain: {
@@ -4312,41 +4262,35 @@ namespace {
43124262
assert(objectTy);
43134263

43144264
auto loc = origComponent.getLoc();
4315-
component = KeyPathExpr::Component::forOptionalChain(objectTy, loc);
4316-
4317-
baseTy = component.getComponentType();
4318-
resolvedComponents.push_back(component);
4265+
resolvedComponents.push_back(
4266+
KeyPathExpr::Component::forOptionalChain(objectTy, loc));
43194267
break;
43204268
}
4321-
case KeyPathExpr::Component::Kind::OptionalForce: {
4322-
auto objectTy = getObjectType(baseTy);
4323-
auto loc = origComponent.getLoc();
4324-
component = KeyPathExpr::Component::forOptionalForce(objectTy, loc);
4325-
baseTy = component.getComponentType();
4326-
resolvedComponents.push_back(component);
4269+
case KeyPathExpr::Component::Kind::OptionalForce:
4270+
buildKeyPathOptionalForceComponent(resolvedComponents);
43274271
break;
4328-
}
4329-
case KeyPathExpr::Component::Kind::Invalid:
4330-
component = origComponent;
4272+
case KeyPathExpr::Component::Kind::Invalid: {
4273+
auto component = origComponent;
43314274
component.setComponentType(leafTy);
4332-
4333-
baseTy = component.getComponentType();
43344275
resolvedComponents.push_back(component);
43354276
break;
4336-
case KeyPathExpr::Component::Kind::Identity:
4337-
component = origComponent;
4277+
}
4278+
case KeyPathExpr::Component::Kind::Identity: {
4279+
auto component = origComponent;
43384280
component.setComponentType(baseTy);
43394281
resolvedComponents.push_back(component);
43404282
break;
4283+
}
43414284
case KeyPathExpr::Component::Kind::Property:
43424285
case KeyPathExpr::Component::Kind::Subscript:
43434286
case KeyPathExpr::Component::Kind::OptionalWrap:
43444287
case KeyPathExpr::Component::Kind::TupleElement:
43454288
llvm_unreachable("already resolved");
43464289
}
43474290

4348-
// By now, "baseTy" is the result type of this component.
4349-
updateCSWithResolvedComponent();
4291+
// Update "baseTy" with the result type of the last component.
4292+
assert(!resolvedComponents.empty());
4293+
baseTy = resolvedComponents.back().getComponentType();
43504294
}
43514295

43524296
// Wrap a non-optional result if there was chaining involved.
@@ -4359,10 +4303,12 @@ namespace {
43594303
auto component = KeyPathExpr::Component::forOptionalWrap(leafTy);
43604304
resolvedComponents.push_back(component);
43614305
baseTy = leafTy;
4362-
updateCSWithResolvedComponent();
43634306
}
4307+
4308+
// Set the resolved components, and cache their types.
43644309
E->resolveComponents(cs.getASTContext(), resolvedComponents);
4365-
4310+
cs.cacheExprTypes(E);
4311+
43664312
// See whether there's an equivalent ObjC key path string we can produce
43674313
// for interop purposes.
43684314
if (cs.getASTContext().LangOpts.EnableObjCInterop) {
@@ -4481,10 +4427,37 @@ namespace {
44814427
return coerceToType(outerApply, exprType, cs.getConstraintLocator(E));
44824428
}
44834429

4484-
KeyPathExpr::Component
4485-
buildKeyPathPropertyComponent(const SelectedOverload &overload,
4486-
SourceLoc componentLoc,
4487-
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);
44884461
if (auto *property = overload.choice.getDeclOrNull()) {
44894462
// Key paths can only refer to properties currently.
44904463
auto varDecl = cast<VarDecl>(property);
@@ -4494,26 +4467,24 @@ namespace {
44944467
// There is a fix which diagnoses such situation already.
44954468
assert(!varDecl->isStatic());
44964469

4497-
auto resolvedTy = overload.openedType;
4498-
resolvedTy = simplifyType(resolvedTy);
4499-
45004470
// Compute the concrete reference to the member.
45014471
auto ref = resolveConcreteDeclRef(property, locator);
4502-
return KeyPathExpr::Component::forProperty(ref, resolvedTy,
4503-
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));
45044478
}
45054479

4506-
auto fieldIndex = overload.choice.getTupleIndex();
4507-
auto resolvedTy = overload.openedType;
4508-
resolvedTy = simplifyType(resolvedTy);
4509-
4510-
return KeyPathExpr::Component::forTupleElement(fieldIndex, resolvedTy,
4511-
componentLoc);
4480+
if (shouldForceUnwrapResult(overload.choice, locator))
4481+
buildKeyPathOptionalForceComponent(components);
45124482
}
45134483

4514-
KeyPathExpr::Component buildKeyPathSubscriptComponent(
4484+
void buildKeyPathSubscriptComponent(
45154485
SelectedOverload &overload, SourceLoc componentLoc, Expr *indexExpr,
4516-
ArrayRef<Identifier> labels, ConstraintLocator *locator) {
4486+
ArrayRef<Identifier> labels, ConstraintLocator *locator,
4487+
SmallVectorImpl<KeyPathExpr::Component> &components) {
45174488
auto subscript = cast<SubscriptDecl>(overload.choice.getDecl());
45184489
assert(!subscript->isGetterMutating());
45194490

@@ -4583,10 +4554,14 @@ namespace {
45834554
conformances.push_back(hashableConformance);
45844555
}
45854556

4586-
return KeyPathExpr::Component::forSubscriptWithPrebuiltIndexExpr(
4557+
auto comp = KeyPathExpr::Component::forSubscriptWithPrebuiltIndexExpr(
45874558
ref, newIndexExpr, cs.getASTContext().AllocateCopy(newLabels),
45884559
resolvedTy, componentLoc,
45894560
cs.getASTContext().AllocateCopy(conformances));
4561+
components.push_back(comp);
4562+
4563+
if (shouldForceUnwrapResult(overload.choice, locator))
4564+
buildKeyPathOptionalForceComponent(components);
45904565
}
45914566

45924567
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)