Add `do_not_recommend` annotation to `all_tuples` to improve error messages

[alice-i-cecile]

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

The error messages that Bevy / Rust provides when a malformed system, query or system set are provided are notoriously unhelpful (rust-lang/rust#89681). A huge complex error message is displayed, suggesting complex types in a tuple pyramid.

What solution would you like?

Per https://blog.weiznich.de/blog/diagnostic-namespace-do-not-recommend/, the #[diagnostic(do_not_recommend)] annotation is now ready to use, added in rust-lang/rust#124708.

We should add the do_not_recommend to the all_tuples! macro: these implementations are universally verbose and confusing.

Because this feature is not yet stable, we need to detect if it's available:

You can use #![cfg_attr(feature = "...", feature(test))]

https://users.rust-lang.org/t/add-unstable-feature-only-if-compiled-on-nightly/27886/2

What alternative(s) have you considered?

We could wait until this feature is stable to use it. This is less than ideal because a) if there are problems for our use case, we want to be able to fix them now. b) stabilization requires successful use in the wild, and Bevy is a prominent user of this functionality.

Or, we could just use the fabled generics. This will take even longer.

[TimJentzsch]

Is this actually available in nightly already? With version rustc 1.82.0-nightly (8e86c9567 2024-08-01) I still get a warning that this is an unknown diagnostic attribute:

warning: unknown diagnostic attribute
   --> crates/bevy_app/src/plugin.rs:167:27
    |
167 |             #[diagnostic::do_not_recommend]
    |                           ^^^^^^^^^^^^^^^^
...
182 |     all_tuples!(impl_plugins_tuples, 0, 15, P, S);
    |     --------------------------------------------- in this macro invocation
    |
    = note: this warning originates in the macro `impl_plugins_tuples` which comes from the expansion of the macro `all_tuples` (in Nightly builds, run with -Z macro-backtrace for more info)

Also what's the preferred way to guard this to nightly?
I tried to just add an #[allow(unknown_or_malformed_diagnostic_attributes)] to suppress the warning, as we know that it exists and diagnostic attributes should be ignored if they are missing.
But that doesn't seem to work.

If we need to go the feature path, I suppose a nightly feature should work? That would spread throughout the different crates though.

On the implementation side, I'm not sure if we can actually add this directly inside of all_tuples or if it needs to be added in each of the inner macros.
I couldn't make the former work yet, but I'm also new to Rust macros so maybe I'm doing something wrong.

[alice-i-cecile]

@weiznich, I'd very much appreciate your guidance here. I haven't worked much with either macros or nightly.

[ptrca]

I think #![feature(do_not_recommend)] should be added to crate root for it to work.
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=a51b02f42e8c87ee9ca7603379fad73e

Edit: It's actually usable on beta without the feature gate. We could suppress the unknown warning.
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=ee91276cb5f727384fb05ed71d4f46c0

[BD103]

I would prefer not to use a build script to detect whether Bevy is being compiled with the nightly toolchain or not. Build scripts are notorious for slowing down compilation time, and are generally risky to work with.

I would prefer to add an opt-in feature, or just wait until this attribute is released to stable Rust.

[weiznich]

Edit: It's actually usable on beta without the feature gate. We could suppress the unknown warning.
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=ee91276cb5f727384fb05ed71d4f46c0

Seems like you found a bug in the implementation. I will likely fill a PR soon to disallow that workaround.

I'd very much appreciate your guidance here. I haven't worked much with either macros or nightly.

It requires the !#[feature(do_not_recommend)] attribute for now. I think for pushing for stabilization it would be helpful to just have the feedback: We tried the attribute with a nightly compiler and got the following results (e.g. Improved that error message in this way, …).
Hopefully it shouldn't be too hard to stabilize this in the next months. I just want to wait some weeks (at least one release cycle) after finishing the implementation and adding the usage in rusts standard library so that I can argue: It's already used and it works there.

Crates like axum use essentially a buildscript/proc macro to detect the current compiler version via rustversion. That has as already pointed out by others a minimal compile time cost as it adds yet another dependency. You should likely measure the impact to get a better understanding whether it's expensive or not. I my experience trivial build scripts are almost free, at least compared to complex crates like axum or diesel.

[soqb]

i'll note that i'm not in favour of using rustversion, not because of buildscripts (although that may indeed be an issue), but because of the liberal use of attribute-form procedural macros.

the heuristics for deciding when proc macros should recompile are hazy at best (it can be as frequent as after any change in the enclosing file), and placing them over entire items/modules has been problematic in my experience.

my preferred solution would be to use cargo feature gating, either a single feature, "unstable-nightly", or several features like "unstable-do-not-recommend" (which is perhaps a bit verbose).