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

Introduce #[do_not_recommend] to control errors for trait impls #2397

Merged
merged 7 commits into from
Jul 2, 2018

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Apr 7, 2018

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Apr 7, 2018
@scottmcm scottmcm added the T-compiler Relevant to the compiler team, which will review and decide on the RFC. label Apr 7, 2018
@Ixrec
Copy link
Contributor

Ixrec commented Apr 7, 2018

At first glance, #[do_not_recommend] sounded to me like "do not recommend using this impl" which is obviously not what the RFC's talking about. Maybe it's not a big deal, but something like #[do_not_recommend_in_errors] seems like a tolerable length to me considering the target audience for this feature.

@Centril
Copy link
Contributor

Centril commented Apr 7, 2018

🚲🏠 We could call this #[error_no_suggest] which isn't exactly valid English, but is consistent with #[no_mangle] (which is not called do_not_mangle).

@sgrif
Copy link
Contributor Author

sgrif commented Apr 8, 2018

I like both suggestions. I agree the name could use some bikeshedding

@gilescope
Copy link

Could we try and draw parallels with how we hide something from rustdoc? I think they use 'hidden' and to me this is conceptually doing a similar thing.

confusing error messages. Perhaps when a function expects `Bar` and it's not
implemented, it would never make sense to implement `Foo` for that type. In this
case, we can put `#[do_not_recommend]` above our impl, and Rust will *never*
recommend implementing `Foo` as a way to get to `Bar`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little bikeshed here: the Guide-level explanation has an illustrating code example of the code we don't want, but not of the code we want. That would be:

#[do_not_recommend]
pub trait Foo {
}

pub trait Bar {
}

impl<T: Foo> Bar for T {
}

Is that right?

Copy link
Contributor

@jplatte jplatte Apr 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the #[do_not_recommend] is supposed to go on the impl, not on the trait. AFAICT, it is supposed to convey "don't recommend implementing Foo to get Bar", not "never recommend implementing Foo to get any other impl".

Copy link
Contributor

@skade skade Apr 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. And there I am, accidentally showing why it's useful to put the positive usage example in the guide ;).

@aturon
Copy link
Member

aturon commented Apr 26, 2018

This is an absolutely beautiful RFC; thank you @sgrif for the care you put into it! I'm very enthusiastic about this avenue for improving our errors, and of course this is a relatively low-risk/commitment feature to add.

The lang team also briefly discussed this RFC in our last meeting and in general the response was positive. So I'm going to go ahead and kick off full review:

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 26, 2018

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Apr 26, 2018
@joshtriplett
Copy link
Member

I do think it's worth further exploring what the option should be called. (Not trying to bikeshed, just genuinely agreeing with the already expressed sentiment that we want something that makes it clear this attribute controls suggestions in errors, and not, for instance, trait selection or specialization.) I like the idea of #[no_suggest], for instance.

That said, name aside, I agree with the remainder of the RFC. Checking my box...

@scottmcm
Copy link
Member

How does this compare to rustc_on_unimplemented? Would the cases that don't want something suggested be happier or sadder to have a custom message?

@sgrif
Copy link
Contributor Author

sgrif commented Apr 26, 2018

This is entirely orthogonal to rustc_on_unimplemented. It can affect whether the trait "suggested" is the one with a custom message or not, which may or may not be desirable depending on the traits and impls involved. That's why the scope of what this affects is very narrow. The case I gave in the body of the RFC around IntoIterator and Iterator is a good example of where the custom error message is actively confusing. However, if the reason some impl were missing was due to Send not being on some type parameter, that's a case where you do likely do want the custom message there.

@estebank
Copy link
Contributor

For a small contrarian opinion, for the presented example

for i in (1, 2, 3) {
    println!("{}", i);
}

we could slightly improve rustc_on_unimplemented to detect tuples (right now it can determine any types, but tuples are special because they do not share a common trait to query for) and instead emit

error[E0277]: can't iterate over tuple `({integer}, {integer}, {integer})`
 --> src/main.rs:2:14
  |
