Improve some error messages for bevy_ecs generated by nightly rustc

[weiznich]

This commit introduces a new feature flags that let's user opt into the
nightly only #[rustc_on_unimplemented] attribute. This attribute can
be used to improve error messages generated by missing trait
implementations. This commit adds annotations to two locations in bevy
to improve error messages submitted as part of
weiznich/rust-foundation-community-grant#3

Addresses #1519

[weiznich]

See weiznich/rust-foundation-community-grant@e69f4f7#diff-c059c07feb90f777392b96c4c59911c9b5895882682eff9b25f7f0539338b6b0 for the relevant output changes.

The exact wording of the error messages is up to discussion and can be easily changed.

[alice-i-cecile]

We should match this for the Component trait :)

[alice-i-cecile]

I am extremely excited to see the upstream work start to pay off :)

Is there any way to automatically detect whether the user is on a compatible nightly version? This is most useful for beginners, so it's a shame it has to be opt-in.

[alice-i-cecile]

The IntoSystemDescriptor error message is the other critical pain point; would that be feasible to address in this PR or are there limitations that would block that?

[mockersf]

It seems rustc_on_unimplemented is never going to be stabilised, and not meant to be used outside of rustc. If that's the case, I don't think it's usable for Bevy

[weiznich]

Is there any way to automatically detect whether the user is on a compatible nightly version? This is most useful for beginners, so it's a shame it has to be opt-in.

I didn't check that, but I would expect that any nightly from the last few years is compatible. The attribute does not really change much as far as I know. ekuber states that there were maybe 3 changes in the last 5 years, all of them backward compatible. As for the opt-in part: See the last part of the comment. At least I as diesel maintainer would be quite interested in having something like that on stable. Maybe we can suggest something like that to the compiler team if there is demand in the ecosystem.

The IntoSystemDescriptor error message is the other critical pain point; would that be feasible to address in this PR or are there limitations that would block that?

I do not know the code base well enough to say if that would be feasible or not. Generally speaking if the error messages is a "… trait not implemented …" error message, then it's likely possible. I might have a look at that if you can provide an example.

It seems rustc_on_unimplemented is never going to be stabilised, and not meant to be used outside of rustc. If that's the case, I don't think it's usable for Bevy

That's partially correct. rustc_on_unimplemented will not be stabilized as far as I can tell. Although it can be used outside of rustc (as shown by this PR). There are considerations to stabilize basically the same thing without guaranteeing anything other than that it does not cause a compiler error if present. I personally want to investigate in some good use-cases outside of rustc for this attribute, so that there are exist a few examples of successful uses of this attribute. This hopefully gives some good arguments to stabilize it in some form. (My personal favorite would be using a tool attribute for this, so similar to the existing #[rustfmt::skip] there would be an #[rust::on_unimplemented] attribute, but that's up to future discussion.)

[weiznich]

Note to myself: I need to check if that actually does something or if it's unnecessary. In the second case I likely need to find a solution for false positives if Self is a tuple or something like that.

[alice-i-cecile]

I do not know the code base well enough to say if that would be feasible or not. Generally speaking if the error messages is a "… trait not implemented …" error message, then it's likely possible. I might have a look at that if you can provide an example.

Sure! Here's a simple failing snippet:

use bevy::prelude::*;

// Should be `mut commands: Commands` as `&mut Commands` doesn't implement `SystemParam`
fn broken_system(commands: &mut Commands) {}

fn main(){
    App::new().add_system(broken_system).run();
}

Maybe we can suggest something like that to the compiler team if there is demand in the ecosystem.

I'd be happy to help advocate for this; this style of work was mentioned in the lang team's roadmap as an area they'd like to move forward with and something I've personally discussed in the past with both Niko and Josh Triplett.

I agree that this work is valuable as a proof of concept for how this feature could improve end user UX.

I'm personally a bit nervous to merge this PR using rustc_on_unimplemented, but if @cart decides that this feature flag approach is acceptable I would want to insist on a step in CI that verifies that this continues to work as intended.

[alice-i-cecile]

I didn't check that, but I would expect that any nightly from the last few years is compatible. The attribute does not really change much as far as I know

@weiznich to clarify, I was wondering if there was a way to detect at runtime if the rustc version was nightly or stable, so we could avoid having a feature flag at all :)

[weiznich]

I'd be happy to help advocate for this; this style of work was mentioned in the lang team's roadmap as an area they'd like to move forward with and something I've personally discussed in the past with both Niko and Josh Triplett.

I should probably link this zulip discussion here as well as this seems to be highly relevant and currently ongoing.

@weiznich to clarify, I was wondering if there was a way to detect at runtime if the rustc version was nightly or stable, so we could avoid having a feature flag at all :)

As far as I know there is currently no way to detect which rustc version is used to compile the current crate. At least not without using a build.rs file and something like version_check or a similar approach. I personally prefer feature flags for such things as this gives users a finer control whether they want to use something that's not considered to be stable.

[cart]

I'm personally a bit nervous to merge this PR using rustc_on_unimplemented, but if @cart decides that this feature flag approach is acceptable I would want to insist on a step in CI that verifies that this continues to work as intend

If this is totally opt-in and the implementation scope is small (and it looks that way to me), I'm down to merge something in this vein if it helps move this compiler effort forward / prove these features are valuable.

Probably worth adding some comment explaining the rationale behind this / the scope of this effort.

That being said, if merging this (as opposed to just testing this branch ourselves and leaving feedback) doesn't directly benefit the effort to stabilize (ex: help answer unanswered questions), then I think we should wait for a stable api.

[weiznich]

I had some discussions with ekuber about how to stabilize the corresponding attribute. He tweeted about that here. The summary of that discussion is that we both agree on that it should be possible to stabilize this "soon". Maybe even without going through a language team decision by using the existing tool attribute RFC. I'm on holiday for the next weeks,after that I plan to fill a compiler MCP for this.

For this PR: It's up to you if you want to provide this improved error messages as early as possible or wait for a final compiler/language team decision. For diesel I recently shipped at least some uses of the corresponding attribute behind a feature flag, as I believe that can help users.

[cart]

Given that this message is intended to help new users, I think the added complexity of requiring nightly / enabling a feature counteracts the improved UX of the better error messages. I think we should probably wait until it stabilizes.

[alice-i-cecile]

Closing this out: we'll redo this properly with the stable equivalent soon :)