From 8e1f660e1db77434d96626d3e40ecd0edfc71b54 Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Sat, 15 Jan 2022 22:14:43 +0000 Subject: [PATCH] Don't panic in macro shape validation (#3647) # Objective Emitting compile errors produces cleaner messages than panicking in a proc-macro. ## Solution - Replace match-with-panic code with call to new `bevy_macro_utils::get_named_struct_fields` function - Replace one use of match-with-panic for enums with inline match _Aside:_ I'm also the maintainer of [`darling`](https://docs.rs/darling), a crate which provides a serde-like API for parsing macro inputs. I avoided using it here because it seemed like overkill, but if there are plans to add lots more attributes/macros then that might be a good way of offloading macro error handling. --- .../bevy-crevice-derive/src/glsl.rs | 13 ++++------- .../bevy-crevice-derive/src/layout.rs | 13 ++++------- crates/bevy_derive/src/enum_variant_meta.rs | 8 +++++-- crates/bevy_ecs/macros/src/lib.rs | 23 +++++++------------ crates/bevy_macro_utils/src/lib.rs | 2 ++ crates/bevy_macro_utils/src/shape.rs | 20 ++++++++++++++++ 6 files changed, 46 insertions(+), 33 deletions(-) create mode 100644 crates/bevy_macro_utils/src/shape.rs diff --git a/crates/bevy_crevice/bevy-crevice-derive/src/glsl.rs b/crates/bevy_crevice/bevy-crevice-derive/src/glsl.rs index e81442bf45..dee33477c2 100644 --- a/crates/bevy_crevice/bevy-crevice-derive/src/glsl.rs +++ b/crates/bevy_crevice/bevy-crevice-derive/src/glsl.rs @@ -1,17 +1,14 @@ +use bevy_macro_utils::get_named_struct_fields; use proc_macro2::{Literal, TokenStream}; use quote::quote; -use syn::{parse_quote, Data, DeriveInput, Fields, Path}; +use syn::{parse_quote, DeriveInput, Path}; pub fn emit(input: DeriveInput) -> TokenStream { let bevy_crevice_path = crate::bevy_crevice_path(); - let fields = match &input.data { - Data::Struct(data) => match &data.fields { - Fields::Named(fields) => fields, - Fields::Unnamed(_) => panic!("Tuple structs are not supported"), - Fields::Unit => panic!("Unit structs are not supported"), - }, - Data::Enum(_) | Data::Union(_) => panic!("Only structs are supported"), + let fields = match get_named_struct_fields(&input.data) { + Ok(fields) => fields, + Err(e) => return e.into_compile_error(), }; let base_trait_path: Path = parse_quote!(#bevy_crevice_path::glsl::Glsl); diff --git a/crates/bevy_crevice/bevy-crevice-derive/src/layout.rs b/crates/bevy_crevice/bevy-crevice-derive/src/layout.rs index 74c04ad110..899343632b 100644 --- a/crates/bevy_crevice/bevy-crevice-derive/src/layout.rs +++ b/crates/bevy_crevice/bevy-crevice-derive/src/layout.rs @@ -1,6 +1,7 @@ +use bevy_macro_utils::get_named_struct_fields; use proc_macro2::{Span, TokenStream}; use quote::{format_ident, quote}; -use syn::{parse_quote, Data, DeriveInput, Fields, Ident, Path, Type}; +use syn::{parse_quote, DeriveInput, Ident, Path, Type}; pub fn emit( input: DeriveInput, @@ -32,13 +33,9 @@ pub fn emit( // Crevice's derive only works on regular structs. We could potentially // support transparent tuple structs in the future. - let fields: Vec<_> = match &input.data { - Data::Struct(data) => match &data.fields { - Fields::Named(fields) => fields.named.iter().collect(), - Fields::Unnamed(_) => panic!("Tuple structs are not supported"), - Fields::Unit => panic!("Unit structs are not supported"), - }, - Data::Enum(_) | Data::Union(_) => panic!("Only structs are supported"), + let fields: Vec<_> = match get_named_struct_fields(&input.data) { + Ok(fields) => fields.named.iter().collect(), + Err(e) => return e.into_compile_error(), }; // Gives the layout-specific version of the given type. diff --git a/crates/bevy_derive/src/enum_variant_meta.rs b/crates/bevy_derive/src/enum_variant_meta.rs index ae0c26d2ea..415c6f291c 100644 --- a/crates/bevy_derive/src/enum_variant_meta.rs +++ b/crates/bevy_derive/src/enum_variant_meta.rs @@ -1,5 +1,5 @@ use bevy_macro_utils::BevyManifest; -use proc_macro::TokenStream; +use proc_macro::{Span, TokenStream}; use quote::quote; use syn::{parse_macro_input, Data, DeriveInput}; @@ -7,7 +7,11 @@ pub fn derive_enum_variant_meta(input: TokenStream) -> TokenStream { let ast = parse_macro_input!(input as DeriveInput); let variants = match &ast.data { Data::Enum(v) => &v.variants, - _ => panic!("Expected an enum."), + _ => { + return syn::Error::new(Span::call_site().into(), "Only enums are supported") + .into_compile_error() + .into() + } }; let bevy_util_path = BevyManifest::default().get_path(crate::modules::BEVY_UTILS); diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index ed6fd79f95..548f2310a1 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -2,7 +2,7 @@ extern crate proc_macro; mod component; -use bevy_macro_utils::{derive_label, BevyManifest}; +use bevy_macro_utils::{derive_label, get_named_struct_fields, BevyManifest}; use proc_macro::TokenStream; use proc_macro2::Span; use quote::{format_ident, quote}; @@ -11,8 +11,7 @@ use syn::{ parse_macro_input, punctuated::Punctuated, token::Comma, - Data, DataStruct, DeriveInput, Field, Fields, GenericParam, Ident, Index, LitInt, Result, - Token, + DeriveInput, Field, GenericParam, Ident, Index, LitInt, Result, Token, }; struct AllTuples { @@ -86,12 +85,9 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { let ast = parse_macro_input!(input as DeriveInput); let ecs_path = bevy_ecs_path(); - let named_fields = match &ast.data { - Data::Struct(DataStruct { - fields: Fields::Named(fields), - .. - }) => &fields.named, - _ => panic!("Expected a struct with named fields."), + let named_fields = match get_named_struct_fields(&ast.data) { + Ok(fields) => &fields.named, + Err(e) => return e.into_compile_error().into(), }; let is_bundle = named_fields @@ -304,12 +300,9 @@ static SYSTEM_PARAM_ATTRIBUTE_NAME: &str = "system_param"; #[proc_macro_derive(SystemParam, attributes(system_param))] pub fn derive_system_param(input: TokenStream) -> TokenStream { let ast = parse_macro_input!(input as DeriveInput); - let fields = match &ast.data { - Data::Struct(DataStruct { - fields: Fields::Named(fields), - .. - }) => &fields.named, - _ => panic!("Expected a struct with named fields."), + let fields = match get_named_struct_fields(&ast.data) { + Ok(fields) => &fields.named, + Err(e) => return e.into_compile_error().into(), }; let path = bevy_ecs_path(); diff --git a/crates/bevy_macro_utils/src/lib.rs b/crates/bevy_macro_utils/src/lib.rs index d59a926496..03a0aa11a0 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -1,9 +1,11 @@ extern crate proc_macro; mod attrs; +mod shape; mod symbol; pub use attrs::*; +pub use shape::*; pub use symbol::*; use cargo_manifest::{DepsSet, Manifest}; diff --git a/crates/bevy_macro_utils/src/shape.rs b/crates/bevy_macro_utils/src/shape.rs new file mode 100644 index 0000000000..36ce443bf4 --- /dev/null +++ b/crates/bevy_macro_utils/src/shape.rs @@ -0,0 +1,20 @@ +use proc_macro::Span; +use syn::{Data, DataStruct, Error, Fields, FieldsNamed}; + +/// Get the fields of a data structure if that structure is a struct with named fields; +/// otherwise, return a compile error that points to the site of the macro invocation. +pub fn get_named_struct_fields(data: &syn::Data) -> syn::Result<&FieldsNamed> { + match data { + Data::Struct(DataStruct { + fields: Fields::Named(fields), + .. + }) => Ok(fields), + _ => Err(Error::new( + // This deliberately points to the call site rather than the structure + // body; marking the entire body as the source of the error makes it + // impossible to figure out which `derive` has a problem. + Span::call_site().into(), + "Only structs with named fields are supported", + )), + } +}