Incorrect suggestion for significant-drop-tightening

[Thomasdezeeuw]

Summary

I've hit a false positive, or at least a bad suggestion, with the significant-drop-tightening lint where a mutex is used in two locations, but Clippy's suggestion doesn't consider the second usage of the locked value.

Initially reported in #9399 (comment).
/cc @c410-f3r

Lint Name

significant-drop-tightening

Reproducer

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).

Version

rustc 1.72.0-nightly (cb80ff132 2023-07-07)
binary: rustc
commit-hash: cb80ff132a0e9aa71529b701427e4e6c243b58df
commit-date: 2023-07-07
host: x86_64-unknown-linux-gnu
release: 1.72.0-nightly
LLVM version: 16.0.5

clippy 0.1.72 (cb80ff1 2023-07-07)

Additional Labels

No response

[c410-f3r]

It is necessary to have a reproducible use-case for testing purposes and the above warning couldn't be reproduced (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=61f94231278339353e05ba09b88a4d5c).

However, an unrelated issue involving drop aliases was found (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=8c0ab70f59fb78fe9ad0447b7a3e717f)

[Thomasdezeeuw]

It is necessary to have a reproducible use-case for testing purposes and the above warning couldn't be reproduced (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=61f94231278339353e05ba09b88a4d5c).

However, an unrelated issue involving drop aliases was found (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=8c0ab70f59fb78fe9ad0447b7a3e717f)

Well if you change the code changes are it's not going to reproduce the same issue... But if you just copy and paste it, it will:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=e47527431e48c6129cd231218d075cc1

[c410-f3r]

Thank you @Thomasdezeeuw. It turns out that the root cause of this issue was already fixed by #11129.

In order to enhance the current test suite and to avoid similar errors in the future, a PR with the provided snippet has been submitted (#11131).

[Thomasdezeeuw]

@c410-f3r it's not fixed in clippy 0.1.72 (7bd81ee 2023-07-13). For https://github.com/Thomasdezeeuw/heph/blob/64a3e330dc9f3bdc40a4cb61b004ec17501cf2d8/inbox/src/lib.rs#L522-L528

it recommends:

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: drop the temporary after the end of its last usage
    |
527 ~                 unlock(sender_wakers);
528 +                 drop(sender_wakers);
    |

But that doesn't work as unlock, which is aliased to drop, already takes ownership.

The suggestion for https://github.com/Thomasdezeeuw/heph/blob/64a3e330dc9f3bdc40a4cb61b004ec17501cf2d8/inbox/src/lib.rs#L572-L580 is similar and also incorrect.

error: temporary with significant `Drop` can be early dropped
   --> inbox/src/lib.rs:573:21
    |
572 |           if let Some(waker) = self.registered_waker.take() {
    |  ___________________________________________________________-
573 | |             let mut join_wakers = self.channel.join_wakers.lock().unwrap();
    | |                     ^^^^^^^^^^^
574 | |             let idx = join_wakers.iter().position(|w| w.will_wake(&waker));
575 | |             if let Some(idx) = idx {
...   |
579 | |             }
580 | |         }
    | |_________- temporary `join_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
help: drop the temporary after the end of its last usage
    |
577 ~                 unlock(join_wakers);
578 +                 drop(join_wakers);
    |

Finally the suggestion for https://github.com/Thomasdezeeuw/heph/blob/64a3e330dc9f3bdc40a4cb61b004ec17501cf2d8/inbox/src/lib.rs#L600-L611 also doesn't make a lot of sense as it suggests to drop the lock before a single constant statement, which I don't think will impact the performance (much).

error: temporary with significant `Drop` can be early dropped
   --> inbox/src/lib.rs:600:21
    |
596 |           Some(w) => {
    |  ____________________-
597 | |             let waker = waker.clone();
598 | |             let old_waker = replace(w, waker.clone());
599 | |
600 | |             let mut channel_wakers = channel_wakers.lock().unwrap();
    | |                     ^^^^^^^^^^^^^^
...   |
611 | |             true
612 | |         }
    | |_________- temporary `channel_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
help: drop the temporary after the end of its last usage
    |
609 ~                 channel_wakers.push(waker);
610 +                 drop(channel_wakers);
    |

Basically it's suggesting the following.

let guard = mutex.lock().unwrap();
if condition(&guard) {
    use(guard);
    // Clippy's suggestion: drop(guard);
}
return true;

[c410-f3r]

@Thomasdezeeuw Local tests with the heph project and the latest toolchain indicated that the lint stopped firing false-positives. Can you confirm?

[Thomasdezeeuw]

@c410-f3r I can confirm that this resolved, it doesn't give any (incorrect) suggestion any more! Thanks for resolving this!