best_blame_constraint: Blame better constraints when the region graph has cycles from invariance or 'static

[dianne]

This fixes #132749 by changing which constraint is blamed for region errors in several cases. best_blame_constraint had a heuristic that tried to pinpoint the constraint causing an error by filtering out any constraints where the outliving region is unified with the ultimate target region being outlived. However, it used the SCCs of the region graph to do this, which is unreliable; in particular, if the target region is 'static, or if there are cycles from the presence of invariant types, it was skipping over the constraints it should be blaming. As is the case in that issue, this could lead to confusing diagnostics. The simplest fix seems to work decently, judging by test stderr: this makes best_blame_constraint no longer filter constraints by their outliving region's SCC.

There are admittedly some quirks in the test output. In many cases, subdiagnostics that depend on the particular constraint being blamed have either started or stopped being emitted. After starting at this for quite a while, I think anything too fickle about whether it outputs based on the particular constraint being blamed should instead be looking at the constraint path as a whole, similar to what's done for the placeholder-from-predicate note.

Very many tests involving invariant types gained a note pointing out the types' invariance, but in a few cases it was lost. A particularly illustrative example is tests/ui/lifetimes/copy_modulo_regions.stderr; I'd argue the new constraint is a better one to blame, but it lacks the variance diagnostic information that's elsewhere in the constraint path. If desired, I can try making that note check the whole path rather than just the blamed constraint.

The subdiagnostic BorrowExplanation::add_object_lifetime_default_note depends on a Cast being blamed, so a special case was necessary to keep it from disappearing from tests specifically testing for it. However, see the FIXME comment in that commit; I think the special case should be removed once that subdiagnostic works properly, but it's nontrivial enough to warrant a separate PR. Incidentally, this removes the note from a test where it was being added erroneously: in tests/ui/borrowck/two-phase-surprise-no-conflict.stderr, the object lifetime is explicitly provided and it's not 'static.

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[nnethercote]

I am not a good person to review nitty gritty borrowck error message changes :) Let's try someone else who is probably more appropriate: r? @lcnr

this may have perf consequence

Let's check: @bors try @rust-timer queue

[bors]

⌛ Trying commit 189b2f8 with merge 889e047...

[bors]

☀️ Try build successful - checks-actions
Build commit: 889e047 (889e047c4bb3d82cd34c0c703f2bd13964c828ca)

[rust-timer]

Finished benchmarking commit (889e047): comparison URL.

Overall result: no relevant changes - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

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

Max RSS (memory usage)

Results (secondary 2.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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: 769.678s -> 767.43s (-0.29%)
Artifact size: 330.86 MiB -> 330.84 MiB (-0.01%)

[lcnr]

I think this mostly makes sense to me, it has been somewhat difficult to review for me however. It would be really nice if you could clean up the surrounding code for a bite, even though it's not trictly part of your change. This would make reviewing this code a lot more pleasant.

• afaict extra_info is only mutated by a single push in this function, so 1) why are we using a Vec instead of an Option here and 2) could we its computation into a separate function
• I can't immediately tell what changes due to removing the 'whatever: 'static edges and what changes due to the explicit check+eager return, can you give me some more info here, or maybe even split it into 2 commits?

[lcnr]

from what I can tell, this FIXME is stating that this branch is causing us to not emit a add_object_lifetime_default_note in cases where we should. Please link to an affected test

[dianne]

ah, the wording of the FIXME is unclear; I'll see if I can make it better. I've made sure to still emit that note where it was previously emitted. that branch is a hack specifically to keep the note present in tests/ui/traits/trait-object-lifetime-default-note.stderr, since otherwise the CallArgument constraint would be blamed and the note would disappear; it'll be more apparent once I get rid of the UseAsConst/UseAsStatic special-casing and split the Cast case into a separate commit. I'll refer to that test in the FIXME too.

the missing notes the FIXME is referring to are cases where the note would make sense (and imo should be emitted)¹, but it wasn't emitted even before this change. the note depends on a coercion being blamed, but it won't always be blamed for region errors from object lifetime defaults (nor will a coercion necessarily be present). handling that properly is (afaict) nontrivial; I'd be happy to do what I can there in a separate PR, but I might either need some help with wording or some direction on what heuristics would make sense, since doing it as perfectly as possible seems quite involved.

Footnotes

1. see GAT type Assoc<T: ?Sized> implicitly requires Self to be 'static #131008, which would probably need a fairly general solution. I remember finding in-tree tests that are missing the note too though (which would require a less-general but still nontrivial fix), so I can dig those up if it'd help ↩

