Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicit unlock for locks #7500

Open
Tracked by #79
HKalbasi opened this issue Jul 28, 2021 · 4 comments
Open
Tracked by #79

Explicit unlock for locks #7500

HKalbasi opened this issue Jul 28, 2021 · 4 comments
Labels
A-lint Area: New lints

Comments

@HKalbasi
Copy link
Member

HKalbasi commented Jul 28, 2021

What it does

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

Categories

  • Kind: Restriction

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
}
@HKalbasi HKalbasi added the A-lint Area: New lints label Jul 28, 2021
@mathstuf
Copy link
Contributor

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

  • an ignored variable name (_arbitrary);
  • the name of the type being assigned;
  • the variable name (*guard* or *lock*); and/or
  • the method name (*lock*)?

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

@ojeda
Copy link
Contributor

ojeda commented Jul 28, 2021

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
Copy link
Contributor

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

4 participants