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

[New Lint Request] If possible, suggest releasing sync locks early #9399

Open
c410-f3r opened this issue Aug 30, 2022 · 4 comments
Open

[New Lint Request] If possible, suggest releasing sync locks early #9399

c410-f3r opened this issue Aug 30, 2022 · 4 comments
Assignees
Labels
A-lint Area: New lints E-hard Call for participation: This a hard problem and requires more experience or effort to work on L-pedantic Lint: Belongs in the pedantic lint group L-restriction Lint: Belongs in the restriction lint group

Comments

@c410-f3r
Copy link
Contributor

c410-f3r commented Aug 30, 2022

Problem

Code that heavily depends on synchronous primitives like Mutex can suffer unnecessary resource contention if the lock is only released at the end of its scope through Drop but could in fact be released early.

fn example_1() {
  let locked = some_sync_resource.lock();
  let owned_rslt = locked.do_stuff_with_resource();
  // Only `owned_rslt` is needed but `locked` is still held
  do_heavy_computation_that_takes_time(owned_rslt);
}

fn example_2() {
  let locked = some_sync_resource.lock();
  let reference = locked.get_inner_resource_ref();
  let _ = reference.do_stuff();
  let _ = reference.do_other_things();
  // From this point forward, `locked` is not needed anymore
  let _ = compute_foo();
  let _ = compute_bar();
  let _ = compute_baz();
}

Request

A new lint called sync_lock_drop that asks for an explicit "inline" expression or an explicit drop call. The above snippets would then be rewritten as follows:

fn example_1() {
  let owned_rslt = some_sync_resource.lock().do_stuff_with_resource();
  do_heavy_computation_that_takes_time(owned_rslt);
}

fn example_2() {
  let locked = some_sync_resource.lock();
  let reference = locked.get_inner_resource_ref();
  let _ = reference.do_stuff();
  let _ = reference.do_other_things();
  drop(locked);
  let _ = compute_foo();
  let _ = compute_bar();
  let _ = compute_baz();
}

Implementation

It is trivial to detect synchronous primitives of the standard library but third-party implementations are tricky to detect. What should be done here? Hard code a list of well-known crates or allow users to provide a list of custom types?

@c410-f3r c410-f3r added the A-lint Area: New lints label Aug 30, 2022
@c410-f3r c410-f3r changed the title [New Lint Request] Suggest explict drop call for every sync lock [New Lint Request] If possible, suggest releasing sync locks early Aug 30, 2022
@flip1995 flip1995 added E-hard Call for participation: This a hard problem and requires more experience or effort to work on L-pedantic Lint: Belongs in the pedantic lint group L-restriction Lint: Belongs in the restriction lint group labels Oct 4, 2022
@flip1995
Copy link
Member

flip1995 commented Oct 4, 2022

To implement this, the has_significant_drop attribute, that can be attached to types could be used, like the clippy::significant_drop_in_scrutinee lint does.

However, I marked it as E-hard because keeping track of references build from the locked variables. So checking for the last use of locked should be easy. But checking that reference (or other bindings) aren't used anymore might be hard.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jan 1, 2023

@rustbot claim

c410-f3r added a commit to c410-f3r/tokio that referenced this issue Feb 2, 2023
`#[clippy::has_significant_drop]` tells that a structure should be considered when evaluating some lints. Examples of such behavior are the existent `clippy::significant_drop_in_scrutinee` and in the soon-to-be-finished rust-lang/rust-clippy#9399.
c410-f3r added a commit to c410-f3r/async-lock that referenced this issue Feb 2, 2023
`#[clippy::has_significant_drop]` tells that a structure should be considered when evaluating some lints. Examples of such behavior are the existent `clippy::significant_drop_in_scrutinee` and in the soon-to-be-finished rust-lang/rust-clippy#9399.
c410-f3r added a commit to c410-f3r/crossbeam that referenced this issue Feb 2, 2023
`#[clippy::has_significant_drop]` tells that a structure should be considered when evaluating some lints. Examples of such behavior are the existent `clippy::significant_drop_in_scrutinee` and in the soon-to-be-finished rust-lang/rust-clippy#9399.
c410-f3r added a commit to c410-f3r/parking_lot that referenced this issue Feb 2, 2023
`#[clippy::has_significant_drop]` tells that a structure should be considered when evaluating some lints. Examples of such behavior are the existent `clippy::significant_drop_in_scrutinee` and in the soon-to-be-finished rust-lang/rust-clippy#9399.
notgull pushed a commit to smol-rs/async-lock that referenced this issue Feb 3, 2023
* Mark `MutexGuard` with `#[clippy::has_significant_drop]`

