refactor(swarm-derive): better error reporting for invalid attributes

This PR refactors the error reporting away from panicking to returning `syn::Result` and adds two unit tests for the parsing of attributes that users interact with.

Pull-Request: #3922.

swarm-derive/src/lib.rs

@@ -21,45 +21,48 @@
 #![recursion_limit = "256"]
 #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
 
+mod syn_ext;
+
+use crate::syn_ext::RequireStrLit;
 use heck::ToUpperCamelCase;
 use proc_macro::TokenStream;
 use quote::quote;
 use syn::punctuated::Punctuated;
 use syn::spanned::Spanned;
-use syn::{
+use syn::{parse_macro_input, Data, DataStruct, DeriveInput, Meta, Token};
-    parse_macro_input, Data, DataStruct, DeriveInput, Expr, ExprLit, Lit, Meta, MetaNameValue,
-    Token,
-};
 
 /// Generates a delegating `NetworkBehaviour` implementation for the struct this is used for. See
 /// the trait documentation for better description.
 #[proc_macro_derive(NetworkBehaviour, attributes(behaviour))]
 pub fn hello_macro_derive(input: TokenStream) -> TokenStream {
     let ast = parse_macro_input!(input as DeriveInput);
-    build(&ast)
+    build(&ast).unwrap_or_else(|e| e.to_compile_error().into())
 }
 
 /// The actual implementation.
