Change plugin to consume self

[hymm]

Objective

• When building a plugin I wanted to be able to temporarily store some stuff that is not clone on the Plugin for initialization, but then move it into a resource when the build function is called. In my specific case this was a Schedule.

Solution

• This changes the build function from &Self to Box and so consumes the Plugin type on build. It needs to be boxed because internally App stores the plugin as a dyn Plugin and this is not sized so cannot be moved. So changing build to use a Box<Self> allows it to be moved into the function.

pub struct AltSchedule(pub Schedule);

pub struct AltSchedulePlugin {
    schedule: AltSchedule
}

impl Plugin for AltSchedulePlugin {
    fn build(self: Box<Self>, app: &mut App) {
        app.insert_resource(self.schedule)
    }
}

Question

• Is it weird to have Box<Self> in a common public API?

[alice-i-cecile]

So, this is interesting. The box is pretty unfortunate; I'm not sure I love it in a public beginner oriented API like this.

That said, plugin building definitely feels like it should take ownership rather than just reference, allowing users to move in unclonable objects.

[bjorn3]

I think plugins should preferrably be configured using resources and be ZST's themself. This would play nicer with the future editor I think.

[alice-i-cecile]

I think plugins should preferrably be configured using resources and be ZST's themself. This would play nicer with the future editor I think.

This also plays nicer with some of my thoughts around structured plugins, by allowing us to avoid accounting for custom configuration parameters.

[mcobzarenco]

The box is a non-starter imo, not only it is not idiomatic, but also unnecessary and would make the common case more expensive (force one to make an allocation) and more confusing. The idiomatic thing to do and the common pattern for builders is to take self by value in the build() function.

I don't understand why not just box the schedule in your example / or have an option.

[mcobzarenco]

Furthermore, this change would break all libraries out there -- I appreciate bevy needs to move fast and break things, but FWIW, the use case presented by the OP does not warrant the breakage as it's very easy to workaround

[alice-i-cecile]

@hymm I think these arguments are compelling: this was a useful experiment, but I think ultimately a non-starter. Shall we close this out?

[bjorn3]

The idiomatic thing to do and the common pattern for builders is to take self by value in the build() function.

That prevents turning Plugin into a trait object. Traits for which any method not marked as where Self: Sized (which makes it unavailable on trait objects) takes self by value are not object safe.

[alice-i-cecile]

And we can't make Plugin Sized here (under the existing pattern), because then it would stop them from storing any internal unsized configuration, right?

[bjorn3]

I don't know of any plugin storing internal unsized configuration. Creating such a plugin requires unsafe code. I was talking about not passing self by value as being required to allow &dyn Plugin and Box<dyn Plugin>.

[hymm]

Yeah, I'll close this out.

[DJMcNab]

Worth noting that there is previous discussion of this, i.e. in #1045 (and https://discord.com/channels/691052431525675048/692572690833473578/748241380010098749). I was surprised that #1045 wasn't linked in the PR description, unless I'm missing it

I don't think there's anything hugely important in the discord link. Only cart's comment:

in general i only make things direct consumers if they need to be. in this case, consuming self doesn't seem to add any additional safety or utility. theres also the (extremely minor) concern that self might not get "optimized out" correctly and result in unnecessary copying.

[mcobzarenco]

That prevents turning Plugin into a trait object. Traits for which any method not marked as where Self: Sized (which makes it unavailable on trait objects) takes self by value are not object safe.

Yes indeed, good point 👍

[cart]

Something else worth calling out: consuming self would mean that the Plugin's init isn't a repeatable process, which might come in handy for things like editors, hot reloading, etc.

I think we should be moving in the direction of "plugins are completely stateless by default and configured with an instance of a specific type (ideally with a default impl)"