Repository navigation
Serialization V2 generator: formatters inside collections (G-1) - #8726
Aaronontheweb merged 2 commits into
Conversation
A registered formatter (custom, or the built-in Address/ActorPath ones) now serves collection elements, dictionary keys and dictionary values, not only fields, for every collection shape the generator supports. The wire form is the array or map the collection already uses, with each element exactly what the formatter writes at field position. A collection keeps its structure through extraction even when a member is not natively representable; validation reports what formatter resolution left unusable (AKKASG003/AKKASG014), with the same diagnostics as before. Part of akkadotnet#8715
There was a problem hiding this comment.
Self-review: what each test proves and why the generator changed. Inline comments have the detail.
Test groups
- Round trips and exact bytes: formatters run per list element, dictionary key and dictionary value.
- Golden and wire snapshots: existing generated code and wire bytes did not move.
- Diagnostics: shapes that were errors before are still errors, with the same text.
- Caching: the new model stays value-equal, so incremental builds still cache.
Overlap
- Hand-built byte tests in
FormatterCollectionSpecrepeat much of what theformatter-*wire snapshots pin. Should_EmitOneFormatterFieldPerTargetmostly repeats the golden file.
Not covered
- A formatter returning
UnknownSizeat element position (the size guard only appears as emitted text). - The CS8620 gap for
List<string?>?(tracked in #8728).
Fixed since this review (e9be192): UnknownSize element test added; duplicate byte tests, the golden-count test and the control diagnostic test removed; snapshot theory renamed.
| /// read temporaries and the nil handling depend on them) and takes its value-type-ness from the | ||
| /// formatter's target, mirroring <see cref="FormatterInfo.IsTargetValueType"/> at field position. | ||
| /// </summary> | ||
| private static TypeMapping ResolveCollectionFormatters(TypeMapping collection, Dictionary<TypeKey, FormatterInfo> formattersByTarget) |
There was a problem hiding this comment.
We resolve formatters inside collections here because they used to apply only to a field itself. Each element, key and value with a registered formatter becomes a Formatted mapping, nested collections included. The collection stays an ordinary array or map, so only the element bytes come from the formatter.
| // reference target, which a collection may hold a null of whatever its annotation says, or a | ||
| // Nullable<T> value target) writes nil for null, which the formatter itself is never asked to | ||
| // represent (see the IAkkaMessagePackFormatter Write remarks). | ||
| if (mapping.Kind == FieldKind.Formatted) |
There was a problem hiding this comment.
We write, read and size a formatted element in these three branches because the element must be exactly what the formatter writes at field position, with no wrapper. Null (or a missing Nullable<T>) becomes nil and the formatter never sees it; SizeOf is summed and stops with UnknownSize if any formatter reports it.
| /// resolution position-independent. A mapping that already names its type (Object, Enum, ...) keeps | ||
| /// the key it has; a generic or array type stays keyless and can never match a formatter. | ||
| /// </summary> | ||
| private static TypeMapping WithFallbackKey(TypeMapping mapping, ITypeSymbol type) |
There was a problem hiding this comment.
We stamp the type key on collection elements, keys and values here, the same way fields get it, because formatter lookup is by that key. Without it a List<Address> element never matches a formatter.
|
|
||
| collapsed = default; | ||
| return false; | ||
| return new TypeMapping(kind, typeArguments: ImmutableArray.Create(key, value)); |
There was a problem hiding this comment.
We delete the up-front collapse of an unusable element because that decision now needs the serializer's formatters, which extraction doesn't have. The collection keeps its shape, and validation reports what is still unusable.
| /// ties in. <paramref name="deciding"/> is the leaf (or, for a non-collection field, the field's | ||
| /// own mapping) the diagnostic's details come from. | ||
| /// </summary> | ||
| private static FieldKind GetDiagnosticKind(FieldInfo field, out TypeMapping deciding) |
There was a problem hiding this comment.
We pick the diagnostic from the first unusable leaf here because it reproduces what the old collapse did: AKKASG003 with the full field type, or AKKASG014 for a wide enum, key before value. Output for code that failed before should not change.
| /// serializer over <c>IProtocol</c>, a message <c>Outer</c> whose constructor is the caller's field list, | ||
| /// a <c>Foreign</c> type with a ready-made formatter, and a <c>long</c>-backed enum. | ||
| /// </summary> | ||
| internal static class DiagnosticSource |
There was a problem hiding this comment.
We add this template so each diagnostics test can swap one field type into a fixed serializer, message, Foreign type and long enum. It is test setup only and proves nothing by itself.
| /// <c>WireSnapshots/README.md</c>); a separate spec so those existing cases, and their snapshots, stay | ||
| /// untouched. Every input is a hardcoded constant, so a mismatch always means the wire format changed. | ||
| /// </summary> | ||
| public sealed class WireFormatSnapshotFormatterSpec : IAsyncLifetime |
There was a problem hiding this comment.
We commit hex dumps of seven messages (WireSnapshots/formatter-*.verified.txt) because it proves the bytes on the wire for formatted collection members never change by accident, via Verify failing on any diff. Inputs are fixed constants, so a diff always means the format moved. Sets and dictionaries hold one item each so hash order can't flake.
Add a test that runs a formatter returning UnknownSize at element, key and value position. Drop hand-built byte tests that the formatter-* wire snapshots already pin, the one-formatter-field golden check, and the AKKASG-free control diagnostics test. Rename the snapshot theory so it names formatter shapes only.
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
Adding support for more collections types in the source generator and tests to validate that the byte signature for their output doesn't change in the future (unless we do it intentionally.)
LGTM
…stacked on #8726 (#8727) * Serialization V2 generator: formatters inside collections (G-1) A registered formatter (custom, or the built-in Address/ActorPath ones) now serves collection elements, dictionary keys and dictionary values, not only fields, for every collection shape the generator supports. The wire form is the array or map the collection already uses, with each element exactly what the formatter writes at field position. A collection keeps its structure through extraction even when a member is not natively representable; validation reports what formatter resolution left unusable (AKKASG003/AKKASG014), with the same diagnostics as before. Part of #8715 * Serialization V2 generator: native TimeSpan and small scalars (G-3) TimeSpan, float, short, byte, sbyte, ushort, uint, ulong and char are now native, as fields, Nullable fields, and collection elements, keys and values. Wire encodings: TimeSpan is the MessagePack integer (int64) of its Ticks; float is float32; short/sbyte/byte/ushort/uint/ulong use the smallest MessagePack integer encoding, as int and long do; char is the unsigned integer of its UTF-16 code unit. Reads range-check and throw a MessagePackSerializationException naming the type instead of truncating. AKKASG007 for a type from another assembly no longer claims the generator cannot read referenced schemas, and no longer suggests declaring a BCL type in this assembly. Closes #8715 * Formatter collection tests: cover UnknownSize, drop duplicates Add a test that runs a formatter returning UnknownSize at element, key and value position. Drop hand-built byte tests that the formatter-* wire snapshots already pin, the one-formatter-field golden check, and the AKKASG-free control diagnostics test. Rename the snapshot theory so it names formatter shapes only. * Native scalars: range-check TimeSpan reads, widen range-check coverage, trim duplicates TimeSpan now reads through a range-checked helper like the other types, so a wire value beyond int64 or of the wrong type throws a MessagePackSerializationException naming System.TimeSpan. The integer and float readers also name the type when the wire holds the wrong MessagePack type. The out-of-range theory now covers every type at field, Nullable, list element and dictionary value position, with uint64-beyond-int64, below -float.MaxValue and wrong-wire-type cases, and checks the message names the type. Drop the test of MessagePackWriter's own integer widths and the all-null byte check the scalars-nullable-all-null snapshot already pins. Rename the scalar snapshot theory. * Import Akka.TestKit for HexDumpFormatter after merging dev (#8725 moved it)
1 participant
First of two stacked PRs for #8715 (G-1; the native scalars in G-3 follow in the second). Formatters, built-in (
Address,ActorPath) or custom (including one forIActorRef), now apply to collection elements, dictionary keys and dictionary values, not only to fields. Wire form is the array or map the collection already uses, with each element exactly what the formatter writes at field position, no wrapper.Changes
Nullable<T>elements are written asnil; the formatter only sees present values.SizeOfis summed, so size hints stay exact.Testing
IActorRef, a custom reference and a custom value-type formatter, across every shape and position), diagnostics for shapes that stay unsupported (for exampleIImmutableDictionary, which is G-2), and an incremental-caching test.ArteryControlMessageSerializer(generated) passes its tests.Breaking changes
None. Output for existing types is byte-identical.
Generator gaps found
ImmutableArray(for exampleList<string?>?) produces CS8620 in the generated code.Part of #8715