-
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] Allow Property Wrappers on Let Declarations #1910
Conversation
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.
@amritpan I think the proposal needs to address nonmutating set
discussion that happened during the pitch either in Detailed Design or in Alternatives Considered.
e14b353
to
67df795
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.
I've got a bunch of editorial suggestions below, but also a number of substantive comments.
The big one is that the examples in the "Detailed design" section don't seem to fulfill what the proposal template is asking for in that section, but fortunately you've got a great example in SE-0258 as to how to approach this, and I think a lot of the questions you answered in the pitch thread supply the material you would need to fill out that section.
In reviewing review thread comments, did you ever get around to looking into @ktoso's question about @TaskLocal
?
Hi @amritpan, just wanted to check in about how things are going. I'm hoping my feedback is straightforward enough that it shouldn't take much to incorporate, but please let me know if there's something I can do to help facilitate. |
Thank you @xwu! I got sidetracked with unrelated work but am prioritizing this for this week. I'll likely have some questions! |
Sure thing! |
Co-authored-by: Xiaodi Wu <13952+xwu@users.noreply.github.com>
385769e
to
aabc6f7
Compare
@xwu I think we are finally ready! |
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.
Initial editorial comments; I have some thoughts on the overall structure (what text goes in what section) which I'll suggest after these changes.
|
||
## Introduction | ||
|
||
[SE-0258 Property Wrappers](https://github.com/apple/swift-evolution/blob/main/proposals/0258-property-wrappers.md) models both mutable and immutable variants of a property wrapper but does not allow wrapped properties explicitly marked as a `let`. This proposal builds on the original vision by expanding property wrapper usage to include let declared properties with a wrapper attribute. |
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.
[SE-0258 Property Wrappers](https://github.com/apple/swift-evolution/blob/main/proposals/0258-property-wrappers.md) models both mutable and immutable variants of a property wrapper but does not allow wrapped properties explicitly marked as a `let`. This proposal builds on the original vision by expanding property wrapper usage to include let declared properties with a wrapper attribute. | |
[SE-0258: Property Wrappers](https://github.com/apple/swift-evolution/blob/main/proposals/0258-property-wrappers.md) does not allow wrapped properties declared with `let`. This proposal builds on the original vision by expanding property wrapper usage to include such properties. |
I'm not sure I understand what's meant by "model[ing] both mutable and immutable variants" here, but it doesn't seem necessary to understand that in order to understand the proposal, so my suggestion is to take it out.
I didn't think there were such things as implicit let
declarations, but if there were I didn't think SE-0258 would allow property wrappers there either, so my suggestion is to strike "explicitly"; the rest are edits for clarity and brevity.
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.
SE-0258 models immutable and mutable variants here. I wanted to make the distinction that we could manually set up property wrappers to behave like let
wrapped declarations but this proposal would rid the need for it.
@xwu Is this new in review process? |
0def8fd
to
e0b7922
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.
Hi @amritpan, thanks for all your hard work and patience! From an editorial standpoint, this looks pretty much ready to go.
Now that WWDC is over, we discussed in the language steering group and folks had some thoughts about other features on let
declarations that may need to be considered in parallel, so they'll comment with their thoughts over the in the forums. The conclusion of that discussion may well be that this proposal goes forward as-is into review and not too long from now, but it was felt that there's some exploration there to be had.
@xwu Thank you! Very curious about those other features so I am looking forward to the review discussion! |
Co-authored-by: Xiaodi Wu <13952+xwu@users.noreply.github.com>
Where does this proposal stand? |
If I remember correctly this is stalled because it would create problems for macros. I don't think @amritpan is working on this so we should probably close it for now. |
This is the formal proposal for allowing let declared property wrappers in Swift.
Implementation: [Sema/SILGen] Allow property wrappers on let declarations #62342
Review: Pitch