Skip to content

Document required components with a marker trait #18169

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

Closed
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
24 changes: 24 additions & 0 deletions crates/bevy_ecs/macros/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,28 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
let struct_name = &ast.ident;
let (impl_generics, type_generics, where_clause) = &ast.generics.split_for_impl();

let required_component_impls = attrs.requires.as_ref().map(|r| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add #[cfg(doc)] to the generated impl/trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly not, I tried but cargo doc doesn't propagate doc to dependencies or macros in a way that worked reliably.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that's because of rust-lang/cargo#8811. For docs.rs and dev-doc.bevyengine.org you could set the flags in Cargo.toml/docs.yaml.

let impls = r
.iter()
.map(|r| {
let path = &r.path;
let insertion_info = match &r.func {
Some(RequireFunc::Closure(closure)) => format!("`{}`", closure.body.to_token_stream()),
Some(RequireFunc::Path(path)) => format!("[`{}`].", path.to_token_stream()),
None => "the [default](Default::default) value.".to_string(),
};
quote! {
/// If not already present, the required component will be inserted using
#[doc = #insertion_info]
impl #impl_generics #bevy_ecs_path::component::document_required_components::Require<#path> for #struct_name #type_generics #where_clause {}
Copy link
Member

@cart cart Mar 7, 2025

Choose a reason for hiding this comment

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

I'm not convinced that this is meaningfully "better" than the current "required list in Component docs":

  1. Because traits are listed alphabetically, users will hit the Component trait before they hit the Require<X> traits.
  2. The require list in Component is significantly less "noisy". You can consume the entire list at a glance, rather than needing to parse out multiple lines of docs for each required component (and needing to look "around" generics / formatting / etc)
  3. The Require trait impl is confusing / introduces a weirdness factor (even if we manage to hide it). This is non-standard trait usage (traits are generally about "rust behaviors"). People reading the docs need to deal with the fact that
  4. This introduces additional codegen, which is something we should be wary of, given that we are already "heavy" on the codegen side.

From my perspective the only real win here is that the require list shows up in the sidebar, but that is also a downside, as a long list takes up significant space and obscures other "real" trait impls. And it doesn't necessarily meaningfully improve the prominence of the require list. The sidebar is often littered with other things (ex: I have to scroll to see the list for Node).

Edit: The backlinks are kind of nice, but the fact that A->B and C->A requires are interleaved together in the list is super confusing to me.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, our docs are just generally already very "noisy" and this adds even more to the pile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these are valid concerns, and I don't think this technique can be amended to satisfy them. Thanks for providing the feedback! I'll close this out

}
});

quote! {
#(#impls)*
}
});

let required_component_docs = attrs.requires.map(|r| {
let paths = r
.iter()
Expand Down Expand Up @@ -270,6 +292,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
#relationship

#relationship_target

#required_component_impls
})
}

Expand Down
18 changes: 18 additions & 0 deletions crates/bevy_ecs/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,24 @@ pub trait Component: Send + Sync + 'static {
fn visit_entities_mut(_this: &mut Self, _f: impl FnMut(&mut Entity)) {}
}

// doc(hidden) module makes it clear that usage of this trait outside of `bevy_ecs` is unsupported.
#[doc(hidden)]
Copy link
Member

Choose a reason for hiding this comment

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

For me, this removes all Require listings from the docs, which defeats the purpose of this feature :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah whoops, should be tested first! I assumed keeping the trait public would keep it in the documentation.

pub mod document_required_components {
use super::Component;

/// Indicates this [`Component`] requires another [`Component`] `C`.
///
/// **This trait does not register required components on its own.**
///
/// This trait is similar to [`Eq`] in the sense that it is up to the implementer to ensure `C`
/// is appropriately registered as a required component.
///
/// You should never manually implement this trait, but it is the implementor's responsibility
/// to ensure the implementation of [`Component::register_required_components`] matches any and
/// all implementations of [`Require`] on this type.
pub trait Require<C: Component>: Component {}
}

mod private {
pub trait Seal {}
}
Expand Down