🍒[5.9][Executors] Clean up how we unwrap the local executor of distributed custom actor #64740
Conversation
|
@swift-ci please test |
| auto witnessCall = B.createApply(loc, witness, subs, {actor}); | ||
| // Find the unwrap function | ||
| FuncDecl *funcDecl = ctx.getGetUnwrapLocalDistributedActorUnownedExecutor(); | ||
| assert(funcDecl); |
There was a problem hiding this comment.
You are likely to end up having this code run against a standard library that doesn't have this function declaration, because it's older. Can we fail gracefully in such cases?
More urgently, doesn't the use of this new function break back-deployment for distributed actors?
There was a problem hiding this comment.
Discussed this and I believe we should address this a little bit, the proposed solution is here: #64800
It specifically is about resilient "old" distributed actors being potentially marked as non default while they should still be default
There was a problem hiding this comment.
Done in #64800 and cherry picked into this PR as well @DougGregor
ktoso
added
concurrency
|
@swift-ci please test |
| if (classDecl->isDistributedActor()) { | ||
| ASTContext &ctx = classDecl->getASTContext(); | ||
| auto customExecutorAvailability = | ||
| ctx.getConcurrencyDistributedActorWithCustomExecutorAvailability(); | ||
|
|
||
| auto actorAvailability = TypeChecker::overApproximateAvailabilityAtLocation( | ||
| classDecl->getStartLoc(), | ||
| classDecl); | ||
|
|
||
| if (!actorAvailability.isContainedIn(customExecutorAvailability)) { | ||
| // Any 'distributed actor' declared with availability lower than the | ||
| // introduction of custom executors for distributed actors, must be treated as default actor, | ||
| // even if it were to declared the unowned executor property, as older compilers | ||
| // do not have the the logic to handle that case. | ||
| return true; |
There was a problem hiding this comment.
Thank you! And thank you for noticing the issue to begin with!
Description: This removes a hacky workaround in how a "hop to" distributed actor is implemented.
Risk: Low, removes an internal underscored hack; The
_method could not have been used by any adopters yet.Review by: @DougGregor @slavapestov
Testing: CI testing
Original PR: #64719 #64800
This now includes fixes from (#64800) to how we deal with resilient cross module distributed actors; and treat pre 5.9 distributed actors as ALWAYS default, which we must do in order to properly backwards compatible handle such actors. Thank you @DougGregor for the review and catch.