Make Test.Attachment generic.
#814
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grynspan
added
enhancement
grynspan
requested review from
stmontgomery,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
@swift-ci test |
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
plemarquand
reviewed
Nov 8, 2024
| _attachableValue | ||
| } | ||
| #endif | ||
| public var attachableValue: AttachableValue | ||
|
|
||
| /// The path to which the this attachment was written, if any. |
There was a problem hiding this comment.
Suggested change
| /// The path to which the this attachment was written, if any. | |
| /// The path to which the attachment was written, if any. |
plemarquand
reviewed
Nov 8, 2024
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test Windows |
This PR makes `Test.Attachment` generic over its attachable value type. It gains conditional conformance to `Copyable` and `Sendable` depending on the attachable value and if you call `attach()` on a move-only or non-sendable attachment, will eagerly serialize the attachable value at that point (rather than during initialization.) There are a few benefits here: 1. Callers can statically know the type of the attachable value in an attachment rather than needing to always deal with an existential box; 2. We can add associated types to `Test.Attachable` that will be readily accessible in `withUnsafeBufferPointer(for:_:)` again without needing an existential; and 3. When we eventually add support for image attachments, we won't need a bunch of additional initializers or intermediate box types or what-have-you; and 4. For Embedded Swift or other environments where existentials are problematic, we can eagerly serialize all attachments and pass a consistent type (`Test.Attachment<[UInt8]>`) to the event handler. There are also some drawbacks: 1. Because conformance to `Copyable` and `Sendable` is conditional, we lose a bit of flexibility if you have a non-sendable `Test.Attachment` instance or whatnot; 2. We still need a lazy, type-erased attachment type that can be passed to the event handler. I played around with `Test.Attachment<Any>` but that causes as many problems as it solves. We end up with `Test.Attachment<any Test.Attachable & Sendable & Copyable>` but, because that's an existential type that doesn't conform to itself, the generic parameter `AttachableValue` is not constrained to `Test.Attachable`. We only provide initializers for types that do conform though (plus the existential one internally) so in practice it's not a huge issue. 3. There is some code duplication necessary (i.e. multiple implementations of `attach()` and `write()`.)
…ttachment is a pain to use with it
…es since we'll need them for image attachments
…le, don't limit our options more than we have to
grynspan
force-pushed
the
jgrynspan/generic-attachment
branch
from
6d28bcc to
ec9542d
Compare
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
stmontgomery
approved these changes
Nov 11, 2024
|
@swift-ci test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes
Test.Attachmentgeneric over its attachable value type. It gains conditional conformance toCopyableandSendabledepending on the attachable value and if you callattach()on a move-only or non-sendable attachment, will eagerly serialize the attachable value at that point (rather than during initialization.)There are a few benefits here:
Test.Attachablethat will be readily accessible inwithUnsafeBufferPointer(for:_:)again without needing an existential; andTest.Attachment<[UInt8]>Test.Attachment<Test.AnyAttachable>) to the event handler.There are also some drawbacks:
Because conformance to
CopyableandSendableis conditional, we lose a bit of flexibility if you have a non-sendableTest.Attachmentinstance or whatnot;We still need a lazy, type-erased attachment type that can be passed to the event handler. I played around with
Test.Attachment<Any>but that causes as many problems as it solves.We end up withTest.Attachment<any Test.Attachable & Sendable & Copyable>but, because that's an existential type that doesn't conform to itself, the generic parameterAttachableValueis not constrained toTest.Attachable. We only provide initializers for types that do conform though (plus the existential one internally) so in practice it's not a huge issue.I replaced
any Test.Attachable & Sendable & Copyablewith a dedicated type-erasing box type,Test.AnyAttachable, that wraps the existential. In Embedded Swift, it wraps a[UInt8]instead.There is some code duplication necessary (i.e. multiple implementations of
attach()and.)write()Non-final classes cannot conform to
Test.Attachable. You can work around this with a box type that's generic over the class and conforms toTest.Attachable: theTest.AttachableContainerprotocol now exists to make this easier.Checklist: