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?

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

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

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

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

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

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.

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.

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.

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?

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

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

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

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.

ok

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.

(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> {
   |        ^^

@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?

Yeah.

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

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

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

okay

Filed #56508 as the followup issue.

:thumbs_up: