buffa-codegen: configurable Box wrapping for message-type oneof variants

[christopherwxyz]

Current behavior

In buffa-codegen/src/oneof.rs:

pub(crate) fn is_boxed_variant(ty: Type) -> bool {
    matches!(ty, Type::TYPE_MESSAGE | Type::TYPE_GROUP)
}

Every message-typed oneof variant is unconditionally wrapped in ::buffa::alloc::boxed::Box<T>:

pub enum Body {
    Foo(::buffa::alloc::boxed::Box<FooBody>),
    Bar(::buffa::alloc::boxed::Box<BarBody>),
    // ...
}

The doc comment near is_boxed_variant notes this is "consistent with MessageField<T> being Option<Box<T>> for singular message fields." Sound rationale, but no opt-out exists today.

Pain point

In code that constructs oneof variants frequently — test fixtures, builders, codegen consumers — every site becomes:

parent.body = Some(message_data::Body::Foo(
    ::buffa::alloc::boxed::Box::new(FooBody { ... })
));

The From<T> impl buffa-codegen emits helps a little (Body::from(foo) → Body::Foo(Box::new(foo))), but it's not always applicable — for example when multiple variants share a Rust type (the codegen explicitly skips From for those, per the comment around line 220 of oneof.rs).

For small messages or hot construction paths, the indirection costs more than it saves: an extra allocation per variant and noisier code at every construction site.

Prior art

prost-build defaulted to not boxing and let callers opt in per-field:

prost_build::Config::new()
    .boxed(".my.package.MyMessage.deep_nested_field")
    .compile_protos(&[...], &[...])?;

This put the size/indirection tradeoff in the user's hands.

Proposal

Add a config knob — one of:

Option A: global toggle (default keeps current behavior).

Config::new().box_message_oneof_variants(false)

Option B: prost-style per-path opt-out.

Config::new().unbox(".my.package.MyMessage.body.small_variant")

Option C: a single "box only when X" heuristic — e.g. box only if the variant struct is statically known to be ≥ N bytes, where N is configurable.

(A) is simplest and matches Config::preserve_unknown_fields(false)'s style. (B) is more granular and matches the existing prost migration story. Either feels in keeping with the existing Config builder.

Default should probably stay as today (always-box) to preserve the documented "consistent with MessageField<T>" invariant without breaking existing code.

Why this matters

MessageField<T> is Option<Box<T>> because it might be unset; the Box there is paying for absence. A oneof variant is present when matched — there's no absence to encode. The "consistency" argument is therefore a code-shape consistency, not a semantic necessity. For consumers who'd rather take the size hit on the enum to win construction ergonomics, the opt-out is worth having.

[sam-shridhar1950f]

I'd be glad to put together a PR for this if you're open to it. I've reproduced the behavior and drafted a rough approach below, and there's one design question I'd want your read on first.

From what I can see, is_boxed_variant (oneof.rs:47) boxes message and group variants without consulting any config, so there isn't currently a way to opt a variant out. codegen_integration.rs:542 captures the current behavior (it asserts the Box<Placeholder> output), and a quick codegen run on a minimal oneof { Inner inner = 1; } showed the non-recursive Inner coming out as Inner(Box<...Inner>).

To get a sense of the cost, I measured the boxing in isolation with a counting allocator (10M constructs, ~80% message-variant):

boxed:  8,000,000 allocs   (current)
inline:         0 allocs   (with an opt-out)

That comes out to roughly one heap allocation per non-recursive message variant (~1.6M/sec at 2M msgs/s). It's a microbenchmark rather than a full decode path, so I wouldn't read the wall-clock as end-to-end, though the allocation delta is the part that carries over, and it tends to grow under contention or on a constrained no_std heap.

A rough sketch, following the existing per-path knobs (bytes_fields, string_type_in):

• buffa-build: unbox_oneof_in(&[paths]) (plus unbox_oneof()), defaulting to today's behavior so existing output is unchanged.
• context.rs: an oneof_unboxed(fqn) query, last-rule-wins like the existing ones.
• oneof.rs (~line 149, where ctx and variant_fqn are already in scope): gate the box with is_boxed_variant(ty) && !ctx.oneof_unboxed(&variant_fqn).
• For recursion, opting a recursive variant out would be a compile error, so I'd document that, and could add a codegen-time check that surfaces a clearer message rather than leaving it to rustc.
• Tests following bytes_type.rs; should land well under the 250-line guideline.

The main thing I'd want your read on @iainmcgin: would you prefer the per-path opt-out above (consistent with the existing knobs, default unchanged), or recursion-aware boxing that boxes where a cycle requires it? The opt-out lines up with how MessageField<T> boxes today and is the smaller change; recursion-aware is closer to an ideal default but would be breaking and more involved I'm sure. I lean toward the opt-out, but happy to follow your preference.

If this seems worth doing, I'd be glad to take it on!

[iainmcgin]

The simpler opt-out seems like the right approach to me. In general, if you want to work on an issue, please do! You don't need pre-approval for approaches, I prefer to just review PRs and patch them based on what Claude and I find to get things in quickly.

One heads-up, since it caught me out when I traced it: the boxing decision isn't made in one place. collect_variant_info (oneof.rs:149) sets the enum shape, but the JSON deserialize path (message.rs:1267), text-format path (impl_text.rs:780), and binary decode/merge arms (impl_message.rs:2425-2458) each decide independently — the first two via their own is_boxed_variant calls, the codec by unconditionally emitting Box::new(...) and **existing. Gate only collect_variant_info and you'll get Foo(FooBody) in the enum but Box::new(...) everywhere else, which won't compile. Best to compute "is this variant boxed?" once as a config-aware value and thread it through all of them.

Two smaller things: the From<T> impls (oneof.rs:245-284) filter on is_boxed and hardcode Box::new, so an unboxed variant loses its From impl unless you extend it to emit Self::Variant(v) — worth keeping. And a codegen-time guard rejecting an opt-out on a recursive variant, as you said, since the raw E0072 won't point back at the config.

On the bytes_fields precedent — good model for the plumbing, but a bit more invasive here: &[u8] deref-coerces and smooths over the call sites, whereas T doesn't coerce to Box<T>, so this touches pattern-match shape in the codec rather than just a type substitution.

[sam-shridhar1950f]

Thanks for the heads-up. I'll compute boxedness once and thread it through collect_variant_info, the JSON/text paths, and the binary codec, keep the From impls for unboxed variants, and add the recursion guard. Posting a PR soon.