Skip to content

[AST/Sema/SIL] Implement @_inheritActorContext(always) #81496

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

Merged
merged 6 commits into from
May 15, 2025

Conversation

xedin
Copy link
Contributor

@xedin xedin commented May 14, 2025

  • A Extend @_inheritActorContext attribute to support optional always modifier. The new modifier will make closure context isolated even if the parameter is not captured by the closure.
  • Implementation @_inheritActorContext attribute validation - it could only be used on parameter that have @Sendable or sending and @isolated(any) or async function type (downgraded to a warning until future major Swift mode to avoid source compatibility issues).
  • Add a new language feature that guards use of @_inheritActorContext(always) in swift interface files
  • Update getLoweredLocalCaptures to add an entry for isolation parameter implicitly captured by @_inheritActorContext(always)
  • Update serialization code to store always modifier

auto isolation = closure->getActorIsolation();
if (isolation.isActorInstanceIsolated()) {
if (auto *var = isolation.getActorInstance()) {
recordCapture(CapturedValue(var, /*flags=*/0, SourceLoc()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slavapestov Maybe you have a better idea how to handle this? I don't think we can do this in Sema because we don't have isolation when we compute captures.

diag::inherit_actor_context_only_on_async_or_isolation_erased_params,
attr)
.warnUntilFutureSwiftVersion();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Great to add the checks!


acceptAsyncSendableClosureInheritingAlways {
v += 1 // Ok
}
}
Copy link
Contributor

@ktoso ktoso May 14, 2025

Choose a reason for hiding this comment

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

Minor followup: Would it be worth adding a test for @MainActor as well, it'll "just work" because _inheritActorContext itself should be doing that, but in order make sure we don't accidentally regress that in _inheritActorContext(always) somehow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah the diff had hidden the testGlobalActorInheritance case, we got that covered then 👍

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for getting this over the finish line! 🙏

@xedin
Copy link
Contributor Author

xedin commented May 14, 2025

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented May 14, 2025

@swift-ci please test macOS platform

xedin added 6 commits May 14, 2025 20:07
…lways` modifier

By default (currently) the closure passed to a parameter with `@_inheritActorContext`
would only inherit isolation from `nonisolated`, global actor isolated or actor
context when "self" is captured by the closure. `always` changes this behavior to
always inherit actor isolation from context regardless of whether it's captured
or not.
…feature flag

This is going to avoid condfails when declarations are printed
in the swiftinterface files.
… lowering captures

`getLoweredLocalCaptures` seems the best place to do it because
during Sema there is a chicken and an egg situation where isolation
depends on captures and in this case captures depend on isolation.
…le`/sending and async/`@isolated(any)` parameters
@xedin xedin force-pushed the inheritActorContext-always branch from 8c9b8a7 to 17b8f7e Compare May 15, 2025 03:08
@xedin
Copy link
Contributor Author

xedin commented May 15, 2025

@swift-ci please test

@@ -1210,6 +1210,8 @@ actor MyServer : Server {
func acceptAsyncSendableClosure<T>(_: @Sendable () async -> T) { }
@available(SwiftStdlib 5.1, *)
func acceptAsyncSendableClosureInheriting<T>(@_inheritActorContext _: @Sendable () async -> T) { }
@available(SwiftStdlib 5.1, *)
func acceptAsyncSendableClosureInheritingAlways<T>(@_inheritActorContext(always) _: @Sendable () async -> T) { }
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, how is this change expected to behave if the isolated parameter is shadowed and captured? e.g.

func isofn(_ iso: isolated any Actor) {
  // or shadow locally here and capture
  inheritAlways { [iso] in ... }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should behave the same as non-always version does - determineClosureIsolation is going to check whether capture is a isolated parameter and act accordingly, if it’s not - “always” is going to force the capture without shadowing based on isolation of context.

@xedin xedin merged commit 917524d into swiftlang:main May 15, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants