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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

estebank
Copy link
Contributor

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`

Fix #133511.

@rustbot
Copy link
Collaborator

rustbot commented Nov 27, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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. labels Nov 27, 2024
@@ -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".

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`
```
@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Nov 28, 2024
@@ -4,7 +4,7 @@ error[E0277]: the trait bound `C: Bar<5>` is not satisfied
LL | pub struct Structure<C: Tec> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Bar<5>` is not implemented for `C`
|
help: consider further restricting this bound
help: consider further restricting this bound with trait `Bar<5>`
Copy link
Member

Choose a reason for hiding this comment

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

Could you change this to use print_only_trait_name rather than printing the binder and the args?

I first of all don't think mentioning the name is really that useful, since it's already there in the suggestion, but if we're going to mention it at all, it's probably only worthwhile to suggest only the trait name.

Copy link
Member

Choose a reason for hiding this comment

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

Rendering the binder and substs runs the risk of printing a really overly long trait ref here, since we may have large types in the args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could only mention the trait for unsafe ones (as I originally intended in the PR, to keep the diff small).

The only issue against using the "name only" is when we have more than one bound being suggested, but we have to handle that anyways...

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine either way, was just mentioning that it feels a bit redundant. Name only seems like a good compromise.

Copy link
Contributor Author

@estebank estebank Nov 28, 2024

Choose a reason for hiding this comment

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

Addressed (edit: but not on this file ? edit 2: tests/rustdoc-ui 🤦 edit 3: actually, just github linking to the older version 2x🤦). It adds a bunch of logic, but it does look better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was just mentioning that it feels a bit redundant

Yeah, but that was a review request though. I feel like it might be useful for the sake of suggestions being presented without context. I don't feel strongly one way or another.

@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Nov 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

This test will break when `Step` gets stabilized, but punt until then.
```
help: consider restricting type parameter `T` with traits `Copy` and `Trait`
   |
LL | fn duplicate_custom<T: Copy + Trait>(t: S<T>) -> (S<T>, S<T>) {
   |                      ++++++++++++++
```

```
help: consider restricting type parameter `V` with trait `Copy`
   |
LL | fn index<'a, K, V: std::marker::Copy>(map: &'a HashMap<K, V>, k: K) -> &'a V {
   |                  +++++++++++++++++++
```
@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2024

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@rust-log-analyzer

This comment has been minimized.

Pass in an appropriate `Option<DefId>` in more cases from hir ty lowering.
@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2024

HIR ty lowering was modified

cc @fmease

@compiler-errors
Copy link
Member

I'll review later

@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 3, 2024

📌 Commit 05e6714 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 3, 2024
…it, r=compiler-errors

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

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

Fix rust-lang#133511.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#133089 (Stabilize noop_waker)
 - rust-lang#133522 (Don't suggest restricting bound with unstable traits on stable and mention it's unstable on nightly)
 - rust-lang#133545 (Lint against Symbol::intern on a string literal)
 - rust-lang#133558 (Structurally resolve in `probe_adt`)
 - rust-lang#133753 (Reduce false positives on some common cases from if-let-rescope lint)
 - rust-lang#133762 (stabilize const_{size,align}_of_val)
 - rust-lang#133777 (document -Zrandomize-layout in the unstable book)
 - rust-lang#133779 (Use correct `hir_id` for array const arg infers)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Dec 3, 2024
…it, r=compiler-errors

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

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

Fix rust-lang#133511.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 3, 2024
…it, r=compiler-errors

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

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

Fix rust-lang#133511.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133522 (Don't suggest restricting bound with unstable traits on stable and mention it's unstable on nightly)
 - rust-lang#133545 (Lint against Symbol::intern on a string literal)
 - rust-lang#133558 (Structurally resolve in `probe_adt`)
 - rust-lang#133753 (Reduce false positives on some common cases from if-let-rescope lint)
 - rust-lang#133762 (stabilize const_{size,align}_of_val)
 - rust-lang#133777 (document -Zrandomize-layout in the unstable book)
 - rust-lang#133779 (Use correct `hir_id` for array const arg infers)

r? `@ghost`
`@rustbot` modify labels: rollup
@@ -16,7 +16,7 @@ LL | trait UnsafeCopy<'a, T: Copy>
LL | where
LL | for<'b> <Self as UnsafeCopy<'b, T>>::Item: std::ops::Deref<Target = T>,
| ^^^^^^^^^^ required by this bound in `UnsafeCopy`
help: consider further restricting this bound
help: consider further restricting this bound with `<Target = T>`
Copy link
Member

Choose a reason for hiding this comment

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

nit: It looks like an extra space snuck in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That output is already not great... I think I'll do a iollow up just reverting to the previous message for that one case. There's another for "with , Output = Ty" which I don't love either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stable rustc 1.82 suggests using an unstable trait
8 participants