From 8cf521d80e9057211629e92aff059dc9770c20bd Mon Sep 17 00:00:00 2001 From: est31 Date: Thu, 20 Oct 2022 09:50:32 +0200 Subject: [PATCH 1/3] Remove drop order twist of && and || and make them associative Previously a short circuiting && chain would drop the first element after all the other elements, and otherwise follow evaluation order, so code like: f(1).g() && f(2).g() && f(3).g() && f(4).g() would drop the temporaries in the order 2,3,4,1. This made && and || non-associative regarding drop order, so adding ()'s to the expression would change drop order: f(1).g() && (f(2).g() && f(3).g()) && f(4).g() for example would drop in the order 3,2,4,1. As, except for the bool result, there is no data returned by the sub-expressions of the short circuiting binops, we can safely discard of any temporaries created by the sub-expr. Previously, code was already putting the rhs's into terminating scopes, but missed it for the lhs's. This commit addresses this "twist". In the expression, we now also put the lhs into a terminating scope. The drop order for the above expressions is 1,2,3,4 now. --- .../rustc_hir_analysis/src/check/region.rs | 25 +++++- src/test/ui/drop/drop_order.rs | 78 +++++++++++++++++-- src/test/ui/drop/issue-103107.rs | 37 +++++++++ 3 files changed, 129 insertions(+), 11 deletions(-) create mode 100644 src/test/ui/drop/issue-103107.rs diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index ff32329e431b6..8c2557464782e 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -241,18 +241,35 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h // scopes, meaning that temporaries cannot outlive them. // This ensures fixed size stacks. hir::ExprKind::Binary( - source_map::Spanned { node: hir::BinOpKind::And, .. }, - _, + source_map::Spanned { node: outer @ hir::BinOpKind::And, .. }, + ref l, ref r, ) | hir::ExprKind::Binary( - source_map::Spanned { node: hir::BinOpKind::Or, .. }, - _, + source_map::Spanned { node: outer @ hir::BinOpKind::Or, .. }, + ref l, ref r, ) => { // For shortcircuiting operators, mark the RHS as a terminating // scope since it only executes conditionally. + // If the LHS is not another binop itself of the same kind as ours, + // we also mark it as terminating, so that in && or || chains, + // the temporaries are dropped in order instead of the very first + // being dropped last. For the Let exception, see below. + let terminate_lhs = match l.kind { + hir::ExprKind::Let(_) => false, + hir::ExprKind::Binary(source_map::Spanned { node, .. }, ..) + if node == outer => + { + false + } + _ => true, + }; + if terminate_lhs { + terminating(l.hir_id.local_id); + } + // `Let` expressions (in a let-chain) shouldn't be terminating, as their temporaries // should live beyond the immediate expression if !matches!(r.kind, hir::ExprKind::Let(_)) { diff --git a/src/test/ui/drop/drop_order.rs b/src/test/ui/drop/drop_order.rs index 42385216ae765..4e5c1205bb9c3 100644 --- a/src/test/ui/drop/drop_order.rs +++ b/src/test/ui/drop/drop_order.rs @@ -43,7 +43,7 @@ impl DropOrderCollector { } if { - if self.option_loud_drop(7).is_some() && self.option_loud_drop(6).is_some() { + if self.option_loud_drop(6).is_some() && self.option_loud_drop(7).is_some() { self.loud_drop(8); true } else { @@ -118,17 +118,71 @@ impl DropOrderCollector { } } + fn and_chain(&self) { + // issue-103107 + if self.option_loud_drop(1).is_some() // 1 + && self.option_loud_drop(2).is_some() // 2 + && self.option_loud_drop(3).is_some() // 3 + && self.option_loud_drop(4).is_some() // 4 + && self.option_loud_drop(5).is_some() // 5 + { + self.print(6); // 6 + } + + let _ = self.option_loud_drop(7).is_some() // 1 + && self.option_loud_drop(8).is_some() // 2 + && self.option_loud_drop(9).is_some(); // 3 + self.print(10); // 4 + + // Test associativity + if self.option_loud_drop(11).is_some() // 1 + && (self.option_loud_drop(12).is_some() // 2 + && self.option_loud_drop(13).is_some() // 3 + && self.option_loud_drop(14).is_some()) // 4 + && self.option_loud_drop(15).is_some() // 5 + { + self.print(16); // 6 + } + } + + fn or_chain(&self) { + // issue-103107 + if self.option_loud_drop(1).is_none() // 1 + || self.option_loud_drop(2).is_none() // 2 + || self.option_loud_drop(3).is_none() // 3 + || self.option_loud_drop(4).is_none() // 4 + || self.option_loud_drop(5).is_some() // 5 + { + self.print(6); // 6 + } + + let _ = self.option_loud_drop(7).is_none() // 1 + || self.option_loud_drop(8).is_none() // 2 + || self.option_loud_drop(9).is_none(); // 3 + self.print(10); // 4 + + // Test associativity + if self.option_loud_drop(11).is_none() // 1 + || (self.option_loud_drop(12).is_none() // 2 + || self.option_loud_drop(13).is_none() // 3 + || self.option_loud_drop(14).is_none()) // 4 + || self.option_loud_drop(15).is_some() // 5 + { + self.print(16); // 6 + } + } + fn let_chain(&self) { // take the "then" branch - if self.option_loud_drop(2).is_some() // 2 - && self.option_loud_drop(1).is_some() // 1 + if self.option_loud_drop(1).is_some() // 1 + && self.option_loud_drop(2).is_some() // 2 && let Some(_d) = self.option_loud_drop(4) { // 4 self.print(3); // 3 } // take the "else" branch - if self.option_loud_drop(6).is_some() // 2 - && self.option_loud_drop(5).is_some() // 1 + if self.option_loud_drop(5).is_some() // 1 + && self.option_loud_drop(6).is_some() // 2 && let None = self.option_loud_drop(8) { // 4 unreachable!(); } else { @@ -152,8 +206,8 @@ impl DropOrderCollector { } // let exprs last - if self.option_loud_drop(20).is_some() // 2 - && self.option_loud_drop(19).is_some() // 1 + if self.option_loud_drop(19).is_some() // 1 + && self.option_loud_drop(20).is_some() // 2 && let Some(_d) = self.option_loud_drop(23) // 5 && let Some(_e) = self.option_loud_drop(22) { // 4 self.print(21); // 3 @@ -187,6 +241,16 @@ fn main() { collector.if_(); collector.assert_sorted(); + println!("-- and chain --"); + let collector = DropOrderCollector::default(); + collector.and_chain(); + collector.assert_sorted(); + + println!("-- or chain --"); + let collector = DropOrderCollector::default(); + collector.or_chain(); + collector.assert_sorted(); + println!("-- if let --"); let collector = DropOrderCollector::default(); collector.if_let(); diff --git a/src/test/ui/drop/issue-103107.rs b/src/test/ui/drop/issue-103107.rs new file mode 100644 index 0000000000000..5f447595662ed --- /dev/null +++ b/src/test/ui/drop/issue-103107.rs @@ -0,0 +1,37 @@ +// check-pass +// compile-flags: -Z validate-mir + +struct Foo<'a>(&'a mut u32); + +impl<'a> Drop for Foo<'a> { + fn drop(&mut self) { + *self.0 = 0; + } +} + +fn and() { + let mut foo = 0; + // This used to compile also before the fix + if true && *Foo(&mut foo).0 == 0 && ({ foo = 0; true}) {} + + // This used to fail before the fix + if *Foo(&mut foo).0 == 0 && ({ foo = 0; true}) {} + + println!("{foo}"); +} + +fn or() { + let mut foo = 0; + // This used to compile also before the fix + if false || *Foo(&mut foo).0 == 1 || ({ foo = 0; true}) {} + + // This used to fail before the fix + if *Foo(&mut foo).0 == 1 || ({ foo = 0; true}) {} + + println!("{foo}"); +} + +fn main() { + and(); + or(); +} From a2076dc0a67b0fec7fe0548d0038eed46e714535 Mon Sep 17 00:00:00 2001 From: est31 Date: Sat, 3 Dec 2022 23:32:14 +0100 Subject: [PATCH 2/3] Improve comments --- .../rustc_hir_analysis/src/check/region.rs | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index 8c2557464782e..6221b1b993798 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -250,13 +250,23 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h ref l, ref r, ) => { - // For shortcircuiting operators, mark the RHS as a terminating - // scope since it only executes conditionally. + // expr is a short circuiting operator (|| or &&). As its + // functionality can't be overridden by traits, it always + // processes bool sub-expressions. bools are Copy and thus we + // can drop any temporaries in evaluation (read) order + // (with the exception of potentially failing let expressions). + // We achieve this by enclosing the operands in a terminating + // scope, both the LHS and the RHS. + + // We optimize this a little in the presence of chains. + // Chains like a && b && c get lowered to AND(AND(a, b), c). + // In here, b and c are RHS, while a is the only LHS operand in + // that chain. This holds true for longer chains as well: the + // leading operand is always the only LHS operand that is not a + // binop itself. Putting a binop like AND(a, b) into a + // terminating scope is not useful, thus we only put the LHS + // into a terminating scope if it is not a binop. - // If the LHS is not another binop itself of the same kind as ours, - // we also mark it as terminating, so that in && or || chains, - // the temporaries are dropped in order instead of the very first - // being dropped last. For the Let exception, see below. let terminate_lhs = match l.kind { hir::ExprKind::Let(_) => false, hir::ExprKind::Binary(source_map::Spanned { node, .. }, ..) @@ -264,6 +274,8 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h { false } + // If the LHS is not another binop itself of the same kind as + // the current binop, mark it as terminating. _ => true, }; if terminate_lhs { From a59a2d3f6a80001b0610f03cfc7a6452b63f8935 Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 4 Dec 2022 00:21:19 +0100 Subject: [PATCH 3/3] Also avoid creating a terminating scope in mixed chains This avoids creation of a terminating scope in chains that contain both && and ||, because also there we know that a terminating scope is not neccessary: all the chain members are already in such terminating scopes. Also add a mixed && / || test. --- .../rustc_hir_analysis/src/check/region.rs | 26 +++++++++---------- src/test/ui/drop/drop_order.rs | 19 ++++++++++++++ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index 6221b1b993798..b315ebad4686c 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -241,12 +241,7 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h // scopes, meaning that temporaries cannot outlive them. // This ensures fixed size stacks. hir::ExprKind::Binary( - source_map::Spanned { node: outer @ hir::BinOpKind::And, .. }, - ref l, - ref r, - ) - | hir::ExprKind::Binary( - source_map::Spanned { node: outer @ hir::BinOpKind::Or, .. }, + source_map::Spanned { node: hir::BinOpKind::And | hir::BinOpKind::Or, .. }, ref l, ref r, ) => { @@ -268,14 +263,19 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h // into a terminating scope if it is not a binop. let terminate_lhs = match l.kind { + // let expressions can create temporaries that live on hir::ExprKind::Let(_) => false, - hir::ExprKind::Binary(source_map::Spanned { node, .. }, ..) - if node == outer => - { - false - } - // If the LHS is not another binop itself of the same kind as - // the current binop, mark it as terminating. + // binops already drop their temporaries, so there is no + // need to put them into a terminating scope. + // This is purely an optimization to reduce the number of + // terminating scopes. + hir::ExprKind::Binary( + source_map::Spanned { + node: hir::BinOpKind::And | hir::BinOpKind::Or, .. + }, + .., + ) => false, + // otherwise: mark it as terminating _ => true, }; if terminate_lhs { diff --git a/src/test/ui/drop/drop_order.rs b/src/test/ui/drop/drop_order.rs index 4e5c1205bb9c3..5ce1fd54a9e62 100644 --- a/src/test/ui/drop/drop_order.rs +++ b/src/test/ui/drop/drop_order.rs @@ -172,6 +172,20 @@ impl DropOrderCollector { } } + fn mixed_and_or_chain(&self) { + // issue-103107 + if self.option_loud_drop(1).is_none() // 1 + || self.option_loud_drop(2).is_none() // 2 + || self.option_loud_drop(3).is_some() // 3 + && self.option_loud_drop(4).is_some() // 4 + && self.option_loud_drop(5).is_none() // 5 + || self.option_loud_drop(6).is_none() // 6 + || self.option_loud_drop(7).is_some() // 7 + { + self.print(8); // 8 + } + } + fn let_chain(&self) { // take the "then" branch if self.option_loud_drop(1).is_some() // 1 @@ -251,6 +265,11 @@ fn main() { collector.or_chain(); collector.assert_sorted(); + println!("-- mixed and/or chain --"); + let collector = DropOrderCollector::default(); + collector.mixed_and_or_chain(); + collector.assert_sorted(); + println!("-- if let --"); let collector = DropOrderCollector::default(); collector.if_let();