From 44f7614b4dd473eba0948fcba255bbde8a5dd360 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 1 Sep 2026 19:38:00 -0700 Subject: [PATCH] ParseWrapper and TokenStreamWrapper don't work with internal buffering `ParseWrapper` and `TokenStreamWrapper` receive their tokens through a side channel that only works while our deserializer is in charge. This doesn't really work with scenarios in which serde performs internal buffering. Any attempt to use these wrappers in such a scenario results in the somewhat unhelpful error message: invalid type: string "foo", expected TokenStream Improve this error message to indicate that the macro needs to be fixed. A different option is to gracefully degrade with internal buffering, but I'd rather put the burden on macro authors to improve their diagnostics. Note that this error message isn't seen with `#[serde(untagged)]` because it swallows each variant's error. But the underlying principle remains the same. --- src/ibidem.rs | 37 +++++++++++-- src/serde_tokenstream.rs | 55 ++++++++++++++++++++ testlib/src/lib.rs | 26 +++++++++ ui-tests/tests/ui/bad_flatten_wrapper.rs | 14 +++++ ui-tests/tests/ui/bad_flatten_wrapper.stderr | 10 ++++ 5 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 ui-tests/tests/ui/bad_flatten_wrapper.rs create mode 100644 ui-tests/tests/ui/bad_flatten_wrapper.stderr diff --git a/src/ibidem.rs b/src/ibidem.rs index afaf943..94f9c8d 100644 --- a/src/ibidem.rs +++ b/src/ibidem.rs @@ -15,12 +15,22 @@ use crate::serde_tokenstream::spanned_error; /// generates code where the caller of that macro might want to augment the /// generated code. /// +/// # Limitations +/// +/// This type can only be deserialized within [`from_tokenstream`] or +/// [`from_tokenstream_spanned`], and only in positions where serde does not +/// perform internal buffering (e.g., it cannot be used inside +/// `#[serde(flatten)]` or `#[serde(untagged)]`). When used with internal +/// buffering, this produces an error. +/// /// # Panics /// /// The [`Deserialize`] implementation for `TokenStreamWrapper` will panic if -/// it is not used in the context of [`from_tokenstream`]. +/// it is not used in the context of [`from_tokenstream`] or +/// [`from_tokenstream_spanned`]. /// /// [`from_tokenstream`]: crate::from_tokenstream +/// [`from_tokenstream_spanned`]: crate::from_tokenstream_spanned #[derive(Clone, Debug, Default)] pub struct TokenStreamWrapper(TokenStream); @@ -59,13 +69,23 @@ impl std::ops::Deref for TokenStreamWrapper { /// This extends [`TokenStreamWrapper`] by further interpreting the TokenStream /// and guiding the user in the case of parse errors. /// +/// # Limitations +/// +/// This type can only be deserialized within [`from_tokenstream`] or +/// [`from_tokenstream_spanned`], and only in positions where serde does not +/// perform internal buffering (e.g., it cannot be used inside +/// `#[serde(flatten)]` or `#[serde(untagged)]`). When used with internal +/// buffering, this produces an error. +/// /// # Panics /// -/// The [`Deserialize`] implementation for [`TokenStreamWrapper`] will panic if -/// it is not used in the context of [`from_tokenstream`]. +/// The [`Deserialize`] implementation for `ParseWrapper` will panic if it is +/// not used in the context of [`from_tokenstream`] or +/// [`from_tokenstream_spanned`]. /// /// [`Parse`]: syn::parse::Parse /// [`from_tokenstream`]: crate::from_tokenstream +/// [`from_tokenstream_spanned`]: crate::from_tokenstream_spanned #[derive(Clone, Debug, Default, Hash, Eq, PartialEq)] pub struct ParseWrapper(P); @@ -158,7 +178,16 @@ impl Visitor<'_> for WrapperVisitor { &self, formatter: &mut std::fmt::Formatter, ) -> std::fmt::Result { - formatter.write_str("TokenStream") + // Serde shows this text to macro users in case of a wrapper being used + // with internal buffering. (Not for untagged, though, unfortunately, + // because serde swallows errors from untagged variants. Why does + // untagged exist at all if the UX is so bad? Great question, and the + // answer will be a mystery.) + formatter.write_str( + "a ParseWrapper or TokenStreamWrapper value; these cannot be used \ + inside `#[serde(flatten)]`, `#[serde(untagged)]`, or similar -- \ + this is a bug in the macro", + ) } fn visit_bytes(self, bytes: &[u8]) -> Result diff --git a/src/serde_tokenstream.rs b/src/serde_tokenstream.rs index c4b5d97..1f550b9 100644 --- a/src/serde_tokenstream.rs +++ b/src/serde_tokenstream.rs @@ -2180,6 +2180,61 @@ mod tests { assert_eq!(wrapper.into_inner().to_string(), "x"); } + #[test] + fn test_parse_wrapper_buffered() { + // In situations with internal buffering, we must produce an error + // rather than panic. + #[derive(Deserialize)] + #[serde(untagged)] + #[allow(dead_code)] + enum Untagged { + I(ParseWrapper), + N(u32), + } + + #[derive(Deserialize)] + struct Test { + #[allow(dead_code)] + u: Untagged, + } + + match from_tokenstream::("e! { u = s }) { + // With untagged, the error produced by us gets swallowed. + Err(err) => assert_eq!( + err.to_string(), + "data did not match any variant of untagged enum Untagged" + ), + Ok(_) => panic!("unexpected success"), + } + + // With flatten, we can produce a helpful message. + #[derive(Deserialize)] + struct Inner { + #[allow(dead_code)] + id: ParseWrapper, + } + + #[derive(Deserialize)] + struct Flat { + #[allow(dead_code)] + n: u32, + #[serde(flatten)] + #[allow(dead_code)] + inner: Inner, + } + + match from_tokenstream::("e! { n = 1, id = s }) { + Err(err) => assert_eq!( + err.to_string(), + "invalid type: string \"s\", expected a ParseWrapper or \ + TokenStreamWrapper value; these cannot be used inside \ + `#[serde(flatten)]`, `#[serde(untagged)]`, or similar -- \ + this is a bug in the macro" + ), + Ok(_) => panic!("unexpected success"), + } + } + #[test] fn parse_u128() { #[derive(Deserialize)] diff --git a/testlib/src/lib.rs b/testlib/src/lib.rs index efd9013..970c01c 100644 --- a/testlib/src/lib.rs +++ b/testlib/src/lib.rs @@ -407,3 +407,29 @@ pub fn newtype_variant( Err(err) => err.to_compile_error().into(), } } + +// Tests the error in case of a ParseWrapper inside a #[serde(flatten)] struct. +#[derive(Deserialize)] +#[allow(dead_code)] +struct FlattenedWrapper { + n: u32, + #[serde(flatten)] + inner: FlattenedWrapperInner, +} + +#[derive(Deserialize)] +#[allow(dead_code)] +struct FlattenedWrapperInner { + id: ParseWrapper, +} + +#[proc_macro_attribute] +pub fn flattened_wrapper( + attr: proc_macro::TokenStream, + item: proc_macro::TokenStream, +) -> proc_macro::TokenStream { + match from_tokenstream::(&attr.into()) { + Ok(_) => item, + Err(err) => err.to_compile_error().into(), + } +} diff --git a/ui-tests/tests/ui/bad_flatten_wrapper.rs b/ui-tests/tests/ui/bad_flatten_wrapper.rs new file mode 100644 index 0000000..72de8c0 --- /dev/null +++ b/ui-tests/tests/ui/bad_flatten_wrapper.rs @@ -0,0 +1,14 @@ +// Copyright 2026 Oxide Computer Company + +// Ensure that a ParseWrapper inside a `#[serde(flatten)]` struct produces a +// helpful error message. + +use testlib::flattened_wrapper; + +#[flattened_wrapper { + n = 1, + id = foo, +}] +fn test() {} + +fn main() {} diff --git a/ui-tests/tests/ui/bad_flatten_wrapper.stderr b/ui-tests/tests/ui/bad_flatten_wrapper.stderr new file mode 100644 index 0000000..13b11d2 --- /dev/null +++ b/ui-tests/tests/ui/bad_flatten_wrapper.stderr @@ -0,0 +1,10 @@ +error: invalid type: string "foo", expected a ParseWrapper or TokenStreamWrapper value; these cannot be used inside `#[serde(flatten)]`, `#[serde(untagged)]`, or similar -- this is a bug in the macro + --> tests/ui/bad_flatten_wrapper.rs:8:1 + | +8 | / #[flattened_wrapper { +9 | | n = 1, +10 | | id = foo, +11 | | }] + | |__^ + | + = note: this error originates in the attribute macro `flattened_wrapper` (in Nightly builds, run with -Z macro-backtrace for more info)