Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

amritpan
Copy link
Member

@amritpan amritpan commented Jan 30, 2025

This is the proposal text for method and initializer key paths, and is ready for review.

@amritpan amritpan changed the title Add proposal text. [SE-NNNN] Method and Initializer Key Paths Jan 30, 2025
@rjmccall rjmccall added the LSG Contains topics under the domain of the Language Steering Group label Feb 10, 2025
@beccadax beccadax self-assigned this Feb 11, 2025
Copy link
Contributor

@beccadax beccadax left a 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.


## Source compatibility

This feature has no effect on source compatibility.
Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that suddenly having a lot of extra methods on types that use @dynamicMemberLookup will cause problems for real-world codebases. The fact that dynamic members are disfavored by the type checker should reduce source compatibility concerns, but it might not be the end of the story.

I suppose there are two pieces to this question:

  1. Does this change cause projects to actually fail to build? You can assess this by asking Swift CI to run a source compatibility test on your PRs; Swift CI will try to build a bunch of open source projects using a compiler with your changes and will report any projects that fail.

  2. How well does this change work for types that have already adopted @dynamicMemberLookup? That's something you'd have to assess manually, by looking at popular libraries with @dynamicMemberLookup types, figuring out how much new API surface this exposes, and thinking about whether that new surface is bad. (To be clear, this is a subjective call!)


## Implications on adoption

This feature has no implications on adoption.
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, these descriptors are used to support Equatable on key paths. If the descriptors are always null, does that mean the Equatable conformance will behave strangely?

@beccadax
Copy link
Contributor

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 fooArray.map(\.foo(bar: baz)) might be worth highlighting!

@amritpan
Copy link
Member Author

amritpan commented Mar 4, 2025

Yes, implicit closure coercion is supported with this feature and I have added a section to note this!

@amritpan amritpan requested a review from beccadax March 4, 2025 17:18
Comment on lines +110 to +112
let kp2 = \S.subtract(this:) // WritableKeyPath<S, (Int) -> Int>
let kp3 = \S.subtract(that:) // WritableKeyPath<S, (Int) -> Int>
let kp4 = \S.subtract(that: 1) // WritableKeyPath<S, Int>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed: Would these be WritableKeyPath or just KeyPath?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSG Contains topics under the domain of the Language Steering Group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants