Lint suggestion: Accidentally acquiring lock which is already held

[tkaitchuck]

What it does

Currently it is possible to accidently write a function which acquires a lock, and then later on in the same function attempts to re-acquire the same lock. For std::sync::Mutex this will deadlock the execution, and be confusing to debug. The same is true for tokio::sync::Mutex.

Lint Name

Acquiring lock already held

Category

correctness

Advantage

For non-reentrant locks, the original code is wrong and will deadlock.

For reentrant locks it is bad form and still almost certainly a mistake. It is likely that the developer believes the guard from the first lock acquisition has already gone out of scope.

Drawbacks

Such a lint will not be comprehensive as it will likely not be possible to detect cases where the re-acquisition is acquired inside of another method.

Example

    let lock = Mutex::new(1);
    {
        let mut guard = lock.lock().unwrap();
        *guard += 1;
        dbg!(*guard);
        {
            let mut oops = lock.lock().unwrap();
            *oops += 1;
        }
    }

Could be written as:

    let lock = Mutex::new(1);
    {
        let mut guard = lock.lock().unwrap();
        *guard += 1;
        dbg!(*guard);
        {
            *guard += 1;
        }
    }

[jplatte]

Another, more subtle case:

if let Some(item) = shared_vec.lock().unwrap().pop() {
    // do some work...
    shared_vec.lock().unwrap().push(/* new item */);
}

which due to temporary lifetime extension also results in a deadlock and has to be changed to

let removed = shared_vec.lock().unwrap().pop();
if let Some(item) = removed {
    // do some work...
    shared_vec.lock().unwrap().push(/* new item */);
}

@rustbot label +L-correctness