Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bevy_reflect: Opt-out attribute for TypePath #9140

Merged
merged 2 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 82 additions & 21 deletions crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use syn::parse::{Parse, ParseStream};
use syn::punctuated::Punctuated;
use syn::spanned::Spanned;
use syn::token::Comma;
use syn::{LitBool, Meta, Path};
use syn::{Expr, LitBool, Meta, Path};

// The "special" trait idents that are used internally for reflection.
// Received via attributes like `#[reflect(PartialEq, Hash, ...)]`
Expand All @@ -28,6 +28,9 @@ pub(crate) const REFLECT_DEFAULT: &str = "ReflectDefault";
// Attributes for `FromReflect` implementation
const FROM_REFLECT_ATTR: &str = "from_reflect";

// Attributes for `TypePath` implementation
const TYPE_PATH_ATTR: &str = "type_path";

// The error message to show when a trait/type is specified multiple times
const CONFLICTING_TYPE_DATA_MESSAGE: &str = "conflicting type data registration";

Expand Down Expand Up @@ -87,7 +90,48 @@ impl FromReflectAttrs {
if existing.value() != new.value() {
return Err(syn::Error::new(
new.span(),
format!("`from_reflect` already set to {}", existing.value()),
format!("`{FROM_REFLECT_ATTR}` already set to {}", existing.value()),
));
}
} else {
self.auto_derive = Some(new);
}
}

Ok(())
}
}

/// A collection of attributes used for deriving `TypePath` via the `Reflect` derive.
///
/// Note that this differs from the attributes used by the `TypePath` derive itself,
/// which look like `[type_path = "my_crate::foo"]`.
/// The attributes used by reflection take the form `#[reflect(type_path = false)]`.
///
/// These attributes should only be used for `TypePath` configuration specific to
/// deriving `Reflect`.
#[derive(Clone, Default)]
pub(crate) struct TypePathAttrs {
auto_derive: Option<LitBool>,
}

