Skip to content

[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

Merged
merged 1 commit into from
Jul 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -4436,8 +4436,8 @@ NOTE(actor_isolated_sync_func,none,
"implicitly asynchronous",
(DescriptiveDeclKind, DeclName))
NOTE(distributed_actor_isolated_method_note,none,
"only 'distributed' functions can be called from outside the distributed actor",
())
"only 'distributed' functions can be called from outside the distributed actor", // TODO: improve error message
Copy link
Contributor Author

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"

())
ERROR(distributed_actor_isolated_method,none,
"only 'distributed' functions can be called from outside the distributed actor", // TODO: improve error message to be more like 'non-distributed' ... defined here
())
Expand Down
10 changes: 10 additions & 0 deletions include/swift/Runtime/Concurrency.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Expand Down
10 changes: 10 additions & 0 deletions include/swift/Runtime/RuntimeFunctions.def
Original file line number Diff line number Diff line change
Expand Up @@ -1671,6 +1671,16 @@ FUNCTION(DistributedActorInitializeRemote,
ARGS(RefCountedPtrTy), // TODO also address and transport?
ATTRS(NoUnwind))

// OpaqueValue* swift_distributedActor_remote_create(
// OpaqueValue *identity,
// OpaqueValue *transport);
FUNCTION(DistributedActorRemoteCreate,
swift_distributedActor_remote_create, SwiftCC,
ConcurrencyAvailability,
RETURNS(OpaquePtrTy), // TODO: is this the right type here?
ARGS(OpaquePtrTy, OpaquePtrTy),
ATTRS(NoUnwind))

// void swift_distributedActor_destroy(DefaultActor *actor); // TODO: ProxyActor *proxy?
FUNCTION(DistributedActorDestroy,
swift_distributedActor_destroy, SwiftCC,
Expand Down
1 change: 1 addition & 0 deletions lib/Sema/CodeSynthesisDistributedActor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ createDistributedActor_init_local(ClassDecl *classDecl,
}

// ==== Distributed Actor: Resolve Initializer ---------------------------------
// TODO: remove resolve initializer in favor of resolve static function

/// Synthesizes the body for
///
Expand Down
49 changes: 34 additions & 15 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ ActorIsolationRestriction ActorIsolationRestriction::forDeclaration(
if (func->isAsyncContext())
isAccessibleAcrossActors = true;

// FIXME: move diagnosis out of this function entirely (!)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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: {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
}
}
Expand Down
12 changes: 9 additions & 3 deletions stdlib/public/Concurrency/Actor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1756,13 +1756,19 @@ void swift::swift_defaultActor_deallocateResilient(HeapObject *actor) {
metadata->getInstanceAlignMask());
}

// TODO: most likely where we'd need to create the "proxy instance" instead?
void swift::swift_distributedActor_remote_initialize(DefaultActor *_actor) { // FIXME: !!!!! remove distributed C++ impl not needed?
// TODO: most likely where we'd need to create the "proxy instance" instead? (most likely remove this and use swift_distributedActor_remote_create instead)
void swift::swift_distributedActor_remote_initialize(DefaultActor *_actor) { // FIXME: remove distributed C++ impl not needed?
auto actor = asImpl(_actor);
actor->initialize(/*remote=*/true);
}

void swift::swift_distributedActor_destroy(DefaultActor *_actor) { // FIXME: !!!!! remove distributed C++ impl not needed?
// TODO: missing implementation of creating a proxy for the remote actor
OpaqueValue* swift::swift_distributedActor_remote_create(OpaqueValue *identity,
OpaqueValue *transport) {
assert(false && "swift_distributedActor_remote_create is not implemented yet!");
}

void swift::swift_distributedActor_destroy(DefaultActor *_actor) { // FIXME: remove distributed C++ impl not needed?
// TODO: need to resign the address before we destroy:
// something like: actor.transport.resignIdentity(actor.address)

Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/Distributed/ActorTransport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Act.ID always is AnyActorIdentity anyway


// ==== ---------------------------------------------------------------------
Expand Down
43 changes: 33 additions & 10 deletions stdlib/public/Distributed/DistributedActor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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, *)
Expand Down Expand Up @@ -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.
///
Expand Down
25 changes: 25 additions & 0 deletions test/Concurrency/actor_isolation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.


// ----------------------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion test/Distributed/distributed_actor_initialization.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ distributed actor OK1 {
// ok, since all fields are initialized, the constructors can be synthesized
}

// TODO: test all the FIXITs in this file (!!!)
// TODO: test all the FIXITs in this file

@available(SwiftStdlib 5.5, *)
distributed actor Bad1 {
Expand Down
30 changes: 30 additions & 0 deletions test/Distributed/distributed_actor_isolation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ distributed actor DistributedActor_1 {
fatalError()
}

static func staticFunc() -> String { "" } // ok

// TODO: should be able to handle a static, global actor isolated function as well
// @MainActor
// static func staticMainActorFunc() -> String { "" } // ok

static distributed func staticDistributedFunc() -> String {
// expected-error@-1{{'distributed' functions cannot be 'static'}}{10-21=}
fatalError()
}

func test_inside() async throws {
_ = self.name
_ = self.computedMutable
Expand Down Expand Up @@ -126,12 +137,31 @@ func test_outside(
_ = distributed.id
_ = distributed.actorTransport

// ==== static functions
_ = distributed.staticFunc() // expected-error{{static member 'staticFunc' cannot be used on instance of type 'DistributedActor_1'}}
_ = DistributedActor_1.staticFunc()

// ==== non-distributed functions
_ = await distributed.hello() // expected-error{{only 'distributed' functions can be called from outside the distributed actor}}
_ = await distributed.helloAsync() // expected-error{{only 'distributed' functions can be called from outside the distributed actor}}
_ = try await distributed.helloAsyncThrows() // expected-error{{only 'distributed' functions can be called from outside the distributed actor}}
}

// ==== Protocols and static (non isolated functions)

@available(SwiftStdlib 5.5, *)
protocol P {
static func hello() -> String
}
@available(SwiftStdlib 5.5, *)
extension P {
static func hello() -> String { "" }
}

@available(SwiftStdlib 5.5, *)
distributed actor ALL: P {
}

// ==== Codable parameters and return types ------------------------------------

@available(SwiftStdlib 5.5, *)
Expand Down
23 changes: 23 additions & 0 deletions test/Distributed/distributed_actor_resolve.swift
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,

}