Skip to content

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-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-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