From adcc10b8412a9c80b1dafbb3d6514d4d8d05533f Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 15 May 2023 14:15:14 +0200 Subject: [PATCH] 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 | 86 ++++++------------- swarm-derive/src/syn_ext.rs | 16 ++++ swarm/tests/ui/fail/prelude_not_string.rs | 11 +++ swarm/tests/ui/fail/prelude_not_string.stderr | 5 ++ swarm/tests/ui/fail/to_swarm_not_string.rs | 19 ++++ .../tests/ui/fail/to_swarm_not_string.stderr | 5 ++ 6 files changed, 81 insertions(+), 61 deletions(-) create mode 100644 swarm-derive/src/syn_ext.rs create mode 100644 swarm/tests/ui/fail/prelude_not_string.rs create mode 100644 swarm/tests/ui/fail/prelude_not_string.stderr create mode 100644 swarm/tests/ui/fail/to_swarm_not_string.rs create mode 100644 swarm/tests/ui/fail/to_swarm_not_string.stderr diff --git a/swarm-derive/src/lib.rs b/swarm-derive/src/lib.rs index 77f25aad..8bcf90e8 100644 --- a/swarm-derive/src/lib.rs +++ b/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::{ - parse_macro_input, Data, DataStruct, DeriveInput, Expr, ExprLit, Lit, Meta, MetaNameValue, - Token, -}; +use syn::{parse_macro_input, Data, DataStruct, DeriveInput, Meta, 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 { match ast.data { Data::Struct(ref s) => build_struct(ast, s), - Data::Enum(_) => unimplemented!("Deriving NetworkBehaviour is not implemented for enums"), - Data::Union(_) => unimplemented!("Deriving NetworkBehaviour is not implemented for unions"), + Data::Enum(_) => Err(syn::Error::new_spanned( + 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 { 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) { - Ok(attrs) => attrs, - Err(e) => return e, - }; + } = parse_attributes(ast)?; 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 { +fn parse_attributes(ast: &DeriveInput) -> syn::Result { 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::parse_terminated) - .expect("`parse_args_with` never fails when parsing nested meta"); + let nested = attr.parse_args_with(Punctuated::::parse_terminated)?; for meta in nested { if meta.path().is_ident("prelude") { - match meta { - Meta::Path(_) => unimplemented!(), - Meta::List(_) => unimplemented!(), - 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()); - } - } + let value = meta.require_name_value()?.value.require_str_lit()?; + + attributes.prelude_path = syn::parse_str(&value)?; continue; } @@ -912,29 +895,10 @@ fn parse_attributes(ast: &DeriveInput) -> Result unimplemented!(), - Meta::List(_) => unimplemented!(), - Meta::NameValue(MetaNameValue { - value: - Expr::Lit(ExprLit { - 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()); - } - } + let value = meta.require_name_value()?.value.require_str_lit()?; + + attributes.user_specified_out_event = Some(syn::parse_str(&value)?); continue; } diff --git a/swarm-derive/src/syn_ext.rs b/swarm-derive/src/syn_ext.rs new file mode 100644 index 00000000..d57a8a5d --- /dev/null +++ b/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; +} + +impl RequireStrLit for Expr { + fn require_str_lit(&self) -> syn::Result { + match self { + Expr::Lit(ExprLit { + lit: Lit::Str(str), .. + }) => Ok(str.value()), + _ => Err(syn::Error::new_spanned(self, "expected a string literal")), + } + } +} diff --git a/swarm/tests/ui/fail/prelude_not_string.rs b/swarm/tests/ui/fail/prelude_not_string.rs new file mode 100644 index 00000000..727f2439 --- /dev/null +++ b/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() { + +} diff --git a/swarm/tests/ui/fail/prelude_not_string.stderr b/swarm/tests/ui/fail/prelude_not_string.stderr new file mode 100644 index 00000000..2c2a7980 --- /dev/null +++ b/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)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/swarm/tests/ui/fail/to_swarm_not_string.rs b/swarm/tests/ui/fail/to_swarm_not_string.rs new file mode 100644 index 00000000..e0ff56e4 --- /dev/null +++ b/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 for FooEvent { + fn from(_: ping::Event) -> Self { + unimplemented!() + } +} + +fn main() { + +} diff --git a/swarm/tests/ui/fail/to_swarm_not_string.stderr b/swarm/tests/ui/fail/to_swarm_not_string.stderr new file mode 100644 index 00000000..f2fbba68 --- /dev/null +++ b/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")] + | ^^^^^^^^