[lcnr]

this feels odd to me, do you want to disable this if there's also one step in the path whose category is a ConstraintCategory::Return or would it be pattern to walk the path to detect whether any ConstraintCategory::UseAsConst exists?

[dianne]

looking at it again, that special case shouldn't be there at all; it's just papering over a more fundamental issue. from what I can tell, the reason tests/ui/consts/const-mut-refs/mut_ref_in_final.stderr needs the special-casing is that UnsafeCell<T> is invariant in T, hence the outliving region of the UseAsConst/UseAsStatic constraint is in the same SCC as the target, hence it gets skipped. for a quick workaround, this could check if there's a constraint on the path with an invariant variance_info before removing any edges from the region graph. it's sort of justifiable, since back-edges from invariance would also need to be removed in order to get the properties I wanted from pruning the 'static: R edges. that said, I think this should handle invariance properly if it can. leaving it sometimes-wrong feels pretty unsatisfying, especially if a proper fix might handle this whole issue cleanly. I'll have to look into it further.

[dianne]

• afaict extra_info is only mutated by a single push in this function, so 1) why are we using a Vec instead of an Option here and 2) could we its computation into a separate function

I think the intent is for there to possibly be more variants of ExtraConstraintInfo. I agree though; think it'd be cleaner to move it to a separate function and get rid of ExtraConstraintInfo entirely. I'll try that!

• I can't immediately tell what changes due to removing the 'whatever: 'static edges and what changes due to the explicit check+eager return, can you give me some more info here, or maybe even split it into 2 commits?

no problem! I'll split it up; that'll help me too. I'll also be removing the special-case for UseAsConst/UseAsStatic as per my reply to your review comment, so hopefully that'll remove some noise.

[dianne]

I've pushed some changes:

