reflect: preserve closed enum unknowns#304
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
…override gap The change is user-visible and belongs in the release notes. The reworded vtable test documents a real disagreement between the generated and reflective decoders over a field-level enum_type:OPEN override, which the runtime pool cannot see. Name the tracking issue in the test so the split is recorded somewhere a reader will find it, not only in the test's name. No-Verification-Needed: changelog fragment and a test comment
The branch predated the preserve-on-decode-error fix and the cross-pool adoption work, which added tests to the same regions of dynamic_e2e. Both sides are additive and independent, so keep both.
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] Thanks — this is the reflective twin of #313 (which landed earlier today for the generated decoder), and the part I most wanted to check came back clean: the allocation guard is not bypassed. record_unknown charges ctx.register_unknown_field()? before every push, and the granularity matches the owned side exactly — one charge per value for singular/oneof/extension and for each element of unpacked and packed repeated fields, and one charge per entry for maps, mirroring merge_entry_with_unknowns. That was the risk worth verifying: getting it wrong would have reintroduced on the reflective side the exact guard bypass #287/#313 just closed on the owned side. All six claimed contexts are implemented and each has a load-bearing test.
I pushed two commits rather than sending this back:
0269817adds the changelog fragment (user-visible behaviour, belongs in the 0.9.0 notes —check-changelogonly verifies CHANGELOG.md is in sync with existing fragments, so its passing wasn't evidence one wasn't needed), and links the tracking issue in the reworded vtable test.bc472d4merges current main. The branch predated today's six merges, and #299/#308 had added tests to the same regions ofdynamic_e2e.rs; both sides are additive so both are kept (26 tests pass together).
On the reworded test — open_enum_override_vtable_reflection_preserves_runtime_closed_semantics documents something real that deserved more than a rename. A field-level enum_type:OPEN override (#291) is invisible to the runtime pool: openness resolves per enum, and FieldDescriptor carries no override, so the generated decoder keeps EnumValue::Unknown(99) where reflection routes the same value to unknown fields and reports the field unset. That is a genuine owned/reflective split, and it stays green only because override_feature_in is a buffa extension outside the conformance corpus, so no mode exercises it. Filed as #316 with a fix sketch, and the test now names it — a divergence recorded only in a test name is one nobody finds. Correctly out of scope here: this PR matches the enum-level semantics #313 established, which is the right call.
One thing I'd flag as a latent maintenance cost, not a change request: reflect_test.fds is a committed binary blob. The regeneration command is documented in pool_e2e.rs and you updated it correctly, but nothing in CI regenerates or diff-checks it, so it will rot silently if someone edits a .proto without rerunning it. That's a pre-existing pattern, not something this PR introduced.
Approving — thank you for the thorough context coverage, and for getting the charging granularity right without the owned-side code to copy from.
The oneof preserve-on-decode-error work added its own helper to the head of this file while this branch added packed and map wire builders there. The two sets are independent and neither name collides, so both stay.
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] Re-approving after merging current main ( — the branch had gone stale behind #300/#302/#303). The conflict was in dynamic_e2e.rs, where #303 added its oneof assertion helper to the same region as this branch's packed and map wire builders; both sets are independent and neither name collides, so both are kept. Verified all four suites coexist: this PR's 5 closed-enum tests plus the 12 from #299/#303/#308 all pass (34 total in the file). fmt and clippy -D warnings clean. The original review stands — the allocation guard is charged identically to #313, per value for scalars and repeated, per entry for maps.
|
[claude code] (Correcting the previous comment — a shell quoting slip ate the commit SHA.) Re-approving after merging current main as Verified all four suites coexist rather than one quietly displacing another: this PR's 5 closed-enum tests plus the 12 from #299/#303/#308 all pass, 34 in the file. The original review stands — the allocation guard is charged identically to #313, once per value for scalars and repeated fields, once per entry for maps. |
What this does
DynamicMessage now consults the linked enum descriptor before materializing binary enum values. Unknown values for closed proto2 and editions enums are routed to unknown fields instead of becoming
Value::EnumNumbervalues. Open enums keep their existing behavior.The handling covers singular fields, oneofs, extensions, unpacked repeated fields, packed repeated fields, and map values. Closed-enum map entries follow last-value-wins semantics: an entry whose final enum value is unknown is preserved as one length-delimited unknown field, while a later known value inserts the entry.
The reflection regression coverage also makes the runtime-pool behavior explicit when generated code has a field-level open-enum override but the linked enum remains closed.
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspacecargo check --workspace --all-features