Use the entire type of a dropped local to compute variance (edge direction) for Polonius alpha

[amandasystems]

Fixes: #160670

The soundness issue is caused by (as suggested by the text extruder) the incorrect variance for a region, which is supposed to be bidirectional (invariant) but is registered as contravariant.

Starting with this example (from the issue):

use std::fmt::Debug;

struct D<T: HasArg>(T::Arg);

trait HasArg {
    type Arg: Debug;
}
impl<'a, T: Debug> HasArg for fn(&'a T) {
    type Arg = &'a T;
}
impl<T: HasArg> Drop for D<T> {
    fn drop(&mut self) {
        println!("{:?}", self.0);
    }
}
fn mk<'a, T: Debug>(r: &'a T) -> D<fn(&'a T)> {
    D(r)
}

fn main() {
    let b = Box::new(vec![vec![1]]);
    let d;
    d = mk(&*b);
    drop(b); // ERROR: move out of borrowed...
}

This generates a path through MIR on the way to a drop that looks like this:

_1 = move _2

/// ...

drop(_1)

In this instance, the types of _1 and _2 are D<fn(&'?1 Vec<...>) and D<fn(&'?2 Vec<...>) respectively.

During liveness computation (in liveness::trace) region liveness is computed from drop liveness and use liveness. Additionally, for each live region (drop-live or use-live), region variance is computed for Polonius' loan propagation. Variance determines the direction of propagation across program flow.

For drop-live locals (variables), the types reported in DropckOutlivesResult::kinds are used to register drop live regions and compute their variances. However, instead of using the full type D<...> for the left-hand side of this assignment statement, kinds starts with a Binder {...} and the function type inside of it. From that it finds region '?1 and records it as contravariant (backwards propagated).

This PR addresses the issue by using the entire type of the drop-live local to compute the variance of any regions referenced inside it, at the cost of potentially doing unnecessary extra work, either when iteration continues over DropckOutlivesResult::kinds (which should be redundant with it in most cases), or if the local contains a region whose variance is actually not needed for computation or in regard to drop liveness (assuming that ever happens).

It also adds some debug statements that helped me debug the issue, and a ui test for the soundness issue.

[rustbot]

r? @mejrs

rustbot has assigned @mejrs.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: borrowck, compiler
• borrowck, compiler expanded to 75 candidates
• Random selection from 16 candidates

[amandasystems]

It might be the case that we want to do record_polonius_region_variance_from_type() on these too; I genuinely don’t know.

View changes since the review

[jackh726]

I'm surprised we don't have a check elsewhere that asserts that we've recorded the variance for all regions.

(For the issue I was looking, I was thinking that we might want to ensure that liveness for every region var is recorded.)

View changes since the review

[amandasystems]

We have the exact opposite in fact! An unwrap-or-default for when we haven’t!

[jackh726]

What's the fallout for just an expect there?

[amandasystems]

(That would be the fallback case that activates if I removed some type from the drop liveness, and it’s fail safe into bidirectional edges)

[lqd]

that we've recorded the variance for all regions.

we don't propagate loans to dead regions throughout the CFG so I may be misunderstanding what you mean

[lqd]

And the default case for a region whose variance we haven't recorded is to create bidir edges, so it wouldn't have helped to record the variance for all regions, depending on what you mean by "all". This case has to be because the variance is different in different types/drop kinds with some shared free region, and we're not recording all of these different contexts, not missing regions.

So the fun thing here looks to be the local is either not drop-live but some of its """drop kinds""" are, or it is and we needed to also, or only, record the variance of free regions in its type instead of just of the ones the actual drop-live drop kinds. Remember: NLL only records the regions in the drop kinds as live. The fact that the variance changes between these and the local's type is just the cherry on top.

[amandasystems]

the local is either not drop-live but some of its """drop kinds""" are

That's possibly what's happening, though I'm not sure because all the parameter names seem to suggest the local is indeed drop live. The local becomes (drop) live in the next statement (I'm pretty sure, haven't fully checked), but for some reason, possibly due to assignment, its own type isn't among the drop-live kinds (only one layer in, for some reason). This might well be a bug or (more likely) an undocumented optimisation in drop live kind computation, but I don't know what it's supposed to do so I didn't dare touch it.

Before this apparently nobody needed the entire type of the local being dropped in all cases, but we do for the variance.

If the drop kinds never add anything to the variance, this PR is correct and may in fact be a slight optimisation, who knows, since it doesn't use the kinds to compute variance. If they may contain regions not in the type of the dropped local, I may lose variance information for them. This is a potential soundness issue if they appear somewhere else with an edge in only one direction, but otherwise just a risk of a compile failure (because we fail safe to a bidir edge). This can be trivially fixed by doing the variance computation on all the live kinds, at the risk of doing duplicate work.

I intentionally don't touch liveness, since it should be correct (tm).