Skip to content

Make the actor transport an associated type of the DistributedActor protocol #39985

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.

Sign up for GitHub

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 2 commits into from
Nov 4, 2021
Merged

Make the actor transport an associated type of the DistributedActor protocol #39985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2ba06c
Make the actor transport an associated type of the DistributedActor p…
DougGregor Nov 2, 2021
f83775b
Fix a few tests for the actor transport associated type
DougGregor Nov 3, 2021
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
2 changes: 2 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4417,6 +4417,8 @@ ERROR(actor_protocol_illegal_inheritance,none,
ERROR(distributed_actor_protocol_illegal_inheritance,none,
"non-distributed actor type %0 cannot conform to the 'DistributedActor' protocol",
(DeclName))
ERROR(broken_distributed_actor_requirement,none,
"DistributedActor protocol is broken: unexpected requirement", ())

ERROR(unowned_executor_outside_actor,none,
"'unownedExecutor' can only be implemented within the main "
Expand Down
2 changes: 2 additions & 0 deletions include/swift/AST/KnownIdentifiers.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ IDENTIFIER(decode)
IDENTIFIER(decodeIfPresent)
IDENTIFIER(Decoder)
IDENTIFIER(decoder)
IDENTIFIER(DefaultActorTransport)
IDENTIFIER_(Differentiation)
IDENTIFIER_WITH_NAME(PatternMatchVar, "$match")
IDENTIFIER(dynamicallyCall)
Expand Down Expand Up @@ -141,6 +142,7 @@ IDENTIFIER_WITH_NAME(SwiftObject, "_TtCs12_SwiftObject")
IDENTIFIER(SwiftNativeNSObject)
IDENTIFIER(to)
IDENTIFIER(toRaw)
IDENTIFIER(Transport)
IDENTIFIER(Type)
IDENTIFIER(type)
IDENTIFIER(typeMismatch)
Expand Down
57 changes: 37 additions & 20 deletions lib/SILGen/SILGenDistributed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,22 +45,45 @@ static VarDecl* lookupProperty(NominalTypeDecl *ty, DeclName name) {
return dyn_cast<VarDecl>(refs.front());
}

/// Emit a reference to a specific stored property of the actor.
static SILValue emitActorPropertyReference(
SILGenFunction &SGF, SILLocation loc, SILValue actorSelf,
VarDecl *property) {
Type formalType = SGF.F.mapTypeIntoContext(property->getInterfaceType());
SILType loweredType = SGF.getLoweredType(formalType).getAddressType();
#if false
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a left-over from debugging?

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoop, I'll remove that in a follow up

if (!loweredType.isAddress()) {
loweredType = SILType::getPrimitiveAddressType(
formalType->getCanonicalType());
}
#endif
return SGF.B.createRefElementAddr(loc, actorSelf, property, loweredType);
}

/// Perform an initializing store to the given property using the value
/// \param actorSelf the value representing `self` for the actor instance.
/// \param prop the property to be initialized.
/// \param value the value to use when initializing the property.
static void initializeProperty(SILGenFunction &SGF, SILLocation loc,
SILValue actorSelf,
VarDecl* prop, SILValue value) {
CanType propType = SGF.F.mapTypeIntoContext(prop->getInterfaceType())
->getCanonicalType();
SILType loweredPropType = SGF.getLoweredType(propType);
auto fieldAddr = SGF.B.createRefElementAddr(loc, actorSelf, prop, loweredPropType);
Type formalType = SGF.F.mapTypeIntoContext(prop->getInterfaceType());
SILType loweredType = SGF.getLoweredType(formalType);

auto fieldAddr = emitActorPropertyReference(SGF, loc, actorSelf, prop);

if (fieldAddr->getType().isAddress())
if (loweredType.isAddressOnly(SGF.F)) {
SGF.B.createCopyAddr(loc, value, fieldAddr, IsNotTake, IsInitialization);
else
SGF.B.emitStoreValueOperation(loc, value, fieldAddr, StoreOwnershipQualifier::Init);
} else {
if (value->getType().isAddress()) {
value = SGF.B.createTrivialLoadOr(
loc, value, LoadOwnershipQualifier::Copy);
}

value = SGF.B.emitCopyValueOperation(loc, value);
SGF.B.emitStoreValueOperation(
loc, value, fieldAddr, StoreOwnershipQualifier::Init);
}
}

/******************************************************************************/
Expand Down Expand Up @@ -355,26 +378,20 @@ void SILGenFunction::emitResignIdentityCall(SILLocation loc,
FormalEvaluationScope scope(*this);

// ==== locate: self.id
auto *idVarDeclRef = lookupProperty(actorDecl, ctx.Id_id);
CanType idType = F.mapTypeIntoContext(
idVarDeclRef->getInterfaceType())->getCanonicalType();
auto idRef = B.createRefElementAddr(loc, actorSelf, idVarDeclRef,
getLoweredType(idType));
auto idRef = emitActorPropertyReference(
*this, loc, actorSelf.getValue(), lookupProperty(actorDecl, ctx.Id_id));

// ==== locate: self.actorTransport
auto transportVarDeclRef = lookupProperty(actorDecl, ctx.Id_actorTransport);
CanType transportType = F.mapTypeIntoContext(
transportVarDeclRef->getInterfaceType())->getCanonicalType();
auto transportRef =
B.createRefElementAddr(loc, actorSelf, transportVarDeclRef,
getLoweredType(transportType));
auto transportRef = emitActorPropertyReference(
*this, loc, actorSelf.getValue(),
lookupProperty(actorDecl, ctx.Id_actorTransport));

// Perform the call.
emitActorTransportWitnessCall(
B, loc, ctx.Id_resignIdentity,
transportRef.getValue(),
transportRef,
SILType(),
{ idRef.getValue() });
{ idRef });
}

void
Expand Down
13 changes: 4 additions & 9 deletions lib/SILOptimizer/Utils/DistributedActor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ void emitActorTransportWitnessCall(
if (methodSILFnTy->getSelfParameter().isFormalIndirect() &&
!transport->getType().isAddress()) {
auto buf = B.createAllocStack(loc, transport->getType(), None);
transport = B.emitCopyValueOperation(loc, transport);
B.emitStoreValueOperation(
loc, transport, buf, StoreOwnershipQualifier::Init);
temporaryTransportBuffer = SILValue(buf);
Expand Down Expand Up @@ -169,18 +170,12 @@ void emitActorTransportWitnessCall(
// If we had to create a buffer to pass the transport
if (temporaryTransportBuffer) {
emitCleanup([&](SILBuilder & builder) {
auto value = builder.emitLoadValueOperation(
loc, *temporaryTransportBuffer, LoadOwnershipQualifier::Take);
builder.emitDestroyValueOperation(loc, value);
builder.createDeallocStack(loc, *temporaryTransportBuffer);
});
}

// If we opened an existential, then destroy the existential.
#if false
if (openedExistential) {
emitCleanup([&](SILBuilder & builder) {
builder.emitDestroyAddr(loc, transport);
});
}
#endif
}

void emitActorReadyCall(SILBuilder &B, SILLocation loc, SILValue actor,
Expand Down
53 changes: 49 additions & 4 deletions lib/Sema/DerivedConformanceDistributedActor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ static FuncDecl *deriveDistributedActor_resolve(DerivedConformance &derived) {
return param;
};

Type addressType = C.getAnyActorIdentityDecl()->getDeclaredInterfaceType();
Type transportType = getDistributedActorTransportType(decl);
auto addressType = C.getAnyActorIdentityDecl()->getDeclaredInterfaceType();
auto transportType = getDistributedActorTransportType(decl);

// (_ identity: AnyActorIdentity, using transport: ActorTransport)
auto *params = ParameterList::create(
Expand Down Expand Up @@ -133,10 +133,11 @@ static ValueDecl *deriveDistributedActor_actorTransport(

// ```
// nonisolated
// let actorTransport: ActorTransport
// let actorTransport: Transport
// ```
// (no need for @actorIndependent because it is an immutable let)
Type propertyType = getDistributedActorTransportType(derived.Nominal);
auto propertyType = getDistributedActorTransportType(derived.Nominal);

VarDecl *propDecl;
PatternBindingDecl *pbDecl;
std::tie(propDecl, pbDecl) = derived.declareDerivedProperty(
Expand All @@ -154,6 +155,36 @@ static ValueDecl *deriveDistributedActor_actorTransport(
return propDecl;
}

static Type deriveDistributedActor_Transport(
DerivedConformance &derived) {
assert(derived.Nominal->isDistributedActor());
auto &C = derived.Context;

// Look for a type DefaultActorTransport within the parent context.
auto defaultTransportLookup = TypeChecker::lookupUnqualified(
derived.getConformanceContext()->getModuleScopeContext(),
DeclNameRef(C.Id_DefaultActorTransport),
derived.ConformanceDecl->getLoc());
TypeDecl *defaultTransportTypeDecl = nullptr;
for (const auto &found : defaultTransportLookup) {
if (auto foundType = dyn_cast_or_null<TypeDecl>(found.getValueDecl())) {
if (defaultTransportTypeDecl) {
// Note: ambiguity, for now just fail.
return nullptr;
}

defaultTransportTypeDecl = foundType;
continue;
}
}

// There is no default, so fail to synthesize.
if (!defaultTransportTypeDecl)
return nullptr;

// Return the default transport type.
return defaultTransportTypeDecl->getDeclaredInterfaceType();
}
Copy link
Contributor

@ktoso ktoso Nov 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So... this means that we look in the current module for DefaultActorTransport, and that dictates the type of the Transport property... I remain worried about this somehow... What happens if there is no default one defined, you'd suggest that each actor must explicitly define the typealias, right?

// no default transport
distributed actor X {} // error: no DefaultActorTransport, provide `typealias Transport` pls
typealias DefaultActorTransport = ClusterTransport // example
distributed actor X {} // ok
X(transport: SomeOtherTransport()) // error, expected ClusterTransport

right?

This piece I like, that makes sense for applications which will usually have one transport.

I was hoping we can derive it from the init declarations:

distributed actor A {
  init(t: MyTransport) {}
  // therefore:
  typealias Transport = MyTransport
}

and also once all transport types must "match":

distributed actor A {
  init(t: MyTransport) {}
  init(number: Int, other: OtherTransport) {}
  // ERROR: distributed actor initializers must accept 
  // the same transport type in all initializers

The DefaultActorTransport we could still keep though! It's really be the default, but this init declaration dance I wonder if would not be worth it...? Maybe let's ignore for now hm...

Copy link
Member Author

Choose a reason for hiding this comment

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

As I've noted in my newly-written comment, I expect that any library providing a transport would provide a DefaultActorTransport typealias pointing at that transport, so importing the library + defining a distributed actor would use that transport. Inference from init(transport:...) would also be possible.

// ==== ------------------------------------------------------------------------

ValueDecl *DerivedConformance::deriveDistributedActor(ValueDecl *requirement) {
Expand All @@ -174,3 +205,17 @@ ValueDecl *DerivedConformance::deriveDistributedActor(ValueDecl *requirement) {

return nullptr;
}

std::pair<Type, TypeDecl *> DerivedConformance::deriveDistributedActor(
AssociatedTypeDecl *assocType) {
if (!canDeriveDistributedActor(Nominal, cast<DeclContext>(ConformanceDecl)))
return std::make_pair(Type(), nullptr);

if (assocType->getName() == Context.Id_Transport) {
return std::make_pair(deriveDistributedActor_Transport(*this), nullptr);
}

Context.Diags.diagnose(assocType->getLoc(),
diag::broken_distributed_actor_requirement);
return std::make_pair(Type(), nullptr);
}
6 changes: 6 additions & 0 deletions lib/Sema/DerivedConformances.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,12 @@ class DerivedConformance {
/// \returns the derived member, which will also be added to the type.
ValueDecl *deriveDistributedActor(ValueDecl *requirement);

/// Derive a DistributedActor associated type for a distributed actor.
///
/// \returns the derived type member, which will also be added to the type.
std::pair<Type, TypeDecl *> deriveDistributedActor(
AssociatedTypeDecl *assocType);

/// Determine if \c Actor can be derived for the given type.
static bool canDeriveActor(DeclContext *DC, NominalTypeDecl *NTD);

Expand Down
23 changes: 11 additions & 12 deletions lib/Sema/TypeCheckDistributed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,20 +184,15 @@ void swift::checkDistributedActorConstructor(const ClassDecl *decl, ConstructorD
if (!ctor->isDesignatedInit())
return;

// === Designated initializers must accept exactly one ActorTransport
auto &C = ctor->getASTContext();
auto module = ctor->getParentModule();

// === Designated initializers must accept exactly one actor transport that
// matches the actor transport type of the actor.
SmallVector<ParamDecl*, 2> transportParams;
int transportParamsCount = 0;
auto protocolDecl = C.getProtocol(KnownProtocolKind::ActorTransport);
auto protocolTy = protocolDecl->getDeclaredInterfaceType();

Type transportTy = ctor->mapTypeIntoContext(
getDistributedActorTransportType(const_cast<ClassDecl *>(decl)));
for (auto param : *ctor->getParameters()) {
auto paramTy = ctor->mapTypeIntoContext(param->getInterfaceType());
auto conformance = TypeChecker::conformsToProtocol(paramTy, protocolDecl, module);

if (paramTy->isEqual(protocolTy) || !conformance.isInvalid()) {
if (paramTy->isEqual(transportTy)) {
transportParamsCount += 1;
transportParams.push_back(param);
}
Expand Down Expand Up @@ -252,9 +247,13 @@ Type swift::getDistributedActorTransportType(NominalTypeDecl *actor) {
assert(actor->isDistributedActor());
auto &ctx = actor->getASTContext();

auto protocol = ctx.getProtocol(KnownProtocolKind::ActorTransport);
auto protocol = ctx.getProtocol(KnownProtocolKind::DistributedActor);
if (!protocol)
return ErrorType::get(ctx);

return protocol->getDeclaredInterfaceType();
// Dig out the actor transport type.
auto module = actor->getParentModule();
Type selfType = actor->getSelfInterfaceType();
auto conformance = module->lookupConformance(selfType, protocol);
return conformance.getTypeWitnessByName(selfType, ctx.Id_Transport);
}
2 changes: 2 additions & 0 deletions lib/Sema/TypeCheckProtocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6601,6 +6601,8 @@ TypeChecker::deriveTypeWitness(DeclContext *DC,
return std::make_pair(derived.deriveCaseIterable(AssocType), nullptr);
case KnownProtocolKind::Differentiable:
return derived.deriveDifferentiable(AssocType);
case KnownProtocolKind::DistributedActor:
return derived.deriveDistributedActor(AssocType);
default:
return std::make_pair(nullptr, nullptr);
}
Expand Down
40 changes: 39 additions & 1 deletion stdlib/public/Distributed/ActorTransport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,47 @@ public protocol ActorTransport: Sendable {
func assignIdentity<Act>(_ actorType: Act.Type) -> AnyActorIdentity
where Act: DistributedActor

func actorReady<Act>(_ actor: Act) where Act: DistributedActor
func actorReady<Act>(_ actor: Act)
where Act: DistributedActor

/// Called during actor deinit/destroy.
func resignIdentity(_ id: AnyActorIdentity)
}

@available(SwiftStdlib 5.6, *)
public struct AnyActorTransport: ActorTransport {
let transport: ActorTransport

public init<Transport: ActorTransport>(_ transport: Transport) {
self.transport = transport
}

public func decodeIdentity(from decoder: Decoder) throws -> AnyActorIdentity {
return try transport.decodeIdentity(from: decoder)
}

public func resolve<Act>(
_ identity: AnyActorIdentity, as actorType: Act.Type
) throws -> Act? where Act: DistributedActor {
return try transport.resolve(identity, as: actorType)
}

public func assignIdentity<Act>(_ actorType: Act.Type) -> AnyActorIdentity
where Act: DistributedActor {
return transport.assignIdentity(actorType)
}

public func actorReady<Act>(_ actor: Act)
where Act: DistributedActor {
transport.actorReady(actor)
}

/// Called during actor deinit/destroy.
public func resignIdentity(_ id: AnyActorIdentity) {
transport.resignIdentity(id)
}
}

/// Use the existential wrapper as the default actor transport.
@available(SwiftStdlib 5.6, *)
public typealias DefaultActorTransport = AnyActorTransport
11 changes: 7 additions & 4 deletions stdlib/public/Distributed/DistributedActor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ public protocol AnyActor: Sendable, AnyObject {}
@available(SwiftStdlib 5.6, *)
public protocol DistributedActor:
AnyActor, Sendable, Identifiable, Hashable, Codable {
/// The type of transport used to communicate with actors of this type.
associatedtype Transport: ActorTransport

/// Resolves the passed in `identity` against the `transport`, returning
/// either a local or remote actor reference.
///
Expand All @@ -52,7 +55,7 @@ public protocol DistributedActor:
// We want to move to accepting a generic or existential identity here
// static func resolve<Identity>(_ identity: Identity, using transport: ActorTransport)
// throws -> Self where Identity: ActorIdentity
static func resolve(_ identity: AnyActorIdentity, using transport: ActorTransport)
static func resolve(_ identity: AnyActorIdentity, using transport: Transport)
throws -> Self

/// The `ActorTransport` associated with this actor.
Expand All @@ -61,7 +64,7 @@ public protocol DistributedActor:
///
/// Conformance to this requirement is synthesized automatically for any
/// `distributed actor` declaration.
nonisolated var actorTransport: ActorTransport { get } // TODO: rename to `transport`?
nonisolated var actorTransport: Transport { get } // TODO: rename to `transport`?

/// Logical identity of this distributed actor.
///
Expand Down Expand Up @@ -100,9 +103,9 @@ extension CodingUserInfoKey {
@available(SwiftStdlib 5.6, *)
extension DistributedActor {
nonisolated public init(from decoder: Decoder) throws {
guard let transport = decoder.userInfo[.actorTransportKey] as? ActorTransport else {
guard let transport = decoder.userInfo[.actorTransportKey] as? Transport else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... I see, this is more restrictive but I think it'll be fine -- realistically people should not be mixing completely random transports and sending an actor from one transport to another transport, I suppose...

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's a good point, this might be a bug! We probably need AnyActorTransport to be Codable over top of the underlying transport, so we can decide it.

Copy link
Contributor

@ktoso ktoso Nov 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nah, transports are not intended to be Codable; This change seems fine.

I was thinking about a weird/obscure use-case that to be honest we don't have to support IMHO: "mixing transports". It would be very complicated and I don't think we really need it in any real use-case to be honest.

Here is the imagined problem:

  • you could receive a websocket://12.3.4:####/greeter identified actor... the WebSocketTransport decodes this fine...
  • you are ALSO part of the cluster, and send the same ref to some cluster actor...
  • encoding just works, it's Codable by the websocket address...
  • the recipient system, attempts to decode it
    • puts self into .actorTransportKey, that's the Cluster transport
    • that as? will fail, since this is Cluster and not WebSocket transport after all

To be honest this is likely good, you can't just randomly share refs like that as the connections and forwarding take a role in all that too... I think this is okey to keep as such runtime `"failed to decode websocket:/// actor using Cluster transport" it makes total sense and is easily fixed by developers with a "forwarder" actor if they need such chain of messages.

Why all of this doesn't really matter:

  • the connection of course didn't "move" with the actor as we sent it over like that...
  • so it does not make sense for that other system to resolve the "actual connection backed actor", since it can't, the connection is on the first node -- that accepted that websocket actor.

So yeah, such weird thing could happen, but honestly I think it's a weird edge case, and by means of our transports being explicit in the types -- we'd see that "huh, why am I sending this websocket actor to the cluster, does this make sense?" One can spin up a simple "forwarder" actor if we needed to support that.

throw DistributedActorCodingError(message:
"Missing ActorTransport (for key .actorTransportKey) " +
"Missing Transport (for key .actorTransportKey) " +
"in Decoder.userInfo, while decoding \(Self.self).")
}

Expand Down
3 changes: 3 additions & 0 deletions test/Distributed/Runtime/distributed_actor_decode.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ struct FakeTransport: ActorTransport {
}
}

@available(SwiftStdlib 5.5, *)
typealias DefaultActorTransport = FakeTransport

// ==== Test Coding ------------------------------------------------------------

class TestEncoder: Encoder {
Expand Down
Loading