Improve cause information for NLL higher-ranked errors by Aaron1011 · Pull Request #89249 · rust-lang/rust

Aaron1011 · 2021-09-25T16:40:58Z

This PR has several interconnected pieces:

In some of the NLL region error code, we now pass
around an ObligationCause, instead of just a plain Span.
This gets forwarded into fulfill_cx.register_predicate_obligation
during error reporting.
The general InferCtxt error reporting code is extended to
handle ObligationCauseCode::BindingObligation
A new enum variant ConstraintCategory::Predicate is added.
We try to avoid using this as the 'best blame constraint' - instead,
we use it to enhance the ObligationCause of the BlameConstraint
that we do end up choosing.

As a result, several NLL error messages now contain the same
"the lifetime requirement is introduced here" message as non-NLL
errors.

Having an ObligationCause available will likely prove useful
for future improvements to NLL error messages.

rust-highfive · 2021-09-25T16:41:01Z

r? @estebank

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

Aaron1011 · 2021-09-25T16:41:23Z

cc @estebank @lqd @matthewjasper @rust-lang/wg-compiler-nll

estebank · 2021-09-26T16:59:43Z

@bors r+

bors · 2021-09-26T16:59:45Z

📌 Commit ae4916bd3d0433eb2375068af42e0b4295279359 has been approved by estebank

Aaron1011 · 2021-09-26T20:07:49Z

This should probably have a perf run beforehand, now that I think about it.

@bors r-

Aaron1011 · 2021-09-26T20:08:03Z

@bors try @rust-timer queue

rust-timer · 2021-09-26T20:08:04Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-09-26T20:08:11Z

⌛ Trying commit ae4916bd3d0433eb2375068af42e0b4295279359 with merge e155d2a2ad47d98b5b62f8b9fba9a9e7b4c6c2de...

bors · 2021-09-26T21:28:00Z

☀️ Try build successful - checks-actions
Build commit: e155d2a2ad47d98b5b62f8b9fba9a9e7b4c6c2de (e155d2a2ad47d98b5b62f8b9fba9a9e7b4c6c2de)

rust-timer · 2021-09-26T21:28:02Z

Queued e155d2a2ad47d98b5b62f8b9fba9a9e7b4c6c2de with parent c09d637, future comparison URL.

rust-timer · 2021-09-27T02:42:43Z

Finished benchmarking commit (e155d2a2ad47d98b5b62f8b9fba9a9e7b4c6c2de): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

Very large regression in instruction counts (up to 4.8% on full builds of wg-grammar)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

This PR has several interconnected pieces: 1. In some of the NLL region error code, we now pass around an `ObligationCause`, instead of just a plain `Span`. This gets forwarded into `fulfill_cx.register_predicate_obligation` during error reporting. 2. The general InferCtxt error reporting code is extended to handle `ObligationCauseCode::BindingObligation` 3. A new enum variant `ConstraintCategory::Predicate` is added. We try to avoid using this as the 'best blame constraint' - instead, we use it to enhance the `ObligationCause` of the `BlameConstraint` that we do end up choosing. As a result, several NLL error messages now contain the same "the lifetime requirement is introduced here" message as non-NLL errors. Having an `ObligationCause` available will likely prove useful for future improvements to NLL error messages.

This shirnks the size of `ConstraintCategory`, hopefully fixing a performance regression.

Aaron1011 · 2021-09-27T15:47:28Z

@bors try @rust-timer queue

rust-timer · 2021-09-27T15:47:30Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-09-27T15:47:36Z

⌛ Trying commit 41ad383 with merge b13e7a00e877f93c9b85b09fddfef60e03d9d07b...

bors · 2021-09-27T17:09:40Z

☀️ Try build successful - checks-actions
Build commit: b13e7a00e877f93c9b85b09fddfef60e03d9d07b (b13e7a00e877f93c9b85b09fddfef60e03d9d07b)

rust-timer · 2021-09-27T17:09:41Z

Queued b13e7a00e877f93c9b85b09fddfef60e03d9d07b with parent 3e8f32e, future comparison URL.

