Descriptive error message for circular required components recursion

[SpecificProtagonist]

Objective

Fixes #16645

Solution

Keep track of components in callstack when registering required components.

Testing

Added a test checking that the error fires.

---

Showcase

#[derive(Component, Default)]
#[require(B)]
struct A;

#[derive(Component, Default)]
#[require(A)]
struct B;
World::new().spawn(A);

thread 'main' panicked at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/component.rs:415:13:
Recursive required components detected: A → B → A

[AlexAegis]

Why not just ignore the circle and just stop the recursion when encountering one already processed? Is there a reason why it must be treated as a hard error?

[SpecificProtagonist]

Because a circular requirement is likely unintentional (such as in the linked issue) – if multiple components require each other, they should be a single component, and there is no reason for a component to directly require itself.

[AlexAegis]

I hadn't thought of that! Maybe it would be worth mentioning in the error message that "maybe you want to merge these two components into one?"

[SpecificProtagonist]

Good point, added.

[chescock]

Quadratic algorithms always make me nervous. Would it make sense to pass a FixedBitSet of ComponentIds alongside the Vec so that this can be an O(1) check? Or would the overhead of updating a second structure make it slower in realistic cases?

[SpecificProtagonist]

I don't think we'll see deep enough chains of required components where the lookup in the vec has a measurable perf impact. The depth will be low, and comparisons with a list of integers that laid out successively in memory is really fast so the constant factor is very small.

For a depth of 50 components depending on another in a chain – much deeper than what we should see – doing all of these checks for all of the components took a combined total of 200µs on my laptop.

Still, while I don't think it's necessary, adding a FixedBitSet check shouldn't make the code much more complicated so I'd be ok with doing that if you want.

[chescock]

Yeah, I think you're right that the chains will be small, so an extra check wouldn't be worth the complexity.

[chescock]

I think doing this check before the push means you have one more level of recursion than is necessary.

Like, if you register a component that requires itself, then the first time this is called the stack will be empty, and the second time it will have one element that it binds to requiree but an empty check list. So it will pass on the second call, even though it had enough information to detect the error!

The simplest thing to do might be to pass self_id to enforce_no_required_components_recursion so that it doesn't need to do split_last().

[SpecificProtagonist]

Performance wise the extra recursion doesn't matter, as it's only in the panic case (unless I've misunderstood something).

Wouldn't make the code simpler either, as I would then have to append the self_id if the check fails so the full cycle is printed.

[chescock]

Yeah, I think you're right that the chains will be small, so an extra check wouldn't be worth the complexity.

[alice-i-cecile]

Great comments :) It's good to improve the messages here.