Implement Label traits for their boxed variants

[vladbat00]

The use-case that I couldn't achieve with the current API of Bevy was iterating over all existing stages and inserting additional stages that would execute before or after the iterated ones.

Here's, for instance, the code that I wrote to measure stages execution time with Puffin that requires this

let stages = builder
    .app
    .schedule
    .iter_stages()
    .map(|(stage_label, _)| stage_label.dyn_clone())
    .collect::<Vec<_>>();
for stage_label in stages {
    let puffin_id: &'static str = Box::leak(format!("{:?}", stage_label).into_boxed_str());
    let before_stage_label: &'static str =
        Box::leak(format!("puffin_before {:?}", stage_label).into_boxed_str());
    let after_stage_label: &'static str =
        Box::leak(format!("puffin_after {:?}", stage_label).into_boxed_str());
    let stage_label_to_remove = stage_label.dyn_clone();
    builder.add_stage_before(
        stage_label.dyn_clone(),
        before_stage_label,
        SystemStage::parallel().with_system(
            (move |_world: &mut World| {
                PUFFIN_SCOPES.with(|scopes| {
                    let mut scopes = scopes.borrow_mut();
                    scopes.insert(
                        stage_label.dyn_clone(),
                        puffin::ProfilerScope::new(
                            puffin_id,
                            puffin::current_file_name!(),
                            "",
                        ),
                    );
                });
            })
            .exclusive_system(),
        ),
    );
    builder.add_stage_after(
        stage_label_to_remove.dyn_clone(),
        after_stage_label,
        SystemStage::parallel().with_system(
            (move |_world: &mut World| {
                PUFFIN_SCOPES.with(|scopes| {
                    let mut scopes = scopes.borrow_mut();
                    scopes.remove(&stage_label_to_remove);
                });
            })
            .exclusive_system(),
        ),
    );
}

[vladbat00]

DynEq won't work when comparing Box<dyn StageLabel> and &dyn StageLabel otherwise. I wasn't able to find any better way of fixing this.

[mockersf]

the dyn_clone may add some overhead, but it shouldn't be much as labels probably won't be big, and it's only during stage setup

[mockersf]

Could you add a test somewhere that would fail with a Box<dyn StageLabel> without this change? As it doesn't fail on compile, it would make sure we don't change back later on

[blaind]

This could work for the comparison:
.find(|(_i, stage_label)| stage_label.as_ref() == target)

[blaind]

Suggested change

	.find(|(_i, stage_label)| stage_label.dyn_clone() == target.dyn_clone())
	.find(|(_i, stage_label)| stage_label.as_ref() == target)

[vladbat00]

I've added the tests. Apologies for taking so long to do it. :)

@blaind, unfortunately, your suggestion didn't work :( It definitely looks cleaner, but my tests fail if I try it this way.

[mockersf]

CI error is an ICE on nightly from wgpu

error: internal compiler error: failed to process buffered lint here (dummy = false)
  --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-core-0.9.2/build.rs:7:5
   |
7  | /     cfg_aliases::cfg_aliases! {
8  | |         // Vendors/systems
9  | |         wasm: { target_arch = "wasm32" },
10 | |         apple: { any(target_os = "ios", target_os = "macos") },
...  |
18 | |         gl: { any(wasm, unix_wo_apple) },
19 | |     }
   | |_____^
   |
   = note: delayed at /rustc/014026d1a7ca991f82f12efa95ef4dffb29dc8af/compiler/rustc_lint/src/early.rs:402:18
   = note: this error: internal compiler error originates in the macro `$crate::cfg_aliases` (in Nightly builds, run with -Z macro-backtrace for more info)

[alice-i-cecile]

Useful, tested and straightforward. This looks good to me. We can do this later for system labels too as the need arises.

[cart]

The downside here is that it seems like this would allow Box<Box<Box<dyn SystemLabel>>> if you aren't careful. The example above already results in one level of nesting. Unfortunately, I don't have a good solution to this problem that doesn't involve changing our interfaces to:

    pub fn add_stage_before<S: Stage>(
        &mut self,
        target: &dyn StageLabel,
        label: &dyn StageLabel,
        stage: S,
    ) -> &mut Self {

Then cloning when ownership is needed. Requiring references here seems like a net loss in UX relative to nested boxes.

[alice-i-cecile]

Agreed. Nested boxes is an obvious code smell, so I'm not too concerned about that. The ergonomics loss is more important.

It would be nice to have the same change made for the other label types, but it's not immediately clear that this PR is the place to do it.

[cart]

In this case, the nested boxing isn't particularly obvious. The type is never written out and the boxing happens internally on a method that accepts a impl StableLabel. If you aren't aware of this behavior, it will "just happen".

Imagine a worst case scenario where you write logic that moves stages around on every frame, cloning and reusing the label on each frame. You would have nested boxing on a level that should never be allowed.

[alice-i-cecile]

@cart what if we change our APIs to use dyn StageLabel and force a Copy bound? The label types should be fine to be copied, and that removes the UX loss.

[cart]

dyn StageLabel isn't an option because it doesn't have a size (which will fail to compile). Only &dyn StageLabel works (and copy isn't required here, clone works equally well / would be abstracted from the user). And requiring references for labels doesn't feel great to me (as I mentioned above).

[vladbat00]

Hey @cart, I agree, allowing Box<Box<Box<dyn StageLabel>>> would be unfortunate.
I assumed that doing as_ref inside the self.as_ref().dyn_clone() expression would avoid that. But I'm not sure how to check it. (Though the thing I'm certain of is that it avoids going into infinite recursion.)

Not sure whether this confirms anything, but the debugger seems to suggest that both variables point to the same address, so I'd assume that there's no double-indirection.

[vladbat00]

Omg, they do point to different addresses! I shouldn't write anything in GitHub right before going to sleep. :D
Yeah, it sucks...

[vladbat00]

Hi @cart. I think I was able to confirm that wrapping into nested Box<Box<Box<T>>> doesn't happen. The debugger was actually showing addresses of Box instances themselves. The code is a bit messy, apologies for that, but I hope it demonstrates the fact that all the boxes point to the same address.

Note the cursed conversion from Box<dyn StageLabel> to Box<&'static str> via std::ptr::read(stage_ptr as *const Box<&'static str>).