rust-timer · 2021-09-27T18:45:58Z

Finished benchmarking commit (b13e7a00e877f93c9b85b09fddfef60e03d9d07b): comparison url.

Summary: This change led to small relevant regressions 😿 in compiler performance.

Small regression in instruction counts (up to 0.5% on incr-unchanged builds of helloworld)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Aaron1011 · 2021-09-27T19:11:23Z

@estebank The regressions are now much smaller. Can you review the latest commit?

estebank · 2021-09-27T20:40:50Z

+                    // We currentl'y doesn't store the `DefId` in the `ConstraintCategory`
+                    // for perforamnce reasons. The error reporting code used by NLL only
+                    // uses the span, so this doesn't cause any problems at the moment.
+                    Some(ObligationCauseCode::BindingObligation(
+                        CRATE_DEF_ID.to_def_id(),
+                        predicate_span,
+                    ))


We should create a new ObligationCauseCode variant instead, but it's fine to do this I guess.

I was trying to mimic the non-NLL code, which also uses ObligationCauseCode::BindingObligation for this message:

rust/compiler/rustc_infer/src/infer/error_reporting/note.rs

Lines 396 to 405 in 2b6ed3b

if let ObligationCauseCode::BindingObligation(_, span) =

&trace.cause.code.peel_derives()

{

let span = *span;

let mut err = self.report_concrete_failure(placeholder_origin, sub, sup);

err.span_note(span, "the lifetime requirement is introduced here");

err

} else {

unreachable!()

}

estebank · 2021-09-27T20:41:03Z

+// Make sure this enum doesn't unintentionally grow
+rustc_data_structures::static_assert_size!(ConstraintCategory, 12);
+


estebank · 2021-09-27T20:42:06Z

@bors r+

The performance impact seems small, but I'm sure the perf triage team can make a better determination about it. It seems to me that the majority of the impact is due to tracking extra metadata. It's a shame it doesn't impact "only" the NLL errors.

bors · 2021-09-27T20:42:07Z

📌 Commit 41ad383 has been approved by estebank

bors · 2021-09-27T21:29:23Z

⌛ Testing commit 41ad383 with merge 8a12be7...

bors · 2021-09-28T00:21:05Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 8a12be7 to master...

rust-timer · 2021-09-28T02:43:08Z

Finished benchmarking commit (8a12be7): 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

…ath, r=JohnTitor borrowck: Introduce BlameConstraint::to_obligation_cause_from_path() Before this commit, `BlameConstraint` is a bit hard to understand at first, because of its `ObligationCause` field. In practice, `BlameConstraint` is just a slimmed down version of an `OutlivesConstraint` from `Vec<OutlivesConstraint>`. The artificial `ObligationCause` is only needed in one place. Rather than having that exception polute the lower level logic, bubble it up as much as we can. That way, it becomes clearer what `BlameConstraint` actually is. For reference, `ObligationCause` was introdued by rust-lang#89249, but the codebase looked quite different back then. This takes us one step closer (this is commit 1 of 4) towards the end result illustrated in rust-lang#158623. # Notes I have removed the comment ```rs // We try to avoid reporting a `ConstraintCategory::Predicate` as our best constraint. // Instead, we use it to produce an improved `ObligationCauseCode`. ```` because I think [this](https://github.com/rust-lang/rust/blob/e2af9c03ef4cfb769e391968a5cfbb1afe6f01e2/compiler/rustc_borrowck/src/region_infer/mod.rs#L1763-L1762) existing comment is sufficient: ```rs // We handle predicates and opaque types specially; don't prioritize them here. ConstraintCategory::Predicate(_) | ConstraintCategory::OpaqueType => 4, ```

