From efebcb6308bb79433f4b58e9380c4f324ff3035a Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 1 Sep 2026 19:31:47 -0700 Subject: [PATCH] delete InternalError::Unknown Previously, `last_err` returned `Unknown` if no tokens had been read. Contrary to the comments there, it is actually possible to hit this case in situations like `Named()` or an empty declarative macro substitution. Change things so that every `TokenDe` now carries the enclosing group. This lets us remove the `Unknown` variant and fix a couple of panics. --- src/serde_tokenstream.rs | 60 +++++++++++-------- testlib/src/lib.rs | 23 +++++++ .../tests/ui/bad_newtype_variant_empty.rs | 13 ++++ .../tests/ui/bad_newtype_variant_empty.stderr | 5 ++ 4 files changed, 75 insertions(+), 26 deletions(-) create mode 100644 ui-tests/tests/ui/bad_newtype_variant_empty.rs create mode 100644 ui-tests/tests/ui/bad_newtype_variant_empty.stderr diff --git a/src/serde_tokenstream.rs b/src/serde_tokenstream.rs index 1444b7a..10ebe12 100644 --- a/src/serde_tokenstream.rs +++ b/src/serde_tokenstream.rs @@ -149,9 +149,6 @@ where "Error::Unspanned should never propagate to the caller: {}", msg ), - Err(InternalError::Unknown) => { - panic!("Error::Unknown should never propagate to the caller") - } } } @@ -159,7 +156,6 @@ where enum InternalError { Spanned(Error), Unspanned(String), - Unknown, } type InternalResult = std::result::Result; @@ -169,6 +165,9 @@ struct TokenDe { current: Option, last: Option, pending_member: bool, + // The group whose contents this deserializer is currently reading. This is + // used as a fallback in case a more specific span isn't available. + enclosing: Group, } impl<'de> TokenDe { @@ -185,10 +184,10 @@ impl<'de> TokenDe { if let Some(span) = span { group.set_span(span.join()); } - TokenDe::new(&TokenStream::from(TokenTree::from(group))) + TokenDe::new(&group, &TokenStream::from(TokenTree::from(group.clone()))) } - fn new(input: &'de TokenStream) -> Self { + fn new(enclosing: &Group, input: &'de TokenStream) -> Self { let t: Box> = Box::new(input.clone().into_iter()); TokenDe { @@ -196,6 +195,7 @@ impl<'de> TokenDe { current: None, last: None, pending_member: false, + enclosing: enclosing.clone(), } } @@ -228,11 +228,12 @@ impl<'de> TokenDe { token.span(), format!("expected {} following `{}`", what, token), ))), - // It should not be possible to reach this point. Although - // `self.last` starts as `None`, the first thing we'll try to do - // is deserialize a structure type based on the `Group` we create - // in `::from_tokenstream`. - None => Err(InternalError::Unknown), + // Nothing's been read yet, so the enclosing group is empty. This + // can happen in situations like `V()`. + None => Err(InternalError::Spanned(Error::new( + self.enclosing.span(), + format!("expected {} inside `{}`", what, self.enclosing), + ))), } } @@ -458,7 +459,10 @@ impl<'de> EnumAccess<'de> for &mut TokenDe { } // This can't happen; we will need to have read a token at // this point. - None => Err(InternalError::Unknown), + None => Err(InternalError::Spanned(Error::new( + self.enclosing.span(), + msg, + ))), }, Err(err) => Err(err), } @@ -480,7 +484,8 @@ impl<'de> VariantAccess<'de> for &mut TokenDe { if let Some(TokenTree::Group(group)) = &next { if let Delimiter::Parenthesis = group.delimiter() { - return seed.deserialize(&mut TokenDe::new(&group.stream())); + return seed + .deserialize(&mut TokenDe::new(group, &group.stream())); } } self.deserialize_error(next, "(") @@ -500,7 +505,7 @@ impl<'de> VariantAccess<'de> for &mut TokenDe { if let TokenTree::Group(group) = token { if let Delimiter::Parenthesis = group.delimiter() { return match visitor - .visit_seq(TokenDe::new(&group.stream())) + .visit_seq(TokenDe::new(group, &group.stream())) { Err(InternalError::Unspanned(msg)) => { Err(InternalError::Spanned(Error::new( @@ -534,7 +539,9 @@ impl<'de> VariantAccess<'de> for &mut TokenDe { // then use that rather than the call to // deserialize_ignored_any to determine if the // given field is valid. - match visitor.visit_map(TokenDe::new(&group.stream())) { + match visitor + .visit_map(TokenDe::new(group, &group.stream())) + { Err(InternalError::Unspanned(msg)) => { return Err(InternalError::Spanned(Error::new( token.span(), @@ -631,7 +638,7 @@ impl<'de> Deserializer<'de> for &mut TokenDe { if let TokenTree::Group(group) = token { if let Delimiter::Bracket = group.delimiter() { return match visitor - .visit_seq(TokenDe::new(&group.stream())) + .visit_seq(TokenDe::new(group, &group.stream())) { Err(InternalError::Unspanned(msg)) => { Err(InternalError::Spanned(Error::new( @@ -666,7 +673,9 @@ impl<'de> Deserializer<'de> for &mut TokenDe { // then use that rather than the call to // deserialize_ignored_any to determine if the // given field is valid. - match visitor.visit_map(TokenDe::new(&group.stream())) { + match visitor + .visit_map(TokenDe::new(group, &group.stream())) + { Err(InternalError::Unspanned(msg)) => { return Err(InternalError::Spanned(Error::new( token.span(), @@ -690,7 +699,7 @@ impl<'de> Deserializer<'de> for &mut TokenDe { if let Some(TokenTree::Group(group)) = &next { if let Delimiter::Brace = group.delimiter() { - return visitor.visit_map(TokenDe::new(&group.stream())); + return visitor.visit_map(TokenDe::new(group, &group.stream())); } } @@ -770,7 +779,7 @@ impl<'de> Deserializer<'de> for &mut TokenDe { if let TokenTree::Group(group) = token { if let Delimiter::Parenthesis = group.delimiter() { return match visitor - .visit_seq(TokenDe::new(&group.stream())) + .visit_seq(TokenDe::new(group, &group.stream())) { Err(InternalError::Unspanned(msg)) => { Err(InternalError::Spanned(Error::new( @@ -797,24 +806,23 @@ impl<'de> Deserializer<'de> for &mut TokenDe { None => self.last_err("a value"), Some(TokenTree::Group(group)) => match group.delimiter() { Delimiter::Brace => { - visitor.visit_map(TokenDe::new(&group.stream())) + visitor.visit_map(TokenDe::new(group, &group.stream())) } Delimiter::Bracket => { - visitor.visit_seq(TokenDe::new(&group.stream())) + visitor.visit_seq(TokenDe::new(group, &group.stream())) } Delimiter::Parenthesis => { let stream = &group.stream(); if stream.is_empty() { visitor.visit_unit() } else { - visitor.visit_seq(TokenDe::new(stream)) + visitor.visit_seq(TokenDe::new(group, stream)) } } // A None delimiter occurs for a macro_rules! substitution. We // can simply descend into those tokens. - Delimiter::None => { - TokenDe::new(&group.stream()).deserialize_any(visitor) - } + Delimiter::None => TokenDe::new(group, &group.stream()) + .deserialize_any(visitor), }, Some(TokenTree::Ident(ident)) if *ident == "true" => { visitor.visit_bool(true) @@ -984,7 +992,7 @@ impl<'de> Deserializer<'de> for &mut TokenDe { // This can't happen -- we need to have read a token in order // for serde to determine that this value will // be ignored. - None => return Err(InternalError::Unknown), + None => TokenTree::Group(self.enclosing.clone()), }; // We know this is going to be an error, but we parse the value diff --git a/testlib/src/lib.rs b/testlib/src/lib.rs index 537779a..4a3e8bb 100644 --- a/testlib/src/lib.rs +++ b/testlib/src/lib.rs @@ -139,3 +139,26 @@ pub fn outer( Err(err) => err.to_compile_error().into(), } } + +#[derive(Deserialize)] +#[allow(dead_code)] +struct NewtypeVariant { + value: Wrapped, +} + +#[derive(Deserialize)] +#[allow(dead_code)] +enum Wrapped { + Named(String), +} + +#[proc_macro_attribute] +pub fn newtype_variant( + 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_newtype_variant_empty.rs b/ui-tests/tests/ui/bad_newtype_variant_empty.rs new file mode 100644 index 0000000..7221fee --- /dev/null +++ b/ui-tests/tests/ui/bad_newtype_variant_empty.rs @@ -0,0 +1,13 @@ +// Copyright 2026 Oxide Computer Company + +// An empty newtype variant body must be an error with the span set to the +// parentheses, not a panic. + +use testlib::newtype_variant; + +#[newtype_variant { + value = Named(), +}] +fn test() {} + +fn main() {} diff --git a/ui-tests/tests/ui/bad_newtype_variant_empty.stderr b/ui-tests/tests/ui/bad_newtype_variant_empty.stderr new file mode 100644 index 0000000..a0a4fe0 --- /dev/null +++ b/ui-tests/tests/ui/bad_newtype_variant_empty.stderr @@ -0,0 +1,5 @@ +error: expected a string inside `()` + --> tests/ui/bad_newtype_variant_empty.rs:9:18 + | +9 | value = Named(), + | ^^