Skip to content

Convert typeck constraints in location-sensitive polonius #134920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 8, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions compiler/rustc_borrowck/src/borrow_set.rs
Original file line number Diff line number Diff line change
@@ -11,7 +11,6 @@ use rustc_mir_dataflow::move_paths::MoveData;
use tracing::debug;

use crate::BorrowIndex;
use crate::path_utils::allow_two_phase_borrow;
use crate::place_ext::PlaceExt;

pub struct BorrowSet<'tcx> {
@@ -350,7 +349,7 @@ impl<'a, 'tcx> GatherBorrows<'a, 'tcx> {
start_location, assigned_place, borrow_index,
);

if !allow_two_phase_borrow(kind) {
if !kind.allows_two_phase_borrow() {
debug!(" -> {:?}", start_location);
return;
}
36 changes: 34 additions & 2 deletions compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs
Original file line number Diff line number Diff line change
@@ -10,6 +10,7 @@ use rustc_hir::intravisit::Visitor;
use rustc_hir::{self as hir, BindingMode, ByRef, Node};
use rustc_middle::bug;
use rustc_middle::hir::place::PlaceBase;
use rustc_middle::mir::visit::PlaceContext;
use rustc_middle::mir::{
self, BindingForm, Local, LocalDecl, LocalInfo, LocalKind, Location, Mutability, Place,
PlaceRef, ProjectionElem,
@@ -22,7 +23,6 @@ use rustc_trait_selection::traits;
use tracing::debug;

use crate::diagnostics::BorrowedContentSource;
use crate::util::FindAssignments;
use crate::{MirBorrowckCtxt, session_diagnostics};

#[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -1084,6 +1084,38 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
}
}

/// Finds all statements that assign directly to local (i.e., X = ...) and returns their
/// locations.
fn find_assignments(&self, local: Local) -> Vec<Location> {
use rustc_middle::mir::visit::Visitor;

struct FindLocalAssignmentVisitor {
needle: Local,
locations: Vec<Location>,
}

impl<'tcx> Visitor<'tcx> for FindLocalAssignmentVisitor {
fn visit_local(
&mut self,
local: Local,
place_context: PlaceContext,
location: Location,
) {
if self.needle != local {
return;
}

if place_context.is_place_assignment() {
self.locations.push(location);
}
}
}

let mut visitor = FindLocalAssignmentVisitor { needle: local, locations: vec![] };
visitor.visit_body(self.body);
visitor.locations
}

