Use the entire type of a dropped local to compute variance (edge direction) for Polonius alpha - #161305
Open
amandasystems wants to merge 5 commits into
Open
Use the entire type of a dropped local to compute variance (edge direction) for Polonius alpha#161305amandasystems wants to merge 5 commits into
amandasystems wants to merge 5 commits into
Conversation
of dropped variables. This works around an off-by-one in type variance computation causing a soundness issue.
rustbot
added
S-waiting-on-review
Collaborator
|
r? @mejrs rustbot has assigned @mejrs. Use Why was this reviewer chosen?The reviewer was selected based on:
|
amandasystems
changed the title
amandasystems
commented
Aug 18, 2026
| // the destructor and must be live at this point. | ||
| for &kind in &drop_data.dropck_result.kinds { | ||
| debug!("Drop-liveness is making the following type live: {kind:?}"); | ||
| Self::make_all_regions_live(self.location_map, self.typeck, kind, live_at); |
Contributor
Author
There was a problem hiding this comment.
It might be the case that we want to do record_polonius_region_variance_from_type() on these too; I genuinely don’t know.
jackh726
reviewed
Aug 18, 2026
| /// points `live_at`. | ||
| fn add_use_live_facts_for(&mut self, value: Ty<'tcx>, live_at: &IntervalSet<PointIndex>) { | ||
| debug!("add_use_live_facts_for(value={:?})", value); | ||
| Self::record_polonius_region_variance_from_type(self.typeck, value); |
Member
There was a problem hiding this comment.
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.)
Contributor
Author
There was a problem hiding this comment.
We have the exact opposite in fact! An unwrap-or-default for when we haven’t!
Member
There was a problem hiding this comment.
What's the fallout for just an expect there?
Contributor
Author
There was a problem hiding this comment.
(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)
Member
There was a problem hiding this comment.
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
Member
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
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).
Member
|
@lqd and I were looking at this a bit today First, this example is a bit more minimal, which should help to understand the MIR a bit more. struct D<T: HasArg>(T::Arg);
trait HasArg {
type Arg;
}
impl<'a, T> HasArg for fn(&'a T) {
type Arg = &'a T;
}
impl<T: HasArg> Drop for D<T> {
fn drop(&mut self) {}
}
fn mk<'a, T>(r: &'a T) -> D<fn(&'a T)> {
D(r)
}
fn main() {
let b = Box::new(0u8);
let d;
d = mk(&*b);
drop(b); // ERROR: move out of borrowed...
}One thing we noticed was that the |
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
lqd
reviewed
Aug 31, 2026
| if self.liveness.is_live_at_point(node.region, node.point) { | ||
| self.live_loans.insert(node.point, loan); | ||
| } else { | ||
| debug!("Region not live at {node:?}; marking loan {loan:?} as not live!") |
Member
There was a problem hiding this comment.
(this is not marking the loan as not live)
lqd
reviewed
Aug 31, 2026
| /// `live_kind` is the type of a use-live of drop-live local. | ||
| /// Record the variance of any region(s) appearing in it for | ||
| /// Polonius. Does nothing if Polonius is not active. | ||
| fn record_polonius_region_variance_from_type( |
Member
There was a problem hiding this comment.
We can name this something simpler like record_variance or something, and we shouldn't to need it to be generic and can use a Ty.
lqd
reviewed
Aug 31, 2026
Member
There was a problem hiding this comment.
We should add some comment about why this test is interesting here, drop polonius alpha from the name and have revisions to show the failure should happen regardless of the borrowck algorithm we use.
lqd
reviewed
Aug 31, 2026
| let mut logical_edges: FxHashMap<_, FxIndexSet<_>> = FxHashMap::default(); | ||
|
|
||
| for outlives_constraint in outlives_constraints { | ||
| debug!(?outlives_constraint); |
Member
There was a problem hiding this comment.
I'm weary all the debug annotations will make logging super verbose, when no one needs them 99% of the time. Having better dumping and debugging would help avoid them, maybe these being at the trace level instead of debug would also help the common case I don't know. wdyt @jackh726
Contributor
Author
There was a problem hiding this comment.
I agree; you generally only want these for one MIR body and often select locals/regions/loans, so something that allows filtering and querying would be much, much better
Contributor
|
☔ The latest upstream changes (presumably #162148) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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):
This generates a path through MIR on the way to a drop that looks like this:
In this instance, the types of
_1and_2areD<fn(&'?1 Vec<...>)andD<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::kindsare used to register drop live regions and compute their variances. However, instead of using the full typeD<...>for the left-hand side of this assignment statement,kindsstarts with aBinder {...}and the function type inside of it. From that it finds region'?1and 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 overif the local contains a region whose variance is actually not needed for computation or in regard to drop liveness (assuming that ever happens).DropckOutlivesResult::kinds(which should be redundant with it in most cases), orIt also adds some debug statements that helped me debug the issue, and a ui test for the soundness issue.