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

significant-drop-in-scrutinee when false positive #8963

Closed
aviramha opened this issue Jun 7, 2022 · 4 comments · Fixed by #12764
Closed

significant-drop-in-scrutinee when false positive #8963

aviramha opened this issue Jun 7, 2022 · 4 comments · Fixed by #12764
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@aviramha
Copy link

aviramha commented Jun 7, 2022

Summary

Last version of clippy seems to have false positive on the lint
happens in our CI
https://github.com/metalbear-co/mirrord/runs/6770432194?check_suite_focus=true

Lint Name

significant-drop-in-scrutinee

Reproducer

I tried this code:

pub(crate) static SOCKETS: SyncLazy<Mutex<HashSet<Socket>>> =
    SyncLazy::new(|| Mutex::new(HashSet::new()));

fn bind(sockfd: c_int, addr: *const sockaddr, addrlen: socklen_t) -> c_int {
    debug!("bind called sockfd: {:?}", sockfd);
    let mut socket = {
        let mut sockets = SOCKETS.lock().unwrap();
        match sockets.take(&sockfd) {
            Some(socket) if !matches!(socket.state, SocketState::Initialized) => {
                error!("socket is in invalid state for bind {:?}", socket.state);
                return libc::EINVAL;
            }
            Some(socket) => socket,
            None => {
                debug!("bind: no socket found for fd: {}", &sockfd);
                return unsafe { libc::bind(sockfd, addr, addrlen) };
            }
        }
    };
....

I saw this happen:

   --> mirrord-layer/src/sockets.rs:168:15
    |
168 |         match sockets.take(&sockfd) {
    |               ^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::significant-drop-in-scrutinee` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee

Version

rustc 1.63.0-nightly (50b00252a 2022-06-06) (from rustc 1.63.0-nightly (50b00252a 2022-06-06))

Additional Labels

No response

@aviramha aviramha added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Jun 7, 2022
@SpriteOvO
Copy link
Contributor

SpriteOvO commented Jun 10, 2022

Same problem:

pub(crate) fn default_thread_pool() -> Arc<ThreadPool> {
    static POOL_WEAK: Lazy<Mutex<Weak<ThreadPool>>> = Lazy::new(|| Mutex::new(Weak::new()));

    let mut pool_weak = POOL_WEAK.lock_expect(); // `lock_expect` is a wrapper of `lock().expect("xxx")`

    match pool_weak.upgrade() {
        Some(pool) => pool,
        None => {
            let pool = Arc::new(ThreadPool::builder().build().unwrap());
            *pool_weak = Arc::downgrade(&pool);
            pool
        }
    }
}

Warning:

warning: temporary with significant drop in match scrutinee
   --> src/thread_pool.rs:155:11
    |
155 |     match pool_weak.upgrade() {
    |           ^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(clippy::significant_drop_in_scrutinee)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee

Version:

$ cargo +nightly --version --verbose
cargo 1.63.0-nightly (4d92f07f3 2022-06-09)
release: 1.63.0-nightly
commit-hash: 4d92f07f34ba7fb7d7f207564942508f46c225d3
commit-date: 2022-06-09
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1n)
os: Arch Linux [64-bit]

@Kixiron
Copy link
Member

Kixiron commented Jun 11, 2022

The false positives also appear in for loops
Playground

use std::{sync::Mutex, collections::HashSet};

fn main() {
    let nodes: Mutex<HashSet<u8>> = Mutex::new(HashSet::new());
    let mut nodes = nodes.lock().unwrap();
    
    for node in nodes.drain() {
        println!("{node}");
    }
}
warning: temporary with significant drop in for loop
 --> src/main.rs:7:17
  |
7 |     for node in nodes.drain() {
  |                 ^^^^^^^^^^^^^
  |
  = note: `#[warn(clippy::significant_drop_in_scrutinee)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee

warning: `playground` (bin "playground") generated 1 warning

Note that this doesn't appear with just HashSet::drain(), as shown here

use std::collections::HashSet;

fn main() {
    let mut nodes = HashSet::<u8>::new();
    for node in nodes.drain() {
        println!("{node}");
    }
}
$ cargo +nightly -vV
cargo 1.63.0-nightly (4d92f07f3 2022-06-09)
release: 1.63.0-nightly
commit-hash: 4d92f07f34ba7fb7d7f207564942508f46c225d3
commit-date: 2022-06-09
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1n)
os: Ubuntu 20.4.0 [64-bit]

@alice-i-cecile
Copy link

We're hitting a false positive here too: bevyengine/bevy#5160.

@SpriteOvO
Copy link
Contributor

For me, this false positive no longer occurs in the latest version.

cargo +nightly --version --verbose
cargo 1.65.0-nightly (efd4ca3dc 2022-08-12)
release: 1.65.0-nightly
commit-hash: efd4ca3dc0b89929dc8c5f5c023d25978d76cb61
commit-date: 2022-08-12
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1q)
os: Arch Linux [64-bit]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
4 participants