fn suggest_make_local_mut(&self, err: &mut Diag<'_>, local: Local, name: Symbol) {
let local_decl = &self.body.local_decls[local];

@@ -1117,7 +1149,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
})) => {
// check if the RHS is from desugaring
let opt_assignment_rhs_span =
self.body.find_assignments(local).first().map(|&location| {
self.find_assignments(local).first().map(|&location| {
if let Some(mir::Statement {
source_info: _,
kind:
21 changes: 10 additions & 11 deletions compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
@@ -17,7 +17,7 @@

use std::cell::RefCell;
use std::marker::PhantomData;
use std::ops::Deref;
use std::ops::{ControlFlow, Deref};

use rustc_abi::FieldIdx;
use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
@@ -81,7 +81,6 @@ mod session_diagnostics;
mod type_check;
mod universal_regions;
mod used_muts;
mod util;

/// A public API provided for the Rust compiler consumers.
pub mod consumers;
@@ -1054,31 +1053,31 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
rw,
(borrow_index, borrow),
);
Control::Continue
ControlFlow::Continue(())
}

(Read(_), BorrowKind::Shared | BorrowKind::Fake(_))
| (
Read(ReadKind::Borrow(BorrowKind::Fake(FakeBorrowKind::Shallow))),
BorrowKind::Mut { .. },
) => Control::Continue,
) => ControlFlow::Continue(()),

(Reservation(_), BorrowKind::Fake(_) | BorrowKind::Shared) => {
// This used to be a future compatibility warning (to be
// disallowed on NLL). See rust-lang/rust#56254
Control::Continue
ControlFlow::Continue(())
}

(Write(WriteKind::Move), BorrowKind::Fake(FakeBorrowKind::Shallow)) => {
// Handled by initialization checks.
Control::Continue
ControlFlow::Continue(())
}

(Read(kind), BorrowKind::Mut { .. }) => {
// Reading from mere reservations of mutable-borrows is OK.
if !is_active(this.dominators(), borrow, location) {
assert!(allow_two_phase_borrow(borrow.kind));
return Control::Continue;
assert!(borrow.kind.allows_two_phase_borrow());
return ControlFlow::Continue(());
}

error_reported = true;
@@ -1094,7 +1093,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
this.buffer_error(err);
}
}
Control::Break
ControlFlow::Break(())
}

(Reservation(kind) | Activation(kind, _) | Write(kind), _) => {
@@ -1141,7 +1140,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
this.report_illegal_mutation_of_borrowed(location, place_span, borrow)
}
}
Control::Break
ControlFlow::Break(())
}
},
);
@@ -1185,7 +1184,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
}
BorrowKind::Mut { .. } => {
let wk = WriteKind::MutableBorrow(bk);
if allow_two_phase_borrow(bk) {
if bk.allows_two_phase_borrow() {
(Deep, Reservation(wk))
} else {
(Deep, Write(wk))
6 changes: 3 additions & 3 deletions compiler/rustc_borrowck/src/nll.rs
Original file line number Diff line number Diff line change
@@ -142,9 +142,9 @@ pub(crate) fn compute_regions<'a, 'tcx>(

// If requested for `-Zpolonius=next`, convert NLL constraints to localized outlives
// constraints.
let localized_outlives_constraints = polonius_context
.as_mut()
.map(|polonius_context| polonius_context.create_localized_constraints(&mut regioncx, body));
let localized_outlives_constraints = polonius_context.as_mut().map(|polonius_context| {
polonius_context.create_localized_constraints(infcx.tcx, &regioncx, body)
});

// If requested: dump NLL facts, and run legacy polonius analysis.
let polonius_output = all_facts.as_ref().and_then(|all_facts| {
22 changes: 5 additions & 17 deletions compiler/rustc_borrowck/src/path_utils.rs
Original file line number Diff line number Diff line change
@@ -1,26 +1,14 @@
use std::ops::ControlFlow;

use rustc_abi::FieldIdx;
use rustc_data_structures::graph::dominators::Dominators;
use rustc_middle::mir::{BasicBlock, Body, BorrowKind, Location, Place, PlaceRef, ProjectionElem};
use rustc_middle::mir::{BasicBlock, Body, Location, Place, PlaceRef, ProjectionElem};
use rustc_middle::ty::TyCtxt;
use tracing::debug;

use crate::borrow_set::{BorrowData, BorrowSet, TwoPhaseActivation};
use crate::{AccessDepth, BorrowIndex, places_conflict};

/// Returns `true` if the borrow represented by `kind` is
/// allowed to be split into separate Reservation and
/// Activation phases.
pub(super) fn allow_two_phase_borrow(kind: BorrowKind) -> bool {
kind.allows_two_phase_borrow()
}

/// Control for the path borrow checking code
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub(super) enum Control {
Continue,
Break,
}

/// Encapsulates the idea of iterating over every borrow that involves a particular path
pub(super) fn each_borrow_involving_path<'tcx, F, I, S>(
s: &mut S,
@@ -31,7 +19,7 @@ pub(super) fn each_borrow_involving_path<'tcx, F, I, S>(
is_candidate: I,
mut op: F,
) where
F: FnMut(&mut S, BorrowIndex, &BorrowData<'tcx>) -> Control,
F: FnMut(&mut S, BorrowIndex, &BorrowData<'tcx>) -> ControlFlow<()>,
I: Fn(BorrowIndex) -> bool,
{
let (access, place) = access_place;
@@ -62,7 +50,7 @@ pub(super) fn each_borrow_involving_path<'tcx, F, I, S>(
i, borrowed, place, access
);
let ctrl = op(s, i, borrowed);
if ctrl == Control::Break {
if matches!(ctrl, ControlFlow::Break(_)) {
return;
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::ops::ControlFlow;

use rustc_data_structures::graph::dominators::Dominators;
use rustc_middle::bug;
use rustc_middle::mir::visit::Visitor;
@@ -260,7 +262,7 @@ impl<'a, 'tcx> LoanInvalidationsGenerator<'a, 'tcx> {
}
BorrowKind::Mut { .. } => {
let wk = WriteKind::MutableBorrow(bk);
if allow_two_phase_borrow(bk) {
if bk.allows_two_phase_borrow() {
(Deep, Reservation(wk))
} else {
(Deep, Write(wk))
@@ -378,8 +380,8 @@ impl<'a, 'tcx> LoanInvalidationsGenerator<'a, 'tcx> {
// Reading from mere reservations of mutable-borrows is OK.
if !is_active(this.dominators, borrow, location) {
// If the borrow isn't active yet, reads don't invalidate it
assert!(allow_two_phase_borrow(borrow.kind));
return Control::Continue;
assert!(borrow.kind.allows_two_phase_borrow());
return ControlFlow::Continue(());
}

// Unique and mutable borrows are invalidated by reads from any
@@ -395,7 +397,7 @@ impl<'a, 'tcx> LoanInvalidationsGenerator<'a, 'tcx> {
this.emit_loan_invalidated_at(borrow_index, location);
}
}
Control::Continue
ControlFlow::Continue(())
},
);
}
47 changes: 7 additions & 40 deletions compiler/rustc_borrowck/src/polonius/mod.rs
Original file line number Diff line number Diff line change
@@ -37,21 +37,20 @@ mod constraints;
mod dump;
pub(crate) mod legacy;
mod liveness_constraints;
mod typeck_constraints;

use std::collections::BTreeMap;

use rustc_index::bit_set::SparseBitMatrix;
use rustc_middle::mir::{Body, Location};
use rustc_middle::ty::RegionVid;
use rustc_middle::mir::Body;
use rustc_middle::ty::{RegionVid, TyCtxt};
use rustc_mir_dataflow::points::PointIndex;

pub(crate) use self::constraints::*;
pub(crate) use self::dump::dump_polonius_mir;
use self::liveness_constraints::create_liveness_constraints;
use self::typeck_constraints::convert_typeck_constraints;
use crate::RegionInferenceContext;
use crate::constraints::OutlivesConstraint;
use crate::region_infer::values::LivenessValues;
use crate::type_check::Locations;

/// This struct holds the data needed to create the Polonius localized constraints.
pub(crate) struct PoloniusContext {
@@ -88,14 +87,17 @@ impl PoloniusContext {
/// - encoding liveness constraints
pub(crate) fn create_localized_constraints<'tcx>(
&self,
tcx: TyCtxt<'tcx>,
regioncx: &RegionInferenceContext<'tcx>,
body: &Body<'tcx>,
) -> LocalizedOutlivesConstraintSet {
let mut localized_outlives_constraints = LocalizedOutlivesConstraintSet::default();
convert_typeck_constraints(
tcx,
body,
regioncx.liveness_constraints(),
regioncx.outlives_constraints(),
regioncx.universal_regions(),
&mut localized_outlives_constraints,
);

@@ -117,38 +119,3 @@ impl PoloniusContext {
localized_outlives_constraints
}
}

/// Propagate loans throughout the subset graph at a given point (with some subtleties around the
/// location where effects start to be visible).
fn convert_typeck_constraints<'tcx>(
body: &Body<'tcx>,
liveness: &LivenessValues,
outlives_constraints: impl Iterator<Item = OutlivesConstraint<'tcx>>,
localized_outlives_constraints: &mut LocalizedOutlivesConstraintSet,
) {
for outlives_constraint in outlives_constraints {
match outlives_constraint.locations {
Locations::All(_) => {
// For now, turn logical constraints holding at all points into physical edges at
// every point in the graph.
// FIXME: encode this into *traversal* instead.
for (block, bb) in body.basic_blocks.iter_enumerated() {
let statement_count = bb.statements.len();
for statement_index in 0..=statement_count {
let current_location = Location { block, statement_index };
let current_point = liveness.point_from_location(current_location);

localized_outlives_constraints.push(LocalizedOutlivesConstraint {
source: outlives_constraint.sup,
from: current_point,
target: outlives_constraint.sub,
to: current_point,
});
}
}
}

_ => {}
}
}
}
241 changes: 241 additions & 0 deletions compiler/rustc_borrowck/src/polonius/typeck_constraints.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
use rustc_data_structures::fx::FxHashSet;
use rustc_middle::mir::{Body, Location, Statement, StatementKind, Terminator, TerminatorKind};
use rustc_middle::ty::{TyCtxt, TypeVisitable};
use rustc_mir_dataflow::points::PointIndex;

use super::{LocalizedOutlivesConstraint, LocalizedOutlivesConstraintSet};
use crate::constraints::OutlivesConstraint;
use crate::region_infer::values::LivenessValues;
use crate::type_check::Locations;
use crate::universal_regions::UniversalRegions;

/// Propagate loans throughout the subset graph at a given point (with some subtleties around the
/// location where effects start to be visible).
pub(super) fn convert_typeck_constraints<'tcx>(
tcx: TyCtxt<'tcx>,
body: &Body<'tcx>,
liveness: &LivenessValues,
outlives_constraints: impl Iterator<Item = OutlivesConstraint<'tcx>>,
universal_regions: &UniversalRegions<'tcx>,
localized_outlives_constraints: &mut LocalizedOutlivesConstraintSet,
) {
for outlives_constraint in outlives_constraints {
match outlives_constraint.locations {
Locations::All(_) => {
// For now, turn logical constraints holding at all points into physical edges at
// every point in the graph.
// FIXME: encode this into *traversal* instead.
for (block, bb) in body.basic_blocks.iter_enumerated() {
let statement_count = bb.statements.len();
for statement_index in 0..=statement_count {
let current_location = Location { block, statement_index };
let current_point = liveness.point_from_location(current_location);

localized_outlives_constraints.push(LocalizedOutlivesConstraint {
source: outlives_constraint.sup,
from: current_point,
target: outlives_constraint.sub,
to: current_point,
});
}
}
}

Locations::Single(location) => {
// This constraint is marked as holding at one location, we localize it to that
// location or its successor, depending on the corresponding MIR
// statement/terminator. Unfortunately, they all show up from typeck as coming "on
// entry", so for now we modify them to take effects that should apply "on exit"
// into account.
//
// FIXME: this approach is subtle, complicated, and hard to test, so we should track
// this information better in MIR typeck instead, for example with a new `Locations`
// variant that contains which node is crossing over between entry and exit.
let point = liveness.point_from_location(location);
let localized_constraint = if let Some(stmt) =
body[location.block].statements.get(location.statement_index)
{
localize_statement_constraint(
tcx,
body,
stmt,
liveness,
&outlives_constraint,
location,
point,
universal_regions,
)
} else {
assert_eq!(location.statement_index, body[location.block].statements.len());
let terminator = body[location.block].terminator();
localize_terminator_constraint(
tcx,
body,
terminator,
liveness,
&outlives_constraint,
point,
universal_regions,
)
};
localized_outlives_constraints.push(localized_constraint);
}
}
}
}

/// For a given outlives constraint arising from a MIR statement, localize the constraint with the
/// needed CFG `from`-`to` intra-block nodes.
fn localize_statement_constraint<'tcx>(
tcx: TyCtxt<'tcx>,
body: &Body<'tcx>,
stmt: &Statement<'tcx>,
liveness: &LivenessValues,
outlives_constraint: &OutlivesConstraint<'tcx>,
current_location: Location,
current_point: PointIndex,
universal_regions: &UniversalRegions<'tcx>,
) -> LocalizedOutlivesConstraint {
match &stmt.kind {
StatementKind::Assign(box (lhs, rhs)) => {
// To create localized outlives constraints without midpoints, we rely on the property
// that no input regions from the RHS of the assignment will flow into themselves: they
// should not appear in the output regions in the LHS. We believe this to be true by
// construction of the MIR, via temporaries, and assert it here.
//
// We think we don't need midpoints because:
// - every LHS Place has a unique set of regions that don't appear elsewhere
// - this implies that for them to be part of the RHS, the same Place must be read and
// written
// - and that should be impossible in MIR
//
// When we have a more complete implementation in the future, tested with crater, etc,
// we can relax this to a debug assert instead, or remove it.
assert!(
{
let mut lhs_regions = FxHashSet::default();
tcx.for_each_free_region(lhs, |region| {
let region = universal_regions.to_region_vid(region);
lhs_regions.insert(region);
});

let mut rhs_regions = FxHashSet::default();
tcx.for_each_free_region(rhs, |region| {
let region = universal_regions.to_region_vid(region);
rhs_regions.insert(region);
});

// The intersection between LHS and RHS regions should be empty.
lhs_regions.is_disjoint(&rhs_regions)
},
"there should be no common regions between the LHS and RHS of an assignment"
);

// As mentioned earlier, we should be tracking these better upstream but: we want to
// relate the types on entry to the type of the place on exit. That is, outlives
// constraints on the RHS are on entry, and outlives constraints to/from the LHS are on
// exit (i.e. on entry to the successor location).
let lhs_ty = body.local_decls[lhs.local].ty;
let successor_location = Location {
block: current_location.block,
statement_index: current_location.statement_index + 1,
};
let successor_point = liveness.point_from_location(successor_location);
compute_constraint_direction(
tcx,
outlives_constraint,
&lhs_ty,
current_point,
successor_point,
universal_regions,
)
}
_ => {
// For the other cases, we localize an outlives constraint to where it arises.
LocalizedOutlivesConstraint {
source: outlives_constraint.sup,
from: current_point,
target: outlives_constraint.sub,
to: current_point,
}
}
}
}

/// For a given outlives constraint arising from a MIR terminator, localize the constraint with the
/// needed CFG `from`-`to` inter-block nodes.
fn localize_terminator_constraint<'tcx>(
tcx: TyCtxt<'tcx>,
body: &Body<'tcx>,
terminator: &Terminator<'tcx>,
liveness: &LivenessValues,
outlives_constraint: &OutlivesConstraint<'tcx>,
current_point: PointIndex,
universal_regions: &UniversalRegions<'tcx>,
) -> LocalizedOutlivesConstraint {
// FIXME: check if other terminators need the same handling as `Call`s, in particular
// Assert/Yield/Drop. A handful of tests are failing with Drop related issues, as well as some
// coroutine tests, and that may be why.
match &terminator.kind {
// FIXME: also handle diverging calls.
TerminatorKind::Call { destination, target: Some(target), .. } => {
// Calls are similar to assignments, and thus follow the same pattern. If there is a
// target for the call we also relate what flows into the destination here to entry to
// that successor.
let destination_ty = destination.ty(&body.local_decls, tcx);
let successor_location = Location { block: *target, statement_index: 0 };
let successor_point = liveness.point_from_location(successor_location);
compute_constraint_direction(
tcx,
outlives_constraint,
&destination_ty,
current_point,
successor_point,
universal_regions,
)
}
_ => {
// Typeck constraints guide loans between regions at the current point, so we do that in
// the general case, and liveness will take care of making them flow to the terminator's
// successors.
LocalizedOutlivesConstraint {
source: outlives_constraint.sup,
from: current_point,
target: outlives_constraint.sub,
to: current_point,
}
}
}
}
/// For a given outlives constraint and CFG edge, returns the localized constraint with the
/// appropriate `from`-`to` direction. This is computed according to whether the constraint flows to
/// or from a free region in the given `value`, some kind of result for an effectful operation, like
/// the LHS of an assignment.
fn compute_constraint_direction<'tcx>(
tcx: TyCtxt<'tcx>,
outlives_constraint: &OutlivesConstraint<'tcx>,
value: &impl TypeVisitable<TyCtxt<'tcx>>,
current_point: PointIndex,
successor_point: PointIndex,
universal_regions: &UniversalRegions<'tcx>,
) -> LocalizedOutlivesConstraint {
let mut to = current_point;
let mut from = current_point;
tcx.for_each_free_region(value, |region| {
let region = universal_regions.to_region_vid(region);
if region == outlives_constraint.sub {
// This constraint flows into the result, its effects start becoming visible on exit.
to = successor_point;
} else if region == outlives_constraint.sup {
// This constraint flows from the result, its effects start becoming visible on exit.
from = successor_point;
}
});
Comment on lines +224 to +233
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is right, but it feels off to me. The loop using for_each_free_region feels like the wrong construct to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also prefer if this differentiation was done in MIR typeck instead, if that's what you mean. It wasn't trivial in the NLL days, according to Niko and some comments in the crate, but should still be possible I think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think given the FIXME further up in the file, we don't need to do anything in this PR, just wanted to point out that it sure does feel weird.


LocalizedOutlivesConstraint {
source: outlives_constraint.sup,
from,
target: outlives_constraint.sub,
to,
}
}
35 changes: 0 additions & 35 deletions compiler/rustc_borrowck/src/util/collect_writes.rs

This file was deleted.

3 changes: 0 additions & 3 deletions compiler/rustc_borrowck/src/util/mod.rs

This file was deleted.

2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/mir/statement.rs
Original file line number Diff line number Diff line change
@@ -455,6 +455,8 @@ impl BorrowKind {
}
}

/// Returns whether borrows represented by this kind are allowed to be split into separate
/// Reservation and Activation phases.
pub fn allows_two_phase_borrow(&self) -> bool {
match *self {
BorrowKind::Shared