Region inference: Use outlives-static constraints in constraint search

[amandasystems]

Revise the extra r: 'static constraints added upon universe issues to add an explanation, and use that explanation during constraint blame search. This greatly simplifies the region inference logic, which now does not need to reverse-engineer the event that caused a region to outlive 'static.

This cosmetically changes the output of two UI tests. I blessed them i separate commits with separate motivations, but that can of course be squashed as desired. We probably want that.

The PR was extracted out of #130227 and consists of one-third of its functional payload.

r? lcnr

[bors]

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

[amandasystems]

(We may also want a perf run to see if I messed something up badly)

[lcnr]

@bors try @rust-timer queue

[bors]

⌛ Trying commit a209255 with merge 0d3d480...

[bors]

☀️ Try build successful - checks-actions
Build commit: 0d3d480 (0d3d48082efe47a20c953415945cc721105e5f85)

[rust-timer]

Finished benchmarking commit (0d3d480): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.3%]	12
Regressions ❌ (secondary)	0.2%	[0.1%, 0.3%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.5%]	6
All ❌✅ (primary)	0.2%	[0.1%, 0.3%]	12

Max RSS (memory usage)

Results (primary 0.4%, secondary -1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.4%	[1.3%, 1.5%]	2
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-1.7%	[-1.7%, -1.7%]	1
Improvements ✅ (secondary)	-1.9%	[-2.4%, -0.6%]	4
All ❌✅ (primary)	0.4%	[-1.7%, 1.5%]	3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 753.257s -> 756.017s (0.37%)
Artifact size: 372.34 MiB -> 372.33 MiB (-0.00%)

[lcnr]

why can we not use the region from max_nameable_universe here?

[amandasystems]

That sometimes slightly worsens the error messages (or at least causes diagnostics regressions) because previous logic picked the first smallest region. It's very annoying.

[lcnr]

can you dump the changes caused by this somewhere? I personally believe that some amount of diagnostics regression is acceptable if it cleans up the code/removes subtly different variations of the same concept.

[amandasystems]

Here's the an output from the UI tests attached when doing that!

$ ./x test ui --keep-stage 1 > simpler-choice-blame-ui-tests.log

simpler-choice-blame-ui-tests.log

There's a bunch of squiggly lines getting longer, which I think is mostly harmless:

• tests/ui/higher-ranked/higher-ranked-lifetime-error.rs: I'd say fully harmless, a slightly longer squiggly line
• tests/ui/higher-ranked/trait-bounds/due-to-where-clause.rs same as above
• tests/ui/mismatched_types/hr-projection-mismatch.rs#current same as above

Then there is a duplicate of a diagnostic due to a longer span which we may or may not care about, in tests/ui/associated-inherent-types/issue-111404-1.rs. This should be very fixable in the error reporting phase, but that part of the code is absolutely incomprehensible (though I've tried), and I've simply given up on it.

I remember this being much, much worse! Maybe this is acceptable?

[amandasystems]

If you think this is ok, say the word and I'll dump like half the file to get rid of this extra logic!

[lcnr]

heavy drag on cigarette I don’t know man. I don’t make the constraint graph, I just walk it.

would be good to know 😁 it implies we somehow end up relating something to a placeholder from a higher universe for a boring reason? As in, even without the hr region error due to the predicate we'd have failed to compile this snippet

---

I feel like mir borrowck generally has to handle the following for when encountering an error:

• there may be multiple paths causing the bad outlives (idk about terminology)
• each path has multiple steps

I generally feel like we want to report the most boring and shortest path and of that path, the most exciting step. Or well, ideally we'd be able to point to segments shared by all paths :3

A step is boring if it exists due to reasons the user can't do much about, e.g. for types to be wf

A step is exciting™️ if it corresponds to something which is actively happening in the source, some where-bound, some assignment. The user is likely to be change a specific part of their code to avoid emitting this constraint.

So in this example, if we get an outlives 'static constraint both because its related to a placeholder for "boring" reasons, and also due to a predicate, we want to treat it as if its due to the boring reason, as fixing the predicate issue likely won't handle the boring constraint.

So yeah, I think it's actually good that we use this boring constraint instead of the predicate constraint here :>

[amandasystems]

This is some edge case though, since it only differs in the final step.

But sure, in general constraint blame search needs a lot of overhaul. It has terrible tech debt (check out the huge closure that arbitrarily tweaks priorities to work around some edge case), and I feel like it would be a lot simpler to attach more complete motivations and catch them later during error reporting than the current approach, which ad hoc recreates the event.

I don't know what to do about this note though. I think what I'd want to do in the best of all worlds™ is:

• Constraint blame search also optionally tells in its result if search was redirected, and if so why. For this solution, we just say "because placeholder outlives unnameable blah blah". That's presumably one enum variant, or a boolean flag, or maybe an Option.
• We somehow also need to find the span that the note about outliving static due to limitations needs. I don't know how to find that. Or do we need to find that?

Or do you mean the note shouldn't be there at all and this is good actually™?

[lcnr]

I think given that the outlives 19 exist with a BoringNoLocation, the predicate is not to blame here. Now, I don't see why that boring region outlives placeholder exists, but imo as long as that outlives exists, we shouldn't add a note stating that it's due to a predicate, because there's a separate "non predicate" reason

I would love for u to look into why that other constraint exists.

Constraint blame search also optionally tells in its result if search was redirected

hmm, that is another question 🤔 if we have 'n: 'placeholder as a cause of a 'm: 'static path, why do we compute the 'n: 'placeholder path instead of the full 'm: 'placeholder path (going through n).

Sorry for giving u a bunch of vague "pls look at X" reviews here btw. I think it's really good that we/you are cleaning this up but I want to make sure we're slowly moving towards something desirable while doing so

[amandasystems]

why do we compute the 'n: 'placeholder path instead of the full 'm: 'placeholder path (going through n).

Technically, the path could be from a placeholder, to an existential I think? In that case 'n would presumably be a placeholder (always the representative of an SCC), and the to-part of the redirect constraint be whatever region lowered the SCC's universe. In either case, 'm would reach that SCC so restarting the search is probably not doing much, except if there is an even shorter path to the target of the redirect constraint.

The meaning of the added annotation to the outlives-static constraint is meant to be "this edge to static was added because 'a: 'b is a path in the constraint graph".

[lcnr]

from placeholder to an existential should error, shouldn't it?

we only add outlives-static whenever we've got "scc_which_cannot_name reaches(transitively outlives) placeholder" constraints 🤔

My question above was, why does

rust/compiler/rustc_borrowck/src/region_infer/mod.rs

Lines 1796 to 1810 in b03635d

	// If we are passing through a constraint added because `'lt: 'unnameable`,
	// where cannot name `'unnameable`, redirect search towards `'unnameable`.
	let due_to_placeholder_outlives = path.iter().find_map(|c| {
	if let ConstraintCategory::OutlivesUnnameablePlaceholder(lt, unnameable) = c.category {
	Some((lt, unnameable))
	} else {
	None
	}
	});
	let path = if let Some((lt, unnameable)) = due_to_placeholder_outlives {
	// This the `false` argument is what prevents circular reasoning here!
	self.constraint_path_to(lt, |r| r == unnameable, false).unwrap().0
	} else {
	path
	};

check constraint_path_to(lt, ..) instead of constraint_path_to(from_region, ..)

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is 28 days old.

  It's recommended to update your branch according to the rustc-dev-guide.

[bors]

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