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

Don't suggest restricting bound with unstable traits on stable and mention it's unstable on nightly #133522

Merged
merged 10 commits into from
Dec 8, 2024
Next Next commit
Don't suggest restricting bound with unstable traits on stable
On nightly, we mention the trait is unstable

```
error[E0277]: the trait bound `T: Unstable` is not satisfied
  --> $DIR/unstable-trait-suggestion.rs:13:9
   |
LL |     foo(t)
   |     --- ^ the trait `Unstable` is not implemented for `T`
   |     |
   |     required by a bound introduced by this call
   |
note: required by a bound in `foo`
  --> $DIR/unstable-trait-suggestion.rs:9:11
   |
LL | fn foo<T: Unstable>(_: T) {}
   |           ^^^^^^^^ required by this bound in `foo`
help: consider restricting type parameter `T` but it is an `unstable` trait
   |
LL | pub fn demo<T: Unstable>(t: T) {
   |              ++++++++++
```

On stable, we don't suggest the trait at all

```
error[E0277]: the trait bound `T: Unstable` is not satisfied
  --> $DIR/unstable-trait-suggestion.rs:13:9
   |
LL |     foo(t)
   |     --- ^ the trait `Unstable` is not implemented for `T`
   |     |
   |     required by a bound introduced by this call
   |
note: required by a bound in `foo`
  --> $DIR/unstable-trait-suggestion.rs:9:11
   |
LL | fn foo<T: Unstable>(_: T) {}
   |           ^^^^^^^^ required by this bound in `foo`
```
  • Loading branch information
estebank committed Dec 7, 2024
commit 68253e14ee3d0e81881bc908b07b5ba141daf226
32 changes: 24 additions & 8 deletions compiler/rustc_middle/src/ty/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
//! Diagnostics related methods for `Ty`.

use std::borrow::Cow;
use std::fmt::Write;
use std::ops::ControlFlow;

