2

I wrote this code for the leetcode same tree problem:

use std::cell::RefCell;
use std::rc::Rc;

// Definition for a binary tree node.
#[derive(Debug, PartialEq, Eq)]
pub struct TreeNode {
    pub val: i32,
    pub left: Option<Rc<RefCell<TreeNode>>>,
    pub right: Option<Rc<RefCell<TreeNode>>>,
}

impl TreeNode {
    #[inline]
    pub fn new(val: i32) -> Self {
        TreeNode {
            val,
            left: None,
            right: None,
        }
    }
}

struct Solution;

impl Solution {
    pub fn is_same_tree(
        p: Option<Rc<RefCell<TreeNode>>>,
        q: Option<Rc<RefCell<TreeNode>>>,
    ) -> bool {
        match (p, q) {
            (None, None) => true,
            (Some(p), Some(q)) if p.borrow().val == q.borrow().val => {
                // this line won't work, it will cause BorrowMutError at runtime
                // but the `a & b` version works
                return (Self::is_same_tree(
                    p.borrow_mut().left.take(),
                    q.borrow_mut().left.take(),
                )) && (Self::is_same_tree(
                    p.borrow_mut().right.take(),
                    q.borrow_mut().right.take(),
                ));

                let a = Self::is_same_tree(p.borrow_mut().left.take(), q.borrow_mut().left.take());
                let b =
                    Self::is_same_tree(p.borrow_mut().right.take(), q.borrow_mut().right.take());
                a && b
            }
            _ => false,
        }
    }
}

fn main() {
    let p = Some(Rc::new(RefCell::new(TreeNode {
        val: 1,
        left: Some(Rc::new(RefCell::new(TreeNode::new(2)))),
        right: Some(Rc::new(RefCell::new(TreeNode::new(3)))),
    })));
    let q = Some(Rc::new(RefCell::new(TreeNode {
        val: 1,
        left: Some(Rc::new(RefCell::new(TreeNode::new(2)))),
        right: Some(Rc::new(RefCell::new(TreeNode::new(3)))),
    })));

    println!("{:?}", Solution::is_same_tree(p, q));
}

playground

thread 'main' panicked at 'already borrowed: BorrowMutError', src/main.rs:39:23

I think && is a short-circuit operator which means the two expressions won't exist at the same time, so two mutable references should not exist at the same time.

Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
Shuumatsu
  • 592
  • 2
  • 5
  • 12
  • 1
    By inferring the missing pieces, I got to [this code](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=af603fc9f53ca8f606cc23397fe17955), which actually compiles. The problem cannot be reproduced. Are you perchance using an old version of the compiler? – E_net4 Nov 17 '20 at 16:36
  • @E_net4ishere I'm using 1.49.0-nightly. btw I have updated my question, you can try it on the Rust Playground. – Shuumatsu Nov 17 '20 at 16:50
  • 2
    Thanks for fixing the example! I'd guess the rust compiler considers whole expressions at once when executing destructors for the mutable handles. You can work around this and still maintain short-circuiting by pulling just the left side of the `&&` into a prior declaration. – PitaJ Nov 17 '20 at 17:36
  • 1
    [A minimized example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=23a94854b302c21f98a1caa64cbc0a39) – Shepmaster Nov 17 '20 at 17:56

1 Answers1

4

A minimized example:

use std::cell::RefCell;

fn main() {
    let x = RefCell::new(true);
    *x.borrow_mut() && *x.borrow_mut();
}
thread 'main' panicked at 'already borrowed: BorrowMutError', src/main.rs:8:27

When you call RefCell::borrow_mut, a temporary value of type RefMut is returned. From the reference:

The drop scope of the temporary is usually the end of the enclosing statement.

And in expanded detail:

Apart from lifetime extension, the temporary scope of an expression is the smallest scope that contains the expression and is for one of the following:

  • The entire function body.
  • A statement.
  • The body of a if, while or loop expression.
  • The else block of an if expression.
  • The condition expression of an if or while expression, or a match guard.
  • The expression for a match arm.
  • The second operand of a lazy boolean expression.

The failing code would look something like this if it were expanded:

{
    let t1 = x.borrow_mut();

    *t1 && {
        let t2 = x.borrow_mut();
        *t2
    }
}

This shows how the double borrow occurs.

As you've already noticed, you can work around this by declaring a variable ahead-of-time. This preserves the short-circuit nature:

let a = *x.borrow_mut();
a && *x.borrow_mut();
Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
  • Thank you. Does "The second operand of a lazy boolean expression." mean that in `a & b`, `a` will be dropped after b has been evaluated, and in `a & b & c = (a & b) & c`, a and be will be dropped before evaluating `c`. so this explains this example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=280ad95988dbdaa89fd67afcfb26d182 – Shuumatsu Nov 18 '20 at 03:22
  • Yes. In my expanded code, you can see that as the nested `{}` block containing `t2`. *in `a & b`* — `&` is **not** lazy / short-circuiting. Only `&&` is. – Shepmaster Nov 18 '20 at 03:26
  • sorry. I just get up, my mind is in a mess. I indeed mean `&&` and I cannot edit my comment after 5min. It should be: Does "The second operand of a lazy boolean expression." mean that in `a && b`, `a` will be dropped after `b` has been evaluated, and in `a && b && c = (a && b) && c`, `a` and `b` will be dropped before evaluating `c`. so this explains this example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=280ad95988dbdaa89fd67afcfb26d182 – Shuumatsu Nov 18 '20 at 03:31