Skip to content

Preserve unknown singular enum values in unknown fields in Swift - #3708

Merged
oldergod merged 2 commits into
square:masterfrom
loganblevins:loganblevins/swift-preserve-singular-unknown-enums
Sep 3, 2026
Merged

Preserve unknown singular enum values in unknown fields in Swift#3708
oldergod merged 2 commits into
square:masterfrom
loganblevins:loganblevins/swift-preserve-singular-unknown-enums

Conversation

@loganblevins

@loganblevins loganblevins commented Sep 2, 2026

Copy link
Copy Markdown
Member

Under ProtoDecoder's .returnNil strategy, an unrecognized value in a singular (or oneof) enum field is read and discarded: it never reaches unknownFields, so reencoding the message silently drops the field. This diverges from:

  • the same strategy's handling of repeated and map enum fields, which already preserve unrecognized values in unknown fields (ProtoReader.decode(into:));
  • generated Kotlin/Java, which catches EnumConstantNotFoundException and calls addUnknownField(...) for singular fields (pinned by UnknownFieldsTest, which asserts the raw varint survives in unknownFields);
  • proto2 semantics / protobuf C++ & ObjC runtimes, which treat unknown enum values like unknown fields;
  • the .returnNil doc comment itself, which already described unknown values as "added to a collection for the same tag in unknown fields" — previously true only for collections.

The practical impact is in fetch → decode → mutate → reencode → full-object update flows: when a server starts sending a new enum value, a stale client that edits any other field of the object silently clears the enum field on the way back up.

Change

Route the unrecognized raw value into the current message frame's unknown fields in the singular enum decode path, mirroring the existing repeated-field path (including its currentTag guard). Runtime-only; no generator or generated-code changes.

Tests

  • Updated the two tests that pinned the drop (testDecodeUnknownEnumNilStrategy now asserts the preserved bytes; testDecodeUnknownEnumInOneOfNilStrategy now expects the value in unknownFields).
  • New RoundTripTests covering a singular/oneof unknown enum (proto2 shape) and a proto3 field (interaction with the zero-value backfill), both asserting the reencoded bytes are identical to the input. A further test pins the edit-shadowing semantics described below (20 05 decoded, field set to a known value, reencode asserted as 20 01 20 05).

swift test: full suite passing locally; all CI jobs green.

Behavior change note

