Skip to content

buffa-build: path-scoped preserve_unknown_fields_in - #395

Open
Divyansh151005 wants to merge 1 commit into
anthropics:mainfrom
Divyansh151005:build/preserve-unknown-fields-in
Open

buffa-build: path-scoped preserve_unknown_fields_in#395
Divyansh151005 wants to merge 1 commit into
anthropics:mainfrom
Divyansh151005:build/preserve-unknown-fields-in

Conversation

@Divyansh151005

Copy link
Copy Markdown
Contributor

Fixes #281.

What this does

preserve_unknown_fields(bool) is all-or-nothing. Disabling it is a legitimate memory optimization (drops the 24-byte UnknownFields Vec header from every message), but it also silently removes round-trip fidelity on the handful of types that still get re-encoded, forwarded, or persisted.

This adds path-scoped overlays on top of the global default, following the existing _in convention (bytes_type_in, unbox_oneof_in):

buffa_build::Config::new()
    .preserve_unknown_fields(false)
    .preserve_unknown_fields_in(&[".wa.CallLogRecord", ".wa.SyncdMutation"])
  • Matching uses matches_proto_prefix (message FQNs and package prefixes). Last matching rule wins.
  • Granularity is per-message: the flag gates whether the generated struct carries __buffa_unknown_fields.
  • Nested messages resolve independently of their enclosing type.
  • protoc-gen-buffa gets the matching repeatable option unknown_fields_in=<path>.

Public _in is enable-only (same as unbox_oneof_in). Internally the rule list is Vec<(String, bool)> so last-match can mix polarities in tests.

Testing

$ rustc --version
rustc 1.95.0 (59807616e 2026-04-14)

$ cargo fmt --all --check
$ cargo clippy --workspace --all-targets -- -D warnings
$ RUSTDOCFLAGS='-D warnings' cargo doc --workspace --all-features --no-deps
$ cargo test --workspace

All passed. Targeted extras: preserve_unknown_fields_* in buffa-codegen, inline_preserve_unknown_fields_in_* in codegen_integration, preserve_unknown_fields_in_normalizes_leading_dot in buffa-build, and unknown_fields_in_* in protoc-gen-buffa.

Conformance (task conformance) was not run locally; this change does not alter the default-on codec path the suite exercises.

Fixes anthropics#281.

preserve_unknown_fields is currently all-or-nothing. Builds that
disable it globally for the 24-byte Vec header then lose round-trip
fidelity on the handful of types that still get re-encoded.

Path-prefix rules (same matching as unbox_oneof_in) now overlay the
global default; last match wins. Nested messages resolve independently.
@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown

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

@Divyansh151005

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@Divyansh151005

Copy link
Copy Markdown
Contributor Author

recheck

@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code]

The shape is right: one resolver (CodeGenContext::preserve_unknown_fields(msg_fqn)) on the existing matches_proto_prefix matcher, and every site that read ctx.config.preserve_unknown_fields converted — I checked, including custom_view_default_impl, generate_custom_default, and build_lazy_to_owned_fields. Default output is byte-identical, so no regeneration was needed and none was done, correctly. It needs the following before it can go in; the first is the one that matters.

  1. Nothing compiles or runs a mixed build. The tests are substring checks on one struct body. buffa-test/build.rs already has three preserve_unknown_fields(false) fixtures (around lines 429, 648, 756 — the last with lazy_views); add a fourth proto with preserve_unknown_fields(false) plus preserve_unknown_fields_in(&[".x.Keep"]), and a test that decodes bytes carrying an unknown tag into Keep and Drop and asserts re-encode preserves in one and drops in the other. That single fixture also compile-checks the text, reflect, ExtensionSet, view, and lazy-view branches this PR just made per-message.
  2. "Nested messages resolve independently" is wrong in the enabling direction. matches_proto_prefix(".pkg.Outer", ".pkg.Outer.Inner") is true, so a rule naming a message covers its nested types; only child→parent is independent (which is what the unit test proves). Same claim in buffa-build/src/lib.rs, buffa-codegen/src/lib.rs, DESIGN.md, the fragment, and the guide section contradicts itself in one sentence. State the two directions separately.
  3. Rebase onto main and use the shared normalizer. The branch is based on 0358564; main now has normalize_proto_path(path, label) in protoc-gen-buffa/src/main.rs (from protoc-gen-buffa: add unbox_oneof option #392) which also trims trailing dots and re-checks for empty. The hand-rolled version accepts unknown_fields_in=wa. and ... (they silently match nothing) and its error text differs from every other path option. normalize_proto_path(value.trim(), "unknown_fields_in")?.
  4. An empty or whitespace path in the builder becomes the . catch-all ("" does not start with ., so it is rewritten to "."), which globally re-enables preservation — the inverse of what a caller with a bad path wants. override_feature_in deliberately warns and ignores instead (open_enums_in_empty_path_is_not_catchall); do the same. The plugin trims whitespace and the builder does not, so " .wa.Msg" also behaves differently between the two.
  5. No inert-rule warning. A typo'd FQN, a field path, or a rule into an extern_pathed package matches nothing silently, and the silent outcome here is lost round-trip fidelity in production. The unbox_oneof_in warning path in buffa-codegen/src/lib.rs (search "An inert rule means") is the precedent; CodeGenWarning is #[non_exhaustive].
  6. Docs understate what the per-message off-switch removes. It is not just __buffa_unknown_fields: that message also loses its ExtensionSet impl (extension()/set_extension()/has_extension()), ReflectMessage::unknown_fields, textproto extension round-trip, and JSON [ext]-key handling. When the knob is per-message the caller is choosing that bundle per message and should see the list.

Smaller, at your discretion: hoist the resolved value into a local in view.rs/lazy_view.rs (five allocating calls each per message) and pass the existing preserve_unknown_fields: bool parameter through in impl_message.rs:3385 instead of recomputing; the resolver could use the .iter().rev().find(...).map_or(...) form the four sibling resolvers use; fragment should cite (#395, closes #281) and — following the shared_descriptor_pool trio — announce the CodeGenConfig field and the plugin option, since buffa-codegen has an external consumer that builds the config directly; the builder doc implies call order matters ("global off switch first, then this") when it does not, which is worth saying because the neighbouring string_type/string_type_in pair is order-sensitive.

Pre-existing but easier to hit with this knob: message_set_wire_format messages emit self.__buffa_unknown_fields unconditionally, so allow_message_set(true) plus preservation off for that type generates code that does not compile — a CodeGenError for the combination would be kind. Not blocking here.

With 0.9.2 close, this will most likely ride the next patch unless the above lands quickly; the build.rs change will need Iain to approve the CI run.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preserve_unknown_fields_in(".pkg.Outer") also matches .pkg.Outer.Inner because this uses proto-prefix matching, so nested messages are not actually independent as documented. Please distinguish package-prefix matching from exact message-FQN matching and add an outer-enabled/inner-disabled regression test.

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.

buffa-build: path-scoped preserve_unknown_fields_in for per-message unknown-field preservation

3 participants