• The first two commits are cleanup of the surrounding code.
• The third is the primary diagnostic change, which I've reworked. I'll try and write up a revised description in the main PR text (edit: it's there now), but in short, I've removed all of the logic to filter constraints by their sup's SCC. The result is simpler and I think it produces better diagnostics (though a few bits could use some work; I give my opinion in the longer writeup).
• The fourth commit is the special case for tests/ui/traits/trait-object-lifetime-default-note.rs, including an updated FIXME comment.

[bors]

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

[lcnr]

r=me on the first two commits, some of the changes look like they make things worse 🤔 have to look into it in more detail

thanks for splitting this PR into these commits, I feel like it's a lot more pleasant to review ❤️

[lcnr]

this seems wrong to me 🤔

the assignment is very clearly not part of the function signature xx that's likely caused by the async fn desugaring first moving bufs into a local, and we then constrain the type of the local to be &'a mut &'a :/

I think we should somehow filter this, I feel like the current approach of figuring out what to blame is generally a bit 🤷 i would expect us to instead iterate over all constraints and take the "best one" (while preferring earlier ones), instead of simply taking the first interesting one

[dianne]

ah, yeah, that's definitely wrong. I had a note about that test but I completely forgot to address it; thank you for the catch! I've added a commit that makes it try not to blame constraints from desugarings. it includes a special case to allow blaming returns from ?, since otherwise tests/ui/async-await/issue-67765-async-diagnostic.stderr would be less clear about why the usage Err(b) is problematic (and ? including a return is a lot more clear than the async sugar including an assignment). I'm not a desugaring expert, though, so I'm not sure if any others are intuitive/meaningful enough to be blamed (possibly with special wording?).

with regard to iterating over all constraints and picking the "best one", I agree that it should look at all the constraints in some way, but I'll have to think about if there's a better heuristic for the single best than position on the path (with some filter). I think returns are generally good to point to, and CopyBound constraints should maybe at least get an honorable mention in a note (if not the full blame) since they're subtle, but beyond that I think it gets a bit fuzzy. e.g. ascriptions and casts are helpful to point to when they syntactically refer to a relevant type/region, but are much less helpful if they leave it up to inference. for another example, UseAsConst is sometimes the best we've got at the top level, but within function bodies it's usually less helpful. besides the category, it feels like it should be possible to use constraints' regions to pinpoint the most blameable constraint, but I haven't found a straightforward way to get blame out of the constraint graph or region graph, given the cycles. constraints' location/span are helpful, but mostly once we're in proper diagnostic code and can look at types and the HIR and MIR and such. the compromise I've imagined is to pick a good-enough-but-maybe-not-best constraint to blame, and anything important enough to surface in addition to that can be picked out from the constraint path by diagnostics (though of course if there's some other way to get the blamed constraint as good as possible too, that'd be ideal).

[bors]

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

[lcnr]

a final suggestion, apart from this r=me 🤔

borrowck diagnostics are :/

[lcnr]

why do we point at ref mut v when talking about "assignment" here?

[lcnr]

okay, we do we consider ref mut x patterns to be an assignment?

[lcnr]

I guess this is a preexisting issue? Though maybe we should never consider this to be an interesting node?

[dianne]

oh, I missed that! I believe those bindings are "assignment"s because once they're lowered to MIR locals, they're assigned to by MIR assignment statements; the constraint generated by that is what's getting blamed. afaict currently there's not enough information in the MIR to efficiently make that distinction when assigning constraint categories. but we do have a span, so it shouldn't be too hard to call assignments "binding"s in diagnostics when syntactically they feel more like a "binding" than an "assignment".

that said, those are the wrong constraints to blame for these tests; these are both regressions, though it's just by chance they were correct in the first place. I assume because the error depends on &mut being invariant in its parameter, we're looking at things kind of backwards¹ and we picked a constraint from the unhelpful end of the path. I think it should be possible to detect when constraints arising from contravariance are being followed and flip the path around if so.. it'll probably make for better diagnostics, but I'll admit I don't love relying so heavily on what direction we traverse the constraint graph in for this. I'm worried something more robust might require collecting more information in borrowck, but I'll look into it at least.

sorry for the huge number of very specific stderr changes, and thank you for taking the time to review this. I definitely agree that it's :/

Footnotes

1. this is present in this test when blaming the correct constraint too: the label "method was supposed to return data with lifetime 'b but it is returning data with lifetime 'a" is peculiar when the return type in the signature has lifetime 'a and the body has lifetime 'b. I think that message was worded assuming return types are always covariant. it should be fixed too (maybe with a few other tweaks as well.. ideally it'd suggest replacing 'a with 'b rather than adding 'a: 'b, like we do for 'static), but probably that should go in a different PR. ↩

[lcnr]

hmm, one approach would be to change this code here to consider more assignments to be boring:

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

Lines 1117 to 1139 in 3378a5e

	let category = match place.as_local() {
	Some(RETURN_PLACE) => {
	let defining_ty = &self.universal_regions.defining_ty;
	if defining_ty.is_const() {
	if tcx.is_static(defining_ty.def_id()) {
	ConstraintCategory::UseAsStatic
	} else {
	ConstraintCategory::UseAsConst
	}
	} else {
	ConstraintCategory::Return(ReturnConstraint::Normal)
	}
	}
	Some(l)
	if matches!(body.local_decls[l].local_info(), LocalInfo::AggregateTemp) =>
	{
	ConstraintCategory::Usage
	}
	Some(l) if !body.local_decls[l].is_user_variable() => {
	ConstraintCategory::Boring
	}
	_ => ConstraintCategory::Assignment,
	};

E.g. by checking for RefForGuard and looking at VarBindingForm::opt_match_place. It's a bit fiddly sadly

[lcnr]

One likely quite impactful suggestion: make it boring if opt_ty_info is None

[dianne]

that's where I was looking, yeah. though I was focusing too much on distinguishing between assignment syntaxes. opt_match_place does work for catching those by-ref bindings. and since we (probably?) don't need the span or place from that, checking the VarBindingForm's binding_mode should be equivalent (I think). I don't think they should all be boring though, so there'd at need to be some other check (if not a different check entirely). e.g., for field-projection-mutating-context.rs:

1	error[E0308]: mismatched types
-	   --> $DIR/field-projection-mutating-context.rs:9:13
+	   --> $DIR/field-projection-mutating-context.rs:9:25
3	    |
4	 LL |     let Foo(ref mut y): Foo<fn(&'static str)> = x;
-	    |             ^^^^^^^^^ one type is more general than the other
+	    |                         ^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
6	    |
7	    = note: expected fn pointer `for<'a> fn(&'a _)`
8	               found fn pointer `fn(&_)`

the assignment is what relates the two types, so it makes more sense to blame than just one of the type annotations. there's also fn-subtype.stderr specifically checking for an assignment being blamed, though that assignment is by-value. in both cases the span associated with the assignment's constraint is unfortunate, but it is the assignment being blamed.

RefForGuard locals being boring to assign to makes sense, but it doesn't look like we blame those assignments in any tests anyway. I'd have to think more to come up with an example, if it's possible.

and opt_ty_info isn't quite what the docs claim. it looks like it's only Some(_) for function inputs (specifically for the inputs themselves, not projections from destructuring patterns). any other binding gets an opt_ty_info of None even if there's an annotation on it. we can get user annotations on normal local variables from a local decl's user_ty field, though. there's a slight asymmetry in how UserTypeProjections accounts nicely for projections and opt_ty_info doesn't, but also I can't imagine ever wanting to point to constraints from an argument being destructured, so that's probably fine (if unsettlingly ad-hoc). just checking for those also misses assignments from paths with explicitly-provided generic arguments, but maybe that's fine too; I think getting all user types and sorting out what's inferred from what's provided would be a much too involved process. in most of the tests this affects, it doesn't matter too much whether the assignment or something else gets blamed; they each provide half of the reason for a region error, so unless we surface more constraints' spans, the user has to hunt down the other half themself either way.

[bors]

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

[dianne]

I've rebased and added an experiment in treating some assignments without sufficiently nearby type annotations as uninteresting. this reverts several tests' output to what they were prior to this PR.

[lcnr]

I think the impact of b041c50 is positive 👍 you need to bless mir-opt tests

wrt to #133858 (comment)

I feel like both "the first constraint" and "more interesting constraint kinds" are important metrics when considering what to blame. In a sense maybe a good compromise would be:

Have different groups of "constraint priorities" and then choose the earliest constraint with the highest priority

This pretty much matches the current behavior except that there only exist 2 priority classes: boring and interesting 🤔 i won't require u to impl it in this PR, I do think something like this would allow for a nice-ish (as nice as it can get) solution here

[dianne]

mir-opt tests should now be blessed.

and I have something like that implemented locally already actually! I was experimenting with that as a way of making the fallback smarter for when an interesting constraint wasn't found due to cycles (back when that was confusing it). I'll see about updating that and checking how it runs tomorrow.

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[dianne]

I've tried ordering constraints by how interesting they are. after some experimentation, it looks like most categories should have the same priority, but there are exceptions. using the ordering of constraints on the path can still be unhelpful due to contravariance/invariance, but prioritizing blaming returns helps quite a bit (at least in several ui tests). a proper variance-respecting heuristic looks like it'd require some pretty substantial changes, so hopefully it's good enough just working around that.

a few more changes:

• pointer comparisons now produce CallArgument constraints, rather than being Boring.
• type annotations from turbofished generic arguments are now TypeAnnotations rather than Boring. this is tested for functions, but I don't think there's a test for blaming the generic arguments of an ADT constructor so that might need one. ideally nll/issue-98589-closures-relate-named-regions.rs should blame the annotations, since they do get a TypeAnnotation category, but all the propagated outlives requirements from the closures have boring categories, due to how propagation is implemented.
• I've added a label to catch when CopyBound and SizedBound constraints are present but not blamed. these only show up in a few tests, but I think the information is helpful. there's no test for the label pointing out a SizedBound constraint, so it might need a test, but there are a couple for CopyBound, and the logic is the same.
• I've implemented an alternative way of filtering out uninteresting assignments. it now only catches assignments from argument patterns, since those specifically I think won't be interesting and shouldn't be called "assignments". it's still a bit hacky, but I like it being very clear and specific about what it's making boring.

as far as ordering constraint interest goes, here's some considerations that didn't fit in the comments:

• type annotations can't be more interesting than assignments, or else those tests for higher-rank subtyping errors will blame the annotation rather than the assignment.
• assigning from a function's returned value shouldn't be more interesting than "usages"; this is what misdirected blame in Confusing error message for lifetime mismatch #132749.
• yields shouldn't be more interesting than type annotations or we'd end up with unhelpful blames like this (from coroutine/check-resume-ty-lifetimes.rs):

7        LL |             let x: &'static str = yield ();
-           |                    ^^^^^^^^^^^^ type annotation requires that `'1` must outlive `'static`
+           |                                   ^^^^^^^^ yielding this value requires that `'1` must outlive `'static`

though the real problem may be that yields need specially worded messages like returns, at which point we could prioritize blaming them. "yielding this value" isn't quite the full story as to what's going on there.

several tests featuring invariance have gone back to blaming returns, and thus have gone back to running afoul of #108636. apart from the message currently being wrong, blaming the return does look better.

the mir-opt tests should be properly blessed now, also!

the Cargo.lock change is because ConstraintCategory no longer needs an Ord impl, and thus no longer needs derive-where.

[bors]

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