fix for issues #51351 and #52113#52620
Merged
bors merged 1 commit intorust-lang:masterfrom Jul 29, 2018
Merged
Conversation
rust-highfive
added
the
S-waiting-on-review
src/test/ui/nll/issue-51351.rs
Outdated
Contributor
There was a problem hiding this comment.
Is this referring to the right issue?
Contributor
Author
There was a problem hiding this comment.
sorry, just copy & paste...
nikomatsakis
requested changes
Jul 23, 2018
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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...
Contributor
There was a problem hiding this comment.
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());
}
nikomatsakis
requested changes
Jul 24, 2018
Contributor
There was a problem hiding this comment.
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.
nikomatsakis
requested changes
Jul 25, 2018
Contributor
There was a problem hiding this comment.
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.
Contributor
|
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 |
mikhail-m1
force-pushed
the
51351
branch
2 times, most recently
from
c668a8b to
2e7a62a
Compare
mikhail-m1
changed the title
mikhail-m1
commented
Jul 27, 2018
Contributor
Author
There was a problem hiding this comment.
I wonder why is this nice error only in compare mode
Contributor
|
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 |
nikomatsakis
changed the title
nikomatsakis
requested changes
Jul 28, 2018
Contributor
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Looks awesome! I just had some documentation nits.
Contributor
There was a problem hiding this comment.
can this be reverted?
Contributor
There was a problem hiding this comment.
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. ;)
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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 = ...;
}
}
Contributor
There was a problem hiding this comment.
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
Contributor
There was a problem hiding this comment.
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.
Contributor
|
@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 =) |
Collaborator
|
✌️ @mikhail-m1 can now approve this pull request |
Contributor
Author
|
@bors r=nikomatsakis |
Collaborator
|
📌 Commit bb66d70 has been approved by |
bors
added
S-waiting-on-bors
Collaborator
bors
added a commit
that referenced
this pull request
Jul 29, 2018
fix simple case of issue #51351 and #52133 r? @nikomatsakis
Collaborator
|
☀️ Test successful - status-appveyor, status-travis |
mikhail-m1
changed the title
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Jul 30, 2018
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
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jul 31, 2018
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
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Aug 1, 2018
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
emilyalbini
added a commit
to emilyalbini/rust
that referenced
this pull request
Aug 1, 2018
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix for #51351 and #52113
r? @nikomatsakis