[Distributed] Require that SerReq can only be used with protocols #42348
Conversation
|
@swift-ci please smoke test |
| @@ -4631,6 +4631,9 @@ ERROR(distributed_actor_func_param_not_codable,none, | |||
| ERROR(distributed_actor_target_result_not_codable,none, | |||
| "result type %0 of %1 %2 does not conform to serialization requirement '%3'", | |||
| (Type, DescriptiveDeclKind, Identifier, StringRef)) | |||
| ERROR(distributed_actor_system_serialization_req_must_be_protocol,none, | |||
| "'SerializationRequirement' type witness %0 must be a protocol type", | |||
There was a problem hiding this comment.
I definitely think the term "type witness" should not be used in diagnostics. The requirement also isn't satisfied by a value, but rather a protocol conformance requirement. What about 'SerializationRequirement' must be satisfied by a protocol? Perhaps with a note that says 'SerializationRequirement' is used as a protocol conformance requirement
There was a problem hiding this comment.
... must be satisfied by protocol or protocol composition then maybe? This is going to be diagnosed in typealias SerializationRequirement = any ... position.
There was a problem hiding this comment.
Thanks! I'll reword somehow like this hmm
| if (auto alias = dyn_cast<TypeAliasType>(existentialRequirementTy.getPointer())) { | ||
| auto ty = alias->getDesugaredType(); | ||
| if (isa<ClassType>(ty) || isa<StructType>(ty) || isa<EnumType>(ty)) { | ||
| // SerializationRequirement cannot be class or struct nowadays |
There was a problem hiding this comment.
I think we should explain this better - it cannot be anything other then a protocol because it would be used to constrain a generic parameter associated with serialization methods.
There was a problem hiding this comment.
And might be check for !ty->is<ProtocolType>() to make it simpler :)
There was a problem hiding this comment.
I think you should use !ty->isConstraintType() which also includes protocol composition types and was added for exactly this purpose 🙂 I think the code right now also doesn't handle bound generic types, e.g. BoundGenericClassType, so checking specifically for constraint types would fix that.
There was a problem hiding this comment.
Yeah, isConstraintType is the best option here.
There was a problem hiding this comment.
I forgot that in the conformance it's written as an existential type, so this should actually check for isExistentialType() I think
There was a problem hiding this comment.
Great, thanks for the info!
There was a problem hiding this comment.
I'm still confused by this code. Why are we explicitly looking for a TypeAliasType here rather than doing a purely semantic check for "is existential type" and then verifying that the only requirements are protocol requirements?
There was a problem hiding this comment.
Sorry folks for the delay, back to it now after the WWDC crunch 🙏
Will send in a PR to clean this up according to comments -- thanks @hborla !
In order to in the future support non "ad-hoc" protocol requirements we must make it possible to express
T: SerializationRequirementin the future.Since the
:implies being a class or protocol, and currently we only have use cases for protocols, let us ban other things so we can figure it out in the near future.resolves rdar://91663499