From 39ee1b2d774d4be6959e3b045052abef54a29e0b Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 5 Jul 2025 15:11:09 +0000 Subject: [PATCH] Remove yields_in_scope from the scope tree. --- .../rustc_hir_analysis/src/check/region.rs | 205 ++++-------------- compiler/rustc_middle/src/middle/region.rs | 92 -------- .../src/builder/expr/as_rvalue.rs | 3 - 3 files changed, 37 insertions(+), 263 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index c458878da15b5..288105dfba714 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -15,7 +15,6 @@ use rustc_hir::def_id::DefId; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::{Arm, Block, Expr, LetStmt, Pat, PatKind, Stmt}; use rustc_index::Idx; -use rustc_middle::bug; use rustc_middle::middle::region::*; use rustc_middle::ty::TyCtxt; use rustc_session::lint; @@ -34,14 +33,6 @@ struct Context { struct ScopeResolutionVisitor<'tcx> { tcx: TyCtxt<'tcx>, - // The number of expressions and patterns visited in the current body. - expr_and_pat_count: usize, - // When this is `true`, we record the `Scopes` we encounter - // when processing a Yield expression. This allows us to fix - // up their indices. - pessimistic_yield: bool, - // Stores scopes when `pessimistic_yield` is `true`. - fixup_scopes: Vec, // The generated scope tree. scope_tree: ScopeTree, @@ -199,19 +190,14 @@ fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir: visitor.cx = prev_cx; } +#[tracing::instrument(level = "debug", skip(visitor))] fn resolve_pat<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, pat: &'tcx hir::Pat<'tcx>) { // If this is a binding then record the lifetime of that binding. if let PatKind::Binding(..) = pat.kind { record_var_lifetime(visitor, pat.hir_id.local_id); } - debug!("resolve_pat - pre-increment {} pat = {:?}", visitor.expr_and_pat_count, pat); - intravisit::walk_pat(visitor, pat); - - visitor.expr_and_pat_count += 1; - - debug!("resolve_pat - post-increment {} pat = {:?}", visitor.expr_and_pat_count, pat); } fn resolve_stmt<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, stmt: &'tcx hir::Stmt<'tcx>) { @@ -243,68 +229,15 @@ fn resolve_stmt<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, stmt: &'tcx hi } } +#[tracing::instrument(level = "debug", skip(visitor))] fn resolve_expr<'tcx>( visitor: &mut ScopeResolutionVisitor<'tcx>, expr: &'tcx hir::Expr<'tcx>, terminating: bool, ) { - debug!("resolve_expr - pre-increment {} expr = {:?}", visitor.expr_and_pat_count, expr); - let prev_cx = visitor.cx; visitor.enter_node_scope_with_dtor(expr.hir_id.local_id, terminating); - let prev_pessimistic = visitor.pessimistic_yield; - - // Ordinarily, we can rely on the visit order of HIR intravisit - // to correspond to the actual execution order of statements. - // However, there's a weird corner case with compound assignment - // operators (e.g. `a += b`). The evaluation order depends on whether - // or not the operator is overloaded (e.g. whether or not a trait - // like AddAssign is implemented). - - // For primitive types (which, despite having a trait impl, don't actually - // end up calling it), the evaluation order is right-to-left. For example, - // the following code snippet: - // - // let y = &mut 0; - // *{println!("LHS!"); y} += {println!("RHS!"); 1}; - // - // will print: - // - // RHS! - // LHS! - // - // However, if the operator is used on a non-primitive type, - // the evaluation order will be left-to-right, since the operator - // actually get desugared to a method call. For example, this - // nearly identical code snippet: - // - // let y = &mut String::new(); - // *{println!("LHS String"); y} += {println!("RHS String"); "hi"}; - // - // will print: - // LHS String - // RHS String - // - // To determine the actual execution order, we need to perform - // trait resolution. Unfortunately, we need to be able to compute - // yield_in_scope before type checking is even done, as it gets - // used by AST borrowcheck. - // - // Fortunately, we don't need to know the actual execution order. - // It suffices to know the 'worst case' order with respect to yields. - // Specifically, we need to know the highest 'expr_and_pat_count' - // that we could assign to the yield expression. To do this, - // we pick the greater of the two values from the left-hand - // and right-hand expressions. This makes us overly conservative - // about what types could possibly live across yield points, - // but we will never fail to detect that a type does actually - // live across a yield point. The latter part is critical - - // we're already overly conservative about what types will live - // across yield points, as the generated MIR will determine - // when things are actually live. However, for typecheck to work - // properly, we can't miss any types. - match expr.kind { // Conditional or repeating scopes are always terminating // scopes, meaning that temporaries cannot outlive them. @@ -360,55 +293,42 @@ fn resolve_expr<'tcx>( let body = visitor.tcx.hir_body(body); visitor.visit_body(body); } + // Ordinarily, we can rely on the visit order of HIR intravisit + // to correspond to the actual execution order of statements. + // However, there's a weird corner case with compound assignment + // operators (e.g. `a += b`). The evaluation order depends on whether + // or not the operator is overloaded (e.g. whether or not a trait + // like AddAssign is implemented). + // + // For primitive types (which, despite having a trait impl, don't actually + // end up calling it), the evaluation order is right-to-left. For example, + // the following code snippet: + // + // let y = &mut 0; + // *{println!("LHS!"); y} += {println!("RHS!"); 1}; + // + // will print: + // + // RHS! + // LHS! + // + // However, if the operator is used on a non-primitive type, + // the evaluation order will be left-to-right, since the operator + // actually get desugared to a method call. For example, this + // nearly identical code snippet: + // + // let y = &mut String::new(); + // *{println!("LHS String"); y} += {println!("RHS String"); "hi"}; + // + // will print: + // LHS String + // RHS String + // + // To determine the actual execution order, we need to perform + // trait resolution. Fortunately, we don't need to know the actual execution order. hir::ExprKind::AssignOp(_, left_expr, right_expr) => { - debug!( - "resolve_expr - enabling pessimistic_yield, was previously {}", - prev_pessimistic - ); - - let start_point = visitor.fixup_scopes.len(); - visitor.pessimistic_yield = true; - - // If the actual execution order turns out to be right-to-left, - // then we're fine. However, if the actual execution order is left-to-right, - // then we'll assign too low a count to any `yield` expressions - // we encounter in 'right_expression' - they should really occur after all of the - // expressions in 'left_expression'. visitor.visit_expr(right_expr); - visitor.pessimistic_yield = prev_pessimistic; - - debug!("resolve_expr - restoring pessimistic_yield to {}", prev_pessimistic); visitor.visit_expr(left_expr); - debug!("resolve_expr - fixing up counts to {}", visitor.expr_and_pat_count); - - // Remove and process any scopes pushed by the visitor - let target_scopes = visitor.fixup_scopes.drain(start_point..); - - for scope in target_scopes { - let yield_data = - visitor.scope_tree.yield_in_scope.get_mut(&scope).unwrap().last_mut().unwrap(); - let count = yield_data.expr_and_pat_count; - let span = yield_data.span; - - // expr_and_pat_count never decreases. Since we recorded counts in yield_in_scope - // before walking the left-hand side, it should be impossible for the recorded - // count to be greater than the left-hand side count. - if count > visitor.expr_and_pat_count { - bug!( - "Encountered greater count {} at span {:?} - expected no greater than {}", - count, - span, - visitor.expr_and_pat_count - ); - } - let new_count = visitor.expr_and_pat_count; - debug!( - "resolve_expr - increasing count for scope {:?} from {} to {} at span {:?}", - scope, count, new_count, span - ); - - yield_data.expr_and_pat_count = new_count; - } } hir::ExprKind::If(cond, then, Some(otherwise)) => { @@ -453,43 +373,6 @@ fn resolve_expr<'tcx>( _ => intravisit::walk_expr(visitor, expr), } - visitor.expr_and_pat_count += 1; - - debug!("resolve_expr post-increment {}, expr = {:?}", visitor.expr_and_pat_count, expr); - - if let hir::ExprKind::Yield(_, source) = &expr.kind { - // Mark this expr's scope and all parent scopes as containing `yield`. - let mut scope = Scope { local_id: expr.hir_id.local_id, data: ScopeData::Node }; - loop { - let data = YieldData { - span: expr.span, - expr_and_pat_count: visitor.expr_and_pat_count, - source: *source, - }; - match visitor.scope_tree.yield_in_scope.get_mut(&scope) { - Some(yields) => yields.push(data), - None => { - visitor.scope_tree.yield_in_scope.insert(scope, vec![data]); - } - } - - if visitor.pessimistic_yield { - debug!("resolve_expr in pessimistic_yield - marking scope {:?} for fixup", scope); - visitor.fixup_scopes.push(scope); - } - - // Keep traversing up while we can. - match visitor.scope_tree.parent_map.get(&scope) { - // Don't cross from closure bodies to their parent. - Some(&superscope) => match superscope.data { - ScopeData::CallSite => break, - _ => scope = superscope, - }, - None => break, - } - } - } - visitor.cx = prev_cx; } @@ -612,8 +495,8 @@ fn resolve_local<'tcx>( } } - // Make sure we visit the initializer first, so expr_and_pat_count remains correct. - // The correct order, as shared between coroutine_interior, drop_ranges and intravisitor, + // Make sure we visit the initializer first. + // The correct order, as shared between drop_ranges and intravisitor, // is to walk initializer, followed by pattern bindings, finally followed by the `else` block. if let Some(expr) = init { visitor.visit_expr(expr); @@ -798,16 +681,7 @@ impl<'tcx> ScopeResolutionVisitor<'tcx> { } fn enter_body(&mut self, hir_id: hir::HirId, f: impl FnOnce(&mut Self)) { - // Save all state that is specific to the outer function - // body. These will be restored once down below, once we've - // visited the body. - let outer_ec = mem::replace(&mut self.expr_and_pat_count, 0); let outer_cx = self.cx; - // The 'pessimistic yield' flag is set to true when we are - // processing a `+=` statement and have to make pessimistic - // control flow assumptions. This doesn't apply to nested - // bodies within the `+=` statements. See #69307. - let outer_pessimistic_yield = mem::replace(&mut self.pessimistic_yield, false); self.enter_scope(Scope { local_id: hir_id.local_id, data: ScopeData::CallSite }); self.enter_scope(Scope { local_id: hir_id.local_id, data: ScopeData::Arguments }); @@ -815,9 +689,7 @@ impl<'tcx> ScopeResolutionVisitor<'tcx> { f(self); // Restore context we had at the start. - self.expr_and_pat_count = outer_ec; self.cx = outer_cx; - self.pessimistic_yield = outer_pessimistic_yield; } } @@ -919,10 +791,7 @@ pub(crate) fn region_scope_tree(tcx: TyCtxt<'_>, def_id: DefId) -> &ScopeTree { let mut visitor = ScopeResolutionVisitor { tcx, scope_tree: ScopeTree::default(), - expr_and_pat_count: 0, cx: Context { parent: None, var_parent: None }, - pessimistic_yield: false, - fixup_scopes: vec![], extended_super_lets: Default::default(), }; diff --git a/compiler/rustc_middle/src/middle/region.rs b/compiler/rustc_middle/src/middle/region.rs index 92eab59dd0274..0f5b63f5c1d24 100644 --- a/compiler/rustc_middle/src/middle/region.rs +++ b/compiler/rustc_middle/src/middle/region.rs @@ -7,7 +7,6 @@ //! [rustc dev guide]: https://rustc-dev-guide.rust-lang.org/borrow_check.html use std::fmt; -use std::ops::Deref; use rustc_data_structures::fx::FxIndexMap; use rustc_data_structures::unord::UnordMap; @@ -228,82 +227,6 @@ pub struct ScopeTree { /// This information is used later for linting to identify locals and /// temporary values that will receive backwards-incompatible drop orders. pub backwards_incompatible_scope: UnordMap, - - /// If there are any `yield` nested within a scope, this map - /// stores the `Span` of the last one and its index in the - /// postorder of the Visitor traversal on the HIR. - /// - /// HIR Visitor postorder indexes might seem like a peculiar - /// thing to care about. but it turns out that HIR bindings - /// and the temporary results of HIR expressions are never - /// storage-live at the end of HIR nodes with postorder indexes - /// lower than theirs, and therefore don't need to be suspended - /// at yield-points at these indexes. - /// - /// For an example, suppose we have some code such as: - /// ```rust,ignore (example) - /// foo(f(), yield y, bar(g())) - /// ``` - /// - /// With the HIR tree (calls numbered for expository purposes) - /// - /// ```text - /// Call#0(foo, [Call#1(f), Yield(y), Call#2(bar, Call#3(g))]) - /// ``` - /// - /// Obviously, the result of `f()` was created before the yield - /// (and therefore needs to be kept valid over the yield) while - /// the result of `g()` occurs after the yield (and therefore - /// doesn't). If we want to infer that, we can look at the - /// postorder traversal: - /// ```plain,ignore - /// `foo` `f` Call#1 `y` Yield `bar` `g` Call#3 Call#2 Call#0 - /// ``` - /// - /// In which we can easily see that `Call#1` occurs before the yield, - /// and `Call#3` after it. - /// - /// To see that this method works, consider: - /// - /// Let `D` be our binding/temporary and `U` be our other HIR node, with - /// `HIR-postorder(U) < HIR-postorder(D)`. Suppose, as in our example, - /// U is the yield and D is one of the calls. - /// Let's show that `D` is storage-dead at `U`. - /// - /// Remember that storage-live/storage-dead refers to the state of - /// the *storage*, and does not consider moves/drop flags. - /// - /// Then: - /// - /// 1. From the ordering guarantee of HIR visitors (see - /// `rustc_hir::intravisit`), `D` does not dominate `U`. - /// - /// 2. Therefore, `D` is *potentially* storage-dead at `U` (because - /// we might visit `U` without ever getting to `D`). - /// - /// 3. However, we guarantee that at each HIR point, each - /// binding/temporary is always either always storage-live - /// or always storage-dead. This is what is being guaranteed - /// by `terminating_scopes` including all blocks where the - /// count of executions is not guaranteed. - /// - /// 4. By `2.` and `3.`, `D` is *statically* storage-dead at `U`, - /// QED. - /// - /// This property ought to not on (3) in an essential way -- it - /// is probably still correct even if we have "unrestricted" terminating - /// scopes. However, why use the complicated proof when a simple one - /// works? - /// - /// A subtle thing: `box` expressions, such as `box (&x, yield 2, &y)`. It - /// might seem that a `box` expression creates a `Box` temporary - /// when it *starts* executing, at `HIR-preorder(BOX-EXPR)`. That might - /// be true in the MIR desugaring, but it is not important in the semantics. - /// - /// The reason is that semantically, until the `box` expression returns, - /// the values are still owned by their containing expressions. So - /// we'll see that `&x`. - pub yield_in_scope: UnordMap>, } /// See the `rvalue_candidates` field for more information on rvalue @@ -316,15 +239,6 @@ pub struct RvalueCandidate { pub lifetime: Option, } -#[derive(Debug, Copy, Clone, HashStable)] -pub struct YieldData { - /// The `Span` of the yield. - pub span: Span, - /// The number of expressions and patterns appearing before the `yield` in the body, plus one. - pub expr_and_pat_count: usize, - pub source: hir::YieldSource, -} - impl ScopeTree { pub fn record_scope_parent(&mut self, child: Scope, parent: Option) { debug!("{:?}.parent = {:?}", child, parent); @@ -380,10 +294,4 @@ impl ScopeTree { true } - - /// Checks whether the given scope contains a `yield`. If so, - /// returns `Some(YieldData)`. If not, returns `None`. - pub fn yield_in_scope(&self, scope: Scope) -> Option<&[YieldData]> { - self.yield_in_scope.get(&scope).map(Deref::deref) - } } diff --git a/compiler/rustc_mir_build/src/builder/expr/as_rvalue.rs b/compiler/rustc_mir_build/src/builder/expr/as_rvalue.rs index 975226bb642a7..daf8fa5f19eee 100644 --- a/compiler/rustc_mir_build/src/builder/expr/as_rvalue.rs +++ b/compiler/rustc_mir_build/src/builder/expr/as_rvalue.rs @@ -171,9 +171,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { this.diverge_from(block); block = success; - // The `Box` temporary created here is not a part of the HIR, - // and therefore is not considered during coroutine auto-trait - // determination. See the comment about `box` at `yield_in_scope`. let result = this.local_decls.push(LocalDecl::new(expr.ty, expr_span)); this.cfg .push(block, Statement::new(source_info, StatementKind::StorageLive(result)));