Local function isolation fixes #82592
Conversation
rjmccall
requested review from
jckarter,
ktoso,
xymus,
hborla,
slavapestov and
xedin
as code owners
|
@swift-ci Please test |
| @@ -183,6 +191,9 @@ bool ActorIsolation::isEqual(const ActorIsolation &lhs, | |||
| auto *lhsActor = lhs.getActorInstance(); | |||
| auto *rhsActor = rhs.getActorInstance(); | |||
| if (lhsActor && rhsActor) { | |||
| if (lhsActor == rhsActor) | |||
| return true; | |||
|
|
|||
There was a problem hiding this comment.
Could the FIXME right below this be addressed now that we're tracking captures?
There was a problem hiding this comment.
I keep looking at this condition, and it terrifies me; I have no clue what this is even trying to do.
There was a problem hiding this comment.
I guess this condition isn't so bad; it's trying to make same-name captures of self be considered the same isolation as the original self. So I think the answer is no, I can't fix that FIXME to work for arbitrary captures without additional changes to the AST, because we only track this for self.
The condition that terrifies me is the one where we don't have VarDecls.
| bool checkIsolatedCapture) { | ||
| // We must have parent isolation determined to get here. | ||
| switch (parentIsolation) { | ||
| case ActorIsolation::CallerIsolationInheriting: |
There was a problem hiding this comment.
I'm surprised that this isn't propagating CallerIsolationInheriting. This isn't new to your PR, though.
There was a problem hiding this comment.
Yeah, I was thinking about this while working on the patch. I'm not sure if it would be more correct to propagate CallerIsolationInheriting itself or to try to capture to specific isolation from the outer function. At any rate, yeah, I didn't want to pile even more into this PR.
|
@swift-ci Please test Linux |
|
@swift-ci Please test macOS |
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
force-pushed
the
local-function-isolation-fixes
branch
from
0266d92 to
3439e0c
Compare
|
@swift-ci Please test |
|
@swift-ci Please test macOS |
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-
@Sendablelocal 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 whetherselfis 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 sendabilityas 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.