SystemParam derive leaks private fields

[JoJoJet]

Bevy version

0.6

What you did

Made a SystemParam struct with the following definition:

#[derive(SystemParam)]
pub(super) struct EguiParams<'w, 's> {
    config: ResMut<'w, TimeScaleConfig>,
    show_popup: Local<'s, ShowPopup>,
    slider: Local<'s, TimeSlider>,
}

#[derive(Debug, Clone, Copy, Default)]
struct ShowPopup(bool);
#[derive(Debug, Clone, Copy)]
struct TimeSlider(u32);

What you expected to happen

I would have a publicly-accessible SystemParam struct with properly encapsulated private fields. This would allow a module to let its parent pass queries or resources into it without exposing the specific implementation details of the module.

What actually happened

Got an error due to private types being leaked.

error[E0446]: private type `ShowPopup` in public interface
   --> time.rs:94:10
    |
94  | #[derive(SystemParam)]
    |          ^^^^^^^^^^^ can't leak private type
...
101 | struct ShowPopup(bool);
    | ----------------------- `ShowPopup` declared as private
    |
    = note: this error originates in the derive macro `SystemParam` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0446]: private type `TimeSlider` in public interface
   --> time.rs:94:10
    |
94  | #[derive(SystemParam)]
    |          ^^^^^^^^^^^ can't leak private type
...
103 | struct TimeSlider(u32);
    | ----------------------- `TimeSlider` declared as private
    |
    = note: this error originates in the derive macro `SystemParam` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0446`.

I suspect that this is due to the derived impl inserting a : SystemParam trait bound for each field, which leaks the private types since trait bounds are public.

Additional information

In my case the fix is fairly easy: I can just add pub(super) to each struct definition and not worry. However, this solution is not ideal for plugin authors who want to make custom SystemParams, as it forces them to expose their private types. The best they could do is #[doc(hidden)].

[kirusfg]

I would like to tackle this issue. A minimal example to reproduce would be nice.

[JoJoJet]

I would like to tackle this issue. A minimal example to reproduce would be nice.

Thanks for the quick reply! I cooked up a min example here. Let me know if that's not exactly what you were looking for.

[kirusfg]

minimal example to reproduce

Yesterday I came up with one and was hoping to solve the issue with some slight change to the SystemParam derive macro, but apparently I did not quite get what your desired outcome is (it was late into the night, forgive me).

After looking at your example today, now I understand it and think of it as a legitimate feature to request.

I believe this can be achieved by introducing, say, a new Private filter for Query (akin to With, Or, and the likes); I am not well-experienced with Bevy's ECS though...

This only means that implementing this will take a while, if someone else does not do it before me :)

[kirusfg]

Yet again, I am not even sure that such a filter is feasible.

[JoJoJet]

So I've tried looking at the generated code using cargo-expand, and it appears my original theory was incorrect. The culprit is this impl, which exposes the private fields via an associated type:

impl<'w, 's> bevy::ecs::system::SystemParam for OpaqueParams<'w, 's> {
    type Fetch = OpaqueParamsState<(
        <Query<'w, 's, Read<Private>> as bevy::ecs::system::SystemParam>::Fetch,
    )>;
}

I think a fairly easy fix would be to just use an opaque type for SystemParam::Fetch. I wrote up an example here:

#[doc(hidden)]
pub type OpaqueFetch = impl for<'w, 's> SystemParamFetch<'w, 's>;
// the purpose of this entire module is just to fulfill the "defining scope"
// for the above opaque type.
// We simply define a function that never gets called in order to clue the compiler
// in on what the concrete type of `OpaqueFetch` is.
mod define {
    use super::*;
    #[allow(unreachable_code)]
    #[allow(unused)]
    fn dummy<'w, 's>() -> OpaqueFetch {
        use bevy::ecs::system::SystemParamState as _;
        <OpaqueParamsState<
            (<Query<'w, 's, Read<Private>> as bevy::ecs::system::SystemParam>::Fetch,),
        >>::init(unreachable!(), unreachable!(), unreachable!())
    }
}
impl<'w, 's> bevy::ecs::system::SystemParam for OpaqueParams<'w, 's> {
    type Fetch = OpaqueFetch;
}

This almost works, but fails when I try to actually use it as a SystemParam due to some inscrutable type errors involving IntoSystem

error[E0277]: the trait bound `for<'r, 's> fn(OpaqueParams<'r, 's>) {print_stuff}: IntoSystem<(), (), _>` is not satisfied
   --> src\main.rs:10:21
    |
10  |         .add_system(print_stuff)
    |          ---------- ^^^^^^^^^^^ the trait `IntoSystem<(), (), _>` is not implemented for `for<'r, 's> fn(OpaqueParams<'r, 's>) {print_stuff}`
    |          |
    |          required by a bound introduced by this call
    |
    = note: required because of the requirements on the impl of `IntoSystemDescriptor<_>` for `for<'r, 's> fn(OpaqueParams<'r, 's>) {print_stuff}`
note: required by a bound in `bevy::prelude::App::add_system`
   --> C:\Users\joe10\.cargo\registry\src\github.com-1ecc6299db9ec823\bevy_app-0.6.0\src\app.rs:325:55
    |
325 |     pub fn add_system<Params>(&mut self, system: impl IntoSystemDescriptor<Params>) -> &mut Self {
    |                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `bevy::prelude::App::add_system`

For more information about this error, try `rustc --explain E0277`.

Hopefully that should be fixable by someone who knows more about the inner-workings of bevy. Otherwise, it works perfectly, satisfying type inference without leaking private fields.
If anyone wants to play around with it, my fix is on this branch.

Of course, none of this is possible yet since type_alias_impl_trait is not stable :(

[arialpew]

If you have params, say for a Local<ShowPopup>, you expect to have a ShowPopup at the end, because Local<T>, Res<T>, ResMut<T>, and so on, are just smart pointer over their T. In your system when you access field on your SystemParam, even if there's many indirection, the end result is a T.

So does it make sense to have T private ? If you make T public, that does not mean T fields are public. You can hide all your fields and consumer can only use public method that encapsulate all your logic that you want to expose.

[Guvante]

IntoSystem requires that the Param is 'static which pub type OpaqueFetch = impl for<'w, 's> SystemParamFetch<'w, 's>; doesn't specify. I haven't played around with type_alias_impl_trait so don't know if this is a fixable problem (I assume it would be).

Unfortunately though I don't know if this would be enough to solve your problem as <<OpaqueParams as SystemParam>::Fetch as SystemParamFetch<'_, '_>>::Item == Private and that means anyone who has OpaqueParams can name Private now.

Whether this is a problem depends on exactly the intent of the privacy system.