Improve cause information for NLL higher-ranked errors #89249

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

compiler/rustc_borrowck/src/region_infer/mod.rs

+                    // 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

compiler/rustc_middle/src/mir/query.rs

+// 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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve cause information for NLL higher-ranked errors #89249

Improve cause information for NLL higher-ranked errors #89249

Aaron1011 commented Sep 25, 2021

rust-highfive commented Sep 25, 2021

Aaron1011 commented Sep 25, 2021

estebank commented Sep 26, 2021

bors commented Sep 26, 2021

Aaron1011 commented Sep 26, 2021

Aaron1011 commented Sep 26, 2021

rust-timer commented Sep 26, 2021

bors commented Sep 26, 2021

bors commented Sep 26, 2021

rust-timer commented Sep 26, 2021

rust-timer commented Sep 27, 2021

Aaron1011 commented Sep 27, 2021

rust-timer commented Sep 27, 2021

bors commented Sep 27, 2021

bors commented Sep 27, 2021

rust-timer commented Sep 27, 2021

rust-timer commented Sep 27, 2021

Aaron1011 commented Sep 27, 2021

estebank Sep 27, 2021

Aaron1011 Sep 27, 2021

estebank Sep 27, 2021

estebank commented Sep 27, 2021

bors commented Sep 27, 2021

bors commented Sep 27, 2021

bors commented Sep 28, 2021

rust-timer commented Sep 28, 2021

	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);

Improve cause information for NLL higher-ranked errors #89249

Improve cause information for NLL higher-ranked errors #89249

Conversation

Aaron1011 commented Sep 25, 2021

rust-highfive commented Sep 25, 2021

Aaron1011 commented Sep 25, 2021

estebank commented Sep 26, 2021

bors commented Sep 26, 2021

Aaron1011 commented Sep 26, 2021

Aaron1011 commented Sep 26, 2021

rust-timer commented Sep 26, 2021

bors commented Sep 26, 2021

bors commented Sep 26, 2021

rust-timer commented Sep 26, 2021

rust-timer commented Sep 27, 2021

Aaron1011 commented Sep 27, 2021

rust-timer commented Sep 27, 2021

bors commented Sep 27, 2021

bors commented Sep 27, 2021

rust-timer commented Sep 27, 2021

rust-timer commented Sep 27, 2021

Aaron1011 commented Sep 27, 2021

estebank Sep 27, 2021

Choose a reason for hiding this comment

Aaron1011 Sep 27, 2021

Choose a reason for hiding this comment

estebank Sep 27, 2021

Choose a reason for hiding this comment

estebank commented Sep 27, 2021

bors commented Sep 27, 2021

bors commented Sep 27, 2021

bors commented Sep 28, 2021

rust-timer commented Sep 28, 2021