Stream: t-compiler/wg-nll

Topic: #55850 generator region name ICE


davidtwco (Dec 03 2018 at 12:36, on Zulip):

I've been looking into this issue. It seems like the region naming code is struggling to get the 'a from the parent function of a generator. Locally I've made some changes so the constraint is categorized as a yield rather than a return, then in that case I can try and name the region from the mir.yield_ty. However, that region ends up being a ty::RegionKind::ReVar that I can't really do anything with in terms of the existing naming code. Any ideas?

pnkfelix (Dec 03 2018 at 12:52, on Zulip):

in my opinion I'd focus on getting rid of the ICE even if it means giving up on getting any name at all

pnkfelix (Dec 03 2018 at 12:53, on Zulip):

especially since I assume this is a PR we'd want to backport

pnkfelix (Dec 03 2018 at 12:53, on Zulip):

and therefore its probably better to ensure the PR is easily to verify and backport?

pnkfelix (Dec 03 2018 at 12:53, on Zulip):

(i don't have immediate advice for how to extract a good name)

davidtwco (Dec 03 2018 at 12:53, on Zulip):

Alright, I'll look into doing that instead, at least as a first step.

davidtwco (Dec 03 2018 at 13:01, on Zulip):

Alright, code is a little less nice now I'm needing to return an Option where there was a panic before, but it doesn't ICE.

davidtwco (Dec 03 2018 at 14:45, on Zulip):

Put a PR in with that fix: #56460. Can decide if it makes more sense just to land that or continue to work on it so that it correctly names the region.

davidtwco (Dec 03 2018 at 14:46, on Zulip):

Given that this seems specific to generators which aren't stable, I'd argue it's fine to iterate on it for a while instead of landing quickly.

pnkfelix (Dec 03 2018 at 14:47, on Zulip):

hmm out of curiosity, why did you opt to make it produce BorrowExplanation::Unexplained rather than have BorrowExplanation::MustBeValidFor carry an Option<RegionName> instead of a RegionName?

pnkfelix (Dec 03 2018 at 14:48, on Zulip):

(maybe it doesn't matter; I haven't yet looked at the tests)

pnkfelix (Dec 03 2018 at 14:48, on Zulip):

or does MustBeValidFor become meaningless if you cannot attach a name to the region?

davidtwco (Dec 03 2018 at 14:49, on Zulip):

I don't think it'd really do much without the name.

davidtwco (Dec 03 2018 at 14:52, on Zulip):

Yeah, it normally highlights where the region is defined (in the fn signature normally) and then labels the place that causes the error saying "this lives for X, must live for Y" (or something along those lines) - but we wouldn't have the place where the region is defined or the name Y - that's the region naming part.

pnkfelix (Dec 03 2018 at 14:53, on Zulip):

ok

davidtwco (Dec 04 2018 at 10:43, on Zulip):

Looking into this more and I'm not really sure what a solution would look like.

pub struct GenIter<G>(G);
impl<G> Iterator for GenIter<G>
where
    G: Generator,
{
    type Item = G::Yield;
    fn next(&mut self) -> Option<Self::Item> {
        unsafe {
            match self.0.resume() {
                Yielded(y) => Some(y),
                _ => None
            }
        }
    }
}
fn bug<'a>() -> impl Iterator<Item = &'a str> {
    GenIter(move || {
        let mut s = String::new();
        yield &s[..]
    })
}

In the generator MIR, there's no outlives constraint between the yield type ('_#4r which is constrained to external region '_#2r) and the lifetime it would need to have 'a (external region '_#1r) - so when the borrow checker realises there's an error, there's no way for it find 'a. It's difficult because the relationship between 'a and the yield type is only apparent when you check the Iterator impl.

davidtwco (Dec 04 2018 at 10:49, on Zulip):

(as a reminder, the goal is to be able to add the NLL equivalent of this diagnostic note:)

note: borrowed value must be valid for the lifetime 'a as defined on the function body at 35:8...
  --> $DIR/issue-55850.rs:35:8
   |
LL | fn bug<'a>() -> impl Iterator<Item = &'a str> {
   |        ^^
pnkfelix (Dec 04 2018 at 15:18, on Zulip):

@davidtwco I assume you marked PR #56460 as WIP because, since it uses generators, you figure we can take a bit more time exploring the solution space?

davidtwco (Dec 04 2018 at 15:18, on Zulip):

Yeah.

pnkfelix (Dec 04 2018 at 15:18, on Zulip):

maybe we can land the fix to the ICE and file an issue for follow-up attempt to improve diagnostic here?

pnkfelix (Dec 04 2018 at 15:19, on Zulip):

(i looked over the PR again just now and it seems fine to me)

davidtwco (Dec 04 2018 at 15:19, on Zulip):

If you want to do that, feel free to un-WIP that PR and review. We can open a follow-up issue.

pnkfelix (Dec 04 2018 at 15:19, on Zulip):

okay

pnkfelix (Dec 04 2018 at 15:24, on Zulip):

Filed #56508 as the followup issue.

davidtwco (Dec 04 2018 at 15:28, on Zulip):

:thumbs_up:

Last update: Nov 21 2019 at 13:50UTC