Stabilize `#[diagnostic::do_not_recommend]` #132056

weiznich · 2024-10-23T06:41:01Z

This PR seeks to stabilize the #[diagnostic::do_not_recommend]attribute.

This attribute was first proposed as #[do_not_recommend] attribute in RFC 2397 (rust-lang/rfcs#2397). It gives the crate authors the ability to not suggest to the compiler to not show certain traits in its error messages.

With the presence of the #[diagnostic] tool attribute namespace it was decided to move the attribute there, as that lowers the amount of guarantees the compiler needs to give about the exact way this influences error messages. It turns the attribute into a hint which can be ignored. In addition to the original proposed functionality this attribute now also hides the marked trait in help messages ("This trait is implemented by: ").

The attribute does not accept any argument and can only be placed on trait implementations. If it is placed somewhere else a lint warning is emitted and the attribute is otherwise ignored. If an argument is detected a lint warning is emitted and the argument is ignored. This follows the rules outlined by the diagnostic namespace.

This attribute allows crates like diesel to improve their error messages drastically. The most common example here is the following error message:

error[E0277]: the trait bound `&str: Expression` is not satisfied
  --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:53:15
   |
LL |     SelectInt.check("bar");
   |               ^^^^^ the trait `Expression` is not implemented for `&str`, which is required by `&str: AsExpression<Integer>`
   |
   = help: the following other types implement trait `Expression`:
             Bound<T>
             SelectInt
note: required for `&str` to implement `AsExpression<Integer>`
  --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:26:13
   |
LL | impl<T, ST> AsExpression<ST> for T
   |             ^^^^^^^^^^^^^^^^     ^
LL | where
LL |     T: Expression<SqlType = ST>,
   |        ------------------------ unsatisfied trait bound introduced here

By applying the new attribute to the wild card trait implementation of
AsExpression for T: Expression the error message becomes:

error[E0277]: the trait bound `&str: AsExpression<Integer>` is not satisfied
  --> $DIR/as_expression.rs:55:15
   |
LL |     SelectInt.check("bar");
   |               ^^^^^ the trait `AsExpression<Integer>` is not implemented for `&str`
   |
   = help: the trait `AsExpression<Text>` is implemented for `&str`
   = help: for that trait implementation, expected `Text`, found `Integer`

which makes it much easier for users to understand that they are facing a type mismatch.

Other explored example usages include:

This standard library error message: Start using #[diagnostic::do_not_recommend] in the standard library #128008
That bevy derived example:
https://github.com/rust-lang/rust/blob/e1f306899514ea80abc1d1c9f6a57762afb304a3/tests/ui/diagnostic_namespace/do_not_recommend/supress_suggestions_in_help.rs (No
more tuple pyramids)

Fixes #51992

r? @compiler-errors

This PR also adds a few more tests, makes sure that all the tests are run for the old and new trait solver and adds a check that the attribute does not contain arguments.

compiler/rustc_passes/src/errors.rs

Noratrieb · 2024-10-23T09:35:56Z

what's the process for stabilizing a diagnostic attr again? i think there was one

compiler-errors · 2024-10-23T13:37:26Z

There is no process yet; iirc, T-lang has expressed that they'd like to get involved in the stabilization of the first few of these attributes just to see how it goes. Ideally, after this stabilization, we should work towards establishing a better policy for this, though.

@rustbot label: I-lang-nominated

bors · 2024-10-23T16:13:13Z

