From b0c0b41dd0b4bd93ae4ddd5e58b2e9045180b5d1 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Tue, 19 Jul 2022 05:21:19 +0000 Subject: [PATCH] Add attribute to ignore fields of derived labels (#5366) # Objective Fixes #5362 ## Solution Add the attribute `#[label(ignore_fields)]` for `*Label` types. ```rust #[derive(SystemLabel)] pub enum MyLabel { One, // Previously this was not allowed since labels cannot contain data. #[system_label(ignore_fields)] Two(PhantomData), } ``` ## Notes This label makes it possible for equality to behave differently depending on whether or not you are treating the type as a label. For example: ```rust #[derive(SystemLabel, PartialEq, Eq)] #[system_label(ignore_fields)] pub struct Foo(usize); ``` If you compare it as a label, it will ignore the wrapped fields as the user requested. But if you compare it as a `Foo`, the derive will incorrectly compare the inner fields. I see a few solutions 1. Do nothing. This is technically intended behavior, but I think we should do our best to prevent footguns. 2. Generate impls of `PartialEq` and `Eq` along with the `#[derive(Label)]` macros. This is a breaking change as it requires all users to remove these derives from their types. 3. Only allow `PhantomData` to be used with `ignore_fields` -- seems needlessly prescriptive. --- ## Changelog * Added the `ignore_fields` attribute to the derive macros for `*Label` types. * Added an example showing off different forms of the derive macro. --- crates/bevy_derive/src/lib.rs | 8 ++- crates/bevy_ecs/Cargo.toml | 4 ++ crates/bevy_ecs/examples/derive_label.rs | 62 ++++++++++++++++++ crates/bevy_ecs/macros/src/lib.rs | 32 ++++++--- crates/bevy_macro_utils/src/lib.rs | 82 ++++++++++++++++++++---- 5 files changed, 166 insertions(+), 22 deletions(-) create mode 100644 crates/bevy_ecs/examples/derive_label.rs diff --git a/crates/bevy_derive/src/lib.rs b/crates/bevy_derive/src/lib.rs index 3c43776ef9961..e2088cfe04ec1 100644 --- a/crates/bevy_derive/src/lib.rs +++ b/crates/bevy_derive/src/lib.rs @@ -80,10 +80,14 @@ pub fn derive_enum_variant_meta(input: TokenStream) -> TokenStream { enum_variant_meta::derive_enum_variant_meta(input) } -#[proc_macro_derive(AppLabel)] +/// Generates an impl of the `AppLabel` trait. +/// +/// This works only for unit structs, or enums with only unit variants. +/// You may force a struct or variant to behave as if it were fieldless with `#[app_label(ignore_fields)]`. +#[proc_macro_derive(AppLabel, attributes(app_label))] pub fn derive_app_label(input: TokenStream) -> TokenStream { let input = syn::parse_macro_input!(input as syn::DeriveInput); let mut trait_path = BevyManifest::default().get_path("bevy_app"); trait_path.segments.push(format_ident!("AppLabel").into()); - derive_label(input, &trait_path) + derive_label(input, &trait_path, "app_label") } diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index 537afbd1dc506..91951d2302a3e 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -42,3 +42,7 @@ path = "examples/resources.rs" [[example]] name = "change_detection" path = "examples/change_detection.rs" + +[[example]] +name = "derive_label" +path = "examples/derive_label.rs" diff --git a/crates/bevy_ecs/examples/derive_label.rs b/crates/bevy_ecs/examples/derive_label.rs new file mode 100644 index 0000000000000..573b42dc8153d --- /dev/null +++ b/crates/bevy_ecs/examples/derive_label.rs @@ -0,0 +1,62 @@ +use std::marker::PhantomData; + +use bevy_ecs::prelude::*; + +fn main() { + // Unit labels are always equal. + assert_eq!(UnitLabel.as_label(), UnitLabel.as_label()); + + // Enum labels depend on the variant. + assert_eq!(EnumLabel::One.as_label(), EnumLabel::One.as_label()); + assert_ne!(EnumLabel::One.as_label(), EnumLabel::Two.as_label()); + + // Labels annotated with `ignore_fields` ignore their fields. + assert_eq!(WeirdLabel(1).as_label(), WeirdLabel(2).as_label()); + + // Labels don't depend only on the variant name but on the full type + assert_ne!( + GenericLabel::::One.as_label(), + GenericLabel::::One.as_label(), + ); +} + +#[derive(SystemLabel)] +pub struct UnitLabel; + +#[derive(SystemLabel)] +pub enum EnumLabel { + One, + Two, +} + +#[derive(SystemLabel)] +#[system_label(ignore_fields)] +pub struct WeirdLabel(i32); + +#[derive(SystemLabel)] +pub enum GenericLabel { + One, + #[system_label(ignore_fields)] + Two(PhantomData), +} + +// FIXME: this should be a compile_fail test +/*#[derive(SystemLabel)] +pub union Foo { + x: i32, +}*/ + +// FIXME: this should be a compile_fail test +/*#[derive(SystemLabel)] +#[system_label(ignore_fields)] +pub enum BadLabel { + One, + Two, +}*/ + +// FIXME: this should be a compile_fail test +/*#[derive(SystemLabel)] +pub struct BadLabel2 { + #[system_label(ignore_fields)] + x: (), +}*/ diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 5722872df15e2..68023e315ddb0 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -434,7 +434,11 @@ pub fn derive_world_query(input: TokenStream) -> TokenStream { derive_world_query_impl(ast) } -#[proc_macro_derive(SystemLabel)] +/// Generates an impl of the `SystemLabel` trait. +/// +/// This works only for unit structs, or enums with only unit variants. +/// You may force a struct or variant to behave as if it were fieldless with `#[system_label(ignore_fields)]`. +#[proc_macro_derive(SystemLabel, attributes(system_label))] pub fn derive_system_label(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); let mut trait_path = bevy_ecs_path(); @@ -442,19 +446,27 @@ pub fn derive_system_label(input: TokenStream) -> TokenStream { trait_path .segments .push(format_ident!("SystemLabel").into()); - derive_label(input, &trait_path) + derive_label(input, &trait_path, "system_label") } -#[proc_macro_derive(StageLabel)] +/// Generates an impl of the `StageLabel` trait. +/// +/// This works only for unit structs, or enums with only unit variants. +/// You may force a struct or variant to behave as if it were fieldless with `#[stage_label(ignore_fields)]`. +#[proc_macro_derive(StageLabel, attributes(stage_label))] pub fn derive_stage_label(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); let mut trait_path = bevy_ecs_path(); trait_path.segments.push(format_ident!("schedule").into()); trait_path.segments.push(format_ident!("StageLabel").into()); - derive_label(input, &trait_path) + derive_label(input, &trait_path, "stage_label") } -#[proc_macro_derive(AmbiguitySetLabel)] +/// Generates an impl of the `AmbiguitySetLabel` trait. +/// +/// This works only for unit structs, or enums with only unit variants. +/// You may force a struct or variant to behave as if it were fieldless with `#[ambiguity_set_label(ignore_fields)]`. +#[proc_macro_derive(AmbiguitySetLabel, attributes(ambiguity_set_label))] pub fn derive_ambiguity_set_label(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); let mut trait_path = bevy_ecs_path(); @@ -462,10 +474,14 @@ pub fn derive_ambiguity_set_label(input: TokenStream) -> TokenStream { trait_path .segments .push(format_ident!("AmbiguitySetLabel").into()); - derive_label(input, &trait_path) + derive_label(input, &trait_path, "ambiguity_set_label") } -#[proc_macro_derive(RunCriteriaLabel)] +/// Generates an impl of the `RunCriteriaLabel` trait. +/// +/// This works only for unit structs, or enums with only unit variants. +/// You may force a struct or variant to behave as if it were fieldless with `#[run_criteria_label(ignore_fields)]`. +#[proc_macro_derive(RunCriteriaLabel, attributes(run_criteria_label))] pub fn derive_run_criteria_label(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); let mut trait_path = bevy_ecs_path(); @@ -473,7 +489,7 @@ pub fn derive_run_criteria_label(input: TokenStream) -> TokenStream { trait_path .segments .push(format_ident!("RunCriteriaLabel").into()); - derive_label(input, &trait_path) + derive_label(input, &trait_path, "run_criteria_label") } pub(crate) fn bevy_ecs_path() -> syn::Path { diff --git a/crates/bevy_macro_utils/src/lib.rs b/crates/bevy_macro_utils/src/lib.rs index 589c7c3372af4..0af86d052a946 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -9,8 +9,9 @@ pub use shape::*; pub use symbol::*; use proc_macro::TokenStream; -use quote::quote; +use quote::{quote, quote_spanned}; use std::{env, path::PathBuf}; +use syn::spanned::Spanned; use toml::{map::Map, Value}; pub struct BevyManifest { @@ -110,8 +111,26 @@ impl BevyManifest { /// /// - `input`: The [`syn::DeriveInput`] for struct that is deriving the label trait /// - `trait_path`: The path [`syn::Path`] to the label trait -pub fn derive_label(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStream { - let ident = input.ident; +pub fn derive_label( + input: syn::DeriveInput, + trait_path: &syn::Path, + attr_name: &str, +) -> TokenStream { + // return true if the variant specified is an `ignore_fields` attribute + fn is_ignore(attr: &syn::Attribute, attr_name: &str) -> bool { + if attr.path.get_ident().as_ref().unwrap() != &attr_name { + return false; + } + + syn::custom_keyword!(ignore_fields); + attr.parse_args_with(|input: syn::parse::ParseStream| { + let ignore = input.parse::>()?.is_some(); + Ok(ignore) + }) + .unwrap() + } + + let ident = input.ident.clone(); let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); let mut where_clause = where_clause.cloned().unwrap_or_else(|| syn::WhereClause { @@ -123,22 +142,56 @@ pub fn derive_label(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStr .push(syn::parse2(quote! { Self: 'static }).unwrap()); let as_str = match input.data { - syn::Data::Struct(d) => match d.fields { - syn::Fields::Unit => { + syn::Data::Struct(d) => { + // see if the user tried to ignore fields incorrectly + if let Some(attr) = d + .fields + .iter() + .flat_map(|f| &f.attrs) + .find(|a| is_ignore(a, attr_name)) + { + let err_msg = format!("`#[{attr_name}(ignore_fields)]` cannot be applied to fields individually: add it to the struct declaration"); + return quote_spanned! { + attr.span() => compile_error!(#err_msg); + } + .into(); + } + // Structs must either be fieldless, or explicitly ignore the fields. + let ignore_fields = input.attrs.iter().any(|a| is_ignore(a, attr_name)); + if matches!(d.fields, syn::Fields::Unit) || ignore_fields { let lit = ident.to_string(); quote! { #lit } + } else { + let err_msg = format!("Labels cannot contain data, unless explicitly ignored with `#[{attr_name}(ignore_fields)]`"); + return quote_spanned! { + d.fields.span() => compile_error!(#err_msg); + } + .into(); } - _ => panic!("Labels cannot contain data."), - }, + } syn::Data::Enum(d) => { - let arms = d.variants.iter().map(|v| match v.fields { - syn::Fields::Unit => { + // check if the user put #[label(ignore_fields)] in the wrong place + if let Some(attr) = input.attrs.iter().find(|a| is_ignore(a, attr_name)) { + let err_msg = format!("`#[{attr_name}(ignore_fields)]` can only be applied to enum variants or struct declarations"); + return quote_spanned! { + attr.span() => compile_error!(#err_msg); + } + .into(); + } + let arms = d.variants.iter().map(|v| { + // Variants must either be fieldless, or explicitly ignore the fields. + let ignore_fields = v.attrs.iter().any(|a| is_ignore(a, attr_name)); + if matches!(v.fields, syn::Fields::Unit) | ignore_fields { let mut path = syn::Path::from(ident.clone()); path.segments.push(v.ident.clone().into()); let lit = format!("{ident}::{}", v.ident.clone()); - quote! { #path => #lit } + quote! { #path { .. } => #lit } + } else { + let err_msg = format!("Label variants cannot contain data, unless explicitly ignored with `#[{attr_name}(ignore_fields)]`"); + quote_spanned! { + v.fields.span() => _ => { compile_error!(#err_msg); } + } } - _ => panic!("Label variants cannot contain data."), }); quote! { match self { @@ -146,7 +199,12 @@ pub fn derive_label(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStr } } } - syn::Data::Union(_) => panic!("Unions cannot be used as labels."), + syn::Data::Union(_) => { + return quote_spanned! { + input.span() => compile_error!("Unions cannot be used as labels."); + } + .into(); + } }; (quote! {