fn test_adding_after_boxed_stage() {
    let mut schedule = Schedule::default();

    let a: &'static str = "first";
    schedule.add_stage(a, SystemStage::single_threaded());
    let stage = schedule.iter_stages().next().unwrap().0.dyn_clone();
    let stage2 = stage.dyn_clone();

    let s = Box::new(a);
    let ss = Box::new(a) as Box<dyn StageLabel>;
    println!("str addr (way 1): {:p}", a);
    println!("str addr (way 2): {:p}", (&s as &'static str as *const str).to_raw_parts().0);
    println!("box addr (way 1): {:p}", (&ss as &dyn StageLabel as *const dyn StageLabel).to_raw_parts().0);
    let ss_ptr = &ss as *const _ as usize;
    println!("box addr (way 2): 0x{:x}", ss_ptr);
    std::mem::forget(ss);
    let ss: Box<&'static str> = unsafe { std::ptr::read(ss_ptr as *const Box<&'static str>) };
    println!("str addr (way 3): {:p}", (&ss as &'static str as *const str).to_raw_parts().0);

    println!("stage box addr (way 1): {:p}", (&stage as &dyn StageLabel as *const dyn StageLabel).to_raw_parts().0);
    let stage_ptr = &stage as *const _ as usize;
    println!("stage box addr (way 2): 0x{:x}", stage_ptr);
    std::mem::forget(stage);
    let stage: Box<&'static str> = unsafe { std::ptr::read(stage_ptr as *const Box<&'static str>) };
    println!("stage value addr: {:p}", (&stage as &'static str as *const str).to_raw_parts().0);

    println!("cloned stage box addr (way 1): {:p}", (&stage2 as &dyn StageLabel as *const dyn StageLabel).to_raw_parts().0);
    let stage2_ptr = &stage2 as *const _ as usize;
    println!("cloned stage box addr (way 2): 0x{:x}", stage2_ptr);
    std::mem::forget(stage2);
    let stage2: Box<&'static str> = unsafe { std::ptr::read(stage2_ptr as *const Box<&'static str>) };
    println!("cloned stage value addr: {:p}", (&stage2 as &'static str as *const str).to_raw_parts().0);
}

[alice-i-cecile]

I've made a little playground reproduction that should be helpful for experiments and explanations.

[cart]

Might be helpful to extend it with an impl Label for Box<dyn Label> and a function that takes an impl Label as input and stores a boxed version of it, for a baseline reproduction of the desired api.

[cart]

Arg sorry you already did the Label impl

[cart]

Boxing a box is very possible with this style of api:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=352832b951b3d9548ed8e46d74a24b06

dyn_clone() can be implemented in a way to avoid the double boxing. However double-boxing can still occur if you call Box::new() on a T: Label, where T is boxed label. In practice this would mean disallowing calling Box::new on Labels by convention (in favor of calling dyn_clone()). This makes me more than a little bit uncomfortable.

@BoxyUwU called out that we could impl Label for BoxedLabel (instead of for Box<dyn Label>), where struct BoxedLabel(Box<dyn Label>), to at least avoid accidentally passing in a Box<dyn Label> into something that accepts T: Label and boxes it internally. But it would still require adopting the "no boxing T: Label by convention" pattern. This is relatively enforceable on our end (but would require vigilance). And users writing functions that accept T: Label and box it should be relatively uncommon.

But this lack of real type safety still gives me pause. People generally assume they can safely box traits.

[vladbat00]

Good call @cart! I've added the wrapper type, which hopefully will help to avoid passing Box<dyn Label> to impl Label functions, but yeah, it's still perfectly possible to add an additional level of wrapping by doing something like BoxedLabel(Box::new(boxed_label)).

Also, unfortunately, it's impossible to make the BoxedLabel constructor private, as we have the StageLabel derive, which requires it.

I get the type safety concern so I'll understand if this won't be merged. But having said that I'd still be glad to see it released, as it unblocks me from using the iter_stages function, until we get stageless executors.

[alice-i-cecile]

Also, unfortunately, it's impossible to make the BoxedLabel constructor private, as we have the StageLabel derive, which requires it.

Could we make it #[doc(hidden)] at least?

[joseph-gio]

What's the status of this now that #4957 has been merged?

[alice-i-cecile]

This can be closed out; we can make an equivalent PR if the need arises.