`#[clippy::has_significant_drop]` tells that a structure should be considered when evaluating some lints. Examples of such behavior are the existent `clippy::significant_drop_in_scrutinee` and in the soon-to-be-finished rust-lang/rust-clippy#9399.

* Include more structures
bors bot added a commit to crossbeam-rs/crossbeam that referenced this issue Feb 4, 2023
958: Mark some structures with `#[clippy::has_significant_drop]` r=taiki-e a=c410-f3r

`#[clippy::has_significant_drop]` tells that a structure should be considered when evaluating some lints. Examples of such behavior are the existent `clippy::significant_drop_in_scrutinee` and the soon-to-be-finished rust-lang/rust-clippy#9399.

Co-authored-by: Caio <c410.f3r@gmail.com>
SchmErik pushed a commit to risc0/crossbeam that referenced this issue Feb 15, 2023
`#[clippy::has_significant_drop]` tells that a structure should be considered when evaluating some lints. Examples of such behavior are the existent `clippy::significant_drop_in_scrutinee` and in the soon-to-be-finished rust-lang/rust-clippy#9399.
bors added a commit that referenced this issue Feb 16, 2023
[significant_drop_tightening] Add MVP

cc #9399

Creates the lint with minimum functionalities, which is a good start IMO.

---

changelog: new lint: [`significant_drop_tightening`]
[#10163](#10163)
<!-- changelog_checked -->
taiki-e pushed a commit to crossbeam-rs/crossbeam that referenced this issue Feb 28, 2023
`#[clippy::has_significant_drop]` tells that a structure should be considered when evaluating some lints. Examples of such behavior are the existent `clippy::significant_drop_in_scrutinee` and in the soon-to-be-finished rust-lang/rust-clippy#9399.
@Thomasdezeeuw
Copy link
Contributor

I've hit a false positive, or at least a bad suggestion, with this lint using clippy 0.1.72 (f4b80ca 2023-06-30).

The related code:
https://github.com/Thomasdezeeuw/heph/blob/8725fe9ed871f135b7ab87b5969eb1213121a242/inbox/src/lib.rs#L523-L529

For which Clippy suggests:

error: temporary with significant `Drop` can be early dropped
   --> inbox/src/lib.rs:523:21
    |
522 |           if let Some(waker) = self.registered_waker.take() {
    |  ___________________________________________________________-
523 | |             let mut sender_wakers = self.channel.sender_wakers.lock().unwrap();
    | |                     ^^^^^^^^^^^^^
524 | |             let idx = sender_wakers.iter().position(|w| w.will_wake(&waker));
525 | |             if let Some(idx) = idx {
...   |
529 | |             }
530 | |         }
    | |_________- temporary `sender_wakers` is currently being dropped at the end of its contained scope
    |
    = note: this might lead to unnecessary resource contention
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_tightening
    = note: `-D clippy::significant-drop-tightening` implied by `-D clippy::nursery`
help: merge the temporary construction with its single usage
    |
523 ~
524 +             let idx = self.channel.sender_wakers.lock().unwrap().position(|w| w.will_wake(&waker));
    |
help: remove separated single usage
    |
524 -             let idx = sender_wakers.iter().position(|w| w.will_wake(&waker));
524 +
    |

It doesn't seem to realize that sender_wakers is used twice, once to iterate over (line 524) and then to mutate it (line 526).

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 1, 2023

At least for f4b80ca, it seems that the Rust project didn't catch up with the latest changes of this lint (https://github.com/rust-lang/rust/blob/f4b80cacf93ca216c75f6ae12f4b9dec19eba42f/src/tools/clippy/clippy_lints/src/significant_drop_tightening.rs).

Can you create another issue if the same behavior continues after the next Clippy update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-hard Call for participation: This a hard problem and requires more experience or effort to work on L-pedantic Lint: Belongs in the pedantic lint group L-restriction Lint: Belongs in the restriction lint group
Projects
None yet
Development

No branches or pull requests

3 participants