[Sema] Add custom functions to ActorIsolationChecker to determine expr type and closure actor isolation #59943
Conversation
ahoppen
force-pushed
the
pr/actorisolation-with-gettype
branch
from
670293c to
e17be26
Compare
ahoppen
changed the title
ahoppen
force-pushed
the
pr/actorisolation-with-gettype
branch
from
e17be26 to
7ab2cfd
Compare
include/swift/AST/ActorIsolation.h
Outdated
| ActorIsolation getActorIsolationOfContext( | ||
| DeclContext *dc, | ||
| llvm::function_ref<ClosureActorIsolation(AbstractClosureExpr *)> | ||
| getClosureActorIsolation); |
There was a problem hiding this comment.
Should this be defaulted to [](AbstractClosureExpr *closure) { return closure->getActorIsolation(); } so 99% of the call sites don't have to repeat it?
There was a problem hiding this comment.
I wanted to do the same but that would mean that we need to include Expr.h here (because we are referencing into AbstractClosureExpr) and Expr.h transitively includes ActorIsolation.h creating an include dependency cycle. 😬
There was a problem hiding this comment.
Couldn't it default to the forward-declaration of __AbstractClosureExpr_getActorIsolation?
There was a problem hiding this comment.
Now that you mention it, I think that should be possible.
There was a problem hiding this comment.
🤞 Yeah, it should work with forward declaration.
There was a problem hiding this comment.
I think it should be possible to move ClosureActorIsolation from Expr.h to ActorIsolation.h?...
There was a problem hiding this comment.
Not easily because it references VarDecl * inside a PointerUnion, which requires knowledge about the type of VarDecl * to compute NumLowBitsAvailable for it.
There was a problem hiding this comment.
Huh, TIL function_ref has SFINAE to allow covariant returns. Shame it doesn't have a constructor that just takes a callable with the exact params/return type. If we really wanted to, we could probably work around it by forward-declaring a trampoline that returns the llvm::function_ref<ClosureActorIsolation(AbstractClosureExpr *)>, but maybe it's not worth it.
There was a problem hiding this comment.
I think that’s a good idea 👍
ahoppen
force-pushed
the
pr/actorisolation-with-gettype
branch
2 times, most recently
from
7bd762e to
38a9325
Compare
ahoppen
force-pushed
the
pr/actorisolation-with-gettype
branch
2 times, most recently
from
e0e5814 to
ebc28d2
Compare
|
@swift-ci Please smoke test |
…ts own function Just a little bit of cleanup because this logic is really quite general and doesn't need to live in `getActorIsolationOfContext`.
…r type and closure actor isolation When we get rid of `LeaveClosureBodiesUnchecked` we no longer save closure types to the AST and thus also don’t save their actor isolation to the AST. Hence, we need to extract types and actor isolations of parent closures from the constraint system solution instead of the AST. This prepares `ActorIsolationChecker` to take custom functions to determine the type of an expression or the actor isolation of a closure.
ahoppen
force-pushed
the
pr/actorisolation-with-gettype
branch
from
ebc28d2 to
442cf9b
Compare
|
@swift-ci Please smoke test |
When we get rid of
LeaveClosureBodiesUncheckedwe no longer save closure types to the AST and thus also don’t save their actor isolation to the AST. Hence, we need to extract types and actor isolations of parent closures from the constraint system solution instead of the AST. This preparesActorIsolationCheckerto take custom functions to determine the type of an expression or the actor isolation of a closure.