codegen: emit write_to fields in field-number order, not by kind#78
Merged
codegen: emit write_to fields in field-number order, not by kind#78
Conversation
Replace the four per-kind buckets in classify_fields with a single field-number-sorted pass that dispatches to the per-kind stmt builders inline. compute_size and write_to are built in the same loop body, so the SizeCache reserve/consume_next traversal order stays aligned by construction. Oneof groups are positioned at their lowest member field number. Applied to both the owned and view encode paths. Fixes byte-level divergence from prost / protoc-C++ for messages that mix a high-numbered singular field with a lower-numbered repeated/map/oneof. Closes #75
|
All contributors have signed the CLA ✍️ ✅ |
Route the sort key through validated_field_number() so a hand-crafted descriptor with a missing or out-of-range field.number surfaces as a CodeGenError instead of silently sorting to position 0. The oneof_index unwrap is provably safe (guarded by is_real_oneof_member) and now documents that with .expect().
kollektiv
approved these changes
Apr 28, 2026
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Closes #75.
Problem
write_toemitted all singular fields before all repeated fields (and those before oneofs, then maps) — fields were grouped by kind rather than serialized in field-number order. For a message like:buffa emitted tags
1,2,3,4,5,8,6,7; prost / protoc-C++ emit1,2,3,4,5,6,7,8. Decoders accept any order so round-trip tests pass, but anything content-addressing the serialized bytes (e.g.blake3(encode(msg))) diverges.Fix
Replaced the four-bucket
ClassifiedFieldsstruct with aFieldKindenum and a singleclassify_fields_ordered()that returns fields sorted by number (oneof groups positioned at their lowest member).generate_message_implandbuild_view_encode_methodsnow iterate that list once, buildingcompute_size/write_to/merge_field/cleartoken streams in lockstep.The lockstep matters: post-#22,
SizeCacheallocates slots viareserve()/set()duringcompute_sizeand reads them viaconsume_next()duringwrite_toin traversal order. Building both from the same loop body keeps them aligned by construction.Net change in
impl_message.rsis −67 lines (excluding new tests).Tests
buffa-test/protos/edge_cases.proto: newFieldOrdering(interleaved scalar/repeated/map/sub-message/oneof) andFieldOrderingShuffled(same fields, declared in reverse number order).edge_cases.rs: tag-sequence assertions for owned and view encode, owned↔view byte equality, golden bytes for the deterministic subset, plus a round-trip.impl_message.rs: unit tests forclassify_fields_orderedcovering cross-kind ordering and oneof-at-min-number placement.Regenerated
buffa-descriptor/src/generated/reordered (790 lines moved, no logic change).buffa-types/src/generated/and the logging example were unaffected — none of the WKTs interleave field kinds.