Skip to content

Make SystemSet marker traits mutually exclusive #8032

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

Closed
wants to merge 3 commits into from

Conversation

joseph-gio
Copy link
Member

Objective

Fix an issue discussed in #7882. The problem comes down to the fact that BaseSystemSet and FreeSystemSet can both be implemented for the same type, which opens the door for misuse and results in actively harmful error messages.

Solution

Use an associated type to determine whether a system set is base or not. Add blanket implementations to the marker traits based on the value of the associated type. Users are disallowed from manually implementing either marker trait due to blanket implementations, which ensures that the two traits are disjoint.


Migration Guide

The marker traits BaseSystemSet and FreeSystemSet can no longer be implemented manually. Instead, implement KindedSystemSet:

use bevy_ecs::schedule;

// Before:
impl schedule::BaseSystemSet for MySet {}

// After:
impl schedule::KindedSystemSet for MySet {
    // This will automatically implement `BaseSystemSet` for `MySet` due to a blanket implementation.
    type Kind = schedule::BaseSystemSetMarker;
}

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Not confident we'll need this if base system sets are removed in #8079.

/// [`is_base`]: SystemSet::is_base
pub trait BaseSystemSet: SystemSet {}
/// [`SystemSet::is_base`] always returns `true` for these types.
pub trait BaseSystemSet: KindedSystemSet<Kind = BaseSystemSetMarker> {}
Copy link
Member

Choose a reason for hiding this comment

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

These traits should also be sealed to avoid users manually implementing them.

@james7132 james7132 added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 16, 2023
@joseph-gio joseph-gio closed this Mar 18, 2023
@joseph-gio joseph-gio deleted the better-set-markers branch March 18, 2023 01:48
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants