rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.3k stars 1.52k forks source link

Explicit unlock for locks #7500

Open HKalbasi opened 3 years ago

HKalbasi commented 3 years ago

What it does

Force locks to have a dedicated block which lock is acquired at first line of it, or an explicit drop.

Categories

Why?

Implicit drops can be surprising and some users of linux community forum lwn have some concerns about it that can cause bug in linux kernel. It can also encourage people to free locks sooner.

Drawbacks

None.

Example

fn sample(l: Locked) -> i32 {
    let mut x = 2;
    let guard = l.lock();
    x += 3;
    guard.do_sth();
    some_other_work_with_lock();
    x += 4;
    x
}

Could be written as:

fn sample(l: Locked) -> i32 {
    let mut x = 2;
    { // lock block
        let guard = l.lock();
        x += 3;
        guard.do_sth();
        some_other_work_with_lock();
    }
    x += 4;
    x
}

Or with drop:

fn sample(l: Locked) -> i32 {
    let mut x = 2;
    let guard = l.lock();
    x += 3;
    guard.do_sth();
    some_other_work_with_lock();
    drop(guard);
    x += 4;
    x
}
mathstuf commented 3 years ago

I think the main problem will be detecting which local variables are guards of this kind. Should it go based on:

How can a project specify the names usually involved with this kind of code?

ojeda commented 3 years ago

If this is to be done, I would avoid guessing via heuristics. I would instead allow marking guard-like types explicitly with a custom attribute or similar.

camsteffen commented 3 years ago

I would avoid guessing via heuristics.

Agreed. We can just include std types like Mutex, Stdin and similar. Maybe we can support something like #[clippy::guard] struct MyGuard if it is wanted.

There is a questionable case here - what if lock() is on the first line of the function? It wouldn't make sense to add a block around the entire function. Would the user just have to add drop? Or is that case already okay?

Perhaps it would be simpler to just always require drop.

HKalbasi commented 3 years ago

There is a questionable case here - what if lock() is on the first line of the function? It wouldn't make sense to add a block around the entire function. Would the user just have to add drop?

If user found it doesn't make sense to add a block around entire function, they have the choice of using drop. Personally I prefer drop in those cases but block in others, because block visually show the scope of lock if it is a small part of a big function.

Or is that case already okay?

It shouldn't be. Locks can come at beginning of functions and loops and ifs accidentally. But if a lock appears in a simple unnecessary block, we can almost be sure that propose of that block is to explicitly declare the scope of guard.

Perhaps it would be simpler to just always require drop.

Maybe. I have no idea of implementation. But as a user I prefer block (in addition to drop) because it is visually cleaner.