Skip to content

cleanup mir_borrowck #139666

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 10 commits into from
Apr 14, 2025
147 changes: 63 additions & 84 deletions compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use std::cell::RefCell;
use std::marker::PhantomData;
use std::ops::{ControlFlow, Deref};

use borrow_set::LocalsStateAtExit;
use root_cx::BorrowCheckRootCtxt;
use rustc_abi::FieldIdx;
use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
Expand Down Expand Up @@ -304,33 +305,13 @@ fn do_mir_borrowck<'tcx>(
root_cx.set_tainted_by_errors(e);
}

let mut local_names = IndexVec::from_elem(None, &input_body.local_decls);
for var_debug_info in &input_body.var_debug_info {
if let VarDebugInfoContents::Place(place) = var_debug_info.value {
if let Some(local) = place.as_local() {
if let Some(prev_name) = local_names[local]
&& var_debug_info.name != prev_name
{
span_bug!(
var_debug_info.source_info.span,
"local {:?} has many names (`{}` vs `{}`)",
local,
prev_name,
var_debug_info.name
);
}
local_names[local] = Some(var_debug_info.name);
}
}
}

// Replace all regions with fresh inference variables. This
// requires first making our own copy of the MIR. This copy will
// be modified (in place) to contain non-lexical lifetimes. It
// will have a lifetime tied to the inference context.
let mut body_owned = input_body.clone();
let mut promoted = input_promoted.to_owned();
let free_regions = nll::replace_regions_in_mir(&infcx, &mut body_owned, &mut promoted);
let universal_regions = nll::replace_regions_in_mir(&infcx, &mut body_owned, &mut promoted);
let body = &body_owned; // no further changes

let location_table = PoloniusLocationTable::new(body);
Expand All @@ -355,7 +336,7 @@ fn do_mir_borrowck<'tcx>(
} = nll::compute_regions(
root_cx,
&infcx,
free_regions,
universal_regions,
body,
&promoted,
&location_table,
Expand All @@ -368,24 +349,23 @@ fn do_mir_borrowck<'tcx>(
// Dump MIR results into a file, if that is enabled. This lets us
// write unit-tests, as well as helping with debugging.
nll::dump_nll_mir(&infcx, body, &regioncx, &opt_closure_req, &borrow_set);
polonius::dump_polonius_mir(
&infcx,
body,
&regioncx,
&opt_closure_req,
&borrow_set,
polonius_diagnostics.as_ref(),
);

// We also have a `#[rustc_regions]` annotation that causes us to dump
// information.
let diags_buffer = &mut BorrowckDiagnosticsBuffer::default();
nll::dump_annotation(&infcx, body, &regioncx, &opt_closure_req, diags_buffer);

let movable_coroutine =
// The first argument is the coroutine type passed by value
if let Some(local) = body.local_decls.raw.get(1)
// Get the interior types and args which typeck computed
&& let ty::Coroutine(def_id, _) = *local.ty.kind()
&& tcx.coroutine_movability(def_id) == hir::Movability::Movable
{
true
} else {
false
};
nll::dump_annotation(&infcx, body, &regioncx, &opt_closure_req);

let movable_coroutine = body.coroutine.is_some()
&& tcx.coroutine_movability(def.to_def_id()) == hir::Movability::Movable;

let diags_buffer = &mut BorrowckDiagnosticsBuffer::default();
// While promoteds should mostly be correct by construction, we need to check them for
// invalid moves to detect moving out of arrays:`struct S; fn main() { &([S][0]); }`.
for promoted_body in &promoted {
Expand All @@ -403,7 +383,6 @@ fn do_mir_borrowck<'tcx>(
location_table: &location_table,
movable_coroutine,
fn_self_span_reported: Default::default(),
locals_are_invalidated_at_exit,
access_place_error_reported: Default::default(),
reservation_error_reported: Default::default(),
uninitialized_error_reported: Default::default(),
Expand Down Expand Up @@ -435,14 +414,33 @@ fn do_mir_borrowck<'tcx>(
promoted_mbcx.report_move_errors();
}

let mut local_names = IndexVec::from_elem(None, &body.local_decls);
for var_debug_info in &body.var_debug_info {
if let VarDebugInfoContents::Place(place) = var_debug_info.value {
if let Some(local) = place.as_local() {
if let Some(prev_name) = local_names[local]
&& var_debug_info.name != prev_name
{
span_bug!(
var_debug_info.source_info.span,
"local {:?} has many names (`{}` vs `{}`)",
local,
prev_name,
var_debug_info.name
);
}
local_names[local] = Some(var_debug_info.name);
}
}
}

let mut mbcx = MirBorrowckCtxt {
root_cx,
infcx: &infcx,
body,
move_data: &move_data,
location_table: &location_table,
movable_coroutine,
locals_are_invalidated_at_exit,
fn_self_span_reported: Default::default(),
access_place_error_reported: Default::default(),
reservation_error_reported: Default::default(),
Expand All @@ -455,9 +453,9 @@ fn do_mir_borrowck<'tcx>(
local_names,
region_names: RefCell::default(),
next_region_name: RefCell::new(1),
polonius_output,
move_errors: Vec::new(),
diags_buffer,
polonius_output: polonius_output.as_deref(),
polonius_diagnostics: polonius_diagnostics.as_ref(),
};

Expand All @@ -474,16 +472,6 @@ fn do_mir_borrowck<'tcx>(

mbcx.report_move_errors();

// If requested, dump polonius MIR.
polonius::dump_polonius_mir(
Copy link
Member

Choose a reason for hiding this comment

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

I put the call here because the dump will use the dataflow results just above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as in, moving this is wrong?

it does not access flow_results or the mbcx, so how does the code above impact this output 🤔

Copy link
Member

@lqd lqd Apr 14, 2025

Choose a reason for hiding this comment

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

It doesn’t on master right now but will, when I open the PR to add the full Polonius debugger to its MIR dump: it shows the loans going out of NLL scope for example.

No worries, I will move it back if it needs to, but I also don’t know if I’ll open that PR before needing to rework active loans away from these dataflow results, so it’s fine.

&infcx,
body,
&regioncx,
&borrow_set,
polonius_diagnostics.as_ref(),
&opt_closure_req,
);

// For each non-user used mutable variable, check if it's been assigned from
// a user-declared local. If so, then put that local into the used_mut set.
// Note that this set is expected to be small - only upvars from closures
Expand Down Expand Up @@ -514,15 +502,14 @@ fn do_mir_borrowck<'tcx>(
};

let body_with_facts = if consumer_options.is_some() {
let output_facts = mbcx.polonius_output;
Some(Box::new(BodyWithBorrowckFacts {
body: body_owned,
promoted,
borrow_set,
region_inference_context: regioncx,
location_table: polonius_input.as_ref().map(|_| location_table),
input_facts: polonius_input,
output_facts,
output_facts: polonius_output,
}))
} else {
None
Expand Down Expand Up @@ -655,13 +642,6 @@ struct MirBorrowckCtxt<'a, 'infcx, 'tcx> {
location_table: &'a PoloniusLocationTable,

movable_coroutine: bool,
/// This keeps track of whether local variables are free-ed when the function
/// exits even without a `StorageDead`, which appears to be the case for
/// constants.
///
/// I'm not sure this is the right approach - @eddyb could you try and
/// figure this out?
locals_are_invalidated_at_exit: bool,
/// This field keeps track of when borrow errors are reported in the access_place function
/// so that there is no duplicate reporting. This field cannot also be used for the conflicting
/// borrow errors that is handled by the `reservation_error_reported` field as the inclusion
Expand Down Expand Up @@ -709,12 +689,11 @@ struct MirBorrowckCtxt<'a, 'infcx, 'tcx> {
/// The counter for generating new region names.
next_region_name: RefCell<usize>,

/// Results of Polonius analysis.
polonius_output: Option<Box<PoloniusOutput>>,

diags_buffer: &'a mut BorrowckDiagnosticsBuffer<'infcx, 'tcx>,
move_errors: Vec<MoveError<'tcx>>,

/// Results of Polonius analysis.
polonius_output: Option<&'a PoloniusOutput>,
/// When using `-Zpolonius=next`: the data used to compute errors and diagnostics.
polonius_diagnostics: Option<&'a PoloniusDiagnosticsContext>,
}
Expand Down Expand Up @@ -938,13 +917,20 @@ impl<'a, 'tcx> ResultsVisitor<'a, 'tcx, Borrowck<'a, 'tcx>> for MirBorrowckCtxt<
| TerminatorKind::Return
| TerminatorKind::TailCall { .. }
| TerminatorKind::CoroutineDrop => {
// Returning from the function implicitly kills storage for all locals and statics.
// Often, the storage will already have been killed by an explicit
// StorageDead, but we don't always emit those (notably on unwind paths),
// so this "extra check" serves as a kind of backup.
for i in state.borrows.iter() {
let borrow = &self.borrow_set[i];
self.check_for_invalidation_at_exit(loc, borrow, span);
match self.borrow_set.locals_state_at_exit() {
LocalsStateAtExit::AllAreInvalidated => {
// Returning from the function implicitly kills storage for all locals and statics.
// Often, the storage will already have been killed by an explicit
// StorageDead, but we don't always emit those (notably on unwind paths),
// so this "extra check" serves as a kind of backup.
for i in state.borrows.iter() {
let borrow = &self.borrow_set[i];
self.check_for_invalidation_at_exit(loc, borrow, span);
}
}
// If we do not implicitly invalidate all locals on exit,
// we check for conflicts when dropping or moving this local.
LocalsStateAtExit::SomeAreInvalidated { has_storage_dead_or_moved: _ } => {}
}
}

Expand Down Expand Up @@ -1716,22 +1702,15 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
// we'll have a memory leak) and assume that all statics have a destructor.
//
// FIXME: allow thread-locals to borrow other thread locals?

let (might_be_alive, will_be_dropped) =
if self.body.local_decls[root_place.local].is_ref_to_thread_local() {
// Thread-locals might be dropped after the function exits
// We have to dereference the outer reference because
// borrows don't conflict behind shared references.
root_place.projection = TyCtxtConsts::DEREF_PROJECTION;
(true, true)
} else {
(false, self.locals_are_invalidated_at_exit)
};

if !will_be_dropped {
debug!("place_is_invalidated_at_exit({:?}) - won't be dropped", place);
return;
}
let might_be_alive = if self.body.local_decls[root_place.local].is_ref_to_thread_local() {
// Thread-locals might be dropped after the function exits
// We have to dereference the outer reference because
// borrows don't conflict behind shared references.
root_place.projection = TyCtxtConsts::DEREF_PROJECTION;
true
} else {
false
};

let sd = if might_be_alive { Deep } else { Shallow(None) };

Expand Down
21 changes: 4 additions & 17 deletions compiler/rustc_borrowck/src/nll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use tracing::{debug, instrument};

use crate::borrow_set::BorrowSet;
use crate::consumers::ConsumerOptions;
use crate::diagnostics::{BorrowckDiagnosticsBuffer, RegionErrors};
use crate::diagnostics::RegionErrors;
use crate::polonius::PoloniusDiagnosticsContext;
use crate::polonius::legacy::{
PoloniusFacts, PoloniusFactsExt, PoloniusLocationTable, PoloniusOutput,
Expand Down Expand Up @@ -117,11 +117,6 @@ pub(crate) fn compute_regions<'a, 'tcx>(
Rc::clone(&location_map),
);

// Create the region inference context, taking ownership of the
// region inference data that was contained in `infcx`, and the
// base constraints generated by the type-check.
let var_infos = infcx.get_region_var_infos();

// If requested, emit legacy polonius facts.
polonius::legacy::emit_facts(
&mut polonius_facts,
Expand All @@ -134,13 +129,8 @@ pub(crate) fn compute_regions<'a, 'tcx>(
&constraints,
);

let mut regioncx = RegionInferenceContext::new(
infcx,
var_infos,
constraints,
universal_region_relations,
location_map,
);
let mut regioncx =
RegionInferenceContext::new(infcx, constraints, universal_region_relations, location_map);

// If requested for `-Zpolonius=next`, convert NLL constraints to localized outlives constraints
// and use them to compute loan liveness.
Expand Down Expand Up @@ -297,7 +287,6 @@ pub(super) fn dump_annotation<'tcx, 'infcx>(
body: &Body<'tcx>,
regioncx: &RegionInferenceContext<'tcx>,
closure_region_requirements: &Option<ClosureRegionRequirements<'tcx>>,
diagnostics_buffer: &mut BorrowckDiagnosticsBuffer<'infcx, 'tcx>,
) {
let tcx = infcx.tcx;
let base_def_id = tcx.typeck_root_def_id(body.source.def_id());
Expand Down Expand Up @@ -335,13 +324,11 @@ pub(super) fn dump_annotation<'tcx, 'infcx>(
} else {
let mut err = infcx.dcx().struct_span_note(def_span, "no external requirements");
regioncx.annotate(tcx, &mut err);

err
};

// FIXME(@lcnr): We currently don't dump the inferred hidden types here.

diagnostics_buffer.buffer_non_error(err);
err.emit();
}

fn for_each_region_constraint<'tcx>(
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/polonius/dump.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ pub(crate) fn dump_polonius_mir<'tcx>(
infcx: &BorrowckInferCtxt<'tcx>,
body: &Body<'tcx>,
regioncx: &RegionInferenceContext<'tcx>,
closure_region_requirements: &Option<ClosureRegionRequirements<'tcx>>,
borrow_set: &BorrowSet<'tcx>,
polonius_diagnostics: Option<&PoloniusDiagnosticsContext>,
closure_region_requirements: &Option<ClosureRegionRequirements<'tcx>>,
) {
let tcx = infcx.tcx;
if !tcx.sess.opts.unstable_opts.polonius.is_next_enabled() {
Expand Down
Loading
Loading