-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[SE-NNNN] Method and Initializer Key Paths #2675
base: main
Are you sure you want to change the base?
Conversation
ef5b967
to
27a40ab
Compare
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.
Excited about this proposal, but there are a few suggestions I'd like to make.
If the member is a metatype (e.g., a static method, class method, initializer, or when referring to the type of an instance), you must explicitly include `.Type` in the key path root type. | ||
|
||
```swift | ||
struct Calculator { | ||
func add(_ a: Int, _ b: Int) -> Int { | ||
return a + b | ||
} | ||
} | ||
|
||
let calc = Calculator.self | ||
let addKeyPath: KeyPath<Calculator.Type, (Calculator) -> (Int, Int) -> Int> = \Calculator.Type.add | ||
``` |
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 found this example confusing because you're using an unapplied instance method, which is a very niche feature (and one that member key paths are meant to supplant!). Instead, I might illustrate this point with an example involving a static method or initializer—which I bet will be more common use cases—and then mention that it also works with unapplied instance methods.
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.
This makes sense - I will update this example
|
||
## Source compatibility | ||
|
||
This feature has no effect on source compatibility. |
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.
How certain are we that expanding @dynamicMemberLookup
to methods won't cause source compatibility breaks in common libraries? Is this something we've tested? Should we discuss an alternative where, for instance, you need to say @dynamicMemberLookup(all)
if you want to support method lookup?
(This wasn't really a concern with metatype keypaths because the subscript(dynamicMember:)
would have to accept KeyPath<Foo.Type, T>
instead of KeyPath<Foo, T>
for the new members to affect it.)
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.
How certain are we that expanding @dynamicMemberLookup to methods won't cause source compatibility breaks in common libraries?
I hadn't considered this! Also to make sure I fully understand - are we talking about being able to differentiate between a static and an instance method via @dynamicMemberLookup
? I have added these passing tests to Interpreter/keypath.swift
for @dynamicMemberLookup
but it doesn't test across libraries. How can I test for this?
|
||
## Implications on adoption | ||
|
||
This feature has no implications on adoption. |
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.
This would be a good place to discuss whether you expect this feature to back-deploy and what constraints there might be on doing so.
In particular, in SE-0438, there were some subtleties around symbols that we needed to start emitting. Are there similar concerns here? How are they being resolved? (If things are shaking out in basically the same way they did in the metatype keypath proposal, you could quickly summarize and link back to it.)
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 initially assumed that this feature would have the same implications on back-deployment as SE-0438. But then in the pitch discussion, it was suggested that methods do not emit descriptors. I also did not run into any linking errors with my Interpreter tests which is what I used to detect missing descriptors for metatype keypaths.
In the SE-0438 implementation I also added handling to account for nil descriptors to KeyPath in the Standard Library and so between that and not emitting any descriptors with this implementation, I made the assumption that this is a non issue and this feature would have no constraints on back-deployment.
|
||
### Effectful value types | ||
|
||
Methods annotated with `nonisolated` and `consuming` are supported by this feature. `mutating`, `throwing` and `async` are not supported for any other component type and will similarly not be supported for methods. Keypaths cannot capture method arguments that are not `Hashable`/`Equatable`, so `escaping` is also not supported. |
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.
A few questions:
- How does
consuming
work? (Maybe there's a statement that ought to be here about whether key path roots and/or arguments can be noncopying?) throwing
should probably be spelledthrows
.- Perhaps instead of singling out
escaping
, you should simply state that keypaths cannot capture closure arguments? (I assume this is the case?)
Oh, one more question that isn't addressed in the proposal: Do these key path literals support the implicit closure coercion like other key path literals do? The ability to say |
This is the proposal text for method and initializer key paths, and is ready for review.