-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Distributed] Prepare stubs for static func resolve #38530
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -579,9 +579,19 @@ SWIFT_EXPORT_FROM(swift_Concurrency) SWIFT_CC(swift) | |
void swift_defaultActor_deallocateResilient(HeapObject *actor); | ||
|
||
/// Initialize the runtime storage for a distributed remote actor. | ||
// TODO: this may end up being removed as we move to the "proxy creation" below | ||
SWIFT_EXPORT_FROM(swift_Concurrency) SWIFT_CC(swift) | ||
void swift_distributedActor_remote_initialize(DefaultActor *actor); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was still from initial attempts to make use of the default actor infra somewhat; but it seams instead we want to create the remote ref instead. So this function would go away completely. |
||
|
||
/// Create a proxy object that will serve as remote distributed actor instance. | ||
SWIFT_EXPORT_FROM(swift_Concurrency) SWIFT_CC(swift) | ||
OpaqueValue* swift_distributedActor_remote_create( | ||
/* +1 */OpaqueValue *identity, | ||
/* +1 */OpaqueValue *transport | ||
// metadata for identity | ||
// metadata for transport | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to pass the generic types here; not sure what we'll want to return so for now i just stubbed as an opaque value |
||
|
||
/// Destroy the runtime storage for a default actor. | ||
SWIFT_EXPORT_FROM(swift_Concurrency) SWIFT_CC(swift) | ||
void swift_distributedActor_destroy(DefaultActor *actor); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -385,6 +385,7 @@ ActorIsolationRestriction ActorIsolationRestriction::forDeclaration( | |
if (func->isAsyncContext()) | ||
isAccessibleAcrossActors = true; | ||
|
||
// FIXME: move diagnosis out of this function entirely (!) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do this cleanup shortly. but separately would be better so we have the new function in so @kavon can start poking at implementing it. |
||
if (func->isDistributed()) { | ||
if (auto classDecl = dyn_cast<ClassDecl>(decl->getDeclContext())) { | ||
if (!classDecl->isDistributedActor()) { | ||
|
@@ -2273,9 +2274,11 @@ namespace { | |
if (dyn_cast<ConstructorDecl>(member)) | ||
return false; | ||
|
||
// While the member may be unrestricted, perhaps | ||
// While the member may be unrestricted, perhaps it is in a | ||
// distributed actor, in which case we need to diagnose it. | ||
if (auto classDecl = dyn_cast<ClassDecl>(member->getDeclContext())) { | ||
if (classDecl->isDistributedActor()) { | ||
fprintf(stderr, "[%s:%d] (%s) HERE\n", __FILE__, __LINE__, __FUNCTION__); | ||
ctx.Diags.diagnose(memberLoc, diag::distributed_actor_isolated_method); | ||
noteIsolatedActorMember(member, context); | ||
return true; | ||
|
@@ -2312,32 +2315,43 @@ namespace { | |
!(isolatedActor.isActorSelf() && | ||
member->isInstanceMember() && | ||
isActorInitOrDeInitContext(getDeclContext()))) { | ||
// invocation on not-'self', is only okey if this is a distributed func | ||
// cross actor invocation is only ok for a distributed or static func | ||
if (auto func = dyn_cast<FuncDecl>(member)) { | ||
if (!func->isDistributed()) { | ||
if (func->isStatic()) { | ||
// FIXME: rather, never end up as distributed actor self isolated | ||
// at all for static funcs | ||
continueToCheckingLocalIsolation = true; | ||
} else if (func->isDistributed()) { | ||
tryMarkImplicitlyAsync(memberLoc, memberRef, context); | ||
tryMarkImplicitlyThrows(memberLoc, memberRef, context); | ||
|
||
// distributed func reference, that passes all checks, great! | ||
continueToCheckingLocalIsolation = true; | ||
} else { | ||
// the func is neither static or distributed | ||
ctx.Diags.diagnose(memberLoc, diag::distributed_actor_isolated_method); | ||
// TODO: offer a fixit to add 'distributed' on the member; how to test fixits? See also https://github.com/apple/swift/pull/35930/files | ||
noteIsolatedActorMember(member, context); | ||
return true; | ||
} | ||
|
||
assert(func->isDistributed()); | ||
tryMarkImplicitlyAsync(memberLoc, memberRef, context); | ||
tryMarkImplicitlyThrows(memberLoc, memberRef, context); | ||
|
||
// distributed func reference, that passes all checks, great! | ||
continueToCheckingLocalIsolation = true; | ||
} // end FuncDecl | ||
|
||
if (!continueToCheckingLocalIsolation) { | ||
// it wasn't a function (including a distributed function), | ||
// so we need to perform some more checks | ||
if (auto var = dyn_cast<VarDecl>(member)) { | ||
// TODO: we want to remove _distributedActorIndependent in favor of nonisolated | ||
// | ||
// @_distributedActorIndependent decls are accessible always, | ||
// regardless of distributed actor-isolation; e.g. actorAddress | ||
if (member->getAttrs().hasAttribute<DistributedActorIndependentAttr>()) | ||
return false; | ||
|
||
// nonisolated decls are accessible always | ||
if (member->getAttrs().hasAttribute<DistributedActorIndependentAttr>()) | ||
return false; | ||
|
||
// otherwise, no other properties are accessible on a distributed actor | ||
if (!continueToCheckingLocalIsolation) { | ||
ctx.Diags.diagnose( | ||
|
@@ -2347,13 +2361,16 @@ namespace { | |
noteIsolatedActorMember(member, context); | ||
return true; | ||
} | ||
} | ||
} // end VarDecl | ||
|
||
// TODO: would have to also consider subscripts and other things | ||
} | ||
} // end !isolatedActor | ||
|
||
return false; | ||
if (!continueToCheckingLocalIsolation) | ||
return false; | ||
|
||
LLVM_FALLTHROUGH; | ||
} | ||
|
||
case ActorIsolationRestriction::ActorSelf: { | ||
|
@@ -2428,9 +2445,10 @@ namespace { | |
case ActorIsolationRestriction::Unsafe: | ||
// This case is hit when passing actor state inout to functions in some | ||
// cases. The error is emitted by diagnoseInOutArg. | ||
|
||
if (auto classDecl = dyn_cast<ClassDecl>(member->getDeclContext())) { | ||
if (classDecl->isDistributedActor()) { | ||
auto classDecl = dyn_cast<ClassDecl>(member->getDeclContext()); | ||
if (classDecl && classDecl->isDistributedActor()) { | ||
auto funcDecl = dyn_cast<AbstractFunctionDecl>(member); | ||
if (funcDecl && !funcDecl->isStatic()) { | ||
member->diagnose(diag::distributed_actor_isolated_method); | ||
return true; | ||
} | ||
|
@@ -3164,7 +3182,8 @@ ActorIsolation ActorIsolationRequest::evaluate( | |
} | ||
|
||
if (auto nominal = value->getDeclContext()->getSelfNominalTypeDecl()) { | ||
if (nominal->isDistributedActor()) { | ||
/// Unless the function is static, it is isolated to the dist actor | ||
if (nominal->isDistributedActor() && !func->isStatic()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit nasty that we had to check ad-hoc here, i'll rethink our isolation in general... |
||
defaultIsolation = ActorIsolation::forDistributedActorInstance(nominal); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,7 @@ public protocol ActorTransport: Sendable { | |
/// | ||
/// Detecting liveness of such remote actors shall be offered / by transport libraries | ||
/// by other means, such as "watching an actor for termination" or similar. | ||
func resolve<Act>(_ identity: Act.ID, as actorType: Act.Type) throws -> ActorResolved<Act> | ||
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type) throws -> ActorResolved<Act> | ||
where Act: DistributedActor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Act.ID always is AnyActorIdentity anyway |
||
|
||
// ==== --------------------------------------------------------------------- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,20 +46,23 @@ public protocol DistributedActor: AnyActor, Identifiable, Hashable, Codable { | |
/// associated with. | ||
init(transport: ActorTransport) | ||
|
||
/// Resolves the passed in `address` against the `transport`, | ||
/// returning either a local or remote actor reference. | ||
@available(*, deprecated, renamed: "SomeDistributedActor.resolve(_:using:)") | ||
init(resolve id: AnyActorIdentity, using transport: ActorTransport) throws | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's completely ignore the existence of this; I'm only keeping it around so we don't have to remove useful tests that we have already. Once the resolve function exists I'll remove all backing infra for this init. |
||
|
||
/// Resolves the passed in `identity` against the `transport`, returning | ||
/// either a local or remote actor reference. | ||
/// | ||
/// The transport will be asked to `resolve` the address and return either | ||
/// a local instance or determine that a proxy instance should be created | ||
/// for this address. A proxy actor will forward all invocations through | ||
/// The transport will be asked to `resolve` the identity and return either | ||
/// a local instance or request a proxy to be created for this identity. | ||
/// | ||
/// A remote distributed actor reference will forward all invocations through | ||
/// the transport, allowing it to take over the remote messaging with the | ||
/// remote actor instance. | ||
/// | ||
/// - Parameter address: the address to resolve, and produce an instance or proxy for. | ||
/// - Parameter transport: transport which should be used to resolve the `address`. | ||
// FIXME: replace with static func resolve(_:using) | ||
// TODO: make this <ID>(resolve id: ID, ...) | ||
init(resolve id: AnyActorIdentity, using transport: ActorTransport) throws | ||
/// - Parameter identity: identity uniquely identifying a, potentially remote, actor in the system | ||
/// - Parameter transport: `transport` which should be used to resolve the `identity`, and be associated with the returned actor | ||
static func resolve<Identity>(_ identity: Identity, using transport: ActorTransport) | ||
throws -> Self where Identity: ActorIdentity | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new hottness ^^ |
||
|
||
/// The `ActorTransport` associated with this actor. | ||
/// It is immutable and equal to the transport passed in the local/resolve | ||
|
@@ -83,6 +86,23 @@ public protocol DistributedActor: AnyActor, Identifiable, Hashable, Codable { | |
nonisolated var id: AnyActorIdentity { get } | ||
} | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
extension DistributedActor { | ||
|
||
public static func resolve<Identity>(_ identity: Identity, using transport: ActorTransport) | ||
throws -> Self where Identity: ActorIdentity { | ||
switch try transport.resolve(AnyActorIdentity(identity), as: Self.self) { | ||
case .resolved(let instance): | ||
return instance | ||
|
||
case .makeProxy: | ||
// FIXME: this needs actual implementation of distributedActorRemoteCreate | ||
let remote: Any = distributedActorRemoteCreate(identity: identity, transport: transport) | ||
return remote as! Self | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'll actually get away with just implementing it like that... It depends how we'd want to make it available for protocols later on, but that's "just" a typechecking change, the implementation is always the same :-) |
||
|
||
// ==== Hashable conformance --------------------------------------------------- | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
|
@@ -210,6 +230,9 @@ func __isLocalActor(_ actor: AnyObject) -> Bool { | |
@_silgen_name("swift_distributedActor_remote_initialize") | ||
func _distributedActorRemoteInitialize(_ actor: AnyObject) | ||
|
||
@_silgen_name("swift_distributedActor_remote_create") | ||
func distributedActorRemoteCreate(identity: Any, transport: Any) -> Any // TODO: make it typed | ||
|
||
/// Called to destroy the default actor instance in an actor. | ||
/// The implementation will call this within the actor's deinit. | ||
/// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -853,6 +853,29 @@ actor MyActorP: P { | |
func h() { g() } | ||
} | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
protocol SP { | ||
static func s() | ||
} | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
actor ASP: SP { | ||
static func s() { } | ||
} | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
protocol SPD { | ||
static func sd() | ||
} | ||
@available(SwiftStdlib 5.5, *) | ||
extension SPD { | ||
static func sd() { } | ||
} | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
actor ASPD: SPD { | ||
} | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
func testCrossActorProtocol<T: P>(t: T) async { | ||
await t.f() | ||
|
@@ -863,6 +886,8 @@ func testCrossActorProtocol<T: P>(t: T) async { | |
t.g() | ||
// expected-error@-1{{expression is 'async' but is not marked with 'await'}}{{3-3=await }} | ||
// expected-note@-2{{calls to instance method 'g()' from outside of its actor context are implicitly asynchronous}} | ||
ASP.s() | ||
ASPD.sd() | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silly, but wanted to add coverage of those since I was touching static funcs in distributed actors. |
||
|
||
// ---------------------------------------------------------------------- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// RUN: %target-typecheck-verify-swift -enable-experimental-distributed | ||
// REQUIRES: concurrency | ||
// REQUIRES: distributed | ||
|
||
import _Distributed | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
distributed actor Capybara { } | ||
|
||
//@available(SwiftStdlib 5.5, *) | ||
//protocol Wheeker: DistributedActor { } | ||
//@available(SwiftStdlib 5.5, *) | ||
//distributed actor GuineaPing: Wheeker { } | ||
|
||
@available(SwiftStdlib 5.5, *) | ||
func test<Identity: ActorIdentity>(identity: Identity, transport: ActorTransport) async throws { | ||
let _: Capybara = try Capybara.resolve(identity, using: transport) | ||
|
||
// TODO: implement resolve being able to be called on a distributed actor protocol | ||
// (yes, normally it is not allowed to call such function... so we need to | ||
// discuss and figure out how we want to expose the resolve of a protocol) | ||
// let c: Wheeker = try Wheeker.resolve(.init(identity), using: transport) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is tricky future work, but leaving the TODO here as a reminder already; it's crucial we get this right, in however shape we'll surface it in the future, |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the note eventually I guess we'll just do a "declared here"