Skip to content

[6.2] Local function isolation fixes #82613

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

Conversation

rjmccall
Copy link
Contributor

Fix a bunch of bugs with the isolation of local funcs. Since we use local funcs to implement defer, this also fixes several bugs with that feature, such as it breaking in nonisolated functions when a default isolation is in effect in the source file.

Change how we compute isolation of local funcs. The rule here is supposed to be that non-@sendable local funcs are isolated the same as their enclosing context. Unlike closure expressions, this is unconditional: in instance-isolated functions, the isolation does not depend on whether self is captured. But the computation was wrong: it didn't translate global actor isolation between contexts, it didn't turn parameter isolation into capture isolation, and it fell through for several other kinds of parent isolation, causing the compiler to try to apply default isolation instead. I've extracted the logic from the closure expression path into a common function and used it for both paths.

The capture computation logic was forcing a capture of the enclosing isolation in local funcs, but only for async functions. Presumably this was conditional because async functions need the isolation for act
or hops, but sync functions don't really need it. However, this was causing crashes with -enable-actor-data-race-checks. (I didn't investigate whether it also failed with the similar assertion we do with preconcurrency.) For now, I've switched this to capture the isolated instance unconditionally. If we need to be more conservative by either only capturing when data-race checks are enabled or disabling the checks when the isolation isn't captured, we can look into that.

Fix a bug in capture isolation checking. We were ignoring captures of nonisolated declarations in order to implement the rule that permits nonisolated(unsafe) variables to be captured in non-sendable closures. This check needs to only apply to variables! The isolation of a local func has nothing to do with its sendability
as a capture.

That fix exposed a problem where we were being unnecessarily restrictive with generic local func declarations because we didn't consider them to have sendable type. This was true even if the genericity was purely from being declared in a generic context, but it doesn't matter, they ought to be sendable regardless.

Finally, fix a handful of bugs where global actor types were not remapped properly in SILGen.

Main PR: #82592
Reviewer: Doug Gregor
Scope: Significant changes to isolation inference for local funcs, a relatively uncommonly-used feature. Also, necessary correctness fixes for a very unlikely feature (dependent global actor isolation), with the non-dependent paths well-covered by the existing test suite.
Risk: Low
Testing: new regression tests

rjmccall added 2 commits June 30, 2025 01:48
use local funcs to implement `defer`, this also fixes several
bugs with that feature, such as it breaking in nonisolated
functions when a default isolation is in effect in the source file.

Change how we compute isolation of local funcs. The rule here is
supposed to be that non-`@Sendable` local funcs are isolated the
same as their enclosing context. Unlike closure expressions, this
is unconditional: in instance-isolated functions, the isolation
does not depend on whether `self` is captured. But the computation
was wrong: it didn't translate global actor isolation between
contexts, it didn't turn parameter isolation into capture isolation,
and it fell through for several other kinds of parent isolation,
causing the compiler to try to apply default isolation instead.
I've extracted the logic from the closure expression path into a
common function and used it for both paths.

The capture computation logic was forcing a capture of the
enclosing isolation in local funcs, but only for async functions.
Presumably this was conditional because async functions need the
isolation for actor hops, but sync functions don't really need it.
However, this was causing crashes with `-enable-actor-data-race-checks`.
(I didn't investigate whether it also failed with the similar
assertion we do with preconcurrency.) For now, I've switched this
to capture the isolated instance unconditionally. If we need to
be more conservative by either only capturing when data-race checks
are enabled or disabling the checks when the isolation isn't captured,
we can look into that.

Fix a bug in capture isolation checking. We were ignoring captures
of nonisolated declarations in order to implement the rule that
permits `nonisolated(unsafe)` variables to be captured in
non-sendable closures. This check needs to only apply to variables!
The isolation of a local func has nothing to do with its sendability
as a capture.

That fix exposed a problem where we were being unnecessarily
restrictive with generic local func declarations because we didn't
consider them to have sendable type. This was true even if the
genericity was purely from being declared in a generic context,
but it doesn't matter, they ought to be sendable regardless.

Finally, fix a handful of bugs where global actor types were not
remapped properly in SILGen.
@rjmccall rjmccall requested a review from a team as a code owner June 30, 2025 05:54
@rjmccall
Copy link
Contributor Author

@swift-ci Please test

@rjmccall
Copy link
Contributor Author

@swift-ci Please test macOS

1 similar comment
@rjmccall
Copy link
Contributor Author

@swift-ci Please test macOS

@rjmccall rjmccall merged commit eff0845 into swiftlang:release/6.2 Jul 1, 2025
5 checks passed
@rjmccall rjmccall deleted the local-function-isolation-fixes-6.2 branch July 1, 2025 17:34
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.

2 participants