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

Sema: Fix local property wrappers on constructor #78470

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

slavapestov
Copy link
Contributor

Fixes rdar://problem/141961300.

@@ -1632,6 +1632,13 @@ class Solution {
/// A map from argument expressions to their applied property wrapper expressions.
llvm::DenseMap<ASTNode, SmallVector<AppliedPropertyWrapper, 2>> appliedPropertyWrappers;

ArrayRef<AppliedPropertyWrapper> getAppliedPropertyWrappers(ASTNode anchor) {
auto found = appliedPropertyWrappers.find(anchor);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we really ought to be keying this with the callee locator itself, not just the anchor. For e.g key paths with subscript components you can have different callee locators that share the same anchor (although we don't currently support external property wrappers for subscripts)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we do know that argument/parameter pair this property wrapper corresponds to, should be able to use full locator while applying and re-create it while coercing arguments...

@slavapestov slavapestov marked this pull request as ready for review January 8, 2025 19:31
@slavapestov slavapestov requested a review from hborla as a code owner January 8, 2025 19:31
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov slavapestov merged commit c12e0a1 into swiftlang:main Jan 9, 2025
3 checks passed
@xedin
Copy link
Contributor

xedin commented Jan 9, 2025

For posterity: we discussed this offline and decided to take this fix tentatively until we can find a better setup by changing how property wrappers are anchored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants