-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Concurrency] suppress global variable static checking for @TaskLocal property wrapper declarations #70949
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
[Concurrency] suppress global variable static checking for @TaskLocal property wrapper declarations #70949
Conversation
lib/Sema/TypeCheckConcurrency.cpp
Outdated
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 should preferably check for the exact decl rather than the name -- I'll whip up a snippet for 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.
Like so:
if (auto *classDecl =
var->getInterfaceType()->getClassOrBoundGenericClass()) {
auto &ctx = var->getASTContext();
auto taskLocalDecl = ctx.getTaskLocalDecl();
if (classDecl == taskLocalDecl) {
return isolation;
}
}That'll avoid triggering for user-defined types of the same name
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.
Thanks very much @ktoso I suspected there was a more robust way to check, and I had seen the TaskLocal info in KnownSDKTypes.def but was unsure how it functioned or might help.
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.
Happy to help :)
Usually if we have a type we know about and want to speak about it we put those into Known... and then can on ASTContext get them to compare etc. 👍
0ee4c9d to
b9b9283
Compare
|
Failures are the removed warning: |
|
Please fill out the 5.10 cherry pick PR description template |
…roperty wrapper declarations
6fa5be7 to
4eaef71
Compare
|
@swift-ci please test |
Explanation: This change resolves a bug in the SE-0412 implementation that makes it impossible to use the
@TaskLocalproperty wrapper with strict concurrency enabled. This is due to the fact that@TaskLocalproperty wrappers are required to bestatic, and their storage is always declaredvar, and so they appear to the type checker as mutable global state, however the implementation of@TaskLocalensures isolation to a single task, thereby obviating data race concerns. This change suppresses strict concurrency diagnostics for@TaskLocaldeclarations. Note that this is a temporary solution for 5.10 to unblock developers from experimenting with strict concurrency. The more complete solution will be to move to aTaskLocalmacro as described in rdar://121054518 and will be developed onmainbranch.Scope: minor change to skip past
@TaskLocaldeclarations during type checking under strict concurrency and the changes are guarded behind an upcoming feature flagIssue: rdar://120907014
Risk: low risk given that the change is in functionality that is guarded behind
-strict-concurrency=completefeature flag and the fact that@TaskLocalproperty wrapper instances do not have concurrency concerns.Testing:
littests includedReviewer: @ktoso @hborla