Stream: t-compiler/wg-nll

Topic: issue-51351


mikhail-m1 (Jul 13 2018 at 09:47, on Zulip):

Hi, I'm trying to fix https://github.com/rust-lang/rust/issues/51351, and would like to discuss it with some one, @nikomatsakis left comments, but looks like he is not around.

DPC (Jul 13 2018 at 09:54, on Zulip):

hi @mikhail-m1 welcome here. Niko is on vacation these days so he isn't online for long periods. Dont' worry, he will reply you here when he is online.

nikomatsakis (Jul 13 2018 at 16:29, on Zulip):

@mikhail-m1 I'm around a bit -- feel free to leave questions, and/or open a [WIP] PR

mikhail-m1 (Jul 16 2018 at 15:59, on Zulip):

@nikomatsakis I added comments to my fix https://github.com/mikhail-m1/rust/commit/f391ec585622673bddd5d0d30a0831910c7844dc , It will be great if you share your thoughts

mikhail-m1 (Jul 16 2018 at 16:00, on Zulip):

it's to ugly to create PR...

nikomatsakis (Jul 16 2018 at 16:03, on Zulip):

@mikhail-m1 ok I sort of forget the context but will take a look!

nikomatsakis (Jul 16 2018 at 16:03, on Zulip):

ah yes, that problem.

nikomatsakis (Jul 16 2018 at 16:03, on Zulip):

/me shudders

nikomatsakis (Jul 16 2018 at 16:48, on Zulip):

@mikhail-m1 ok so I read the diffs, now I have to refresh my memory from the issue itself :)

nikomatsakis (Jul 16 2018 at 16:49, on Zulip):

ok right so this is the "simple case" of the problem I think...

mikhail-m1 (Jul 16 2018 at 21:09, on Zulip):

I don't understand why for fn with_signature<'a, T, F>(cell: Cell<&'a ()>, t: T, op: F) rustc::middle::resolve_lifetime: insert_late_bound_lifetimes: lifetime T#0 with id NodeId(17) is late-bound and same for F, but T and F are not lifetimes

nikomatsakis (Jul 17 2018 at 03:31, on Zulip):

@mikhail-m1 so, that example is the more complex one I think

nikomatsakis (Jul 17 2018 at 03:31, on Zulip):

that is, there are two cases

nikomatsakis (Jul 17 2018 at 03:31, on Zulip):

the simplest case is the one that people reduced the ICE to:

fn foo<'a>() {
    let x: &'a u32 = &22;
}

Here, 'a is not used anywhere in the signature.

nikomatsakis (Jul 17 2018 at 03:32, on Zulip):

This is the one I was targeting initially.

nikomatsakis (Jul 17 2018 at 03:32, on Zulip):

Sorry I didn't get back to you last night, ran out of time.

nikomatsakis (Jul 17 2018 at 03:38, on Zulip):

but I think it's probably worth talking through the closure case

nikomatsakis (Jul 17 2018 at 03:39, on Zulip):

that said, I can't really do that right now, I'll be back in ~7 hours or something :)

mikhail-m1 (Jul 17 2018 at 08:50, on Zulip):

@nikomatsakis yes, my changes don't fix next ICE, trying find why

fn produce<'a>() {
    move || { let x: &'a () = &(); };
}
mikhail-m1 (Jul 17 2018 at 10:28, on Zulip):

I've verified my assumption, for the closure tcx.is_late_bound_map returns None, we need to find another way to get late bound regions

nikomatsakis (Jul 17 2018 at 11:23, on Zulip):

yeah... I think I know why they don't fix it :) but I have to think about how to best fix it

mikhail-m1 (Jul 19 2018 at 14:29, on Zulip):

hi @nikomatsakis i found a way to fix the closure case by get up by calling hir.get_parent_node(..), and add late_bound regions for parent too,
but your comment about add it to ClosureRegionRequirements looks better, but I need more details to start and an example for tests propagation to parent

nikomatsakis (Jul 19 2018 at 14:31, on Zulip):

argh I forgot to get back to you on this, yes. Just added it to my to-do list for today to write up something

nikomatsakis (Jul 19 2018 at 17:52, on Zulip):

@mikhail-m1 how much have you looked at the ClosureRegionRequirements code? i.e., you get the basic idea of it?

nikomatsakis (Jul 19 2018 at 21:16, on Zulip):

I left some notes in a github comment @mikhail-m1 -- not sure if they are helpful or not -- let me know and I will try to elaborate. =) At least I worked through what I think should happen in my own head, though, and I think it should all work out.

mikhail-m1 (Jul 20 2018 at 08:49, on Zulip):

@nikomatsakis thanks for helpful comment, just two questions:
1. What should i do with 51351? I can create PR from commit but it doesn't look right:

DPC (Jul 20 2018 at 10:18, on Zulip):

