Example code in `significant_drop_in_scrutinee` documentation gives bad advice

[zackw]

The documentation for the significant_drop_in_scrutinee lint currently suggests that code like this...

let mutex = Mutex::new(State {});

match mutex.lock().unwrap().foo() {
    true => {
        mutex.lock().unwrap().bar(); // Deadlock!
    }
    false => {}
};

ought to be rewritten like this...

let mutex = Mutex::new(State {});

let is_foo = mutex.lock().unwrap().foo();  // lock taken and then released
match is_foo {
    true => {
        mutex.lock().unwrap().bar(); // lock taken again
    }
    false => {}
};

This is very likely to have fixed the deadlock by introducing a TOCTOU race. The result of the foo predicate is probably only reliable for as long as the code that called foo continues to hold the lock -- meaning that, by the time the call to bar happens, it might not be appropriate to call bar anymore! Even if this isn't the case, locking a mutex is expensive, so dropping a lock only to reclaim it again almost immediately is almost always the wrong choice for performance's sake.

A better suggestion would be to make the guard's lifetime explicit:

let mutex = Mutex::new(State {});
{
    let state = mutex.lock().unwrap();
    match state.foo() {
        true => state.bar(),
        false => {}
    }
} // lock released here 

[nk9]

@rustbot label A-documentation S-needs-discussion