Skip to content

codegen: text impls for custom maps, repeated collections and oneof pointers - #802

Open
v0ropaev wants to merge 1 commit into
anthropics:mainfrom
v0ropaev:fix/text-custom-containers
Open

v0ropaev wants to merge 1 commit into
anthropics:mainfrom
v0ropaev:fix/text-custom-containers

Conversation

@v0ropaev

Copy link
Copy Markdown
Contributor

generate_text(true) did not compile together with map_type_custom, repeated_type_custom, or a box_type_custom pointer on a message member of a oneof. The text encoder looped over &self.field, which a newtype map or list doesn't implement IntoIterator for. The map decoder called an inherent insert, the repeated decoder passed the field to read_repeated_scalar_into and read_repeated_into, which take &mut Vec<T>, and the oneof decoder wrapped a new member in Box::new whatever pointer was configured. buffa-test worked around the last one by turning text off for the tc5 fixture, and the other custom fixtures never had text on.

To reproduce, build a small crate per knob with generate_text(true). On main the custom map gives E0277 (&CustomMap<..> is not an iterator) and E0599 (no method named insert), the custom list gives 27 errors, E0277 at the encoder loops and E0308 where the Vec readers get the field, and the custom pointer gives one E0308 at the Box::new in the oneof merge arm.

The text impl now uses what the binary codec already uses for these fields. A map other than the default HashMap goes through MapStorage::storage_iter and storage_insert. A custom collection is written through its Deref slice with the same repeated_for_iter helper the binary encoder uses, and read into a temporary Vec whose elements are then appended with ProtoList::push, so the TextDecoder API stays as it is. A custom oneof pointer is built with ProtoBox::new through PointerRepr::pointer_new. Output for the default HashMap, Vec and Box doesn't change, and regenerating the WKT, option and bootstrap types gives no diff.

buffa-test turns text on for the box_type, map_type_custom, repeated_type and tc5 fixtures, so building the crate now compiles those text paths, and adds text round-trip and merge tests for each. The merge tests cover appending to a custom list in both repeated forms, last-write-wins on a custom map, and merging into a oneof member behind the custom pointer. buffa-codegen gets two tests that pin the emitted calls for custom and default containers. The MapStorage, ProtoList and ProtoBox docs now say text needs nothing beyond the trait.

With only the test changes on top of main, buffa-test fails to build (26 errors from the generated box_type, map_type_custom, repeated_type and tc5 code) and custom_containers_go_through_their_traits fails in buffa-codegen.

fmt, clippy with -D warnings, the buffa-codegen and buffa-test suites and cargo doc with -D warnings are clean on 1.95.0.

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

…ointers

generate_text(true) did not compile together with map_type_custom,
repeated_type_custom or a custom pointer on a message member of a oneof.
The text encoder iterated `&self.field`, the map decoder called an
inherent `insert`, the repeated decoder passed the field to a reader that
takes `&mut Vec<T>`, and the oneof decoder always wrapped a new member in
`Box::new`.

The text impl now uses the same trait surface as the binary codec for
these fields: MapStorage::storage_iter and storage_insert for any map
other than the default HashMap, the Deref slice and ProtoList::push for a
custom collection (read into a temporary Vec first, so the decoder API is
unchanged), and ProtoBox::new for a custom oneof pointer. Output for the
default HashMap, Vec and Box is byte-identical, so no checked-in code
changes.

buffa-test turns text on for the box_type, map_type_custom,
repeated_type and tc5 fixtures, dropping the workaround that kept it off
for tc5, and adds text round-trip and merge tests for each.

This branch has not been deployed

No deployments
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.

1 participant