From 5055864071a821f505a16c7afc8c6eea2f0807b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Mon, 30 Dec 2024 20:35:25 +0000 Subject: [PATCH 01/12] disable NLL liveness optimization when using polonius in NLLs some locals are marked live at all points if one of their regions escapes the function but that doesn't work in a flow-sensitive setting like polonius --- .../src/type_check/liveness/mod.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_borrowck/src/type_check/liveness/mod.rs b/compiler/rustc_borrowck/src/type_check/liveness/mod.rs index f23602d03588f..4e0b2a4e29681 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/mod.rs @@ -38,11 +38,19 @@ pub(super) fn generate<'a, 'tcx>( ) { debug!("liveness::generate"); - let free_regions = regions_that_outlive_free_regions( - typeck.infcx.num_region_vars(), - &typeck.universal_regions, - &typeck.constraints.outlives_constraints, - ); + // NLLs can avoid computing some liveness data here because its constraints are + // location-insensitive, but that doesn't work in polonius: locals whose type contains a region + // that outlives a free region are not necessarily live everywhere in a flow-sensitive setting, + // unlike NLLs. + let free_regions = if !typeck.tcx().sess.opts.unstable_opts.polonius.is_next_enabled() { + regions_that_outlive_free_regions( + typeck.infcx.num_region_vars(), + &typeck.universal_regions, + &typeck.constraints.outlives_constraints, + ) + } else { + typeck.universal_regions.universal_regions_iter().collect() + }; let (relevant_live_locals, boring_locals) = compute_relevant_live_locals(typeck.tcx(), &free_regions, body); From 0c978bc4e6632972a484f3b39c83dee1e151daa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 31 Dec 2024 16:17:28 +0000 Subject: [PATCH 02/12] introduce reachability to the constraint graph --- compiler/rustc_borrowck/src/nll.rs | 10 +-- .../src/polonius/loan_liveness.rs | 82 +++++++++++++++++++ compiler/rustc_borrowck/src/polonius/mod.rs | 30 +++++-- 3 files changed, 112 insertions(+), 10 deletions(-) create mode 100644 compiler/rustc_borrowck/src/polonius/loan_liveness.rs diff --git a/compiler/rustc_borrowck/src/nll.rs b/compiler/rustc_borrowck/src/nll.rs index aa0bfd7214729..590cbef05d53e 100644 --- a/compiler/rustc_borrowck/src/nll.rs +++ b/compiler/rustc_borrowck/src/nll.rs @@ -103,7 +103,7 @@ pub(crate) fn compute_regions<'a, 'tcx>( constraints, universal_region_relations, opaque_type_values, - mut polonius_context, + polonius_context, } = type_check::type_check( infcx, body, @@ -142,10 +142,10 @@ pub(crate) fn compute_regions<'a, 'tcx>( location_map, ); - // 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(infcx.tcx, ®ioncx, body) + // If requested for `-Zpolonius=next`, convert NLL constraints to localized outlives constraints + // and use them to compute loan liveness. + let localized_outlives_constraints = polonius_context.as_ref().map(|polonius_context| { + polonius_context.compute_loan_liveness(infcx.tcx, ®ioncx, body, borrow_set) }); // If requested: dump NLL facts, and run legacy polonius analysis. diff --git a/compiler/rustc_borrowck/src/polonius/loan_liveness.rs b/compiler/rustc_borrowck/src/polonius/loan_liveness.rs new file mode 100644 index 0000000000000..796a63da79d6e --- /dev/null +++ b/compiler/rustc_borrowck/src/polonius/loan_liveness.rs @@ -0,0 +1,82 @@ +use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet}; +use rustc_middle::mir::Body; +use rustc_middle::ty::{RegionVid, TyCtxt}; +use rustc_mir_dataflow::points::PointIndex; + +use super::{LiveLoans, LocalizedOutlivesConstraintSet}; +use crate::BorrowSet; +use crate::region_infer::values::LivenessValues; + +/// With the full graph of constraints, we can compute loan reachability, and trace loan liveness +/// throughout the CFG. +pub(super) fn compute_loan_liveness<'tcx>( + _tcx: TyCtxt<'tcx>, + _body: &Body<'tcx>, + liveness: &LivenessValues, + borrow_set: &BorrowSet<'tcx>, + localized_outlives_constraints: &LocalizedOutlivesConstraintSet, +) -> LiveLoans { + let mut live_loans = LiveLoans::new(borrow_set.len()); + let graph = index_constraints(&localized_outlives_constraints); + let mut visited = FxHashSet::default(); + let mut stack = Vec::new(); + + // Compute reachability per loan by traversing each loan's subgraph starting from where it is + // introduced. + for (loan_idx, loan) in borrow_set.iter_enumerated() { + visited.clear(); + stack.clear(); + + let start_node = LocalizedNode { + region: loan.region, + point: liveness.point_from_location(loan.reserve_location), + }; + stack.push(start_node); + + while let Some(node) = stack.pop() { + if !visited.insert(node) { + continue; + } + + // Record the loan as being live on entry to this point. + live_loans.insert(node.point, loan_idx); + + for succ in outgoing_edges(&graph, node) { + stack.push(succ); + } + } + } + + live_loans +} + +/// The localized constraint graph is currently the per-node map of its physical edges. In the +/// future, we'll add logical edges to model constraints that hold at all points in the CFG. +type LocalizedConstraintGraph = FxHashMap>; + +/// A node in the graph to be traversed, one of the two vertices of a localized outlives constraint. +#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)] +struct LocalizedNode { + region: RegionVid, + point: PointIndex, +} + +/// Index the outlives constraints into a graph of edges per node. +fn index_constraints(constraints: &LocalizedOutlivesConstraintSet) -> LocalizedConstraintGraph { + let mut edges = LocalizedConstraintGraph::default(); + for constraint in &constraints.outlives { + let source = LocalizedNode { region: constraint.source, point: constraint.from }; + let target = LocalizedNode { region: constraint.target, point: constraint.to }; + edges.entry(source).or_default().insert(target); + } + + edges +} + +/// Returns the outgoing edges of a given node, not its transitive closure. +fn outgoing_edges( + graph: &LocalizedConstraintGraph, + node: LocalizedNode, +) -> impl Iterator + use<'_> { + graph.get(&node).into_iter().flat_map(|edges| edges.iter().copied()) +} diff --git a/compiler/rustc_borrowck/src/polonius/mod.rs b/compiler/rustc_borrowck/src/polonius/mod.rs index 7d0f9397021be..313ed5d66493b 100644 --- a/compiler/rustc_borrowck/src/polonius/mod.rs +++ b/compiler/rustc_borrowck/src/polonius/mod.rs @@ -37,6 +37,7 @@ mod constraints; mod dump; pub(crate) mod legacy; mod liveness_constraints; +mod loan_liveness; mod typeck_constraints; use std::collections::BTreeMap; @@ -49,8 +50,12 @@ 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::loan_liveness::compute_loan_liveness; use self::typeck_constraints::convert_typeck_constraints; -use crate::RegionInferenceContext; +use crate::dataflow::BorrowIndex; +use crate::{BorrowSet, RegionInferenceContext}; + +pub(crate) type LiveLoans = SparseBitMatrix; /// This struct holds the data needed to create the Polonius localized constraints. pub(crate) struct PoloniusContext { @@ -82,14 +87,20 @@ impl PoloniusContext { Self { live_region_variances: BTreeMap::new(), live_regions: None } } - /// Creates a constraint set for `-Zpolonius=next` by: + /// Computes live loans using the set of loans model for `-Zpolonius=next`. + /// + /// First, creates a constraint graph combining regions and CFG points, by: /// - converting NLL typeck constraints to be localized /// - encoding liveness constraints - pub(crate) fn create_localized_constraints<'tcx>( + /// + /// Then, this graph is traversed, and combined with kills, reachability is recorded as loan + /// liveness, to be used by the loan scope and active loans computations. + pub(crate) fn compute_loan_liveness<'tcx>( &self, tcx: TyCtxt<'tcx>, regioncx: &RegionInferenceContext<'tcx>, body: &Body<'tcx>, + borrow_set: &BorrowSet<'tcx>, ) -> LocalizedOutlivesConstraintSet { let mut localized_outlives_constraints = LocalizedOutlivesConstraintSet::default(); convert_typeck_constraints( @@ -113,8 +124,17 @@ impl PoloniusContext { &mut localized_outlives_constraints, ); - // FIXME: here, we can trace loan reachability in the constraint graph and record this as loan - // liveness for the next step in the chain, the NLL loan scope and active loans computations. + // Now that we have a complete graph, we can compute reachability to trace the liveness of + // loans for the next step in the chain, the NLL loan scope and active loans computations. + let _live_loans = compute_loan_liveness( + tcx, + body, + regioncx.liveness_constraints(), + borrow_set, + &localized_outlives_constraints, + ); + // FIXME: record the live loans in the regioncx's liveness constraints, where the + // location-insensitive variant's data is stored. localized_outlives_constraints } From 67a1bb1554c96c887c0b61bd21a21826aa3b4d8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 31 Dec 2024 16:39:17 +0000 Subject: [PATCH 03/12] replace location-insensitive analysis with location-sensitive analysis we're in in the endgame now set up the location-sensitive analysis end to end: - stop recording inflowing loans and loan liveness in liveness - replace location-insensitive liveness data with live loans computed by reachability - remove equivalence between polonius scopes and NLL scopes, and only run one scope computation --- compiler/rustc_borrowck/src/dataflow.rs | 63 ++++++++----------- compiler/rustc_borrowck/src/nll.rs | 2 +- compiler/rustc_borrowck/src/polonius/mod.rs | 7 +-- .../rustc_borrowck/src/region_infer/mod.rs | 37 +++++------ .../rustc_borrowck/src/region_infer/values.rs | 57 ++++------------- .../src/type_check/liveness/trace.rs | 33 +--------- 6 files changed, 59 insertions(+), 140 deletions(-) diff --git a/compiler/rustc_borrowck/src/dataflow.rs b/compiler/rustc_borrowck/src/dataflow.rs index a7a6f2da509c1..3a200642ce58c 100644 --- a/compiler/rustc_borrowck/src/dataflow.rs +++ b/compiler/rustc_borrowck/src/dataflow.rs @@ -325,10 +325,17 @@ impl<'tcx> PoloniusOutOfScopePrecomputer<'_, 'tcx> { let sccs = self.regioncx.constraint_sccs(); let universal_regions = self.regioncx.universal_regions(); + // The loop below was useful for the location-insensitive analysis but shouldn't be + // impactful in the location-sensitive case. It seems that it does, however, as without it a + // handful of tests fail. That likely means some liveness or outlives data related to choice + // regions is missing + // FIXME: investigate the impact of loans traversing applied member constraints and why some + // tests fail otherwise. + // // We first handle the cases where the loan doesn't go out of scope, depending on the // issuing region's successors. for successor in graph::depth_first_search(&self.regioncx.region_graph(), issuing_region) { - // 1. Via applied member constraints + // Via applied member constraints // // The issuing region can flow into the choice regions, and they are either: // - placeholders or free regions themselves, @@ -346,14 +353,6 @@ impl<'tcx> PoloniusOutOfScopePrecomputer<'_, 'tcx> { return; } } - - // 2. Via regions that are live at all points: placeholders and free regions. - // - // If the issuing region outlives such a region, its loan escapes the function and - // cannot go out of scope. We can early return. - if self.regioncx.is_region_live_at_all_points(successor) { - return; - } } let first_block = loan_issued_at.block; @@ -461,34 +460,26 @@ impl<'a, 'tcx> Borrows<'a, 'tcx> { regioncx: &RegionInferenceContext<'tcx>, borrow_set: &'a BorrowSet<'tcx>, ) -> Self { - let mut borrows_out_of_scope_at_location = - calculate_borrows_out_of_scope_at_location(body, regioncx, borrow_set); - - // The in-tree polonius analysis computes loans going out of scope using the set-of-loans - // model, and makes sure they're identical to the existing computation of the set-of-points - // model. - if tcx.sess.opts.unstable_opts.polonius.is_next_enabled() { - let mut polonius_prec = PoloniusOutOfScopePrecomputer::new(body, regioncx); - for (loan_idx, loan_data) in borrow_set.iter_enumerated() { - let issuing_region = loan_data.region; - let loan_issued_at = loan_data.reserve_location; - - polonius_prec.precompute_loans_out_of_scope( - loan_idx, - issuing_region, - loan_issued_at, - ); - } - - assert_eq!( - borrows_out_of_scope_at_location, polonius_prec.loans_out_of_scope_at_location, - "polonius loan scopes differ from NLL borrow scopes, for body {:?}", - body.span, - ); - - borrows_out_of_scope_at_location = polonius_prec.loans_out_of_scope_at_location; - } + let borrows_out_of_scope_at_location = + if !tcx.sess.opts.unstable_opts.polonius.is_next_enabled() { + calculate_borrows_out_of_scope_at_location(body, regioncx, borrow_set) + } else { + // The in-tree polonius analysis computes loans going out of scope using the + // set-of-loans model. + let mut polonius_prec = PoloniusOutOfScopePrecomputer::new(body, regioncx); + for (loan_idx, loan_data) in borrow_set.iter_enumerated() { + let issuing_region = loan_data.region; + let loan_issued_at = loan_data.reserve_location; + + polonius_prec.precompute_loans_out_of_scope( + loan_idx, + issuing_region, + loan_issued_at, + ); + } + polonius_prec.loans_out_of_scope_at_location + }; Borrows { tcx, body, borrow_set, borrows_out_of_scope_at_location } } diff --git a/compiler/rustc_borrowck/src/nll.rs b/compiler/rustc_borrowck/src/nll.rs index 590cbef05d53e..35264bd1a7075 100644 --- a/compiler/rustc_borrowck/src/nll.rs +++ b/compiler/rustc_borrowck/src/nll.rs @@ -145,7 +145,7 @@ pub(crate) fn compute_regions<'a, 'tcx>( // If requested for `-Zpolonius=next`, convert NLL constraints to localized outlives constraints // and use them to compute loan liveness. let localized_outlives_constraints = polonius_context.as_ref().map(|polonius_context| { - polonius_context.compute_loan_liveness(infcx.tcx, ®ioncx, body, borrow_set) + polonius_context.compute_loan_liveness(infcx.tcx, &mut regioncx, body, borrow_set) }); // If requested: dump NLL facts, and run legacy polonius analysis. diff --git a/compiler/rustc_borrowck/src/polonius/mod.rs b/compiler/rustc_borrowck/src/polonius/mod.rs index 313ed5d66493b..52a5f75d8a239 100644 --- a/compiler/rustc_borrowck/src/polonius/mod.rs +++ b/compiler/rustc_borrowck/src/polonius/mod.rs @@ -98,7 +98,7 @@ impl PoloniusContext { pub(crate) fn compute_loan_liveness<'tcx>( &self, tcx: TyCtxt<'tcx>, - regioncx: &RegionInferenceContext<'tcx>, + regioncx: &mut RegionInferenceContext<'tcx>, body: &Body<'tcx>, borrow_set: &BorrowSet<'tcx>, ) -> LocalizedOutlivesConstraintSet { @@ -126,15 +126,14 @@ impl PoloniusContext { // Now that we have a complete graph, we can compute reachability to trace the liveness of // loans for the next step in the chain, the NLL loan scope and active loans computations. - let _live_loans = compute_loan_liveness( + let live_loans = compute_loan_liveness( tcx, body, regioncx.liveness_constraints(), borrow_set, &localized_outlives_constraints, ); - // FIXME: record the live loans in the regioncx's liveness constraints, where the - // location-insensitive variant's data is stored. + regioncx.record_live_loans(live_loans); localized_outlives_constraints } diff --git a/compiler/rustc_borrowck/src/region_infer/mod.rs b/compiler/rustc_borrowck/src/region_infer/mod.rs index c177538ee177a..d2268c4779d63 100644 --- a/compiler/rustc_borrowck/src/region_infer/mod.rs +++ b/compiler/rustc_borrowck/src/region_infer/mod.rs @@ -31,6 +31,7 @@ use crate::constraints::{ConstraintSccIndex, OutlivesConstraint, OutlivesConstra use crate::dataflow::BorrowIndex; use crate::diagnostics::{RegionErrorKind, RegionErrors, UniverseInfo}; use crate::member_constraints::{MemberConstraintSet, NllMemberConstraintIndex}; +use crate::polonius::LiveLoans; use crate::polonius::legacy::PoloniusOutput; use crate::region_infer::reverse_sccs::ReverseSccGraph; use crate::region_infer::values::{LivenessValues, RegionElement, RegionValues, ToElementIndex}; @@ -2171,28 +2172,6 @@ impl<'tcx> RegionInferenceContext<'tcx> { self.constraint_graph.region_graph(&self.constraints, self.universal_regions().fr_static) } - /// Returns whether the given region is considered live at all points: whether it is a - /// placeholder or a free region. - pub(crate) fn is_region_live_at_all_points(&self, region: RegionVid) -> bool { - // FIXME: there must be a cleaner way to find this information. At least, when - // higher-ranked subtyping is abstracted away from the borrowck main path, we'll only - // need to check whether this is a universal region. - let origin = self.region_definition(region).origin; - let live_at_all_points = matches!( - origin, - NllRegionVariableOrigin::Placeholder(_) | NllRegionVariableOrigin::FreeRegion - ); - live_at_all_points - } - - /// Returns whether the `loan_idx` is live at the given `location`: whether its issuing - /// region is contained within the type of a variable that is live at this point. - /// Note: for now, the sets of live loans is only available when using `-Zpolonius=next`. - pub(crate) fn is_loan_live_at(&self, loan_idx: BorrowIndex, location: Location) -> bool { - let point = self.liveness_constraints.point_from_location(location); - self.liveness_constraints.is_loan_live_at(loan_idx, point) - } - /// Returns the representative `RegionVid` for a given SCC. /// See `RegionTracker` for how a region variable ID is chosen. /// @@ -2208,6 +2187,20 @@ impl<'tcx> RegionInferenceContext<'tcx> { pub(crate) fn liveness_constraints(&self) -> &LivenessValues { &self.liveness_constraints } + + /// When using `-Zpolonius=next`, records the given live loans for the loan scopes and active + /// loans dataflow computations. + pub(crate) fn record_live_loans(&mut self, live_loans: LiveLoans) { + self.liveness_constraints.record_live_loans(live_loans); + } + + /// Returns whether the `loan_idx` is live at the given `location`: whether its issuing + /// region is contained within the type of a variable that is live at this point. + /// Note: for now, the sets of live loans is only available when using `-Zpolonius=next`. + pub(crate) fn is_loan_live_at(&self, loan_idx: BorrowIndex, location: Location) -> bool { + let point = self.liveness_constraints.point_from_location(location); + self.liveness_constraints.is_loan_live_at(loan_idx, point) + } } impl<'tcx> RegionDefinition<'tcx> { diff --git a/compiler/rustc_borrowck/src/region_infer/values.rs b/compiler/rustc_borrowck/src/region_infer/values.rs index 11fb125ca2281..f1bcb353dc61c 100644 --- a/compiler/rustc_borrowck/src/region_infer/values.rs +++ b/compiler/rustc_borrowck/src/region_infer/values.rs @@ -11,6 +11,7 @@ use rustc_mir_dataflow::points::{DenseLocationMap, PointIndex}; use tracing::debug; use crate::BorrowIndex; +use crate::polonius::LiveLoans; rustc_index::newtype_index! { /// A single integer representing a `ty::Placeholder`. @@ -50,29 +51,8 @@ pub(crate) struct LivenessValues { /// region is live, only that it is. points: Option>, - /// When using `-Zpolonius=next`, for each point: the loans flowing into the live regions at - /// that point. - pub(crate) loans: Option, -} - -/// Data used to compute the loans that are live at a given point in the CFG, when using -/// `-Zpolonius=next`. -pub(crate) struct LiveLoans { - /// The set of loans that flow into a given region. When individual regions are marked as live - /// in the CFG, these inflowing loans are recorded as live. - pub(crate) inflowing_loans: SparseBitMatrix, - - /// The set of loans that are live at a given point in the CFG. - pub(crate) live_loans: SparseBitMatrix, -} - -impl LiveLoans { - pub(crate) fn new(num_loans: usize) -> Self { - LiveLoans { - live_loans: SparseBitMatrix::new(num_loans), - inflowing_loans: SparseBitMatrix::new(num_loans), - } - } + /// When using `-Zpolonius=next`, the set of loans that are live at a given point in the CFG. + live_loans: Option, } impl LivenessValues { @@ -82,7 +62,7 @@ impl LivenessValues { live_regions: None, points: Some(SparseIntervalMatrix::new(location_map.num_points())), location_map, - loans: None, + live_loans: None, } } @@ -95,7 +75,7 @@ impl LivenessValues { live_regions: Some(Default::default()), points: None, location_map, - loans: None, + live_loans: None, } } @@ -129,13 +109,6 @@ impl LivenessValues { } else if self.location_map.point_in_range(point) { self.live_regions.as_mut().unwrap().insert(region); } - - // When available, record the loans flowing into this region as live at the given point. - if let Some(loans) = self.loans.as_mut() { - if let Some(inflowing) = loans.inflowing_loans.row(region) { - loans.live_loans.union_row(point, inflowing); - } - } } /// Records `region` as being live at all the given `points`. @@ -146,17 +119,6 @@ impl LivenessValues { } else if points.iter().any(|point| self.location_map.point_in_range(point)) { self.live_regions.as_mut().unwrap().insert(region); } - - // When available, record the loans flowing into this region as live at the given points. - if let Some(loans) = self.loans.as_mut() { - if let Some(inflowing) = loans.inflowing_loans.row(region) { - if !inflowing.is_empty() { - for point in points.iter() { - loans.live_loans.union_row(point, inflowing); - } - } - } - } } /// Records `region` as being live at all the control-flow points. @@ -213,12 +175,17 @@ impl LivenessValues { self.location_map.to_location(point) } + /// When using `-Zpolonius=next`, records the given live loans for the loan scopes and active + /// loans dataflow computations. + pub(crate) fn record_live_loans(&mut self, live_loans: LiveLoans) { + self.live_loans = Some(live_loans); + } + /// When using `-Zpolonius=next`, returns whether the `loan_idx` is live at the given `point`. pub(crate) fn is_loan_live_at(&self, loan_idx: BorrowIndex, point: PointIndex) -> bool { - self.loans + self.live_loans .as_ref() .expect("Accessing live loans requires `-Zpolonius=next`") - .live_loans .contains(point, loan_idx) } } diff --git a/compiler/rustc_borrowck/src/type_check/liveness/trace.rs b/compiler/rustc_borrowck/src/type_check/liveness/trace.rs index 4c0d3138f2d37..c564d85616e25 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/trace.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/trace.rs @@ -16,7 +16,7 @@ use rustc_trait_selection::traits::query::type_op::{DropckOutlives, TypeOp, Type use tracing::debug; use crate::polonius; -use crate::region_infer::values::{self, LiveLoans}; +use crate::region_infer::values; use crate::type_check::liveness::local_use_map::LocalUseMap; use crate::type_check::{NormalizeLocation, TypeChecker}; @@ -44,37 +44,6 @@ pub(super) fn trace<'a, 'tcx>( boring_locals: Vec, ) { let local_use_map = &LocalUseMap::build(&relevant_live_locals, location_map, body); - - // When using `-Zpolonius=next`, compute the set of loans that can reach a given region. - if typeck.tcx().sess.opts.unstable_opts.polonius.is_next_enabled() { - let borrow_set = &typeck.borrow_set; - let mut live_loans = LiveLoans::new(borrow_set.len()); - let outlives_constraints = &typeck.constraints.outlives_constraints; - let graph = outlives_constraints.graph(typeck.infcx.num_region_vars()); - let region_graph = - graph.region_graph(outlives_constraints, typeck.universal_regions.fr_static); - - // Traverse each issuing region's constraints, and record the loan as flowing into the - // outlived region. - for (loan, issuing_region_data) in borrow_set.iter_enumerated() { - for succ in rustc_data_structures::graph::depth_first_search( - ®ion_graph, - issuing_region_data.region, - ) { - // We don't need to mention that a loan flows into its issuing region. - if succ == issuing_region_data.region { - continue; - } - - live_loans.inflowing_loans.insert(succ, loan); - } - } - - // Store the inflowing loans in the liveness constraints: they will be used to compute live - // loans when liveness data is recorded there. - typeck.constraints.liveness_constraints.loans = Some(live_loans); - }; - let cx = LivenessContext { typeck, body, From 550cf1f4a4ccee7ef2e49eab7e7b6dd4d2930712 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 31 Dec 2024 17:32:26 +0000 Subject: [PATCH 04/12] handle kills in reachability --- .../src/polonius/loan_liveness.rs | 133 +++++++++++++++++- 1 file changed, 126 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_borrowck/src/polonius/loan_liveness.rs b/compiler/rustc_borrowck/src/polonius/loan_liveness.rs index 796a63da79d6e..aacc342e0f123 100644 --- a/compiler/rustc_borrowck/src/polonius/loan_liveness.rs +++ b/compiler/rustc_borrowck/src/polonius/loan_liveness.rs @@ -1,22 +1,34 @@ +use std::collections::{BTreeMap, BTreeSet}; + use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet}; -use rustc_middle::mir::Body; +use rustc_middle::mir::visit::Visitor; +use rustc_middle::mir::{ + Body, Local, Location, Place, Rvalue, Statement, StatementKind, Terminator, TerminatorKind, +}; use rustc_middle::ty::{RegionVid, TyCtxt}; use rustc_mir_dataflow::points::PointIndex; use super::{LiveLoans, LocalizedOutlivesConstraintSet}; -use crate::BorrowSet; +use crate::dataflow::BorrowIndex; use crate::region_infer::values::LivenessValues; +use crate::{BorrowSet, PlaceConflictBias, places_conflict}; -/// With the full graph of constraints, we can compute loan reachability, and trace loan liveness -/// throughout the CFG. +/// With the full graph of constraints, we can compute loan reachability, stop at kills, and trace +/// loan liveness throughout the CFG. pub(super) fn compute_loan_liveness<'tcx>( - _tcx: TyCtxt<'tcx>, - _body: &Body<'tcx>, + tcx: TyCtxt<'tcx>, + body: &Body<'tcx>, liveness: &LivenessValues, borrow_set: &BorrowSet<'tcx>, localized_outlives_constraints: &LocalizedOutlivesConstraintSet, ) -> LiveLoans { let mut live_loans = LiveLoans::new(borrow_set.len()); + + // FIXME: it may be preferable for kills to be encoded in the edges themselves, to simplify and + // likely make traversal (and constraint generation) more efficient. We also display kills on + // edges when visualizing the constraint graph anyways. + let kills = collect_kills(body, tcx, borrow_set); + let graph = index_constraints(&localized_outlives_constraints); let mut visited = FxHashSet::default(); let mut stack = Vec::new(); @@ -41,7 +53,16 @@ pub(super) fn compute_loan_liveness<'tcx>( // Record the loan as being live on entry to this point. live_loans.insert(node.point, loan_idx); + // Continuing traversal will depend on whether the loan is killed at this point. + let current_location = liveness.location_from_point(node.point); + let is_loan_killed = + kills.get(¤t_location).is_some_and(|kills| kills.contains(&loan_idx)); + for succ in outgoing_edges(&graph, node) { + // If the loan is killed at this point, it is killed _on exit_. + if is_loan_killed { + continue; + } stack.push(succ); } } @@ -61,7 +82,7 @@ struct LocalizedNode { point: PointIndex, } -/// Index the outlives constraints into a graph of edges per node. +/// Traverses the constraints and returns the indexable graph of edges per node. fn index_constraints(constraints: &LocalizedOutlivesConstraintSet) -> LocalizedConstraintGraph { let mut edges = LocalizedConstraintGraph::default(); for constraint in &constraints.outlives { @@ -80,3 +101,101 @@ fn outgoing_edges( ) -> impl Iterator + use<'_> { graph.get(&node).into_iter().flat_map(|edges| edges.iter().copied()) } + +/// Traverses the MIR and collects kills. +fn collect_kills<'tcx>( + body: &Body<'tcx>, + tcx: TyCtxt<'tcx>, + borrow_set: &BorrowSet<'tcx>, +) -> BTreeMap> { + let mut collector = KillsCollector { borrow_set, tcx, body, kills: BTreeMap::default() }; + for (block, data) in body.basic_blocks.iter_enumerated() { + collector.visit_basic_block_data(block, data); + } + collector.kills +} + +struct KillsCollector<'a, 'tcx> { + body: &'a Body<'tcx>, + tcx: TyCtxt<'tcx>, + borrow_set: &'a BorrowSet<'tcx>, + + /// The set of loans killed at each location. + kills: BTreeMap>, +} + +// This visitor has a similar structure to the `Borrows` dataflow computation with respect to kills, +// and the datalog polonius fact generation for the `loan_killed_at` relation. +impl<'tcx> KillsCollector<'_, 'tcx> { + /// Records the borrows on the specified place as `killed`. For example, when assigning to a + /// local, or on a call's return destination. + fn record_killed_borrows_for_place(&mut self, place: Place<'tcx>, location: Location) { + let other_borrows_of_local = self + .borrow_set + .local_map + .get(&place.local) + .into_iter() + .flat_map(|bs| bs.iter()) + .copied(); + + // If the borrowed place is a local with no projections, all other borrows of this + // local must conflict. This is purely an optimization so we don't have to call + // `places_conflict` for every borrow. + if place.projection.is_empty() { + if !self.body.local_decls[place.local].is_ref_to_static() { + self.kills.entry(location).or_default().extend(other_borrows_of_local); + } + return; + } + + // By passing `PlaceConflictBias::NoOverlap`, we conservatively assume that any given + // pair of array indices are not equal, so that when `places_conflict` returns true, we + // will be assured that two places being compared definitely denotes the same sets of + // locations. + let definitely_conflicting_borrows = other_borrows_of_local.filter(|&i| { + places_conflict( + self.tcx, + self.body, + self.borrow_set[i].borrowed_place, + place, + PlaceConflictBias::NoOverlap, + ) + }); + + self.kills.entry(location).or_default().extend(definitely_conflicting_borrows); + } + + /// Records the borrows on the specified local as `killed`. + fn record_killed_borrows_for_local(&mut self, local: Local, location: Location) { + if let Some(borrow_indices) = self.borrow_set.local_map.get(&local) { + self.kills.entry(location).or_default().extend(borrow_indices.iter()); + } + } +} + +impl<'tcx> Visitor<'tcx> for KillsCollector<'_, 'tcx> { + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { + // Make sure there are no remaining borrows for locals that have gone out of scope. + if let StatementKind::StorageDead(local) = statement.kind { + self.record_killed_borrows_for_local(local, location); + } + + self.super_statement(statement, location); + } + + fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { + // When we see `X = ...`, then kill borrows of `(*X).foo` and so forth. + self.record_killed_borrows_for_place(*place, location); + self.super_assign(place, rvalue, location); + } + + fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { + // A `Call` terminator's return value can be a local which has borrows, so we need to record + // those as killed as well. + if let TerminatorKind::Call { destination, .. } = terminator.kind { + self.record_killed_borrows_for_place(destination, location); + } + + self.super_terminator(terminator, location); + } +} From 0f3dd33e1d39f242c2da4d13c650b14b11c0d3c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 31 Dec 2024 17:37:58 +0000 Subject: [PATCH 05/12] deal with naive reachability weakness it's a bit mind-bending --- .../src/polonius/loan_liveness.rs | 95 ++++++++++++++++--- 1 file changed, 82 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_borrowck/src/polonius/loan_liveness.rs b/compiler/rustc_borrowck/src/polonius/loan_liveness.rs index aacc342e0f123..c519453652fe8 100644 --- a/compiler/rustc_borrowck/src/polonius/loan_liveness.rs +++ b/compiler/rustc_borrowck/src/polonius/loan_liveness.rs @@ -53,15 +53,69 @@ pub(super) fn compute_loan_liveness<'tcx>( // Record the loan as being live on entry to this point. live_loans.insert(node.point, loan_idx); - // Continuing traversal will depend on whether the loan is killed at this point. + // Here, we have a conundrum. There's currently a weakness in our theory, in that + // we're using a single notion of reachability to represent what used to be _two_ + // different transitive closures. It didn't seem impactful when coming up with the + // single-graph and reachability through space (regions) + time (CFG) concepts, but in + // practice the combination of time-traveling with kills is more impactful than + // initially anticipated. + // + // Kills should prevent a loan from reaching its successor points in the CFG, but not + // while time-traveling: we're not actually at that CFG point, but looking for + // predecessor regions that contain the loan. One of the two TCs we had pushed the + // transitive subset edges to each point instead of having backward edges, and the + // problem didn't exist before. In the abstract, naive reachability is not enough to + // model this, we'd need a slightly different solution. For example, maybe with a + // two-step traversal: + // - at each point we first traverse the subgraph (and possibly time-travel) looking for + // exit nodes while ignoring kills, + // - and then when we're back at the current point, we continue normally. + // + // Another (less annoying) subtlety is that kills and the loan use-map are + // flow-insensitive. Kills can actually appear in places before a loan is introduced, or + // at a location that is actually unreachable in the CFG from the introduction point, + // and these can also be encountered during time-traveling. + // + // The simplest change that made sense to "fix" the issues above is taking into + // account kills that are: + // - reachable from the introduction point + // - encountered during forward traversal. Note that this is not transitive like the + // two-step traversal described above: only kills encountered on exit via a backward + // edge are ignored. + // + // In our test suite, there are a couple of cases where kills are encountered while + // time-traveling, however as far as we can tell, always in cases where they would be + // unreachable. We have reason to believe that this is a property of the single-graph + // approach (but haven't proved it yet): + // - reachable kills while time-traveling would also be encountered via regular + // traversal + // - it makes _some_ sense to ignore unreachable kills, but subtleties around dead code + // in general need to be better thought through (like they were for NLLs). + // - ignoring kills is a conservative approximation: the loan is still live and could + // cause false positive errors at another place access. Soundness issues in this + // domain should look more like the absence of reachability instead. + // + // This is enough in practice to pass tests, and therefore is what we have implemented + // for now. + // + // FIXME: all of the above. Analyze potential unsoundness, possibly in concert with a + // borrowck implementation in a-mir-formality, fuzzing, or manually crafting + // counter-examples. + + // Continuing traversal will depend on whether the loan is killed at this point, and + // whether we're time-traveling. let current_location = liveness.location_from_point(node.point); let is_loan_killed = kills.get(¤t_location).is_some_and(|kills| kills.contains(&loan_idx)); for succ in outgoing_edges(&graph, node) { - // If the loan is killed at this point, it is killed _on exit_. + // If the loan is killed at this point, it is killed _on exit_. But only during + // forward traversal. if is_loan_killed { - continue; + let destination = liveness.location_from_point(succ.point); + if current_location.is_predecessor_of(destination, body) { + continue; + } } stack.push(succ); } @@ -130,6 +184,16 @@ impl<'tcx> KillsCollector<'_, 'tcx> { /// Records the borrows on the specified place as `killed`. For example, when assigning to a /// local, or on a call's return destination. fn record_killed_borrows_for_place(&mut self, place: Place<'tcx>, location: Location) { + // For the reasons described in graph traversal, we also filter out kills + // unreachable from the loan's introduction point, as they would stop traversal when + // e.g. checking for reachability in the subset graph through invariance constraints + // higher up. + let filter_unreachable_kills = |loan| { + let introduction = self.borrow_set[loan].reserve_location; + let reachable = introduction.is_predecessor_of(location, self.body); + reachable + }; + let other_borrows_of_local = self .borrow_set .local_map @@ -143,7 +207,10 @@ impl<'tcx> KillsCollector<'_, 'tcx> { // `places_conflict` for every borrow. if place.projection.is_empty() { if !self.body.local_decls[place.local].is_ref_to_static() { - self.kills.entry(location).or_default().extend(other_borrows_of_local); + self.kills + .entry(location) + .or_default() + .extend(other_borrows_of_local.filter(|&loan| filter_unreachable_kills(loan))); } return; } @@ -152,15 +219,17 @@ impl<'tcx> KillsCollector<'_, 'tcx> { // pair of array indices are not equal, so that when `places_conflict` returns true, we // will be assured that two places being compared definitely denotes the same sets of // locations. - let definitely_conflicting_borrows = other_borrows_of_local.filter(|&i| { - places_conflict( - self.tcx, - self.body, - self.borrow_set[i].borrowed_place, - place, - PlaceConflictBias::NoOverlap, - ) - }); + let definitely_conflicting_borrows = other_borrows_of_local + .filter(|&i| { + places_conflict( + self.tcx, + self.body, + self.borrow_set[i].borrowed_place, + place, + PlaceConflictBias::NoOverlap, + ) + }) + .filter(|&loan| filter_unreachable_kills(loan)); self.kills.entry(location).or_default().extend(definitely_conflicting_borrows); } From 735013d281c991187f327da0c660a026deb255de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 31 Dec 2024 17:39:00 +0000 Subject: [PATCH 06/12] remove location-insensitive ICE test --- tests/crashes/127628.rs | 14 -------------- 1 file changed, 14 deletions(-) delete mode 100644 tests/crashes/127628.rs diff --git a/tests/crashes/127628.rs b/tests/crashes/127628.rs deleted file mode 100644 index f11ab3f7e8d85..0000000000000 --- a/tests/crashes/127628.rs +++ /dev/null @@ -1,14 +0,0 @@ -//@ known-bug: #127628 -//@ compile-flags: -Zpolonius=next - -use std::io::{self, Read}; - -pub struct Container<'a> { - reader: &'a mut dyn Read, -} - -impl<'a> Container { - pub fn wrap<'s>(reader: &'s mut dyn io::Read) -> Container<'s> { - Container { reader: reader } - } -} From 8ac045dd4c1aad7eca07522a4dd9c22181674728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Fri, 10 Jan 2025 08:42:00 +0000 Subject: [PATCH 07/12] move out of scope precomputer code this addresses review comments while: - keeping the symmetry between the NLL and Polonius out of scope precomputers - keeping the unstable `calculate_borrows_out_of_scope_at_location` function to avoid churn for consumers --- compiler/rustc_borrowck/src/dataflow.rs | 66 ++++++++++++------------- 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/compiler/rustc_borrowck/src/dataflow.rs b/compiler/rustc_borrowck/src/dataflow.rs index 3a200642ce58c..7511a55b03ae5 100644 --- a/compiler/rustc_borrowck/src/dataflow.rs +++ b/compiler/rustc_borrowck/src/dataflow.rs @@ -187,19 +187,28 @@ struct OutOfScopePrecomputer<'a, 'tcx> { borrows_out_of_scope_at_location: FxIndexMap>, } -impl<'a, 'tcx> OutOfScopePrecomputer<'a, 'tcx> { - fn new(body: &'a Body<'tcx>, regioncx: &'a RegionInferenceContext<'tcx>) -> Self { - OutOfScopePrecomputer { +impl<'tcx> OutOfScopePrecomputer<'_, 'tcx> { + fn compute( + body: &Body<'tcx>, + regioncx: &RegionInferenceContext<'tcx>, + borrow_set: &BorrowSet<'tcx>, + ) -> FxIndexMap> { + let mut prec = OutOfScopePrecomputer { visited: DenseBitSet::new_empty(body.basic_blocks.len()), visit_stack: vec![], body, regioncx, borrows_out_of_scope_at_location: FxIndexMap::default(), + }; + for (borrow_index, borrow_data) in borrow_set.iter_enumerated() { + let borrow_region = borrow_data.region; + let location = borrow_data.reserve_location; + prec.precompute_borrows_out_of_scope(borrow_index, borrow_region, location); } + + prec.borrows_out_of_scope_at_location } -} -impl<'tcx> OutOfScopePrecomputer<'_, 'tcx> { fn precompute_borrows_out_of_scope( &mut self, borrow_index: BorrowIndex, @@ -280,15 +289,7 @@ pub fn calculate_borrows_out_of_scope_at_location<'tcx>( regioncx: &RegionInferenceContext<'tcx>, borrow_set: &BorrowSet<'tcx>, ) -> FxIndexMap> { - let mut prec = OutOfScopePrecomputer::new(body, regioncx); - for (borrow_index, borrow_data) in borrow_set.iter_enumerated() { - let borrow_region = borrow_data.region; - let location = borrow_data.reserve_location; - - prec.precompute_borrows_out_of_scope(borrow_index, borrow_region, location); - } - - prec.borrows_out_of_scope_at_location + OutOfScopePrecomputer::compute(body, regioncx, borrow_set) } struct PoloniusOutOfScopePrecomputer<'a, 'tcx> { @@ -300,19 +301,30 @@ struct PoloniusOutOfScopePrecomputer<'a, 'tcx> { loans_out_of_scope_at_location: FxIndexMap>, } -impl<'a, 'tcx> PoloniusOutOfScopePrecomputer<'a, 'tcx> { - fn new(body: &'a Body<'tcx>, regioncx: &'a RegionInferenceContext<'tcx>) -> Self { - Self { +impl<'tcx> PoloniusOutOfScopePrecomputer<'_, 'tcx> { + fn compute( + body: &Body<'tcx>, + regioncx: &RegionInferenceContext<'tcx>, + borrow_set: &BorrowSet<'tcx>, + ) -> FxIndexMap> { + // The in-tree polonius analysis computes loans going out of scope using the + // set-of-loans model. + let mut prec = PoloniusOutOfScopePrecomputer { visited: DenseBitSet::new_empty(body.basic_blocks.len()), visit_stack: vec![], body, regioncx, loans_out_of_scope_at_location: FxIndexMap::default(), + }; + for (loan_idx, loan_data) in borrow_set.iter_enumerated() { + let issuing_region = loan_data.region; + let loan_issued_at = loan_data.reserve_location; + prec.precompute_loans_out_of_scope(loan_idx, issuing_region, loan_issued_at); } + + prec.loans_out_of_scope_at_location } -} -impl<'tcx> PoloniusOutOfScopePrecomputer<'_, 'tcx> { /// Loans are in scope while they are live: whether they are contained within any live region. /// In the location-insensitive analysis, a loan will be contained in a region if the issuing /// region can reach it in the subset graph. So this is a reachability problem. @@ -464,21 +476,7 @@ impl<'a, 'tcx> Borrows<'a, 'tcx> { if !tcx.sess.opts.unstable_opts.polonius.is_next_enabled() { calculate_borrows_out_of_scope_at_location(body, regioncx, borrow_set) } else { - // The in-tree polonius analysis computes loans going out of scope using the - // set-of-loans model. - let mut polonius_prec = PoloniusOutOfScopePrecomputer::new(body, regioncx); - for (loan_idx, loan_data) in borrow_set.iter_enumerated() { - let issuing_region = loan_data.region; - let loan_issued_at = loan_data.reserve_location; - - polonius_prec.precompute_loans_out_of_scope( - loan_idx, - issuing_region, - loan_issued_at, - ); - } - - polonius_prec.loans_out_of_scope_at_location + PoloniusOutOfScopePrecomputer::compute(body, regioncx, borrow_set) }; Borrows { tcx, body, borrow_set, borrows_out_of_scope_at_location } } From f78a1bd89ade0d326a47222e51c61bd631530416 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 15 Jan 2025 22:09:52 +0000 Subject: [PATCH 08/12] Detect if-else chains with a missing final else in type errors ``` error[E0308]: `if` and `else` have incompatible types --> $DIR/if-else-chain-missing-else.rs:12:12 | LL | let x = if let Ok(x) = res { | ______________- LL | | x | | - expected because of this LL | | } else if let Err(e) = res { | | ____________^ LL | || return Err(e); LL | || }; | || ^ | ||_____| | |_____`if` and `else` have incompatible types | expected `i32`, found `()` | = note: `if` expressions without `else` evaluate to `()` = note: consider adding an `else` block that evaluates to the expected type ``` We probably want a longer explanation and fewer spans on this case. Partially address #133316. --- .../src/error_reporting/infer/mod.rs | 8 +++++++ .../ui/expr/if/if-else-chain-missing-else.rs | 20 +++++++++++++++++ .../expr/if/if-else-chain-missing-else.stderr | 22 +++++++++++++++++++ 3 files changed, 50 insertions(+) create mode 100644 tests/ui/expr/if/if-else-chain-missing-else.rs create mode 100644 tests/ui/expr/if/if-else-chain-missing-else.stderr diff --git a/compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs b/compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs index 7032f7b9d318e..9778299eb191f 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs @@ -620,6 +620,14 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { }) => { let then_span = self.find_block_span_from_hir_id(then_id); let else_span = self.find_block_span_from_hir_id(else_id); + if let hir::Node::Expr(e) = self.tcx.hir_node(else_id) + && let hir::ExprKind::If(_cond, _then, None) = e.kind + && else_ty.is_unit() + { + // Account for `let x = if a { 1 } else if b { 2 };` + err.note("`if` expressions without `else` evaluate to `()`"); + err.note("consider adding an `else` block that evaluates to the expected type"); + } err.span_label(then_span, "expected because of this"); if let Some(sp) = outer_span { err.span_label(sp, "`if` and `else` have incompatible types"); diff --git a/tests/ui/expr/if/if-else-chain-missing-else.rs b/tests/ui/expr/if/if-else-chain-missing-else.rs new file mode 100644 index 0000000000000..995aac07f2f76 --- /dev/null +++ b/tests/ui/expr/if/if-else-chain-missing-else.rs @@ -0,0 +1,20 @@ +enum Cause { Cause1, Cause2 } +struct MyErr { x: Cause } + +fn main() { + _ = f(); +} + +fn f() -> Result { + let res = could_fail(); + let x = if let Ok(x) = res { + x + } else if let Err(e) = res { //~ ERROR `if` and `else` + return Err(e); + }; + Ok(x) +} + +fn could_fail() -> Result { + Ok(0) +} diff --git a/tests/ui/expr/if/if-else-chain-missing-else.stderr b/tests/ui/expr/if/if-else-chain-missing-else.stderr new file mode 100644 index 0000000000000..374c4927e3003 --- /dev/null +++ b/tests/ui/expr/if/if-else-chain-missing-else.stderr @@ -0,0 +1,22 @@ +error[E0308]: `if` and `else` have incompatible types + --> $DIR/if-else-chain-missing-else.rs:12:12 + | +LL | let x = if let Ok(x) = res { + | ______________- +LL | | x + | | - expected because of this +LL | | } else if let Err(e) = res { + | | ____________^ +LL | || return Err(e); +LL | || }; + | || ^ + | ||_____| + | |_____`if` and `else` have incompatible types + | expected `i32`, found `()` + | + = note: `if` expressions without `else` evaluate to `()` + = note: consider adding an `else` block that evaluates to the expected type + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0308`. From 9397d133f683070863029552313b5e083398bf42 Mon Sep 17 00:00:00 2001 From: binarycat Date: Mon, 13 Jan 2025 15:31:15 -0600 Subject: [PATCH 09/12] Treat other items as functions for the purpose of type-based search constants and statics are nullary functions, and struct fields are unary functions. functions (along with methods and trait methods) are prioritized over other items, like fields and constants. --- .../rustdoc/src/read-documentation/search.md | 14 +++++++++- src/librustdoc/html/render/search_index.rs | 27 +++++++++++++++++++ src/librustdoc/html/static/js/search.js | 16 +++++++++++ tests/rustdoc-gui/search-tab.goml | 2 +- tests/rustdoc-js-std/const-is-nullary-func.js | 7 +++++ tests/rustdoc-js-std/field-is-unary-func.js | 7 +++++ 6 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 tests/rustdoc-js-std/const-is-nullary-func.js create mode 100644 tests/rustdoc-js-std/field-is-unary-func.js diff --git a/src/doc/rustdoc/src/read-documentation/search.md b/src/doc/rustdoc/src/read-documentation/search.md index e06dcdb7ed2fb..bace2f5f95390 100644 --- a/src/doc/rustdoc/src/read-documentation/search.md +++ b/src/doc/rustdoc/src/read-documentation/search.md @@ -52,9 +52,10 @@ methods on the allocator or free functions. [`Layout`]: ../../alloc/index.html?search=Layout&filter-crate=alloc -## Searching By Type Signature for functions +## Searching By Type Signature If you know more specifically what the function you want to look at does, +or you want to know how to get from one type to another, Rustdoc can search by more than one type at once in the parameters and return value. Multiple parameters are separated by `,` commas, and the return value is written with after a `->` arrow. @@ -86,6 +87,17 @@ the standard library and functions that are included in the results list: [iterasslice]: ../../std/vec/struct.Vec.html?search=vec%3A%3Aintoiter%20->%20[T]&filter-crate=std [iterreduce]: ../../std/index.html?search=iterator%2C%20fnmut%20->%20T&filter-crate=std +### Non-functions in type-based search +Certain items that are not functions are treated as though they +were a semantically equivelent function. + +For example, struct fields are treated as though they were getter methods. +This means that a search for `CpuidResult -> u32` will show +the `CpuidResult::eax` field in the results. + +Additionally, `const` and `static` items are treated as nullary functions, +so `-> u32` will match `u32::MAX`. + ### How type-based search works In a complex type-based search, Rustdoc always treats every item's name as literal. diff --git a/src/librustdoc/html/render/search_index.rs b/src/librustdoc/html/render/search_index.rs index 66c52bec4bad7..e4a9a2b512e50 100644 --- a/src/librustdoc/html/render/search_index.rs +++ b/src/librustdoc/html/render/search_index.rs @@ -840,6 +840,22 @@ pub(crate) fn get_function_type_for_search( | clean::RequiredMethodItem(ref f) => { get_fn_inputs_and_outputs(f, tcx, impl_or_trait_generics, cache) } + clean::ConstantItem(ref c) => make_nullary_fn(&c.type_), + clean::StaticItem(ref s) => make_nullary_fn(&s.type_), + clean::StructFieldItem(ref t) => { + let Some(parent) = parent else { + return None; + }; + let mut rgen: FxIndexMap)> = + Default::default(); + let output = get_index_type(t, vec![], &mut rgen); + let input = RenderType { + id: Some(RenderTypeId::DefId(parent)), + generics: None, + bindings: None, + }; + (vec![input], vec![output], vec![], vec![]) + } _ => return None, }; @@ -1353,6 +1369,17 @@ fn simplify_fn_constraint<'a>( res.push((ty_constrained_assoc, ty_constraints)); } +/// Create a fake nullary function. +/// +/// Used to allow type-based search on constants and statics. +fn make_nullary_fn( + clean_type: &clean::Type, +) -> (Vec, Vec, Vec, Vec>) { + let mut rgen: FxIndexMap)> = Default::default(); + let output = get_index_type(clean_type, vec![], &mut rgen); + (vec![], vec![output], vec![], vec![]) +} + /// Return the full list of types when bounds have been resolved. /// /// i.e. `fn foo>(x: u32, y: B)` will return diff --git a/src/librustdoc/html/static/js/search.js b/src/librustdoc/html/static/js/search.js index 0a0550ab82f9b..660484c133c36 100644 --- a/src/librustdoc/html/static/js/search.js +++ b/src/librustdoc/html/static/js/search.js @@ -63,6 +63,9 @@ const TY_PRIMITIVE = itemTypes.indexOf("primitive"); const TY_GENERIC = itemTypes.indexOf("generic"); const TY_IMPORT = itemTypes.indexOf("import"); const TY_TRAIT = itemTypes.indexOf("trait"); +const TY_FN = itemTypes.indexOf("fn"); +const TY_METHOD = itemTypes.indexOf("method"); +const TY_TYMETHOD = itemTypes.indexOf("tymethod"); const ROOT_PATH = typeof window !== "undefined" ? window.rootPath : "../"; // Hard limit on how deep to recurse into generics when doing type-driven search. @@ -191,6 +194,10 @@ function isEndCharacter(c) { return "=,>-])".indexOf(c) !== -1; } +function isFnLikeTy(ty) { + return ty === TY_FN || ty === TY_METHOD || ty === TY_TYMETHOD; +} + /** * Returns `true` if the given `c` character is a separator. * @@ -2766,6 +2773,15 @@ class DocSearch { return a - b; } + // in type based search, put functions first + if (parsedQuery.hasReturnArrow) { + a = !isFnLikeTy(aaa.item.ty); + b = !isFnLikeTy(bbb.item.ty); + if (a !== b) { + return a - b; + } + } + // Sort by distance in the path part, if specified // (less changes required to match means higher rankings) a = aaa.path_dist; diff --git a/tests/rustdoc-gui/search-tab.goml b/tests/rustdoc-gui/search-tab.goml index 3879c127fd0f8..eea561e0c6760 100644 --- a/tests/rustdoc-gui/search-tab.goml +++ b/tests/rustdoc-gui/search-tab.goml @@ -79,7 +79,7 @@ set-window-size: (851, 600) // Check the size and count in tabs assert-text: ("#search-tabs > button:nth-child(1) > .count", " (26) ") -assert-text: ("#search-tabs > button:nth-child(2) > .count", " (6)  ") +assert-text: ("#search-tabs > button:nth-child(2) > .count", " (7)  ") assert-text: ("#search-tabs > button:nth-child(3) > .count", " (0)  ") store-property: ("#search-tabs > button:nth-child(1)", {"offsetWidth": buttonWidth}) assert-property: ("#search-tabs > button:nth-child(2)", {"offsetWidth": |buttonWidth|}) diff --git a/tests/rustdoc-js-std/const-is-nullary-func.js b/tests/rustdoc-js-std/const-is-nullary-func.js new file mode 100644 index 0000000000000..e929741b038f4 --- /dev/null +++ b/tests/rustdoc-js-std/const-is-nullary-func.js @@ -0,0 +1,7 @@ +const EXPECTED = { + 'query': '-> char', + 'others': [ + { 'path': 'std::char', 'name': 'from_digit' }, + { 'path': 'std::char', 'name': 'MAX' }, + ], +} diff --git a/tests/rustdoc-js-std/field-is-unary-func.js b/tests/rustdoc-js-std/field-is-unary-func.js new file mode 100644 index 0000000000000..09ce8a0dde0a4 --- /dev/null +++ b/tests/rustdoc-js-std/field-is-unary-func.js @@ -0,0 +1,7 @@ +const EXPECTED = { + // one of the only non-generic structs with public fields + 'query': 'CpuidResult -> u32', + 'others': [ + { 'path': 'core::arch::x86::CpuidResult', 'name': 'eax' }, + ], +} From 08c1256dc27e4af07940440252e6356d5e39c8d8 Mon Sep 17 00:00:00 2001 From: binarycat Date: Thu, 16 Jan 2025 12:10:38 -0600 Subject: [PATCH 10/12] fix error for when results in a rustdoc-js test are in the wrong order see COMPILETEST_FORCE_STAGE0=1 --- src/tools/rustdoc-js/tester.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/rustdoc-js/tester.js b/src/tools/rustdoc-js/tester.js index 7aa5e102e6d2a..ea575f27799a9 100644 --- a/src/tools/rustdoc-js/tester.js +++ b/src/tools/rustdoc-js/tester.js @@ -256,7 +256,7 @@ async function runSearch(query, expected, doSearch, loadedFile, queryName) { JSON.stringify(results[key][index]) + "'"); } else if (ignore_order === false && entry_pos < prev_pos) { error_text.push(queryName + "==> '" + JSON.stringify(elem) + "' was supposed " + - "to be before '" + JSON.stringify(results[key][entry_pos]) + "'"); + "to be before '" + JSON.stringify(results[key][prev_pos]) + "'"); } else { prev_pos = entry_pos; } From 8d59545daa879365e8bab002b1c7f8a09c62d41d Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Wed, 15 Jan 2025 20:50:51 +0100 Subject: [PATCH 11/12] Fix suggestion to convert dereference of raw pointer to ref --- .../src/fn_ctxt/suggestions.rs | 1 + tests/ui/suggestions/raw-to-ref.fixed | 19 ++++++++++++++ tests/ui/suggestions/raw-to-ref.rs | 19 ++++++++++++++ tests/ui/suggestions/raw-to-ref.stderr | 25 +++++++++++++++++++ 4 files changed, 64 insertions(+) create mode 100644 tests/ui/suggestions/raw-to-ref.fixed create mode 100644 tests/ui/suggestions/raw-to-ref.rs create mode 100644 tests/ui/suggestions/raw-to-ref.stderr diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index 39511ca30e0cf..53e055fdeef44 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -2682,6 +2682,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if let hir::ExprKind::Unary(hir::UnOp::Deref, inner) = expr.kind && let Some(1) = self.deref_steps_for_suggestion(expected, checked_ty) + && self.typeck_results.borrow().expr_ty(inner).is_ref() { // We have `*&T`, check if what was expected was `&T`. // If so, we may want to suggest removing a `*`. diff --git a/tests/ui/suggestions/raw-to-ref.fixed b/tests/ui/suggestions/raw-to-ref.fixed new file mode 100644 index 0000000000000..17d61e67e1f61 --- /dev/null +++ b/tests/ui/suggestions/raw-to-ref.fixed @@ -0,0 +1,19 @@ +//@ run-rustfix +// Regression test for #135580: check that we do not suggest to simply drop +// the `*` to make the types match when the source is a raw pointer while +// the target type is a reference. + +struct S; + +fn main() { + let mut s = S; + let x = &raw const s; + let _: &S = unsafe { &*x }; + //~^ ERROR mismatched types + //~| HELP consider borrowing here + + let x = &raw mut s; + let _: &mut S = unsafe { &mut *x }; + //~^ ERROR mismatched types + //~| HELP consider mutably borrowing here +} diff --git a/tests/ui/suggestions/raw-to-ref.rs b/tests/ui/suggestions/raw-to-ref.rs new file mode 100644 index 0000000000000..2be8f881b5c3a --- /dev/null +++ b/tests/ui/suggestions/raw-to-ref.rs @@ -0,0 +1,19 @@ +//@ run-rustfix +// Regression test for #135580: check that we do not suggest to simply drop +// the `*` to make the types match when the source is a raw pointer while +// the target type is a reference. + +struct S; + +fn main() { + let mut s = S; + let x = &raw const s; + let _: &S = unsafe { *x }; + //~^ ERROR mismatched types + //~| HELP consider borrowing here + + let x = &raw mut s; + let _: &mut S = unsafe { *x }; + //~^ ERROR mismatched types + //~| HELP consider mutably borrowing here +} diff --git a/tests/ui/suggestions/raw-to-ref.stderr b/tests/ui/suggestions/raw-to-ref.stderr new file mode 100644 index 0000000000000..ca358d268f018 --- /dev/null +++ b/tests/ui/suggestions/raw-to-ref.stderr @@ -0,0 +1,25 @@ +error[E0308]: mismatched types + --> $DIR/raw-to-ref.rs:11:26 + | +LL | let _: &S = unsafe { *x }; + | ^^ expected `&S`, found `S` + | +help: consider borrowing here + | +LL | let _: &S = unsafe { &*x }; + | + + +error[E0308]: mismatched types + --> $DIR/raw-to-ref.rs:16:30 + | +LL | let _: &mut S = unsafe { *x }; + | ^^ expected `&mut S`, found `S` + | +help: consider mutably borrowing here + | +LL | let _: &mut S = unsafe { &mut *x }; + | ++++ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0308`. From 9bdc65866ced6531e988674324ffc72af817c880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 16 Jan 2025 23:53:49 +0000 Subject: [PATCH 12/12] Expand docs for `E0207` with additional example --- .../src/error_codes/E0207.md | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/compiler/rustc_error_codes/src/error_codes/E0207.md b/compiler/rustc_error_codes/src/error_codes/E0207.md index 95e7c9fc76ce2..5b35748f4723c 100644 --- a/compiler/rustc_error_codes/src/error_codes/E0207.md +++ b/compiler/rustc_error_codes/src/error_codes/E0207.md @@ -195,6 +195,30 @@ impl<'a> Contains for Foo { Please note that unconstrained lifetime parameters are not supported if they are being used by an associated type. +In cases where the associated type's lifetime is meant to be tied to the the +self type, and none of the methods on the trait need ownership or different +mutability, then an option is to implement the trait on a borrowed type: + +```rust +struct Foo(i32); + +trait Contents { + type Item; + + fn get(&self) -> Self::Item; +} + +// Note the lifetime `'a` is used both for the self type... +impl<'a> Contents for &'a Foo { + // ...and the associated type. + type Item = &'a i32; + + fn get(&self) -> Self::Item { + &self.0 + } +} +``` + ### Additional information For more information, please see [RFC 447].