2 |     for x in (1, 2, 3) {
  |              ^^^^^^^^^ `({integer}, {integer}, {integer})` is not an iterator
  |
  = note: tuples are not iterable, XXX instead
  = note: the trait bound `({integer}, {integer}, {integer}): std::iter::Iterator` is not satisfied

That being said, this attribute could still be useful for non-rustc libraries.

This error message is particularly bad for a failed IntoIterator constraint.
The only type in std which has a method called iter that doesn't implement
IntoIterator is a fixed sized array. For all of those types, it's generally
more idiomatic to just put an & in front of the value.

I believe we can query for traits that are implemented, checking for IntoIterator and supplying a better message (this is not an iterator, but if you borrow it with & it will be) should be an easy win.

@sgrif
Copy link
Contributor Author

sgrif commented Apr 26, 2018

That would definitely be a nice addition, but has little to do with the issue at hand. Focusing specifically on IntoIterator, I'd like to re-iterate a point I made in the RFC text. The rustc_on_unimplemented message for Iterator is never helpful in a context expecting IntoIterator. The only type that does not implement IntoIterator, but does have a method called iter is a fixed size array, and in that case it's more idiomatic to put an & in front than to call iter. The error message for Iterator is useful in contexts expecting Iterator, but not IntoIterator.

@Nemo157
Copy link
Member

Nemo157 commented Apr 27, 2018

A relevant thread just appeared on u.rl.o: https://users.rust-lang.org/t/experimenting-with-generic-tryfrom-troubles-occur/17113, this is a different context to any of the current examples, specifically having the compiler suggest different trait bounds for a generic parameter. I assume this error could also be adjusted based on the proposed attribute?

@sgrif
Copy link
Contributor Author

sgrif commented Apr 27, 2018

Yes, this would allow that to be fixed if the attribute were placed on the impl<T: From<U>, U> TryFrom<U> for T. Whether that is a case we want to apply this is a separate discussion.

@Centril
Copy link
Contributor

Centril commented Apr 27, 2018

@sgrif Can you note the naming discussion as part of the unresolved questions? Then we can resolve it before stabilizing (or before merging the RFC if we like..).

@oli-obk
Copy link
Contributor

oli-obk commented May 18, 2018

@rfcbot reviewed

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels May 18, 2018
@petrochenkov
Copy link
Contributor

As long as @nikomatsakis thinks this is ok from implementation point of view, I'm ok with this too.

@nagisa
Copy link
Member

nagisa commented Jun 21, 2018

This feels like a very directed solution at a problem that’s not entirely understood. Not necessarily a bad thing, but I feel that if we thought more intensely about this, we might be able to come up with a solution that’s more general.

For example, one of the changes that would probably make this attribute more general is ability to place it on bounds rather than the whole impl (similarly to how we ended up implementing the #[may_dangle] attribute). In that case the library author could select which bounds they want to ignore for the purposes of the diagnostics and keep the current behaviour some other bounds, that the they expect the library user to satisfy.

Is that useful? I don’t know. It is just a first generalization that came to mind.

(/me is unsure whether this is something worth registering a fcp concern over or not)

@sgrif
Copy link
Contributor Author

sgrif commented Jun 21, 2018

@nagisa If anything that sounds more specific not more general. I do think that there's a pattern here in that most examples that this would apply to are Foo/IntoFoo pairs, but I don't think codifying that into the compiler is any better. Ultimately I opted for a very specific solution because I'm not sure the compiler can ever truly figure out "how deep" the author intends it to go, without getting it wrong some of the time.

Having it on the bounds doesn't really help at all in this case, since every single place that someone writes T: IntoIterator<Item = Foo> they'd need to annotate it or get the confusing error.

@cramertj
Copy link
Member

Ping @nagisa @estebank @Zoxc FCP here is blocked on approval from one of y'all.

@Aehmlo
Copy link

Aehmlo commented Jun 28, 2018

I’m sorry if this was mentioned above and I missed it, but what about a name like recommend(never)? This brings up the question of what other values are valid, and maybe we could discuss the validity of #[recommend($priority)] or something. Just a thought; I think the underlying idea here is a great one.

@moxian
Copy link

moxian commented Jun 29, 2018

This RFC title randomly caught my eye in the issue list, and I would like to voice my agreement with @Ixrec and @Centril above - the name is confusing and reads very much as "do not recommend using the method". (at least that was my initial impression upon seeing the RFC title).

Can this still be renamed/bikeshedded, or am I too late?

From my side I can suggest #[do_not_autosuggest]. Both #[error_no_suggest] and #[do_not_recommend_in_errors] mentioned above also sound fine to me - they are at the very least cryptic enough to not assume the "do not use" intention at first glance.

@Centril
Copy link
Contributor

Centril commented Jun 29, 2018

@moxian

The RFC explicitly has an unresolved question:

What other names could we go with besides #[do_not_recommend]?

This means that as part of stabilizing the feature, there will be a bikeshed which you are welcome to join. :)

@moxian
Copy link

moxian commented Jun 29, 2018

So, just to be clear, the name bikeshedding will happen not here, but in a separate "tracking issue" in rust-lang/rust repository, that would be created after this RFC is merged - is that correct?

@Centril
Copy link
Contributor

Centril commented Jun 29, 2018

@moxian Pretty much yeah.

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 1, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jul 1, 2018
@Centril Centril merged commit 15e4dfd into rust-lang:master Jul 2, 2018
@Centril
Copy link
Contributor

Centril commented Jul 2, 2018

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#51992

@Centril Centril added A-diagnostics Proposals relating to diagnostics (error messages). A-attributes Proposals relating to attributes labels Nov 23, 2018
weiznich added a commit to weiznich/rust that referenced this pull request Oct 23, 2024
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
weiznich added a commit to weiznich/rust that referenced this pull request Oct 23, 2024
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
weiznich added a commit to weiznich/rust that referenced this pull request Oct 24, 2024
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
weiznich added a commit to weiznich/rust that referenced this pull request Oct 28, 2024
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
weiznich added a commit to weiznich/rust that referenced this pull request Nov 6, 2024
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
weiznich added a commit to weiznich/rust that referenced this pull request Nov 18, 2024
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
weiznich added a commit to weiznich/rust that referenced this pull request Nov 30, 2024
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
weiznich added a commit to weiznich/rust that referenced this pull request Dec 4, 2024
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
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 16, 2024
…nd_final_tests, r=compiler-errors

Stabilize `#[diagnostic::do_not_recommend]`

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: 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

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.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 16, 2024
…nd_final_tests, r=compiler-errors

Stabilize `#[diagnostic::do_not_recommend]`

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: 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

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.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 16, 2024
…nd_final_tests, r=compiler-errors

Stabilize `#[diagnostic::do_not_recommend]`

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: 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

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.
weiznich added a commit to weiznich/rust that referenced this pull request Dec 17, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Proposals relating to attributes A-diagnostics Proposals relating to diagnostics (error messages). disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.