Reword "Required because of the requirements on the impl of ..."

[cbeuw]

Rephrases the awkward "Required because of the requirements on the impl of {trait} for {type}" to "required for {type} to implement {trait}"

[rust-highfive]

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

[Dylan-DPC]

@bors rollup=never

[compiler-errors]

@bors r-

i like this but maybe let's tweak this further. give me some time to do some thinking.

[Dylan-DPC]

@compiler-errors it wasnt approved, just marked never for rollup 😂

[bors]

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

[compiler-errors]

@Dylan-DPC -- ack, this is why I should fully wake up before I start to review emails. sorry for the mistake.

[compiler-errors]

Anyways, @cbeuw, maybe let's try some wording that still mentions this is a requirement that results from another:

maybe something along the lines of "required because of the the impl of X for Y"

[cbeuw]

required because of the the impl of X for Y

I feel like this is ambiguous as to whether an impl of X for Y currently exists. It could be read as if there is an impl X for Y somewhere - which generally isn't the case. When this note is emitted the code is demanding the type Y to implement X, but for various reasons this isn't the case, such as due to the lack of an explicit impl instance or a generic type bound isn't being satisfied

[compiler-errors]

@cbeuw, this is an ImplDerivedObligation, so this requirement should always be due to an impl's where-clause requirements (or implied bounds). The old messaging also mentioned that this was due to an impl, and i think it should preserve that.

Can you find me an example where mentioning that this is due to an impl is misleading?

[cbeuw]

Can you find me an example where mentioning that this is due to an impl is misleading?

When the trait is supposed to be derived. For example, this test

rust/src/test/ui/consts/const-blocks/fn-call-in-non-const.rs

Lines 1 to 16 in 0e21a27

	// Some type that is not copyable.
	struct Bar;
	const fn no_copy() -> Option<Bar> {
	None
	}
	const fn copy() -> u32 {
	3
	}
	fn main() {
	let _: [u32; 2] = [copy(); 2];
	let _: [Option<Bar>; 2] = [no_copy(); 2];
	//~^ ERROR the trait bound `Bar: Copy` is not satisfied
	}

The current message says "required because of the requirements on the impl of Copy for Option<Bar>", besides the roundabout phrasing, impl Copy for Option<Bar> does not exist in the code, and cannot be written. We want Option<Bar> to be Copy somehow, and in this case an impl is not relevent

And in cases where the error could be resolved with an explicit impl, it's not always possible to write it due to the orphan rule

rust/src/test/ui/integral-indexing.stderr

Lines 1 to 9 in 0e21a27

	error[E0277]: the type `[isize]` cannot be indexed by `u8`
	--> $DIR/integral-indexing.rs:6:5
	|
	LL | v[3u8];
	| ^^^^^^ slice indices are of type `usize` or ranges of `usize`
	|
	= help: the trait `SliceIndex<[isize]>` is not implemented for `u8`
	= help: the trait `SliceIndex<[T]>` is implemented for `usize`
	= note: required because of the requirements on the impl of `Index<u8>` for `Vec<isize>`

I would be nice if impl Index<u8> for Vec<isize> exists, but the user can't do anything about it.

As I pointed out above, this could also be misread in the opposite direction: that an impl Index<u8> for Vec<isize> currently exists somewhere which imposed some requirements that we failed to satisfy, whereas in reality it's the other way round, that our code demanded Vec<isize>: Index<u8>, which does not stand.

[compiler-errors]

As I pointed out above, this could also be misread in the opposite direction: that an impl Index for Vec currently exists somewhere which imposed some requirements that we failed to satisfy, whereas in reality it's the other way round, that our code demanded Vec: Index, which does not stand.

So the fact that this is an ImplDerivedObligation means that Rust is saying a bit more than just "our code demanded Vec<isize>: Index<u8>, which does not stand." It's saying that there is an impl which is compatible with this demand, but the where clauses of that impl (after we substitute any generics) are not satisfied.

The current message says "required because of the requirements on the impl of Copy for Option", besides the roundabout phrasing, impl Copy for Option does not exist in the code, and cannot be written. We want Option to be Copy somehow, and in this case an impl is not relevent

I guess what we disagree on is the fact that the impl exists. Side-note that I totally agree with "required because of the requirements" is a bit roundabout, but that's a bit unrelated with my concerns about this rewording.

While it's true that impl Copy for Option<Bar> doesn't exist written like that in any explicitly written or macro-generated code, the impl that ObligationCauseCode::ImplDerivedObligation is pointing to is the one that comes from the expansion of this derive(Copy), which does exist, and is the reason that the resulting trait bound Bar: Copy (from the primary error) is being required in the first place.

I guess we could be more explicit, and say something like "required due to the impl of Copy for Option<T> where T = Bar", but that seems a bit verbose.