@mikhail-m1 you can create a 'WIP' PR (put [WIP] in the title of the PR so people know you are working on it). Becomes easy for people to review it.

nikomatsakis (Jul 21 2018 at 04:07, on Zulip):

@mikhail-m1 first off, @DPC is right that a [WIP] PR is often very helpful.

To answer your questions:

Is order of how function processed is determinate? Are closures processed first?

Sort of — closures will always finish being processed first, at least. This is because the closure's creator will invoke the mir_borrowck query on each closure that appears within its function body in order to get the ClosureRegionRequirements.

create region, FreeRegion, BrRegion

I don't quite understand what you mean by this -- oh, you mean create one of those for each late-bound-region on the fn -- this sounds roughly right, yes. One thing to be careful of though is that we already have late-bound regions for things that appear in the fn arguments etc. For example:

fn foo<'a>(x: &'a u32) { }

in that case, 'a is a LBR, but we will have created a corresponding universal region for it already. So I guess we want to check the table for that to see if there is already an entry.

What should i do with 51351?

I think a [WIP] PR is a good idea -- it seems like addressing the case of fn foo<'a>() { let x: &'a u32 = ...; } is a good starting point in any case for the more complex problem, since it too requires enumerating the late-bound regions. Though there are some differences (e.g., in the closure case, we won't have entries for any late-bound regions from the enclosing fn's signature, I don't think).

nikomatsakis (Jul 23 2018 at 14:57, on Zulip):

@mikhail-m1 left some comments on your PR -- looks basically good, but I did have one concern about how we check for duplicates

mikhail-m1 (Jul 23 2018 at 15:12, on Zulip):

@nikomatsakis thanks, I will try to update it today, just two questions:
1. do I need to add more tests like vector.push(an_reference) from an closure?
2. I don't see anything else to do for https://github.com/rust-lang/rust/issues/52113 , did I missed something?

nikomatsakis (Jul 23 2018 at 15:20, on Zulip):

@mikhail-m1 I don't know what the vector.push(an_reference) is referring to...

nikomatsakis (Jul 23 2018 at 15:21, on Zulip):

I think that there is more to do for #52113, let me see... maybe I'll try to check out your branch and test it...

nikomatsakis (Jul 23 2018 at 15:21, on Zulip):

I imagine though that something like this might pass or fail incorrectly...

nikomatsakis (Jul 23 2018 at 15:26, on Zulip):
#![allow(warnings)]
#![feature(nll)]

trait Bazinga { }

impl<F> Bazinga for F { }

fn produce<'a>(data: &mut Vec<&'a u32>, value: &'a u32) -> impl Bazinga + 'a {
    let x = move || {
        let value: &'a u32 = value;
        data.push(value);
    };
    x
}

fn main() { }

or maybe not that... I'm basically nervous about constraints getting created between the 'a that results from instantiate the LBR of the parent and the 'a that appears in the upvars.

nikomatsakis (Jul 23 2018 at 15:26, on Zulip):

that said, it would also be interesting to see if I can concoct a test that fails with your branch as is

nikomatsakis (Jul 23 2018 at 15:27, on Zulip):

I'm not sure if it's possible -- basically we'd want to engineer some kind of false conflict between the LBR in a closure's parameters and the LBR from the enclosing fn --

nikomatsakis (Jul 23 2018 at 15:27, on Zulip):

given that, currently, I think the LBR in a closure sig are always anonymous

nikomatsakis (Jul 23 2018 at 15:27, on Zulip):

that may not be possible (today)

mikhail-m1 (Jul 23 2018 at 15:55, on Zulip):

@nikomatsakis I mean

fn foo<'a, 'b>(x: &'a u32, mut y: Vec<&'b u32>) {
  let closure = move || y.push(x);
 }

for 'a:'b and 'b:'a

mikhail-m1 (Jul 24 2018 at 10:54, on Zulip):

@nikomatsakis your sample

#![allow(warnings)]
#![feature(nll)]

trait Bazinga { }

impl<F> Bazinga for F { }

fn produce<'a>(data: &mut Vec<&'a u32>, value: &'a u32) -> impl Bazinga + 'a {
    let x = move || {
        let value: &'a u32 = value;
        data.push(value);
    };
    x
}

fn main() { }

or maybe not that... I'm basically nervous about constraints getting created between the 'a that results from instantiate the LBR of the parent and the 'a that appears in the upvars.

after change to data: &'a mut Vec<&'a u32> gives

15 | fn produce<'a>(data: &'a mut Vec<&'a u32>, value: &'a u32) -> impl Bazinga + 'a {
   |                ---- lifetime `'1` appears in the type of `data`
...
18 |         data.push(value);
   |         ^^^^^^^^^^^^^^^^ argument requires that `'a` must outlive `'1`