Expand Down Expand Up @@ -278,8 +277,21 @@ pub fn suggest_constraining_type_params<'a>(
span_to_replace: Option<Span>,
) -> bool {
let mut grouped = FxHashMap::default();
let mut unstable_suggestion = false;
param_names_and_constraints.for_each(|(param_name, constraint, def_id)| {
grouped.entry(param_name).or_insert(Vec::new()).push((constraint, def_id))
let stable = match def_id {
Some(def_id) => match tcx.lookup_stability(def_id) {
Some(s) => s.level.is_stable(),
None => true,
},
None => true,
};
if stable || tcx.sess.is_nightly_build() {
grouped.entry(param_name).or_insert(Vec::new()).push((constraint, def_id));
if !stable {
unstable_suggestion = true;
}
}
});

let mut applicability = Applicability::MachineApplicable;
Expand Down Expand Up @@ -464,28 +476,32 @@ pub fn suggest_constraining_type_params<'a>(

if suggestions.len() == 1 {
let (span, suggestion, msg) = suggestions.pop().unwrap();
let post = if unstable_suggestion { " but it is an `unstable` trait" } else { "" };
let msg = match msg {
SuggestChangingConstraintsMessage::RestrictBoundFurther => {
Cow::from("consider further restricting this bound")
format!("consider further restricting this bound{post}")
}
SuggestChangingConstraintsMessage::RestrictType { ty } => {
Cow::from(format!("consider restricting type parameter `{ty}`"))
format!("consider restricting type parameter `{ty}`{post}")
}
SuggestChangingConstraintsMessage::RestrictTypeFurther { ty } => {
Cow::from(format!("consider further restricting type parameter `{ty}`"))
format!("consider further restricting type parameter `{ty}`{post}")
}
SuggestChangingConstraintsMessage::RemoveMaybeUnsized => {
Cow::from("consider removing the `?Sized` bound to make the type parameter `Sized`")
format!(
"consider removing the `?Sized` bound to make the type parameter `Sized`{post}"
)
}
SuggestChangingConstraintsMessage::ReplaceMaybeUnsizedWithSized => {
Cow::from("consider replacing `?Sized` with `Sized`")
format!("consider replacing `?Sized` with `Sized`{post}")
}
};

err.span_suggestion_verbose(span, msg, suggestion, applicability);
} else if suggestions.len() > 1 {
let post = if unstable_suggestion { " but some of them are `unstable` traits" } else { "" };
err.multipart_suggestion_verbose(
"consider restricting type parameters",
format!("consider restricting type parameters{post}"),
suggestions.into_iter().map(|(span, suggestion, _)| (span, suggestion)).collect(),
applicability,
);
Expand Down
10 changes: 5 additions & 5 deletions tests/ui/layout/rust-call-abi-not-a-tuple-ice-81974.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ LL | impl<A, B> FnOnce<A> for CachedFun<A, B>
|
note: required by a bound in `FnOnce`
--> $SRC_DIR/core/src/ops/function.rs:LL:COL
help: consider further restricting this bound
help: consider further restricting this bound but it is an `unstable` trait
Copy link
Member

Choose a reason for hiding this comment

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

nit: unstable is not a keyword, and "but" feels awkward here... maybe put it in parens and use "although" instead?

consider further restricting this bound (although this is an unstable trait)

Copy link
Member

Choose a reason for hiding this comment

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

maybe "although note that it is unstable" in the parentheses?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think the "it" here is a bit unclear, on first reading I thought "it" referred to T and was then thrown off by T being an unstable trait

Perhaps the trait could be named or something like "the new bound" or "the bound added below" be used instead?

Copy link
Member

Choose a reason for hiding this comment

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

the "it" is the trait, not really the bound

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"but note that the trait is unstable"?

Copy link
Member

Choose a reason for hiding this comment

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

sure, but if you're going to use "but" please still put it in parentheses or at least add a comma, since it's still a bit awkward grammatically

Copy link
Member

Choose a reason for hiding this comment

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

Drive-by comment: perhaps it would be easier if the types were explicitly named? For example:

error[E0277]: functions with the "rust-call" ABI must take a single non-self tuple argument
  --> $DIR/rust-call-abi-not-a-tuple-ice-81974.rs:31:5
   |
LL |     extern "rust-call" fn call_once(mut self, a: A) -> Self::Output {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Tuple` is not implemented for `A`
   |
help: consider adding the unstable trait `Tuple` to the bounds of `A`
   |
LL |     A: Eq + Hash + Clone + std::marker::Tuple,
   |                          ++++++++++++++++++++

Then the stability or non-stability can be added via the phrase "consider adding the trait Tuple to the bounds of A" / "consider adding the unstable trait Tuple to the bounds of A`".

Alternatively, it could be suggested via a separate note:

error[E0277]: functions with the "rust-call" ABI must take a single non-self tuple argument
  --> $DIR/rust-call-abi-not-a-tuple-ice-81974.rs:31:5
   |
LL |     extern "rust-call" fn call_once(mut self, a: A) -> Self::Output {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Tuple` is not implemented for `A`
   |
help: consider adding the trait `Tuple` to the bounds of `A`
   |
LL |     A: Eq + Hash + Clone + std::marker::Tuple,
   |                          ++++++++++++++++++++
note: the trait `Tuple` is unstable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to "help: consider restricting type parameter T with unstable trait Unstable".

|
LL | A: Eq + Hash + Clone + std::marker::Tuple,
| ++++++++++++++++++++
Expand All @@ -19,7 +19,7 @@ LL | impl<A, B> FnMut<A> for CachedFun<A, B>
|
note: required by a bound in `FnMut`
--> $SRC_DIR/core/src/ops/function.rs:LL:COL
help: consider further restricting this bound
help: consider further restricting this bound but it is an `unstable` trait
|
LL | A: Eq + Hash + Clone + std::marker::Tuple,
| ++++++++++++++++++++
Expand All @@ -30,7 +30,7 @@ error[E0277]: functions with the "rust-call" ABI must take a single non-self tup
LL | extern "rust-call" fn call_once(mut self, a: A) -> Self::Output {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Tuple` is not implemented for `A`
|
help: consider further restricting this bound
help: consider further restricting this bound but it is an `unstable` trait
|
LL | A: Eq + Hash + Clone + std::marker::Tuple,
| ++++++++++++++++++++
Expand All @@ -41,7 +41,7 @@ error[E0277]: functions with the "rust-call" ABI must take a single non-self tup
LL | extern "rust-call" fn call_mut(&mut self, a: A) -> Self::Output {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Tuple` is not implemented for `A`
|
help: consider further restricting this bound
help: consider further restricting this bound but it is an `unstable` trait
|
LL | A: Eq + Hash + Clone + std::marker::Tuple,
| ++++++++++++++++++++
Expand All @@ -56,7 +56,7 @@ LL | self.call_mut(a)
|
note: required by a bound in `call_mut`
--> $SRC_DIR/core/src/ops/function.rs:LL:COL
help: consider further restricting this bound
help: consider further restricting this bound but it is an `unstable` trait
|
LL | A: Eq + Hash + Clone + std::marker::Tuple,
| ++++++++++++++++++++
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/mir/validate/validate-unsize-cast.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ note: required by a bound in `CastTo`
|
LL | pub trait CastTo<U: ?Sized>: Unsize<U> {}
| ^^^^^^^^^ required by this bound in `CastTo`
help: consider further restricting this bound
help: consider further restricting this bound but it is an `unstable` trait
|
LL | impl<T: ?Sized + std::marker::Unsize<U>, U: ?Sized> CastTo<U> for T {}
| ++++++++++++++++++++++++
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/trait-bounds/unstable-trait-suggestion.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#![feature(staged_api)]
#![allow(internal_features)]
#![stable(feature = "unit_test", since = "1.0.0")]

#[unstable(feature = "step_trait", issue = "42168")]
pub trait Unstable {}

#[stable(feature = "unit_test", since = "1.0.0")]
fn foo<T: Unstable>(_: T) {}

#[stable(feature = "unit_test", since = "1.0.0")]
pub fn demo<T>(t: T) { //~ HELP consider restricting type parameter `T` but it is an `unstable` trait
foo(t) //~ ERROR E0277
}
fn main() {}
21 changes: 21 additions & 0 deletions tests/ui/trait-bounds/unstable-trait-suggestion.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error[E0277]: the trait bound `T: Unstable` is not satisfied
--> $DIR/unstable-trait-suggestion.rs:13:9
|
LL | foo(t)
| --- ^ the trait `Unstable` is not implemented for `T`
| |
| required by a bound introduced by this call
|
note: required by a bound in `foo`
--> $DIR/unstable-trait-suggestion.rs:9:11
|
LL | fn foo<T: Unstable>(_: T) {}
| ^^^^^^^^ required by this bound in `foo`
help: consider restricting type parameter `T` but it is an `unstable` trait
|
LL | pub fn demo<T: Unstable>(t: T) {
| ++++++++++

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.
2 changes: 1 addition & 1 deletion tests/ui/tuple/builtin-fail.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ note: required by a bound in `assert_is_tuple`
|
LL | fn assert_is_tuple<T: std::marker::Tuple + ?Sized>() {}
| ^^^^^^^^^^^^^^^^^^ required by this bound in `assert_is_tuple`
help: consider restricting type parameter `T`
help: consider restricting type parameter `T` but it is an `unstable` trait
|
LL | fn from_param_env<T: std::marker::Tuple>() {
| ++++++++++++++++++++
Expand Down