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.

Sign up for GitHub

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

Jump to bottom

[Sema] Diagnose available attribute on wrapped and lazy properties #41112

Merged
merged 2 commits into from
Feb 7, 2022
Merged

[Sema] Diagnose available attribute on wrapped and lazy properties #41112

merged 2 commits into from
Feb 7, 2022

Conversation

tshortli
Copy link
Contributor

@tshortli tshortli commented Feb 1, 2022 •
edited
Loading

Emit an error when @available is used on property declarations that either have attached property wrappers or are lazy since availability is not supported on properties with backing storage.

Resolves rdar://82713248

@tshortli
Copy link
Contributor Author

tshortli commented Feb 1, 2022

@swift-ci please test source compatibility

@xedin xedin requested a review from hborla February 1, 2022 03:49
@tshortli
Copy link
Contributor Author

tshortli commented Feb 1, 2022

@swift-ci please smoke test

slavapestov
slavapestov approved these changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

It might be worth quickly searching for more places to use the new predicate, but I'll let you decide if you want to do that.

lib/AST/Decl.cpp
if (hasStorage())
return true;
auto *backing = getPropertyWrapperBackingProperty();
if (backing && backing->hasStorage())
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the backing property is always necessarily stored

lib/AST/Decl.cpp
bool VarDecl::hasStorageOrWrapsStorage() const {
if (hasStorage())
return true;
auto *backing = getPropertyWrapperBackingProperty();
Copy link
Contributor

@slavapestov slavapestov Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you also add a check for getAttributes().hasAttribute<LazyAttr>()?

Copy link
Member

Choose a reason for hiding this comment

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

I assume this suggestion is to avoid going into the request evaluator when it's not necessary. Should we instead add this check inside the VarDecl::getPropertyWrapper* methods to avoid doing it at all of the call sites?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, a formatting error there -- I meant to write getAttributes().hasAttribute<LazyAttr>() -- we need the same check for lazy properties as well.

I'm assuming the short-circuiting check you're thinking of is for CustomAttr. I remember this was worthwhile at one point because we weren't caching the results of some of these requests, but I'm not sure anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've added the check for the lazy attribute as well.

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, yes, I thought you were talking about checking for custom attributes. Sorry!

@tshortli
Copy link
Contributor Author

tshortli commented Feb 1, 2022

It might be worth quickly searching for more places to use the new predicate, but I'll let you decide if you want to do that.

I did scrutinize many of the uses of hasStorage() throughout the project to see if they ought to use the new predicate as well but I wasn't able to find any more that I was confident required this check. The ones I was least certain of were the diagnostics for use of @inlinable and @transparent for declarations with storage.

@tshortli tshortli force-pushed the diagnose-available-wrapped-properties branch 2 times, most recently from 1c788f5 to 02fa6f3 Compare February 1, 2022 19:11
@tshortli tshortli changed the title [Sema] Diagnose available attribute on wrapped properties [Sema] Diagnose available attribute on wrapped and lazy properties Feb 2, 2022
@tshortli
Copy link
Contributor Author

tshortli commented Feb 2, 2022

swiftlang/swift-package-manager#4075

@swift-ci Please test source compatibility

@tshortli tshortli force-pushed the diagnose-available-wrapped-properties branch from 02fa6f3 to ec10a7b Compare February 2, 2022 21:10
@tshortli
Copy link
Contributor Author

tshortli commented Feb 2, 2022

swiftlang/swift-package-manager#4075

@swift-ci Please test source compatibility

@tshortli
Copy link
Contributor Author

tshortli commented Feb 3, 2022

@swift-ci Please test source compatibility

@tshortli
Copy link
Contributor Author

tshortli commented Feb 3, 2022

@swift-ci please smoke test

@tshortli
[Sema] Emit an error for attempts to use @available on var declarat…
093b8d3 
…ions that have attached property wrappers. Availability is not supported on stored properties, and properties with property wrappers are stored properties with a layer of indirection. Also, add the same missing diagnostic for `lazy` vars.

Resolves rdar://82713248
@tshortli tshortli force-pushed the diagnose-available-wrapped-properties branch from ec10a7b to 093b8d3 Compare February 3, 2022 17:03
@tshortli
Copy link
Contributor Author

tshortli commented Feb 3, 2022

Had to fix fallout in additional test but it looks like source compatibility passed: https://ci.swift.org/job/swift-PR-source-compat-suite/5809/

@tshortli
Copy link
Contributor Author

tshortli commented Feb 3, 2022

@swift-ci please smoke test

1 similar comment
@tshortli
Copy link
Contributor Author

tshortli commented Feb 4, 2022

@swift-ci please smoke test

@tshortli
Copy link
Contributor Author

tshortli commented Feb 4, 2022

@slavapestov @hborla Now that we're correctly reflecting the fact that lazy var declarations have storage a number of tests had to be updated because they were taking advantage of the missing diagnostics previously. I would appreciated it if you double check that you agree with the changes I made to the tests.

@tshortli
Copy link
Contributor Author

tshortli commented Feb 4, 2022

@swift-ci please smoke test

@tshortli tshortli merged commit 70a7d85 into swiftlang:main Feb 7, 2022
@tshortli tshortli deleted the diagnose-available-wrapped-properties branch February 8, 2022 07:32
This was referenced Jun 22, 2022
Merged
Merged
@Empty2k12 Empty2k12 mentioned this pull request Jul 19, 2022
Open
@martinmitrevski martinmitrevski mentioned this pull request Jul 29, 2022
Merged
6 tasks
@CFKevinRef CFKevinRef mentioned this pull request Sep 20, 2022
Merged
CFKevinRef added a commit to aws-samples/amazon-ivs-player-ios-sample that referenced this pull request Sep 20, 2022
@CFKevinRef
Update AmazonIVSPlayer Pod to 1.13.0
201c571 
Gem dependencies have been updated as well.

The project has been updated for Xcode 14, including recommended project file updates and to resolve [this new diagnostic in the Swift 5.7 compiler](swiftlang/swift#41112).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavapestov slavapestov slavapestov approved these changes

@hborla hborla Awaiting requested review from hborla hborla is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tshortli @slavapestov @hborla