Regression between nightlies with `-Zpolonius`

[Nadrieril]

Code

I tried this code:

// GAT hack taken from https://docs.rs/lending-iterator/latest/lending_iterator.
pub trait LendingIterator: Sized
where
    Self: for<'item> LendingIteratorItem<'item>,
{
    fn next(&mut self) -> Option<<Self as LendingIteratorItem>::Item>;
}

/// Hack to express a GAT without GATs.
pub trait LendingIteratorItem<'item> {
    type Item;
}

pub struct Wrapper<I> {
    wrapped: Option<I>,
}

impl<'item, I> LendingIteratorItem<'item> for Wrapper<I>
where
    I: LendingIteratorItem<'item>,
{
    type Item = I::Item;
}

impl<I> LendingIterator for Wrapper<I>
where
    I: LendingIterator,
{
    fn next(&mut self) -> Option<<I as LendingIteratorItem>::Item> {
        if let Some(first) = &mut self.wrapped {
            if let Some(next) = first.next() {
                return Some(next);
            } else {
                self.wrapped = None;
            }
        }
        None
    }
}

When running this with -Zpolonius, this worked with nightly-2024-05-31. With nightly-2024-06-01, I get this error message:

error[E0506]: cannot assign to `self.wrapped` because it is borrowed
  --> src/lib.rs:34:17
   |
29 |     fn next(&mut self) -> Option<<I as LendingIteratorItem>::Item> {
   |             - let's call the lifetime of this reference `'1`
30 |         if let Some(first) = &mut self.wrapped {
   |                              ----------------- `self.wrapped` is borrowed here
31 |             if let Some(next) = first.next() {
32 |                 return Some(next);
   |                        ---------- returning this value requires that `self.wrapped` is borrowed for `'1`
33 |             } else {
34 |                 self.wrapped = None;
   |                 ^^^^^^^^^^^^ `self.wrapped` is assigned to here but it was already borrowed

Bisection

searched nightlies: from nightly-2024-05-31 to nightly-2024-06-01 regressed nightly: nightly-2024-06-01 searched commit range: https://github.com/rust-lang/rust/compare/6f3df08aadf71e8d4bf7e49f5dc10dfa6f254cb4...ada5e2c7b5427a591e30baeeee2698a5eb6db0bd regressed commit: https://github.com/rust-lang/rust/commit/ada5e2c7b5427a591e30baeeee2698a5eb6db0bd

bisected with cargo-bisect-rustc v0.6.8

Host triple: x86_64-unknown-linux-gnu Reproduce with: ```bash cargo bisect-rustc --start=2024-05-31 --end=2024-06-01 ```

This is likely caused by https://github.com/rust-lang/rust/pull/125652, cc @amandasystems.

[amandasystems]

Ok that was almost certainly caused by my fix, thanks for finding this. I’ll look into it this week!

[lqd]

Do we know if this an actual regression or a false negative that was "fixed" by #125652? I myself didn't have time to look at the differences in fact generation produced by the PR, nor test cases that could be impacted -- and I don't know of Amanda's own results there.

This looks like the former but -Zpolonius is incomplete and misses errors in some/most/all [^1] cases of higher-ranked subtyping, because it'd require interactions with the trait solver. #123720 changes the situation here by lowering incompatible universe constraints to the root universe, so it will also be interesting to check: what happens in the complete 4-case matrix of with/without #125652 + with/without #123720?

And finally, since this pattern is obviously one of our most important use-cases, I'd also be interested in knowing why use the hack to define your lending iterators? Are there other constraints that prevent using regular GATs, or was that maybe a constraint of your own tooling?

[^1]: rayer les mentions inutiles

[amandasystems]

My understanding (though not very firm) was that #125652 should be a no-op for Polonius facts and I’ll have to look into why it isn’t, so this bug is useful in the sense that it proves this belief wrong.

In general this code path is too complicated and I’d like to clear it up; this seems like a good motivation to do that now.

[Nadrieril]

Do we know if this an actual regression or a false negative that was "fixed" by #125652?

I don't know if this is a valid argument, but I was able to make this code work using polonius_the_crab, which suggests to me that it should be accepted.

And finally, since this pattern is obviously one of our most important use-cases, I'd also be interested in knowing why use the hack to define your lending iterators?

Yep, I can't use GATs because a well-known limitation: I want to define a trait alias

pub trait TypeWalker:
    Sized + LendingIterator + for<'item> LendingIterator<Item<'item> = (&'item mut dyn Any, Event)>
{}
impl<T> TypeWalker for T
where
    T: LendingIterator,
    T: for<'item> LendingIterator<Item<'item> = (&'item mut dyn Any, Event)>,
{}

but if I use real GATs this bound forces Self: 'static, which is undesireable for my use case.

[amandasystems]

First things first, I guess:

@rustbot claim

I have looked into this a bit, and the code I reactivated in #125652 now seems deeply suspicious to me. I'll continue investigations this week.

This looks like the former but -Zpolonius is incomplete and misses errors in some/most/all cases of higher-ranked subtyping, because it'd require interactions with the trait solver. #123720 changes the situation here by lowering incompatible universe constraints to the root universe, so it will also be interesting to check: what happens in the complete 4-case matrix of with/without #125652 + with/without #123720?

The results are in!

	Polonius --> NLL	No Polonius --> NLL
Higher-kinded constraints lowered	Fails	Compiles
Higher-kinded constraints not lowered	Fails	Compiles

Specifically, removing the call to polonius::add_drop_of_var_derefs_origin(self.cx.typeck, dropped_local, &kind); when adding drop facts from Polonius using add_drop_live_facts_for() is enough to address the issue.