The problem I have with your proposed wording is that loses the fact that this is a derived bound, and one that results from an impl (and not from, for example, the trait itself, or well-formedness, or any other reason why Ty: Trait would be required by the code).

[compiler-errors]

Given this sample code:

struct Foo;

trait Bar {}

trait Qux {}

impl<T> Bar for Option<T> where T: Qux {}

fn needs_bar(t: impl Bar) {}

fn main() {
    needs_bar(Some(Foo));
}

We currently say:

error[[E0277]](https://doc.rust-lang.org/stable/error-index.html#E0277): the trait bound `Foo: Qux` is not satisfied
  --> src/main.rs:12:15
   |
12 |     needs_bar(Some(Foo));
   |     --------- ^^^^^^^^^ the trait `Qux` is not implemented for `Foo`
   |     |
   |     required by a bound introduced by this call
   |
   = help: the trait `Bar` is implemented for `Option<T>`
note: required because of the requirements on the impl of `Bar` for `Option<Foo>`
  --> src/main.rs:7:9
   |
7  | impl<T> Bar for Option<T> where T: Qux {}
   |         ^^^     ^^^^^^^^^
note: required by a bound in `needs_bar`
  --> src/main.rs:9:22
   |
9  | fn needs_bar(t: impl Bar) {}
   |                      ^^^ required by this bound in `needs_bar`

I think that the proposed rewording is a bit awkward specifically when we're actually able to point out the impl in code:

note: required because `Option<Foo>` needs to implement `Bar`
  --> src/main.rs:7:9
   |
7  | impl<T> Bar for Option<T> where T: Qux {}
   |         ^^^     ^^^^^^^^^

[compiler-errors]

Side-note, we could probably specify two different messages depending on if the impl is able to be pointed out or not.

If we have a impl that is associated with a span we can point out (i.e. not DUMMY_SP, I think), we could probably say something like "required for this impl to satisfy {predicate}"

and if we don't, we could probably go with a slightly modified wording of "required for {type} to implement {trait}".

Thoughts?

[cbeuw]

I agree that impl<T> Copy for Option<T> where T: Copy exists as a result of derive macro expansion, though I like to think this different from impl Copy for Option<Bar>, especially when specialisation comes into play.

I think having a different message when a span is immediately printed below is a good idea, and I like the "required for X to do Y" wording

On another note, in your example, it said

the trait `Bar` is implemented for `Option<T>`

but not really. Maybe it should point out the relevant unsatisfied predicate, i.e. "the trait Bar is implemented for Option<T>, but requires T: Qux?

[JohnCSimon]

Ping from triage:
@cbeuw What is the status of this PR? Can you fix the merge conflicts?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

[cbeuw]

Sorry for leaving this for a while. I've now changed it to "required for {type} to implement {trait}".

I tried to refine it to "required for this impl to satisfy {predicate}" when the impl block is printed below. We already have a code path for this but I don't know how to get the where clause T: Qux in impl<T> Bar for Option<T> where T: Qux {} from a ImplDerivedObligationCause - it seems to only contain the parent obligation, but not the current one?
https://github.com/rust-lang/rust/blob/6fa6e2dc675035b782c292d939f309de2ab734d5/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs#L2556-L2566

@rustbot ready

[bors]

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

[compiler-errors]

Ugh, this is bitrotty of course.

Please rebase this, then r=me (i.e. write @bors r=compiler-errors in a github comment)

@bors delegate+ p=1

[bors]

📌 Commit 6fa6e2dc675035b782c292d939f309de2ab734d5 has been approved by `compiler-errors``

It is now in the queue for this repository.

[compiler-errors]

oops haha @bors r-

[cbeuw]

@bors r=compiler-errors

[bors]

@cbeuw: 🔑 Insufficient privileges: Not in reviewers

[cbeuw]

@rustbot ready

[compiler-errors]

I am dumb and forgot to delegate, sorry.

@bors r+

[bors]

📌 Commit 84a1993 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 84a1993 with merge 0b79f75...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 0b79f75 to master...

[rust-timer]

Finished benchmarking commit (0b79f75): comparison url.

Instruction count

• Primary benchmarks: ❌ relevant regression found
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions ❌ (primary)	1.0%	1.0%	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	1.0%	1

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.3%	3.3%	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	-2.0%	2
All ❌✅ (primary)	-	-	0

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions ❌ (primary)	4.8%	4.8%	1
Regressions ❌ (secondary)	4.0%	4.0%	1
Improvements ✅ (primary)	-2.8%	-2.8%	1
Improvements ✅ (secondary)	-4.2%	-4.2%	1
All ❌✅ (primary)	1.0%	4.8%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³