From 9d0960a6f8fbd561b2f5d9fb54ba49d533f61832 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 5 Jun 2019 19:54:34 -0400 Subject: [PATCH 1/6] Fix HIR visit order Fixes #61442 When rustc::middle::region::ScopeTree ccomputes its yield_in_scope field, it relies on the HIR visitor order to properly compute which types must be live across yield points. In order for the computed scopes to agree with the generated MIR, we must ensure that expressions evaluated before a yield point are visited before the 'yield' expression. However, the visitor order for ExprKind::AssignOp was incorrect. The left-hand side of a compund assignment expression is evaluated before the right-hand side, but the right-hand expression was being visited before the left-hand expression. If the left-hand expression caused a new type to be introduced (e.g. through a deref-coercion), the new type would be incorrectly seen as occuring *after* the yield point, instead of before. This leads to a mismatch between the computed generator types and the MIR, since the MIR will correctly see the type as being live across the yield point. To fix this, we correct the visitor order for ExprKind::AssignOp to reflect the actual evaulation order. --- src/librustc/hir/intravisit.rs | 2 +- src/test/run-pass/issues/issue-61442.rs | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 src/test/run-pass/issues/issue-61442.rs diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs index 1777d765cc8a6..05bef951ddbdd 100644 --- a/src/librustc/hir/intravisit.rs +++ b/src/librustc/hir/intravisit.rs @@ -1055,8 +1055,8 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr) { visitor.visit_expr(left_hand_expression) } ExprKind::AssignOp(_, ref left_expression, ref right_expression) => { + visitor.visit_expr(left_expression); visitor.visit_expr(right_expression); - visitor.visit_expr(left_expression) } ExprKind::Field(ref subexpression, ident) => { visitor.visit_expr(subexpression); diff --git a/src/test/run-pass/issues/issue-61442.rs b/src/test/run-pass/issues/issue-61442.rs new file mode 100644 index 0000000000000..89d1dac08bc10 --- /dev/null +++ b/src/test/run-pass/issues/issue-61442.rs @@ -0,0 +1,12 @@ +#![feature(generators)] + +fn foo() { + let _x = static || { + let mut s = String::new(); + s += { yield; "" }; + }; +} + +fn main() { + foo() +} From 0fa945e184c9903ed204a7aa49dac9f9da77ae42 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 6 Jun 2019 22:23:28 -0400 Subject: [PATCH 2/6] Change how we compute yield_in_scope Compound operators (e.g. 'a += b') have two different possible evaluation orders. When the left-hand side is a primitive type, the expression is evaluated right-to-left. However, when the left-hand side is a non-primitive type, the expression is evaluated left-to-right. This causes problems when we try to determine if a type is live across a yield point. Since we need to perform this computation before typecheck has run, we can't simply check the types of the operands. This commit calculates the most 'pessimistic' scenario - that is, erring on the side of treating more types as live, rather than fewer. This is perfectly safe - in fact, this initial liveness computation is already overly conservative (e.g. issue #57478). The important thing is that we compute a superset of the types that are actually live across yield points. When we generate MIR, we'll determine which types actually need to stay live across a given yield point, and which ones cam actually be dropped. Concretely, we force the computed HIR traversal index for right-hand-side yield expression to be equal to the maximum index for the left-hand side. This covers both possible execution orders: * If the expression is evalauted right-to-left, our 'pessismitic' index is unecessary, but safe. We visit the expressions in an ExprKind::AssignOp from right to left, so it actually would have been safe to do nothing. However, while increasing the index of a yield point might cause the compiler to reject code that could actually compile, it will never cause incorrect code to be accepted. * If the expression is evaluated left-to-right, our 'pessimistic' index correctly ensures that types in the left-hand-side are seen as occuring before the yield - which is exactly what we want --- src/librustc/hir/intravisit.rs | 2 +- src/librustc/middle/region.rs | 115 ++++++++++++++++++ .../check/generator_interior.rs | 6 + src/test/run-pass/issues/issue-61442.rs | 16 +++ 4 files changed, 138 insertions(+), 1 deletion(-) diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs index 05bef951ddbdd..9c05f18762df1 100644 --- a/src/librustc/hir/intravisit.rs +++ b/src/librustc/hir/intravisit.rs @@ -1055,8 +1055,8 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr) { visitor.visit_expr(left_hand_expression) } ExprKind::AssignOp(_, ref left_expression, ref right_expression) => { - visitor.visit_expr(left_expression); visitor.visit_expr(right_expression); + visitor.visit_expr(left_expression); } ExprKind::Field(ref subexpression, ident) => { visitor.visit_expr(subexpression); diff --git a/src/librustc/middle/region.rs b/src/librustc/middle/region.rs index d9ccb9d42f236..13d07c720cb89 100644 --- a/src/librustc/middle/region.rs +++ b/src/librustc/middle/region.rs @@ -371,6 +371,19 @@ struct RegionResolutionVisitor<'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. + // Each time we encounter an ExprKind::AssignOp, we push + // a new Vec into the outermost Vec. This inner Vec is uesd + // to store any scopes we encounter when visiting the inenr expressions + // of the AssignOp. Once we finish visiting the inner expressions, we pop + // off the inner Vec, and process the Scopes it contains. + // This allows us to handle nested AssignOps - while a terrible idea, + // they are valid Rust, so we need to handle them. + fixup_scopes: Vec>, // Generated scope tree: scope_tree: ScopeTree, @@ -947,12 +960,108 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h } } + let prev_pessimistic = visitor.pessimistic_yield; + + // Ordinarily, we can rely on the visit order of HIR intravisit + // to correspond to the actual exectuion order of statements. + // However, there's a weird corner case with compund 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 evluation 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 sufficies 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.node { // Manually recurse over closures, because they are the only // case of nested bodies that share the parent environment. hir::ExprKind::Closure(.., body, _, _) => { let body = visitor.tcx.hir().body(body); visitor.visit_body(body); + }, + hir::ExprKind::AssignOp(_, ref left_expression, ref right_expression) => { + debug!("resolve_expr - enabling pessimistic_yield, was previously {}", + prev_pessimistic); + + visitor.fixup_scopes.push(vec![]); + 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 of 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_expression); + + visitor.pessimistic_yield = prev_pessimistic; + + let target_scopes = visitor.fixup_scopes.pop().unwrap(); + debug!("resolve_expr - restoring pessimistic_yield to {}", prev_pessimistic); + + + visitor.visit_expr(&left_expression); + debug!("resolve_expr - fixing up counts to {}", visitor.expr_and_pat_count); + + for scope in target_scopes { + let (span, count) = visitor.scope_tree.yield_in_scope.get_mut(&scope).unwrap(); + let count = *count; + let span = *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); + + visitor.scope_tree.yield_in_scope.insert(scope, (span, new_count)); + } + } _ => intravisit::walk_expr(visitor, expr) @@ -972,6 +1081,10 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h source: *source, }; visitor.scope_tree.yield_in_scope.insert(scope, data); + if visitor.pessimistic_yield { + debug!("resolve_expr in pessimistic_yield - marking scope {:?} for fixup", scope); + visitor.fixup_scopes.last_mut().unwrap().push(scope); + } // Keep traversing up while we can. match visitor.scope_tree.parent_map.get(&scope) { @@ -1360,6 +1473,8 @@ fn region_scope_tree<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> &'tcx ScopeTree var_parent: None, }, terminating_scopes: Default::default(), + pessimistic_yield: false, + fixup_scopes: vec![] }; let body = tcx.hir().body(body_id); diff --git a/src/librustc_typeck/check/generator_interior.rs b/src/librustc_typeck/check/generator_interior.rs index 72f42c85eadaf..d295c1f00c69f 100644 --- a/src/librustc_typeck/check/generator_interior.rs +++ b/src/librustc_typeck/check/generator_interior.rs @@ -28,8 +28,14 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { source_span: Span) { use syntax_pos::DUMMY_SP; + debug!("generator_interior: attempting to record type {:?} {:?} {:?} {:?}", + ty, scope, expr, source_span); + + let live_across_yield = scope.map(|s| { self.region_scope_tree.yield_in_scope(s).and_then(|yield_data| { + + // If we are recording an expression that is the last yield // in the scope, or that has a postorder CFG index larger // than the one of all of the yields, then its value can't diff --git a/src/test/run-pass/issues/issue-61442.rs b/src/test/run-pass/issues/issue-61442.rs index 89d1dac08bc10..83b2c4b8189b0 100644 --- a/src/test/run-pass/issues/issue-61442.rs +++ b/src/test/run-pass/issues/issue-61442.rs @@ -5,6 +5,22 @@ fn foo() { let mut s = String::new(); s += { yield; "" }; }; + + let _y = static || { + let x = &mut 0; + *{ yield; x } += match String::new() { _ => 0 }; + }; + + // Please don't ever actually write something like this + let _z = static || { + let x = &mut 0; + *{ + let inner = &mut 1; + *{ yield (); inner } += match String::new() { _ => 1}; + yield; + x + } += match String::new() { _ => 2 }; + }; } fn main() { From 8450289e65b07678dff32b67a18944170527c806 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 22 Jun 2019 17:30:56 -0400 Subject: [PATCH 3/6] Fix fallout from rebase --- src/librustc/middle/region.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc/middle/region.rs b/src/librustc/middle/region.rs index 13d07c720cb89..b137961b08ac0 100644 --- a/src/librustc/middle/region.rs +++ b/src/librustc/middle/region.rs @@ -1044,9 +1044,9 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h debug!("resolve_expr - fixing up counts to {}", visitor.expr_and_pat_count); for scope in target_scopes { - let (span, count) = visitor.scope_tree.yield_in_scope.get_mut(&scope).unwrap(); - let count = *count; - let span = *span; + let mut yield_data = visitor.scope_tree.yield_in_scope.get_mut(&scope).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 @@ -1059,7 +1059,7 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h debug!("resolve_expr - increasing count for scope {:?} from {} to {} at span {:?}", scope, count, new_count, span); - visitor.scope_tree.yield_in_scope.insert(scope, (span, new_count)); + yield_data.expr_and_pat_count = new_count; } } From 93aa60b4bf50ec619f6f55bb5cd6d27ea280a573 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 22 Jun 2019 17:31:14 -0400 Subject: [PATCH 4/6] Move run-pass test to run-pass/generator --- .../{issues/issue-61442.rs => generator/addassign-yield.rs} | 6 ++++++ 1 file changed, 6 insertions(+) rename src/test/run-pass/{issues/issue-61442.rs => generator/addassign-yield.rs} (67%) diff --git a/src/test/run-pass/issues/issue-61442.rs b/src/test/run-pass/generator/addassign-yield.rs similarity index 67% rename from src/test/run-pass/issues/issue-61442.rs rename to src/test/run-pass/generator/addassign-yield.rs index 83b2c4b8189b0..6a417936384b9 100644 --- a/src/test/run-pass/issues/issue-61442.rs +++ b/src/test/run-pass/generator/addassign-yield.rs @@ -1,3 +1,9 @@ +// Regression test for broken MIR error (#61442) +// Due to the two possible evaluation orders for +// a '+=' expression (depending on whether or not the 'AddAssign' trait +// is being used), we were failing to account for all types that might +// possibly be live across a yield point. + #![feature(generators)] fn foo() { From e3d8001f00380bcce64a79fae8c165d21cfc30ed Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 23 Jun 2019 15:50:59 -0400 Subject: [PATCH 5/6] Fix typos pointed out by @varkor Co-Authored-By: varkor --- src/librustc/middle/region.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/librustc/middle/region.rs b/src/librustc/middle/region.rs index b137961b08ac0..c43a375c56782 100644 --- a/src/librustc/middle/region.rs +++ b/src/librustc/middle/region.rs @@ -371,14 +371,14 @@ struct RegionResolutionVisitor<'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 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. // Each time we encounter an ExprKind::AssignOp, we push - // a new Vec into the outermost Vec. This inner Vec is uesd - // to store any scopes we encounter when visiting the inenr expressions + // a new Vec into the outermost Vec. This inner Vec is used + // to store any scopes we encounter when visiting the inner expressions // of the AssignOp. Once we finish visiting the inner expressions, we pop // off the inner Vec, and process the Scopes it contains. // This allows us to handle nested AssignOps - while a terrible idea, @@ -963,9 +963,9 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h let prev_pessimistic = visitor.pessimistic_yield; // Ordinarily, we can rely on the visit order of HIR intravisit - // to correspond to the actual exectuion order of statements. + // to correspond to the actual execution order of statements. // However, there's a weird corner case with compund assignment - // operators (e.g. 'a += b'). The evaluation order depends on whether + // 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). @@ -996,10 +996,10 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h // 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 + // used by AST borrowcheck. // // Fortunately, we don't need to know the actual execution order. - // It sufficies to know the 'worst case' order with respect to yields. + // 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 @@ -1029,7 +1029,7 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h // 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 of a count to any 'yield' expressions + // 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_expression); @@ -1474,7 +1474,7 @@ fn region_scope_tree<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> &'tcx ScopeTree }, terminating_scopes: Default::default(), pessimistic_yield: false, - fixup_scopes: vec![] + fixup_scopes: vec![], }; let body = tcx.hir().body(body_id); From 770655a47f9577b15e499a76f87b903bbde93c3b Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 23 Jun 2019 20:22:02 -0400 Subject: [PATCH 6/6] Replace Vec> with Vec<_> --- src/librustc/middle/region.rs | 27 +++++++------------ .../check/generator_interior.rs | 2 -- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/src/librustc/middle/region.rs b/src/librustc/middle/region.rs index c43a375c56782..114684b152402 100644 --- a/src/librustc/middle/region.rs +++ b/src/librustc/middle/region.rs @@ -376,15 +376,7 @@ struct RegionResolutionVisitor<'tcx> { // up their indices. pessimistic_yield: bool, // Stores scopes when pessimistic_yield is true. - // Each time we encounter an ExprKind::AssignOp, we push - // a new Vec into the outermost Vec. This inner Vec is used - // to store any scopes we encounter when visiting the inner expressions - // of the AssignOp. Once we finish visiting the inner expressions, we pop - // off the inner Vec, and process the Scopes it contains. - // This allows us to handle nested AssignOps - while a terrible idea, - // they are valid Rust, so we need to handle them. - fixup_scopes: Vec>, - + fixup_scopes: Vec, // Generated scope tree: scope_tree: ScopeTree, @@ -1020,11 +1012,11 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h let body = visitor.tcx.hir().body(body); visitor.visit_body(body); }, - hir::ExprKind::AssignOp(_, ref left_expression, ref right_expression) => { + hir::ExprKind::AssignOp(_, ref left_expr, ref right_expr) => { debug!("resolve_expr - enabling pessimistic_yield, was previously {}", prev_pessimistic); - visitor.fixup_scopes.push(vec![]); + let start_point = visitor.fixup_scopes.len(); visitor.pessimistic_yield = true; // If the actual execution order turns out to be right-to-left, @@ -1032,17 +1024,16 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h // 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_expression); - + visitor.visit_expr(&right_expr); visitor.pessimistic_yield = prev_pessimistic; - let target_scopes = visitor.fixup_scopes.pop().unwrap(); debug!("resolve_expr - restoring pessimistic_yield to {}", prev_pessimistic); - - - visitor.visit_expr(&left_expression); + 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 mut yield_data = visitor.scope_tree.yield_in_scope.get_mut(&scope).unwrap(); let count = yield_data.expr_and_pat_count; @@ -1083,7 +1074,7 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h visitor.scope_tree.yield_in_scope.insert(scope, data); if visitor.pessimistic_yield { debug!("resolve_expr in pessimistic_yield - marking scope {:?} for fixup", scope); - visitor.fixup_scopes.last_mut().unwrap().push(scope); + visitor.fixup_scopes.push(scope); } // Keep traversing up while we can. diff --git a/src/librustc_typeck/check/generator_interior.rs b/src/librustc_typeck/check/generator_interior.rs index d295c1f00c69f..0bd078dace410 100644 --- a/src/librustc_typeck/check/generator_interior.rs +++ b/src/librustc_typeck/check/generator_interior.rs @@ -34,8 +34,6 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { let live_across_yield = scope.map(|s| { self.region_scope_tree.yield_in_scope(s).and_then(|yield_data| { - - // If we are recording an expression that is the last yield // in the scope, or that has a postorder CFG index larger // than the one of all of the yields, then its value can't