Region inference: split results from RegionInferenceContext#151688
Region inference: split results from RegionInferenceContext#151688amandasystems wants to merge 10 commits intorust-lang:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
6befd69 to
13a57f6
Compare
This comment has been minimized.
This comment has been minimized.
13a57f6 to
00e8121
Compare
00e8121 to
454f98a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This ensures all of region inference is immutable, and makes every operation that requires region inference to have been executed to run explicitly require the results.
Since this collapses `RegionInferenceContext`, `::solve()` now consumes the inference context.
on `InferredRegions`.
4c01f95 to
174023c
Compare
|
This PR was rebased onto a different main 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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| liveness: &LivenessValues, | ||
| pass_where: PassWhere, | ||
| out: &mut dyn io::Write, | ||
| ) -> io::Result<()> { |
There was a problem hiding this comment.
pls use a consistent function param ordering between the functions here :<
| let (definitions, _has_placeholders) = region_definitions( | ||
| infcx, | ||
| universal_regions, | ||
| &mut constraints.liveness_constraints.clone(), |
There was a problem hiding this comment.
this is... very odd. You give a mutable reference to a clone?
please move the cloned thing into a let binding first
There was a problem hiding this comment.
That's a quick hack workaround, I think we should talk about how to not have to do that, where to put region_definitions(), and whether the liveness constarints stuff is necessary in sync because it's pretty much the same thing.
| outlives_constraints: &'a OutlivesConstraintSet<'tcx>, | ||
| mut w: &mut dyn Write, | ||
| ) -> io::Result<()> { | ||
| dot::render(&RawConstraints { tcx, region_definitions, outlives_constraints }, &mut w) |
There was a problem hiding this comment.
why reference RawConstraints. That type should be Copy
There was a problem hiding this comment.
It's because dot::render() takes its arguments by reference (and it wasn't written by me).
| dot::render(&SccConstraints { tcx, region_definitions, constraint_sccs, nodes_per_scc }, &mut w) | ||
| } | ||
|
|
||
| struct RawConstraints<'a, 'tcx> { |
There was a problem hiding this comment.
make this copy and take it by value
| nodes_per_scc: IndexVec<ConstraintSccIndex, Vec<RegionVid>>, | ||
| region_definitions: &'a IndexVec<RegionVid, RegionDefinition<'tcx>>, | ||
| constraint_sccs: &'a ConstraintSccs, | ||
| } |
| outlives_requirements.as_mut(), | ||
| &mut errors_buffer, | ||
| type_tests, | ||
| ); |
There was a problem hiding this comment.
hmm, I would prefer check_type_tests to stay on the region inference context 🤔
they are a part of region inference to me
There was a problem hiding this comment.
or like, the RegionInferenceCtxt should either not exist at all anymore, or still be used for the region computations 🤔 maybe chat about it in sync
| if let NllRegionVariableOrigin::FreeRegion = origin { | ||
| // Add all nodes in the CFG to liveness constraints for free regions | ||
| liveness_constraints.add_all_points(rvid); | ||
| } |
There was a problem hiding this comment.
this is a functional change? 🤔
There was a problem hiding this comment.
It should not be, this always happens during region inference, I just moved it slightly earlier.
compiler/rustc_borrowck/src/nll.rs
Outdated
| // These clones can more or less be avoided, | ||
| // since they are probably idempotent. |
There was a problem hiding this comment.
what exactly does this comment mean? It's confusing to me
There was a problem hiding this comment.
It's a grammar error; what I meant was that the clones make the operations local since it operates on cloned values as opposed to shared ones, but the operations being performed are probably idempotent so doing them twice (in case of shared values) is likely fine. In other words, just dropping the clones will probably do the right thing.
|
Can you rebase, I will then do a perf run and locally look at the docs after this PR as that should make it clearer how the API has changed |
What?
This PR turns
RegionInferenceContextinto a builder that produces the inferred region values,InferredRegions. The resulting struct implements the public API ofRegionInferenceContextand replaces it inconsumers.rs.RegionInferenceContext::solve()now consumes (moves) the inference context. It is completely private to region inference.Why?
RegionInferenceContexthas become a huge dump for various values people want to access.region_inferitself is a very large file that's difficult to find your way around.RegionInferenceContextnow takes almost all of its fields by referenceKnock-on effects
RegionInferenceContextRegionInferenceContextnow almost exclusively contains references to values, as opposed to owning them. This addresses most offn compute_closure_requirements_modulo_opaquesshouldn't clone all its inputs #146079region_infergains two child modules and becomes a lot less of a behemoth.Detailed overview of changes
region_infer::constraint_searchandregion_infer::universal_regions.handle_placeholdersnow consumes less input and does constraint rewriting via mutable referencesconsumers.rsnow has aInferenceResultsinstead of aRegionInferenceContexteval_{equal, outlives}are moved toInferredRegionsConsumerOptionsis now calledInferredRegionscalculate_borrows_out_of_scope_at_locationof course now takesInferredRegionsLivenessValuesfor free regions is now initialised as live at all points along with the constraint rewriting during placeholder handling, as opposed to during construction ofRegionInferenceContextr? @lcnr
Closes #146079.