-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
fix for issues #51351 and #52113 #52620
Conversation
src/test/ui/nll/issue-51351.rs
Outdated
// | ||
// An additional regression test for the issue #50716 “NLL ignores lifetimes | ||
// bounds derived from `Sized` requirements” that checks that the fixed compiler | ||
// accepts this code fragment with both AST and MIR borrow checkers. |
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.
Is this referring to the right 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.
sorry, just copy & paste...
@@ -490,6 +490,29 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> { | |||
&bound_inputs_and_output, | |||
&mut indices, | |||
); | |||
|
|||
// Add late-bound regions which are not used in in inputs and outhout |
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 it'd be nice to put this into a fn, along with a somewhat more extensive comment that (at minimum) cites the issue number.
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 a comment like this would be good:
Finds late-bound regions that do not appear in the parameter listing and adds them to the indices vector. Typically, we identify late-bound regions as we process the inputs and outputs of the closure/function. However, sometimes there are late-bound regions which do not appear in the fn parameters but which are nonetheless in scope. The simplest case of this are unused functions, like fn foo<'a>() { }
(see eg., #51351). Despite not being used, users can still reference these regions (e.g., let x: &'a u32 = &22;
), so we need to create entries for them and store them in the indices
map. This code iterates over the complete set of late-bound regions and checks for any that we have not yet seen, adding them to the inputs vector.
let name = map.name(map.hir_to_node_id(hir_id)).as_interned_str(); | ||
let def_id = map.local_def_id(map.hir_to_node_id(hir_id)); | ||
let region = ty::BoundRegion::BrNamed(def_id, name); | ||
if added_from_input_and_output.contains(®ion) { continue; } |
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.
Nit: I was thinking we could check indices
to detect if the region already exists, but I guess in that case we'd have to create the liberated_region
first. Still, we are doing that already below...
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.
Hmm, thinking a bit more about this, something seems a touch off. In particular, if we are looking at a closure, the added_from_input_and_output
is going to contain late-bound regions from the closure's inputs and outputs. But here we are comparing against late-bound regions bound on the closure owner (closure_base_def_id
).
I think comparing against entries in indices
would be more robust. The diff would basically be to remove added_from_input_and_output
and instead do:
let liberated_region = ...;
if !indices.contains_key(&liberated_region) {
let region_vid = self.infcx.next_nll_region_var(FR);
indices.insert_late_bound_region(liberated_region, region_vid.to_region_vid());
}
@@ -490,6 +490,9 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> { | |||
&bound_inputs_and_output, | |||
&mut indices, | |||
); | |||
|
|||
self.infcx.replace_late_bound_regions_with_nll_infer_vars(self.mir_def_id, &mut indices); |
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.
As I noted on Zulip, in the case of a closure, this code needs to execute earlier -- that is, the late-bound regions on the closure's creator are not local free regions, so they must be created up above, before the first_local_index
variable is assigned.
@@ -479,6 +479,9 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> { | |||
let mut indices = self.compute_indices(fr_static, defining_ty); | |||
debug!("build: indices={:?}", indices); | |||
|
|||
let _first_late_bound_index = self.infcx.num_region_vars(); | |||
self.infcx.replace_late_bound_regions_with_nll_infer_vars(self.mir_def_id, &mut indices); |
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 we only want to do this here if self.mir_def_id != closure_base_def_id
-- i.e., if this is a closure/generator -- otherwise, we want to do it in the location you originally had it.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
c668a8b
to
2e7a62a
Compare
@@ -0,0 +1,29 @@ | |||
error[E0597]: `*value` does not live long enough (Ast) |
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 wonder why is this nice error only in compare mode
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
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.
Looks awesome! I just had some documentation nits.
let closure_mapping = | ||
&UniversalRegions::closure_mapping(tcx, user_closure_ty, self.num_external_vids); | ||
let closure_base_def_id = tcx.closure_base_def_id(closure_def_id); | ||
/* |
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.
can this be reverted?
@@ -882,3 +929,23 @@ impl<'tcx> FreeRegionRelations<'tcx> for UniversalRegions<'tcx> { | |||
self.outlives(longer, shorter) | |||
} | |||
} | |||
|
|||
fn fold_late_bound_regions<'tcx>( |
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.
Nit: I would call this for_each_late_bound_region
-- fold
suggests to me that we are propagating a result back. I'd probably even call it for_each_late_bound_region_defined_on
but I sort of have a penchant for long names. ;)
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.
Also a comment is a good idea, something like:
Iterates over the late-bound regions defined on fn_def_id
and invokes `` with the liberated form of each one.
@@ -479,6 +482,14 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> { | |||
let mut indices = self.compute_indices(fr_static, defining_ty); | |||
debug!("build: indices={:?}", indices); | |||
|
|||
let closure_base_def_id = self.infcx.tcx.closure_base_def_id(self.mir_def_id); | |||
|
|||
if self.mir_def_id != closure_base_def_id { |
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 merits a comment. Something like
If this is a closure or generator, then the late-bound regions from the enclosing function are actually external regions to us. For example, here, 'a
is not local to the closure c
(although it is local to the fn foo
):
fn foo<'a>() {
let c = || {
let x: &'a u32 = ...;
}
}
@@ -490,6 +501,13 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> { | |||
&bound_inputs_and_output, | |||
&mut indices, | |||
); | |||
|
|||
if self.mir_def_id == closure_base_def_id { |
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.
Similarly, I would put a comment here like
Converse of above, if this is a function then the late-bound regions declared on its signature are local to the fn.
src/test/ui/nll/issue-51351.rs
Outdated
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
// | ||
// revisions: ast mir |
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.
Short comment documenting the purpose of the test would be nice:
Regression test for #51351 and #52133: In the case of #51351, late-bound regions (like 'a
) that were unused within the arguments of a function were overlooked and could case an ICE. In the case of #52133, LBR defined on the creator function needed to be added to the free regions of the closure, as they were not present in the closure's generic declarations otherwise.
@bors delegate=mikhail-m1 @mikhail-m1 -- I delegated to you, so once the nits are fixed, you can feel free to write "r=nikomatsakis" to bors =) |
✌️ @mikhail-m1 can now approve this pull request |
@bors r=nikomatsakis |
📌 Commit bb66d70 has been approved by |
fix simple case of issue #51351 and #52133 r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
Fix wrong issue number in the test name I made a mistake in previous PR rust-lang#52620, second issue number was wrong, changing from rust-lang#52133 to rust-lang#52113 r? @kennytm
Fix wrong issue number in the test name I made a mistake in previous PR rust-lang#52620, second issue number was wrong, changing from rust-lang#52133 to rust-lang#52113 r? @kennytm
Fix wrong issue number in the test name I made a mistake in previous PR rust-lang#52620, second issue number was wrong, changing from rust-lang#52133 to rust-lang#52113 r? @kennytm
Fix wrong issue number in the test name I made a mistake in previous PR rust-lang#52620, second issue number was wrong, changing from rust-lang#52133 to rust-lang#52113 r? @kennytm
fix for #51351 and #52113
r? @nikomatsakis