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.

Author: est31

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(_)) {

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();

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();
+}