-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
// TODO: @TaskLocal should be a macro <rdar://120914014> | ||
if (auto *classDecl = | ||
var->getInterfaceType()->getClassOrBoundGenericClass(); | ||
classDecl && classDecl->getName().is("TaskLocal")) { |
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
@TaskLocal
property wrapper with strict concurrency enabled. This is due to the fact that@TaskLocal
property 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@TaskLocal
ensures isolation to a single task, thereby obviating data race concerns. This change suppresses strict concurrency diagnostics for@TaskLocal
declarations. 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 aTaskLocal
macro as described in rdar://121054518 and will be developed onmain
branch.Scope: minor change to skip past
@TaskLocal
declarations 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=complete
feature flag and the fact that@TaskLocal
property wrapper instances do not have concurrency concerns.Testing:
lit
tests includedReviewer: @ktoso @hborla