and a sample from https://github.com/rust-lang/rust/issues/52113#issue-338985041 gives

8 |     let x = move || {
  |         ^ free region requires that `'a` must outlive `'static`

without nll both compile. I'm trying to understand why...

nikomatsakis (Jul 24 2018 at 11:53, on Zulip):

ok — I suspect I know why but I'll have to try it out

nikomatsakis (Jul 24 2018 at 11:54, on Zulip):

I think my build of your branch is done

nikomatsakis (Jul 24 2018 at 11:56, on Zulip):

oh, ok, so

nikomatsakis (Jul 24 2018 at 11:56, on Zulip):

the first problem is this:

nikomatsakis (Jul 24 2018 at 11:56, on Zulip):

the late-bound lifetimes from the base fn are being (I believe) created at the wrong point

nikomatsakis (Jul 24 2018 at 11:56, on Zulip):

in particular, they are considered late-bound on the closure itself

nikomatsakis (Jul 24 2018 at 11:56, on Zulip):

which is incorrect

nikomatsakis (Jul 24 2018 at 11:57, on Zulip):

this is what causes us to product the first error:

error: unsatisfied lifetime constraints
  --> /Users/nmatsakis/tmp/m1.rs:11:9
   |
8  | fn produce<'a>(data: &mut Vec<&'a u32>, value: &'a u32) -> impl Bazinga + 'a {
   |                ---- lifetime `'1` appears in the type of `data`
...
11 |         data.push(value);
   |         ^^^^^^^^^^^^^^^^ argument requires that `'a` must outlive `'1`
nikomatsakis (Jul 24 2018 at 11:57, on Zulip):

in this case, the 'a is considered a "local" region to the closure, and hence not one where we are able to propagate a "closure requirement" back to the caller

nikomatsakis (Jul 24 2018 at 11:57, on Zulip):

to fix this, we have to create those LBR from the base-def-id at a different point

nikomatsakis (Jul 24 2018 at 11:58, on Zulip):

in particular, before this line executes

nikomatsakis (Jul 25 2018 at 10:41, on Zulip):

@mikhail-m1 ping

mikhail-m1 (Jul 25 2018 at 10:45, on Zulip):

hi @nikomatsakis , I didn't have time to rewrite it because I need to change way how regions from inputs_output filtered out

nikomatsakis (Jul 25 2018 at 10:45, on Zulip):

ok :)

nikomatsakis (Jul 25 2018 at 10:45, on Zulip):

just checking in

mikhail-m1 (Jul 25 2018 at 10:45, on Zulip):

hope I will do it today

nikomatsakis (Jul 25 2018 at 13:16, on Zulip):

let me know if I can help

mikhail-m1 (Jul 25 2018 at 20:29, on Zulip):

@nikomatsakis I'm stuck with update closure_mapping, cannot find a way how to add later bound regions to region_mapping

mikhail-m1 (Jul 25 2018 at 20:30, on Zulip):

because now only regions from closure_ty are added, and assert fails

nikomatsakis (Jul 25 2018 at 20:30, on Zulip):

@mikhail-m1 you are trying to do the change I suggested, specifically? (That is, the reordering sort of?)

nikomatsakis (Jul 25 2018 at 20:31, on Zulip):

can I see the diff?

mikhail-m1 (Jul 25 2018 at 20:33, on Zulip):

diff is almost the same, yes i made your suggestion, but after that assert inside fn closure_mapping starts failing

mikhail-m1 (Jul 25 2018 at 20:34, on Zulip):
        assert_eq!(
            region_mapping.len(),
            expected_num_vars,
            "index vec had unexpected number of variables");
nikomatsakis (Jul 25 2018 at 20:34, on Zulip):

would still be helpful to see the diff

nikomatsakis (Jul 25 2018 at 20:34, on Zulip):

I don't remember the details :)

nikomatsakis (Jul 25 2018 at 20:34, on Zulip):

that is, exactly wht change you made :)

mikhail-m1 (Jul 25 2018 at 20:35, on Zulip):

just a sec

mikhail-m1 (Jul 25 2018 at 20:39, on Zulip):

i've updater PR https://github.com/rust-lang/rust/pull/52620

nikomatsakis (Jul 25 2018 at 20:45, on Zulip):

diff is almost the same, yes i made your suggestion, but after that assert inside fn closure_mapping starts failing

yes, so, this doesn't surprise me :) in fact, this is good

nikomatsakis (Jul 25 2018 at 20:45, on Zulip):

I left one comment but I don't think it'll fix your problem per se

nikomatsakis (Jul 25 2018 at 20:46, on Zulip):

but the reason that the assertion is failing -- I think -- is precisely because we're not handling the closure case correctly

nikomatsakis (Jul 25 2018 at 20:46, on Zulip):

that is, we need to upgrade the code around the ClosureRegionRequirements

