From 7b5b7a70101324e590709c0e80effc5d99fc69de Mon Sep 17 00:00:00 2001 From: Amanda Stjerna Date: Wed, 5 Jun 2024 10:57:27 +0200 Subject: [PATCH] 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](https://github.com/rust-lang/rust/blob/85f90a461262f7ca37a6e629933d455fa9c3ee48/compiler/rustc_borrowck/src/type_check/liveness/trace.rs#L219) should be inspected by a reviewer, in particular the one on L#259 in this PR, which should be trivial for someone with the right background knowledge. I also included some minor lints I found on the way there that I couldn't help myself from addressing. --- compiler/rustc_borrowck/src/borrow_set.rs | 2 +- .../rustc_borrowck/src/borrowck_errors.rs | 2 +- compiler/rustc_borrowck/src/dataflow.rs | 6 +- .../src/diagnostics/region_errors.rs | 1 - .../src/diagnostics/region_name.rs | 10 ++-- compiler/rustc_borrowck/src/nll.rs | 1 - compiler/rustc_borrowck/src/polonius/mod.rs | 4 +- .../rustc_borrowck/src/region_infer/mod.rs | 4 +- .../src/region_infer/opaque_types.rs | 4 +- .../src/type_check/liveness/mod.rs | 10 +--- .../src/type_check/liveness/polonius.rs | 6 +- .../src/type_check/liveness/trace.rs | 55 ++++++++++++------- compiler/rustc_borrowck/src/type_check/mod.rs | 3 +- 13 files changed, 57 insertions(+), 51 deletions(-) diff --git a/compiler/rustc_borrowck/src/borrow_set.rs b/compiler/rustc_borrowck/src/borrow_set.rs index 3e6d0311b2703..0bae1bd07a298 100644 --- a/compiler/rustc_borrowck/src/borrow_set.rs +++ b/compiler/rustc_borrowck/src/borrow_set.rs @@ -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(), diff --git a/compiler/rustc_borrowck/src/borrowck_errors.rs b/compiler/rustc_borrowck/src/borrowck_errors.rs index 622feb4e4c736..387492651ff7d 100644 --- a/compiler/rustc_borrowck/src/borrowck_errors.rs +++ b/compiler/rustc_borrowck/src/borrowck_errors.rs @@ -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")); diff --git a/compiler/rustc_borrowck/src/dataflow.rs b/compiler/rustc_borrowck/src/dataflow.rs index ec7d4582a6013..f2b5ddcd7827c 100644 --- a/compiler/rustc_borrowck/src/dataflow.rs +++ b/compiler/rustc_borrowck/src/dataflow.rs @@ -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( diff --git a/compiler/rustc_borrowck/src/diagnostics/region_errors.rs b/compiler/rustc_borrowck/src/diagnostics/region_errors.rs index 15a8764aab3c4..37a5df57924a6 100644 --- a/compiler/rustc_borrowck/src/diagnostics/region_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/region_errors.rs @@ -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 { diff --git a/compiler/rustc_borrowck/src/diagnostics/region_name.rs b/compiler/rustc_borrowck/src/diagnostics/region_name.rs index 2cf548e28b18a..bb1bf4f39998f 100644 --- a/compiler/rustc_borrowck/src/diagnostics/region_name.rs +++ b/compiler/rustc_borrowck/src/diagnostics/region_name.rs @@ -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>` @@ -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)); } _ => { @@ -654,7 +654,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(); @@ -719,7 +719,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" } @@ -960,7 +960,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(); diff --git a/compiler/rustc_borrowck/src/nll.rs b/compiler/rustc_borrowck/src/nll.rs index 49f50babdcb96..10bab700a9a21 100644 --- a/compiler/rustc_borrowck/src/nll.rs +++ b/compiler/rustc_borrowck/src/nll.rs @@ -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 diff --git a/compiler/rustc_borrowck/src/polonius/mod.rs b/compiler/rustc_borrowck/src/polonius/mod.rs index 9984f76e6d446..c590104978c2d 100644 --- a/compiler/rustc_borrowck/src/polonius/mod.rs +++ b/compiler/rustc_borrowck/src/polonius/mod.rs @@ -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); diff --git a/compiler/rustc_borrowck/src/region_infer/mod.rs b/compiler/rustc_borrowck/src/region_infer/mod.rs index b57cf9066cf33..e50015debee72 100644 --- a/compiler/rustc_borrowck/src/region_infer/mod.rs +++ b/compiler/rustc_borrowck/src/region_infer/mod.rs @@ -269,7 +269,7 @@ fn sccs_info<'tcx>(infcx: &BorrowckInferCtxt<'tcx>, sccs: Rc 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. diff --git a/compiler/rustc_borrowck/src/region_infer/opaque_types.rs b/compiler/rustc_borrowck/src/region_infer/opaque_types.rs index 06adb686ed48e..3c76e6f80d6b5 100644 --- a/compiler/rustc_borrowck/src/region_infer/opaque_types.rs +++ b/compiler/rustc_borrowck/src/region_infer/opaque_types.rs @@ -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, diff --git a/compiler/rustc_borrowck/src/type_check/liveness/mod.rs b/compiler/rustc_borrowck/src/type_check/liveness/mod.rs index 8b863efad6ca2..b777e01f7a6c2 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/mod.rs @@ -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, }; @@ -38,7 +36,6 @@ pub(super) fn generate<'mir, 'tcx>( elements: &Rc, flow_inits: &mut ResultsCursor<'mir, 'tcx, MaybeInitializedPlaces<'mir, 'tcx>>, move_data: &MoveData<'tcx>, - use_polonius: bool, ) { debug!("liveness::generate"); @@ -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, diff --git a/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs b/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs index d8f03a07a63ca..a009e28a0ddd0 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs @@ -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, diff --git a/compiler/rustc_borrowck/src/type_check/liveness/trace.rs b/compiler/rustc_borrowck/src/type_check/liveness/trace.rs index 50843c602cc84..359c4ea0eb1e2 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/trace.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/trace.rs @@ -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(()) } diff --git a/compiler/rustc_borrowck/src/type_check/mod.rs b/compiler/rustc_borrowck/src/type_check/mod.rs index 291d2782c32cc..c1a756689ac09 100644 --- a/compiler/rustc_borrowck/src/type_check/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/mod.rs @@ -133,7 +133,6 @@ pub(crate) fn type_check<'mir, 'tcx>( move_data: &MoveData<'tcx>, elements: &Rc, 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 { @@ -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();