Non-lexical lifetimes region renumberer #43559
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| block: BasicBlock, | ||
| statement: &mut Statement<'tcx>, | ||
| location: Location) { | ||
| if let StatementKind::EndRegion(_) = statement.kind { |
There was a problem hiding this comment.
I wasn't sure if this should also be used from the erase_regions pass. Should the be changed, left as is or just removed?
There was a problem hiding this comment.
EndRegion is not going to be relevant with NLL, so I think this is OK
src/librustc_mir/transform/nll.rs
Outdated
| } | ||
| let mut renumbered_mir = mir.clone(); | ||
|
|
||
| // Note that we're using the passed-in mir for the visitor. This is |
There was a problem hiding this comment.
Heh, it's either ugly... or genius. I'm not sure which. In any case, if it's only being used for the "origin data" of region variables, we could probably sidestep the question for a bit. For example, instead of using the proper span, we can use dummy-span, and track (for each region variable) the location. Or we can make a RegionVariableOrigin that includes the Location instead.
I'm sort of inclined to do the former -- just sidestep the existing inference infrastructure, and just try to remember, for each region variable index, what the location was for later.
Is there a reason that's a bad idea?
There was a problem hiding this comment.
@arielb1 I agree it's ugly 😄 . That's why I added the comment so it could draw attention. The only part I thought worked well is that I avoid any additional copies of the Mir or redesign of its struct. Though the latter might have been (could be?) the better route at some point. I definitely think the existing version is hacky, though.
@nikomatsakis, when we plan to read these locations in future implementations, do you imagine that we'd need to have the Mir mutable as it is in this case? This is what caused me trouble, as I couldn't think of how to give the visitor structure the mutable Mir it needed while also referencing into other parts of the Mir immutably. If we only need the Mir immutable after the visitor pass (or at least when we try to lookup the Locations in the Mir), then I don't see any issue with what you proposed. Just a map from region variable index to Location/SourceInfo, right? (The SourceInfo is because not all Ty regions provide a Location.)
There was a problem hiding this comment.
@Nashenas88 I do not expect us to be mutating the MIR as we perform the region inference -- it's more just using the MIR as an input.
carols10cents
added
the
S-waiting-on-review
… reference to mir
src/librustc_mir/transform/nll.rs
Outdated
| struct NLLVisitor<'a, 'tcx: 'a> { | ||
| tcx: TyCtxt<'a, 'tcx, 'tcx>, | ||
| struct NLLVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { | ||
| pub lookup_map: HashMap<RegionVid, Lookup>, |
There was a problem hiding this comment.
Would it make sense to add a method on NLLVisitor that takes ownership of self and returns the HashMap? This is rather than making the field public.
fn to_lookup(self) -> HashMap<RegionVid, Lookup> { self.lookup_map }
There was a problem hiding this comment.
I would usually do something like that, yeah, but I'd call it into_lookup() or into_results().
|
@arielb1 @nikomatsakis I believe the latest change should address the comments on the previous commit. Please let me know if I missed or misunderstood anything. |
|
@nikomatsakis is on vacation until Aug 15. I think I'll review this myself. r? @arielb1 |
|
I thought I could create a nicer way of getting the |
|
So @bors r+ |
|
📌 Commit 0d29cd4 has been approved by |
Non-lexical lifetimes region renumberer Regenerates region variables for all regions in a cloned MIR in the nll mir pass. This is part of the work for #43234.
|
☀️ Test successful - status-appveyor, status-travis |
Regenerates region variables for all regions in a cloned MIR in the nll mir pass. This is part of the work for #43234.