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: 6f3df08...ada5e2c
regressed commit: ada5e2c

bisected with cargo-bisect-rustc v0.6.8

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2024-05-31 --end=2024-06-01

This is likely caused by #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 ¹ 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?

Footnotes

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.

[danielhenrymantilla]

Heh, polonius-the-crab does not run into this because of the way it phrases the properties using that higher-order partitioning function. And "enforces" it through unsafe code.

I do, however, have a "soundness test", wherein I delegate a proof of soundness of my API to -Zpolonius, by disabling that unsafe code and making sure cargo rustc -- -Zpolonius passes. And now that I look at it, the test has been regressing since around the time of this issue, it looks like (good catch @Nadrieril!).

I have produced a smaller repro, at the very least: https://rust.godbolt.org/z/bb6n7b8Yx:

trait ForLt { type Of<'__>; }

fn polonius<T, R: ForLt>(
    input: &mut T,
    f: impl FnOnce(&mut T) -> Option<R::Of<'_>>,
) -> Result<R::Of<'_>, &mut T> {
    if let Some(r) = f(input) {
        Ok(r)
    } else {
        Err(input)
    }
}

---

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

This is indeed an important question!

I cannot answer that, but someone here mentioned the word "drop", so I decided to try and rewrite this very snippet in a way that would show the Option<R::Of<'_>> being "provably dropped" before the "polonius borrow-shortening logic" were needed:

trait ForLt { type Of<'__>; }

fn polonius<T, R: ForLt>(
    input: &mut T,
    f: impl FnOnce(&mut T) -> Option<R::Of<'_>>,
) -> Result<R::Of<'_>, &mut T> {
    if let Some(r) = f(input) {
        return Ok(r);
    }
    // else { drop(f_input) }
    Err(input)
}

• Godbolt

and that one works! So it just looks like the logic is a bit "too" scared of the drop of None::<Option<R::Of<'_>>>, which could be legitimate for other cases which might be too difficult for the compiler to distinguish? 🤷

[amandasystems]

it just looks like the logic is a bit "too" scared of the drop of None::<Option<R::Of<'_>>>, which could be legitimate for other cases which might be too difficult for the compiler to distinguish?

This checks out; I think me connecting a few loose pipes I found in #125652 must have caused too much drop-liveness logic to be applied. I'm very tempted to remove all of it but I'll have to look into it a bit more. So far we don't have a counterexample that shows unsoundness without the extra drop handling, but that doesn't mean one isn't hiding out there somewhere.

Anyway, thanks for the smaller repro, that one will be a lot easier to work on.