diff --git a/.changes/unreleased/breaking-changes-20260914-000000.yaml b/.changes/unreleased/breaking-changes-20260914-000000.yaml new file mode 100644 index 00000000..8703c854 --- /dev/null +++ b/.changes/unreleased/breaking-changes-20260914-000000.yaml @@ -0,0 +1,6 @@ +kind: Breaking changes +body: |- + **`ProtoString` uses `copy_from_str(&str)` instead of requiring `for<'a> From<&'a str>`** (#442, closes #441). This lets string libraries retain borrowing `From<&str>` conversions while copying temporary JSON inputs for non-optional singular fields, and borrowed view text in view-to-owned conversions of custom-representation string fields, into owned storage. The new method defaults to `From`; override it to avoid the intermediate allocation for inline/shared strings. The remote derive and `buffa-smolstr` example preserve their direct borrowed-text construction path. + A custom string type that relied on an efficient `From<&str>` (for example inline storage for short strings) and does not override `copy_from_str` now pays an intermediate `String` allocation when a view converts to an owned message (singular, optional, repeated and oneof string fields; map keys and values already went through `From`) and when JSON deserialization of a non-optional singular field receives borrowed text, because the default can no longer call `From<&str>`; such a type still compiles and produces the same values. Override `copy_from_str` to restore the fast path. Types that use `#[derive(buffa_remote_derive::ProtoString)]` need no change: the derive forwards `copy_from_str` to the remote type's `From<&str>`. + Generic callers that relied on `S: ProtoString` to imply `From<&str>` should use `S::copy_from_str(value)` or add an explicit conversion bound. `json_helpers::proto_string::deserialize` now requires `ProtoString`, not just the two `From` conversions; hand-written users of that helper must implement the trait or use their own deserializer. Regenerate custom-string view code before using a representation without the old bound. Binary decoding, wire format, and default `String` code generation are unchanged. +time: 2026-09-14T00:00:00+00:00 diff --git a/DESIGN.md b/DESIGN.md index d631be38..715f16d2 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -531,9 +531,9 @@ Vtable mode is what makes reflection cheap enough to put on a hot path: reflecti By default a `string` field generates as `String` and a `bytes` field as `Vec`, but both are pluggable: `buffa_build`'s `string_type` / `bytes_type` knobs select a different in-memory representation per field-path prefix, with no change to the wire format. The built-in alternative for `bytes` is `bytes::Bytes` (decoded zero-copy from a `Bytes`-backed buffer); beyond that, either knob accepts any type named by its Rust path — for example `smol_str::SmolStr` or `ecow::EcoString` for read-mostly string fields, or a small-buffer-optimized byte container. Views are unaffected: a `MyMessageView<'a>` always borrows `&'a str` / `&'a [u8]` regardless of the owned representation, and `map<_, string>` keys and values always stay `String`. -**The contract is a `from_wire` constructor, not a blanket impl.** `ProtoString` and `ProtoBytes` (in `buffa`) each require `fn from_wire(WirePayload<'_>) -> Result`, atop the supertraits `Clone + PartialEq + Default + Debug + Send + Sync`, plus `Deref` + `AsRef` + `From` + `From<&str>` for strings, and `Deref` + `AsRef<[u8]>` + `From>` for bytes. The `Deref` bound is load-bearing rather than cosmetic: generated code borrows a field as `&str` / `&[u8]` by plain reference coercion (`&self.field`), so a representation must `Deref` to the slice type, not merely offer `AsRef`. (`ProtoBytes` deliberately omits `From<&[u8]>`, because `bytes::Bytes` implements that only for `&'static [u8]`; requiring it would exclude `Bytes` itself.) There is deliberately **no blanket impl**: a representation implements the trait (a foreign type via a local newtype), and buffa ships the built-in impls for `String`, `Vec`, and `bytes::Bytes`. The earlier design *was* a blanket impl over `From` / `From>`; it was tightened to `from_wire` because the blanket path always pays an allocate-and-copy and a transient heap allocation — even for a short string an inline type could keep on the stack — so it disadvantaged every custom type. +**The contract is a `from_wire` constructor, not a blanket impl.** `ProtoString` and `ProtoBytes` (in `buffa`) each require `fn from_wire(WirePayload<'_>) -> Result`, atop the supertraits `Clone + PartialEq + Default + Debug + Send + Sync`, plus `Deref` + `AsRef` + `From` for strings, and `Deref` + `AsRef<[u8]>` + `From>` for bytes. The `Deref` bound is load-bearing rather than cosmetic: generated code borrows a field as `&str` / `&[u8]` by plain reference coercion (`&self.field`), so a representation must `Deref` to the slice type, not merely offer `AsRef`. (`ProtoBytes` deliberately omits `From<&[u8]>`, because `bytes::Bytes` implements that only for `&'static [u8]`; requiring it would exclude `Bytes` itself.) There is deliberately **no blanket impl**: a representation implements the trait (a foreign type via a local newtype), and buffa ships the built-in impls for `String`, `Vec`, and `bytes::Bytes`. The earlier design *was* a blanket impl over `From` / `From>`; it was tightened to `from_wire` because the blanket path always pays an allocate-and-copy and a transient heap allocation — even for a short string an inline type could keep on the stack — so it disadvantaged every custom type. -**`from_wire` owns validation and borrow-vs-own; the defaults keep their specialized paths.** `WirePayload` is the decoder's gift to the representation: an opaque struct holding the field's bytes either borrowed from the input chunk (the common case for slice- and `Bytes`-backed sources) or owned as `Bytes` otherwise, exposing `as_slice()` (zero-copy borrow), `to_str()` (UTF-8-validated borrow, slack-aware on the borrowed path), `into_bytes()` (zero-copy when owned), and `is_owned()` to branch on which is free. So an inline string type validates via `to_str()` and stores it without a heap allocation, and a refcounted bytes type takes ownership with `into_bytes()`. `decode_string_to::()` / `decode_bytes_to::()` read one length-delimited field, build the payload, and call `S::from_wire` / `B::from_wire`. The default representations keep their hand-tuned fast paths: `String` reuses its buffer in place via `merge_string`, `Vec` via `merge_bytes` (`bytes::Bytes` and every other type go through `from_wire`). Clearing a field resets it to `Default::default()` rather than `.clear()`, because a substituted type may be immutable. The `From` / `From<&str>` supertraits remain, serving the JSON, text-format, and view→owned paths. The `arbitrary` support is type-agnostic: codegen attaches a generic builder (`arbitrary_proto_string::` / `arbitrary_proto_bytes::`) that materializes the canonical `String` / `Vec` and converts via `From`, so a substituted type needs no native `Arbitrary` impl. +**`from_wire` owns validation and borrow-vs-own; the defaults keep their specialized paths.** `WirePayload` is the decoder's gift to the representation: an opaque struct holding the field's bytes either borrowed from the input chunk (the common case for slice- and `Bytes`-backed sources) or owned as `Bytes` otherwise, exposing `as_slice()` (zero-copy borrow), `to_str()` (UTF-8-validated borrow, slack-aware on the borrowed path), `into_bytes()` (zero-copy when owned), and `is_owned()` to branch on which is free. So an inline string type validates via `to_str()` and stores it without a heap allocation, and a refcounted bytes type takes ownership with `into_bytes()`. `decode_string_to::()` / `decode_bytes_to::()` read one length-delimited field, build the payload, and call `S::from_wire` / `B::from_wire`. The default representations keep their hand-tuned fast paths: `String` reuses its buffer in place via `merge_string`, `Vec` via `merge_bytes` (`bytes::Bytes` and every other type go through `from_wire`). Clearing a field resets it to `Default::default()` rather than `.clear()`, because a substituted type may be immutable. Owned JSON strings and text-format parsing still use `From`. Borrowed JSON strings in non-optional singular fields and view→owned conversions use `ProtoString::copy_from_str(&str)`, so a string library can retain a separate borrowing `From<&str>` conversion. The copying method defaults to allocating a `String` and converting it; inline/shared-string implementations should override it to avoid this intermediate allocation. The remote derive and the `buffa-smolstr` example provide that override. The `arbitrary` support is type-agnostic: codegen attaches a generic builder (`arbitrary_proto_string::` / `arbitrary_proto_bytes::`) that materializes the canonical `String` / `Vec` and converts via `From`, so a substituted type needs no native `Arbitrary` impl. A note on what is *not* yet optimal: a borrowed payload (single-chunk source — the common case) copies into an owned `Bytes` only when a representation calls `into_bytes()`, and an owned payload is currently produced only for multi-chunk sources. So a refcounted bytes representation does not yet zero-copy *share* a single `Bytes` input buffer — it is no worse than `Vec` today, and a per-type `PREFERS_OWNED_BYTES` preference is a future, additive (non-breaking) enhancement. Inline string representations already get their full win (zero heap for short strings) regardless. diff --git a/buffa-codegen/src/view.rs b/buffa-codegen/src/view.rs index 0ce236f1..23c7c705 100644 --- a/buffa-codegen/src/view.rs +++ b/buffa-codegen/src/view.rs @@ -2059,21 +2059,14 @@ fn build_to_owned_fields( /// /// The default `String` keeps the original `binding.to_string()` (which /// auto-derefs through a `&&str` binding), so default output is byte-identical -/// to before this knob existed. Other representations build via `From<&str>` -/// with the owned field type driving `Into` inference; `Into::into` takes its -/// argument by value and does not auto-deref, so `double_ref` sites (iterator -/// and match-ergonomics bindings that are `&&str`) get one explicit `*`. -fn str_view_to_owned( - repr: crate::StringRepr, - binding: TokenStream, - double_ref: bool, -) -> TokenStream { +/// to before this knob existed. Other representations use +/// `ProtoString::copy_from_str`, with the field type driving inference. +/// Both paths auto-deref iterator and match-ergonomics `&&str` bindings. +fn str_view_to_owned(repr: crate::StringRepr, binding: TokenStream) -> TokenStream { if repr.is_default() { quote! { #binding.to_string() } - } else if double_ref { - quote! { ::core::convert::Into::into(*#binding) } } else { - quote! { ::core::convert::Into::into(#binding) } + quote! { ::buffa::ProtoString::copy_from_str(#binding) } } } @@ -2094,13 +2087,13 @@ pub(crate) fn singular_to_owned( return Ok(match ty { Type::TYPE_STRING => { // Option<&str>::map. The default keeps `|s| s.to_string()`; for - // other reprs pass the `Into::into` fn directly rather than + // other reprs pass `ProtoString::copy_from_str` directly rather than // wrapping it in a closure (avoids clippy::redundant_closure in // the consumer crate). Inference picks the target from the field. if field_string_repr(ctx, proto_fqn, field_name).is_default() { quote! { self.#ident.map(|s| s.to_string()) } } else { - quote! { self.#ident.map(::core::convert::Into::into) } + quote! { self.#ident.map(::buffa::ProtoString::copy_from_str) } } } Type::TYPE_BYTES => { @@ -2114,7 +2107,6 @@ pub(crate) fn singular_to_owned( Type::TYPE_STRING => str_view_to_owned( field_string_repr(ctx, proto_fqn, field_name), quote! { self.#ident }, - false, ), Type::TYPE_BYTES => bytes_to_owned(ctx, proto_fqn, field_name, quote! { self.#ident }), Type::TYPE_MESSAGE | Type::TYPE_GROUP => { @@ -2150,11 +2142,8 @@ pub(crate) fn repeated_to_owned( match ty { Type::TYPE_STRING => { // RepeatedView<&str>::iter() yields `&&str` (double ref). - let conv = str_view_to_owned( - field_string_repr(ctx, proto_fqn, field_name), - quote! { s }, - true, - ); + let conv = + str_view_to_owned(field_string_repr(ctx, proto_fqn, field_name), quote! { s }); quote! { self.#ident.iter().map(|s| #conv).collect() } } Type::TYPE_BYTES => { @@ -2205,13 +2194,12 @@ pub(crate) fn map_to_owned_expr( // A custom `string_type` on the outer map field promotes a `string` key / // value to the configured owned type, matching the owned-side map type. The - // view always yields `&str`, so the custom type constructs from a fresh - // `String` (via its required `From`), mirroring the bytes path. + // view always yields `&str`, so custom types use their copying constructor. let key_string_repr = map_string_repr(ctx, key_ty, proto_fqn, field_name); let key_conv = match key_ty { Type::TYPE_STRING => match key_string_repr { crate::StringRepr::String => quote! { k.to_string() }, - crate::StringRepr::Custom(_) => quote! { ::core::convert::Into::into(k.to_string()) }, + crate::StringRepr::Custom(_) => quote! { ::buffa::ProtoString::copy_from_str(k) }, }, // utf8_validation = NONE on a string map key: &[u8] → Vec. Type::TYPE_BYTES => quote! { k.to_vec() }, @@ -2228,7 +2216,7 @@ pub(crate) fn map_to_owned_expr( let val_conv = match val_ty { Type::TYPE_STRING => match val_string_repr { crate::StringRepr::String => quote! { v.to_string() }, - crate::StringRepr::Custom(_) => quote! { ::core::convert::Into::into(v.to_string()) }, + crate::StringRepr::Custom(_) => quote! { ::buffa::ProtoString::copy_from_str(v) }, }, Type::TYPE_BYTES => match value_bytes_repr { crate::BytesRepr::Vec => quote! { v.to_vec() }, @@ -2272,11 +2260,7 @@ pub(crate) fn oneof_variant_to_owned( match ty { Type::TYPE_STRING => { // Match ergonomics on `&ViewEnum` binds `v` as `&&str` (double ref). - str_view_to_owned( - field_string_repr(ctx, proto_fqn, field_name), - quote! { v }, - true, - ) + str_view_to_owned(field_string_repr(ctx, proto_fqn, field_name), quote! { v }) } // match-ergonomics on &ViewEnum → v: &&[u8]. bytes_to_owned handles it. Type::TYPE_BYTES => bytes_to_owned(ctx, proto_fqn, field_name, quote! { v }), diff --git a/buffa-remote-derive/src/lib.rs b/buffa-remote-derive/src/lib.rs index 33e8ff42..f4ef4cb6 100644 --- a/buffa-remote-derive/src/lib.rs +++ b/buffa-remote-derive/src/lib.rs @@ -61,10 +61,12 @@ //! `From<&str>`, and `buffa::ProtoString` impls that would otherwise be //! hand-written (compare to the worked example in `buffa-smolstr` or //! `examples/custom-types`). The remote type must already satisfy -//! `ProtoString`'s non-buffa-owned supertraits (`Clone`, `PartialEq`, +//! the forwarding bounds (`Clone`, `PartialEq`, //! `Default`, `Debug`, `Send`, `Sync`, `AsRef`, `From`, -//! `From<&str>`) — true of essentially every inline/shared-string crate, since -//! that's the API surface they're built to offer as a `String` substitute. +//! `for<'a> From<&'a str>`). The derive uses the borrowed conversion for +//! `copy_from_str` as well, avoiding an intermediate `String`. This last bound +//! belongs to the derive, not to `ProtoString`: if the remote type's `From<&str>` +//! borrows, implement `ProtoString` by hand and provide an owning copy operation. //! On the newtype itself, derive the derivable subset (`Clone`, //! `PartialEq`, `Default`, `Debug`) yourself — `Send`/`Sync` are automatic //! for a single-field wrapper, and for the generic list/map derives @@ -208,6 +210,7 @@ mod string; /// `Deref`, `AsRef`, `From`, `From<&str>`, and /// `buffa::ProtoString` for a single-field newtype wrapping the type named by /// `#[buffa(remote = ...)]`. +/// The generated `copy_from_str` forwards to the inner type's `From<&str>`. #[proc_macro_derive(ProtoString, attributes(buffa))] pub fn derive_proto_string(input: TokenStream) -> TokenStream { expand(input, string::derive) diff --git a/buffa-remote-derive/src/string.rs b/buffa-remote-derive/src/string.rs index 3333d367..635ed8e4 100644 --- a/buffa-remote-derive/src/string.rs +++ b/buffa-remote-derive/src/string.rs @@ -64,6 +64,11 @@ pub fn derive(input: DeriveInput) -> syn::Result { } impl #impl_generics ::buffa::ProtoString for #ident #ty_generics #where_clause { + #[inline] + fn copy_from_str(s: &str) -> Self { + #ctor_from_str + } + #[inline] fn from_wire( payload: ::buffa::WirePayload<'_>, diff --git a/buffa-remote-derive/tests/proto_string.rs b/buffa-remote-derive/tests/proto_string.rs index eaff1749..48f0491a 100644 --- a/buffa-remote-derive/tests/proto_string.rs +++ b/buffa-remote-derive/tests/proto_string.rs @@ -28,6 +28,7 @@ fn from_string_and_from_str_round_trip() { let from_owned = MyEcoString::from(String::from("owned")); let from_borrowed = MyEcoString::from("owned"); assert_eq!(from_owned, from_borrowed); + assert_eq!(MyEcoString::copy_from_str("owned"), from_borrowed); } // Named-field struct shape (not just tuple structs) is also supported. @@ -41,6 +42,7 @@ struct NamedEcoString { fn named_field_struct_works() { let s = NamedEcoString::from("named"); assert_eq!(s.as_ref(), "named"); + assert_eq!(NamedEcoString::copy_from_str("named"), s); } // A remote type implementing both `AsRef` and `AsRef<[u8]>` would make diff --git a/buffa-test/build.rs b/buffa-test/build.rs index d293a062..74f48e49 100644 --- a/buffa-test/build.rs +++ b/buffa-test/build.rs @@ -10,6 +10,31 @@ fn main() { .compile() .expect("buffa_build failed for basic.proto"); + // `OwnedStr` is `BorrowingStr<'static>`, whose `From<&'a str>` borrows and + // so cannot serve view-to-owned or JSON text of arbitrary lifetime. Compile + // every field shape against it: those paths must go through + // `ProtoString::copy_from_str`. + buffa_build::Config::new() + .files(&["protos/string_copy.proto"]) + .includes(&["protos/"]) + .string_type_custom("crate::string_copy::OwnedStr") + .generate_json(true) + .generate_text(true) + .compile() + .expect("buffa_build failed for string_copy.proto"); + + // `CountedStr` counts its `copy_from_str` calls, and its `From<&str>` and + // `From` do not: a test can tell whether generated code used + // `copy_from_str` for each field shape. It needs its own package because the + // generated module is named after the proto package. + buffa_build::Config::new() + .files(&["protos/string_copy_counted.proto"]) + .includes(&["protos/"]) + .string_type_custom("crate::string_copy_counted::CountedStr") + .generate_json(true) + .compile() + .expect("buffa_build failed for string_copy_counted.proto"); + // box_type: a crate-LOCAL `CustomBox` pointer (a `ProtoBox` impl) for // singular message fields, via the `*`-templated knob. The crate compiling // is most of the test — the field type, decode (`get_or_insert_default`), diff --git a/buffa-test/protos/string_copy.proto b/buffa-test/protos/string_copy.proto new file mode 100644 index 00000000..38340637 --- /dev/null +++ b/buffa-test/protos/string_copy.proto @@ -0,0 +1,13 @@ +syntax = "proto3"; +package string_copy; + +message Strings { + string name = 1; + optional string maybe = 2; + repeated string items = 3; + map by_name = 4; + oneof choice { + string label = 5; + int32 number = 6; + } +} diff --git a/buffa-test/protos/string_copy_counted.proto b/buffa-test/protos/string_copy_counted.proto new file mode 100644 index 00000000..666b5039 --- /dev/null +++ b/buffa-test/protos/string_copy_counted.proto @@ -0,0 +1,12 @@ +syntax = "proto3"; +package string_copy_counted; + +message Strings { + string name = 1; + optional string maybe = 2; + repeated string items = 3; + map by_name = 4; + oneof choice { + string label = 5; + } +} diff --git a/buffa-test/src/lib.rs b/buffa-test/src/lib.rs index 1b185626..ceb9c610 100644 --- a/buffa-test/src/lib.rs +++ b/buffa-test/src/lib.rs @@ -112,6 +112,10 @@ pub mod vtable_string_repr { } } impl ::buffa::ProtoString for LocalStr { + fn copy_from_str(value: &str) -> Self { + Self::from(value) + } + fn from_wire( payload: ::buffa::WirePayload<'_>, ) -> ::core::result::Result { @@ -271,6 +275,10 @@ pub mod reprs { } } impl ::buffa::ProtoString for EcoStr { + fn copy_from_str(value: &str) -> Self { + Self::from(value) + } + fn from_wire( payload: ::buffa::WirePayload<'_>, ) -> ::core::result::Result { @@ -312,6 +320,10 @@ pub mod reprs { } } impl ::buffa::ProtoString for CompactStr { + fn copy_from_str(value: &str) -> Self { + Self::from(value) + } + fn from_wire( payload: ::buffa::WirePayload<'_>, ) -> ::core::result::Result { @@ -404,6 +416,10 @@ pub mod string_map { } } impl ::buffa::ProtoString for MapStr { + fn copy_from_str(value: &str) -> Self { + Self::from(value) + } + fn from_wire( payload: ::buffa::WirePayload<'_>, ) -> ::core::result::Result { @@ -1001,3 +1017,6 @@ pub mod basic_no_uf { )] #[cfg(test)] mod tests; + +pub mod string_copy; +pub mod string_copy_counted; diff --git a/buffa-test/src/string_copy.rs b/buffa-test/src/string_copy.rs new file mode 100644 index 00000000..0901a13a --- /dev/null +++ b/buffa-test/src/string_copy.rs @@ -0,0 +1,45 @@ +//! A string representation with distinct borrowing and owning constructors. + +use std::borrow::Cow; + +/// Models a string library whose `From<&str>` preserves the input lifetime. +/// Native serde is needed for optional, repeated, map, and oneof JSON fields. +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] +pub struct BorrowingStr<'a>(pub Cow<'a, str>); + +impl<'a> From<&'a str> for BorrowingStr<'a> { + fn from(value: &'a str) -> Self { + Self(Cow::Borrowed(value)) + } +} + +impl From for BorrowingStr<'_> { + fn from(value: String) -> Self { + Self(Cow::Owned(value)) + } +} + +impl std::ops::Deref for BorrowingStr<'_> { + type Target = str; + + fn deref(&self) -> &str { + &self.0 + } +} + +impl AsRef for BorrowingStr<'_> { + fn as_ref(&self) -> &str { + &self.0 + } +} + +/// The generated message stores owned strings, not borrows from its input. +pub type OwnedStr = BorrowingStr<'static>; + +impl buffa::ProtoString for OwnedStr { + fn from_wire(payload: buffa::WirePayload<'_>) -> Result { + payload.to_str().map(|value| Self::from(value.to_owned())) + } +} + +buffa::include_proto!("string_copy"); diff --git a/buffa-test/src/string_copy_counted.rs b/buffa-test/src/string_copy_counted.rs new file mode 100644 index 00000000..c8acec28 --- /dev/null +++ b/buffa-test/src/string_copy_counted.rs @@ -0,0 +1,59 @@ +//! A string representation that counts its `copy_from_str` calls. + +use std::cell::Cell; + +thread_local! { + static COPIES: Cell = const { Cell::new(0) }; +} + +/// The number of `CountedStr::copy_from_str` calls made on this thread. +/// +/// The count is per thread, so tests running in parallel do not disturb each +/// other; compare two readings rather than assuming a starting value. +pub fn copies() -> usize { + COPIES.with(Cell::get) +} + +/// Only `copy_from_str` bumps [`copies`]; `From<&str>` and `From` do +/// not, so a test can tell which constructor generated code called. +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] +pub struct CountedStr(String); + +impl From<&str> for CountedStr { + fn from(value: &str) -> Self { + Self(value.to_owned()) + } +} + +impl From for CountedStr { + fn from(value: String) -> Self { + Self(value) + } +} + +impl std::ops::Deref for CountedStr { + type Target = str; + + fn deref(&self) -> &str { + &self.0 + } +} + +impl AsRef for CountedStr { + fn as_ref(&self) -> &str { + &self.0 + } +} + +impl buffa::ProtoString for CountedStr { + fn copy_from_str(value: &str) -> Self { + COPIES.with(|copies| copies.set(copies.get() + 1)); + Self(value.to_owned()) + } + + fn from_wire(payload: buffa::WirePayload<'_>) -> Result { + payload.to_str().map(Self::from) + } +} + +buffa::include_proto!("string_copy_counted"); diff --git a/buffa-test/tests/string_copy.rs b/buffa-test/tests/string_copy.rs new file mode 100644 index 00000000..d380ec09 --- /dev/null +++ b/buffa-test/tests/string_copy.rs @@ -0,0 +1,74 @@ +use buffa::{Message, MessageView, ProtoString}; +use buffa_test::string_copy::{ + __buffa::{oneof::strings::Choice, view::StringsView}, + BorrowingStr, OwnedStr, Strings, +}; +use std::borrow::Cow; + +fn sample() -> Strings { + Strings { + name: OwnedStr::from(String::from("hello")), + maybe: Some(OwnedStr::from(String::from("optional"))), + items: vec![OwnedStr::from(String::from("item")), OwnedStr::default()], + by_name: [( + OwnedStr::from(String::from("key")), + OwnedStr::from(String::from("value")), + )] + .into_iter() + .collect(), + choice: Some(Choice::Label(OwnedStr::from(String::from("chosen")))), + ..Default::default() + } +} + +#[test] +fn copying_does_not_change_borrowing_conversion() { + let owned = { + let input = String::from("temporary"); + let borrowed = BorrowingStr::from(input.as_str()); + assert!(matches!(borrowed.0, Cow::Borrowed(_))); + OwnedStr::copy_from_str(&input) + }; + assert_eq!(owned.as_ref(), "temporary"); + assert!(matches!(owned.0, Cow::Owned(_))); +} + +#[test] +fn all_string_shapes_outlive_the_view_input() { + let expected = sample(); + let owned = { + let wire = expected.encode_to_vec(); + let view = StringsView::decode_view(&wire).unwrap(); + view.to_owned_message().unwrap() + }; + assert_eq!(owned, expected); + assert_eq!( + Strings::decode(&mut expected.encode_to_vec().as_slice()).unwrap(), + expected + ); +} + +#[test] +fn json_copies_temporary_strings_and_preserves_null_defaults() { + let expected = sample(); + let decoded: Strings = { + let json = serde_json::to_string(&expected).unwrap(); + serde_json::from_str(&json).unwrap() + }; + assert_eq!(decoded, expected); + let empty: Strings = + serde_json::from_str(r#"{"name":null,"maybe":null,"label":null}"#).unwrap(); + assert!(empty.name.is_empty()); + assert!(empty.maybe.is_none()); + assert!(empty.choice.is_none()); +} + +#[test] +fn text_and_clear_keep_existing_behavior() { + let expected = sample(); + let text = buffa::text::encode_to_string(&expected); + let mut decoded: Strings = buffa::text::decode_from_str(&text).unwrap(); + assert_eq!(decoded, expected); + decoded.clear(); + assert_eq!(decoded, Strings::default()); +} diff --git a/buffa-test/tests/string_copy_counted.rs b/buffa-test/tests/string_copy_counted.rs new file mode 100644 index 00000000..639c6a52 --- /dev/null +++ b/buffa-test/tests/string_copy_counted.rs @@ -0,0 +1,69 @@ +//! Generated code must build custom strings with `ProtoString::copy_from_str`, +//! not `From` or `From<&str>`, so an override that avoids the +//! intermediate allocation actually runs. + +use buffa::{Message, MessageView}; +use buffa_test::string_copy_counted::{ + __buffa::{oneof::strings::Choice, view::StringsView}, + copies, CountedStr, Strings, +}; + +fn s(value: &str) -> CountedStr { + CountedStr::from(value.to_owned()) +} + +fn with(populate: impl FnOnce(&mut Strings)) -> Strings { + let mut msg = Strings::default(); + populate(&mut msg); + msg +} + +/// The `copy_from_str` calls made while converting the view of `msg` back to +/// an owned message. +fn copies_in_view_to_owned(msg: &Strings) -> usize { + let wire = msg.encode_to_vec(); + let view = StringsView::decode_view(&wire).unwrap(); + let before = copies(); + let owned = view.to_owned_message().unwrap(); + let count = copies() - before; + assert_eq!(&owned, msg); + count +} + +#[test] +fn view_to_owned_copies_each_string_shape_with_copy_from_str() { + // The implicit-presence `name` is converted even when empty, so it adds one + // copy to every case; each other shape adds one per string it holds. + let cases = [ + ("singular", with(|m| m.name = s("n")), 1), + ("optional", with(|m| m.maybe = Some(s("m"))), 1 + 1), + ("repeated", with(|m| m.items = vec![s("a"), s("b")]), 1 + 2), + ( + "map key and value", + with(|m| m.by_name = [(s("k"), s("v"))].into_iter().collect()), + 1 + 2, + ), + ( + "oneof", + with(|m| m.choice = Some(Choice::Label(s("l")))), + 1 + 1, + ), + ]; + for (shape, msg, expected) in cases { + assert_eq!(copies_in_view_to_owned(&msg), expected, "{shape}"); + } +} + +#[test] +fn json_deserialize_copies_with_copy_from_str() { + let before = copies(); + let msg: Strings = serde_json::from_str(r#"{"name":"n"}"#).unwrap(); + assert_eq!(&*msg.name, "n"); + assert_eq!(copies() - before, 1); + + // `null` builds the empty default the same way. + let before = copies(); + let msg: Strings = serde_json::from_str(r#"{"name":null}"#).unwrap(); + assert!(msg.name.is_empty()); + assert_eq!(copies() - before, 1); +} diff --git a/buffa/src/json_helpers.rs b/buffa/src/json_helpers.rs index db8d0756..27f2ddf2 100644 --- a/buffa/src/json_helpers.rs +++ b/buffa/src/json_helpers.rs @@ -521,30 +521,30 @@ pub mod proto_string { /// Generic over the return type so that codegen's `string_type` knob (which /// can map the field to `smol_str::SmolStr`, `ecow::EcoString`, etc.) works /// without a per-type shim. The visitor constructs the target type directly: - /// `visit_str` goes through `From<&str>`, so a short string is inlined by an - /// SSO type without first allocating an intermediate `String`. `String` - /// itself satisfies both `From<&str>` and `From`, keeping the + /// `visit_str` goes through `ProtoString::copy_from_str`, which an SSO type + /// can override to inline short strings without an intermediate `String`. + /// `String` itself implements `ProtoString`, keeping the /// default path zero-extra-cost. Type inference picks `T` from the field /// type at the serde call site. pub fn deserialize<'de, T, D>(d: D) -> Result where - T: for<'a> From<&'a str> + From, + T: crate::ProtoString, D: Deserializer<'de>, { struct V(core::marker::PhantomData); impl<'de, T> serde::de::Visitor<'de> for V where - T: for<'a> From<&'a str> + From, + T: crate::ProtoString, { type Value = T; fn expecting(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { f.write_str("a string or null") } fn visit_unit(self) -> Result { - Ok(T::from("")) + Ok(T::copy_from_str("")) } fn visit_str(self, v: &str) -> Result { - Ok(T::from(v)) + Ok(T::copy_from_str(v)) } fn visit_string(self, v: alloc::string::String) -> Result { Ok(T::from(v)) diff --git a/buffa/src/json_helpers/tests.rs b/buffa/src/json_helpers/tests.rs index 3c6cbcb7..ecdd9ffb 100644 --- a/buffa/src/json_helpers/tests.rs +++ b/buffa/src/json_helpers/tests.rs @@ -1338,9 +1338,9 @@ fn proto_string_null_is_empty() { } /// A stand-in for a configurable string type (`SmolStr`/`EcoString`/...): it -/// implements just the `From`/`From<&str>`/`AsRef` surface the -/// generic `proto_string` path relies on, proving the with-module deserializes -/// into an arbitrary target type without a per-type shim. +/// implements `ProtoString` with no `From<&str>` impl at all, proving the +/// generic `proto_string` with-module deserializes into an arbitrary target +/// type without a per-type shim. #[derive(Clone, PartialEq, Debug, Default)] struct MyStr(alloc::string::String); impl From for MyStr { @@ -1348,9 +1348,15 @@ impl From for MyStr { MyStr(s) } } -impl From<&str> for MyStr { - fn from(s: &str) -> Self { - MyStr(s.into()) +impl core::ops::Deref for MyStr { + type Target = str; + fn deref(&self) -> &str { + &self.0 + } +} +impl crate::ProtoString for MyStr { + fn from_wire(payload: crate::WirePayload<'_>) -> Result { + payload.to_str().map(|s| Self(s.into())) } } impl AsRef for MyStr { @@ -1366,7 +1372,7 @@ struct SerdeCustomStr(#[serde(with = "proto_string")] MyStr); fn proto_string_deserializes_into_custom_type() { let recovered: SerdeCustomStr = serde_json::from_str(r#""hello""#).unwrap(); assert_eq!(recovered.0, MyStr("hello".into())); - // null still maps to the empty value via the `From` conversion. + // null still maps to the empty value, built by `copy_from_str("")`. let empty: SerdeCustomStr = serde_json::from_str("null").unwrap(); assert_eq!(empty.0, MyStr::default()); // Round-trips back out, serialized via `AsRef`. diff --git a/buffa/src/types.rs b/buffa/src/types.rs index e387b692..8829f1be 100644 --- a/buffa/src/types.rs +++ b/buffa/src/types.rs @@ -1293,9 +1293,11 @@ pub fn string_encoded_len(value: &str) -> usize { /// [`encode_string`] / [`string_encoded_len`] expect `&str`), so the /// representation must `Deref` to `str`; `AsRef` is also required for the /// call sites that ask for it explicitly. -/// - `From` and `From<&str>` — used by the JSON, text-format, and -/// view→owned paths to construct the field from freshly decoded text (binary -/// decode uses [`from_wire`](ProtoString::from_wire) instead). +/// - `From` — constructs fields from owned JSON strings, text-format +/// parsing, and proto2 defaults. +/// - [`copy_from_str`](ProtoString::copy_from_str) — copies borrowed JSON +/// strings in non-optional singular fields, and the text of string view +/// fields of custom representations, into owned storage. Binary decoding uses [`from_wire`](ProtoString::from_wire). /// /// For the default `String` representation every conversion is the identity, so /// the generic path costs nothing relative to the specialized one. @@ -1311,16 +1313,17 @@ pub fn string_encoded_len(value: &str) -> usize { /// - `Deref`, `AsRef`, and the constructors observe the same content — encoding /// borrows via `Deref` / `AsRef` and the view / reflect paths read the same /// way; if they disagree, a value encodes differently than it reads back. -/// - `from_wire` is value-equivalent to `From` / `From<&str>` — binary -/// decode uses `from_wire` while JSON / text / view→owned use `From`, so a -/// representation must not transform the text (e.g. case-fold) in one path but -/// not the other. +/// - `from_wire` is value-equivalent to `From` / `copy_from_str` — binary +/// decode uses `from_wire` while JSON / text / view→owned use the other +/// constructors, so a representation must not transform the text (e.g. +/// case-fold) in one path but not the other. +/// - `copy_from_str(s)` is value-equivalent to `From::from(s.to_owned())` — an +/// override exists only to skip the intermediate `String`, so it must not +/// change the resulting value. /// /// # Limitations /// -/// These apply to a custom type used as a `repeated` element or in a `map` slot -/// (`map` key, `map` value). Singular / optional / oneof -/// uses have none of them and work with a foreign type directly. See the user +/// The requirements below depend on where the custom string appears. See the user /// guide's "String and bytes field representations" section for the full table. /// /// - **Must be crate-local.** A custom type in a `repeated` element or `map` @@ -1328,10 +1331,11 @@ pub fn string_encoded_len(value: &str) -> usize { /// vtable reflection), which the orphan rule permits only when the type is /// local to the generating crate. A *foreign* custom type in those positions /// fails to compile — wrap it in a crate-local newtype. -/// - **JSON needs native `serde`.** A custom string used as a `repeated` element -/// or in a `map` serializes through its own `serde`, so it must derive +/// - **JSON needs native `serde` outside non-optional singular fields.** A custom +/// string in an optional, repeated, map, or oneof field uses its own `serde`, +/// so it must implement /// `Serialize` / `Deserialize` (and, for an external type, enable its `serde` -/// feature). Singular / optional / oneof custom strings use the `proto_string` +/// feature). Non-optional singular custom strings use the `proto_string` /// with-module and need no `serde` impl. /// - **A `map` key needs `Hash + Eq`** (default / `HashMap` container) or `Ord` /// (`map_type(BTreeMap)`); the bound is enforced at the generated field type. @@ -1344,8 +1348,8 @@ pub fn string_encoded_len(value: &str) -> usize { since(1.78), diagnostic::on_unimplemented( message = "`{Self}` cannot be used as a buffa custom string type", - note = "buffa owns `ProtoString`, so a foreign type can't implement it directly (orphan rule). \ - Wrap it in a crate-local newtype and implement `ProtoString` on the newtype. \ + note = "Implement `ProtoString` and its supertraits on your type. \ + If the type is defined in another crate, wrap it in a crate-local newtype first. \ See `examples/custom-types` in the buffa repository for a template." ) )] @@ -1359,8 +1363,28 @@ pub trait ProtoString: + core::ops::Deref + AsRef + From - + for<'a> From<&'a str> { + /// Copy text into storage that does not borrow from `value`. + /// + /// The default constructs a [`String`] and passes it to `From`. + /// Unlike `From<&str>`, this method does not tie the result's lifetime to + /// the input, so a type can retain a separate borrowing `From<&str>` + /// implementation. + /// + /// Override it if the type stores short strings inline or shares them. The + /// default cannot call `From<&str>`, so a type with a cheaper + /// borrowed-text constructor that does not override this method pays the + /// intermediate `String` allocation wherever generated code copies borrowed + /// text: view-to-owned conversion of singular, optional, repeated, map and + /// oneof string fields, and JSON deserialization of non-optional singular + /// fields. The values are unchanged; only the cost differs. + /// `#[derive(buffa_remote_derive::ProtoString)]` forwards this method to + /// the remote type's `From<&str>`, so derived types are already covered. + #[inline] + fn copy_from_str(value: &str) -> Self { + Self::from(String::from(value)) + } + /// Construct the representation from a decoded `string` field's wire payload. /// /// This is the decode constructor: it owns the validation/ownership choice, @@ -1370,7 +1394,7 @@ pub trait ProtoString: /// picks up the `fast-utf8` feature), or read raw bytes with /// [`WirePayload::as_slice`]. There is /// intentionally no blanket impl — every representation provides its own - /// optimal `from_wire`; the `From`/`From<&str>` supertraits remain + /// optimal `from_wire`; `From` and [`Self::copy_from_str`] remain /// for the JSON, text, and view→owned paths. /// /// # Errors diff --git a/docs/guide.md b/docs/guide.md index 50279e74..38e725ab 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -349,8 +349,12 @@ use buffa::{DecodeError, ProtoString, WirePayload}; #[derive(Clone, PartialEq, Eq, Default, Debug)] pub struct CompactStr(pub compact_str::CompactString); -// … Deref, AsRef, From, From<&str> forwards … +// … Deref, AsRef, From forwards … impl ProtoString for CompactStr { + fn copy_from_str(value: &str) -> Self { + CompactStr(value.into()) // Inline short strings without a temporary String. + } + fn from_wire(p: WirePayload<'_>) -> Result { let s = core::str::from_utf8(p.as_slice()).map_err(|_| DecodeError::InvalidUtf8)?; Ok(CompactStr(s.into())) @@ -358,7 +362,11 @@ impl ProtoString for CompactStr { } ``` -If you see a `ProtoString` / `ProtoBytes` bound error pointing at *generated* code, your newtype is missing one of the supertraits — check the full bound list (`Clone + PartialEq + Default + Debug + Send + Sync`, `Deref`, `AsRef`, the `From` conversions, and `from_wire`). If instead you see an *orphan-rule* error (`E0117` / `E0210`) in generated code mentioning `ReflectElement` or `ReflectMapKey`, you used a **foreign** custom type as a `repeated` element or `map` key/value under vtable reflection — wrap it in a crate-local newtype (see the `map`/`repeated` key points below). +`ProtoString::copy_from_str` copies borrowed JSON text in non-optional singular fields and the string view fields of custom representations into owned storage. Its default goes through `String` and `From`; override it as above to avoid a temporary allocation for inline/shared strings. The trait does not require `From<&str>`, so a string library can keep that conversion's borrowing semantics. The remote derive still requires `From<&str>` for every input lifetime and forwards `copy_from_str` through it; types with a borrowing conversion should implement `ProtoString` by hand. + +When migrating generic code that relied on `S: ProtoString` to imply `From<&str>`, use `S::copy_from_str(value)` or add an explicit conversion bound. Regenerate custom-string views before using a representation without the old bound. Hand-written users of `json_helpers::proto_string::deserialize` now need `ProtoString`, not just the two `From` conversions; implement the trait or provide a separate deserializer. + +If you see a `ProtoString` / `ProtoBytes` bound error pointing at *generated* code, your newtype may be missing the trait implementation or a supertrait — check `from_wire` and the full bound list (`Clone + PartialEq + Default + Debug + Send + Sync`, `Deref`, `AsRef`, and `From` / `From>`). If instead you see an *orphan-rule* error (`E0117` / `E0210`) in generated code mentioning `ReflectElement` or `ReflectMapKey`, you used a **foreign** custom type as a `repeated` element or `map` key/value under vtable reflection — wrap it in a crate-local newtype (see the `map`/`repeated` key points below). Key points: @@ -366,7 +374,7 @@ Key points: - **Only the owned struct field type changes.** The wire format is identical regardless of representation, and view types still borrow `&str` / `&[u8]`. - **The rule also covers `map` `string` slots.** A `string_type` rule on a `map` / `map` field applies to the key and/or value — one rule on the field path covers both slots of a `map`. The `map` container itself stays the configured type (the `map_type` knob); only the `string` element type changes. (`bytes` is value-only here, since proto forbids `bytes` map keys.) Because the rule is keyed on the field path, a `map` is all-or-nothing: you cannot give the key a custom type and leave the value `String` (or vice versa) on the same field. Asymmetric cases where only one slot is `string` (`map`, `map`) are unaffected. - **A custom type needs no `Arbitrary` impl — except in a `map`.** Under `generate_arbitrary`, singular / optional / repeated fields get a generic builder. The `map` arbitrary path currently has no per-key shim, so a custom string used as a `map` key or value must itself derive `Arbitrary` (a one-line derive on the newtype). -- **JSON of an `optional` or `repeated` custom string, or any custom string in a `map`,** serializes through the element's native `serde`, so such a newtype must derive `Serialize` / `Deserialize` (`buffa-smolstr`'s `serde` feature does this). Singular and oneof string fields use buffa's `proto_string` with-module and need no `serde` impl. +- **JSON of an `optional`, `repeated`, or `oneof` custom string, or any custom string in a `map`,** serializes through the element's native `serde`, so such a newtype must derive `Serialize` / `Deserialize` (`buffa-smolstr`'s `serde` feature does this). Non-optional singular string fields use buffa's `proto_string` with-module and need no `serde` impl. - **A custom string used as a `map` key needs `Hash + Eq`** (for the default / `HashMap` container) or `Ord` (for `map_type(BTreeMap)`). The bound is enforced at the generated map field type, so a missing impl is a clear compile error at that field. - **A custom type used as a `repeated` element, or as a `map` key/value, must be crate-local.** Codegen emits per-element `ReflectElement` (vtable reflection), `ReflectMapKey` (vtable, for a custom `string` map key), and base64 `ProtoElemJson` (JSON, bytes only) impls for it, which the orphan rule forbids for a foreign type — a local newtype satisfies this. Singular / optional / oneof uses have no such restriction. diff --git a/examples/buffa-smolstr/src/lib.rs b/examples/buffa-smolstr/src/lib.rs index 37523eaa..017a6c35 100644 --- a/examples/buffa-smolstr/src/lib.rs +++ b/examples/buffa-smolstr/src/lib.rs @@ -84,6 +84,11 @@ impl From<&str> for SmolStr { } impl ProtoString for SmolStr { + #[inline] + fn copy_from_str(value: &str) -> Self { + Self(smol_str::SmolStr::from(value)) + } + /// Validate the payload as UTF-8 and build a `SmolStr` directly from the /// borrowed `&str` — short strings inline with no heap allocation, avoiding /// the transient `String` a `From` decode path would allocate. @@ -110,6 +115,13 @@ mod tests { ); } + #[test] + fn copying_short_string_keeps_inline_storage() { + let copied = SmolStr::copy_from_str("hello"); + assert_eq!(copied.as_ref(), "hello"); + assert!(!copied.0.is_heap_allocated()); + } + #[test] fn long_string_roundtrips() { let long = "x".repeat(64); diff --git a/examples/custom-types/README.md b/examples/custom-types/README.md index a97ce24a..ee1f41b9 100644 --- a/examples/custom-types/README.md +++ b/examples/custom-types/README.md @@ -51,7 +51,7 @@ For the common case the newtype's whole buffa-facing surface can be generated fr pub struct FlexStr(pub flexstr::SharedStr); ``` -The derive expands the `buffa::ProtoString` impl plus the rest of the required surface (`Deref`, `AsRef`, `From`, `From<&str>`), each a one-line forward to the inner type. The other newtypes in this crate hand-write that same surface — `small_bytes.rs` and `small_vec.rs` deliberately so, because their impls carry allocation behavior the generic derive can't express (a `from_wire` with no intermediate `Vec`, a capacity-retaining `clear`) — so the module shows both the generated and the written-out form of the pattern. The `assert_transparent!` macro in [`src/types/mod.rs`](src/types/mod.rs) freezes the zero-cost guarantee — if a second field ever sneaks into the wrapper, the build fails. +The derive expands the `buffa::ProtoString` impl plus forwarding implementations of `Deref`, `AsRef`, `From`, and `From<&str>`. Its `copy_from_str` calls the inner type's `From<&str>` directly, avoiding an intermediate `String`. The derive requires that conversion for any input lifetime; `ProtoString` itself does not, so a type whose conversion borrows can implement the trait by hand instead. The other newtypes in this crate also show hand-written implementations — `small_bytes.rs` and `small_vec.rs` deliberately so, because their impls carry allocation behavior the generic derive can't express (a `from_wire` with no intermediate `Vec`, a capacity-retaining `clear`). The `assert_transparent!` macro in [`src/types/mod.rs`](src/types/mod.rs) freezes the zero-cost guarantee — if a second field ever sneaks into the wrapper, the build fails. ## What each newtype needs for JSON