-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Region inference: Use outlives-static constraints in constraint search #140737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Region inference: Use outlives-static constraints in constraint search #140737
Conversation
f4af776
to
72e81ea
Compare
72e81ea
to
9a1face
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7902ae9
to
6539053
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #140466) made this pull request unmergeable. Please resolve the merge conflicts. |
6539053
to
a209255
Compare
(We may also want a perf run to see if I messed something up badly) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Region inference: Use outlives-static constraints in constraint search 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. It is based on #140466, so that has to land first. We probably want a perf run of this. It shouldn't have much of an impact and a positive one if any, but I have been wrong before. In particular, SCC annotations are heavier now. r? lcnr
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (0d3d480): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 753.257s -> 756.017s (0.37%) |
a209255
to
335fa61
Compare
335fa61
to
6a325fd
Compare
This comment has been minimized.
This comment has been minimized.
…=BoxyUwU rework GAT borrowck limitation error The old one depends on the `ConstraintCategory` of the constraint which meant we did not emit this note if we had to prove the higher ranked trait bound due to e.g. normalization. This made it annoying brittle and caused MIR borrowck errors to be order dependent, fixes the issue in rust-lang#140737 (comment). r? types cc `@amandasystems`
…=BoxyUwU rework GAT borrowck limitation error The old one depends on the `ConstraintCategory` of the constraint which meant we did not emit this note if we had to prove the higher ranked trait bound due to e.g. normalization. This made it annoying brittle and caused MIR borrowck errors to be order dependent, fixes the issue in rust-lang#140737 (comment). r? types cc ``@amandasystems``
…=BoxyUwU rework GAT borrowck limitation error The old one depends on the `ConstraintCategory` of the constraint which meant we did not emit this note if we had to prove the higher ranked trait bound due to e.g. normalization. This made it annoying brittle and caused MIR borrowck errors to be order dependent, fixes the issue in rust-lang#140737 (comment). r? types cc ```@amandasystems```
…=BoxyUwU rework GAT borrowck limitation error The old one depends on the `ConstraintCategory` of the constraint which meant we did not emit this note if we had to prove the higher ranked trait bound due to e.g. normalization. This made it annoying brittle and caused MIR borrowck errors to be order dependent, fixes the issue in rust-lang#140737 (comment). r? types cc `@amandasystems`
…=BoxyUwU rework GAT borrowck limitation error The old one depends on the `ConstraintCategory` of the constraint which meant we did not emit this note if we had to prove the higher ranked trait bound due to e.g. normalization. This made it annoying brittle and caused MIR borrowck errors to be order dependent, fixes the issue in rust-lang#140737 (comment). r? types cc ``@amandasystems``
…=BoxyUwU rework GAT borrowck limitation error The old one depends on the `ConstraintCategory` of the constraint which meant we did not emit this note if we had to prove the higher ranked trait bound due to e.g. normalization. This made it annoying brittle and caused MIR borrowck errors to be order dependent, fixes the issue in rust-lang#140737 (comment). r? types cc ```@amandasystems```
Rollup merge of #145041 - lcnr:borrowck-limitations-error, r=BoxyUwU rework GAT borrowck limitation error The old one depends on the `ConstraintCategory` of the constraint which meant we did not emit this note if we had to prove the higher ranked trait bound due to e.g. normalization. This made it annoying brittle and caused MIR borrowck errors to be order dependent, fixes the issue in #140737 (comment). r? types cc ```@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.
5a4a082
to
c11f113
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@rustbot ready |
We may also want a new perf run since I have changed the code that runs during SCC construction which might affect performance (positively, I hope) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Region inference: Use outlives-static constraints in constraint search
} | ||
}); | ||
|
||
// Edge case: it's possible that `'from_region` is an unnameable placeholder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what exactly is this edge case and when does it occur, also
// This the
false
argument is what prevents circular reasoning here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry; was babying too hard to think with computer about this.
I meant that there is a risk of this being infinitely recursive if this passes through the same outlives static constraint again. Sometimes that's even a shortcut, since static leads everywhere, in which case we're at risk of blaming boring constraints or recursing forever. It's also nicer from a purely logical standpoint to do the search in the original graph before rewriting it (ie without the new edges) since it's closer to the original set of constraints.
As for the other case, this would happen when we have an SCC representative r for SCC S whose computed universe was lowered. I think this always happens because it shares SCC with an existential e with a lower universe. In that case r would be the unnameable region, which in some cases triggers a search for a path from r to r, which ICEs, so I blame e instead.
This should become unnecessary once the placeholder outlives existential check is enabled in the next PR, but for now this is the only way to detect that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this could also happen if some other region that’s a less desirable representative ended up in the same SCC, but I don’t know if that ever happens for regions which aren’t either placeholders or existentialis both of which are errors.
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (834fbf1): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.7%, secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.8%, secondary 43.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 471.447s -> 469.972s (-0.31%) |
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