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

[Merged by Bors] - Fix trait bounds for run conditions #7688

Closed
wants to merge 5 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Feb 15, 2023

Objective

The trait Condition<> is implemented for any type that can be converted into a ReadOnlySystem which takes no inputs and returns a bool. However, due to the current implementation, consumers of the trait cannot rely on the fact that <T as Condition>::System implements ReadOnlySystem. In cases such as the not combinator (added in #7559), we are required to add redundant T::System: ReadOnlySystem trait bounds, even though this should be implied by the Condition<> trait.

Solution

Add a hidden associated type which allows the compiler to figure out that the System associated type implements ReadOnlySystem.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Feb 15, 2023
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Feb 15, 2023
@JoJoJet JoJoJet requested a review from Shatur February 16, 2023 16:20
Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

Good change!
not() is definitely a special case. I thought about making not like the upcoming or / and #7605, but not(system) is more ergonomic for sure.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 16, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Feb 16, 2023
# Objective

The trait `Condition<>` is implemented for any type that can be converted into a `ReadOnlySystem` which takes no inputs and returns a bool. However, due to the current implementation, consumers of the trait cannot rely on the fact that `<T as Condition>::System` implements `ReadOnlySystem`. In cases such as the `not` combinator (added in #7559), we are required to add redundant `T::System: ReadOnlySystem` trait bounds, even though this should be implied by the `Condition<>` trait.

## Solution

Add a hidden associated type which allows the compiler to figure out that the `System` associated type implements `ReadOnlySystem`.
@bors bors bot changed the title Fix trait bounds for run conditions [Merged by Bors] - Fix trait bounds for run conditions Feb 16, 2023
@bors bors bot closed this Feb 16, 2023
@JoJoJet JoJoJet deleted the read-only-condition branch February 16, 2023 17:25
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 16, 2023
# Objective

The trait `Condition<>` is implemented for any type that can be converted into a `ReadOnlySystem` which takes no inputs and returns a bool. However, due to the current implementation, consumers of the trait cannot rely on the fact that `<T as Condition>::System` implements `ReadOnlySystem`. In cases such as the `not` combinator (added in bevyengine#7559), we are required to add redundant `T::System: ReadOnlySystem` trait bounds, even though this should be implied by the `Condition<>` trait.

## Solution

Add a hidden associated type which allows the compiler to figure out that the `System` associated type implements `ReadOnlySystem`.
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-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants