Skip to content

Commit

Permalink
Auto merge of #126023 - amandasystems:you-dropped-this-again, r=nikom…
Browse files Browse the repository at this point in the history
…atsakis

Remove confusing `use_polonius` flag and do less cloning

The `use_polonius` flag is both redundant and confusing since every function it's propagated to also checks if `all_facts` is `Some`, the true test of whether to generate Polonius facts for Polonius or for external consumers. This PR makes that path clearer by simply doing away with the argument and handling the logic in precisely two places: where facts are populated (check for `Some`), and where `all_facts` are initialised. It also delays some statements until after that check to avoid the miniscule performance penalty of executing them when Polonius is disabled.

This also addresses `@lqd's` concern in #125652 by reducing the size of what is cloned out of Polonius facts to just the facts being added, as opposed to the entire vector of potential inputs, and added descriptive comments.

*Reviewer note*: the comments in `add_extra_drop_facts` should be inspected by a reviewer, in particular the one on [L#259](https://github.com/rust-lang/rust/compare/master...amandasystems:you-dropped-this-again?expand=1#diff-aa727290e6670264df2face84f012897878e11a70e9c8b156543cfcd9619bac3R259) in this PR, which should be trivial for someone with the right background knowledge to address.

I also included some lints I found on the way there that I couldn't help myself from addressing.
  • Loading branch information
bors committed Jun 24, 2024
2 parents bcf94de + 7b5b7a7 commit d49994b
Show file tree
Hide file tree
Showing 13 changed files with 57 additions and 51 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/borrow_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ impl<'tcx> BorrowSet<'tcx> {
) -> Self {
let mut visitor = GatherBorrows {
tcx,
body: body,
body,
location_map: Default::default(),
activation_map: Default::default(),
local_map: Default::default(),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/borrowck_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
via(msg_old),
);

if msg_new == "" {
if msg_new.is_empty() {
// If `msg_new` is empty, then this isn't a borrow of a union field.
err.span_label(span, format!("{kind_new} borrow occurs here"));
err.span_label(old_span, format!("{kind_old} borrow occurs here"));
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_borrowck/src/dataflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ impl<'mir, 'tcx> ResultsVisitable<'tcx> for BorrowckResults<'mir, 'tcx> {
}

fn reset_to_block_entry(&self, state: &mut Self::FlowState, block: BasicBlock) {
state.borrows.clone_from(&self.borrows.entry_set_for_block(block));
state.uninits.clone_from(&self.uninits.entry_set_for_block(block));
state.ever_inits.clone_from(&self.ever_inits.entry_set_for_block(block));
state.borrows.clone_from(self.borrows.entry_set_for_block(block));
state.uninits.clone_from(self.uninits.entry_set_for_block(block));
state.ever_inits.clone_from(self.ever_inits.entry_set_for_block(block));
}

fn reconstruct_before_statement_effect(
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/region_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,6 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
for alias_ty in alias_tys {
if alias_ty.span.desugaring_kind().is_some() {
// Skip `async` desugaring `impl Future`.
()
}
if let TyKind::TraitObject(_, lt, _) = alias_ty.kind {
if lt.ident.name == kw::Empty {
Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_borrowck/src/diagnostics/region_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
}

// Otherwise, let's descend into the referent types.
search_stack.push((*referent_ty, &referent_hir_ty.ty));
search_stack.push((*referent_ty, referent_hir_ty.ty));
}

// Match up something like `Foo<'1>`
Expand Down Expand Up @@ -558,7 +558,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
}

(ty::RawPtr(mut_ty, _), hir::TyKind::Ptr(mut_hir_ty)) => {
search_stack.push((*mut_ty, &mut_hir_ty.ty));
search_stack.push((*mut_ty, mut_hir_ty.ty));
}

_ => {
Expand Down Expand Up @@ -652,7 +652,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
let upvar_index = self.regioncx.get_upvar_index_for_region(self.infcx.tcx, fr)?;
let (upvar_name, upvar_span) = self.regioncx.get_upvar_name_and_span_for_region(
self.infcx.tcx,
&self.upvars,
self.upvars,
upvar_index,
);
let region_name = self.synthesize_region_name();
Expand Down Expand Up @@ -717,7 +717,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
.output;
span = output.span();
if let hir::FnRetTy::Return(ret) = output {
hir_ty = Some(self.get_future_inner_return_ty(*ret));
hir_ty = Some(self.get_future_inner_return_ty(ret));
}
" of async function"
}
Expand Down Expand Up @@ -958,7 +958,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
{
let (upvar_name, upvar_span) = self.regioncx.get_upvar_name_and_span_for_region(
self.infcx.tcx,
&self.upvars,
self.upvars,
upvar_index,
);
let region_name = self.synthesize_region_name();
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_borrowck/src/nll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ pub(crate) fn compute_regions<'cx, 'tcx>(
move_data,
elements,
upvars,
polonius_input,
);

// Create the region inference context, taking ownership of the
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_borrowck/src/polonius/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ pub(crate) fn emit_facts<'tcx>(
emit_universal_region_facts(
all_facts,
borrow_set,
&universal_regions,
&universal_region_relations,
universal_regions,
universal_region_relations,
);
emit_cfg_and_loan_kills_facts(all_facts, tcx, location_table, body, borrow_set);
emit_loan_invalidations_facts(all_facts, tcx, location_table, body, borrow_set);
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_borrowck/src/region_infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ fn sccs_info<'tcx>(infcx: &BorrowckInferCtxt<'tcx>, sccs: &ConstraintSccs) {

for (reg_var_idx, scc_idx) in sccs.scc_indices().iter().enumerate() {
let reg_var = ty::RegionVid::from_usize(reg_var_idx);
let origin = var_to_origin.get(&reg_var).unwrap_or_else(|| &RegionCtxt::Unknown);
let origin = var_to_origin.get(&reg_var).unwrap_or(&RegionCtxt::Unknown);
components[scc_idx.as_usize()].insert((reg_var, *origin));
}

Expand Down Expand Up @@ -2216,7 +2216,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
// #114907 where this happens via liveness and dropck outlives results.
// Therefore, we return a default value in case that happens, which should at worst emit a
// suboptimal error, instead of the ICE.
self.universe_causes.get(&universe).cloned().unwrap_or_else(|| UniverseInfo::other())
self.universe_causes.get(&universe).cloned().unwrap_or_else(UniverseInfo::other)
}

/// Tries to find the terminator of the loop in which the region 'r' resides.
Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_borrowck/src/region_infer/opaque_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,9 +418,7 @@ fn check_opaque_type_parameter_valid<'tcx>(
let opaque_param = opaque_generics.param_at(i, tcx);
let kind = opaque_param.kind.descr();

if let Err(guar) = opaque_env.param_is_error(i) {
return Err(guar);
}
opaque_env.param_is_error(i)?;

return Err(tcx.dcx().emit_err(NonGenericOpaqueTypeParam {
ty: arg,
Expand Down
10 changes: 2 additions & 8 deletions compiler/rustc_borrowck/src/type_check/liveness/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ use rustc_mir_dataflow::ResultsCursor;
use std::rc::Rc;

use crate::{
constraints::OutlivesConstraintSet,
facts::{AllFacts, AllFactsExt},
region_infer::values::LivenessValues,
constraints::OutlivesConstraintSet, region_infer::values::LivenessValues,
universal_regions::UniversalRegions,
};

Expand All @@ -38,7 +36,6 @@ pub(super) fn generate<'mir, 'tcx>(
elements: &Rc<DenseLocationMap>,
flow_inits: &mut ResultsCursor<'mir, 'tcx, MaybeInitializedPlaces<'mir, 'tcx>>,
move_data: &MoveData<'tcx>,
use_polonius: bool,
) {
debug!("liveness::generate");

Expand All @@ -49,11 +46,8 @@ pub(super) fn generate<'mir, 'tcx>(
);
let (relevant_live_locals, boring_locals) =
compute_relevant_live_locals(typeck.tcx(), &free_regions, body);
let facts_enabled = use_polonius || AllFacts::enabled(typeck.tcx());

if facts_enabled {
polonius::populate_access_facts(typeck, body, move_data);
};
polonius::populate_access_facts(typeck, body, move_data);

trace::trace(
typeck,
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_borrowck/src/type_check/liveness/polonius.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,10 @@ pub(super) fn populate_access_facts<'a, 'tcx>(
body: &Body<'tcx>,
move_data: &MoveData<'tcx>,
) {
debug!("populate_access_facts()");
let location_table = typeck.borrowck_context.location_table;

if let Some(facts) = typeck.borrowck_context.all_facts.as_mut() {
debug!("populate_access_facts()");
let location_table = typeck.borrowck_context.location_table;

let mut extractor = UseFactsExtractor {
var_defined_at: &mut facts.var_defined_at,
var_used_at: &mut facts.var_used_at,
Expand Down
55 changes: 36 additions & 19 deletions compiler/rustc_borrowck/src/type_check/liveness/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,35 +217,52 @@ impl<'me, 'typeck, 'flow, 'tcx> LivenessResults<'me, 'typeck, 'flow, 'tcx> {
/// Add facts for all locals with free regions, since regions may outlive
/// the function body only at certain nodes in the CFG.
fn add_extra_drop_facts(&mut self, relevant_live_locals: &[Local]) -> Option<()> {
let drop_used = self
.cx
.typeck
.borrowck_context
.all_facts
.as_ref()
.map(|facts| facts.var_dropped_at.clone())?;
// This collect is more necessary than immediately apparent
// because these facts go into `add_drop_live_facts_for()`,
// which also writes to `all_facts`, and so this is genuinely
// a simulatneous overlapping mutable borrow.
// FIXME for future hackers: investigate whether this is
// actually necessary; these facts come from Polonius
// and probably maybe plausibly does not need to go back in.
// It may be necessary to just pick out the parts of
// `add_drop_live_facts_for()` that make sense.
let facts_to_add: Vec<_> = {
let drop_used = &self.cx.typeck.borrowck_context.all_facts.as_ref()?.var_dropped_at;

let relevant_live_locals: FxIndexSet<_> =
relevant_live_locals.iter().copied().collect();

drop_used
.iter()
.filter_map(|(local, location_index)| {
let local_ty = self.cx.body.local_decls[*local].ty;
if relevant_live_locals.contains(local) || !local_ty.has_free_regions() {
return None;
}

let relevant_live_locals: FxIndexSet<_> = relevant_live_locals.iter().copied().collect();

let locations = IntervalSet::new(self.cx.elements.num_points());

for (local, location_index) in drop_used {
if !relevant_live_locals.contains(&local) {
let local_ty = self.cx.body.local_decls[local].ty;
if local_ty.has_free_regions() {
let location = match self
.cx
.typeck
.borrowck_context
.location_table
.to_location(location_index)
.to_location(*location_index)
{
RichLocation::Start(l) => l,
RichLocation::Mid(l) => l,
};
self.cx.add_drop_live_facts_for(local, local_ty, &[location], &locations);
}
}

Some((*local, local_ty, location))
})
.collect()
};

// FIXME: these locations seem to have a special meaning (e.g. everywhere, at the end, ...), but I don't know which one. Please help me rename it to something descriptive!
// Also, if this IntervalSet is used in many places, it maybe should have a newtype'd
// name with a description of what it means for future mortals passing by.
let locations = IntervalSet::new(self.cx.elements.num_points());

for (local, local_ty, location) in facts_to_add {
self.cx.add_drop_live_facts_for(local, local_ty, &[location], &locations);
}
Some(())
}
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ pub(crate) fn type_check<'mir, 'tcx>(
move_data: &MoveData<'tcx>,
elements: &Rc<DenseLocationMap>,
upvars: &[&ty::CapturedPlace<'tcx>],
use_polonius: bool,
) -> MirTypeckResults<'tcx> {
let implicit_region_bound = ty::Region::new_var(infcx.tcx, universal_regions.fr_fn_body);
let mut constraints = MirTypeckRegionConstraints {
Expand Down Expand Up @@ -189,7 +188,7 @@ pub(crate) fn type_check<'mir, 'tcx>(
checker.equate_inputs_and_outputs(body, universal_regions, &normalized_inputs_and_output);
checker.check_signature_annotation(body);

liveness::generate(&mut checker, body, elements, flow_inits, move_data, use_polonius);
liveness::generate(&mut checker, body, elements, flow_inits, move_data);

translate_outlives_facts(&mut checker);
let opaque_type_values = infcx.take_opaque_types();
Expand Down

0 comments on commit d49994b

Please sign in to comment.