nikomatsakis (Jul 25 2018 at 20:46, on Zulip):

right now it works kind of implicitly

nikomatsakis (Jul 25 2018 at 20:46, on Zulip):

that is

nikomatsakis (Jul 25 2018 at 20:47, on Zulip):

there is a closure "defining type"

mikhail-m1 (Jul 25 2018 at 20:47, on Zulip):

as i understand, after analysis, apply_requirements calls closure_mapping for get all external regions for ClosureRegionRequirements, and I need find a way how to add late bound regions, because now only regions from closure_ty are added

nikomatsakis (Jul 25 2018 at 20:47, on Zulip):

and we basically number the regions as they appear in there, and reference those numbers

nikomatsakis (Jul 25 2018 at 20:47, on Zulip):

right, it needs to sort of "mirror" that set so it knows how to map them

nikomatsakis (Jul 25 2018 at 20:49, on Zulip):

so annoying

mikhail-m1 (Jul 25 2018 at 20:49, on Zulip):

I cannot find a function to get Region by RegionVid

nikomatsakis (Jul 25 2018 at 20:49, on Zulip):

that's not really a thing

nikomatsakis (Jul 25 2018 at 20:50, on Zulip):

I'm not quite sure what you mean :)

nikomatsakis (Jul 25 2018 at 20:50, on Zulip):

you can do tcx.mk_region(ty::ReVar(v2))

nikomatsakis (Jul 25 2018 at 20:50, on Zulip):

to create a Region<'tcx> for a given region vid-- maybe that's what you mean?

nikomatsakis (Jul 25 2018 at 20:51, on Zulip):

so one thing we could do maybe

mikhail-m1 (Jul 25 2018 at 20:51, on Zulip):

After adding late_bound regions I have RegionVid for them, and I think I need get back Regions and add them to vector

nikomatsakis (Jul 25 2018 at 20:51, on Zulip):

is to have the ClosureRegionRequirements also include the list of late-bound regions from the parent fn that were instantiated

nikomatsakis (Jul 25 2018 at 20:52, on Zulip):

After adding late_bound regions I have RegionVid for them, and I think I need get back Regions and add them to vector

do you mean you want to get back the "named" regions? i.e., with the names from the signature?

nikomatsakis (Jul 25 2018 at 20:53, on Zulip):

if the CRR contained this list, then I guess that the apply_requirements code could -- when constructing this map -- basically go over each of those regions and look it up in the indices vector to find the corresponding RegionVid in the creator

mikhail-m1 (Jul 25 2018 at 20:53, on Zulip):

yes, but it's just idea

nikomatsakis (Jul 25 2018 at 20:53, on Zulip):

yeah I think that's the thing to do

nikomatsakis (Jul 25 2018 at 20:53, on Zulip):

ok, so you can get back the original Region... well, the indices map goes the wrong way

nikomatsakis (Jul 25 2018 at 20:53, on Zulip):

we build up a reverse vector in the RegionInferenceContext

nikomatsakis (Jul 25 2018 at 20:54, on Zulip):

right here

nikomatsakis (Jul 25 2018 at 20:54, on Zulip):

but maybe we just want to create a Vec<Region> containing all the LBR from the closure parent

nikomatsakis (Jul 25 2018 at 20:54, on Zulip):

in the UniversalRegions

nikomatsakis (Jul 25 2018 at 20:55, on Zulip):

and then when creating the ClosureRegionRequirements, we would embed that vector

nikomatsakis (Jul 25 2018 at 20:55, on Zulip):

something like that?

nikomatsakis (Jul 25 2018 at 20:56, on Zulip):

(we could create the vector in the replace_late_bound_regions_with_nll_infer_vars that you added, I mean)

mikhail-m1 (Jul 25 2018 at 20:56, on Zulip):

yes It should help

nikomatsakis (Jul 26 2018 at 15:28, on Zulip):

@mikhail-m1 I'm looking over the list and this is one of the only major issues without a PR; were you able to make any progress? can I help in some way?

mikhail-m1 (Jul 27 2018 at 09:00, on Zulip):

@nikomatsakis Sorry, I didn't received a notification about your message. I tried to pass a vector with regions from replace_late_bound_regions_with_nll_infer_varsto apply_requirements I didn't find a way, because UniveralRegions from closure analysis is already destroyed and I have only ClosureRegionRequirements<'gcx> maybe I should try just to get regions again by tcx.is_late_bound_map and recreate ty::Regions. In any case I hope I will have enough time during weekend to fix it.

mikhail-m1 (Jul 27 2018 at 11:16, on Zulip):

It works! I need to cleanup my code, I will update the PR today or tomorrow.

nikomatsakis (Jul 28 2018 at 04:17, on Zulip):

@mikhail-m1 nice!

Last update: Nov 21 2019 at 13:40UTC