Make top-level code variables @MainActor @preconcurrency
#40998
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
etcwilde
added
the
concurrency
etcwilde
changed the title
@MainActor @_predatesConcurrency@MainActor @_predatesConcurrency
DougGregor
reviewed
Jan 25, 2022
lib/AST/Decl.cpp
Outdated
| // Variables declared in top-level code are @_predatesConcurrency | ||
| if (const VarDecl *var = dyn_cast<VarDecl>(this)) { | ||
| return getASTContext().LangOpts.EnableExperimentalAsyncTopLevel && | ||
| var->isTopLevelGlobal(); |
There was a problem hiding this comment.
Should we also check for Swift version < 6 here?
There was a problem hiding this comment.
I think that should get picked up by contextRequiresStrictConcurrencyChecking, which is used by adjustVarTypeForConcurrecy to strip off the actor from the type.
I'll throw it in just to ensure that nothing leaks past though.
etcwilde
changed the title
@MainActor @_predatesConcurrency@MainActor @_predatesConcurrency
etcwilde
force-pushed
the
ewilde/mainactor-top-level-vars
branch
from
06eac77
to
e6c437f
Compare
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
ktoso
reviewed
Jan 28, 2022
| @@ -4741,7 +4741,9 @@ ERROR(global_actor_non_unsafe_init,none, | |||
| "global actor attribute %0 argument can only be '(unsafe)'", (Type)) | |||
| ERROR(global_actor_non_final_class,none, | |||
| "non-final class %0 cannot be a global actor", (DeclName)) | |||
|
|
|||
| ERROR(global_actor_top_level_var,none, | |||
| "top-level code variables cannot have a global actor", ()) | |||
There was a problem hiding this comment.
add a test to cover this? there doesn't seem to be one
There was a problem hiding this comment.
ktoso
approved these changes
Jan 28, 2022
|
@swift-ci please test |
|
Build failed |
|
It looks like all of the API's have changed... D: |
|
|
|
Build failed |
etcwilde
force-pushed
the
ewilde/mainactor-top-level-vars
branch
from
8a8bd57
to
294b0d0
Compare
This patch sets the actor isolation of the top-level code contexts to be on the main actor. This holds since the implicit `async_Main` function generated is executed on the main actor executor.
This is an interesting patch that may raise some eyebrows. The machinery we have now can completely handle arbitrary global-actor isolation on the global variables. I'm forcing the variables to be on the main actor because in an ideal future, the variables won't be global and will actually be local to the implicit `async_Main` function, where they will be isolated to the main actor by virtue of the `async_Main` function being run by the main actor executor. For that to work, we can't really allow top-level variables to have global actor isolation since local variables can't have global actor isolation. So while there is no technical reason we can't handle it now, to avoid source-breaking changes in the future, I'm removing the ability to explicitly declare a global actor on top-level global variables and forcing them all to be on the MainActor implicitly.
Top-level global variables should have the `@predatesConcurrency @MainActor` behaviour. This allows them to be somewhat concurrency safe while still working with old code.
Since we're forcing all of the variables at the top-level to be main actor isolated, there were some changes to where actor-hops were are to happen. Additionally, I had to pull of the actor isolation on `a` since we don't allow explicit global actor isolation. Given this, it doesn't really make sense to keep `b` around. As a nice change though, we can call synchronous functions directly in top level code, passing variables directly.
Ensure that the predates concurrency behavior is only applied for Swift 5 and not for Swift 6.
Adding a test to ensure that we emit an error message if there are global actors on top-level variables.
etcwilde
force-pushed
the
ewilde/mainactor-top-level-vars
branch
from
294b0d0
to
8d509ad
Compare
|
@swift-ci please test |
etcwilde
changed the title
@MainActor @_predatesConcurrency@MainActor @preconcurrency
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch gets the semantics roughly where we want them. The top-level code context is set to run on the main actor and the top-level variables are
@MainActor @preconcurrency. This pairing reduces the number of hops between actors for top-level code, provides some level of safety, and keeps the code breakage fairly minimal.The next PR contains
awaitdetection to turn the top-level code into an async context once the first use ofawaitis used in the top-level. If noawaits are used, the top-level should behave exactly the same as it does today to keep current scripts working.