diff --git a/.changes/unreleased/fixed-20261010-remote-derive-shadowed-names.yaml b/.changes/unreleased/fixed-20261010-remote-derive-shadowed-names.yaml new file mode 100644 index 00000000..08b7bd4a --- /dev/null +++ b/.changes/unreleased/fixed-20261010-remote-derive-shadowed-names.yaml @@ -0,0 +1,4 @@ +kind: Fixed +body: |- + **`buffa-remote-derive`: a constant or const generic named `iter`, `v`, `s`, `payload`, `value`, `key` or `u` no longer breaks the derives** (#693). The generated methods bound parameters with those names, and an identifier pattern that names a constant in scope is a constant pattern, so `#[derive(ProtoList)]` on `struct L(Vec)`, or any derive beside a `const v: u8`, failed with E0308 inside the derive. A static, unit struct or tuple struct with one of those names failed the same way. Every name the generated impls bind now starts with `__buffa_`, like the derives' other generated names. A one-segment override path such as `new = value` or `insert = key` now calls the function of that name instead of resolving to a generated parameter. +time: 2026-10-10T17:56:01.000000Z diff --git a/buffa-remote-derive/Cargo.toml b/buffa-remote-derive/Cargo.toml index bf89a418..c5da047f 100644 --- a/buffa-remote-derive/Cargo.toml +++ b/buffa-remote-derive/Cargo.toml @@ -26,3 +26,4 @@ ecow = { workspace = true } smallvec = { version = "1", default-features = false } smallbox = { version = "0.8", default-features = false } indexmap = { version = "2", default-features = false, features = ["std", "serde"] } +syn = { workspace = true, features = ["visit"] } diff --git a/buffa-remote-derive/src/box_ptr.rs b/buffa-remote-derive/src/box_ptr.rs index 68ed267a..461cce3e 100644 --- a/buffa-remote-derive/src/box_ptr.rs +++ b/buffa-remote-derive/src/box_ptr.rs @@ -30,7 +30,7 @@ pub fn derive(input: DeriveInput) -> syn::Result { let into_inner_call = remote_field::overridable_call(&overrides, "into_inner", field_ty, "into_inner"); - let ctor_new = remote.construct(quote! { #new_call(value) }); + let ctor_new = remote.construct(quote! { #new_call(__buffa_value) }); let arbitrary_impl = forwarders::arbitrary( &remote, @@ -60,7 +60,7 @@ pub fn derive(input: DeriveInput) -> syn::Result { impl #impl_generics ::buffa::ProtoBox<#element_ty> for #ident #ty_generics #where_clause { #[inline] - fn new(value: #element_ty) -> Self { + fn new(__buffa_value: #element_ty) -> Self { #ctor_new } diff --git a/buffa-remote-derive/src/bytes.rs b/buffa-remote-derive/src/bytes.rs index 48b5f2b3..c5b329bb 100644 --- a/buffa-remote-derive/src/bytes.rs +++ b/buffa-remote-derive/src/bytes.rs @@ -26,8 +26,9 @@ pub fn derive(input: DeriveInput) -> syn::Result { let as_bytes = remote_field::qualified_call(field_ty, quote! { ::core::convert::AsRef<[u8]> }, "as_ref"); - let ctor_from_vec = remote.construct(quote! { #from_vec(v) }); - let ctor_from_wire = remote.construct(quote! { #from_vec(payload.as_slice().to_vec()) }); + let ctor_from_vec = remote.construct(quote! { #from_vec(__buffa_vec) }); + let ctor_from_wire = + remote.construct(quote! { #from_vec(__buffa_payload.as_slice().to_vec()) }); // Unlike the `ProtoBox`/`MapStorage` overrides there is no conventional // method name to default to: absent the key, nothing is generated and @@ -69,7 +70,7 @@ pub fn derive(input: DeriveInput) -> syn::Result { impl #impl_generics ::core::convert::From<::buffa::alloc::vec::Vec> for #ident #ty_generics #where_clause { #[inline] - fn from(v: ::buffa::alloc::vec::Vec) -> Self { + fn from(__buffa_vec: ::buffa::alloc::vec::Vec) -> Self { #ctor_from_vec } } @@ -77,7 +78,7 @@ pub fn derive(input: DeriveInput) -> syn::Result { impl #impl_generics ::buffa::ProtoBytes for #ident #ty_generics #where_clause { #[inline] fn from_wire( - payload: ::buffa::WirePayload<'_>, + __buffa_payload: ::buffa::WirePayload<'_>, ) -> ::core::result::Result { ::core::result::Result::Ok(#ctor_from_wire) } diff --git a/buffa-remote-derive/src/forwarders.rs b/buffa-remote-derive/src/forwarders.rs index 58002506..20872a19 100644 --- a/buffa-remote-derive/src/forwarders.rs +++ b/buffa-remote-derive/src/forwarders.rs @@ -275,9 +275,10 @@ pub fn arbitrary( TakeRest::Seed => quote! { #[inline] fn arbitrary_take_rest( - u: ::#krate::Unstructured<#lifetime>, + __buffa_unstructured: ::#krate::Unstructured<#lifetime>, ) -> ::#krate::Result { - let __buffa_seed: #seed = ::#krate::Arbitrary::arbitrary_take_rest(u)?; + let __buffa_seed: #seed = + ::#krate::Arbitrary::arbitrary_take_rest(__buffa_unstructured)?; ::core::result::Result::Ok(#build) } }, @@ -290,9 +291,9 @@ pub fn arbitrary( { #[inline] fn arbitrary( - u: &mut ::#krate::Unstructured<#lifetime>, + __buffa_unstructured: &mut ::#krate::Unstructured<#lifetime>, ) -> ::#krate::Result { - let __buffa_seed: #seed = ::#krate::Arbitrary::arbitrary(u)?; + let __buffa_seed: #seed = ::#krate::Arbitrary::arbitrary(__buffa_unstructured)?; ::core::result::Result::Ok(#build) } @@ -393,6 +394,90 @@ mod tests { } } + /// Every binding an expansion introduces starts with `__buffa_`: the + /// parameters, the closure parameters and the `let` bindings. A plain + /// name becomes a constant pattern beside a constant of that name, and + /// takes the place of a one-segment override path with that name (#652). + #[test] + fn every_binding_starts_with_the_reserved_prefix() { + struct Bindings(Vec); + impl<'ast> syn::visit::Visit<'ast> for Bindings { + fn visit_pat_ident(&mut self, pat: &'ast syn::PatIdent) { + self.0.push(pat.ident.clone()); + syn::visit::visit_pat_ident(self, pat); + } + } + // A named field and every override key, beside the tuple structs + // without overrides that the other helpers expand. + type Derive = fn(syn::DeriveInput) -> syn::Result; + let overridden: [(&'static str, Derive, syn::DeriveInput); 5] = [ + ( + "string", + crate::string::derive, + parse_quote! { + #[buffa(remote = Remote, arbitrary, serde)] + struct S { inner: Remote } + }, + ), + ( + "bytes", + crate::bytes::derive, + parse_quote! { + #[buffa(remote = Remote, as_shared = shared, arbitrary, serde)] + struct B { inner: Remote } + }, + ), + ( + "list", + crate::list::derive, + parse_quote! { + #[buffa(remote = Remote, arbitrary, serde)] + struct L { inner: Remote } + }, + ), + ( + "box", + crate::box_ptr::derive, + parse_quote! { + #[buffa(remote = Remote, new = make, into_inner = take, arbitrary, serde)] + struct P { inner: Remote } + }, + ), + ( + "map", + crate::map::derive, + parse_quote! { + #[buffa( + remote = Remote, len = count, insert = put, clear = wipe, iter = entries, + arbitrary, serde, + )] + struct M { inner: Remote } + }, + ), + ]; + let overridden = overridden.into_iter().map(|(name, derive, input)| { + ( + name, + derive(input).expect("overridden expansion").to_string(), + ) + }); + let arbitrary = expansions() + .into_iter() + .map(|(name, _, keyed)| (name, keyed)); + for (name, expansion) in arbitrary.chain(serde_expansions()).chain(overridden) { + let file: syn::File = syn::parse_str(&expansion).expect("the expansion parses"); + let mut bindings = Bindings(Vec::new()); + syn::visit::Visit::visit_file(&mut bindings, &file); + assert!(!bindings.0.is_empty(), "{name}: no bindings:\n{expansion}"); + for binding in bindings.0 { + assert!( + binding.to_string().starts_with("__buffa_"), + "{name}: binding `{binding}`:\n{expansion}" + ); + } + } + } + /// The string impls call `serde` and the `ProtoString` surface only, so a /// crate can use the key on a string newtype without `buffa/json`. The /// bytes impls take base64 from `buffa::json_helpers`. diff --git a/buffa-remote-derive/src/lib.rs b/buffa-remote-derive/src/lib.rs index 97e087d5..6547364f 100644 --- a/buffa-remote-derive/src/lib.rs +++ b/buffa-remote-derive/src/lib.rs @@ -335,17 +335,16 @@ //! //! # Reserved identifiers //! -//! The generated impls declare lifetimes and type parameters of their own, -//! and each of those names starts with `__buffa` or `__Buffa` (for example -//! `'__buffa_iter` and `__BuffaIter`). Keep those two prefixes out of the -//! newtype's own lifetime and generic parameter names and out of the types and -//! override paths it names. A parameter such as `'a` or `T` cannot collide with -//! a generated one. -//! -//! The generated methods also bind the parameters `value`, `key` and `u`. -//! Write a `new` or `insert` override as a path with two or more segments, -//! such as `Type::method` or `self::helper`, because a bare `value`, `key` or -//! `u` resolves to the parameter. +//! Every name the generated impls introduce starts with `__buffa` or +//! `__Buffa`: their lifetimes, type parameters, function and closure +//! parameters, and local bindings and items (for example `'__buffa_iter`, +//! `__BuffaIter` and `__buffa_value`). These are also the parameter names the +//! newtype's rustdoc shows for its trait impls. Keep the two prefixes out of +//! the newtype's own lifetime and generic parameter names, out of the types +//! and override paths it names, and out of every item in scope at the derive. +//! A name such as `'a`, `T`, `const N`, a constant or static `value`, or the +//! path in an override such as `insert = key` cannot collide with a generated +//! one. //! //! # Why a `remote` attribute that just repeats the field's type? //! diff --git a/buffa-remote-derive/src/list.rs b/buffa-remote-derive/src/list.rs index 44cd4067..690a5dae 100644 --- a/buffa-remote-derive/src/list.rs +++ b/buffa-remote-derive/src/list.rs @@ -41,8 +41,8 @@ pub fn derive(input: DeriveInput) -> syn::Result { "extend", ); - let ctor_from_iter = remote.construct(quote! { #from_iter(iter) }); - let ctor_from_vec = remote.construct(quote! { #from_vec(v) }); + let ctor_from_iter = remote.construct(quote! { #from_iter(__buffa_iter) }); + let ctor_from_vec = remote.construct(quote! { #from_vec(__buffa_vec) }); // The `ProtoList` impl needs bounds beyond the struct's own (the element // bounds, `Extend`, `Default`), so it can't reuse `#where_clause` like @@ -87,7 +87,7 @@ pub fn derive(input: DeriveInput) -> syn::Result { impl #impl_generics ::core::iter::FromIterator<#element_ty> for #ident #ty_generics #where_clause { #[inline] fn from_iter<__BuffaIter: ::core::iter::IntoIterator>( - iter: __BuffaIter, + __buffa_iter: __BuffaIter, ) -> Self { #ctor_from_iter } @@ -95,7 +95,7 @@ pub fn derive(input: DeriveInput) -> syn::Result { impl #impl_generics ::core::convert::From<::buffa::alloc::vec::Vec<#element_ty>> for #ident #ty_generics #where_clause { #[inline] - fn from(v: ::buffa::alloc::vec::Vec<#element_ty>) -> Self { + fn from(__buffa_vec: ::buffa::alloc::vec::Vec<#element_ty>) -> Self { #ctor_from_vec } } @@ -104,8 +104,8 @@ pub fn derive(input: DeriveInput) -> syn::Result { #list_where_clause { #[inline] - fn push(&mut self, value: #element_ty) { - #extend(&mut #accessor, ::core::iter::once(value)); + fn push(&mut self, __buffa_value: #element_ty) { + #extend(&mut #accessor, ::core::iter::once(__buffa_value)); } // Reinitializes via `Default` rather than forwarding to a native diff --git a/buffa-remote-derive/src/map.rs b/buffa-remote-derive/src/map.rs index e2ced87e..97fdb4f8 100644 --- a/buffa-remote-derive/src/map.rs +++ b/buffa-remote-derive/src/map.rs @@ -54,8 +54,8 @@ pub fn derive(input: DeriveInput) -> syn::Result { } #[inline] - fn storage_insert(&mut self, key: #key_ty, value: #value_ty) { - #insert_call(&mut #accessor, key, value); + fn storage_insert(&mut self, __buffa_key: #key_ty, __buffa_value: #value_ty) { + #insert_call(&mut #accessor, __buffa_key, __buffa_value); } #[inline] diff --git a/buffa-remote-derive/src/string.rs b/buffa-remote-derive/src/string.rs index 786687fb..a3693da4 100644 --- a/buffa-remote-derive/src/string.rs +++ b/buffa-remote-derive/src/string.rs @@ -30,9 +30,8 @@ pub fn derive(input: DeriveInput) -> syn::Result { let as_str = remote_field::qualified_call(field_ty, quote! { ::core::convert::AsRef }, "as_ref"); - let ctor_from_string = remote.construct(quote! { #from_string(s) }); - let ctor_from_str = remote.construct(quote! { #from_str(s) }); - let ctor_from_wire = remote.construct(quote! { #from_str(s) }); + let ctor_from_string = remote.construct(quote! { #from_string(__buffa_string) }); + let ctor_from_str = remote.construct(quote! { #from_str(__buffa_str) }); let arbitrary_impl = forwarders::arbitrary( &remote, @@ -62,29 +61,29 @@ pub fn derive(input: DeriveInput) -> syn::Result { impl #impl_generics ::core::convert::From<::buffa::alloc::string::String> for #ident #ty_generics #where_clause { #[inline] - fn from(s: ::buffa::alloc::string::String) -> Self { + fn from(__buffa_string: ::buffa::alloc::string::String) -> Self { #ctor_from_string } } impl #impl_generics ::core::convert::From<&str> for #ident #ty_generics #where_clause { #[inline] - fn from(s: &str) -> Self { + fn from(__buffa_str: &str) -> Self { #ctor_from_str } } impl #impl_generics ::buffa::ProtoString for #ident #ty_generics #where_clause { #[inline] - fn copy_from_str(s: &str) -> Self { + fn copy_from_str(__buffa_str: &str) -> Self { #ctor_from_str } #[inline] fn from_wire( - payload: ::buffa::WirePayload<'_>, + __buffa_payload: ::buffa::WirePayload<'_>, ) -> ::core::result::Result { - payload.to_str().map(|s| #ctor_from_wire) + __buffa_payload.to_str().map(|__buffa_str| #ctor_from_str) } } diff --git a/buffa-remote-derive/tests/shadowed_names.rs b/buffa-remote-derive/tests/shadowed_names.rs new file mode 100644 index 00000000..919e62b0 --- /dev/null +++ b/buffa-remote-derive/tests/shadowed_names.rs @@ -0,0 +1,251 @@ +//! The parameters of the generated methods are not taken over by a constant +//! in scope at the derive (#652). +//! +//! An identifier pattern that names a constant in scope is a constant +//! pattern, not a new binding. So a constant named like a generated parameter +//! turned that parameter into a pattern of the constant's type, and a bare +//! override path with the parameter's name resolved to the parameter. + +use std::collections::HashMap; +use std::hash::Hash; + +use arbitrary::{Arbitrary, Unstructured}; +use buffa::{ + MapStorage as _, ProtoBox as _, ProtoBytes as _, ProtoList as _, ProtoString as _, WirePayload, +}; +use buffa_remote_derive::{MapStorage, ProtoBox, ProtoBytes, ProtoList, ProtoString}; + +const INPUT: &[u8] = b"acegikmoqsuwyACEGIKMOQSUWYacegikmoqsuwyACEGIKMOQSUWY"; + +// An `into_inner` override takes the pointer by value, so the `Box` +// parameter is the required signature. +#[allow(clippy::boxed_local)] +fn unbox(boxed: Box) -> T { + *boxed +} + +/// Every parameter name the generated methods used before #652, as a +/// constant in scope at each derive. No constant has its parameter's type. +#[allow(dead_code, non_upper_case_globals)] +mod constants { + use super::*; + + const iter: u8 = 0; + const v: u8 = 0; + const s: u8 = 0; + const payload: u8 = 0; + const value: u8 = 0; + const key: u8 = 0; + const u: u8 = 0; + + #[derive(Clone, PartialEq, Default, Debug, ProtoString)] + #[buffa(remote = String, arbitrary, serde)] + pub struct Text(pub String); + + #[derive(Clone, PartialEq, Default, Debug, ProtoBytes)] + #[buffa(remote = Vec, arbitrary, serde)] + pub struct Bytes(pub Vec); + + #[derive(Clone, PartialEq, Debug, ProtoList)] + #[buffa(remote = Vec, arbitrary, serde)] + pub struct List(pub Vec); + + impl Default for List { + fn default() -> Self { + Self(Vec::new()) + } + } + + #[derive(Clone, PartialEq, Debug, ProtoBox)] + #[buffa(remote = Box, into_inner = unbox, arbitrary, serde)] + pub struct Pointer(pub Box); + + #[derive(Clone, PartialEq, Debug, MapStorage)] + #[buffa(remote = HashMap, arbitrary, serde)] + pub struct Map(pub HashMap); + + impl Default for Map { + fn default() -> Self { + Self(HashMap::new()) + } + } + + impl FromIterator<(K, V)> for Map { + fn from_iter>(entries: I) -> Self { + Self(HashMap::from_iter(entries)) + } + } +} + +/// Const generic parameters named like generated parameters, which are in +/// scope in every generated impl. +#[allow(non_upper_case_globals)] +mod const_generics { + use super::*; + + #[derive(Clone, PartialEq, Debug, ProtoList)] + #[buffa(remote = Vec)] + pub struct List(pub Vec); + + impl Default for List { + fn default() -> Self { + Self(Vec::new()) + } + } + + #[derive(ProtoBox)] + #[buffa(remote = Box, into_inner = unbox, arbitrary)] + pub struct Pointer(pub Box); + + #[derive(MapStorage)] + #[buffa(remote = HashMap)] + pub struct Map(pub HashMap); +} + +/// A static, a unit struct and a tuple struct named like generated +/// parameters. A binding cannot shadow any of them either. +#[allow(dead_code, non_camel_case_types, non_upper_case_globals)] +mod other_items { + use super::*; + + static v: u8 = 0; + struct value; + struct key(u8); + + #[derive(Clone, PartialEq, Debug, ProtoList)] + #[buffa(remote = Vec)] + pub struct List(pub Vec); + + impl Default for List { + fn default() -> Self { + Self(Vec::new()) + } + } + + #[derive(MapStorage)] + #[buffa(remote = HashMap)] + pub struct Map(pub HashMap); +} + +/// One-segment override paths named like the parameters the generated +/// methods bound before #652. +mod bare_overrides { + use super::*; + + pub fn value(inner: T) -> Box { + Box::new(inner) + } + + pub fn u(inner: T) -> Box { + Box::new(inner) + } + + pub fn key(map: &mut HashMap, k: K, val: V) { + map.insert(k, val); + } + + #[derive(ProtoBox)] + #[buffa(remote = Box, new = value, into_inner = unbox)] + pub struct ValuePointer(pub Box); + + // The `Arbitrary` impl calls `new` where `u` was its parameter. + #[derive(ProtoBox)] + #[buffa(remote = Box, new = u, into_inner = unbox, arbitrary)] + pub struct UPointer(pub Box); + + #[derive(MapStorage)] + #[buffa(remote = HashMap, insert = key)] + pub struct KeyMap(pub HashMap); +} + +#[test] +fn string_beside_constants() { + use constants::Text; + assert_eq!(Text::from("borrowed").0, "borrowed"); + assert_eq!(Text::from(String::from("owned")).0, "owned"); + assert_eq!(Text::copy_from_str("copied").0, "copied"); + let decoded = Text::from_wire(WirePayload::borrowed(b"wire")).unwrap(); + assert_eq!(decoded.0, "wire"); +} + +#[test] +fn bytes_beside_constants() { + use constants::Bytes; + assert_eq!(Bytes::from(vec![1, 2]).0, [1, 2]); + let decoded = Bytes::from_wire(WirePayload::borrowed(&[3, 4])).unwrap(); + assert_eq!(decoded.0, [3, 4]); +} + +#[test] +fn list_beside_constants() { + let mut list: constants::List = (1..=2).collect(); + list.push(3); + assert_eq!(list, constants::List::from(vec![1, 2, 3])); + + let mut list: const_generics::List = (1..=2).collect(); + list.push(3); + assert_eq!(list, const_generics::List::from(vec![1, 2, 3])); + + let mut list = other_items::List::from(vec![1, 2]); + list.push(3); + assert_eq!(&*list, [1, 2, 3]); +} + +#[test] +fn pointer_beside_constants() { + assert_eq!(constants::Pointer::new(7).into_inner(), 7); + assert_eq!(const_generics::Pointer::<_, 1, 2>::new(7).into_inner(), 7); +} + +#[test] +fn map_beside_constants() { + let mut map = constants::Map::default(); + map.storage_insert(1, "one"); + assert_eq!(map.storage_iter().collect::>(), [(&1, &"one")]); + + let mut map = const_generics::Map::<_, _, 1, 2>(HashMap::new()); + map.storage_insert(1, "one"); + assert_eq!(map.storage_iter().collect::>(), [(&1, &"one")]); + + let mut map = other_items::Map(HashMap::new()); + map.storage_insert(1, "one"); + assert_eq!(map.storage_iter().collect::>(), [(&1, &"one")]); +} + +/// The `u` parameter belongs to the `Arbitrary` impls. Each newtype builds +/// the value its canonical type builds from the same input. +#[test] +fn arbitrary_beside_constants() { + fn check(unwrap: impl Fn(N) -> C) + where + N: for<'a> Arbitrary<'a>, + C: for<'a> Arbitrary<'a> + PartialEq + core::fmt::Debug, + { + let got = unwrap(N::arbitrary(&mut Unstructured::new(INPUT)).unwrap()); + let want = C::arbitrary(&mut Unstructured::new(INPUT)).unwrap(); + assert_eq!(got, want); + let got = unwrap(N::arbitrary_take_rest(Unstructured::new(INPUT)).unwrap()); + let want = C::arbitrary_take_rest(Unstructured::new(INPUT)).unwrap(); + assert_eq!(got, want); + } + check(|text: constants::Text| text.0); + check(|bytes: constants::Bytes| bytes.0); + check(|list: constants::List| list.0); + check(|pointer: constants::Pointer| pointer.0); + check(|map: constants::Map| map.0); + check(|pointer: const_generics::Pointer| pointer.0); +} + +#[test] +fn bare_override_paths_resolve_to_the_named_functions() { + assert_eq!(bare_overrides::ValuePointer::new(5).into_inner(), 5); + + let mut map = bare_overrides::KeyMap(HashMap::new()); + map.storage_insert(1, "one"); + assert_eq!(map.storage_iter().collect::>(), [(&1, &"one")]); + + let mut input = Unstructured::new(INPUT); + let pointer = bare_overrides::UPointer::::arbitrary(&mut input).unwrap(); + let want = u64::arbitrary(&mut Unstructured::new(INPUT)).unwrap(); + assert_eq!(pointer.into_inner(), want); +}