Reduce the number of temporary variabkes created by using braces

Rust 2024 treates the last expr as temporary, allowing us to write
*x.borrow_mut() = { *x.borrow() +1 }
without causing borrowing errors

We must careful only with side effects and evaluation orders

Author: nunoplopes

cpp2rust/converter/converter.cpp

@@ -2928,24 +2928,18 @@ bool Converter::VisitUnaryOperator(clang::UnaryOperator *expr) {
 bool Converter::VisitStmtExpr(clang::StmtExpr *expr) {
   auto *body = expr->getSubStmt();
   PushBrace brace(*this);
-  auto stmts = body->body();
-  size_t n = static_cast<size_t>(stmts.end() - stmts.begin());
-  size_t i = 0;
-  for (auto *s : stmts) {
-    ++i;
-    if (i == n) {
-      if (auto *tail = clang::dyn_cast<clang::Expr>(s)) {
-        EmitStmtExprTail(tail);
-        continue;
-      }
+  for (auto *s : body->body()) {
+    // The last expression is the value of the block, so it has no semicolon.
+    auto *tail = clang::dyn_cast<clang::Expr>(s);
+    if (tail && s == body->body_back()) {
+      Convert(tail);
+    } else {
+      Convert(s);
     }
-    Convert(s);
   }
   return false;
 }
 
-void Converter::EmitStmtExprTail(clang::Expr *tail) { Convert(tail); }
-
 bool Converter::VisitConditionalOperator(clang::ConditionalOperator *expr) {
   StrCat(keyword::kIf);
   ConvertCondition(expr->getCond());
@@ -3896,9 +3890,7 @@ bool Converter::VisitSwitchStmt(clang::SwitchStmt *stmt) {
   if (needs_switch_macro) {
     StrCat("match", ToString(stmt->getCond()));
   } else {
-    StrCat(
-        std::format("let __match_cond = {};", ConvertRValue(stmt->getCond())));
-    StrCat("match __match_cond");
+    StrCat(std::format("match {{ {} }}", ConvertRValue(stmt->getCond())));
   }
 
   PushBrace match_brace(*this);

cpp2rust/converter/converter.h

@@ -402,8 +402,6 @@ class Converter : public clang::RecursiveASTVisitor<Converter> {
 
   virtual bool VisitStmtExpr(clang::StmtExpr *expr);
 
-  virtual void EmitStmtExprTail(clang::Expr *tail);
-
   virtual bool VisitConditionalOperator(clang::ConditionalOperator *expr);
 
   virtual bool VisitDeclRefExpr(clang::DeclRefExpr *expr);

cpp2rust/converter/models/converter_refcount.cpp

@@ -1558,15 +1558,9 @@ bool ConverterRefCount::VisitUnaryExprOrTypeTraitExpr(
 
 bool ConverterRefCount::VisitStmtExpr(clang::StmtExpr *expr) {
   PushConversionKind push(*this, ConversionKind::FullRefCount);
-  return Converter::VisitStmtExpr(expr);
-}
-
-void ConverterRefCount::EmitStmtExprTail(clang::Expr *tail) {
-  StrCat("let __result = ");
-  Convert(tail);
-  StrCat(token::kSemiColon);
-  StrCat("__result");
-  SetFreshType(tail->getType());
+  Converter::VisitStmtExpr(expr);
+  SetFreshType(expr->getType());
+  return false;
 }
 
 void ConverterRefCount::ConvertBinaryOperator(clang::BinaryOperator *expr) {
@@ -1580,8 +1574,7 @@ void ConverterRefCount::ConvertBinaryOperator(clang::BinaryOperator *expr) {
       assign && GetSafeTypeAsString(lhs_type) !=
                     GetSafeTypeAsString(assign->getComputationResultType())) {
     auto computation_result_type = assign->getComputationResultType();
-    PushBrace brace(*this);
-    StrCat(keyword::kLet, "rhs_0", token::kAssign);
+    std::optional<Buffer> buf(std::in_place, *this);
     if (IsUnsignedArithOp(assign)) {
       PushParen outer(*this);
       {
@@ -1607,15 +1600,16 @@ void ConverterRefCount::ConvertBinaryOperator(clang::BinaryOperator *expr) {
     } else {
       ConvertCast(lhs_type);
     }
-    StrCat(token::kSemiColon);
-    EmitSetOrAssign(lhs, "rhs_0");
+    auto value = std::move(*buf).str();
+    buf.reset();
+    EmitCompoundSetOrAssign(lhs, rhs, value);
     return;
   }
 
   if (IsUnsignedArithOp(expr)) {
-    PushBrace brace(*this, expr->isCompoundAssignmentOp());
+    std::optional<Buffer> buf;
     if (expr->isCompoundAssignmentOp()) {
-      StrCat(keyword::kLet, "rhs_0", token::kAssign);
+      buf.emplace(*this);
     }
     {
       PushParen paren(*this);
@@ -1627,8 +1621,9 @@ void ConverterRefCount::ConvertBinaryOperator(clang::BinaryOperator *expr) {
     }
     ConvertUnsignedArithBinaryOperator(expr, rhs);
     if (expr->isCompoundAssignmentOp()) {
-      StrCat(token::kSemiColon);
-      EmitSetOrAssign(lhs, "rhs_0");
+      auto value = std::move(*buf).str();
+      buf.reset();
+      EmitCompoundSetOrAssign(lhs, rhs, value);
     } else {
       computed_expr_type_ = ComputedExprType::FreshValue;
     }
@@ -2424,16 +2419,39 @@ void ConverterRefCount::EmitSetOrAssign(clang::Expr *lhs,
   computed_expr_type_ = ComputedExprType::FreshValue;
 }
 
+bool ConverterRefCount::CanBraceAssignedValue(
+    clang::Expr *lhs, clang::Expr *rhs, std::string_view assign_operator) {
+  return (assign_operator == "=" || lhs->getType()->isArithmeticType()) &&
+         !lhs->HasSideEffects(ctx_) && !rhs->HasSideEffects(ctx_);
+}
+
+void ConverterRefCount::EmitCompoundSetOrAssign(clang::Expr *lhs,
+                                                clang::Expr *rhs,
+                                                std::string_view value) {
+  if (CanBraceAssignedValue(lhs, rhs, "=")) {
+    PushBrace brace(*this, isRValue());
+    EmitSetOrAssign(lhs, std::format("{{ {} }}", value));
+    return;
+  }
+  PushBrace brace(*this);
+  StrCat(keyword::kLet, "rhs_0", token::kAssign, value, token::kSemiColon);
+  EmitSetOrAssign(lhs, "rhs_0");
+}
+
 void ConverterRefCount::ConvertAssignment(clang::Expr *lhs, clang::Expr *rhs,
                                           std::string_view assign_operator) {
   auto rhs_as_string = ConvertFreshRValue(rhs, lhs->getType());
 
   PushBrace brace(*this, isRValue());
 
   if (MayCauseBorrowMutError(lhs, rhs)) {
-    StrCat(keyword::kLet, "__rhs", token::kAssign, rhs_as_string,
-           token::kSemiColon);
-    rhs_as_string = "__rhs";
+    if (CanBraceAssignedValue(lhs, rhs, assign_operator)) {
+      rhs_as_string = std::format("{{ {} }}", rhs_as_string);
+    } else {
+      StrCat(keyword::kLet, "__rhs", token::kAssign, rhs_as_string,
+             token::kSemiColon);
+      rhs_as_string = "__rhs";
+    }
   }
 
   if (assign_operator == "=") {
@@ -2492,7 +2510,7 @@ void ConverterRefCount::ConvertGenericBinaryOperator(
 
   if (may_cause_borrow_mut_err) {
     StrCat(std::format(
-        "{{ let _lhs = {}; _lhs {} {} }}",
+        "({{ {} }} {} {{ {} }})",
         ConvertFreshRValue(
             lhs, GetOperandImplicitConversionTarget(ctx_, expr, lhs, rhs)),
         opcode,

cpp2rust/converter/models/converter_refcount.h

@@ -134,8 +134,6 @@ class ConverterRefCount final : public Converter {
 
   bool VisitStmtExpr(clang::StmtExpr *expr) override;
 
-  void EmitStmtExprTail(clang::Expr *tail) override;
-
   bool VisitInitListExpr(clang::InitListExpr *expr) override;
 
   bool VisitCXXStdInitializerListExpr(
@@ -335,6 +333,19 @@ class ConverterRefCount final : public Converter {
 
   void EmitSetOrAssign(clang::Expr *lhs, std::string_view rhs);
 
+  // Whether an assigned value can be wrapped in braces to release its borrows
+  // before lhs is borrowed, as Rust 2024 drops the temporaries of a block's
+  // tail expression at the end of the block. This requires the value to be
+  // evaluated before lhs (or the order to be unobservable); otherwise it must
+  // be bound to a variable first.
+  bool CanBraceAssignedValue(clang::Expr *lhs, clang::Expr *rhs,
+                             std::string_view assign_operator);
+
+  // Emits `lhs = value`, where value is the result of the compound assignment
+  // `lhs op= rhs` and thus reads lhs.
+  void EmitCompoundSetOrAssign(clang::Expr *lhs, clang::Expr *rhs,
+                               std::string_view value);
+
   // If lhs is a direct reference to a global/static value (not a reference
   // type), emits `var.with(|rc| *rc.borrow_mut() <op> <rhs>)` and returns
   // true. This avoids cloning the Rc just to assign through it. Returns

tests/multi-file/defaulted_move_cross_tu/out/refcount/defaulted_move_cross_tu.rs

@@ -51,17 +51,17 @@ impl Default for S {
     }
 }
 pub fn sum_0(s: Ptr<S>) -> i32 {
-    return {
-        let _lhs = {
-            let _lhs = ((*s.with(|__s| __s.v.clone()).borrow()).len() as i32);
-            _lhs + ((array_field_ptr!(s, n) as Ptr<i32>)
+    return ({
+        ({ ((*s.with(|__s| __s.v.clone()).borrow()).len() as i32) } + {
+            ((array_field_ptr!(s, n) as Ptr<i32>)
                 .offset((0) as isize)
                 .read())
-        };
-        _lhs + ((array_field_ptr!(s, n) as Ptr<i32>)
+        })
+    } + {
+        ((array_field_ptr!(s, n) as Ptr<i32>)
             .offset((1) as isize)
             .read())
-    };
+    });
 }
 pub fn main() {
     __cpp2rust_init_globals();

tests/multi-file/friend_operator/out/refcount/friend_operator.rs

@@ -58,13 +58,8 @@ fn main_0() -> i32 {
     return 0;
 }
 pub fn operator_eq_0(x: Ptr<S>, y: Ptr<S>) -> bool {
-    return ({
-        let _lhs = x.with(|__s| __s.a);
-        _lhs == y.with(|__s| __s.a)
-    }) && ({
-        let _lhs = x.with(|__s| __s.b);
-        _lhs == y.with(|__s| __s.b)
-    });
+    return ({ x.with(|__s| __s.a) } == { y.with(|__s| __s.a) })
+        && ({ x.with(|__s| __s.b) } == { y.with(|__s| __s.b) });
 }
 pub fn operator_ne_1(x: Ptr<S>, y: Ptr<S>) -> bool {
     return !({
@@ -74,16 +69,9 @@ pub fn operator_ne_1(x: Ptr<S>, y: Ptr<S>) -> bool {
     });
 }
 pub fn operator_lt_2(x: Ptr<S>, y: Ptr<S>) -> bool {
-    return ({
-        let _lhs = x.with(|__s| __s.a);
-        _lhs < y.with(|__s| __s.a)
-    }) || (({
-        let _lhs = x.with(|__s| __s.a);
-        _lhs == y.with(|__s| __s.a)
-    }) && ({
-        let _lhs = x.with(|__s| __s.b);
-        _lhs < y.with(|__s| __s.b)
-    }));
+    return ({ x.with(|__s| __s.a) } < { y.with(|__s| __s.a) })
+        || (({ x.with(|__s| __s.a) } == { y.with(|__s| __s.a) })
+            && ({ x.with(|__s| __s.b) } < { y.with(|__s| __s.b) }));
 }
 pub fn compare_3(x: Ptr<S>, y: Ptr<S>) -> i32 {
     if ({

tests/ub/out/refcount/max.rs

@@ -29,9 +29,6 @@ fn main_0() -> i32 {
             __tmp_1.as_pointer()
         }
     });
-    return (({
-        let _lhs = (a.read());
-        _lhs == (b.read())
-    }) as i32);
+    return (({ (a.read()) } == { (b.read()) }) as i32);
 }
 pub fn __cpp2rust_init_globals() {}

tests/ub/out/refcount/ub4.rs

@@ -7,10 +7,7 @@ use std::io::{Read, Seek, Write};
 use std::os::fd::AsFd;
 use std::rc::{Rc, Weak};
 pub fn smaller_0(x1: Ptr<i32>, x2: Ptr<i32>) -> Ptr<i32> {
-    return if ({
-        let _lhs = (x1.read());
-        _lhs < (x2.read())
-    }) {
+    return if ({ (x1.read()) } < { (x2.read()) }) {
         (x1).clone()
     } else {
         (x2).clone()

tests/ub/out/refcount/ub6.rs

@@ -39,10 +39,7 @@ pub fn fill_1(arr: Ptr<Option<Value<Box<[Ptr<i32>]>>>>, n1: Ptr<i32>) {
 pub fn any_2(arr: Ptr<Option<Value<Box<[Ptr<i32>]>>>>, n1: Ptr<i32>) -> bool {
     let out: Value<bool> = Rc::new(RefCell::new(false));
     let i: Value<i32> = Rc::new(RefCell::new(0));
-    'loop_: while {
-        let _lhs = (*i.borrow());
-        _lhs < (n1.read())
-    } {
+    'loop_: while ({ (*i.borrow()) } < { (n1.read()) }) {
         let __rhs = (*out.borrow())
             || (((*arr.upgrade().deref()).as_ref().unwrap().borrow()
                 [((*i.borrow()) as usize) as usize]

tests/unit/out/refcount/02_address_taken.rs

@@ -16,8 +16,7 @@ fn main_0() -> i32 {
     (*b_ptr.borrow()).write(3);
     let b_ptr_ptr: Value<Ptr<Ptr<i32>>> = Rc::new(RefCell::new((b_ptr.as_pointer())));
     ((*b_ptr_ptr.borrow()).read()).write(4);
-    let __rhs = (((*b_ptr_ptr.borrow()).read()).read());
-    (*b_ptr.borrow()).write(__rhs);
+    (*b_ptr.borrow()).write({ (((*b_ptr_ptr.borrow()).read()).read()) });
     let offset: Value<usize> = Rc::new(RefCell::new(
         ((((*b_ptr.borrow()).clone() - (*b_ptr.borrow()).clone()) as i64) as usize),
     ));