Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trait fn returning impl Fn() cannot be used as a system when called from generic type #13436

Closed
benfrankel opened this issue May 20, 2024 · 4 comments
Labels
C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled

Comments

@benfrankel
Copy link
Contributor

benfrankel commented May 20, 2024

Bevy version

0.13.2

What you did

trait Foo {
    fn foo() -> impl Fn() {
        || {}
    }
}

enum ConcreteFoo {}

impl Foo for ConcreteFoo {}

fn frobnicate<GenericFoo: Foo>(app: &mut App) {
    app.add_systems(Update, <ConcreteFoo as Foo>::foo()); // Is a system
    app.add_systems(Update, <GenericFoo as Foo>::foo()); // Is not a system
}

What went wrong

The second foo() is not considered a system, so it doesn't compile.

Additional information

This looks like a Rust bug, but I have no idea what the source of the bug is (something to do with RPITIT?), and this affects my usage of Bevy, so I'm creating an issue here for now.

@benfrankel benfrankel added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels May 20, 2024
@benfrankel
Copy link
Contributor Author

Workaround:

trait Foo {
    fn foo() -> BoxedSystem {
        Box::new(IntoSystem::into_system(|| {}))
    }
}

@benfrankel
Copy link
Contributor Author

benfrankel commented May 20, 2024

This bug applies to run conditions as well, but BoxedCondition is not a proper workaround because it doesn't impl Condition. You have to use .run_if_dyn instead of .run_if, but .run_if_dyn is a real pain to use because it doesn't return Self:

fn my_system() {}

fn always_box() -> BoxedCondition {
    Box::new(IntoSystem::into_system(|| true))
}

fn foo(app: &mut App) {
    app.add_systems(Update, my_system.run_if(always_box())); // Does not compile
    app.add_systems(Update, {
        let mut configs = my_system.into_configs();
        configs.run_if_dyn(always_box());
        configs
    }); // Does compile
}

@chescock
Copy link
Contributor

I think you need fn foo() -> impl Fn() + Sync + Send + 'static {.

System requires those trait bounds, and for ConcreteFoo the compiler can see the actual type and validate them. But for GenericFoo it has to protect against something like

struct EvilFoo;
impl Foo for EvilFoo {
    fn foo() -> impl Fn() {
        let x = Cell::new(0);
        // This is not `Sync` and so cannot be a `System`.
        move || {
            x.set(0);
        }
    }
}

@benfrankel
Copy link
Contributor Author

Interesting. That works. I guess this isn't a bug in Rust or Bevy, but it is surprising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled
Projects
None yet
Development

No branches or pull requests

2 participants