diff --git a/.changes/unreleased/added-20260923-codec-strategy-table.yaml b/.changes/unreleased/added-20260923-codec-strategy-table.yaml index 8f240a69..757b250f 100644 --- a/.changes/unreleased/added-20260923-codec-strategy-table.yaml +++ b/.changes/unreleased/added-20260923-codec-strategy-table.yaml @@ -1,4 +1,4 @@ kind: Added body: |- - **Table-driven message codec** (#469, refs #463). `buffa_build::Config::codec_strategy(CodecStrategy::Table)` (plugin option `codec_strategy=table`, `CodeGenConfig::codec_strategy`) generates each message's binary `Message` implementation from a static table and interpreters that every message shares, instead of code specialised to the message's fields. It roughly halves the compiled size of a large schema at `opt-level = "z"`, and messages made of many small fields are slower. `CodecStrategy::Unrolled` stays the default. `codec_strategy_in(strategy, &[paths])` (plugin option `codec_strategy_in==`, repeatable) chooses the strategy for matching messages and the messages nested in them, and the last matching rule wins. The wire format does not change. A message that has a `oneof`, `map`, or group field, or holds a message that is not a table, stays unrolled, and `CodeGenWarning::TableCodecFallbackSummary` counts those; a rule that names such a message by its exact path is an error. The generated code needs Rust 1.77 or later, which `buffa-build` checks, and compiles in a crate with `#![forbid(unsafe_code)]`; see the guide's "Smaller generated code" section for the rest. + **Table-driven message codec** (#469, refs #463). `buffa_build::Config::codec_strategy(CodecStrategy::Table)` (plugin option `codec_strategy=table`, `CodeGenConfig::codec_strategy`) generates each message's binary `Message` implementation from a static table and interpreters that every message shares, instead of code specialised to the message's fields. The compiled code is substantially smaller, and messages made of many small fields are slower. `CodecStrategy::Unrolled` stays the default. `codec_strategy_in(strategy, &[paths])` (plugin option `codec_strategy_in==`, repeatable) chooses the strategy for matching messages and the messages nested in them, and the last matching rule wins. The wire format does not change. Messages the table cannot handle stay unrolled, and `CodeGenWarning::TableCodecFallbackSummary` counts them. A rule that selects the table, by exact path, for a message that cannot use it is an error. The generated code needs Rust 1.77 or later, which `buffa-build` checks, and compiles in a crate with `#![forbid(unsafe_code)]`. The guide's "Smaller generated code" section has the measurements, the messages that stay unrolled and the behavioural differences. time: 2026-09-23T03:10:00+00:00 diff --git a/DESIGN.md b/DESIGN.md index ee41af9f..dd8e4f92 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -558,7 +558,7 @@ Within that bound, the shortcuts do not pay off. `merge` does not help: it consu ### 13. Table-Driven Codec (`CodecStrategy::Table`) -The size, write, and merge code of an unrolled message is specialised to its fields. `CodecStrategy::Table` (a per-message option, `Unrolled` by default) replaces it with a static `buffa::table::Table` and a `Message` impl that forwards to interpreters in `buffa::table`. The measurements and the decision to keep `Unrolled` as the default are in [#463](https://github.com/anthropics/buffa/issues/463). +The size, write, and merge code of an unrolled message is specialised to its fields. `CodecStrategy::Table` (a per-message option, `Unrolled` by default) replaces it with a static `buffa::table::Table` and a `Message` impl that forwards to interpreters in `buffa::table`. The decision to keep `Unrolled` as the default is in [#463](https://github.com/anthropics/buffa/issues/463), and the guide's "Smaller generated code" section has the measurements and the list of messages that stay unrolled. A table holds a sorted array of 12-byte entries `{tag, offset, kind, tag_len, aux}`, a dense array that maps field numbers below 64 to entries, and the offset of the unknown-fields slot. `kind` is the field type crossed with its cardinality, so the interpreter dispatches once per field. Message, repeated-message, and enum fields carry a small descriptor (`Aux`) with the accessors that their storage needs, because a `MessageField`, a `Vec`, and an `EnumValue` cannot be read through an offset alone. Offsets come from `core::mem::offset_of!`, so the table needs Rust 1.77 and the generated code refers to it through `buffa::__table!`, which is a compile error on an older compiler. @@ -566,7 +566,7 @@ Three decisions shape the runtime: - **The `unsafe` lives in `buffa`.** `__table!` and `__table_entry!` contain the `unsafe` blocks and witness each field's type against its kind, so a table that names the wrong kind for a field does not compile, and generated code compiles under `#![forbid(unsafe_code)]`. `Table::new` also checks the layout constants at compile time. The interpreters run under Miri in CI. - **The interpreters are not generic over the sink or the input where that is avoidable.** `Message::encode` and its siblings write any `BufMut` through one shared, non-generic cursor (`buffa/src/encode_sink.rs`), and decoding runs over a contiguous `&[u8]`, so the interpreters are compiled once in `buffa`, at its `opt-level`, and not once per caller. -- **A table refers to the tables of its children,** so a message can use the table only if every message it holds does. The planner in `buffa-codegen` (`table_plan.rs`) starts from the messages that asked for the table, removes those the interpreters cannot handle (oneofs, maps, groups and the types of group fields, `MessageSet`, extension ranges with JSON, custom string, bytes, or collection types), and then removes every message that holds a removed one, until none is left. A message that holds a type from another crate, such as a well-known type, is removed the same way, because the static table of that type is not visible. +- **A child reaches the interpreters through its table or its `Message` impl.** A message field's `Aux` descriptor holds either the child's `MessageTable` or a `DynVt` of function pointers instantiated for the child type (`table/bridge.rs`), so a table message can hold any message it is not required to leave unrolled. The write pointer takes the `PreSized` cursor because a function pointer cannot be generic over the sink. A pointer per child type and sink type would avoid the copy that other sinks cost, at the price of code in every crate that uses the table. The planner in `buffa-codegen` (`table_plan.rs`) selects the messages that asked for the table and drops those the interpreters cannot handle. Among them it drops, transitively, the holders of a message of the run with a non-default bytes type, because the table decodes from one contiguous slice and would copy the `bytes::Bytes` fields that unrolled code shares with a `Bytes` input. A child from another crate is not inspected, so a holder of `google.protobuf.Any`, whose `value` is `bytes::Bytes`, keeps its table and copies the payload. ### Owned decode: intentional throughput trade-offs diff --git a/buffa-build/src/lib.rs b/buffa-build/src/lib.rs index 78c64771..fc37e132 100644 --- a/buffa-build/src/lib.rs +++ b/buffa-build/src/lib.rs @@ -1554,14 +1554,13 @@ impl Config { /// Choose how the binary `Message` implementation of every message is /// generated (default: [`CodecStrategy::Unrolled`]). /// - /// On a schema it fully covers, [`CodecStrategy::Table`] makes the - /// compiled size about half as big at `opt-level = "z"`, and it slows - /// messages made of many small fields; a message that cannot use it keeps - /// its size. [`CodecStrategy::Table`] has the measurements, says which - /// messages stay unrolled, and lists how a table message behaves - /// differently. This build reports - /// those in one `cargo:warning`. The option never changes the wire - /// format. The generated code needs Rust 1.77 or later, and `compile` + /// On a large schema, [`CodecStrategy::Table`] makes the compiled code + /// about 40% smaller at `opt-level = "z"`, and it slows messages made of + /// many small fields; a message that cannot use it keeps its size. + /// [`CodecStrategy::Table`] says which messages stay unrolled and how a + /// table message behaves differently, and this build reports the messages + /// that stay unrolled in one `cargo:warning`. The option never changes the + /// wire format. The generated code needs Rust 1.77 or later, and `compile` /// returns an error on an older compiler when a build script runs it (the /// compiler is read from `RUSTC`). /// @@ -1601,13 +1600,8 @@ impl Config { /// names the message by its exact path. A rule that matches no message /// produces a warning. /// - /// A table message holds only table messages, and a rule does not extend - /// to the messages a message holds. Selecting a message with a rule - /// therefore also needs rules for everything it holds, unless the global - /// setting is [`CodecStrategy::Table`]. Choosing [`CodecStrategy::Unrolled`] - /// for a message keeps every message that holds it unrolled, with no - /// warning, and that usually includes the root message an application - /// encodes. + /// A rule does not extend to the messages a message holds; see + /// [`CodecStrategy::Table`]. #[must_use] pub fn codec_strategy_in(mut self, strategy: CodecStrategy, paths: &[impl AsRef]) -> Self { for raw in paths.iter().map(AsRef::as_ref) { diff --git a/buffa-codegen/src/lib.rs b/buffa-codegen/src/lib.rs index 0caa2227..7f91174e 100644 --- a/buffa-codegen/src/lib.rs +++ b/buffa-codegen/src/lib.rs @@ -1086,11 +1086,11 @@ pub enum CodecStrategy { #[default] Unrolled, /// Emit one static table per message and forward `Message` to interpreters - /// shared by every message, in the `buffa` crate. On a schema the table - /// fully covers, it roughly halves the compiled size at - /// `opt-level = "z"`; a message that cannot use it keeps its size. It costs - /// run time on messages made of many small fields. The measurements are in - /// anthropics/buffa#463. + /// shared by every message, in the `buffa` crate. On a large schema it + /// makes the compiled code about 40% smaller at `opt-level = "z"`; a + /// message that cannot use it keeps its size. It costs run time on messages + /// made of many small fields. The guide's "Smaller generated code" section + /// has the measurements. /// /// Not every message can use it. These stay [`Unrolled`](Self::Unrolled): /// @@ -1102,19 +1102,40 @@ pub enum CodecStrategy { /// - a message with a field of a non-default string, bytes, or collection /// type, such as `use_bytes_type`, `string_type`, `bytes_type`, and /// `repeated_type` select; - /// - a message with a field whose message type stays `Unrolled` for any - /// reason, is not selected for the table, or is generated by another - /// crate, such as a well-known type. - /// - /// A table message therefore holds only table messages, and selecting a - /// message does not select the messages it holds. - /// - /// A table message differs from an unrolled one in three ways. A field that + /// - a message that holds, in a singular, repeated, `oneof`, or map value + /// field, a message of the same run that has a `bytes` field of a + /// non-default type, or that holds one. The table decodes from one + /// contiguous slice, so it would copy `bytes::Bytes` fields that + /// unrolled code decoding from a `Bytes` shares with the input. + /// + /// A table message may hold any other message. It reaches a child that is a + /// table message through the child's table, and any other child, whether + /// it is [`Unrolled`](Self::Unrolled), generated by another crate, or a + /// well-known type, through the child's `Message` impl, which costs a + /// function call per child. Selecting a message does not select the + /// messages it holds, and keeping a child [`Unrolled`](Self::Unrolled) does + /// not keep the messages that hold it unrolled: to keep a whole path + /// specialised, set its holders to `Unrolled` too. + /// + /// A table message differs from an unrolled one in these ways. A field that /// declares a length past the end of its enclosing message fails at once /// with `DecodeError::UnexpectedEof`, where unrolled code can report a - /// different error for the same rejected input. `clear()` resets the message - /// to its default, which releases the capacity of its strings and vectors. + /// different error for the same rejected input; a child reached through its + /// `Message` impl is read from the slice of the nearest enclosing table + /// message, so it is bounded there. `clear()` resets the message to its + /// default, which releases the capacity of its strings and vectors. /// Decoding gathers a `Buf` that is not contiguous into one buffer first. + /// Encoding into any sink other than the cursor that `Message::encode` and + /// its siblings write a `BufMut` through, meaning a `Rope`, a sink defined + /// outside `buffa`, or a `BufMut` passed straight to `Message::write_to`, + /// stages each child reached through its `Message` impl in a scratch + /// buffer, and a `Rope` copies it again and cannot share the `bytes` + /// fields inside it by reference count. Codegen cannot see the fields of a + /// message from another crate, so a table message that holds one with a + /// `bytes` field of a non-default type copies it on decode. The well-known + /// type `google.protobuf.Any` is one, because its `value` is + /// `bytes::Bytes`. To keep the payload shared with a `Bytes` input, set the + /// holder to [`Unrolled`](Self::Unrolled) with `codec_strategy_in`. /// /// A message that another crate or codegen run uses as the type of a group /// or editions `DELIMITED` field must not be selected, because @@ -1929,12 +1950,8 @@ pub struct CodeGenConfig { /// asked for something impossible. A rule that matches no generated message /// produces a [`CodeGenWarning::CodecStrategyRuleMatchedNothing`]. /// - /// A table message holds only table messages, and a rule does not extend to - /// the messages a message holds. A [`CodecStrategy::Table`] rule for a - /// message therefore needs rules for the messages it holds, or a global - /// [`CodecStrategy::Table`]; an exact-path rule fails without them. - /// Setting a message to [`CodecStrategy::Unrolled`] keeps every message that - /// holds it unrolled too, without a warning. + /// A rule does not extend to the messages a message holds; see + /// [`CodecStrategy::Table`]. pub codec_strategy_in: Vec<(String, CodecStrategy)>, } @@ -2390,15 +2407,12 @@ pub enum CodeGenWarning { /// [`codec_strategy`](CodeGenConfig::codec_strategy) or by a /// [`codec_strategy_in`](CodeGenConfig::codec_strategy_in) rule, cannot use /// the table and are generated [`CodecStrategy::Unrolled`]. One warning - /// covers the whole run. A message that falls back only because a message - /// it holds is set to `Unrolled` is not counted, because the setting is - /// the reason. + /// covers the whole run. #[non_exhaustive] TableCodecFallbackSummary { /// The number of messages that fell back. fallbacks: usize, - /// The number of messages selected for the table, not counting those - /// kept unrolled only by a message the user set to `Unrolled`. + /// The number of messages selected for the table. selected: usize, /// Why they fell back, most common reason first. reasons: Vec, @@ -2562,7 +2576,7 @@ impl core::fmt::Display for CodeGenWarning { write!( f, "{fallbacks} of {selected} messages selected for the table codec use the \ - unrolled codec instead, which works but is larger" + unrolled codec instead" )?; for (i, r) in reasons.iter().enumerate() { let sep = if i == 0 { ": " } else { "; " }; diff --git a/buffa-codegen/src/table_codec.rs b/buffa-codegen/src/table_codec.rs index dd45c29d..5fa8cc5d 100644 --- a/buffa-codegen/src/table_codec.rs +++ b/buffa-codegen/src/table_codec.rs @@ -196,11 +196,14 @@ fn field_entry( )) }) }; - let type_path = |what: &str| -> Result { - let type_name = field + let type_name = || { + field .type_name .as_deref() - .ok_or(CodeGenError::MissingField("field.type_name"))?; + .ok_or(CodeGenError::MissingField("field.type_name")) + }; + let type_path = |what: &str| -> Result { + let type_name = type_name()?; ctx.rust_type_relative(type_name, current_package, nesting) .ok_or_else(|| CodeGenError::Other(format!("{what} type '{type_name}' not found"))) }; @@ -208,10 +211,7 @@ fn field_entry( // The table is not among the imports that `idiomatic_imports` shortens // paths with, so its path is built from the unshortened one. let unshortened_path = || -> Result { - let type_name = field - .type_name - .as_deref() - .ok_or(CodeGenError::MissingField("field.type_name"))?; + let type_name = type_name()?; let split = ctx .rust_type_relative_split(type_name, current_package, nesting) .ok_or_else(|| CodeGenError::Other(format!("message type '{type_name}' not found")))?; @@ -225,23 +225,30 @@ fn field_entry( match f.ty { Type::TYPE_MESSAGE => { let child = type_path("message")?; - let child_table = table_path(&unshortened_path()?)?; let child_ty = rust_path_to_tokens(&child); + // A child without a table here is reached through its `Message` + // impl. + let child_table = if ctx.uses_table_codec(type_name()?) { + Some(table_path(&unshortened_path()?)?) + } else { + None + }; let (slot, aux_item) = if f.card == Card::Repeated { + let vt = match &child_table { + Some(table) => quote! { ::buffa::table::RepVt::new::<#child_ty>(&#table) }, + None => quote! { ::buffa::table::RepVt::new_via_message::<#child_ty>() }, + }; ( quote! { ::buffa::alloc::vec::Vec<#child_ty> }, - quote! { - ::buffa::table::Aux::Rep(&::buffa::table::RepVt::new::<#child_ty>(&#child_table)) - }, + quote! { ::buffa::table::Aux::Rep(&#vt) }, ) } else { let slot = classify_field(scope, msg, field, resolver)?.rust_type; - ( - slot.clone(), - quote! { - ::buffa::table::Aux::Msg(&::buffa::table::MsgVt::new::<#slot>(&#child_table)) - }, - ) + let vt = match &child_table { + Some(table) => quote! { ::buffa::table::MsgVt::new::<#slot>(&#table) }, + None => quote! { ::buffa::table::MsgVt::new_via_message::<#slot>() }, + }; + (slot, quote! { ::buffa::table::Aux::Msg(&#vt) }) }; let aux = aux_u16()?; Ok(( diff --git a/buffa-codegen/src/table_plan.rs b/buffa-codegen/src/table_plan.rs index 451c1b8d..3bfe446e 100644 --- a/buffa-codegen/src/table_plan.rs +++ b/buffa-codegen/src/table_plan.rs @@ -2,11 +2,12 @@ //! table field looks like. //! //! A message can use the table if the interpreters in `buffa::table` cover -//! every field and every message it holds is a table message too, because a -//! table records the tables of its children. The set of table messages is the -//! largest set of requested, locally eligible messages closed under that -//! rule, found by removing messages until none is left holding a non-table -//! child. +//! every field it has, with one exception that depends on the messages it +//! holds: a message with a bytes field of a non-default type would lose its +//! zero-copy decode inside a table message, so the messages that hold one, +//! directly or through other messages, stay unrolled. Any other child is +//! reached through its table or its `Message` impl, so it need not be a table +//! message itself. use std::collections::{HashMap, HashSet}; @@ -15,10 +16,10 @@ use crate::features::ResolvedFeatures; use crate::generated::descriptor::field_descriptor_proto::{Label, Type}; use crate::generated::descriptor::{DescriptorProto, FieldDescriptorProto, FileDescriptorProto}; use crate::impl_message::{ - effective_type, is_explicit_presence_scalar, is_field_packed, is_real_oneof_member, - is_required_field, + effective_type, field_bytes_repr, is_explicit_presence_scalar, is_field_packed, + is_real_oneof_member, is_required_field, map_value_bytes_repr, }; -use crate::message::{find_map_entry, is_closed_enum}; +use crate::message::{find_map_entry, is_closed_enum, map_entry_key_type, map_entry_value_type}; use crate::{CodeGenError, CodeGenWarning, CodecStrategy, TableCodecFallbackReason}; /// The cardinality half of a field's `buffa::table::Kind`. @@ -86,21 +87,10 @@ fn type_stem(ty: Type, card: Card) -> Option<&'static str> { #[derive(Clone, Debug)] pub(crate) struct Ineligible { /// The reason in a few words, which the summary warning groups messages - /// by. For a message that holds another that cannot use the table, this - /// includes the other's reason. + /// by. pub(crate) reason: String, /// The reason for this message, naming the field or type. pub(crate) detail: String, - /// The message's own reason if it is the cause of a fallback, and - /// otherwise the cause of the message it holds, which is followed down to - /// the message that cannot use the table itself. - pub(crate) root: String, - /// Whether the fallback follows from a strategy the user chose (a message - /// it holds is set to `Unrolled`), so that it needs no warning. - pub(crate) silent: bool, - /// What to do about it when a rule that names the message exactly asked - /// for the table, if it differs from the general advice. - pub(crate) hint: Option, } /// An [`Ineligible`] whose reason needs no more detail. @@ -109,13 +99,9 @@ fn same(reason: &str) -> Ineligible { } fn ineligible(reason: impl Into, detail: impl Into) -> Ineligible { - let reason = reason.into(); Ineligible { - root: reason.clone(), - reason, + reason: reason.into(), detail: detail.into(), - silent: false, - hint: None, } } @@ -219,9 +205,67 @@ pub(crate) fn table_fields<'a>( /// One message of the run and what the plan needs to know about it. struct Candidate<'a> { fqn: String, - /// The proto paths of the message types of its fields. - children: Vec, fields: Result>, Ineligible>, + /// Whether a `bytes` field or map value of the message has a non-default + /// type. + own_bytes: bool, + /// The proto paths of the message types of its fields: singular, + /// repeated, in a oneof, as a map value, or a group. + holds: Vec, +} + +/// Whether `msg` has a `bytes` field, or a map with `bytes` values, stored +/// as `bytes::Bytes` or a custom type. +fn has_non_default_bytes( + ctx: &CodeGenContext, + msg: &DescriptorProto, + fqn: &str, + features: &ResolvedFeatures, +) -> bool { + let proto_fqn = fqn.trim_start_matches('.'); + msg.field.iter().any(|f| { + let name = f.name.as_deref().unwrap_or(""); + let repr = match find_map_entry(msg, f) { + Some(entry) => map_value_bytes_repr( + ctx, + map_entry_key_type(ctx, entry, features), + map_entry_value_type(ctx, entry, features), + proto_fqn, + name, + ), + None if effective_type(ctx, f, features) == Type::TYPE_BYTES => { + field_bytes_repr(ctx, proto_fqn, name) + } + None => return false, + }; + !repr.is_default() + }) +} + +/// The proto paths of the message types `msg` has fields of. +fn held_messages( + ctx: &CodeGenContext, + msg: &DescriptorProto, + features: &ResolvedFeatures, +) -> Vec { + msg.field + .iter() + .filter_map(|f| { + if let Some(entry) = find_map_entry(msg, f) { + if map_entry_value_type(ctx, entry, features) != Some(Type::TYPE_MESSAGE) { + return None; + } + let value = entry.field.iter().find(|v| v.number == Some(2))?; + return value.type_name.clone(); + } + matches!( + effective_type(ctx, f, features), + Type::TYPE_MESSAGE | Type::TYPE_GROUP + ) + .then(|| f.type_name.clone()) + .flatten() + }) + .collect() } /// Every message of `messages` and the messages nested in them that has a @@ -256,18 +300,16 @@ fn collect<'a>( .rust_type_relative(&fqn, package, 0) .is_some_and(|path| path.starts_with("::") || path.starts_with("crate::")); if !is_map_entry && !is_extern { - let mut children = Vec::new(); for f in &msg.field { - match effective_type(ctx, f, &features) { - Type::TYPE_MESSAGE => children.extend(f.type_name.clone()), - Type::TYPE_GROUP => group_types.extend(f.type_name.clone()), - _ => {} + if effective_type(ctx, f, &features) == Type::TYPE_GROUP { + group_types.extend(f.type_name.clone()); } } out.push(Candidate { fields: table_fields(ctx, msg, &fqn, &features), + own_bytes: has_non_default_bytes(ctx, msg, &fqn, &features), + holds: held_messages(ctx, msg, &features), fqn: fqn.clone(), - children, }); } collect( @@ -282,6 +324,38 @@ fn collect<'a>( } } +/// The messages that hold, directly or through other messages, one that this +/// run generates with a `bytes` field of a non-default type, each with the +/// message it holds that leads there. +/// +/// The table decodes over one contiguous `&[u8]`, where `Buf::copy_to_bytes` +/// copies, so a `Bytes` field of such a child would stop being decoded +/// without a copy. A child of another crate is not a candidate and is not +/// inspected. +fn holders_of_non_default_bytes<'a>(candidates: &'a [Candidate<'_>]) -> HashMap<&'a str, &'a str> { + let mut tainted: HashSet<&str> = candidates + .iter() + .filter(|c| c.own_bytes) + .map(|c| c.fqn.as_str()) + .collect(); + let mut holders = HashMap::new(); + loop { + let before = tainted.len(); + for c in candidates { + if tainted.contains(c.fqn.as_str()) { + continue; + } + if let Some(child) = c.holds.iter().find(|h| tainted.contains(h.as_str())) { + tainted.insert(&c.fqn); + holders.insert(c.fqn.as_str(), child.as_str()); + } + } + if tainted.len() == before { + return holders; + } + } +} + /// The messages generated with the table codec in one run. #[derive(Default)] pub(crate) struct TablePlan { @@ -295,64 +369,10 @@ impl TablePlan { } } -/// Why a message that holds `child` cannot use the table, when `child` has no -/// table. -fn child_without_table( - ctx: &CodeGenContext, - child: &str, - reasons: &HashMap<&str, Ineligible>, - generated: &HashSet<&str>, -) -> Ineligible { - if let Some(held) = reasons.get(child) { - return Ineligible { - reason: format!("holds a message that {}", held.root), - detail: format!( - "it has a field of message type `{child}`, which {}", - held.root - ), - root: held.root.clone(), - silent: held.silent, - hint: held.hint.clone(), - }; - } - if !generated.contains(child) { - return ineligible( - "holds a message that another crate or run generates", - format!("it has a field of message type `{child}`, which is not generated here"), - ); - } - // Not selected: the strategy for it is unrolled, by the user's rule or by - // the global default. - if ctx.codec_strategy_rule(child).is_some() { - Ineligible { - silent: true, - ..ineligible( - "holds a message set to the unrolled codec", - format!( - "it has a field of message type `{child}`, which is set to the unrolled codec" - ), - ) - } - } else { - Ineligible { - hint: Some(format!( - "Select `{child}` as well, and every message it holds (buffa-build: \ - `.codec_strategy_in(CodecStrategy::Table, &[\"{child}\"])`; plugin: \ - `codec_strategy_in={child}=table`)" - )), - ..ineligible( - "holds a message not selected for the table", - format!("it has a field of message type `{child}`, which is not selected for the table codec"), - ) - } - } -} - /// Decide which messages of `files_to_generate` use the table codec. /// /// Returns the plan and a summary warning about the messages that asked for -/// the table and cannot have it, unless the user's own choice of `Unrolled` -/// for a message they hold is the only reason. +/// the table and cannot have it. /// /// # Errors /// @@ -386,118 +406,53 @@ pub(crate) fn plan( &mut group_types, ); } - let generated: HashSet<&str> = candidates.iter().map(|c| c.fqn.as_str()).collect(); - - // The messages that asked for the table, each with the reason it cannot - // have it, if there is one. - let mut reasons: HashMap<&str, Ineligible> = HashMap::new(); - let mut selected: Vec<&Candidate> = Vec::new(); - for c in &candidates { - if ctx.codec_strategy(&c.fqn) != CodecStrategy::Table { - continue; - } - selected.push(c); - if let Err(why) = &c.fields { - reasons.insert(&c.fqn, why.clone()); - } else if group_types.contains(&c.fqn) { - reasons.insert(&c.fqn, same("is the type of a group field")); - } - } - - // Remove every message that holds a child without a table, until none is - // left. - let mut remaining: HashSet<&str> = selected - .iter() - .filter(|c| !reasons.contains_key(c.fqn.as_str())) - .map(|c| c.fqn.as_str()) - .collect(); - loop { - let mut removed = Vec::new(); - for c in selected - .iter() - .filter(|c| remaining.contains(c.fqn.as_str())) - { - // The reason for the first child without a table that the user did - // not choose, if there is one, and otherwise for the first without - // one at all. - let whys: Vec = c - .children - .iter() - .filter(|ch| !remaining.contains(ch.as_str())) - .map(|child| child_without_table(ctx, child, &reasons, &generated)) - .collect(); - if let Some(why) = whys.iter().find(|w| !w.silent).or(whys.first()) { - removed.push((c.fqn.as_str(), why.clone())); - } - } - if removed.is_empty() { - break; - } - for (fqn, why) in removed { - remaining.remove(fqn); - reasons.insert(fqn, why); - } - } - // A holder removed early may have looked like it fell back only because of - // a message the user set to `Unrolled`, before a message it also holds was - // itself removed for a reason of its own. Look again with the final - // reasons, until no silent holder changes. - loop { - let mut changed = Vec::new(); - for c in selected.iter().filter(|c| { - reasons - .get(c.fqn.as_str()) - .is_some_and(|why| why.silent && !c.children.is_empty()) - }) { - let loud = c - .children - .iter() - .filter(|ch| !remaining.contains(ch.as_str())) - .map(|child| child_without_table(ctx, child, &reasons, &generated)) - .find(|why| !why.silent); - if let Some(why) = loud { - changed.push((c.fqn.as_str(), why)); - } - } - if changed.is_empty() { - break; - } - reasons.extend(changed); - } + let bytes_holders = holders_of_non_default_bytes(&candidates); - // A rule that names a message exactly and cannot be honoured is an error, - // and all of them are reported together. The rest are counted by reason in - // one warning, except for the messages whose fallback the user chose. + // For each message that asked for the table, the table if it can have + // one, and otherwise the reason it cannot, which an exact rule turns into + // an error and the summary counts. + let mut tables = HashSet::new(); + let mut selected = 0; let mut errors = Vec::new(); let mut summary: Vec = Vec::new(); let mut fallbacks = 0; - let mut held_back = 0; - for c in &selected { - let Some(why) = reasons.get(c.fqn.as_str()) else { + for c in &candidates { + if ctx.codec_strategy(&c.fqn) != CodecStrategy::Table { continue; + } + selected += 1; + let why = match &c.fields { + Err(why) => why.clone(), + Ok(_) if group_types.contains(&c.fqn) => same("is the type of a group field"), + Ok(_) if bytes_holders.contains_key(c.fqn.as_str()) => ineligible( + "holds a message with bytes fields of a non-default type", + format!( + "it holds `{}`, which has bytes fields of a non-default type or holds a \ + message that has", + bytes_holders[c.fqn.as_str()] + ), + ), + Ok(_) => { + tables.insert(c.fqn.clone()); + continue; + } }; + // A rule that names a message exactly and cannot be honoured is an + // error, and all of them are reported together. The rest are counted + // by reason in one warning. if let Some((rule, _)) = ctx.codec_strategy_rule(&c.fqn) { if *rule == c.fqn { - let advice = why.hint.clone().unwrap_or_else(|| { - format!( - "Select the unrolled codec for it instead (buffa-build: \ - `.codec_strategy_in(CodecStrategy::Unrolled, &[\"{rule}\"])`; plugin: \ - `codec_strategy_in={rule}=unrolled`), or remove the rule" - ) - }); errors.push(format!( "codec_strategy_in rule '{rule}' selects the table codec for a message that \ - cannot use it: {}. {advice}", + cannot use it: {}. Select the unrolled codec for it instead (buffa-build: \ + `.codec_strategy_in(CodecStrategy::Unrolled, &[\"{rule}\"])`; plugin: \ + `codec_strategy_in={rule}=unrolled`), or remove the rule", why.detail )); continue; } } - if why.silent { - held_back += 1; - continue; - } fallbacks += 1; let entry = match summary.iter().position(|r| r.reason == why.reason) { Some(index) => &mut summary[index], @@ -519,11 +474,9 @@ pub(crate) fn plan( summary.sort_by_key(|reason| std::cmp::Reverse(reason.messages.len())); warnings.push(CodeGenWarning::TableCodecFallbackSummary { fallbacks, - selected: selected.len() - held_back, + selected, reasons: summary, }); } - - let tables = remaining.into_iter().map(str::to_string).collect(); Ok((TablePlan { tables }, warnings)) } diff --git a/buffa-codegen/src/tests/table_codec.rs b/buffa-codegen/src/tests/table_codec.rs index 2366044e..f48633a6 100644 --- a/buffa-codegen/src/tests/table_codec.rs +++ b/buffa-codegen/src/tests/table_codec.rs @@ -26,7 +26,8 @@ fn scalar(name: &str, number: i32, ty: Type) -> FieldDescriptorProto { /// Package `t` with: /// /// - `Plain`, `Leaf`, and `HasLeaf` (holds a `Leaf`), which can use the table; -/// - `Oneofy` (has a oneof) and `HasOneofy` (holds an `Oneofy`), which cannot; +/// - `Oneofy` (has a oneof), which cannot, and `HasOneofy` (holds an `Oneofy`), +/// which can, because a table message may hold a message that has no table; /// - `Outer` with a nested `Inner`, both plain. fn schema() -> FileDescriptorProto { let mut oneofy = message( @@ -144,22 +145,15 @@ fn the_global_setting_gives_a_table_to_every_message_that_can_use_one() { let (code, warnings) = run(&table_config(CodecStrategy::Table)).unwrap(); assert_eq!( tables(&code), - ["Plain", "Leaf", "HasLeaf", "Outer", "Inner"] + ["Plain", "Leaf", "HasLeaf", "HasOneofy", "Outer", "Inner"] ); - // The other two fall back, and one warning covers the run. + // The one with a oneof falls back, and one warning covers the run. let (counts, reasons) = summary(&warnings); - assert_eq!(counts, (2, 7)); - assert_eq!( - reasons, - [("has a oneof", 1), ("holds a message that has a oneof", 1),] - ); + assert_eq!(counts, (1, 7)); + assert_eq!(reasons, [("has a oneof", 1)]); let text = table_warnings(&warnings)[0].to_string(); - assert!(text.starts_with("2 of 7 messages selected for the table codec")); + assert!(text.starts_with("1 of 7 messages selected for the table codec")); assert!(text.contains("has a oneof (1: .t.Oneofy)"), "{text}"); - assert!( - text.contains("holds a message that has a oneof (1: .t.HasOneofy)"), - "{text}" - ); assert!(text.contains("codec_strategy_in==unrolled"), "{text}"); } @@ -190,6 +184,94 @@ fn a_child_message_is_referenced_through_its_own_table() { ); } +#[test] +fn a_child_without_a_table_is_reached_through_its_message_impl() { + let (code, _) = run(&table_config(CodecStrategy::Table)).unwrap(); + let code = squashed(&code); + // `HasOneofy` is a table message that holds `Oneofy`, which is unrolled. + let holder = code.split("static__BUFFA_TABLE_HasOneofy").nth(1).unwrap(); + let holder = holder + .split("impl::buffa::MessageforHasOneofy") + .next() + .unwrap(); + assert!( + holder.contains("Aux::Msg(&::buffa::table::MsgVt::new_via_message::<::buffa::MessageField>>())"), + "{holder}" + ); + assert!(!holder.contains("__BUFFA_TABLE_Oneofy"), "{holder}"); +} + +#[test] +fn a_repeated_child_without_a_table_is_reached_through_its_message_impl() { + let mut file = schema(); + file.message_type[4].field[0].label = Some(Label::LABEL_REPEATED); + let (files, _) = generate_with_diagnostics( + &[file], + &["t.proto".to_string()], + &table_config(CodecStrategy::Table), + ) + .unwrap(); + let code = squashed(&joined(&files)); + let holder = code.split("static__BUFFA_TABLE_HasOneofy").nth(1).unwrap(); + let holder = holder + .split("impl::buffa::MessageforHasOneofy") + .next() + .unwrap(); + assert!( + holder.contains("Aux::Rep(&::buffa::table::RepVt::new_via_message::())"), + "{holder}" + ); + assert!(holder.contains("Vec"), "{holder}"); +} + +#[test] +fn a_child_in_another_package_is_reached_through_the_path_idiomatic_imports_shortens() { + let other = FileDescriptorProto { + package: Some("a".to_string()), + message_type: vec![message("Cold", vec![scalar("c", 1, Type::TYPE_INT32)])], + ..proto3_file("a.proto") + }; + let holder = FileDescriptorProto { + package: Some("b".to_string()), + dependency: vec!["a.proto".to_string()], + message_type: vec![message( + "Holder", + vec![ + message_field("cold", 1, ".a.Cold"), + repeated_message_field("colds", 2, ".a.Cold"), + ], + )], + ..proto3_file("b.proto") + }; + let config = CodeGenConfig { + codec_strategy_in: vec![(".a.Cold".to_string(), CodecStrategy::Unrolled)], + idiomatic_imports: true, + file_per_package: true, + ..table_config(CodecStrategy::Table) + }; + let (files, warnings) = generate_with_diagnostics( + &[other, holder], + &["a.proto".to_string(), "b.proto".to_string()], + &config, + ) + .unwrap(); + assert!(table_warnings(&warnings).is_empty(), "{warnings:?}"); + let code = squashed(&joined(&files)); + let holder = code.split("static__BUFFA_TABLE_Holder").nth(1).unwrap(); + let holder = holder + .split("impl::buffa::MessageforHolder") + .next() + .unwrap(); + assert!( + holder.contains("MsgVt::new_via_message::>>()"), + "{holder}" + ); + assert!( + holder.contains("RepVt::new_via_message::()"), + "{holder}" + ); +} + #[test] fn a_rule_selects_messages_when_the_global_setting_is_unrolled() { let config = CodeGenConfig { @@ -214,11 +296,11 @@ fn the_last_matching_rule_wins() { }; let (code, _) = run(&config).unwrap(); // `Outer` is unrolled, though its nested message is a table. - assert_eq!(tables(&code), ["Leaf", "HasLeaf", "Inner"]); + assert_eq!(tables(&code), ["Leaf", "HasLeaf", "HasOneofy", "Inner"]); } #[test] -fn an_exact_path_rule_for_a_message_that_holds_an_unrolled_child_is_an_error() { +fn a_message_can_have_a_table_when_it_holds_a_message_set_to_unrolled() { let config = CodeGenConfig { codec_strategy_in: vec![ (".t.HasLeaf".to_string(), CodecStrategy::Table), @@ -226,33 +308,29 @@ fn an_exact_path_rule_for_a_message_that_holds_an_unrolled_child_is_an_error() { ], ..Default::default() }; - // The exact-path rule for `HasLeaf` cannot be honoured: its `Leaf` has no - // table, so this is an error and not a warning. - let err = run(&config).unwrap_err().to_string(); - assert!( - err.contains("codec_strategy_in rule '.t.HasLeaf'") - && err.contains("`.t.Leaf`, which is set to the unrolled codec") - && err.contains("codec_strategy_in=.t.HasLeaf=unrolled"), - "{err}" - ); + let (code, warnings) = run(&config).unwrap(); + assert_eq!(tables(&code), ["HasLeaf"]); + assert!(table_warnings(&warnings).is_empty(), "{warnings:?}"); + assert!(squashed(&code).contains("MsgVt::new_via_message::<")); } #[test] -fn an_exact_path_rule_does_not_select_the_messages_the_message_holds() { - // The global setting stays unrolled, so `Leaf` has not been selected. +fn a_rule_for_a_message_does_not_select_the_messages_it_holds() { + // The global setting stays unrolled, so `Leaf` is not selected, and + // `HasLeaf` reaches it through its `Message` impl. let config = CodeGenConfig { codec_strategy_in: vec![(".t.HasLeaf".to_string(), CodecStrategy::Table)], ..Default::default() }; - let err = run(&config).unwrap_err().to_string(); - assert!( - err.contains("`.t.Leaf`, which is not selected for the table codec") - && err.contains("codec_strategy_in(CodecStrategy::Table, &[\".t.Leaf\"])") - && !err.contains("set to the unrolled"), - "{err}" - ); + let (code, warnings) = run(&config).unwrap(); + assert_eq!(tables(&code), ["HasLeaf"]); + assert!(table_warnings(&warnings).is_empty(), "{warnings:?}"); + let code = squashed(&code); + assert!(code.contains("MsgVt::new_via_message::<"), "{code}"); + assert!(!code.contains("(&__BUFFA_TABLE_Leaf)"), "{code}"); - // A rule for the child as well gives both a table. + // A rule for the child as well gives both a table, and the parent then + // uses the child's. let config = CodeGenConfig { codec_strategy_in: vec![ (".t.HasLeaf".to_string(), CodecStrategy::Table), @@ -263,20 +341,33 @@ fn an_exact_path_rule_does_not_select_the_messages_the_message_holds() { let (code, warnings) = run(&config).unwrap(); assert_eq!(tables(&code), ["Leaf", "HasLeaf"]); assert!(table_warnings(&warnings).is_empty()); + let code = squashed(&code); + assert!(code.contains("(&__BUFFA_TABLE_Leaf)"), "{code}"); + assert!(!code.contains("new_via_message"), "{code}"); } #[test] fn every_exact_path_rule_that_cannot_be_honoured_is_reported_at_once() { + let mut file = schema(); + let mut second = file.message_type[3].clone(); + second.name = Some("Oneofy2".to_string()); + file.message_type.push(second); let config = CodeGenConfig { codec_strategy_in: vec![ (".t.Oneofy".to_string(), CodecStrategy::Table), + (".t.Oneofy2".to_string(), CodecStrategy::Table), (".t.HasOneofy".to_string(), CodecStrategy::Table), ], ..Default::default() }; - let err = run(&config).unwrap_err().to_string(); + let err = generate_with_diagnostics(&[file], &["t.proto".to_string()], &config) + .unwrap_err() + .to_string(); + // `HasOneofy` can use the table, so it is not among them. assert!( - err.contains("rule '.t.Oneofy'") && err.contains("rule '.t.HasOneofy'"), + err.contains("rule '.t.Oneofy'") + && err.contains("rule '.t.Oneofy2'") + && !err.contains("rule '.t.HasOneofy'"), "{err}" ); } @@ -301,30 +392,29 @@ fn a_broad_rule_that_covers_such_a_message_only_warns() { let (code, warnings) = run(&config).unwrap(); assert_eq!( tables(&code), - ["Plain", "Leaf", "HasLeaf", "Outer", "Inner"] + ["Plain", "Leaf", "HasLeaf", "HasOneofy", "Outer", "Inner"] ); // Messages a rule selects are counted like the ones the global setting does. - assert_eq!(summary(&warnings).0, (2, 7)); + assert_eq!(summary(&warnings).0, (1, 7)); } #[test] -fn setting_a_message_to_unrolled_keeps_the_messages_that_hold_it_unrolled_quietly() { +fn setting_a_message_to_unrolled_does_not_affect_the_messages_that_hold_it() { let config = CodeGenConfig { codec_strategy_in: vec![(".t.Leaf".to_string(), CodecStrategy::Unrolled)], ..table_config(CodecStrategy::Table) }; let (code, warnings) = run(&config).unwrap(); - // `HasLeaf` holds the `Leaf` the user set to unrolled: not a table, and - // not in the warning either, which is left with the two that cannot. - assert_eq!(tables(&code), ["Plain", "Outer", "Inner"]); - let (counts, reasons) = summary(&warnings); - assert_eq!(counts, (2, 5)); - assert!( - reasons.iter().all(|(r, _)| !r.contains("set to")), - "{reasons:?}" + // `HasLeaf` holds the `Leaf` the user set to unrolled and has a table + // anyway. The warning counts the messages selected, which leaves out `Leaf`. + assert_eq!( + tables(&code), + ["Plain", "HasLeaf", "HasOneofy", "Outer", "Inner"] ); + assert_eq!(summary(&warnings).0, (1, 6)); - // With the two that cannot set to unrolled as well, nothing is left to say. + // Once the message that cannot use the table is set to unrolled too, no + // fallback is left to report. let config = CodeGenConfig { codec_strategy_in: vec![ (".t.Leaf".to_string(), CodecStrategy::Unrolled), @@ -360,7 +450,7 @@ fn a_rule_that_matches_no_message_warns() { } #[test] -fn a_message_type_from_another_crate_is_not_a_table() { +fn a_message_type_from_another_crate_is_reached_through_its_message_impl() { // `HasLeaf.leaf` now names a type mapped to another crate. let mut file = schema(); file.message_type[2].field[0].type_name = Some(".other.Foreign".to_string()); @@ -376,18 +466,19 @@ fn a_message_type_from_another_crate_is_not_a_table() { let (files, warnings) = generate_with_diagnostics(&[file, other], &["t.proto".to_string()], &config).unwrap(); let code = joined(&files); - assert!(!tables(&code).contains(&"HasLeaf".to_string()), "{code}"); + assert!(tables(&code).contains(&"HasLeaf".to_string()), "{code}"); assert!(tables(&code).contains(&"Leaf".to_string())); - let text = table_warnings(&warnings)[0].to_string(); - assert!(text.contains(".t.HasLeaf"), "{text}"); + assert_eq!(table_warnings(&warnings).len(), 1, "{warnings:?}"); + let code = squashed(&code); + let holder = code.split("static__BUFFA_TABLE_HasLeaf").nth(1).unwrap(); assert!( - text.contains("holds a message that another crate or run generates"), - "{text}" + holder.contains("MsgVt::new_via_message::<::buffa::MessageField<::other_crate::Foreign"), + "{holder}" ); } #[test] -fn a_child_mapped_to_another_crate_falls_back_though_this_run_generates_it_too() { +fn a_child_mapped_to_another_crate_is_not_a_table_though_this_run_generates_it_too() { let mut file = schema(); file.message_type[2].field[0].type_name = Some(".other.Foreign".to_string()); let other = FileDescriptorProto { @@ -400,19 +491,18 @@ fn a_child_mapped_to_another_crate_falls_back_though_this_run_generates_it_too() ..table_config(CodecStrategy::Table) }; // Both files are generated, but `HasLeaf` names `::other_crate::Foreign`, - // which has no table here, so this must be a fallback and not an error. - let (files, warnings) = generate_with_diagnostics( + // which has no table here, so it is reached through its `Message` impl and + // its table is not looked up, which would not build. + let (files, _) = generate_with_diagnostics( &[file, other], &["t.proto".to_string(), "other.proto".to_string()], &config, ) .unwrap(); let code = joined(&files); - assert!(!tables(&code).contains(&"HasLeaf".to_string()), "{code}"); + assert!(tables(&code).contains(&"HasLeaf".to_string()), "{code}"); assert!(!tables(&code).contains(&"Foreign".to_string())); - assert!(table_warnings(&warnings)[0] - .to_string() - .contains(".t.HasLeaf")); + assert!(!code.contains("__BUFFA_TABLE_Foreign"), "{code}"); } #[test] @@ -554,29 +644,6 @@ fn the_plan_judges_fields_under_the_same_features_as_the_generator() { assert_eq!(summary(&warnings).0, (2, 2)); } -#[test] -fn a_child_the_user_did_not_choose_does_not_hide_one_that_cannot_use_the_table() { - // `Both` holds `Leaf`, which the user sets to unrolled, and then - // `HasOneofy`, which cannot use the table. The first child alone would - // make the fallback silent, but `Both` falls back for the second too. - let mut file = schema(); - file.message_type.push(message( - "Both", - vec![ - message_field("leaf", 1, ".t.Leaf"), - message_field("o", 2, ".t.HasOneofy"), - ], - )); - let config = CodeGenConfig { - codec_strategy_in: vec![(".t.Leaf".to_string(), CodecStrategy::Unrolled)], - ..table_config(CodecStrategy::Table) - }; - let (_, warnings) = - generate_with_diagnostics(&[file], &["t.proto".to_string()], &config).unwrap(); - let text = table_warnings(&warnings)[0].to_string(); - assert!(text.contains(".t.Both"), "{text}"); -} - #[test] fn the_generated_abi_is_a_literal_that_matches_the_runtime() { // The runtime refuses a table generated for another ABI, so the generator @@ -591,3 +658,364 @@ fn the_generated_abi_is_a_literal_that_matches_the_runtime() { ); assert!(!code.contains("abi=::buffa::table::ABI"), "{code}"); } + +// --------------------------------------------------------------------------- +// Messages that hold a message stored with a non-default bytes type +// --------------------------------------------------------------------------- + +const HOLDS_BYTES: &str = "holds a message with bytes fields of a non-default type"; +const CUSTOM_FIELD: &str = "has a field with a custom string, bytes or collection type"; + +fn bytes_field(name: &str, number: i32) -> FieldDescriptorProto { + scalar(name, number, Type::TYPE_BYTES) +} + +fn repeated_message_field(name: &str, number: i32, type_name: &str) -> FieldDescriptorProto { + FieldDescriptorProto { + label: Some(Label::LABEL_REPEATED), + ..message_field(name, number, type_name) + } +} + +/// Package `b` with: +/// +/// - `Blob` (a bytes field, which the rules below give the type `Bytes`) and +/// `PlainBytes` (a bytes field that keeps `Vec`); +/// - `HasBlob`, `HasBlobs` (repeated) and `HoldsHasBlob`, which hold a `Blob` +/// directly, in a list and through `HasBlob`; +/// - `OneofBlob`, which has a oneof member of type `Blob`, and `HoldsOneofBlob` +/// (holds `OneofBlob`); +/// - `MapBlob`, which has a map with `Blob` values, and `HoldsMapBlob`; +/// - `HoldsPlain` (holds `PlainBytes`), `Leaf` and `HoldsLeaf`, which are +/// unaffected. +fn bytes_schema() -> FileDescriptorProto { + let mut oneof_blob = message( + "OneofBlob", + vec![FieldDescriptorProto { + oneof_index: Some(0), + ..message_field("blob", 1, ".b.Blob") + }], + ); + oneof_blob.oneof_decl = vec![OneofDescriptorProto { + name: Some("choice".to_string()), + ..Default::default() + }]; + let mut map_blob = message( + "MapBlob", + vec![repeated_message_field("blobs", 1, ".b.MapBlob.BlobsEntry")], + ); + map_blob.nested_type = vec![DescriptorProto { + name: Some("BlobsEntry".to_string()), + field: vec![ + scalar("key", 1, Type::TYPE_STRING), + message_field("value", 2, ".b.Blob"), + ], + options: (MessageOptions { + map_entry: Some(true), + ..Default::default() + }) + .into(), + ..Default::default() + }]; + FileDescriptorProto { + package: Some("b".to_string()), + message_type: vec![ + message("Blob", vec![bytes_field("data", 1)]), + message("PlainBytes", vec![bytes_field("data", 1)]), + message("HasBlob", vec![message_field("blob", 1, ".b.Blob")]), + message( + "HasBlobs", + vec![repeated_message_field("blobs", 1, ".b.Blob")], + ), + message("HoldsHasBlob", vec![message_field("has", 1, ".b.HasBlob")]), + oneof_blob, + message( + "HoldsOneofBlob", + vec![message_field("o", 1, ".b.OneofBlob")], + ), + map_blob, + message("HoldsMapBlob", vec![message_field("m", 1, ".b.MapBlob")]), + message("HoldsPlain", vec![message_field("p", 1, ".b.PlainBytes")]), + message("Leaf", vec![scalar("x", 1, Type::TYPE_INT32)]), + message("HoldsLeaf", vec![message_field("leaf", 1, ".b.Leaf")]), + ], + ..proto3_file("b.proto") + } +} + +fn run_bytes(config: &CodeGenConfig) -> Result<(String, Vec), CodeGenError> { + let (files, warnings) = + generate_with_diagnostics(&[bytes_schema()], &["b.proto".to_string()], config)?; + Ok((joined(&files), warnings)) +} + +/// The table strategy, with `Blob.data` stored as `Bytes`. +fn blob_config() -> CodeGenConfig { + CodeGenConfig { + bytes_fields: vec![(".b.Blob.data".to_string(), BytesRepr::Bytes)], + ..table_config(CodecStrategy::Table) + } +} + +#[test] +fn a_message_that_holds_a_message_with_a_bytes_type_stays_unrolled() { + let (code, warnings) = run_bytes(&blob_config()).unwrap(); + // The holders of `Blob` fall back, directly, in a list, transitively, and + // through a oneof member and a map value, though the last two also hold + // messages that cannot use the table for other reasons. + assert_eq!( + tables(&code), + ["PlainBytes", "HoldsPlain", "Leaf", "HoldsLeaf"] + ); + let (counts, reasons) = summary(&warnings); + assert_eq!(counts, (8, 12)); + // `OneofBlob` and `MapBlob` fall back for their oneof and map, so they are + // not counted as holders. + assert_eq!( + reasons, + [ + (HOLDS_BYTES, 5), + (CUSTOM_FIELD, 1), + ("has a oneof", 1), + ("has a map field", 1), + ] + ); + let text = table_warnings(&warnings)[0].to_string(); + assert!(text.contains(HOLDS_BYTES), "{text}"); + assert!(text.contains(".b.HasBlob"), "{text}"); +} + +#[test] +fn a_message_with_a_plain_bytes_field_is_unaffected() { + let (code, _) = run_bytes(&blob_config()).unwrap(); + let plain = squashed(&code); + assert!(tables(&code).contains(&"PlainBytes".to_string())); + // `HoldsPlain` reaches `PlainBytes` through its table. + let holder = plain + .split("static__BUFFA_TABLE_HoldsPlain") + .nth(1) + .unwrap(); + assert!(holder.contains("(&__BUFFA_TABLE_PlainBytes)"), "{holder}"); +} + +#[test] +fn a_child_set_to_unrolled_without_bytes_still_lets_its_holder_use_the_table() { + let config = CodeGenConfig { + codec_strategy_in: vec![(".b.Leaf".to_string(), CodecStrategy::Unrolled)], + ..blob_config() + }; + let (code, _) = run_bytes(&config).unwrap(); + assert!(tables(&code).contains(&"HoldsLeaf".to_string()), "{code}"); + assert!(!tables(&code).contains(&"Leaf".to_string())); + assert!(squashed(&code).contains("MsgVt::new_via_message::<")); +} + +#[test] +fn a_child_set_to_unrolled_that_has_a_bytes_type_keeps_its_holder_unrolled() { + // The rule does not decide it: `Blob` is stored as `Bytes` either way. + let config = CodeGenConfig { + codec_strategy_in: vec![(".b.Blob".to_string(), CodecStrategy::Unrolled)], + ..blob_config() + }; + let (code, warnings) = run_bytes(&config).unwrap(); + assert!(!tables(&code).contains(&"HasBlob".to_string()), "{code}"); + assert!(summary(&warnings).1.contains(&(HOLDS_BYTES, 5))); +} + +#[test] +fn a_bytes_type_for_every_field_keeps_the_holders_of_every_bytes_message_unrolled() { + let config = CodeGenConfig { + bytes_fields: vec![(".".to_string(), BytesRepr::Bytes)], + ..table_config(CodecStrategy::Table) + }; + let (code, _) = run_bytes(&config).unwrap(); + // With this rule `PlainBytes` has a `Bytes` field, so it stays unrolled and + // so does every message that holds it. + assert_eq!(tables(&code), ["Leaf", "HoldsLeaf"]); +} + +#[test] +fn a_bytes_type_on_another_message_does_not_affect_a_holder() { + let config = CodeGenConfig { + bytes_fields: vec![(".b.PlainBytes.data".to_string(), BytesRepr::Bytes)], + ..table_config(CodecStrategy::Table) + }; + let (code, _) = run_bytes(&config).unwrap(); + assert!(tables(&code).contains(&"HasBlob".to_string()), "{code}"); + assert!(!tables(&code).contains(&"HoldsPlain".to_string())); +} + +#[test] +fn an_exact_path_rule_for_a_holder_of_a_bytes_typed_message_is_an_error() { + let config = CodeGenConfig { + codec_strategy_in: vec![(".b.HasBlob".to_string(), CodecStrategy::Table)], + ..blob_config() + }; + let err = run_bytes(&config).unwrap_err().to_string(); + assert!(err.contains("cannot use it"), "{err}"); + assert!(err.contains("it holds `.b.Blob`"), "{err}"); +} + +// --------------------------------------------------------------------------- +// How far the bytes rule reaches +// --------------------------------------------------------------------------- + +fn repeated_bytes_field(name: &str, number: i32) -> FieldDescriptorProto { + FieldDescriptorProto { + label: Some(Label::LABEL_REPEATED), + ..bytes_field(name, number) + } +} + +/// A message with the one map field `m` whose entries have a key and a value +/// of the given types. +fn message_with_map(name: &str, key: Type, value: Type) -> DescriptorProto { + let mut msg = message( + name, + vec![repeated_message_field("m", 1, &format!(".c.{name}.MEntry"))], + ); + msg.nested_type = vec![DescriptorProto { + name: Some("MEntry".to_string()), + field: vec![scalar("key", 1, key), scalar("value", 2, value)], + options: (MessageOptions { + map_entry: Some(true), + ..Default::default() + }) + .into(), + ..Default::default() + }]; + msg +} + +/// Package `c` with: +/// +/// - `ChainA` holds `ChainB` holds `ChainC` holds `Blob`, declared holder +/// first, so a holder is judged before the message it holds; +/// - `CycleP` and `CycleQ` hold each other, and `CycleQ` has a bytes field, and +/// `CycleX` and `CycleY` hold each other and have none; +/// - `RepBytes` (a repeated bytes field), `MapBytes` (a `map`) +/// and `Outer.Inner` (a nested message), each with a holder; +/// - `BytesKeyMap`, a `map`, whose values keep `Vec` whatever +/// the rule says, and its holder. Protoc rejects a bytes key, so only a +/// hand-built descriptor can have one. +fn taint_schema() -> FileDescriptorProto { + let mut outer = message("Outer", vec![]); + outer.nested_type = vec![message("Inner", vec![bytes_field("data", 1)])]; + FileDescriptorProto { + package: Some("c".to_string()), + message_type: vec![ + message("ChainA", vec![message_field("b", 1, ".c.ChainB")]), + message("ChainB", vec![message_field("c", 1, ".c.ChainC")]), + message("ChainC", vec![message_field("blob", 1, ".c.Blob")]), + message("Blob", vec![bytes_field("data", 1)]), + message("CycleP", vec![message_field("q", 1, ".c.CycleQ")]), + message( + "CycleQ", + vec![message_field("p", 1, ".c.CycleP"), bytes_field("data", 2)], + ), + message("CycleX", vec![message_field("y", 1, ".c.CycleY")]), + message("CycleY", vec![message_field("x", 1, ".c.CycleX")]), + message("RepBytes", vec![repeated_bytes_field("chunks", 1)]), + message("HoldsRepBytes", vec![message_field("r", 1, ".c.RepBytes")]), + message_with_map("MapBytes", Type::TYPE_STRING, Type::TYPE_BYTES), + message("HoldsMapBytes", vec![message_field("m", 1, ".c.MapBytes")]), + message_with_map("BytesKeyMap", Type::TYPE_BYTES, Type::TYPE_BYTES), + message( + "HoldsBytesKeyMap", + vec![message_field("m", 1, ".c.BytesKeyMap")], + ), + outer, + message("HoldsInner", vec![message_field("i", 1, ".c.Outer.Inner")]), + ], + ..proto3_file("c.proto") + } +} + +/// What the plan made of `taint_schema`. +struct Taint { + tables: Vec, + /// The messages that fell back and the messages selected. + counts: (usize, usize), + /// The messages that fell back for holding a message with a bytes type. + holders: usize, +} + +/// The table strategy with every bytes field of `taint_schema` stored as +/// `Bytes`. +fn run_taint() -> Taint { + let config = CodeGenConfig { + bytes_fields: [ + ".c.Blob.data", + ".c.CycleQ.data", + ".c.RepBytes.chunks", + ".c.MapBytes.m", + ".c.BytesKeyMap.m", + ".c.Outer.Inner.data", + ] + .map(|path| (path.to_string(), BytesRepr::Bytes)) + .to_vec(), + ..table_config(CodecStrategy::Table) + }; + let (files, warnings) = + generate_with_diagnostics(&[taint_schema()], &["c.proto".to_string()], &config).unwrap(); + let (counts, reasons) = summary(&warnings); + let holders = reasons + .iter() + .find_map(|&(reason, n)| (reason == HOLDS_BYTES).then_some(n)) + .unwrap_or(0); + Taint { + tables: tables(&joined(&files)), + counts, + holders, + } +} + +#[test] +fn a_chain_of_holders_declared_holder_first_is_unrolled_all_the_way() { + let tables = run_taint().tables; + for name in ["ChainA", "ChainB", "ChainC", "Blob"] { + assert!(!tables.contains(&name.to_string()), "{name}: {tables:?}"); + } +} + +#[test] +fn a_cycle_that_reaches_a_bytes_message_is_unrolled_and_one_that_does_not_is_not() { + let tables = run_taint().tables; + for name in ["CycleP", "CycleQ"] { + assert!(!tables.contains(&name.to_string()), "{name}: {tables:?}"); + } + for name in ["CycleX", "CycleY"] { + assert!(tables.contains(&name.to_string()), "{name}: {tables:?}"); + } +} + +#[test] +fn a_message_holding_repeated_map_or_nested_bytes_is_unrolled() { + let plan = run_taint(); + for name in ["HoldsRepBytes", "HoldsMapBytes", "HoldsInner"] { + assert!( + !plan.tables.contains(&name.to_string()), + "{name}: {:?}", + plan.tables + ); + } + // A message with a plain nested declaration is unaffected. + assert!( + plan.tables.contains(&"Outer".to_string()), + "{:?}", + plan.tables + ); + // The holders counted under the one reason are the three above, the chain + // and the cycle. + assert_eq!(plan.holders, 7); + assert_eq!(plan.counts, (13, 17)); +} + +#[test] +fn a_map_with_a_bytes_key_keeps_vec_values_so_its_holder_uses_the_table() { + let tables = run_taint().tables; + assert!( + tables.contains(&"HoldsBytesKeyMap".to_string()), + "{tables:?}" + ); +} diff --git a/buffa-test/build.rs b/buffa-test/build.rs index 59cbfee4..4912c108 100644 --- a/buffa-test/build.rs +++ b/buffa-test/build.rs @@ -1,8 +1,9 @@ /// Compile a schema twice, with its `package ` renamed to `u` and /// generated with the default unrolled codec, and to `t` and generated -/// with `codec_strategy = Table`. A test compares the two codecs on the same -/// schema. `file` names the schema in messages. -fn compile_both_codecs(file: &str, source: &str, base: &str) { +/// with `codec_strategy = Table`, except for the messages in `unrolled` (paths +/// below the package, such as `Hot`), which stay unrolled. A test compares the +/// two codecs on the same schema. `file` names the schema in messages. +fn compile_both_codecs(file: &str, source: &str, base: &str, unrolled: &[&str]) { let out = std::path::PathBuf::from(std::env::var("OUT_DIR").expect("OUT_DIR")); let package = format!("package {base};"); assert!(source.contains(&package), "{file} must declare `{package}`"); @@ -16,29 +17,79 @@ fn compile_both_codecs(file: &str, source: &str, base: &str) { source.replace(&package, &format!("package {base}{suffix};")), ) .expect("write renamed proto"); + let rules: Vec = unrolled + .iter() + .map(|path| format!(".{base}{suffix}.{path}")) + .collect(); buffa_build::Config::new() .files(&[renamed]) .includes(&[&out]) .generate_json(true) .generate_text(true) .codec_strategy(strategy) + .codec_strategy_in(buffa_build::CodecStrategy::Unrolled, &rules) .compile() .unwrap_or_else(|e| panic!("buffa_build failed for {file} ({suffix}): {e}")); } } +/// A package `xe` of table messages, and packages `xfu` (unrolled) and `xft` +/// (table) with messages that hold them, generated in their own runs with `xe` +/// mapped to the crate's `xe` module. A table message then holds messages from +/// another crate, whose table it cannot name. +fn compile_extern_children() { + let out = std::path::PathBuf::from(std::env::var("OUT_DIR").expect("OUT_DIR")); + let leaf = out.join("xe.proto"); + std::fs::write( + &leaf, + "syntax = \"proto3\";\npackage xe;\n\ + message Leaf { int32 x = 1; string s = 2; repeated Leaf kids = 3; }\n", + ) + .expect("write proto"); + buffa_build::Config::new() + .files(&[&leaf]) + .includes(&[&out]) + .codec_strategy(buffa_build::CodecStrategy::Table) + .compile() + .expect("buffa_build failed for xe.proto"); + for (suffix, strategy) in [ + ("u", buffa_build::CodecStrategy::Unrolled), + ("t", buffa_build::CodecStrategy::Table), + ] { + let holder = out.join(format!("xf{suffix}.proto")); + std::fs::write( + &holder, + format!( + "syntax = \"proto3\";\npackage xf{suffix};\nimport \"xe.proto\";\n\ + message Holder {{ xe.Leaf leaf = 1; repeated xe.Leaf leaves = 2; int32 tail = 3; }}\n" + ), + ) + .expect("write proto"); + buffa_build::Config::new() + .files(&[&holder]) + .includes(&[&out]) + .extern_path(".xe", "crate::xe") + .codec_strategy(strategy) + .compile() + .unwrap_or_else(|e| panic!("buffa_build failed for xf{suffix}.proto: {e}")); + } +} + /// Two packages, the second holding messages of the first, compiled three /// ways: unrolled (`xau`, `xbu`), table (`xat`, `xbt`), and table with /// `file_per_package` and `idiomatic_imports` (`xati`, `xbti`), which shortens /// the paths of types in other packages and so changes what a table path may -/// be. +/// be. `Cold`, which `Holder` holds singly and in a list, stays unrolled in +/// all three, so the table holders reach it through its `Message` impl with +/// the shortened path. fn compile_cross_package() { let out = std::path::PathBuf::from(std::env::var("OUT_DIR").expect("OUT_DIR")); let sources = |suffix: &str| { let dep = format!( "syntax = \"proto3\";\npackage xa{suffix};\n\ message Leaf {{ int32 x = 1; string s = 2; }}\n\ - message Wrap {{ Leaf leaf = 1; repeated Leaf leaves = 2; }}\n" + message Wrap {{ Leaf leaf = 1; repeated Leaf leaves = 2; }}\n\ + message Cold {{ int64 c = 1; string s = 2; }}\n" ); let user = format!( "syntax = \"proto3\";\npackage xb{suffix};\nimport \"xa{suffix}.proto\";\n\ @@ -47,6 +98,8 @@ fn compile_cross_package() { repeated xa{suffix}.Leaf leaves = 2;\n\ xa{suffix}.Wrap wrap = 3;\n\ Sub sub = 4;\n\ + xa{suffix}.Cold cold = 5;\n\ + repeated xa{suffix}.Cold colds = 6;\n\ message Sub {{ xa{suffix}.Leaf l = 1; }}\n\ }}\n" ); @@ -67,7 +120,11 @@ fn compile_cross_package() { let mut config = buffa_build::Config::new() .files(&[dep_path, user_path]) .includes(&[&out]) - .codec_strategy(strategy); + .codec_strategy(strategy) + .codec_strategy_in( + buffa_build::CodecStrategy::Unrolled, + &[format!(".xa{suffix}.Cold")], + ); if idiomatic { let dir = out.join("cross_package_idiomatic"); std::fs::create_dir_all(&dir).expect("create dir"); @@ -167,19 +224,42 @@ fn main() { println!("cargo:rustc-check-cfg=cfg(has_table_codec)"); if rustc_minor() >= 77 { println!("cargo:rustc-cfg=has_table_codec"); - compile_both_codecs("table_codec.proto", &read_proto("table_codec.proto"), "tc"); + compile_both_codecs( + "table_codec.proto", + &read_proto("table_codec.proto"), + "tc", + &[], + ); compile_both_codecs( "table_codec2.proto", &read_proto("table_codec2.proto"), "tc2", + &[], ); compile_both_codecs( "table_codec3.proto", &read_proto("table_codec3.proto"), "tc3", + &[], + ); + compile_both_codecs("the generated wide schema", &wide_proto(), "wide", &[]); + compile_both_codecs( + "table_bridge.proto", + &read_proto("table_bridge.proto"), + "br", + &["Hot"], ); - compile_both_codecs("the generated wide schema", &wide_proto(), "wide"); compile_cross_package(); + compile_extern_children(); + // `bytes` fields as `bytes::Bytes`, which the messages that hold one + // must keep unrolled. + buffa_build::Config::new() + .files(&["protos/table_bytes.proto"]) + .includes(&["protos/"]) + .codec_strategy(buffa_build::CodecStrategy::Table) + .use_bytes_type() + .compile() + .expect("buffa_build failed for table_bytes.proto"); compile_table_with_options("table_codec.proto", "tc"); } diff --git a/buffa-test/protos/table_bridge.proto b/buffa-test/protos/table_bridge.proto new file mode 100644 index 00000000..3bfda171 --- /dev/null +++ b/buffa-test/protos/table_bridge.proto @@ -0,0 +1,57 @@ +syntax = "proto3"; + +// build.rs renames this package to `bru` (everything unrolled) and `brt` +// (`codec_strategy = Table`, with `Hot` set to unrolled by a rule), so a test +// can compare the two codecs. In `brt`, `Cold` and `Leaf` are table messages +// and `Hot` is not, so the messages hold each other across the two codecs in +// both directions. +package br; + +import "google/protobuf/any.proto"; +import "google/protobuf/duration.proto"; +import "google/protobuf/empty.proto"; +import "google/protobuf/field_mask.proto"; +import "google/protobuf/struct.proto"; +import "google/protobuf/timestamp.proto"; +import "google/protobuf/wrappers.proto"; + +message Leaf { + int32 x = 1; + string s = 2; + repeated int32 tags = 3; +} + +// Unrolled in `brt`, and holds a table message, and the table message that +// holds it. +message Hot { + int32 a = 1; + Leaf leaf = 2; + Cold back = 3; + repeated Leaf leaves = 4; + bytes blob = 5; +} + +message Cold { + Hot hot = 1; + repeated Hot hots = 2; + Leaf leaf = 3; + repeated Leaf leaves = 4; + int32 tail = 5; +} + +// Well-known types, which no run generates as table messages. +message Wkt { + google.protobuf.Timestamp ts = 1; + google.protobuf.Duration dur = 2; + google.protobuf.Any any = 3; + google.protobuf.Struct st = 4; + google.protobuf.Value val = 5; + google.protobuf.FieldMask mask = 6; + google.protobuf.Int32Value wrapped = 7; + google.protobuf.StringValue wrapped_s = 8; + google.protobuf.Empty empty = 9; + repeated google.protobuf.Timestamp times = 10; + repeated google.protobuf.Any anys = 11; + repeated google.protobuf.Value values = 12; + Cold cold = 13; +} diff --git a/buffa-test/protos/table_bytes.proto b/buffa-test/protos/table_bytes.proto new file mode 100644 index 00000000..96e0347f --- /dev/null +++ b/buffa-test/protos/table_bytes.proto @@ -0,0 +1,32 @@ +syntax = "proto3"; + +// build.rs compiles this once, as package `tbz`, with `codec_strategy = Table` +// and `bytes` fields stored as `bytes::Bytes`. A table decodes from a +// contiguous slice, where `Bytes` fields could not alias the input, so `Blob`, +// which has such fields, is unrolled, and so are `HoldsBlob`, which holds +// `Blob`, and `HoldsBlobs`, which holds `HoldsBlob`. `Plain` and `HoldsPlain` +// have no `bytes` field and are tables. +package tbz; + +message Blob { + bytes data = 1; + repeated bytes chunks = 2; +} + +message HoldsBlob { + Blob blob = 1; + repeated Blob blobs = 2; + int32 tail = 3; +} + +message HoldsBlobs { + HoldsBlob inner = 1; +} + +message Plain { + int32 x = 1; +} + +message HoldsPlain { + Plain plain = 1; +} diff --git a/buffa-test/protos/table_codec.proto b/buffa-test/protos/table_codec.proto index 5ee90e1e..d0981c1b 100644 --- a/buffa-test/protos/table_codec.proto +++ b/buffa-test/protos/table_codec.proto @@ -102,8 +102,9 @@ message Sparse { message Empty {} -// A oneof, a map, and a message that holds one: none can use the table, so -// they stay unrolled when the table is requested. +// A oneof and a map, which the table cannot handle, so `WithOneof` and +// `WithMap` stay unrolled when the table is requested. `HoldsOneof` holds both +// and is a table message, which reaches them through their `Message` impl. message WithOneof { oneof choice { int32 a = 1; @@ -124,8 +125,8 @@ message HoldsOneof { WithMap m = 3; } -// An unrolled message that holds table messages, next to unrolled ones that -// hold a oneof and a map of them. +// A table message that holds table messages (`Inner`, `Nested`) and +// `HoldsOneof`, whose own children are unrolled. message Mixed { Inner inner = 1; HoldsOneof holds = 2; diff --git a/buffa-test/src/lib.rs b/buffa-test/src/lib.rs index 8bc38f2c..3910f88c 100644 --- a/buffa-test/src/lib.rs +++ b/buffa-test/src/lib.rs @@ -1159,6 +1159,43 @@ pub mod tcx { buffa::include_proto!("tcx"); } +// `bru` is `table_bridge.proto` unrolled, and `brt` has the table codec except +// for `Hot`, so table and unrolled messages hold each other. `xe` is a package +// of table messages that `xfu` (unrolled) and `xft` (table) hold through an +// `extern_path`. +#[allow(clippy::derivable_impls, clippy::match_single_binding)] +#[cfg(has_table_codec)] +pub mod bru { + buffa::include_proto!("bru"); +} +#[forbid(unsafe_code)] +#[allow(clippy::derivable_impls, clippy::match_single_binding)] +#[cfg(has_table_codec)] +pub mod brt { + buffa::include_proto!("brt"); +} +#[forbid(unsafe_code)] +#[cfg(has_table_codec)] +pub mod xe { + buffa::include_proto!("xe"); +} +#[cfg(has_table_codec)] +pub mod xfu { + buffa::include_proto!("xfu"); +} +#[forbid(unsafe_code)] +#[cfg(has_table_codec)] +pub mod xft { + buffa::include_proto!("xft"); +} + +// `tbz` has the table codec and `bytes` fields stored as `bytes::Bytes`. The +// messages that have or hold such a field stay unrolled. +#[cfg(has_table_codec)] +pub mod tbz { + buffa::include_proto!("tbz"); +} + // Two packages, the second holding messages of the first: `xau`/`xbu` unrolled, // `xat`/`xbt` with the table codec, and `xti` with the table codec and // `file_per_package` with `idiomatic_imports`, which holds `xati` and `xbti`. diff --git a/buffa-test/src/tests/table_codec.rs b/buffa-test/src/tests/table_codec.rs index 3a59bb55..2b39cfd1 100644 --- a/buffa-test/src/tests/table_codec.rs +++ b/buffa-test/src/tests/table_codec.rs @@ -386,8 +386,8 @@ fn invalid_utf8_is_rejected() { #[test] fn messages_the_table_cannot_handle_still_work() { - // A oneof, a map, and the messages that hold them are unrolled, and a - // table message may sit next to them. + // A oneof and a map are unrolled, and the messages that hold them are + // tables that reach them through their `Message` impl. let with_oneof = crate::tct::WithOneof { choice: Some(crate::tct::with_oneof::Choice::B("x".into())), c: 4, @@ -761,8 +761,10 @@ fn messages_named_like_what_generated_code_uses_agree() { } #[test] -fn a_table_message_inside_an_unrolled_tree_agrees() { - // `Mixed.inner` is a table in `tct`, and `Mixed` and `HoldsOneof` are not. +fn unrolled_messages_inside_a_table_tree_agree() { + // `Mixed.inner` is a table in `tct`, and so are `Mixed` and `HoldsOneof`, + // which hold `WithOneof` and `WithMap`, which are not, and `WithOneof` + // holds a table message in a oneof variant. let wire = assert_same_codec(&shapes_u::mixed(), &shapes_t::mixed()); assert_same_chained::(&wire); let wire = assert_same_codec( @@ -922,6 +924,8 @@ fn the_messages_the_table_can_handle_use_it() { crate::tct::__BUFFA_TABLE_Entry, crate::tct::__BUFFA_TABLE_Aux, crate::tct::__BUFFA_TABLE_Keywords, + crate::tct::__BUFFA_TABLE_HoldsOneof, + crate::tct::__BUFFA_TABLE_Mixed, crate::tc2t::__BUFFA_TABLE_Req, crate::tc2t::__BUFFA_TABLE_AllRequired, crate::tc2t::__BUFFA_TABLE_AllRepeated, @@ -940,6 +944,11 @@ fn the_messages_the_table_can_handle_use_it() { crate::xti::xati::__BUFFA_TABLE_Wrap, crate::xti::xbti::__BUFFA_TABLE_Holder, crate::xti::xbti::holder::__BUFFA_TABLE_Sub, + crate::brt::__BUFFA_TABLE_Leaf, + crate::brt::__BUFFA_TABLE_Cold, + crate::brt::__BUFFA_TABLE_Wkt, + crate::xe::__BUFFA_TABLE_Leaf, + crate::xft::__BUFFA_TABLE_Holder, ); } @@ -965,6 +974,19 @@ fn messages_held_across_packages_agree_in_every_layout() { l: MessageField::some(leaf(6)), ..Default::default() }), + // Unrolled in the table layouts. + cold: MessageField::some($xa::Cold { + c: 7, + s: "cold".into(), + ..Default::default() + }), + colds: vec![ + $xa::Cold::default(), + $xa::Cold { + c: 8, + ..Default::default() + }, + ], ..Default::default() } }}; @@ -982,4 +1004,396 @@ fn messages_held_across_packages_agree_in_every_layout() { .encode_to_vec(), wire ); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); +} + +// --------------------------------------------------------------------------- +// Messages that hold messages without a table +// --------------------------------------------------------------------------- + +/// Builds the same values of `table_bridge.proto` in `$m`: `bru` or `brt`. +macro_rules! bridge_samples { + ($name:ident, $m:ident) => { + mod $name { + use crate::$m::{Cold, Hot, Leaf, Wkt}; + use buffa::MessageField; + use buffa_types::google::protobuf::{ + Any, Duration, Empty, FieldMask, Int32Value, StringValue, Struct, Timestamp, Value, + }; + + pub fn leaf(x: i32, s: &str, tags: &[i32]) -> Leaf { + Leaf { + x, + s: s.into(), + tags: tags.to_vec(), + ..Default::default() + } + } + + /// A `Hot`, which holds a `Cold` if `with_back`. + pub fn hot(a: i32, with_back: bool) -> Hot { + Hot { + a, + leaf: MessageField::some(leaf(a, "hot leaf", &[1, 2])), + back: if with_back { + MessageField::some(Cold { + tail: a + 100, + leaf: MessageField::some(leaf(a, "back", &[])), + ..Default::default() + }) + } else { + MessageField::none() + }, + leaves: vec![leaf(a + 1, "l", &[3]), Leaf::default()], + blob: vec![1, 2, 3], + ..Default::default() + } + } + + pub fn cold() -> Cold { + Cold { + hot: MessageField::some(hot(1, true)), + hots: vec![hot(2, false), Hot::default(), hot(3, true)], + leaf: MessageField::some(leaf(4, "c", &[5, 6])), + leaves: vec![leaf(6, "", &[]), Leaf::default()], + tail: 7, + ..Default::default() + } + } + + pub fn wkt() -> Wkt { + let mut st = Struct::new(); + st.insert("k", 1.5); + Wkt { + ts: MessageField::some(Timestamp::from_unix(1_700_000_000, 5)), + dur: MessageField::some(Duration::from_secs_nanos(-3, -4)), + any: MessageField::some(Any::pack( + &Timestamp::from_unix(1, 2), + "type.googleapis.com/google.protobuf.Timestamp", + )), + st: MessageField::some(st), + val: MessageField::some(Value::from("v")), + mask: MessageField::some(FieldMask::from_paths(["a.b", "c"])), + wrapped: MessageField::some(Int32Value::from(0)), + wrapped_s: MessageField::some(StringValue::from("w")), + empty: MessageField::some(Empty::default()), + times: vec![Timestamp::from_unix(1, 0), Timestamp::default()], + anys: vec![Any::default()], + values: vec![Value::from(true), Value::null(), Value::from(2.5)], + cold: MessageField::some(cold()), + ..Default::default() + } + } + } + }; +} + +bridge_samples!(bru_s, bru); +bridge_samples!(brt_s, brt); + +#[test] +fn messages_that_hold_each_other_across_both_codecs_agree() { + // `Cold` is a table and holds `Hot`, which is not and holds a `Cold`. + let wire = assert_same_codec(&bru_s::cold(), &brt_s::cold()); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); + + // The unrolled message as a root, holding table messages. + let wire = assert_same_codec(&bru_s::hot(9, true), &brt_s::hot(9, true)); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); +} + +#[test] +fn well_known_types_held_by_a_table_message_agree() { + let wire = assert_same_codec(&bru_s::wkt(), &brt_s::wkt()); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); + // Set to their defaults, they are still written, as empty records. + let wire = assert_same_codec( + &crate::bru::Wkt { + ts: buffa::MessageField::some(Default::default()), + wrapped: buffa::MessageField::some(Default::default()), + ..Default::default() + }, + &crate::brt::Wkt { + ts: buffa::MessageField::some(Default::default()), + wrapped: buffa::MessageField::some(Default::default()), + ..Default::default() + }, + ); + assert_eq!(wire, [0x0a, 0x00, 0x3a, 0x00]); +} + +/// The tests that compare a table message with an unrolled one that it holds +/// mean nothing if the rule that leaves the child unrolled stops matching, so +/// these check the generated code itself. +#[test] +fn the_children_set_to_unrolled_have_no_table_and_their_holders_reach_them_through_the_message_impl( +) { + let out = |file: &str| match file { + "brt" => include_str!(concat!(env!("OUT_DIR"), "/brt.rs")), + "xat" => include_str!(concat!(env!("OUT_DIR"), "/xat.rs")), + "xbt" => include_str!(concat!(env!("OUT_DIR"), "/xbt.rs")), + "xati" => include_str!(concat!(env!("OUT_DIR"), "/cross_package_idiomatic/xati.rs")), + "xbti" => include_str!(concat!(env!("OUT_DIR"), "/cross_package_idiomatic/xbti.rs")), + "xft" => include_str!(concat!(env!("OUT_DIR"), "/xft.rs")), + _ => unreachable!(), + }; + // `Hot` is unrolled, and `Cold`, which holds it, is a table. + let bridge = out("brt"); + assert!(bridge.contains("pub struct Hot ")); + assert!(!bridge.contains("__BUFFA_TABLE_Hot")); + assert!(bridge.contains("static __BUFFA_TABLE_Cold")); + assert!(bridge.contains("new_via_message")); + // `Cold` is unrolled in every layout of the cross-package schema, and + // `Holder` reaches it and the tables of `Leaf` and `Wrap` next to it. + for (dep, user) in [("xat", "xbt"), ("xati", "xbti")] { + assert!(out(dep).contains("pub struct Cold "), "{dep}"); + assert!(!out(dep).contains("__BUFFA_TABLE_Cold"), "{dep}"); + assert!(out(dep).contains("static __BUFFA_TABLE_Leaf"), "{dep}"); + assert!(out(user).contains("new_via_message"), "{user}"); + assert!(out(user).contains("static __BUFFA_TABLE_Holder"), "{user}"); + } + // The messages of `xe` come from another crate, so `Holder` cannot name + // their tables. + assert!(out("xft").contains("new_via_message")); +} + +/// `Any.value` is a `Bytes` in `buffa-types`, which codegen cannot inspect, so +/// a table message that holds an `Any` copies the payload out of the slice it +/// decodes from, where its unrolled twin shares it with a `Bytes` input. The +/// documentation names this, so it is pinned here. +#[test] +fn a_table_message_copies_the_payload_of_an_any_it_holds() { + use buffa::bytes::Bytes; + use buffa::MessageField; + use buffa_types::google::protobuf::Any; + fn any(fill: u8) -> Any { + let mut any = Any::default(); + any.type_url = "type.googleapis.com/x".into(); + any.value = Bytes::from(vec![fill; 64]); + any + } + macro_rules! sample { + ($m:ident) => { + crate::$m::Wkt { + any: MessageField::some(any(1)), + anys: vec![any(2), any(3)], + ..Default::default() + } + }; + } + let wire = assert_same_codec(&sample!(bru), &sample!(brt)); + let src = Bytes::from(wire); + let range = src.as_ptr() as usize..src.as_ptr() as usize + src.len(); + let aliases = |b: &Bytes| range.contains(&(b.as_ptr() as usize)); + + let unrolled = crate::bru::Wkt::decode(&mut src.clone()).unwrap(); + let table = crate::brt::Wkt::decode(&mut src.clone()).unwrap(); + assert_eq!(unrolled.any.as_option().unwrap().value, any(1).value); + assert_eq!(table.any.as_option().unwrap().value, any(1).value); + assert!(aliases(&unrolled.any.as_option().unwrap().value)); + assert!(unrolled.anys.iter().all(|a| aliases(&a.value))); + assert!(!aliases(&table.any.as_option().unwrap().value)); + assert!(table.anys.iter().all(|a| !aliases(&a.value))); + assert_eq!(table.anys, unrolled.anys); +} + +#[test] +fn messages_from_another_crate_agree() { + macro_rules! sample { + ($m:ident) => {{ + let leaf = |x: i32, s: &str| crate::xe::Leaf { + x, + s: s.into(), + kids: vec![crate::xe::Leaf::default()], + ..Default::default() + }; + crate::$m::Holder { + leaf: buffa::MessageField::some(leaf(1, "a")), + leaves: vec![leaf(2, "b"), leaf(3, "")], + tail: 4, + ..Default::default() + } + }}; + } + let wire = assert_same_codec(&sample!(xfu), &sample!(xft)); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); +} + +/// The wire form of `Cold` with `hot` set to a `Hot` with `back` set to a +/// `Cold`, and so on, `pairs` times, innermost empty. +fn cold_hot_chain(pairs: usize) -> Vec { + let wrap = |tag: u8, inner: Vec| { + let mut wire = vec![tag]; + buffa::encoding::encode_varint(inner.len() as u64, &mut wire); + wire.extend(inner); + wire + }; + let mut wire = Vec::new(); + for _ in 0..pairs { + // `Hot.back` (3), then `Cold.hot` (1). + wire = wrap(0x0a, wrap(0x1a, wire)); + } + wire +} + +#[test] +fn nesting_through_both_codecs_is_limited_alike() { + for pairs in [1, 40, 49, 50, 51, 60] { + let wire = cold_hot_chain(pairs); + assert_same_decode::(&wire, false); + } + assert_eq!( + ::decode_from_slice(&cold_hot_chain(60)), + Err(DecodeError::RecursionLimitExceeded) + ); + assert!(::decode_from_slice(&cold_hot_chain(40)).is_ok()); + for limit in [1, 2, 3, 10] { + let wire = cold_hot_chain(6); + assert_eq!( + buffa::DecodeOptions::new() + .with_recursion_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()), + buffa::DecodeOptions::new() + .with_recursion_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()), + "limit {limit}" + ); + } +} + +#[test] +fn the_element_memory_limit_covers_repeated_children_without_a_table() { + // 1000 empty elements of `Cold.hots` (2), which is unrolled in `brt`, then of + // `Cold.leaves` (4), which is a table. + for tag in [0x12, 0x22] { + let wire: Vec = (0..1000).flat_map(|_| [tag, 0x00]).collect(); + for limit in [100, 10_000, 100_000_000] { + let decode_u = buffa::DecodeOptions::new() + .with_element_memory_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()); + let decode_t = buffa::DecodeOptions::new() + .with_element_memory_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()); + assert_eq!(decode_u, decode_t, "tag {tag:#x}, limit {limit}"); + } + } + let wire: Vec = (0..1000).flat_map(|_| [0x12, 0x00]).collect(); + assert_eq!( + buffa::DecodeOptions::new() + .with_element_memory_limit(100) + .decode_from_slice::(&wire) + .map(|_| ()), + Err(DecodeError::ElementMemoryLimitExceeded) + ); +} + +#[test] +fn a_child_split_across_occurrences_merges_alike() { + // `Cold.hot` (1) twice: {a = 1}, then {leaf = {x = 5}}; and `Cold.leaf` (3). + let wire = [ + 0x0a, 0x02, 0x08, 0x01, // hot { a = 1 } + 0x0a, 0x04, 0x12, 0x02, 0x08, 0x05, // hot { leaf { x = 5 } } + 0x1a, 0x02, 0x08, 0x02, // leaf { x = 2 } + 0x1a, 0x02, 0x12, 0x00, // leaf { s = "" } + ]; + assert_same_decode::(&wire, false); + let merged = ::decode_from_slice(&wire).unwrap(); + let hot = merged.hot.as_option().unwrap(); + assert_eq!((hot.a, hot.leaf.as_option().unwrap().x), (1, 5)); + assert_eq!(merged.leaf.as_option().unwrap().x, 2); +} + +#[test] +fn unknown_fields_in_a_child_without_a_table_are_kept() { + // `Cold.hot` holds `{a = 1, }`. + let wire = [0x0a, 0x05, 0x08, 0x01, 0xa0, 0x06, 0x03]; + assert_same_decode::(&wire, false); + let decoded = ::decode_from_slice(&wire).unwrap(); + assert_eq!( + decoded + .hot + .as_option() + .unwrap() + .__buffa_unknown_fields + .len(), + 1 + ); + assert_eq!(decoded.encode_to_vec(), wire); +} + +#[test] +fn a_child_without_a_table_is_encoded_into_every_kind_of_sink() { + let mut cold = brt_s::cold(); + // Enough bytes in a child that has no table to fill several segments. + cold.hots[1].blob = vec![0xab; 64 * 1024]; + let expected = cold.encode_to_vec(); + assert_eq!(expected.len() as u32, cold.encoded_len()); + let mut unrolled = bru_s::cold(); + unrolled.hots[1].blob = vec![0xab; 64 * 1024]; + assert_eq!(unrolled.encode_to_vec(), expected); + + let mut rope = buffa::Rope::new(); + cold.encode(&mut rope); + assert_eq!(&rope.to_contiguous_bytes()[..], &expected[..]); + let mut bytes_mut = buffa::bytes::BytesMut::new(); + cold.encode(&mut bytes_mut); + assert_eq!(&bytes_mut[..], &expected[..]); + // Room for only a few bytes at a time, so the message is staged. + let mut chunked = buffa::bytes::BytesMut::with_capacity(1); + cold.encode_length_delimited(&mut chunked); + let mut framed = Vec::new(); + buffa::encoding::encode_varint(expected.len() as u64, &mut framed); + framed.extend_from_slice(&expected); + assert_eq!(&chunked[..], &framed[..]); +} + +#[test] +fn a_holder_of_a_bytes_typed_message_is_unrolled_and_decodes_without_copying() { + use crate::tbz::{Blob, HoldsBlob, HoldsBlobs}; + use buffa::bytes::Bytes; + use buffa::MessageField; + // The plan ran on this schema, so `Plain` has a table, and `HoldsBlob` + // and `HoldsBlobs` do not, or their `Bytes` fields would be copied. + let generated = include_str!(concat!(env!("OUT_DIR"), "/table_bytes.rs")); + assert!(generated.contains("static __BUFFA_TABLE_Plain")); + assert!(generated.contains("static __BUFFA_TABLE_HoldsPlain")); + assert!(!generated.contains("__BUFFA_TABLE_Blob")); + assert!(!generated.contains("__BUFFA_TABLE_HoldsBlob")); + + let blob = |fill: u8| Blob { + data: Bytes::from(vec![fill; 64]), + chunks: vec![Bytes::from(vec![fill + 1; 64])], + ..Default::default() + }; + let msg = HoldsBlobs { + inner: MessageField::some(HoldsBlob { + blob: MessageField::some(blob(1)), + blobs: vec![blob(3), blob(5)], + tail: 9, + ..Default::default() + }), + ..Default::default() + }; + let src = Bytes::from(msg.encode_to_vec()); + let range = src.as_ptr() as usize..src.as_ptr() as usize + src.len(); + let aliases = |b: &Bytes| range.contains(&(b.as_ptr() as usize)); + + let decoded = HoldsBlobs::decode(&mut src.clone()).unwrap(); + assert_eq!(decoded, msg); + let inner = decoded.inner.as_option().unwrap(); + let held = inner.blob.as_option().unwrap(); + assert!(aliases(&held.data) && aliases(&held.chunks[0])); + assert!(inner + .blobs + .iter() + .all(|b| aliases(&b.data) && aliases(&b.chunks[0]))); } diff --git a/buffa/src/table/bridge.rs b/buffa/src/table/bridge.rs new file mode 100644 index 00000000..15265f0a --- /dev/null +++ b/buffa/src/table/bridge.rs @@ -0,0 +1,223 @@ +//! Message-typed fields whose message is not a table message. +//! +//! A table records how to size, write and decode the child of each message +//! field. A child that has a table is reached through it. Any other child (an +//! unrolled message, one generated by another crate, or a well-known type) is +//! reached through its [`Message`] impl by the function pointers of a +//! [`DynVt`], one per operation, compiled once per child type. It keeps its +//! own codec, so it writes the same bytes and accepts the same input as it +//! does inside an unrolled parent. +//! +//! A pointer cannot be generic over the write sink, so the write function +//! takes the [`PreSized`] cursor, which `Message::encode` and its siblings +//! write every `BufMut` through. Any other sink (a [`Rope`](crate::Rope), a +//! sink defined outside this crate, or a `BufMut` passed straight to +//! `Message::write_to`) gets the child from a scratch `Vec` allocated for it, +//! once per element of a repeated field, and the scratch is copied to the +//! sink. A `Rope` therefore copies the child a second time into its tail, and +//! cannot share the `bytes` fields inside it by reference count. +//! +//! Decoding reads a child from the slice of the nearest enclosing table +//! message, so the read of a child that overruns is bounded there and not by +//! the whole input. Every `bytes::Bytes` field of the child is copied out of +//! that slice, where unrolled code decoding from a `Bytes` shares it with the +//! input. Code generation keeps the holders of such a message of its own run +//! unrolled, but it cannot see into another crate, so a child from another +//! crate that has a `Bytes` field, such as `google.protobuf.Any`, is copied. + +use core::marker::PhantomData; + +use super::{decode, encode, size, MessageTable}; +use crate::encode_sink::{write_to_new_vec, PreSized}; +use crate::encoding::encode_varint; +use crate::{DecodeContext, DecodeError, EncodeSink, Message, SizeCache}; + +/// The operations of one child message type, reached through its [`Message`] +/// impl. Built by [`MsgVt::new_via_message`](super::MsgVt::new_via_message) and +/// [`RepVt::new_via_message`](super::RepVt::new_via_message). +pub(super) struct DynVt { + /// The encoded size of the child, recording nested sizes in the cache. + /// + /// # Safety + /// + /// The argument points to a live message of the type the vtable was built + /// for. + size: unsafe fn(*const u8, &mut SizeCache) -> u32, + /// Write the child through a cursor, consuming sizes from the cache. + /// + /// # Safety + /// + /// As for `size`. + write: unsafe fn(*const u8, &mut SizeCache, &mut PreSized<'_>), + /// Merge a length-prefixed encoding of the child from the front of the + /// buffer, as generated code does for a message field. + /// + /// # Safety + /// + /// As for `size`, and the argument is exclusive. + merge: unsafe fn(*mut u8, &mut &[u8], DecodeContext<'_>) -> Result<(), DecodeError>, +} + +/// # Safety +/// +/// `msg` points to a live `T`. +unsafe fn size_thunk(msg: *const u8, cache: &mut SizeCache) -> u32 { + // SAFETY: the caller passes a pointer to a live `T`. + unsafe { (*msg.cast::()).compute_size(cache) } +} + +/// # Safety +/// +/// `msg` points to a live `T`. +unsafe fn write_thunk(msg: *const u8, cache: &mut SizeCache, buf: &mut PreSized<'_>) { + // SAFETY: the caller passes a pointer to a live `T`. + unsafe { (*msg.cast::()).write_to(cache, buf) } +} + +/// # Safety +/// +/// `msg` points to a live `T` that nothing else accesses. +unsafe fn merge_thunk( + msg: *mut u8, + buf: &mut &[u8], + ctx: DecodeContext<'_>, +) -> Result<(), DecodeError> { + // SAFETY: the caller passes an exclusive pointer to a live `T`. + unsafe { (*msg.cast::()).merge_length_delimited(buf, ctx) } +} + +/// Holds the `'static` [`DynVt`] of `T`. +struct Thunks(PhantomData T>); + +impl Thunks { + const VT: &'static DynVt = &DynVt { + size: size_thunk::, + write: write_thunk::, + merge: merge_thunk::, + }; +} + +/// How to reach the message of a message field: through its table, or through +/// its [`Message`] impl. +#[derive(Clone, Copy)] +pub(super) enum Child { + Table(&'static MessageTable), + Dyn(&'static DynVt), +} + +impl Child { + pub(super) const fn of() -> Self { + Self::Dyn(Thunks::::VT) + } + + /// The encoded size of the message at `base`, recording nested sizes in + /// `cache`. + /// + /// # Safety + /// + /// `base` points to a live message of the type this was built for. + #[inline] + pub(super) unsafe fn compute_size(self, base: *const u8, cache: &mut SizeCache) -> u32 { + // SAFETY: forwarded from the caller. + unsafe { + match self { + Self::Table(table) => size::compute_size(table, base, cache), + Self::Dyn(vt) => (vt.size)(base, cache), + } + } + } + + /// Write the message at `base`, whose encoded size is `len`, to `buf`. + /// + /// # Safety + /// + /// `base` points to a live message of the type this was built for. + #[inline] + pub(super) unsafe fn write_to( + self, + base: *const u8, + len: u32, + cache: &mut SizeCache, + buf: &mut K, + ) { + // SAFETY: forwarded from the caller. + unsafe { + match self { + Self::Table(table) => encode::write_message(table, base, cache, buf), + Self::Dyn(vt) => write_dyn(vt, base, len, cache, buf), + } + } + } + + /// Decode a length-prefixed message from the front of `buf` into the + /// message at `base`. + /// + /// # Safety + /// + /// `base` points to a live message of the type this was built for, which + /// nothing else accesses. + #[inline] + pub(super) unsafe fn merge_sub( + self, + base: *mut u8, + buf: &mut &[u8], + ctx: DecodeContext<'_>, + ) -> Result<(), DecodeError> { + // SAFETY: forwarded from the caller. + unsafe { + match self { + Self::Table(table) => decode::merge_sub(table, base, buf, ctx), + Self::Dyn(vt) => (vt.merge)(base, buf, ctx), + } + } + } +} + +/// Write the message at `base`, of encoded size `len`, to `buf` through its +/// [`DynVt`]. +/// +/// # Safety +/// +/// `base` points to a live message of the type `vt` was built for. +#[inline(never)] +unsafe fn write_dyn( + vt: &DynVt, + base: *const u8, + len: u32, + cache: &mut SizeCache, + buf: &mut K, +) { + let mut through_cursor = |cursor: &mut PreSized<'_>| { + // SAFETY: forwarded from the caller. + unsafe { (vt.write)(base, &mut *cache, cursor) } + }; + if buf.__with_pre_sized(&mut through_cursor) { + return; + } + let len = len as usize; + let scratch = write_to_new_vec(len, |cursor| { + // SAFETY: forwarded from the caller. + unsafe { (vt.write)(base, &mut *cache, cursor) } + }); + crate::message::debug_assert_two_pass(scratch.len(), len); + buf.put_slice(&scratch); +} + +/// Write `len` and then the message at `base` to `buf`, as the value of a +/// length-delimited field whose tag has been written. +/// +/// # Safety +/// +/// `base` points to a live message of the type `child` was built for. +#[inline] +pub(super) unsafe fn write_field_value( + child: Child, + base: *const u8, + cache: &mut SizeCache, + buf: &mut K, +) { + let len = cache.consume_next(); + encode_varint(u64::from(len), buf); + // SAFETY: forwarded from the caller. + unsafe { child.write_to(base, len, cache, buf) }; +} diff --git a/buffa/src/table/decode.rs b/buffa/src/table/decode.rs index cce1bc19..1054ad01 100644 --- a/buffa/src/table/decode.rs +++ b/buffa/src/table/decode.rs @@ -121,7 +121,7 @@ unsafe fn merge_slice( /// # Safety /// /// `base` points to a live message of the type `table` describes. -unsafe fn merge_sub( +pub(super) unsafe fn merge_sub( table: &MessageTable, base: *mut u8, buf: &mut &[u8], @@ -351,7 +351,7 @@ unsafe fn merge_msg( let vt = table.rep_vt(e); ctx.register_element_memory(vt.size)?; let elem = (vt.push)(slot); - let decoded = merge_sub(vt.table, elem, buf, ctx); + let decoded = vt.child.merge_sub(elem, buf, ctx); if decoded.is_err() { // Like unrolled code, which decodes into a local and pushes // it only on success, leave no partial element behind. @@ -361,7 +361,7 @@ unsafe fn merge_msg( } else { let vt = table.msg_vt(e); let child = (vt.place)(slot); - merge_sub(vt.table, child, buf, ctx) + vt.child.merge_sub(child, buf, ctx) } } } diff --git a/buffa/src/table/encode.rs b/buffa/src/table/encode.rs index aeb8ef42..fda7f6af 100644 --- a/buffa/src/table/encode.rs +++ b/buffa/src/table/encode.rs @@ -1,5 +1,6 @@ //! The write pass: [`write_to`] and its per-kind arms. +use super::bridge::write_field_value; use super::scalar::Sc; use super::{ Bool, Double, Entry, Fixed32, Fixed64, Float, Int32, Int64, Kind, MessageTable, Sfixed32, @@ -55,7 +56,7 @@ unsafe fn write_pre_sized( /// # Safety /// /// As for [`write_to`]. -unsafe fn write_message( +pub(super) unsafe fn write_message( table: &MessageTable, base: *const u8, cache: &mut SizeCache, @@ -314,16 +315,14 @@ unsafe fn write_msg( let (ptr, len) = (vt.parts)(slot); for i in 0..len { put_tag(e, buf); - encode_varint(u64::from(cache.consume_next()), buf); - write_message(vt.table, ptr.add(i * vt.size), cache, buf); + write_field_value(vt.child, ptr.add(i * vt.size), cache, buf); } } else { let vt = table.msg_vt(e); let child = (vt.get)(slot); if !child.is_null() { put_tag(e, buf); - encode_varint(u64::from(cache.consume_next()), buf); - write_message(vt.table, child, cache, buf); + write_field_value(vt.child, child, cache, buf); } } } diff --git a/buffa/src/table/mod.rs b/buffa/src/table/mod.rs index 1fdd784c..e8bca0cf 100644 --- a/buffa/src/table/mod.rs +++ b/buffa/src/table/mod.rs @@ -32,7 +32,11 @@ //! gathered into a buffer first, and a field that declares a length past //! the end of its enclosing message fails at once with //! [`DecodeError::UnexpectedEof`], where unrolled code reads on into the -//! enclosing message and can report a different error. +//! enclosing message and can report a different error. A child reached +//! through its [`Message`](crate::Message) impl is read from the slice of +//! the nearest enclosing table message, so a read that overruns it fails +//! at that message's end, and not where a tree of unrolled messages would +//! notice. //! - [`Table::merge_field`] decodes one field and cannot gather, so it //! returns [`DecodeError::UnexpectedEof`] for a buffer that is not one //! chunk. Only a caller that drives `merge_field` itself is affected, such @@ -40,6 +44,13 @@ //! group field must not use the table strategy. //! - [`clear`](crate::Message::clear) resets to `Default`, which releases //! allocations that unrolled code keeps. +//! - A child reached through its [`Message`](crate::Message) impl (see +//! [`MsgVt::new_via_message`]) has the same wire format and accepts the same +//! input, but it is staged in a scratch buffer and copied when it is written +//! to any sink other than the cursor that `Message::encode` and its siblings +//! write a [`BufMut`](crate::bytes::BufMut) through, and its `bytes::Bytes` +//! fields are copied out of the slice it is decoded from, where unrolled +//! code decoding from a `Bytes` shares them with the input. //! //! # Where the code is compiled //! @@ -47,10 +58,10 @@ //! [`BufMut`](crate::bytes::BufMut) through `Message::encode` and its //! siblings are non-generic functions compiled in this crate, at this crate's //! optimisation level, once for all messages. A build can therefore optimise -//! this crate for speed and its own generated code for size. Encoding into a -//! sink that is not a `BufMut`, such as [`Rope`](crate::Rope), and the -//! generic wrappers around decoding are instantiated in the crate that calls -//! them. +//! this crate for speed and its own generated code for size. Encoding into +//! any other sink, such as a [`Rope`](crate::Rope) or a `BufMut` passed +//! straight to `Message::write_to`, and the generic wrappers around +//! decoding are instantiated in the crate that calls them. use core::marker::PhantomData; @@ -298,6 +309,7 @@ pub trait KindSlot { kind_table!(define_kind); // After the macros above, whose textual scope covers only what follows them. +mod bridge; mod decode; mod encode; mod scalar; @@ -479,8 +491,7 @@ impl Table { /// `Bytes*`: `Vec`, `Option>` or `Vec>`; /// - `Enum*`: the storage the entry's [`EnumVt`] was built for; /// - `MsgSingular`: the storage the [`MsgVt`] was built for, and - /// `MsgRepeated`: a `Vec` of the messages the [`RepVt`]'s table - /// describes. + /// `MsgRepeated`: the `Vec` the [`RepVt`] was built for. /// /// `unknown`, if present, must be the offset of a field of type /// `UnknownFields`. The `__table_entry!` macro checks the field types diff --git a/buffa/src/table/shape.rs b/buffa/src/table/shape.rs index 214d47ba..903bf20c 100644 --- a/buffa/src/table/shape.rs +++ b/buffa/src/table/shape.rs @@ -4,9 +4,10 @@ use core::marker::PhantomData; +use super::bridge::Child; use super::{Table, IMPLICIT, OPTIONAL, REPEATED}; use crate::alloc::vec::Vec; -use crate::{EnumValue, Enumeration, MessageField, ProtoBox}; +use crate::{EnumValue, Enumeration, Message, MessageField, ProtoBox}; // --------------------------------------------------------------------------- // Singular message fields @@ -36,10 +37,10 @@ impl> MsgSlot for MessageField { } } -/// Descriptor of a singular message field: the child's table and how to reach -/// the child through the field's storage. +/// Descriptor of a singular message field: how to encode, size and decode the +/// child and how to reach it through the field's storage. pub struct MsgVt { - pub(super) table: &'static super::MessageTable, + pub(super) child: Child, /// The message in the field, created with its default if unset. /// /// # Safety @@ -74,11 +75,30 @@ unsafe fn get_impl(slot: *const u8) -> *const u8 { } impl MsgVt { - /// Describe a field of type `F`, whose messages `table` describes. + /// Describe a field of type `F`, whose message is a table message and + /// `table` is its table. #[must_use] pub const fn new(table: &'static Table) -> Self { Self { - table: &table.raw, + child: Child::Table(&table.raw), + place: place_impl::, + get: get_impl::, + } + } + + /// Describe a field of type `F` whose message is reached through its + /// [`Message`] impl, for a message whose table is not visible here. + /// Prefer [`MsgVt::new`] when it is, because the interpreters then decode + /// the child without a function call. The child is decoded from a slice, + /// so its `bytes::Bytes` fields are copied, where unrolled code decoding + /// from a `Bytes` shares them. + #[must_use] + pub const fn new_via_message() -> Self + where + F::Msg: Message, + { + Self { + child: Child::of::(), place: place_impl::, get: get_impl::, } @@ -91,7 +111,7 @@ impl MsgVt { /// Descriptor of a repeated message field, a `Vec`. pub struct RepVt { - pub(super) table: &'static super::MessageTable, + pub(super) child: Child, /// The size in bytes of one element. pub(super) size: usize, /// Append a default element and return a pointer to it. @@ -144,11 +164,26 @@ unsafe fn parts_impl(slot: *const u8) -> (*const u8, usize) { } impl RepVt { - /// Describe a `Vec` field, whose messages `table` describes. + /// Describe a `Vec` field whose messages are table messages, `table` + /// being their table. #[must_use] pub const fn new(table: &'static Table) -> Self { Self { - table: &table.raw, + child: Child::Table(&table.raw), + size: core::mem::size_of::(), + push: push_impl::, + pop: pop_impl::, + parts: parts_impl::, + } + } + + /// Describe a `Vec` field whose messages are reached through their + /// [`Message`] impl. Like [`MsgVt::new_via_message`], it copies the + /// `bytes::Bytes` fields of the elements when it decodes. + #[must_use] + pub const fn new_via_message() -> Self { + Self { + child: Child::of::(), size: core::mem::size_of::(), push: push_impl::, pop: pop_impl::, diff --git a/buffa/src/table/size.rs b/buffa/src/table/size.rs index e2430f32..f983cfb8 100644 --- a/buffa/src/table/size.rs +++ b/buffa/src/table/size.rs @@ -233,7 +233,7 @@ unsafe fn size_msg( let mut size = 0; for i in 0..len { let idx = cache.reserve(); - let inner = compute_size(vt.table, ptr.add(i * vt.size), cache); + let inner = vt.child.compute_size(ptr.add(i * vt.size), cache); cache.set(idx, inner); size += tl + varint_len(u64::from(inner)) as u64 + u64::from(inner); } @@ -245,7 +245,7 @@ unsafe fn size_msg( return 0; } let idx = cache.reserve(); - let inner = compute_size(vt.table, child, cache); + let inner = vt.child.compute_size(child, cache); cache.set(idx, inner); tl + varint_len(u64::from(inner)) as u64 + u64::from(inner) } diff --git a/buffa/src/table/tests.rs b/buffa/src/table/tests.rs index 05346705..dee82f4a 100644 --- a/buffa/src/table/tests.rs +++ b/buffa/src/table/tests.rs @@ -3,9 +3,10 @@ use super::*; use crate::alloc::{string::String, vec, vec::Vec}; use crate::bytes::Buf; +use crate::encoding::WireType; use crate::{ - DecodeOptions, EnumValue, Enumeration, Inline, Message, MessageField, Rope, UnknownFieldData, - UnknownFields, + types, DecodeOptions, EnumValue, Enumeration, Inline, Message, MessageField, Rope, + UnknownFieldData, UnknownFields, }; // --------------------------------------------------------------------------- @@ -1001,3 +1002,513 @@ fn writing_through_a_nested_cursor_continues_after_the_bytes_already_written() { msg.encode(&mut rope); assert_eq!(&rope.to_contiguous_bytes()[..], &expected[..]); } + +// --------------------------------------------------------------------------- +// Children without a table +// --------------------------------------------------------------------------- + +/// `int32 n = 1; string s = 2; Hand next = 3; Inner inner = 4;`, written by +/// hand as the unrolled codec writes a message, so that it has no table. +#[derive(Clone, Debug, Default, PartialEq)] +struct Hand { + n: i32, + s: String, + next: MessageField, + inner: MessageField, + unknown: UnknownFields, +} + +crate::impl_default_instance!(Hand); + +impl Message for Hand { + fn compute_size(&self, cache: &mut SizeCache) -> u32 { + let mut size = 0u64; + if self.n != 0 { + size += 1 + types::int32_encoded_len(self.n) as u64; + } + if !self.s.is_empty() { + size += 1 + types::string_encoded_len(&self.s) as u64; + } + if let Some(next) = self.next.as_option() { + let slot = cache.reserve(); + let inner = next.compute_size(cache); + cache.set(slot, inner); + size += 1 + crate::encoding::varint_len(u64::from(inner)) as u64 + u64::from(inner); + } + if let Some(inner) = self.inner.as_option() { + let slot = cache.reserve(); + let len = inner.compute_size(cache); + cache.set(slot, len); + size += 1 + crate::encoding::varint_len(u64::from(len)) as u64 + u64::from(len); + } + size += self.unknown.encoded_len() as u64; + crate::saturate_size(size) + } + + fn write_to(&self, cache: &mut SizeCache, buf: &mut impl EncodeSink) { + if self.n != 0 { + types::put_int32_field(1, self.n, buf); + } + if !self.s.is_empty() { + types::put_string_field(2, &self.s, buf); + } + if let Some(next) = self.next.as_option() { + buf.put_u8(0x1a); + crate::encoding::encode_varint(u64::from(cache.consume_next()), buf); + next.write_to(cache, buf); + } + if let Some(inner) = self.inner.as_option() { + buf.put_u8(0x22); + crate::encoding::encode_varint(u64::from(cache.consume_next()), buf); + inner.write_to(cache, buf); + } + self.unknown.write_to(buf); + } + + fn merge_field( + &mut self, + tag: Tag, + buf: &mut impl Buf, + ctx: DecodeContext<'_>, + ) -> Result<(), DecodeError> { + match tag.field_number() { + 1 => { + crate::encoding::check_wire_type(tag, WireType::Varint)?; + self.n = types::decode_int32(buf)?; + } + 2 => { + crate::encoding::check_wire_type(tag, WireType::LengthDelimited)?; + self.s = types::decode_string(buf)?; + } + 3 => { + crate::encoding::check_wire_type(tag, WireType::LengthDelimited)?; + self.next + .get_or_insert_default() + .merge_length_delimited(buf, ctx)?; + } + 4 => { + crate::encoding::check_wire_type(tag, WireType::LengthDelimited)?; + self.inner + .get_or_insert_default() + .merge_length_delimited(buf, ctx)?; + } + _ => self + .unknown + .push(crate::encoding::decode_unknown_field(tag, buf, ctx)?), + } + Ok(()) + } + + fn clear(&mut self) { + *self = Self::default(); + } +} + +/// A table message whose children are reached through `Message`: a hand-written +/// message and a table message, singular, repeated and inline, next to one +/// child reached through its table. +#[derive(Clone, Debug, Default, PartialEq)] +struct Bridged { + hand: MessageField, + hands: Vec, + tabled: MessageField, + tabled_list: Vec, + direct: MessageField, + hand_inline: MessageField>, + tail: i32, + unknown: UnknownFields, +} + +static BRIDGED: Table = crate::__table!( + Bridged, + abi = ABI, + entries = [ + crate::__table_entry!( + Bridged, + hand, + MsgSingular, + 1, + aux = 0, + slot = MessageField + ), + crate::__table_entry!(Bridged, hands, MsgRepeated, 2, aux = 1, slot = Vec), + crate::__table_entry!( + Bridged, + tabled, + MsgSingular, + 3, + aux = 2, + slot = MessageField + ), + crate::__table_entry!( + Bridged, + tabled_list, + MsgRepeated, + 4, + aux = 3, + slot = Vec + ), + crate::__table_entry!( + Bridged, + direct, + MsgSingular, + 5, + aux = 4, + slot = MessageField + ), + crate::__table_entry!( + Bridged, + hand_inline, + MsgSingular, + 6, + aux = 5, + slot = MessageField> + ), + crate::__table_entry!(Bridged, tail, Int32Implicit, 7), + ], + dense = &dense::<8>(&[1, 2, 3, 4, 5, 6, 7]), + aux = [ + Aux::Msg(&MsgVt::new_via_message::>()), + Aux::Rep(&RepVt::new_via_message::()), + Aux::Msg(&MsgVt::new_via_message::>()), + Aux::Rep(&RepVt::new_via_message::()), + Aux::Msg(&MsgVt::new::>(&INNER)), + Aux::Msg(&MsgVt::new_via_message::>>()), + ], + unknown = unknown, +); + +table_message!(Bridged, BRIDGED); + +fn hand(n: i32, s: &str) -> Hand { + Hand { + n, + s: s.into(), + ..Hand::default() + } +} + +fn bridged() -> Bridged { + let mut deep = hand(1, "deep"); + deep.next = MessageField::some(hand(2, "")); + deep.inner = MessageField::some(Inner { + id: 3, + label: "tabled inside".into(), + next: MessageField::some(Inner { + id: 4, + ..Inner::default() + }), + ..Inner::default() + }); + let mut with_unknown = Inner { + id: 9, + ..Inner::default() + }; + with_unknown + .unknown + .push(crate::UnknownField { + number: 900, + data: UnknownFieldData::Varint(5), + }); + Bridged { + hand: MessageField::some(deep), + hands: vec![hand(7, "a"), Hand::default(), hand(0, "c")], + tabled: MessageField::some(with_unknown), + tabled_list: vec![ + Inner { + id: 1, + ..Inner::default() + }, + Inner::default(), + ], + direct: MessageField::some(Inner { + label: "direct".into(), + ..Inner::default() + }), + hand_inline: MessageField::some(hand(-1, "inline")), + tail: 70, + unknown: UnknownFields::new(), + } +} + +#[test] +fn a_child_without_a_table_encodes_the_bytes_a_table_child_does() { + let child = Inner { + id: 5, + label: "same".into(), + ..Inner::default() + }; + let through_message = Bridged { + tabled: MessageField::some(child.clone()), + ..Bridged::default() + } + .encode_to_vec(); + let through_table = Bridged { + direct: MessageField::some(child.clone()), + ..Bridged::default() + } + .encode_to_vec(); + // Field 3 against field 5: only the tag differs. + assert_eq!(through_message[0], 0x1a); + assert_eq!(through_table[0], 0x2a); + assert_eq!(through_message[1..], through_table[1..]); + + let wire = Bridged { + hand: MessageField::some(hand(1, "a")), + hands: vec![hand(2, "")], + tail: 3, + ..Bridged::default() + } + .encode_to_vec(); + let expected: &[u8] = &[ + 0x0a, 0x05, 0x08, 0x01, 0x12, 0x01, b'a', // 1: {n: 1, s: "a"} + 0x12, 0x02, 0x08, 0x02, // 2: {n: 2} + 0x38, 0x03, // 7: 3 + ]; + assert_eq!(wire, expected); +} + +#[test] +fn children_without_a_table_round_trip() { + let msg = bridged(); + let bytes = msg.encode_to_vec(); + assert_eq!(bytes.len() as u32, msg.encoded_len()); + assert_eq!(Bridged::decode_from_slice(&bytes).unwrap(), msg); + assert_eq!( + Bridged::decode_from_slice(&Bridged::default().encode_to_vec()).unwrap(), + Bridged::default() + ); +} + +#[test] +fn a_child_without_a_table_reaches_every_sink() { + let msg = bridged(); + let expected = msg.encode_to_vec(); + + let mut rope = Rope::new(); + msg.encode(&mut rope); + assert_eq!(&rope.to_contiguous_bytes()[..], &expected[..]); + + let mut bytes_mut = crate::bytes::BytesMut::new(); + msg.encode(&mut bytes_mut); + assert_eq!(&bytes_mut[..], &expected[..]); + + let mut roomy = Vec::with_capacity(expected.len()); + msg.encode(&mut roomy); + assert_eq!(roomy, expected); + + // A sink whose chunk is shorter than the message receives the whole + // message from a scratch buffer. + let mut small = crate::bytes::BytesMut::with_capacity(1); + msg.encode_length_delimited(&mut small); + let mut framed = Vec::new(); + crate::encoding::encode_varint(expected.len() as u64, &mut framed); + framed.extend_from_slice(&expected); + assert_eq!(&small[..], &framed[..]); +} + +#[test] +fn children_without_a_table_decode_from_a_non_contiguous_buffer() { + let msg = bridged(); + let bytes = msg.encode_to_vec(); + for split in 1..bytes.len() { + let (head, tail) = bytes.split_at(split); + let mut chained = head.chain(tail); + let mut decoded = Bridged::default(); + with_ctx(|ctx| decoded.merge(&mut chained, ctx)).unwrap(); + assert_eq!(decoded, msg, "split at {split}"); + } +} + +#[test] +fn a_singular_child_without_a_table_merges() { + let mut msg = Bridged::default(); + for wire in [ + // hand {n = 1, s = "x"}, then hand {s = "y", next = {n = 2}}. + &[0x0a, 0x05, 0x08, 0x01, 0x12, 0x01, b'x'][..], + &[0x0a, 0x07, 0x12, 0x01, b'y', 0x1a, 0x02, 0x08, 0x02][..], + ] { + with_ctx(|ctx| msg.merge(&mut &wire[..], ctx)).unwrap(); + } + let hand = msg.hand.as_option().unwrap(); + assert_eq!((hand.n, hand.s.as_str()), (1, "y")); + assert_eq!(hand.next.as_option().unwrap().n, 2); +} + +#[test] +fn unknown_fields_in_a_child_without_a_table_are_kept() { + let msg = bridged(); + let decoded = Bridged::decode_from_slice(&msg.encode_to_vec()).unwrap(); + let tabled = decoded.tabled.as_option().unwrap(); + assert_eq!(tabled.unknown.iter().count(), 1); +} + +#[test] +fn the_recursion_limit_applies_through_a_child_without_a_table() { + let wrap = |field: u8, inner: Vec| { + let mut wire = vec![field]; + crate::encoding::encode_varint(inner.len() as u64, &mut wire); + wire.extend(inner); + wire + }; + // The child is a hand-written message, so its own decode counts the depth. + assert!(Bridged::decode_from_slice(&wrap(0x0a, nested(50))).is_ok()); + assert!(matches!( + Bridged::decode_from_slice(&wrap(0x0a, nested(150))), + Err(DecodeError::RecursionLimitExceeded) + )); + // And a table child reached through `Message` counts it too. + assert!(Bridged::decode_from_slice(&wrap(0x1a, nested(50))).is_ok()); + assert!(matches!( + Bridged::decode_from_slice(&wrap(0x1a, nested(150))), + Err(DecodeError::RecursionLimitExceeded) + )); + assert!(matches!( + DecodeOptions::new() + .with_recursion_limit(10) + .decode_from_slice::(&wrap(0x0a, nested(50))), + Err(DecodeError::RecursionLimitExceeded) + )); +} + +#[test] +fn the_element_memory_limit_applies_to_repeated_children_without_a_table() { + // 1000 empty elements of `hands` (2), then of `tabled_list` (4). + let hands: Vec = (0..1000).flat_map(|_| [0x12, 0x00]).collect(); + let tabled: Vec = (0..1000).flat_map(|_| [0x22, 0x00]).collect(); + for wire in [&hands, &tabled] { + assert!(Bridged::decode_from_slice(wire).is_ok()); + assert!(matches!( + DecodeOptions::new() + .with_element_memory_limit(100) + .decode_from_slice::(wire), + Err(DecodeError::ElementMemoryLimitExceeded) + )); + } +} + +#[test] +fn a_repeated_child_without_a_table_that_fails_to_decode_is_not_kept() { + // `hands`: one valid element, then one whose string is not UTF-8. + let wire = [0x12, 0x02, 0x08, 0x01, 0x12, 0x03, 0x12, 0x01, 0xff]; + let mut msg = Bridged::default(); + let result = with_ctx(|ctx| msg.merge(&mut &wire[..], ctx)); + assert_eq!(result, Err(DecodeError::InvalidUtf8)); + assert_eq!(msg.hands.len(), 1); + assert_eq!(msg.hands[0].n, 1); +} + +#[test] +fn a_truncated_child_without_a_table_is_an_error_or_ends_on_a_field_boundary() { + let bytes = bridged().encode_to_vec(); + let mut ok = 0; + for end in 0..bytes.len() { + match Bridged::decode_from_slice(&bytes[..end]) { + Ok(_) => ok += 1, + Err(e) => assert!( + matches!(e, DecodeError::UnexpectedEof | DecodeError::VarintTooLong), + "prefix {end}: {e}" + ), + } + } + assert!(ok > 1 && ok < bytes.len() / 2); + assert!(Bridged::decode_from_slice(&bytes).is_ok()); +} + +#[test] +fn a_length_that_runs_past_a_child_without_a_table_is_an_error() { + // `hand` is 2 bytes and its string declares 5. + assert_eq!( + Bridged::decode_from_slice(&[0x0a, 0x02, 0x12, 0x05, b'a', b'b', b'c']), + Err(DecodeError::UnexpectedEof) + ); + // A child longer than the size limit. + assert_eq!( + Bridged::decode_from_slice(&[0x0a, 0x80, 0x80, 0x80, 0x80, 0x08]), + Err(DecodeError::MessageTooLarge) + ); +} + +/// Declares `size` bytes and writes `writes` of them. +#[derive(Clone, Default, PartialEq)] +struct Liar { + size: u32, + writes: u8, +} +crate::impl_default_instance!(Liar); + +impl Message for Liar { + fn compute_size(&self, _: &mut SizeCache) -> u32 { + self.size + } + fn write_to(&self, _: &mut SizeCache, buf: &mut impl EncodeSink) { + for _ in 0..self.writes { + buf.put_u8(1); + } + } + fn merge_field( + &mut self, + _: Tag, + _: &mut impl Buf, + _: DecodeContext<'_>, + ) -> Result<(), DecodeError> { + unreachable!() + } + fn clear(&mut self) {} +} + +#[derive(Clone, Default, PartialEq)] +struct HoldsLiar { + liar: MessageField, +} + +static HOLDS_LIAR: Table = crate::__table!( + HoldsLiar, + abi = ABI, + entries = [crate::__table_entry!( + HoldsLiar, + liar, + MsgSingular, + 1, + aux = 0, + slot = MessageField + )], + dense = &dense::<2>(&[1]), + aux = [Aux::Msg(&MsgVt::new_via_message::>())], + unknown = none, +); +table_message!(HoldsLiar, HOLDS_LIAR); + +fn holds_liar(size: u32, writes: u8) -> HoldsLiar { + HoldsLiar { + liar: MessageField::some(Liar { size, writes }), + } +} + +#[test] +#[should_panic(expected = "more bytes than compute_size declared")] +fn a_child_that_writes_more_than_it_sized_panics_in_the_scratch_buffer() { + // A Rope is not written through the cursor, so the child is staged in a + // buffer of the size `compute_size` gave, which the write overruns. + holds_liar(0, 1).encode(&mut Rope::new()); +} + +#[test] +#[cfg(debug_assertions)] +#[should_panic(expected = "different byte count than compute_size declared")] +fn a_child_that_writes_less_than_it_sized_panics_in_debug_builds() { + holds_liar(3, 1).encode(&mut Rope::new()); +} + +#[test] +fn a_child_can_be_written_by_calling_write_to_on_a_buffer() { + // `write_to` on a `Vec` is not the pre-sized path `encode` takes, so each + // child is staged and copied. The bytes are the same. + let msg = bridged(); + let mut cache = SizeCache::new(); + let size = msg.compute_size(&mut cache); + let mut out = Vec::new(); + msg.write_to(&mut cache, &mut out); + assert_eq!(out.len(), size as usize); + assert_eq!(out, msg.encode_to_vec()); +} diff --git a/docs/guide.md b/docs/guide.md index b0117437..db95d543 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -216,7 +216,7 @@ The macro pulls in `OUT_DIR/.mod.rs`, which in turn includes the per | `.extern_path(proto, rust)` | — | Map a proto package or a single type to an external Rust path (see below) | | `.exclude_package(pkg)` | — | Drop a proto package (and its sub-packages) from code generation. Useful when directory globbing pulls in option-only packages (e.g. `buf.validate`) that you don't want Rust types for. A leading dot is accepted and stripped. Pair with `.extern_path` if kept files reference types from the excluded package; the generator emits a `cargo:warning` for each such cross-package reference. | | `.type_name_prefix(prefix)` | `""` | Prepend a PascalCase prefix (`[A-Z][A-Za-z0-9]*`; anything else is rejected at generation time) to every generated message/enum type name (`message User` → `struct RpcUser`); modules, oneof enums, extern-mapped types, and the wire format are unaffected. A crate referencing these types via `extern_path` must spell out the prefixed name (`::crate_a::RpcUser`) | -| `.codec_strategy(strategy)` | `Unrolled` | Generate each message's binary `Message` code specialised to its fields (`CodecStrategy::Unrolled`), or from a static table and interpreters shared by every message (`CodecStrategy::Table`), which on a schema it fully covers is about half the compiled size and slower on messages of many small fields; see [Smaller generated code](#smaller-generated-code-codec_strategy) | +| `.codec_strategy(strategy)` | `Unrolled` | Generate each message's binary `Message` code specialised to its fields (`CodecStrategy::Unrolled`), or from a static table and interpreters shared by every message (`CodecStrategy::Table`), which makes a large schema about 40% smaller and is slower on messages of many small fields; see [Smaller generated code](#smaller-generated-code-codec_strategy) | | `.codec_strategy_in(strategy, &[...])` | — | Choose the strategy for matching messages and the messages nested in them (proto-path prefixes; the last matching rule wins), on top of the global setting | | `.use_bytes_type()` | — | Use `bytes::Bytes` for all bytes fields, including `map` values | | `.use_bytes_type_in(&[...])` | — | Use `bytes::Bytes` for matching bytes fields (same `map` rule) | @@ -632,7 +632,7 @@ Passed via `opt:` (works for `remote:` and `local:`): | `idiomatic_enum_aliases=false` | Omit the `UpperCamelCase` associated-const aliases for enum values (`Status::Active`); the `SHOUTY_SNAKE_CASE` variants are unaffected (default: emitted). See [Enums](#enumvaluet--type-safe-open-enums) | | `override_feature_in==:` | Apply a path-scoped editions feature override (currently `enum_type:OPEN`) to the compiled descriptors. Repeatable | | `open_enums_in=` | Shorthand for `override_feature_in==enum_type:OPEN`. Repeatable | -| `codec_strategy=table` | Generate every message's binary `Message` code from a static table and shared interpreters instead of code specialised to its fields (default `unrolled`). The plugin cannot check the compiler version: on Rust before 1.77 the generated code fails to compile. See [Smaller generated code](#smaller-generated-code-codec_strategy) | +| `codec_strategy=table` | Generate each message's binary `Message` code from a static table and shared interpreters instead of code specialised to its fields, for the messages the table can handle (default `unrolled`). The plugin cannot check the compiler version: on Rust before 1.77 the generated code fails to compile. See [Smaller generated code](#smaller-generated-code-codec_strategy) | | `codec_strategy_in==` | Choose `table` or `unrolled` for matching messages and the messages nested in them. Repeatable; leading dot optional; the last matching rule wins | | `unbox_oneof=true` | Store every non-recursive message/group oneof variant inline instead of `Box`. Recursive variants stay boxed. | | `unbox_oneof_in=` | Store matching non-recursive message/group oneof variants inline instead of `Box`. Repeatable; leading dot optional. Use `.` to match all variants. Recursive variants stay boxed for broad matches; exact recursive matches are rejected. | @@ -1100,7 +1100,7 @@ Buffa uses a two-pass model to avoid the exponential-time size computation that ### Smaller generated code: `codec_strategy` -By default every generated message contains its own size, write, and merge code, specialised to its fields. `CodecStrategy::Table` replaces it with one static table per message and interpreters in `buffa` that every message shares. On a schema of 334 messages and 3,477 fields, the compiled size at `opt-level = "z"` went from 1,644 KB to 817 KB (measured with oneofs and maps flattened, which the table cannot handle; see [#463](https://github.com/anthropics/buffa/issues/463) for the method). The cost is speed on messages made of many small fields, where encoding takes up to about 3.5 times as long as with the default `CodecStrategy::Unrolled` and decoding up to 1.6 times. Messages dominated by bulk data, such as large strings, bytes, and packed arrays, show no difference. +By default every generated message contains its own size, write, and merge code, specialised to its fields. `CodecStrategy::Table` replaces it with one static table per message and interpreters in `buffa` that every message shares. On the WhatsApp schema (`whatsapp.proto` from `waproto`: 334 top-level messages, 752 with the nested ones, 3,477 fields), built with `Box` message fields, no unknown-field preservation, fat LTO, and `panic = "abort"`, the text section of the binary at `opt-level = "z"` went from 1,670 KB to 1,010 KB (−40%). 43 of its 752 messages (23 of 334 top-level) stay unrolled because they have a `oneof` or a `map`. [#463](https://github.com/anthropics/buffa/issues/463) describes the method. The cost is speed on messages made of many small fields, where encoding takes up to about 3.5 times as long as with the default `CodecStrategy::Unrolled` and decoding up to 1.6 times. Messages dominated by bulk data, such as large strings, bytes, and packed arrays, show no difference. ```rust,ignore // build.rs @@ -1113,13 +1113,15 @@ buffa_build::Config::new() .compile()?; ``` -A table holds only table messages, so a message that holds an unrolled one is unrolled too, and a `codec_strategy_in` rule for a message does not select the messages it holds: with the global setting left at `Unrolled`, select a message and everything it holds. In the example, every message that contains `.wa.Message` stays unrolled, and that usually includes the root message an application encodes; codegen does not warn about a fallback that follows from your own `Unrolled` rule. +Apart from the holders listed below, a table message may hold any message. It reaches a child that has a table through it, and any other child (a message you set to `Unrolled`, one generated by another crate, or a well-known type such as `Timestamp`) through its `Message` impl, which costs a function call per child. A `codec_strategy_in` rule selects the message it names and the messages nested in it, and does not extend to the messages it holds. In the example, `.wa.Message` and `.wa.Receipt` stay specialised and the messages that hold them use the table. Setting a message to `Unrolled` therefore does not keep the messages that hold it unrolled: to keep a whole path specialised, set its holders to `Unrolled` too. -The option changes only the binary `Message` implementation. The wire format and the JSON, text, view, and reflection code are the same under both strategies, and a table message encodes to the same bytes and decodes the same accepted input as its unrolled twin. It differs in three ways: +The option changes only the binary `Message` implementation. The wire format and the JSON, text, view, and reflection code are the same under both strategies, and a table message encodes to the same bytes and decodes the same accepted input as its unrolled twin. It differs in these ways: -- A field that declares a length past the end of its enclosing message fails at once with `DecodeError::UnexpectedEof`, where unrolled code reads on into the enclosing message and can report a different error for the same rejected input. +- A field that declares a length past the end of its enclosing message fails at once with `DecodeError::UnexpectedEof`, where unrolled code reads on into the enclosing message and can report a different error for the same rejected input. A child reached through its `Message` impl is read from the slice of the nearest enclosing table message, so it is bounded there. - The table decodes from one contiguous slice, so a `Buf` that is not contiguous is gathered into one buffer first. `Message::merge_field` on a table message cannot gather, and returns `UnexpectedEof` for such a buffer; only code that calls it directly is affected, such as the default `merge_group`. A message that another crate or another codegen run uses as the type of a group or `DELIMITED` field must therefore stay `Unrolled`. Within one run, codegen keeps the type of a group field unrolled itself. - `clear()` resets a table message to `Default`, so it releases the capacity of its strings and vectors instead of keeping it. +- Encoding into any sink other than the cursor that `Message::encode` and its siblings write a `BufMut` through stages each child reached through its `Message` impl in a scratch buffer first. Those sinks are a `Rope`, a sink defined outside `buffa`, and a `BufMut` passed straight to `Message::write_to`. A `Rope` copies the child again and cannot share the `bytes` fields inside it by reference count. +- Codegen cannot see the fields of a message from another crate, so a table message that holds one with a `bytes` field of a non-default type copies it on decode. The well-known type `google.protobuf.Any` is one, because its `value` is `bytes::Bytes`. To keep the payload shared with a `Bytes` input, set the holder to `Unrolled` with `codec_strategy_in`. These stay unrolled, whatever the setting: @@ -1128,13 +1130,13 @@ These stay unrolled, whatever the setting: - a message that uses the `MessageSet` wire format; - a message with extension ranges, when JSON code is generated and unknown fields are preserved; - a message with a field of a non-default string, bytes, or collection type, which `use_bytes_type`, `string_type`, `bytes_type`, and `repeated_type` select; -- a message that holds any message that stays unrolled, is not selected for the table, or is generated by another crate, such as a well-known type. +- a message that holds, in a singular, repeated, `oneof`, or map value field, a message of the same run that has a `bytes` field of a non-default type, or that holds one. The table decodes from one contiguous slice, so it would copy the `bytes::Bytes` fields that unrolled code decoding from a `Bytes` shares with the input. Codegen prints one warning per run that counts the messages that fell back, groups them by reason, and names a few of each. Setting the messages that cause a fallback to `Unrolled` with `codec_strategy_in` silences it. A `codec_strategy_in` rule that selects the table for a message by its exact path, when the message cannot use it, is an error, because the rule asked for something impossible. The table code needs Rust 1.77 or later; `buffa-build` returns an error on an older compiler, and the plugin's output does not compile on one. It contains `unsafe` code, in macros inside `buffa`, so the generated code compiles in a crate with `#![forbid(unsafe_code)]`. The `buffa::table` module the code calls may change in any release, so regenerate the code whenever you update `buffa`; a mismatch is a compile error. -`compute_size`, decoding from a contiguous buffer, and encoding into a `BufMut` are compiled in `buffa`, at the `opt-level` `buffa` is built with. A build that sets `opt-level = "z"` for everything can spend a little size to recover speed with `[profile.release.package.buffa] opt-level = 3`. Encoding into a sink that is not a `BufMut`, such as `Rope`, and the generic wrappers around decoding are compiled in your crate. +`compute_size`, decoding from a contiguous buffer, and encoding into a `BufMut` are compiled in `buffa`, at the `opt-level` `buffa` is built with. A build that sets `opt-level = "z"` for everything can spend a little size to recover speed with `[profile.release.package.buffa] opt-level = 3`. Encoding into any other sink, such as a `Rope`, and the generic wrappers around decoding are compiled in your crate. ### Error handling diff --git a/protoc-gen-buffa/src/main.rs b/protoc-gen-buffa/src/main.rs index cf62fbbf..da94fee4 100644 --- a/protoc-gen-buffa/src/main.rs +++ b/protoc-gen-buffa/src/main.rs @@ -348,8 +348,8 @@ fn parse_config(params: &str) -> Result { .push(normalize_unbox_oneof_path(value.trim())?); } // `codec_strategy=table` generates the binary `Message` impl of - // every message from a static table and shared interpreters - // (default `unrolled`). Path-scoped rules use the repeatable + // each message the table can handle from a static table and shared + // interpreters (default `unrolled`). Path-scoped rules use the repeatable // `codec_strategy_in==`, whatever the option // order; the last matching rule wins. "codec_strategy" => codegen.codec_strategy = parse_codec_strategy(value)?,