impl TypePathAttrs {
/// Returns true if `TypePath` should be automatically derived as part of the `Reflect` derive.
pub fn should_auto_derive(&self) -> bool {
self.auto_derive
.as_ref()
.map(|lit| lit.value())
.unwrap_or(true)
}

/// Merges this [`TypePathAttrs`] with another.
pub fn merge(&mut self, other: TypePathAttrs) -> Result<(), syn::Error> {
if let Some(new) = other.auto_derive {
if let Some(existing) = &self.auto_derive {
if existing.value() != new.value() {
return Err(syn::Error::new(
new.span(),
format!("`{TYPE_PATH_ATTR}` already set to {}", existing.value()),
));
}
} else {
Expand Down Expand Up @@ -165,7 +209,8 @@ pub(crate) struct ReflectTraits {
debug: TraitImpl,
hash: TraitImpl,
partial_eq: TraitImpl,
from_reflect: FromReflectAttrs,
from_reflect_attrs: FromReflectAttrs,
type_path_attrs: TypePathAttrs,
idents: Vec<Ident>,
}

Expand Down Expand Up @@ -244,25 +289,18 @@ impl ReflectTraits {
}
Meta::NameValue(pair) => {
if pair.path.is_ident(FROM_REFLECT_ATTR) {
traits.from_reflect.auto_derive = match &pair.value {
syn::Expr::Lit(syn::ExprLit {
lit: syn::Lit::Bool(lit),
..
}) => {
traits.from_reflect_attrs.auto_derive =
Some(extract_bool(&pair.value, |lit| {
// Override `lit` if this is a `FromReflect` derive.
// This typically means a user is opting out of the default implementation
// from the `Reflect` derive and using the `FromReflect` derive directly instead.
is_from_reflect_derive
.then(|| LitBool::new(true, Span::call_site()))
.or_else(|| Some(lit.clone()))
}
_ => {
return Err(syn::Error::new(
pair.value.span(),
"Expected a boolean value",
))
}
};
.unwrap_or_else(|| lit.clone())
})?);
} else if pair.path.is_ident(TYPE_PATH_ATTR) {
traits.type_path_attrs.auto_derive =
Some(extract_bool(&pair.value, Clone::clone)?);
} else {
return Err(syn::Error::new(pair.path.span(), "Unknown attribute"));
}
Expand All @@ -284,10 +322,15 @@ impl ReflectTraits {
&self.idents
}

/// The `FromReflect` attributes on this type.
/// The `FromReflect` configuration found within `#[reflect(...)]` attributes on this type.
#[allow(clippy::wrong_self_convention)]
pub fn from_reflect(&self) -> &FromReflectAttrs {
&self.from_reflect
pub fn from_reflect_attrs(&self) -> &FromReflectAttrs {
&self.from_reflect_attrs
}

/// The `TypePath` configuration found within `#[reflect(...)]` attributes on this type.
pub fn type_path_attrs(&self) -> &TypePathAttrs {
&self.type_path_attrs
}

/// Returns the implementation of `Reflect::reflect_hash` as a `TokenStream`.
Expand Down Expand Up @@ -366,7 +409,8 @@ impl ReflectTraits {
self.debug.merge(other.debug)?;
self.hash.merge(other.hash)?;
self.partial_eq.merge(other.partial_eq)?;
self.from_reflect.merge(other.from_reflect)?;
self.from_reflect_attrs.merge(other.from_reflect_attrs)?;
self.type_path_attrs.merge(other.type_path_attrs)?;
for ident in other.idents {
add_unique_ident(&mut self.idents, ident)?;
}
Expand All @@ -392,3 +436,20 @@ fn add_unique_ident(idents: &mut Vec<Ident>, ident: Ident) -> Result<(), syn::Er
idents.push(ident);
Ok(())
}

/// Extract a boolean value from an expression.
///
/// The mapper exists so that the caller can conditionally choose to use the given
/// value or supply their own.
fn extract_bool(
value: &Expr,
mut mapper: impl FnMut(&LitBool) -> LitBool,
) -> Result<LitBool, syn::Error> {
match value {
syn::Expr::Lit(syn::ExprLit {
lit: syn::Lit::Bool(lit),
..
}) => Ok(mapper(lit)),
_ => Err(syn::Error::new(value.span(), "Expected a boolean value")),
}
}
2 changes: 1 addition & 1 deletion crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ impl<'a> ReflectMeta<'a> {
/// The `FromReflect` attributes on this type.
#[allow(clippy::wrong_self_convention)]
pub fn from_reflect(&self) -> &FromReflectAttrs {
self.traits.from_reflect()
self.traits.from_reflect_attrs()
}

/// The name of this struct.
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_reflect/bevy_reflect_derive/src/impls/typed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ pub(crate) fn impl_type_path(
meta: &ReflectMeta,
where_clause_options: &WhereClauseOptions,
) -> proc_macro2::TokenStream {
if !meta.traits().type_path_attrs().should_auto_derive() {
return proc_macro2::TokenStream::new();
}

let type_path = meta.type_path();
let bevy_reflect_path = meta.bevy_reflect_path();

Expand Down
7 changes: 7 additions & 0 deletions crates/bevy_reflect/bevy_reflect_derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ pub(crate) static TYPE_NAME_ATTRIBUTE_NAME: &str = "type_name";
///
/// Note that in the latter case, `ReflectFromReflect` will no longer be automatically registered.
///
/// ## `#[reflect(type_path = false)]`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this with #[reflect_value(type_path = false)] and that worked as well. Maybe could add a comment to say this works with reflect_value too, not just #[reflect]?

///
/// This attribute will opt-out of the default `TypePath` implementation.
///
/// This is useful for when a type can't or shouldn't implement `TypePath`,
/// or if a manual implementation is desired.
///
/// # Field Attributes
///
/// Along with the container attributes, this macro comes with some attributes that may be applied
Expand Down
26 changes: 16 additions & 10 deletions crates/bevy_reflect/bevy_reflect_derive/src/utility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,22 @@ impl WhereClauseOptions {
})
.unzip();

let (parameter_types, parameter_trait_bounds): (Vec<_>, Vec<_>) = meta
.type_path()
.generics()
.type_params()
.map(|param| {
let ident = param.ident.clone();
let bounds = quote!(#bevy_reflect_path::TypePath);
(ident, bounds)
})
.unzip();
let (parameter_types, parameter_trait_bounds): (Vec<_>, Vec<_>) =
if meta.traits().type_path_attrs().should_auto_derive() {
meta.type_path()
.generics()
.type_params()
.map(|param| {
let ident = param.ident.clone();
let bounds = quote!(#bevy_reflect_path::TypePath);
(ident, bounds)
})
.unzip()
} else {
// If we don't need to derive `TypePath` for the type parameters,
// we can skip adding its bound to the `where` clause.
(Vec::new(), Vec::new())
};

Self {
active_types: active_types.into_boxed_slice(),
Expand Down
49 changes: 49 additions & 0 deletions crates/bevy_reflect/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,7 @@ mod tests {
use super::*;
use crate as bevy_reflect;
use crate::serde::{ReflectSerializer, UntypedReflectDeserializer};
use crate::utility::GenericTypePathCell;

#[test]
fn reflect_struct() {
Expand Down Expand Up @@ -1863,6 +1864,54 @@ bevy_reflect::tests::should_reflect_debug::Test {
let _ = <Recurse<Recurse<()>> as TypePath>::type_path();
}

#[test]
fn can_opt_out_type_path() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a test for reflect_value since this is affected by the changes too.

#[derive(Reflect)]
#[reflect(type_path = false)]
struct Foo<T> {
#[reflect(ignore)]
_marker: PhantomData<T>,
}

struct NotTypePath;

impl<T: 'static> TypePath for Foo<T> {
fn type_path() -> &'static str {
std::any::type_name::<Self>()
}

fn short_type_path() -> &'static str {
static CELL: GenericTypePathCell = GenericTypePathCell::new();
CELL.get_or_insert::<Self, _>(|| {
bevy_utils::get_short_name(std::any::type_name::<Self>())
})
}

fn crate_name() -> Option<&'static str> {
Some("bevy_reflect")
}

fn module_path() -> Option<&'static str> {
Some("bevy_reflect::tests")
}

fn type_ident() -> Option<&'static str> {
Some("Foo")
}
}

// Can use `TypePath`
let path = <Foo<NotTypePath> as TypePath>::type_path();
assert_eq!("bevy_reflect::tests::can_opt_out_type_path::Foo<bevy_reflect::tests::can_opt_out_type_path::NotTypePath>", path);

// Can register the type
let mut registry = TypeRegistry::default();
registry.register::<Foo<NotTypePath>>();

let registration = registry.get(TypeId::of::<Foo<NotTypePath>>()).unwrap();
assert_eq!("Foo<NotTypePath>", registration.short_name());
}

#[cfg(feature = "glam")]
mod glam {
use super::*;
Expand Down