Remove impl ScheduleLabel for Box<dyn ScheduleLabel>

[DanielJin21]

Objective

Within stageless, schedule labels are stored in a HashMap as Box<dyn ScheduleLabel> in the Schedules resource. In order to support inserting both concrete labels and boxed labels, the insert method of Schedules takes an impl ScheduleLabel, and a global impl of impl ScheduleLabel for Box<dyn ScheduleLabel> was added.

However, this introduced a footgun where a user with a boxed label could accidently forget to dereference their label while working with it. Since the label always implements ScheduleLabel no matter how many pointers it is behind, this is not caught by the compiler and can lead to confusing errors at runtime.

• For example, a method takes a &dyn ScheduleLabel as argument. If the user has a my_schedule_label: Box<dyn ScheduleLabel>, they should pass it by &*my_schedule_label. It is however easy to forget to dereference and pass in &my_schedule_label instead. This compiles without warnings but means something entirely different.

Solution

Remove impl ScheduleLabel for Box<dyn ScheduleLabel>. In order to continue to support inserting boxed labels, a insert_boxed method is added to Schedules. Two places in the code now uses insert_boxed where they were using insert before.

Changelog

(Only relevant to bevy main users)

• Remove impl ScheduleLabel for Box<dyn ScheduleLabel>
• Add insert_boxed to Schedules
• Schedules::insert no longer accepts a boxed schedule label. Use insert_boxed instead.

[alice-i-cecile]

I'm very glad to see that this wasn't ultimately needed. Thanks for cleaning this up for me.

[hymm]

Is this really a problem? All the internal uses use dyn_clone which clones the inner value into a box.

[alice-i-cecile]

Is this really a problem? All the internal uses use dyn_clone which clones the inner value into a box.

Hmm, fair. I'm split on whether or not this is a better design or not, now that you mention it. Externally, being able to interact with boxed labels is very convenient.

[DanielJin21]

Is this really a problem? All the internal uses use dyn_clone which clones the inner value into a box.

You are still able to use boxed schedule labels. This PR doesn't reduce its capabilities in any way. The main purpose of this PR is to prevent users from passing in a &Box<dyn ScheduleLabel> where a method expects a &dyn ScheduleLabel. For example:

#[derive(ScheduleLabel, Clone, Debug, PartialEq, Eq, Hash)]
pub enum MySchedule {
    MyLabel,
}
...

let label = MySchedule::MyLabel;
schedules.insert(label, my_schedule);

let boxed_label: Box<dyn ScheduleLabel> = Box::new(label);

// Correct, pass in a &dyn ScheduleLabel and remove the label.
schedules.remove(&*boxed_label);
// Incorrect, pass in a &Box<dyn ScheduleLabel>, this does not remove the label from the hashmap.
schedules.remove(&boxed_label);

[hymm]

That makes sense. I'm just uncomfortable with needing both a insert and a insert_boxed. Feels like it's complexity that's pointing to other problems with the API's on Schedules. Inserting with an owned value, but then having remove_entry return the boxed value definitely feels somewhat confusing. You need to know to switch to using insert_boxed when using the returned label in the current pr.

Maybe it'd be better to make insert take a &dyn ScheduleLabel to to make it clear that the owned value isn't being used as the key and not have a insert_boxed? We can leave the methods on World and App taking a owned value. That is where most users are going to interact with the labels and keeping it cleaner looking there is probably better. Not sure if this is actually better than having 2 methods.

[DanielJin21]

I understand your concerns. In my opinion, having two methods insert and insert_boxed is fairly natural as one is simply a pre-allocated version of another, with the understanding that some allocation must happen as we are working with trait objects.

If you prefer I can amend the docs to clarify the need to box the label. This should also alleviate the potential surprise that remove_entry returns a boxed version of the inserted schedule label.

[DanielJin21]

One way to both remove the impl ScheduleLabel for Box<dyn ScheduleLabel> and keep only one insert method is to define a new trait IntoBoxedScheduleLabel, and implement it for T: ScheduleLabel and Box<dyn ScheduleLabel>.

The insert method would then take T: IntoBoxedScheduleLabel. What do you think about this?

[hymm]

To clarify my suggestion, I was saying to remove the ScheduleLabel on boxed labels still, but only have one insert method that takes a &dyn ScheduleLabel. So to insert you'd need to do .insert(&MyLabel, Schedule::new()) or .insert(&*MyBoxedLabel, Schedule::new()).

[DanielJin21]

To clarify my suggestion, I was saying to remove the ScheduleLabel on boxed labels still, but only have one insert method that takes a &dyn ScheduleLabel. So to insert you'd need to do .insert(&MyLabel, Schedule::new()) or .insert(&*MyBoxedLabel, Schedule::new()).

Thanks for the clarification. I think this is a valid alternative to having two methods and definitely worth considering. I realize that I may need to have some more thoughts on this on what the best way to approach this problem. If it's alright with you and @alice-i-cecile, I will leave this PR open until when I make a patch with a better solution.

[DanielJin21]

Closing this PR in favor of alternatives including #5715, #7760, and potentially #7762. All of these PRs will improve the situation around label traits and their use, with the possiblity of removing this footgun.