Rollup merge of #158624 - Enselic:to_obligation_cause_from_path, r=JohnTitor borrowck: Introduce BlameConstraint::to_obligation_cause_from_path() Before this commit, `BlameConstraint` is a bit hard to understand at first, because of its `ObligationCause` field. In practice, `BlameConstraint` is just a slimmed down version of an `OutlivesConstraint` from `Vec<OutlivesConstraint>`. The artificial `ObligationCause` is only needed in one place. Rather than having that exception polute the lower level logic, bubble it up as much as we can. That way, it becomes clearer what `BlameConstraint` actually is. For reference, `ObligationCause` was introdued by #89249, but the codebase looked quite different back then. This takes us one step closer (this is commit 1 of 4) towards the end result illustrated in #158623. # Notes I have removed the comment ```rs // We try to avoid reporting a `ConstraintCategory::Predicate` as our best constraint. // Instead, we use it to produce an improved `ObligationCauseCode`. ```` because I think [this](https://github.com/rust-lang/rust/blob/e2af9c03ef4cfb769e391968a5cfbb1afe6f01e2/compiler/rustc_borrowck/src/region_infer/mod.rs#L1763-L1762) existing comment is sufficient: ```rs // We handle predicates and opaque types specially; don't prioritize them here. ConstraintCategory::Predicate(_) | ConstraintCategory::OpaqueType => 4, ```

rust-highfive assigned estebank Sep 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2021

Aaron1011 mentioned this pull request Sep 25, 2021

[NLL] Bad higher ranked subtype error #57374

Open

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 Sep 26, 2021

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 26, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 26, 2021

rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 27, 2021

Aaron1011 added 2 commits September 27, 2021 10:23

Remove DefId from ConstraintCategory::Predicate

41ad383

This shirnks the size of `ConstraintCategory`, hopefully fixing a performance regression.

Aaron1011 force-pushed the higher-ranked-cause branch from ae4916b to 41ad383 Compare September 27, 2021 15:47

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 27, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 27, 2021

estebank approved these changes Sep 27, 2021

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 28, 2021

bors merged commit 8a12be7 into rust-lang:master Sep 28, 2021

rustbot added this to the 1.57.0 milestone Sep 28, 2021

rustbot removed the perf-regression Performance regression. label Sep 28, 2021

Enselic mentioned this pull request Jun 30, 2026

borrowck: Introduce BlameConstraint::to_obligation_cause_from_path() #158624

Merged

	if let ObligationCauseCode::BindingObligation(_, span) =
	&trace.cause.code.peel_derives()
	{
	let span = *span;
	let mut err = self.report_concrete_failure(placeholder_origin, sub, sup);
	err.span_note(span, "the lifetime requirement is introduced here");
	err
	} else {
	unreachable!()
	}

		// Make sure this enum doesn't unintentionally grow
		rustc_data_structures::static_assert_size!(ConstraintCategory, 12);

Uh oh!

Uh oh!

Conversation

Aaron1011 commented Sep 25, 2021

Uh oh!

rust-highfive commented Sep 25, 2021

Uh oh!

Aaron1011 commented Sep 25, 2021

Uh oh!

estebank commented Sep 26, 2021

Uh oh!

bors commented Sep 26, 2021

Uh oh!

Aaron1011 commented Sep 26, 2021

Uh oh!

Aaron1011 commented Sep 26, 2021

Uh oh!

rust-timer commented Sep 26, 2021

Uh oh!

bors commented Sep 26, 2021

Uh oh!

bors commented Sep 26, 2021

Uh oh!

rust-timer commented Sep 26, 2021

Uh oh!

rust-timer commented Sep 27, 2021

Uh oh!

Aaron1011 commented Sep 27, 2021

Uh oh!

rust-timer commented Sep 27, 2021

Uh oh!

bors commented Sep 27, 2021

Uh oh!

bors commented Sep 27, 2021

Uh oh!

rust-timer commented Sep 27, 2021

Uh oh!

rust-timer commented Sep 27, 2021

Uh oh!

Aaron1011 commented Sep 27, 2021

Uh oh!

estebank Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

Aaron1011 Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

estebank Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

estebank commented Sep 27, 2021

Uh oh!

bors commented Sep 27, 2021

Uh oh!

bors commented Sep 27, 2021

Uh oh!

bors commented Sep 28, 2021

Uh oh!

rust-timer commented Sep 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants