Point at source of trait bound obligations in more places

[estebank]

Be more thorough in using ItemObligation and BindingObligation when
evaluating obligations so that we can point at trait bounds that
introduced unfulfilled obligations. We no longer incorrectly point at
unrelated trait bounds (substs-ppaux.verbose.stderr).

In particular, we now point at trait bounds on method calls.

We no longer point at "obvious" obligation sources (we no longer have a
note pointing at Trait saying "required by a bound in Trait", like
in associated-types-no-suitable-supertrait*).

We no longer point at associated items (ImplObligation), as they didn't
add any user actionable information, they just added noise.

Address part of #89418.

[rust-highfive]

r? @nagisa

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

[estebank]

I wonder if the error is because predicates_of now returns a DUMMY_SP for the trait's obligation. That would only affect this test if the span is the only thing being tracked in incremental, which would be a bug, right?

Edit: We can land this with extra verbosity pointing at traits, I can change the test to ignore predicates_of or I can rework the entire approach to try and signal that an obligation is coming from a trait in some other out of band way, instead of with DUMMY_SP (which arguably is the right thing to do, we just don't carry that kind of metadata today to tie the predicate to the ObligationCauseCode).

@cjgillot, I see that you touched this file. Do you know if there's any reliance on predicates_of changing when trait modifiers like pub and unsafe are changed? Currently they do because the change of the span, but with this PR's change, that's no longer true. Would it be reasonable to change the test to no longer look for that?

Another option would be to add an empty predicate type that represents pub and unsafe as a safeguard, but we likely don't want/need that.

Edit 2: I was halfway through atttempting the "signal origin in PredicateKind" approach when remembered that naïvely extending PredicateKind will cause fingerprinting issues, as I already tried to do something similar some time back and had to back off.

[cjgillot]

Should this test be performed around the hir.span(hir_id) call?

[estebank]

I thought about it, but because it is in a nested match doing that would have meant making span mutable and reassign to it in that case. Doing this kind of rebinding feels to me like a better approach most of the time.

[cjgillot]

@cjgillot, I see that you touched this file. Do you know if there's any reliance on predicates_of changing when trait modifiers like pub and unsafe are changed? Currently they do because the change of the span, but with this PR's change, that's no longer true. Would it be reasonable to change the test to no longer look for that?

I don't know enough the trait solving algorithm to comment on the need for the value to change when pub and unsafe change. OTOH, I would consider that any incremental code that relies on span-related invalidation to work correctly would be a red flag. So IMHO, it's fine to update the test.

If you want confirmation, you'd need to ask to professional trait solvers like @jackh726.

[jackh726]

If you want confirmation, you'd need to ask to professional trait solvers like @jackh726.

😂❤you give me too much credit

Do you know if there's any reliance on predicates_of changing when trait modifiers like pub and unsafe are changed?

I would be extremely surprised if this were the case. I imagine for everything but diagnostics, we throw away the span anyways.

[nagisa]

This seems… weird. Self::DIM is not a… bound, is it? Though it looks like this error itself is not quite correct in the first place.

[estebank]

Associated consts in array length position cause these. It's a preexisting problem that it's only exposed here because of better obligation tracking.

[nagisa]

Looked through the test changes for now, there seem to be some unfortunate regressions at least from my perspective.

[nagisa]

This looks like a regression?

[estebank]

It kind of is, but I feel it is acceptable given how many other things improve by screwing this one up (by using DUMMY_SP for trait implicit obligations).

[nagisa]

This seems like a regression? Is it unavoidable?

[estebank]

The previous note span was wrong :-|

[estebank]

After the last change we no longer show this note (and arguably never should have).

[nagisa]

This seems very nice. r=me with one remaining nit.

[estebank]

@bors r=nagisa

[bors]

📌 Commit a8dcc87 has been approved by nagisa

[rust-timer]

Finished benchmarking commit (b8e5ab2): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

• Large improvement in instruction counts (up to -5.0% on incr-unchanged builds of inflate)
• Small regression in instruction counts (up to 1.1% on incr-unchanged builds of wg-grammar)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

[pnkfelix]

Visiting for weekly performance triage.

Many other PR's over the past week have observed a similar delta to incr-unchanged builds of inflate, so I am dismissing that change as spurious. (The current hypothesis is that it is due to rust-lang/rustc-perf#1105.)

Other than that, there were a broad set of small regressions. Putting aside the ones tagged with ? ("noisy"), there are 19 benchmarks that regressed by 0.10% to 0.42%. This seems like an acceptable cost.

@rustbot label: +perf-regression-triaged