Better error messages for IntoSystem and IntoExclusive System

[alice-i-cecile]

What problem does this solve or what need does it fill?

The current error messages for when system parameters is wrong is very opaque. As an example:

 no method named `system` found for fn item `for<'r, 's, 't0, 't1> fn(Query<'r, (bevy::prelude::Entity, &'s AttackBonus, &'t0 Defense, &'t1 Advantage), bevy::prelude::With<(Active, Attack)>>, Commands) {check_attacks}` in the current scope
`check_attacks` is a function, perhaps you wish to call it

This is particularly painful because these mistakes are a common beginner issue and there's no clear path forward.

What solution would you like?

The compiler examines each of your system's parameters, explains which one fails, and ideally makes a simple suggestion on how to correct it.

What alternative(s) have you considered?

None.

Additional context

The relevant macros would be cleaned up substantially if variadic generics ever lands.

[llogiq]

Ok, first: How do we detect if a parameter fails? What about missing or superfluous parameters? What "simple" suggestions could we make (e.g. look at the variables in scope and suggest one or all of a matching type)?

[alice-i-cecile]

The simplest start IMO is that each of the function parameters needs a valid SystemParam impl.

The lowest hanging fruit is to simply tell you which parameters this failed for. This would be a huge win, because it would let users narrow down their problem and catch obvious mistakes.

The next step would be to try and guess which impl they meant to use, then statically list off common fixes: see this page on the Cheatbook.

From there, we could have tangible suggestions on how to fix the problem to conform to the target impl.

Missing parameters are very rare so far, although see #1587 for an exception. Superfluous parameters are already caught nicely by existing "unused variable" / "unused mut" warnings and lints.

[cart]

Random thought: with specialization, we could implement SystemParam for every type by default, then override the default for "valid" params. .system() would then fail for any "default" implementations with a "T is not a valid SystemParam" error message. But of course specialization is X years away where X is 1-never.

[cart]

The benefit is that users get useful error messages. The downside is that "invalid" systems are no longer compile time errors, so it's probably not worth it.

[concave-sphere]

This is probably my number one headache with Bevy (edit "number one" may be an exaggeration, I wrote it just after a frustrating session debugging systems that weren't working, but it is definitely up there). I spend an inordinate amount of time trying to figure out why the compiler is insisting that my function doesn't implement .system. Most of the other issues I ran into early on have lessened with familiarity, but while I'm better than I was at debugging these issues, it still feels like it takes a lot more time than it should, especially when the problem is just that I added or removed an &, or that In has to be the first parameter to the function.

I wonder if this would actually work OK for a "normal" type (e.g, a tuple). The compiler is normally pretty good about telling me which constraint is preventing a trait from being implemented. But that error seems to get overridden for functions with the error quoted above ("... X is a function, did you mean to call it?") I tried an experiment of adding an AsSystem wrapper struct and using that to drive the conversion instead of .system(), to see if the errors would be saner. Unfortunately, that just got me an error like this:

error[E0277]: the trait bound `into_system::AsSystem<&for<'r, 's> fn(&'r system_param::Res<'s, u32>) {invalid_system}>: into_system::SystemParamFunction<_, _, _, _>` is not satisfied
   --> crates/bevy_ecs/src/system/into_system.rs:330:40
    |
277 |     fn requires_system_param_function<
    |        ------------------------------ required by a bound in this
...
282 |         F: SystemParamFunction<In, Out, Param, Marker>,
    |            ------------------------------------------- required by this bound in `requires_system_param_function`
...
330 |         requires_system_param_function(AsSystem(&invalid_system));
    |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `into_system::SystemParamFunction<_, _, _, _>` is not implemented for `into_system::AsSystem<&for<'r, 's> fn(&'r system_param::Res<'s, u32>) {invalid_system}>`
    |
    = help: the following implementations were found:
              <into_system::AsSystem<Func> as into_system::SystemParamFunction<(), Out, (), ()>>
              <into_system::AsSystem<Func> as into_system::SystemParamFunction<(), Out, (F0, F1), ()>>
              <into_system::AsSystem<Func> as into_system::SystemParamFunction<(), Out, (F0, F1, F2), ()>>
              <into_system::AsSystem<Func> as into_system::SystemParamFunction<(), Out, (F0, F1, F2, F3), ()>>
            and 20 others

(note: it appears to be a compiler quirk that the one-parameter tuple variant isn't printed here -- I did check that I could get my code to compile if the system signature was correct)

I suspect that the compiler is not printing why the implementations can't be matched because there are so many candidates. I know I've seen it print more guidance in simpler cases (when there is just one candidate, for instance).

Another idea: maybe it would work to use an attribute procmacro to mark functions as "intended to be System"? (probably the macro would spit out an impl System block) It looks to me like attribute macros are passed the AST, which should be enough to flag most problems.

[alice-i-cecile]

Something I should have linked on this thread earlier: bevycheck a wonderful macro crate for helping you debug your systems.

@concave-sphere does this line up with your attribute procmacro idea?

[concave-sphere]

@alice-i-cecile Oh, that looks great! Will definitely cut down on my debug cycles. IMO this should be the default behavior for the ECS.

One note: the page says:

bevycheck can't figure out all valid system parameters, for example, custom types with #[derive(SystemParam)] won't work.

I wonder if this could be fixed by adding where clauses for types that "ought to" be system parameters? e.g, the macro could turn this:

fn my_cool_system(Query<&Foo, With<Bar>>, some_param: &SomeParam) {
  code_goes_here();
}

into this:

fn my_cool_system(Query<&Foo, With<Bar>>, some_param: &SomeParam) where
    Foo: bevy_ecs::component::Component,
    Bar: bevy_ecs::component::Component,
    SomeParam: bevy_ecs::system::SystemParam,
{
  code_goes_here();
}

I just checked in the playground, and while placing type bounds on concrete types is weird, it works fine (including generating sensible compile errors when they don't match).

[alice-i-cecile]

I've made a reproduction of the problems at https://github.com/alice-i-cecile/bevy_compiler_errors.