-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Fix KeyPath with default arg #27951
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
Fix KeyPath with default arg #27951
Changes from all commits
e87d243
0376dd0
4da9c42
14a2ad6
ff29b8e
5b4e385
327c08a
00ebbee
3da2799
ecba9c3
8c3bfc5
f7dd813
b6bca52
b0877fb
921c1eb
18f918b
e385439
51dc7c8
27ffff1
0f3dd52
0824469
853dfbe
5e1e907
800feda
775c062
0374c94
3a98452
976c8c9
fd02663
a8953b1
b8947c4
3f7ee85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4590,9 +4590,6 @@ namespace { | |
| /*applyExpr*/ nullptr, labels, | ||
| /*hasTrailingClosure*/ false, locator); | ||
|
|
||
| auto component = KeyPathExpr::Component::forSubscriptWithPrebuiltIndexExpr( | ||
| ref, newIndexExpr, labels, resolvedTy, componentLoc, {}); | ||
|
|
||
| // We need to be able to hash the captured index values in order for | ||
| // KeyPath itself to be hashable, so check that all of the subscript | ||
| // index components are hashable and collect their conformances here. | ||
|
|
@@ -4602,7 +4599,10 @@ namespace { | |
| cs.getASTContext().getProtocol(KnownProtocolKind::Hashable); | ||
|
|
||
| auto fnType = overload.openedType->castTo<FunctionType>(); | ||
| for (const auto ¶m : fnType->getParams()) { | ||
| SmallVector<Identifier, 4> newLabels; | ||
| for (auto ¶m : fnType->getParams()) { | ||
| newLabels.push_back(param.getLabel()); | ||
|
|
||
| auto indexType = simplifyType(param.getPlainType()); | ||
| // Index type conformance to Hashable protocol has been | ||
| // verified by the solver, we just need to get it again | ||
|
|
@@ -4615,8 +4615,10 @@ namespace { | |
| conformances.push_back(hashableConformance); | ||
| } | ||
|
|
||
| component.setSubscriptIndexHashableConformances(conformances); | ||
| return component; | ||
| return KeyPathExpr::Component::forSubscriptWithPrebuiltIndexExpr( | ||
| ref, newIndexExpr, cs.getASTContext().AllocateCopy(newLabels), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like KeyPathExpr::Component::getSubscriptLabels() is only called for KeyPathExpr::Component::Kind::UnresolvedSubscript. It might be worth a try to stick an assertion there if the kind is anything else, and remove the parameter from forSubscriptWithPrebuiltIndexExpr() altogether. |
||
| resolvedTy, componentLoc, | ||
| cs.getASTContext().AllocateCopy(conformances)); | ||
| } | ||
|
|
||
| Expr *visitKeyPathDotExpr(KeyPathDotExpr *E) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
|
|
||
| public func foo(x: Int = 0) -> Int { x } | ||
|
|
||
| public struct Subscript1 { | ||
| public init() { } | ||
| public subscript(x: Int = 0) -> Int { x } | ||
| } | ||
|
|
||
| public struct Subscript2 { | ||
| public init() { } | ||
| public subscript(x: String = #function) -> String { x } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| // RUN: %empty-directory(%t) | ||
| // RUN: %target-swift-frontend -emit-module -emit-module-path=%t/default_arg_other.swiftmodule -module-name=default_arg_other %S/Inputs/default_arg_other.swift | ||
| // RUN: %target-swift-emit-silgen -module-name default_arg_multiple_modules -I %t %s | %FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-%target-runtime | ||
|
|
||
| import default_arg_other | ||
|
|
||
| // CHECK-LABEL: sil hidden [ossa] @${{.*}}test1{{.*}} | ||
| func test1() -> Int { | ||
| // CHECK: [[DEF_ARG_FN:%[0-9]+]] = function_ref @$s17default_arg_other3foo1xS2i_tFfA_ : $@convention(thin) () -> Int | ||
| // CHECK: [[DEF_ARG:%[0-9]+]] = apply [[DEF_ARG_FN]]() {{.*}} | ||
| // CHECK: [[FN:%[0-9]+]] = function_ref @$s17default_arg_other3foo1xS2i_tF : $@convention(thin) (Int) -> Int | ||
| // CHECK: [[CALL:%[0-9]+]] = apply [[FN]]([[DEF_ARG]]) {{.*}} | ||
| // CHECK: return [[CALL]] : $Int | ||
| return foo() | ||
| } | ||
|
|
||
| // CHECK-LABEL: sil hidden [ossa] @${{.*}}test2{{.*}} | ||
| func test2() -> Int { | ||
| // CHECK: [[DEF_ARG_FN:%[0-9]+]] = function_ref @$s17default_arg_other10Subscript1VyS2icipfA_ : $@convention(thin) () -> Int | ||
| // CHECK: [[DEF_ARG:%[0-9]+]] = apply [[DEF_ARG_FN]]() {{.*}} | ||
| // CHECK: [[FN:%[0-9]+]] = function_ref @$s17default_arg_other10Subscript1VyS2icig : $@convention(method) (Int, Subscript1) -> Int | ||
| // CHECK: [[CALL:%[0-9]+]] = apply [[FN]]([[DEF_ARG]], {{.*}} | ||
| // CHECK: return [[CALL]] : $Int | ||
| return Subscript1()[] | ||
| } | ||
|
|
||
| // CHECK-LABEL: sil hidden [ossa] @${{.*}}test3{{.*}} | ||
| func test3() -> String { | ||
| // This should not call default arg constructor | ||
| // CHECK: [[STR_LIT:%[0-9]+]] = string_literal utf8 "test3()" | ||
| // CHECK: [[DEF_ARG:%[0-9]+]] = apply %{{[0-9]+}}([[STR_LIT]], {{.*}} | ||
| // CHECK: [[FN:%[0-9]+]] = function_ref @$s17default_arg_other10Subscript2VyS2Scig : $@convention(method) (@guaranteed String, Subscript2) -> @owned String | ||
| // CHECK: [[CALL:%[0-9]+]] = apply [[FN]]([[DEF_ARG]], {{.*}} | ||
| // CHECK: return [[CALL]] : $String | ||
| return Subscript2()[] | ||
| } | ||
|
|
||
| // CHECK-LABEL: sil hidden [ossa] @${{.*}}test4{{.*}} | ||
| func test4() { | ||
| // CHECK: [[DEF_ARG_FN:%[0-9]+]] = function_ref @$s17default_arg_other10Subscript1VyS2icipfA_ : $@convention(thin) () -> Int | ||
| // CHECK: [[DEF_ARG:%[0-9]+]] = apply [[DEF_ARG_FN]]() {{.*}} | ||
| // CHECK: keypath $KeyPath<Subscript1, Int>, (root $Subscript1; gettable_property $Int, id @$s17default_arg_other10Subscript1VyS2icig : $@convention(method) (Int, Subscript1) -> Int, getter @$s17default_arg_other10Subscript1VyS2icipACTK : $@convention(thin) (@in_guaranteed Subscript1, UnsafeRawPointer) -> @out Int, indices [%$0 : $Int : $Int], indices_equals @$sSiTH : $@convention(thin) (UnsafeRawPointer, UnsafeRawPointer) -> Bool, indices_hash @$sSiTh : $@convention(thin) (UnsafeRawPointer) -> Int, external #Subscript1.subscript) ([[DEF_ARG]]) | ||
| _ = \Subscript1.[] | ||
| } | ||
|
|
||
| // CHECK-LABEL: sil hidden [ossa] @${{.*}}test5{{.*}} | ||
| func test5() { | ||
| // This should not call default arg constructor | ||
| // CHECK: [[STR_LIT:%[0-9]+]] = string_literal utf8 "test5()" | ||
| // CHECK: [[DEF_ARG:%[0-9]+]] = apply %{{[0-9]+}}([[STR_LIT]], {{.*}} | ||
| // CHECK: keypath $KeyPath<Subscript2, String>, (root $Subscript2; gettable_property $String, id @$s17default_arg_other10Subscript2VyS2Scig : $@convention(method) (@guaranteed String, Subscript2) -> @owned String, getter @$s17default_arg_other10Subscript2VyS2ScipACTK : $@convention(thin) (@in_guaranteed Subscript2, UnsafeRawPointer) -> @out String, indices [%$0 : $String : $String], indices_equals @$sSSTH : $@convention(thin) (UnsafeRawPointer, UnsafeRawPointer) -> Bool, indices_hash @$sSSTh : $@convention(thin) (UnsafeRawPointer) -> Int, external #Subscript2.subscript) ([[DEF_ARG]]) | ||
| _ = \Subscript2.[] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Using ArgEmitter is definitely the right approach here.
It appears that the old code didn't correctly handle keypaths with variadic subscript components either, eg
Crashes with this on master
It looks like a similar issue. Can you see if your fix addresses this as well? If not, a follow up PR would be great :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to create a follow-up patch for variadics as it looks like that will require some additional work.