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

static assert that bundles do not have duplicate types #2388

Closed

Conversation

NathanSWard
Copy link
Contributor

@NathanSWard NathanSWard commented Jun 24, 2021

Objective

  • Currently it is possible for a user to define a bundle that contains duplicate types. However, this is incorrect and can lead to various bugs.
  • Partially addresses Enforce type-uniqueness of Bundles #2387

Solution

  • Implement a static check for Bundles that when using #[derive(Bundle)] will fail to compile if any of the inner types are duplicates.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 24, 2021
@NathanSWard
Copy link
Contributor Author

NathanSWard commented Jun 24, 2021

As a quick note, this is sort of a first pass solution.
I don't believe this handles nested bundles yet, however, I just wanted to see if this is the general direction we want to go with this issue.

@NathanSWard NathanSWard added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jun 24, 2021
@alice-i-cecile alice-i-cecile self-requested a review June 24, 2021 18:17
@bjorn3
Copy link
Contributor

bjorn3 commented Jun 24, 2021

Another option would be to have trait AssertDistinctComponentsMyBundle {} and then an impl of this trait for each field type. That will result in a conflicting trait impl if the types are equal.

@NathanSWard
Copy link
Contributor Author

Another option would be to have trait AssertDistinctComponentsMyBundle {} and then an impl of this trait for each field type. That will result in a conflicting trait impl if the types are equal.

This is how the current implementation is conducted... 🙃 (I'm pretty sure?)

@bjorn3
Copy link
Contributor

bjorn3 commented Jun 24, 2021

Oops, I thought it used a trait method and then error on ambiguity or something like that.

@NathanSWard
Copy link
Contributor Author

NathanSWard commented Jun 24, 2021

This should now also work with nested bundles!! :)

Edit: nvm, cross-crate implementations are giving me problems.....

@NathanSWard
Copy link
Contributor Author

Currently this implementation only support a single layer of checking.
e.g.

#[derive(Bundle)]
struct A {
    a: usize,
    b: usize,
}

will fail to compile because a and b are in the same structure.

However,

#[derive(Bundle)]
struct A {
    a: usize,
}

#[derive(Bundle)]
struct A {
    #[bundle]
    a: A,
    b: usize,
}

will still compile since you cannot implement traits for foreign types...

I still think this addition is a nice QOL improvement, however we should keep an issue open to track nested bundle static checking.

@NathanSWard
Copy link
Contributor Author

@TheRawMeatball I'm curious what your thoughts are with this issue.
From what we've seen, it's not possible to assert at compile time that bundles are correct (due to nested bundles).
However is this current PR even worth pursuing?
As in should we support static checking for a single layer or not support it until we can find a solution for nested bundles.

@alice-i-cecile
Copy link
Member

IMO failing if we know something is incorrect is worth doing, even if we can't catch every case.

@mockersf
Copy link
Member

mockersf commented Jun 26, 2021

Going on a tangent about nested bundles: is the possibility to do it really worth it? I'm not completely convinced. The issues raised in #1588 and #2331 when trying to use it are valid, and if we don't even use it in standard Bevy I would argue this should be removed.

I would prefer having bundles be statically checked over having nested bundles.

@TheRawMeatball
Copy link
Member

I'd strongly prefer nested bundles over fully static assertions personally. They're very useful for spawning more complex entities.

@mockersf
Copy link
Member

#[derive(Bundle, Debug)]
pub struct Position {
    pub x: u8,
    pub y: u8,
}

gives:

error[E0119]: conflicting implementations of trait `static_assert_Position_does_not_have_duplicate_types::AssertDistinctComponentsForPosition` for type `u8`
  --> src/main.rs:46:10
   |
46 | #[derive(Bundle, Debug)]
   |          ^^^^^^
   |          |
   |          first implementation here
   |          conflicting implementation for `u8`
   |
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

@TheRawMeatball
Copy link
Member

Looks pretty good to me! Hopefully in the far future when more rust features are stable we can make it work for nested stuff too, but for now this seems like a useful compromise.

@NathanSWard
Copy link
Contributor Author

I just realized, this impl may not work with generic bundles....

@NathanSWard
Copy link
Contributor Author

There's a few problems with the PR, including generic bundles, and not being able to recurse and check nested bundles.
I'm going to close this for now, and if I can find a more viable solution I'll open an new PR accordingly.

@NathanSWard NathanSWard closed this Jul 2, 2021
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

Successfully merging this pull request may close these issues.

6 participants