-fn build(ast: &DeriveInput) -> TokenStream {
+fn build(ast: &DeriveInput) -> syn::Result<TokenStream> {
     match ast.data {
         Data::Struct(ref s) => build_struct(ast, s),
-        Data::Enum(_) => unimplemented!("Deriving NetworkBehaviour is not implemented for enums"),
+        Data::Enum(_) => Err(syn::Error::new_spanned(
-        Data::Union(_) => unimplemented!("Deriving NetworkBehaviour is not implemented for unions"),
+            ast,
+            "Cannot derive `NetworkBehaviour` on enums",
+        )),
+        Data::Union(_) => Err(syn::Error::new_spanned(
+            ast,
+            "Cannot derive `NetworkBehaviour` on union",
+        )),
     }
 }
 
 /// The version for structs
-fn build_struct(ast: &DeriveInput, data_struct: &DataStruct) -> TokenStream {
+fn build_struct(ast: &DeriveInput, data_struct: &DataStruct) -> syn::Result<TokenStream> {
     let name = &ast.ident;
     let (_, ty_generics, where_clause) = ast.generics.split_for_impl();
     let BehaviourAttributes {
         prelude_path,
         user_specified_out_event,
         deprecation_tokenstream,
-    } = match parse_attributes(ast) {
+    } = parse_attributes(ast)?;
-        Ok(attrs) => attrs,
-        Err(e) => return e,
-    };
 
     let multiaddr = quote! { #prelude_path::Multiaddr };
     let trait_to_impl = quote! { #prelude_path::NetworkBehaviour };
@@ -849,7 +852,7 @@ fn build_struct(ast: &DeriveInput, data_struct: &DataStruct) -> TokenStream {
         }
     };
 
-    final_quote.into()
+    Ok(final_quote.into())
 }
 
 struct BehaviourAttributes {
@@ -859,7 +862,7 @@ struct BehaviourAttributes {
 }
 
 /// Parses the `value` of a key=value pair in the `#[behaviour]` attribute into the requested type.
-fn parse_attributes(ast: &DeriveInput) -> Result<BehaviourAttributes, TokenStream> {
+fn parse_attributes(ast: &DeriveInput) -> syn::Result<BehaviourAttributes> {
     let mut attributes = BehaviourAttributes {
         prelude_path: syn::parse_quote! { ::libp2p::swarm::derive_prelude },
         user_specified_out_event: None,
@@ -871,33 +874,13 @@ fn parse_attributes(ast: &DeriveInput) -> Result<BehaviourAttributes, TokenStrea
         .iter()
         .filter(|attr| attr.path().is_ident("behaviour"))
     {
-        let nested = attr
+        let nested = attr.parse_args_with(Punctuated::<Meta, Token![,]>::parse_terminated)?;
-            .parse_args_with(Punctuated::<Meta, Token![,]>::parse_terminated)
-            .expect("`parse_args_with` never fails when parsing nested meta");
 
         for meta in nested {
             if meta.path().is_ident("prelude") {
-                match meta {
+                let value = meta.require_name_value()?.value.require_str_lit()?;
-                    Meta::Path(_) => unimplemented!(),
+
-                    Meta::List(_) => unimplemented!(),
+                attributes.prelude_path = syn::parse_str(&value)?;
-                    Meta::NameValue(MetaNameValue {
-                        value:
-                            Expr::Lit(ExprLit {
-                                lit: Lit::Str(s), ..
-                            }),
-                        ..
-                    }) => {
-                        attributes.prelude_path = syn::parse_str(&s.value()).unwrap();
-                    }
-                    Meta::NameValue(name_value) => {
-                        return Err(syn::Error::new_spanned(
-                            name_value.value,
-                            "`prelude` value must be a quoted path",
-                        )
-                        .to_compile_error()
-                        .into());
-                    }
-                }
 
                 continue;
             }
@@ -912,29 +895,10 @@ fn parse_attributes(ast: &DeriveInput) -> Result<BehaviourAttributes, TokenStrea
 
                     attributes.deprecation_tokenstream = quote::quote! { #warning };
                 }
-                match meta {
-                    Meta::Path(_) => unimplemented!(),
-                    Meta::List(_) => unimplemented!(),
 
-                    Meta::NameValue(MetaNameValue {
+                let value = meta.require_name_value()?.value.require_str_lit()?;
-                        value:
+
-                            Expr::Lit(ExprLit {
+                attributes.user_specified_out_event = Some(syn::parse_str(&value)?);
-                                lit: Lit::Str(s), ..
-                            }),
-                        ..
-                    }) => {
-                        attributes.user_specified_out_event =
-                            Some(syn::parse_str(&s.value()).unwrap());
-                    }
-                    Meta::NameValue(name_value) => {
-                        return Err(syn::Error::new_spanned(
-                            name_value.value,
-                            "`to_swarm` value must be a quoted type",
-                        )
-                        .to_compile_error()
-                        .into());
-                    }
-                }
 
                 continue;
             }

swarm-derive/src/syn_ext.rs

@@ -0,0 +1,16 @@
+use syn::{Expr, ExprLit, Lit};
+
+pub(crate) trait RequireStrLit {
+    fn require_str_lit(&self) -> syn::Result<String>;
+}
+
+impl RequireStrLit for Expr {
+    fn require_str_lit(&self) -> syn::Result<String> {
+        match self {
+            Expr::Lit(ExprLit {
+                lit: Lit::Str(str), ..
+            }) => Ok(str.value()),
+            _ => Err(syn::Error::new_spanned(self, "expected a string literal")),
+        }
+    }
+}

swarm/tests/ui/fail/prelude_not_string.rs

@@ -0,0 +1,11 @@
+use libp2p_ping as ping;
+
+#[derive(libp2p_swarm::NetworkBehaviour)]
+#[behaviour(prelude = libp2p_swarm::derive_prelude)]
+struct Foo {
+    ping: ping::Behaviour,
+}
+
+fn main() {
+
+}

swarm/tests/ui/fail/prelude_not_string.stderr

@@ -0,0 +1,5 @@
+error: expected a string literal
+ --> tests/ui/fail/prelude_not_string.rs:4:23
+  |
+4 | #[behaviour(prelude = libp2p_swarm::derive_prelude)]
+  |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

swarm/tests/ui/fail/to_swarm_not_string.rs

@@ -0,0 +1,19 @@
+use libp2p_ping as ping;
+
+#[derive(libp2p_swarm::NetworkBehaviour)]
+#[behaviour(out_event = FooEvent, prelude = "libp2p_swarm::derive_prelude")]
+struct Foo {
+    ping: ping::Behaviour,
+}
+
+struct FooEvent;
+
+impl From<ping::Event> for FooEvent {
+    fn from(_: ping::Event) -> Self {
+        unimplemented!()
+    }
+}
+
+fn main() {
+
+}

swarm/tests/ui/fail/to_swarm_not_string.stderr

@@ -0,0 +1,5 @@
+error: expected a string literal
+ --> tests/ui/fail/to_swarm_not_string.rs:4:25
+  |
+4 | #[behaviour(out_event = FooEvent, prelude = "libp2p_swarm::derive_prelude")]
+  |                         ^^^^^^^^