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

Enforce type-uniqueness of Bundles #2387

Closed
alice-i-cecile opened this issue Jun 24, 2021 · 9 comments
Closed

Enforce type-uniqueness of Bundles #2387

alice-i-cecile opened this issue Jun 24, 2021 · 9 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Currently, bundles can have fields of duplicate types. For example:

use bevy::prelude::*;

struct Foo(u32);

#[derive(Bundle)
struct Bar{
  foo1: Foo,
  foo2: Foo,
}

compiles with no warning.

However, this behaviour is deeply wrong, and leads to silent, subtle bugs in end user code.
Each field in a bundle represents one component that should be inserted into the final entity, and entities can only have one unique component of each (Rust) type.

Any duplicate components will have their data silently erased by the last field of the same type; wasting work and causing subtle and serious logic bugs. In the demonstrated example, setting the value of foo1 has no effect: it will always be overwritten by foo2.

This problem is particularly rough when working with type aliases (where the problem is not immediately obvious) and nested bundles (where there can be a large number of elided types, and new errors can occur without any change to user code on a new release if more components are added).

What solution would you like?

The #[derive(Bundle) macro should check for duplicated types, and error (ideally at compile time) if they are found with an error message that displays both the field and type names.

Be sure to test that this check works with:

  1. type aliases
  2. nested bundles

What alternative(s) have you considered?

  1. This could be enforced at component-insertion time, using Command error handling #2241. I would prefer to leave that as an additional layer of opt-in safety, and instead provide compile time checks for obvious mistakes.
  2. We may want a way to opt-out of this; perhaps with an #[allow_duplicate] attribute on specific fields.
  3. An error, especially at compile time, is better than a warning, as it will either be immediately fixed or permanently ignored.
  4. It is by far more important that this is enforced for the Bundle derive macro, but it might be nice to enforce it on manual impls too.
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 24, 2021
@bjorn3
Copy link
Contributor

bjorn3 commented Jun 24, 2021

Be sure to test that this check works with:

  1. type aliases
  2. nested bundles

Macros don't have access to type information. Typechecking requires the full crate to be expanded first.

@alice-i-cecile
Copy link
Member Author

Macros don't have access to type information. Typechecking requires the full crate to be expanded first.

Oof :( That doesn't surprise me but is still sad. So the complex cases are approximately impossible to check for statically? That pushes me much further towards "inserting duplicate components should be an error by default".

@TheRawMeatball
Copy link
Member

An alternative would be to issue a warning whenever duplicate components up in a bundle, and more importantly this should be possible to silence: I can see a use case being to put a large nested bundle at the top of a struct, and then listing components to override under it.

@NathanSWard
Copy link
Contributor

NathanSWard commented Jun 24, 2021

Oof :( That doesn't surprise me but is still sad. So the complex cases are approximately impossible to check for statically? That pushes me much further towards "inserting duplicate components should be an error by default".

You might be able to generate some code which checks the type info at compile time.
Ideally though this should be done at compile time and not runtime (imo).

@NathanSWard
Copy link
Contributor

I can see a use case being to put a large nested bundle at the top of a struct, and then listing components to override under it.

Could you provide an example of this?
I don't (yet) see how this can be useful.

@cart
Copy link
Member

cart commented Jun 24, 2021

Just a heads up that we do already ensure that there aren't duplicate components when bundles are inserted. This test passes. Static validation of bundle derives is definitely nicer though!

#[test]
#[should_panic]
fn duplicate_components_panic() {
    let mut world = World::new();
    world.spawn().insert_bundle((1, 2));
}

@NathanSWard
Copy link
Contributor

So the current impl I have working for a single layer of components, however, nested bundles are not yet supported because idk if it's possible across crate boundaries...

@james7132
Copy link
Member

It seems like this is checked (at runtime) now after #2975, #6054, and #6039. Is this still relevant?

@alice-i-cecile
Copy link
Member Author

Yeah, this is basically resolved. Compile-time checks would be great, but not happening without work on the language itself. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

6 participants