[PATCH 1/2] rust: macros: improve panic messages

Tamir Duberstein posted 2 patches 1 year ago
There is a newer version of this series
[PATCH 1/2] rust: macros: improve panic messages
Posted by Tamir Duberstein 1 year ago
Include unexpected input on parsing failures.  This has the side effect
of avoiding a spurious rust-analyzer warning:

  Variable `None` should have snake_case name, e.g. `none`

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/macros/module.rs | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index cdf94f4982df..ca1b7e6a71ff 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -11,12 +11,14 @@ fn expect_string_array(it: &mut token_stream::IntoIter) -> Vec<String> {
     let mut it = group.stream().into_iter();
 
     while let Some(val) = try_string(&mut it) {
-        assert!(val.is_ascii(), "Expected ASCII string");
+        assert!(val.is_ascii(), "Expected ASCII string, got {}", val);
         values.push(val);
-        match it.next() {
-            Some(TokenTree::Punct(punct)) => assert_eq!(punct.as_char(), ','),
-            None => break,
-            _ => panic!("Expected ',' or end of array"),
+        let Some(token) = it.next() else {
+            break;
+        };
+        match token {
+            TokenTree::Punct(punct) => assert_eq!(punct.as_char(), ','),
+            token => panic!("Expected ',' or end of array, got {}", token),
         }
     }
     values
@@ -116,11 +118,10 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
         const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
         let mut seen_keys = Vec::new();
 
-        loop {
-            let key = match it.next() {
-                Some(TokenTree::Ident(ident)) => ident.to_string(),
-                Some(_) => panic!("Expected Ident or end"),
-                None => break,
+        while let Some(token) = it.next() {
+            let key = match token {
+                TokenTree::Ident(ident) => ident.to_string(),
+                token => panic!("Expected Ident or end, got {}", token),
             };
 
             if seen_keys.contains(&key) {

-- 
2.48.1
Re: [PATCH 1/2] rust: macros: improve panic messages
Posted by Miguel Ojeda 1 year ago
On Fri, Feb 7, 2025 at 6:22 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Include unexpected input on parsing failures.  This has the side effect
> of avoiding a spurious rust-analyzer warning:
>
>   Variable `None` should have snake_case name, e.g. `none`

Hmm... That should be solved independently, but sure.

In any case, how is this related to the second patch in the series?
i.e. do you need both to solve the macOS issue?

> +        let Some(token) = it.next() else {
> +            break;
> +        };
> +        match token {
> +            TokenTree::Punct(punct) => assert_eq!(punct.as_char(), ','),
> +            token => panic!("Expected ',' or end of array, got {}", token),

Do we want to shadow here?

Also, I think you could write `{token}`. Same above.

Could you please show how the new output would look like in the commit message?

Cheers,
Miguel
Re: [PATCH 1/2] rust: macros: improve panic messages
Posted by Tamir Duberstein 1 year ago
On Fri, Feb 7, 2025 at 1:12 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Feb 7, 2025 at 6:22 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Include unexpected input on parsing failures.  This has the side effect
> > of avoiding a spurious rust-analyzer warning:
> >
> >   Variable `None` should have snake_case name, e.g. `none`
>
> Hmm... That should be solved independently, but sure.
>
> In any case, how is this related to the second patch in the series?
> i.e. do you need both to solve the macOS issue?

No, it's not required - you're right to point that out. I'll send it
separately. Is it very inconvenient to consider the other patch on its
own?

> > +        let Some(token) = it.next() else {
> > +            break;
> > +        };
> > +        match token {
> > +            TokenTree::Punct(punct) => assert_eq!(punct.as_char(), ','),
> > +            token => panic!("Expected ',' or end of array, got {}", token),
>
> Do we want to shadow here?

I personally like this style because the shadow and shadowed are the
same variable and the presence of a binding in the match expression
hints to the reader that it's used somewhere in the body of the match
arm. Do you have another preference?

> Also, I think you could write `{token}`. Same above.

Good call. Will make it so.

> Could you please show how the new output would look like in the commit message?

Yes, I will include before and after output.

> Cheers,
> Miguel