-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Move placeholder error handling to before region inference #142623
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?
Move placeholder error handling to before region inference #142623
Conversation
This comment has been minimized.
This comment has been minimized.
Wow, a merge conflict with upstream for a commit I JUST PUSHED, that's record time! |
fd18095
to
199f961
Compare
@rustbot blocked |
199f961
to
3a46042
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3a46042
to
9b555d3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9b555d3
to
f31393f
Compare
@rustbot ready |
= note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`... | ||
= note: `Getter<'_>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'1>>`, for any two lifetimes `'0` and `'1`... |
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.
I hate this, but can't find a way around it. My gut feeling says it's another case of an order-dependent extraction procedure somewhere. For what it's worth, it wasn't in the earlier version of this code but it seems to have appeared after the recent changes to the base commit. Presumably the difference is from selecting smallest placeholder by rvid, etc. Help very much appreciated!
f31393f
to
f542ec5
Compare
error_element | ||
{ | ||
let adjusted_universe = | ||
error_placeholder.universe.as_u32().checked_sub(base_universe.as_u32()); |
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.
I think the "checked sub" part is what really broke me here. Why are universes being adjusted? What's a base universe? And why can the subtraction sometimes overflow?!?!
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.
See also bound_region_errors
, line 502
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.
For the record, these are the only reads of base_universe
on CanonicalTypeOpDeeplyNormalizeGoal
, CanonicalTypeOpAscribeUserTypeGoal
and InstantiateOpaqueType
.
This comment has been minimized.
This comment has been minimized.
f542ec5
to
8269a42
Compare
This comment has been minimized.
This comment has been minimized.
I don't understand where these errors come from. On my branch every single commit passes the |
8269a42
to
e0b30fa
Compare
fix 2 borrowck issues fixes rust-lang#146467 cc `@amandasystems` our understanding here is as follows: region constraints from computing implied bounds gets `ConstraintCategory::Internal`. If there's a higher-ranked subtyping errors while computing implied bounds we then ended up with only `ConstraintCategory::Internal` and `ConstraintCategory::OutlivesUnnameablePlaceholder(_)` constraints. The path was something like - `'placeholderU2: 'placeholderU1` (`Internal`) - `'placeholderU1: 'static` (`OutlivesUnnameablePlaceholder('placeholderU2)`) It's generally somewhat subtle here as ideally relating placeholders doesn't introduce `'static` constraints. Relating the placeholders themselves will always error regardless, cc rust-lang#142623. --- separately fixes rust-lang#145925 (comment) by updating the location for deferred closure requirements inside of promoteds. I am not updating their category as doing so is 1) effort and 2) imo actually undesirable 🤔 see the comments in `TypeChecker::check_promoted` cc `@lqd` r? lqd
fix 2 borrowck issues fixes rust-lang#146467 cc ``@amandasystems`` our understanding here is as follows: region constraints from computing implied bounds gets `ConstraintCategory::Internal`. If there's a higher-ranked subtyping errors while computing implied bounds we then ended up with only `ConstraintCategory::Internal` and `ConstraintCategory::OutlivesUnnameablePlaceholder(_)` constraints. The path was something like - `'placeholderU2: 'placeholderU1` (`Internal`) - `'placeholderU1: 'static` (`OutlivesUnnameablePlaceholder('placeholderU2)`) It's generally somewhat subtle here as ideally relating placeholders doesn't introduce `'static` constraints. Relating the placeholders themselves will always error regardless, cc rust-lang#142623. --- separately fixes rust-lang#145925 (comment) by updating the location for deferred closure requirements inside of promoteds. I am not updating their category as doing so is 1) effort and 2) imo actually undesirable 🤔 see the comments in `TypeChecker::check_promoted` cc ``@lqd`` r? lqd
Rollup merge of #146711 - lcnr:fix-placeholder-ice, r=lqd fix 2 borrowck issues fixes #146467 cc ``@amandasystems`` our understanding here is as follows: region constraints from computing implied bounds gets `ConstraintCategory::Internal`. If there's a higher-ranked subtyping errors while computing implied bounds we then ended up with only `ConstraintCategory::Internal` and `ConstraintCategory::OutlivesUnnameablePlaceholder(_)` constraints. The path was something like - `'placeholderU2: 'placeholderU1` (`Internal`) - `'placeholderU1: 'static` (`OutlivesUnnameablePlaceholder('placeholderU2)`) It's generally somewhat subtle here as ideally relating placeholders doesn't introduce `'static` constraints. Relating the placeholders themselves will always error regardless, cc #142623. --- separately fixes #145925 (comment) by updating the location for deferred closure requirements inside of promoteds. I am not updating their category as doing so is 1) effort and 2) imo actually undesirable 🤔 see the comments in `TypeChecker::check_promoted` cc ``@lqd`` r? lqd
fix 2 borrowck issues fixes rust-lang/rust#146467 cc ``@amandasystems`` our understanding here is as follows: region constraints from computing implied bounds gets `ConstraintCategory::Internal`. If there's a higher-ranked subtyping errors while computing implied bounds we then ended up with only `ConstraintCategory::Internal` and `ConstraintCategory::OutlivesUnnameablePlaceholder(_)` constraints. The path was something like - `'placeholderU2: 'placeholderU1` (`Internal`) - `'placeholderU1: 'static` (`OutlivesUnnameablePlaceholder('placeholderU2)`) It's generally somewhat subtle here as ideally relating placeholders doesn't introduce `'static` constraints. Relating the placeholders themselves will always error regardless, cc rust-lang/rust#142623. --- separately fixes rust-lang/rust#145925 (comment) by updating the location for deferred closure requirements inside of promoteds. I am not updating their category as doing so is 1) effort and 2) imo actually undesirable 🤔 see the comments in `TypeChecker::check_promoted` cc ``@lqd`` r? lqd
23899d8
to
351a854
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
actually an existential.
875cbf5
to
264683d
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. |
@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.
Move placeholder error handling to before region inference
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (242f8ac): comparison URL. Overall result: ❌ regressions - 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 -4.1%, secondary -0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 474.753s -> 473.062s (-0.36%) |
origin_a: ty::PlaceholderRegion, | ||
origin_b: ty::PlaceholderRegion, |
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.
do we need these two origins? i feel like we should be able to just recompute them
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.
Yep, all we need is the SCC annotation and the defintions.
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.
Actually, the definitions are of course enough.
/// The smallest max nameable universe of all | ||
/// regions reachable from this SCC. | ||
min_max_nameable_universe: UniverseIndex, | ||
|
||
/// The existential region with the smallest universe, if any. | ||
min_universe_existential: Option<(UniverseIndex, RegionVid)>, |
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.
this split seems odd to me. In a sense we'd want a single "min (max nameable universe) < placeholder universe" and then check the source of that "min (max nameable universe)" to figure out whether its
placeholder: placeholder
existential: placeholder
location: placeholder
Do we even care about location: placeholder
? I
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.
Do we even care about location: placeholder? I
Some experiments I did suggested we do (otherwise we miss errors) but uh from what I've had explained to me we probably shouldn't? It's strange and I don't like it.
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.
But this points towards a potentially much nicer variant that's a state machine tracking precisely this.
diag: &mut Diag<'_>, | ||
lower_bound: RegionVid, | ||
) { | ||
) -> Option<()> { |
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.
Option<()>
?
264683d
to
05d58ab
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
This PR breaks out yet another part of #130227. It prepares for the removal of precise placeholder tracking by colocating detecting placeholder errors for higher-ranked relations (placeholders outliving existentials that can't name them, other placeholders) with the preprocessing step that adds outlives-static constraints. This also relieves that step from the burden of ensuring these errors are detected via propagation, and fully splits the logic into one part that rewrites the constraint graph to add edges to
'static
and one that detects hard errors.For stepping-on-toes reasons it is currently based on #140737, but
can be rebased on regular master and merged separatelynope that's a terrible idea actually, it relies on some of the tracking introduced there.This step also makes region errors more fine-grained, which is useful to simplify the error reporting pipeline, which is currently based on a very brittle method of reconstructing an error event after the fact to figure out what went wrong. Doing that is left as future work, though.
This changes the logic of error reporting slightly to not emit errors for placeholder/higher kinded constraints if there are other borrowck errors. This somewhat lessens the flood of errors and was described as if anything an improvement by @nikomatsakis in informal review.
r? lcnr
Fixes: #146467