enforce under strict concurrency that globals and statics be either… #67974
Conversation
sophiapoirier
requested review from
ktoso,
kavon,
hborla,
slavapestov,
xedin and
tshortli
as code owners
| break; | ||
| } | ||
| } | ||
| auto inferredIsolation = [&](ActorIsolation inferred, |
There was a problem hiding this comment.
inlining an immediately invoked lambda added an extra level of indentation to the body of inferredIsolation, so if you filter white space from the diff, it should reduce it significantly.
lib/Sema/TypeCheckConcurrency.cpp
Outdated
| var->isStatic() || var->getDeclContext()->isModuleScopeContext() || | ||
| (var->getDeclContext()->isTypeContext() && !var->isInstanceMember()); | ||
| auto *classDecl = var->getDeclContext()->getSelfClassDecl(); | ||
| const bool isActorType = classDecl && classDecl->isActor(); |
There was a problem hiding this comment.
This should be using isAnyActor I think?
edit: Hmmm, ah no. Since this is just about global actors -- could use a comment why we don't care about distributed actor here though, mind adding one? I.e. because distributed actors cannot be global (at least not today)
There was a problem hiding this comment.
Well I don't think it would hurt to make it isAnyActor. Distributed actors won't reach this code path at this point anyway, since they can't be global, but if someday they can, then things will continue to work (and no explanatory comment required).
There was a problem hiding this comment.
Yeah maybe can use the isAnyActor right away, your call :)
Thank you!
| @@ -5243,6 +5243,12 @@ NOTE(actor_mutable_state,none, | |||
| WARNING(shared_mutable_state_access,none, | |||
| "reference to %kind0 is not concurrency-safe because it involves " | |||
| "shared mutable state", (const ValueDecl *)) | |||
| ERROR(shared_mutable_state_decl,none, | |||
| "%kind0 is not concurrency-safe because it is involves shared mutable " | |||
There was a problem hiding this comment.
| "%kind0 is not concurrency-safe because it is involves shared mutable " | |
| "%kind0 is not concurrency-safe because it involves shared mutable " |
| @TestGlobalActor | ||
| var mutableIsolatedGlobal = 1 | ||
|
|
||
| var mutableNonisolatedGlobal = 1 // expected-error{{var 'mutableNonisolatedGlobal' is not concurrency-safe because it is involves shared mutable state}} |
There was a problem hiding this comment.
I'd nitpick the error a bit to be honest "involves" is not quite right here.
- maybe
"variable 'mutableNonisolatedGlobal' is not concurrency-safe because it is global shared mutable state" - consider "variable" vs "var" I don't think we use short names like that in messages much
- consider adding a note that can explain what people can do about it?
- maybe "note: isolate this property using a global actor, or convert it to a constant"?
There was a problem hiding this comment.
The var part is generated via %kind0 however otherwise making these changes, thanks for the improvement suggestions.
sophiapoirier
force-pushed
the
global-static-data-race-safety
branch
3 times, most recently
from
0dc2b14 to
c4313ea
Compare
sophiapoirier
force-pushed
the
global-static-data-race-safety
branch
from
4fedd08 to
30435bb
Compare
lib/Sema/TypeCheckDeclPrimary.cpp
Outdated
| // Declarations inside types are handled in checkConformancesInContext() to | ||
| // avoid cycles involving associated type inference. | ||
| if (!VD->getDeclContext()->isTypeContext()) { | ||
| (void)getActorIsolation(VD); |
There was a problem hiding this comment.
I actually just realized that this invocation of getActorIsolation() is redundant since it is already called on all ValueDecl instances during DeclChecker::visit() (from which this method is called). About to push a commit removing this...
sophiapoirier
force-pushed
the
global-static-data-race-safety
branch
from
807ca8d to
137a6d8
Compare
| var mutableIsolatedGlobal = 1 | ||
|
|
||
| var mutableNonisolatedGlobal = 1 // expected-error{{var 'mutableNonisolatedGlobal' is not concurrency-safe because it is global shared mutable state}} | ||
| // expected-note@-1{{isolate 'mutableNonisolatedGlobal' to a global actor, or convert it to a constant}} |
There was a problem hiding this comment.
Maybe it could be useful to produce a fix-it to make var into let?
There was a problem hiding this comment.
Your feedback is making me realize that the diagnostics could be improved by being more detailed, and I just completed some rewordings. I think however offering this fixit is complicated by the fact that there are two potential solutions here (either isolating to a main actor, or making a constant that is also 'Sendable', and Sensibility was something that you made me realize is missing from this note), and we don't automatically know which would be most suitable.
There was a problem hiding this comment.
Ah I see, yeah it makes sense and new diagnostic with Sendable bit sounds good =]
…solated to a global actor or Sendable plus immutable rdar://81629027 Global and static variable data-race safety
sophiapoirier
force-pushed
the
global-static-data-race-safety
branch
from
dd0efd0 to
936ab20
Compare
|
@swift-ci please test |
…isolated to a global actor or Sendable plus immutable
rdar://81629027 Global and static variable data-race safety