From f8f02b473ec7bbe54f1d08cc7bfc105c4061d9f9 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 1 Sep 2026 19:34:41 -0700 Subject: [PATCH] add a better syn::Error::new_spanned See comments in the commit for why this is useful. --- clippy.toml | 4 + src/lib.rs | 1 + src/serde_tokenstream.rs | 121 +++++++++++++++--- .../tests/ui/bad_string_from_macro_rules.rs | 22 ++++ .../ui/bad_string_from_macro_rules.stderr | 5 + .../ui/bad_string_from_macro_rules_path.rs | 22 ++++ .../bad_string_from_macro_rules_path.stderr | 5 + .../ui/bad_string_from_nested_macro_rules.rs | 28 ++++ .../bad_string_from_nested_macro_rules.stderr | 5 + .../tests/ui/bad_untagged_from_macro_rules.rs | 19 +++ .../ui/bad_untagged_from_macro_rules.stderr | 5 + 11 files changed, 221 insertions(+), 16 deletions(-) create mode 100644 clippy.toml create mode 100644 ui-tests/tests/ui/bad_string_from_macro_rules.rs create mode 100644 ui-tests/tests/ui/bad_string_from_macro_rules.stderr create mode 100644 ui-tests/tests/ui/bad_string_from_macro_rules_path.rs create mode 100644 ui-tests/tests/ui/bad_string_from_macro_rules_path.stderr create mode 100644 ui-tests/tests/ui/bad_string_from_nested_macro_rules.rs create mode 100644 ui-tests/tests/ui/bad_string_from_nested_macro_rules.stderr create mode 100644 ui-tests/tests/ui/bad_untagged_from_macro_rules.rs create mode 100644 ui-tests/tests/ui/bad_untagged_from_macro_rules.stderr diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000..cb340bb --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +disallowed-methods = [ + { path = "syn::Error::new", reason = "use spanned_error, which reports macro_rules substitutions at the invocation" }, + { path = "syn::Error::new_spanned", reason = "use spanned_error, which reports macro_rules substitutions at the invocation" }, +] diff --git a/src/lib.rs b/src/lib.rs index 3b032e7..050acf7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -65,3 +65,4 @@ pub use crate::serde_tokenstream::Error; pub use crate::serde_tokenstream::Result; pub use crate::serde_tokenstream::from_tokenstream; pub use crate::serde_tokenstream::from_tokenstream_spanned; +pub use crate::serde_tokenstream::spanned_error; diff --git a/src/serde_tokenstream.rs b/src/serde_tokenstream.rs index 24729ca..ee3dc33 100644 --- a/src/serde_tokenstream.rs +++ b/src/serde_tokenstream.rs @@ -153,8 +153,8 @@ where // On success, check that there aren't additional, unparsed tokens. match deserializer.next() { None => Ok(result), - Some(token) => Err(Error::new( - token.span(), + Some(token) => Err(spanned_error( + &token, format!("expected EOF but found `{}`", token), )), } @@ -172,7 +172,7 @@ impl InternalError { fn into_error(self, fallback: impl ToTokens) -> Error { match self { InternalError::Spanned(err) => err, - InternalError::Unspanned(msg) => Error::new_spanned(fallback, msg), + InternalError::Unspanned(msg) => spanned_error(fallback, msg), } } @@ -182,6 +182,95 @@ impl InternalError { } } +/// A better `syn::Error::new_spanned`. +/// +/// With `macro_rules` macros, `syn::Error::new_spanned` on a proc macro +/// invocation inside the declarative macro would result in the span being the +/// macro declaration. This constructor pierces through that to ensure better +/// diagnostic reporting for this cases. +/// +/// # Example +/// +/// Consider a declarative macro that forwards an expression into an attribute +/// macro that uses `serde_tokenstream`: +/// +/// ```text +/// macro_rules! wrap { +/// ($s:expr) => { +/// #[annotation { +/// string = $s, +/// options = OptionA, +/// unit = (), +/// tup = (1, 2.0), +/// }] +/// fn test() {} +/// }; +/// } +/// +/// wrap!(foo::bar); +/// ``` +/// +/// `string` is expected to be an identifier, but a path `foo::bar` is passed +/// in. In this case, `syn::Error::new_spanned` takes the spans of the first and +/// last tokens it is given, which are both the group, so the diagnostic points to +/// the definition: +/// +/// ```text +/// error: expected a string, but found `foo::bar` +/// --> tests/ui/bad_string_from_macro_rules_path.rs:11:22 +/// | +/// 11 | string = $s, +/// | ^^ +/// ... +/// 20 | wrap!(foo::bar); +/// | --------------- in this macro invocation +/// ``` +/// +/// What `spanned_error` does is replace `Delimiter::None` groups with their contents +/// (recursively, to handle nested declarative macros) before taking the first +/// and last spans, so the same error is reported at the invocation: +/// +/// ```text +/// error: expected a string, but found `foo::bar` +/// --> tests/ui/bad_string_from_macro_rules_path.rs:20:7 +/// | +/// 20 | wrap!(foo::bar); +/// | ^^^^^^^^ +/// ``` +/// +/// An empty substitution (for example an empty `$v:vis`) has nothing to +/// underline, so the group's own span is used as a fallback. +#[expect(clippy::disallowed_methods)] +pub fn spanned_error(tokens: impl ToTokens, msg: impl Display) -> Error { + let tokens = tokens.into_token_stream(); + let substituted = flatten_none_groups(tokens.clone()); + if substituted.is_empty() { + // If flattening groups resulted in nothing, we've got to have + // _something_ to point to. Use tokens as a fallback. + Error::new_spanned(tokens, msg) + } else { + Error::new_spanned(substituted, msg) + } +} + +/// Replaces `Delimiter::None` groups with their contents, recursively. +/// +/// `Delimiter::None` groups can occur with declarative macro substitutions. +fn flatten_none_groups(stream: TokenStream) -> TokenStream { + stream + .into_iter() + .flat_map(|tt| match tt { + TokenTree::Group(group) if group.delimiter() == Delimiter::None => { + flatten_none_groups(group.stream()) + } + TokenTree::Group(_) + | TokenTree::Ident(_) + | TokenTree::Punct(_) + | TokenTree::Literal(_) => TokenStream::from(tt), + }) + .collect() +} + type InternalResult = std::result::Result; struct TokenDe { @@ -211,8 +300,8 @@ impl TokenDe { match self.next() { None => Ok(()), Some(TokenTree::Punct(punct)) if punct.as_char() == ',' => Ok(()), - Some(token) => Err(InternalError::Spanned(Error::new( - token.span(), + Some(token) => Err(InternalError::Spanned(spanned_error( + &token, format!("expected `,` or nothing, but found `{}`", token), ))), } @@ -232,14 +321,14 @@ impl TokenDe { fn last_err(&self, what: &str) -> InternalResult { match &self.last { - Some(token) => Err(InternalError::Spanned(Error::new( - token.span(), + Some(token) => Err(InternalError::Spanned(spanned_error( + token, format!("expected {} following `{}`", what, token), ))), // 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(), + None => Err(InternalError::Spanned(spanned_error( + &self.enclosing, format!("expected {} inside `{}`", what, self.enclosing), ))), } @@ -251,8 +340,8 @@ impl TokenDe { what: &str, ) -> InternalResult { match next { - Some(token) => Err(InternalError::Spanned(Error::new( - token.span(), + Some(token) => Err(InternalError::Spanned(spanned_error( + &token, format!("expected {}, but found `{}`", what, token), ))), None => self.last_err(what), @@ -381,14 +470,14 @@ impl<'de> MapAccess<'de> for TokenDe { } Some(token) => { - return Err(InternalError::Spanned(Error::new( - token.span(), + return Err(InternalError::Spanned(spanned_error( + &token, format!("expected `=`, but found `{}`", token), ))); } None => { - return Err(InternalError::Spanned(Error::new( - keytok.span(), + return Err(InternalError::Spanned(spanned_error( + &keytok, format!("expected `=` following `{}`", keytok), ))); } @@ -971,7 +1060,7 @@ impl<'de> Deserializer<'de> for &mut TokenDe { ts.extend(vec![keytok.clone(), self.previous().unwrap()]); // Create an error that underlines key, =, and value. - let mut err = Error::new_spanned(ts, msg); + let mut err = spanned_error(ts, msg); // Add in the value error if there was one. if let Err(e2) = value { diff --git a/ui-tests/tests/ui/bad_string_from_macro_rules.rs b/ui-tests/tests/ui/bad_string_from_macro_rules.rs new file mode 100644 index 0000000..abd8595 --- /dev/null +++ b/ui-tests/tests/ui/bad_string_from_macro_rules.rs @@ -0,0 +1,22 @@ +// Copyright 2026 Oxide Computer Company + +// A macro_rules substitution that is not a string must be reported at the +// invocation, not the declaration. + +use testlib::annotation; + +macro_rules! wrap { + ($s:expr) => { + #[annotation { + string = $s, + options = OptionA, + unit = (), + tup = (1, 2.0), + }] + fn test() {} + }; +} + +wrap!(123); + +fn main() {} diff --git a/ui-tests/tests/ui/bad_string_from_macro_rules.stderr b/ui-tests/tests/ui/bad_string_from_macro_rules.stderr new file mode 100644 index 0000000..29993b5 --- /dev/null +++ b/ui-tests/tests/ui/bad_string_from_macro_rules.stderr @@ -0,0 +1,5 @@ +error: expected a string, but found `123` + --> tests/ui/bad_string_from_macro_rules.rs:20:7 + | +20 | wrap!(123); + | ^^^ diff --git a/ui-tests/tests/ui/bad_string_from_macro_rules_path.rs b/ui-tests/tests/ui/bad_string_from_macro_rules_path.rs new file mode 100644 index 0000000..efd2999 --- /dev/null +++ b/ui-tests/tests/ui/bad_string_from_macro_rules_path.rs @@ -0,0 +1,22 @@ +// Copyright 2026 Oxide Computer Company + +// A multi-token macro_rules substitution must be rejected, not truncated to its +// first token. + +use testlib::annotation; + +macro_rules! wrap { + ($s:expr) => { + #[annotation { + string = $s, + options = OptionA, + unit = (), + tup = (1, 2.0), + }] + fn test() {} + }; +} + +wrap!(foo::bar); + +fn main() {} diff --git a/ui-tests/tests/ui/bad_string_from_macro_rules_path.stderr b/ui-tests/tests/ui/bad_string_from_macro_rules_path.stderr new file mode 100644 index 0000000..f201cf1 --- /dev/null +++ b/ui-tests/tests/ui/bad_string_from_macro_rules_path.stderr @@ -0,0 +1,5 @@ +error: expected a string, but found `foo::bar` + --> tests/ui/bad_string_from_macro_rules_path.rs:20:7 + | +20 | wrap!(foo::bar); + | ^^^^^^^^ diff --git a/ui-tests/tests/ui/bad_string_from_nested_macro_rules.rs b/ui-tests/tests/ui/bad_string_from_nested_macro_rules.rs new file mode 100644 index 0000000..5c7e44c --- /dev/null +++ b/ui-tests/tests/ui/bad_string_from_nested_macro_rules.rs @@ -0,0 +1,28 @@ +// Copyright 2026 Oxide Computer Company + +// We must handle nested macro_rules substitutions the same way we handle +// single-level substitutions. + +use testlib::annotation; + +macro_rules! inner { + ($s:expr) => { + #[annotation { + string = $s, + options = OptionA, + unit = (), + tup = (1, 2.0), + }] + fn test() {} + }; +} + +macro_rules! outer { + ($s:expr) => { + inner!($s); + }; +} + +outer!(123); + +fn main() {} diff --git a/ui-tests/tests/ui/bad_string_from_nested_macro_rules.stderr b/ui-tests/tests/ui/bad_string_from_nested_macro_rules.stderr new file mode 100644 index 0000000..f7858c3 --- /dev/null +++ b/ui-tests/tests/ui/bad_string_from_nested_macro_rules.stderr @@ -0,0 +1,5 @@ +error: expected a string, but found `123` + --> tests/ui/bad_string_from_nested_macro_rules.rs:26:8 + | +26 | outer!(123); + | ^^^ diff --git a/ui-tests/tests/ui/bad_untagged_from_macro_rules.rs b/ui-tests/tests/ui/bad_untagged_from_macro_rules.rs new file mode 100644 index 0000000..67341a8 --- /dev/null +++ b/ui-tests/tests/ui/bad_untagged_from_macro_rules.rs @@ -0,0 +1,19 @@ +// Copyright 2026 Oxide Computer Company + +// When macro_rules substitutions are involved, failures must point at the +// invocation, not at the macro definition. + +use testlib::untagged_list; + +macro_rules! wrap { + ($e:expr) => { + #[untagged_list { + items = [$e], + }] + fn test() {} + }; +} + +wrap!({ c = 1 }); + +fn main() {} diff --git a/ui-tests/tests/ui/bad_untagged_from_macro_rules.stderr b/ui-tests/tests/ui/bad_untagged_from_macro_rules.stderr new file mode 100644 index 0000000..d8008cf --- /dev/null +++ b/ui-tests/tests/ui/bad_untagged_from_macro_rules.stderr @@ -0,0 +1,5 @@ +error: data did not match any variant of untagged enum UntaggedConfig + --> tests/ui/bad_untagged_from_macro_rules.rs:17:7 + | +17 | wrap!({ c = 1 }); + | ^^^^^^^^^