Handle all isolation checking for function calls in one place #66928
Conversation
Isolation checking for calls had two separate implementation places: one that looked at the declaration being called (for member declarations) and one that worked on the actual call expression. Unify on the latter implementation, which is more general and has access to the specific call arguments. Improve diagnostics here somewher so we don't regress in that area. This refactoring shouldn't change the actual semantics, but it makes upcoming semantic changes easier.
The move to perform all call checking that can cross concurrency domains into one place dropped the specific logic for distributed actor calls. Introduce that logic, cleaning it up to consistently use the "known to be local" semantics needed for distributed actors.
…te path Remove the "old" path that diagnosed actor isolation and sendability issues with some calls. It is now unused.
DougGregor
force-pushed
the
unify-call-isolation-checking
branch
from
e9f3ec7 to
11dd80a
Compare
|
@swift-ci please smoke test |
| @@ -2646,6 +2596,8 @@ namespace { | |||
| } | |||
| } | |||
|
|
|||
| // FIXME: Subscript? | |||
There was a problem hiding this comment.
we don't support distributed subscripts; unsure if we need handling here after the changes?
There was a problem hiding this comment.
Looking at the line below I think we just handle it via fallthrough into the general case:
// This is either non-distributed variable, subscript, or something else.
ctx.Diags.diagnose(declLoc,
diag::distributed_actor_isolated_non_self_reference,so nothing to do here for the fixme?
There was a problem hiding this comment.
I didn't change anything with respect to distributed subscripts, but wanted to note that this is where we'd deal with them if/when we do
| } | ||
|
|
||
| func f() {} // expected-note {{distributed actor-isolated instance method 'f()' declared here}} | ||
| func f() {} // expected-note {{calls to instance method 'f()' from outside of its actor context are implicitly asynchronous}} |
There was a problem hiding this comment.
yeah this message is good, the "more general" isolation rules just apply here 👍
| @@ -2434,41 +2415,6 @@ namespace { | |||
| return ReferencedActor(var, isPotentiallyIsolated, ReferencedActor::NonIsolatedParameter); | |||
| } | |||
|
|
|||
| VarDecl *findReferencedBaseSelf(Expr *expr) { | |||
There was a problem hiding this comment.
oh sweet to get rid of this entire func, thanks!
| break; | ||
| } | ||
|
|
||
| // If we're calling an async function that's nonisolated, and we're in | ||
| // an isolated context, then we're exiting the actor context. |
There was a problem hiding this comment.
👍 that's not something we were diagnosing before right? Nice.
Isolation checking for calls had two separate implementation places:
one that looked at the declaration being called (for member
declarations) and one that worked on the actual call expression. Unify
on the latter implementation, which is more general and has access to
the specific call arguments. Improve diagnostics here somewher so we
don't regress in that area.
This refactoring shouldn't change the actual semantics, but it makes
upcoming semantic changes easier.