☔ The latest upstream changes (presumably #132027) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2024-10-23T17:14:53Z

Can you please post a documentation PR before stabilizing?

weiznich · 2024-10-24T05:42:53Z

@ehuss I've opened rust-lang/reference#1663 for that

ehuss · 2024-10-24T16:26:26Z

compiler/rustc_passes/src/check_attr.rs

+        hir_id: HirId,
+        target: Target,
+        attr: &Attribute,
+    ) {
        if !matches!(target, Target::Impl) {


Should this also be checking that the target is a trait impl and not inherent?
Otherwise it seems to accept #[diagnostic::do_not_recommend] on impl T {}

Good catch. I've pushed 3d814b3 to fix this.

bors · 2024-10-26T02:00:55Z

☔ The latest upstream changes (presumably #131349) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-11-04T21:04:34Z

☔ The latest upstream changes (presumably #132603) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-11-17T19:37:47Z

☔ The latest upstream changes (presumably #133135) made this pull request unmergeable. Please resolve the merge conflicts.

This PR aims to improve the testing coverage for `#[diagnostic::do_not_recommend]`. It ensures that all tests are run for the old and new solver to verify that the behaviour is the same for both variants. It also adds two new tests: * A test with 4 traits having wild card impl for each other, with alternating `#[diagnostic::do_not_recommend]` attributse * A test with a lifetime dependend wild card impl, which is something that's not supported yet

This commit adds a check that verifies that no arguments are passed to `#[diagnostic::do_not_recommend]`. If we detect arguments we emit a warning.

This commit seeks to stabilize the `#[diagnostic::do_not_recommend]` attribute. This attribute was first proposed as `#[do_not_recommend`] attribute in RFC 2397 (rust-lang/rfcs#2397). It gives the crate authors the ability to not suggest to the compiler to not show certain traits in it's error messages. With the presence of the `#[diagnostic]` tool attribute namespace it was decided to move the attribute there, as that lowers the amount of guarantees the compiler needs to give about the exact way this influences error messages. It turns the attribute into a hint which can be ignored. In addition to the original proposed functionality this attribute now also hides the marked trait in help messages ("This trait is implemented by: "). The attribute does not accept any argument and can only be placed on trait implementations. If it is placed somewhere else a lint warning is emitted and the attribute is otherwise ignored. If an argument is detected a lint warning is emitted and the argument is ignored. This follows the rules outlined by the diagnostic namespace. This attribute allows crates like diesel to improve their error messages drastically. The most common example here is the following error message: ``` error[E0277]: the trait bound `&str: Expression` is not satisfied --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:53:15 | LL | SelectInt.check("bar"); | ^^^^^ the trait `Expression` is not implemented for `&str`, which is required by `&str: AsExpression<Integer>` | = help: the following other types implement trait `Expression`: Bound<T> SelectInt note: required for `&str` to implement `AsExpression<Integer>` --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:26:13 | LL | impl<T, ST> AsExpression<ST> for T | ^^^^^^^^^^^^^^^^ ^ LL | where LL | T: Expression<SqlType = ST>, | ------------------------ unsatisfied trait bound introduced here ``` By applying the new attribute to the wild card trait implementation of `AsExpression` for `T: Expression` the error message becomes: ``` error[E0277]: the trait bound `&str: AsExpression<Integer>` is not satisfied --> $DIR/as_expression.rs:55:15 | LL | SelectInt.check("bar"); | ^^^^^ the trait `AsExpression<Integer>` is not implemented for `&str` | = help: the trait `AsExpression<Text>` is implemented for `&str` = help: for that trait implementation, expected `Text`, found `Integer` ``` which makes it much easier for users to understand that they are facing a type mismatch. Other explored example usages included * This standard library error message: rust-lang#128008 * That bevy derived example: https://github.com/rust-lang/rust/blob/e1f306899514ea80abc1d1c9f6a57762afb304a3/tests/ui/diagnostic_namespace/do_not_recommend/supress_suggestions_in_help.rs (No more tuple pyramids) Fixes rust-lang#51992

traviscross · 2024-11-20T16:23:43Z

We reviewed this on the triage call today, and it seemed reasonable to us, so...

@rfcbot fcp merge

traviscross · 2024-11-20T16:24:08Z

@rfcbot fcp merge

rfcbot · 2024-11-20T16:24:26Z

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

rfcbot · 2024-11-20T16:28:13Z

🔔 This is now entering its final comment period, as per the review above. 🔔

traviscross · 2024-11-20T17:16:54Z

@rustbot labels -I-lang-nominated

We discussed this in lang triage today, resulting the FCP above. Thanks @weiznich for putting forward this stabilization.

rustbot assigned compiler-errors Oct 23, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 23, 2024

This comment has been minimized.

Sign in to view

weiznich force-pushed the diagnostic_do_not_recommend_final_tests branch from 367f8b3 to ae7979b Compare October 23, 2024 07:10

lqd reviewed Oct 23, 2024

View reviewed changes

compiler/rustc_passes/src/errors.rs Outdated Show resolved Hide resolved

weiznich force-pushed the diagnostic_do_not_recommend_final_tests branch from ae7979b to 797351d Compare October 23, 2024 12:18

rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 23, 2024

weiznich force-pushed the diagnostic_do_not_recommend_final_tests branch from 797351d to a8f82b1 Compare October 24, 2024 05:29

weiznich mentioned this pull request Oct 24, 2024

Add documentation for #[diagnostic::do_not_recommend] rust-lang/reference#1663

Open

ehuss reviewed Oct 24, 2024

View reviewed changes

weiznich force-pushed the diagnostic_do_not_recommend_final_tests branch from 3d814b3 to e760622 Compare October 28, 2024 07:18

compiler-errors added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2024

weiznich force-pushed the diagnostic_do_not_recommend_final_tests branch from e760622 to db7239a Compare November 6, 2024 08:25

weiznich added 4 commits November 18, 2024 08:08

Check #[diagnostic::do_not_recommend] for arguments

15c81ec

This commit adds a check that verifies that no arguments are passed to `#[diagnostic::do_not_recommend]`. If we detect arguments we emit a warning.

Also warn against #[diagnostic::do_not_recommend] on plain impls

3b184b6

weiznich force-pushed the diagnostic_do_not_recommend_final_tests branch from db7239a to 3b184b6 Compare November 18, 2024 07:37

rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize `#[diagnostic::do_not_recommend]` #132056

Stabilize `#[diagnostic::do_not_recommend]` #132056

weiznich commented Oct 23, 2024 •

edited by compiler-errors

Loading

This comment has been minimized.

Noratrieb commented Oct 23, 2024

compiler-errors commented Oct 23, 2024

bors commented Oct 23, 2024

ehuss commented Oct 23, 2024

weiznich commented Oct 24, 2024

ehuss Oct 24, 2024

weiznich Oct 25, 2024

bors commented Oct 26, 2024

bors commented Nov 4, 2024

bors commented Nov 17, 2024

traviscross commented Nov 20, 2024

traviscross commented Nov 20, 2024

rfcbot commented Nov 20, 2024 •

edited by pnkfelix

Loading

rfcbot commented Nov 20, 2024

traviscross commented Nov 20, 2024

Stabilize #[diagnostic::do_not_recommend] #132056

Are you sure you want to change the base?

Stabilize #[diagnostic::do_not_recommend] #132056

Conversation

weiznich commented Oct 23, 2024 • edited by compiler-errors Loading

This comment has been minimized.

Noratrieb commented Oct 23, 2024

compiler-errors commented Oct 23, 2024

bors commented Oct 23, 2024

ehuss commented Oct 23, 2024

weiznich commented Oct 24, 2024

ehuss Oct 24, 2024

Choose a reason for hiding this comment

weiznich Oct 25, 2024

Choose a reason for hiding this comment

bors commented Oct 26, 2024

bors commented Nov 4, 2024

bors commented Nov 17, 2024

traviscross commented Nov 20, 2024

traviscross commented Nov 20, 2024

rfcbot commented Nov 20, 2024 • edited by pnkfelix Loading

rfcbot commented Nov 20, 2024

traviscross commented Nov 20, 2024

Stabilize `#[diagnostic::do_not_recommend]` #132056

Stabilize `#[diagnostic::do_not_recommend]` #132056

weiznich commented Oct 23, 2024 •

edited by compiler-errors

Loading

rfcbot commented Nov 20, 2024 •

edited by pnkfelix

Loading