fix: issue #24334

[tevans-3]

Objective

Fixes #24334

Solution

I added a bool flag to ComponentDescriptor and a check helper to that struct's impl. During component registration, i.e. in register_component_inner, new checks determine (A) if the component is a resource and (B) if its required_by set is non-empty. If (A) and (B) are true, it warns. I also updated all files referencing new_with_layout.

Testing

Yes. I ran cargo test -p bevy_ecs component::.

cargo build compiled successfully.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

[mockersf]

I tried to trigger this warning but didn't get it, where should it appear?

I think it would make more sense to compile fail if we want to disable this use case.

Also could you change the PR title to reflect what you're changing? The PR title will appear in the commit log of Bevy once merged.

[SpecificProtagonist]

Hm, do we have a way to write tests that check for warnings?

Two reasons no warning is emitted:

• the resource derive macro never sets the resource flag to true
• required_by starts empty when the component is being registered and gets populated in Components::register_required_by; the check should be there

I think it would make more sense to compile fail if we want to disable this use case.

I initially thought it would be annoying to implement well for not much benefit (esp. since I want to be careful around adding too much code generation to each component and affecting compile times), but urben1680 found a way that should work well:

trait Component {
    const ASSERT_NOT_RESOURCE: ();
    fn register_required();
    …
}

struct MyComponent;
struct MyResource;

impl Component for MyComponent {
    const ASSERT_NOT_RESOURCE: () = ();
    fn register_required() {
        // #[require(MyResource)] is used, so proc macro adds following line
        // (varies depending on wheather a function call or type was specified)
        let () = MyResource::ASSERT_NOT_RESOURCE; // this panics during compilation
        
        // actual requiring code follows here
    }
}

impl Component for MyResource {
    const ASSERT_NOT_RESOURCE: () = panic!("is resource"); // only panics if evaluated somewhere
    …
}

[Jengamon]

This happens to be used in a benchmark (why CI is failing): https://github.com/bevyengine/bevy/blob/main/benches/benches/bevy_ecs/resources.rs#L20

[tevans-3]

Hm, do we have a way to write tests that check for warnings?

Two reasons no warning is emitted:

* the resource derive macro never sets the `resource` flag to true

* `required_by` starts empty when the component is being registered and gets populated in `Components::register_required_by`; the check should be there

Is compile fail the preferred solution, then? Should I take a stab at implementing that instead of fixing this up?

[cart]

Using a Resource as a required component isn't inherently broken, it is just likely to be. Additionally, given that we already log an error when there are multiple "resource entities", I think I'd prefer to not add additional per-component-registration checks / inlined data to cover this case.