Point at argument instead of call for their obligations

[estebank]

When an obligation is introduced by a specific fn argument, point at
the argument instead of the fn call if the obligation fails to be
fulfilled.

Move the information about pointing at the call argument expression in
an unmet obligation span from the FulfillmentError to a new
ObligationCauseCode.

When giving an error about an obligation introduced by a function call
that an argument doesn't fulfill, and that argument is a block, add a
span_label pointing at the innermost tail expression.

Current output:

error[E0425]: cannot find value `x` in this scope
 --> f10.rs:4:14
  |
4 |         Some(x * 2)
  |              ^ not found in this scope

error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<_>`
 --> f10.rs:2:31
  |
2 |       let p = Some(45).and_then({
  |  ______________________--------_^
  | |                      |
  | |                      required by a bound introduced by this call
3 | |         |x| println!("doubling {}", x);
4 | |         Some(x * 2)
  | |         -----------
5 | |     });
  | |_____^ expected an `FnOnce<({integer},)>` closure, found `Option<_>`
  |
  = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<_>`

Previous output:

error[E0425]: cannot find value `x` in this scope
 --> f10.rs:4:14
  |
4 |         Some(x * 2)
  |              ^ not found in this scope

error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<_>`
 --> f10.rs:2:22
  |
2 |     let p = Some(45).and_then({
  |                      ^^^^^^^^ expected an `FnOnce<({integer},)>` closure, found `Option<_>`
  |
  = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<_>`

Partially address #27300. Will require rebasing on top of #88546.

[rust-highfive]

r? @nagisa

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

[nagisa]

Are there any cases where we introduce the "required by a bound introduced by this call" note on something else than the immediate method being called? So far all of the test cases seem to annotate the method and the source of the obligation seems pretty obvious. Due to this note the presentation of certain diagnostics such as the ones involving closures become fairly awkward due to the overlapping spans.

Seems good from the implementation perspective. Definitely looks cleaner than passing an argument around.

[estebank]

Are there any cases where we introduce the "required by a bound introduced by this call" note on something else than the immediate method being called? So far all of the test cases seem to annotate the method and the source of the obligation seems pretty obvious. Due to this note the presentation of certain diagnostics such as the ones involving closures become fairly awkward due to the overlapping spans.

I wanted to make it clear that the reason the argument had some bound imposed on it was because of the call. This might not be needed given that have the span_note immediately afterwards in (I think) all cases, so I can remove it (or leave it with a span label with no text).

If by the overlapping case you are referring to is https://github.com/rust-lang/rust/pull/88719/files/ec904af0fc3d1dcbbd3514139e7cf6f31ce0c58b#diff-003f5fb5b17c96d98b8183c313df38486269166791f27003c3b62a4bfe251b81R8-R20, then I would say that that error is going to look ugly no matter what :-/ If we don't point to the call, that error will be less clear, IMO. If you were referring to cases like https://github.com/rust-lang/rust/pull/88719/files/ec904af0fc3d1dcbbd3514139e7cf6f31ce0c58b#diff-a6816ee609a762d3ac2a91869bb43e3d53ed29359a9259b011664dc961dcd52a, then I agree, and am somewhat surprised at the regression: the expected span should point at annotate only, not the whole expression, but thought that could be dealt with later.

[estebank]

This case has a MiscObligation, so for some where clauses we need to do better, but at least we point at the relevant argument. The new span pointing at the call at least hints at the user that they should look at the equals implementation to figure out why the bound was added.

[nagisa]

@bors r+

[bors]

📌 Commit 7a8de2eca8f9b2707ec9a6ac25fcfdfc845a1227 has been approved by nagisa

[bors]

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

[nagisa]

@bors rollup=never

[bors]

⌛ Testing commit 7a8de2eca8f9b2707ec9a6ac25fcfdfc845a1227 with merge 633c54588419b027ba35ce87ff6ba15061de8ad7...

[bors]

💔 Test failed - checks-actions

[estebank]

@bors r=nagisa

[bors]

📌 Commit 0a4540b has been approved by nagisa

[bors]

⌛ Testing commit 0a4540b with merge e366210...

[bors]

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing e366210 to master...

[rust-timer]

Finished benchmarking commit (e366210): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

[pnkfelix]

Huh. I'm doing performance triage now, and the triage report points to this PR as the source of a 1.9% regression on diesel.

There has been some associated discussion of the matter on zulip : https://zulip-archive.rust-lang.org/stream/247081-t-compiler/performance/topic/triage.202021.2021.2009.html#254207973

Anyway, it seems like this might be a legitimate regression to investigate, at least for the diesel-doc case.

@rustbot label: +perf-regression