Rollup merge of #143494 - cjgillot:no-yield-in-scope, r=compiler-errors

Remove yields_in_scope from the scope tree.

I believe this has not been in use since we removed the HIR-based generator interior type computation.

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 c458878da15..288105dfba7 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<Scope>,
     // 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 92eab59dd02..0f5b63f5c1d 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<hir::ItemLocalId, Scope>,
-
-    /// 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<T>` 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<Scope, Vec<YieldData>>,
 }
 
 /// See the `rvalue_candidates` field for more information on rvalue
@@ -316,15 +239,6 @@ pub struct RvalueCandidate {
     pub lifetime: Option<Scope>,
 }
 
-#[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<Scope>) {
         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 975226bb642..daf8fa5f19e 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<T>` 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)));