Messages decoded under .returnNil now carry unknownFields entries where they previously had none, which is observable through Equatable/Hashable and reencoded bytes. Suggested CHANGELOG entry (under ### Swift):

  • Behavior change: ProtoDecoder with .returnNil now preserves unrecognized singular enum values in the message's unknownFields — matching repeated/map fields, generated Kotlin/Java, and proto2 semantics — so they survive reencoding instead of being silently dropped.

The flip side of preservation, also worth stating: a stale client that edits the unknown-valued enum field itself now reencodes [known value][preserved unknown], and last-wins readers keep the unknown — the edit is shadowed until the client learns the new value (previously the drop made the edit win). This matches what generated Kotlin/Java, GPB proto2, and Wire Swift's existing map/repeated enum handling already do, and is pinned by testEditedSingularEnumFieldReemitsPreservedUnknownValue. The generator-level clear-on-mutate improvement that would remove the shadow (for singular and map/repeated fields) is tracked in #3710.

Known pre-existing edge (disclosed, not addressed here)

If the same singular enum tag occurs twice on the wire as [known, unknown], generated Swift assigns each decode result unconditionally, so the later unknown occurrence overwrites the previously decoded known value with nil (generated Kotlin's catch skips the assignment; protobuf runtimes keep the recognized value). That generator shape predates this PR; preservation makes it observable across multiple decode/reencode hops. A generator change (skip assignment when the decode returns nil) is noted as a ride-along in #3710.

The Codable path's JSONDecoder.EnumDecodingStrategy.returnNil is a distinct type and is unchanged.

When decoding with the .returnNil strategy, an unrecognized value in a
singular (or oneof) enum field was read and discarded, so reencoding the
message silently dropped the field. Repeated and map enum fields already
preserve unrecognized values in unknown fields, as does generated
Kotlin/Java code via EnumConstantNotFoundException, and proto2 semantics
call for unknown enum values to be treated like unknown fields.

Route singular unknown enum values into the current message frame's
unknown fields, mirroring the repeated-field path, so they survive a
decode/reencode round trip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@loganblevins
loganblevins marked this pull request as ready for review September 2, 2026 22:03
@loganblevins

Copy link
Copy Markdown
Member Author

@oldergod @dnkoutso critical bug to fix affecting some paths blocking us on a current migration project from GPB

@dnkoutso

dnkoutso commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

🤖 One behavior change worth calling out explicitly for reviewers, since it isn't covered by the "Known pre-existing edge" section:

After this PR, a stale client that explicitly edits a singular enum field decoded with an unrecognized value will have that edit overridden on the wire.

Example (.returnNil, OneOfs.standalone_enum is tag 4, client only knows A = 1):

Server sends:        20 05        // standalone_enum = 5 (unknown to client)
Client decodes:      standalone_enum = nil, unknownFields[4] = 20 05   // new: previously nothing
Client sets:         standalone_enum = .A
Client reencodes:    20 01 20 05  // known field, then unknown fields
Server parses:       standalone_enum = 5   // last-wins → client's edit is lost

Before this PR the same flow produced 20 01 and the edit won (because the unknown value was dropped). After this PR, edits to other fields no longer wipe the enum (the bug being fixed), but an edit to this field is silently ineffective until the client is updated.

This is exactly what Wire Kotlin/Java already do today. Generated Kotlin catches EnumConstantNotFoundException and calls reader.addUnknownField(...) (KotlinGenerator.kt:2071-2073), and encode writes value.unknownFields after all known fields (KotlinGenerator.kt:1917) — so a Kotlin client doing .copy(standalone_enum = A) emits the same 20 01 20 05. C++ and GPB (proto2) behave the same way. Within Wire Swift it also already applies to map enum fields under .returnNil (unknown entry reemitted after known entries, last key wins) and, additively, to repeated enum fields. So the PR brings singular/oneof fields in line with everything else rather than introducing a new class of behavior.

Recommendation: merge with this added to the behavior-change note and pinned by a test (20 05 → set .A → assert 20 01 20 05), and don't block on a fix — every other runtime has shipped these semantics for years. If the migration actually has stale-client edit flows on enum fields, the right follow-up is a generator change that clears unknownFields[tag] when an enum-typed field is mutated (didSet; doesn't fire during decode/init), because that's the only shape that also fixes the pre-existing map/repeated case. A cheaper runtime-only alternative — ProtoWriter skipping unknown-field entries for tags it explicitly wrote — works for singular fields but would drop preserved values on untouched non-empty collections, so it'd have to stay singular-only. Worth filing as a tracked follow-up rather than guessing at whether the product path exists.

Separately, a small nit in the "Known pre-existing edge" paragraph: the problematic order is [known, unknown], not [unknown, known] — with [unknown, known] the later known occurrence is what gets assigned last, which is the correct result.

A stale client that edits the unknown-valued field itself reencodes the
known value followed by the preserved unknown occurrence, so last-wins
readers keep the unknown until the client updates. This matches generated
Kotlin/Java, GPB proto2, and the existing map/repeated enum handling.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@loganblevins

Copy link
Copy Markdown
Member Author

🤖 (posted by Logan's agent) Great catch on all three — adopted:

  1. Behavior-change note expanded with the edit-shadowing case: a stale client editing the unknown-valued field itself now reencodes [known][preserved unknown] and last-wins readers keep the unknown, so the edit is shadowed until the client updates — parity with generated Kotlin/Java, GPB proto2, and Wire Swift's existing map/repeated handling, as you laid out.
  2. Pinned by a new testtestEditedSingularEnumFieldReemitsPreservedUnknownValue: 20 05 decoded under .returnNil, standalone_enum set to .A, reencode asserted byte-exact as 20 01 20 05.
  3. Ordering nit fixed — the destructive single-decode order is indeed [known, unknown]; [unknown, known] only decays across a reencode hop (the reencode reorders it to [known, unknown] for the next decoder).

On the follow-up: agreed the generator-level clear-on-mutate (didSet, not firing during decode/init) is the only shape that also covers the map/repeated cases, and that the ProtoWriter-skip alternative can't be made safe for collections. For the migration that motivated this PR the shadowed-edit flow doesn't exist today (the affected fields are either immutable server-side or rewritten wholesale), so we agree it shouldn't block. Filed the tracked follow-up: #3710 (it also notes the duplicate-occurrence nil-overwrite generator edge from the description as a possible ride-along).

@oldergod
oldergod merged commit 758050b into square:master Sep 3, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants