Skip to content

False postive for if-let-rescope on hashbrown::HashMap::get? #137411

New issue

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

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

Already on GitHub? # to your account

Closed
Imberflur opened this issue Feb 22, 2025 · 9 comments · Fixed by #137444
Closed

False postive for if-let-rescope on hashbrown::HashMap::get? #137411

Imberflur opened this issue Feb 22, 2025 · 9 comments · Fixed by #137444
Assignees
Labels
A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. I-edition-triaged Issue: This issue has been reviewed and triaged by the Edition team. L-false-positive Lint: False positive (should not have fired). L-if_let_rescope Lint: if_let_rescope T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Imberflur
Copy link

hashbrown::HashMap::get returns a reference which has no significant drop.

I've see this on other methods like hashbrown::HashMap::get_mut, the equivalent methods on std::collections::HashMap don't seem to have this false positive.

Code

#![warn(if_let_rescope)]
#![allow(unused_variables)]

fn main() {
    let h = hashbrown::HashMap::<u32, u32>::default();
    if let Some(value) = h.get(&0) {
        // do something
    } else {
        // do something else
    }
}

Code in the playground.

Current output

warning: `if let` assigns a shorter lifetime since Edition 2024
 --> src/main.rs:6:8
  |
6 |     if let Some(value) = h.get(&0) {
  |        ^^^^^^^^^^^^^^^^^^-^^^^^^^^
  |                          |
  |                          this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
  |
  = warning: this changes meaning in Rust 2024
  = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-if-let-scope.html>
help: the value is now dropped here in Edition 2024
 --> src/main.rs:8:5
  |
8 |     } else {
  |     ^
note: the lint level is defined here
 --> src/main.rs:1:9
  |
1 | #![warn(if_let_rescope)]
  |         ^^^^^^^^^^^^^^
help: a `match` with a single arm can preserve the drop order up to Edition 2021
  |
6 ~     match h.get(&0) { Some(value) => {
7 |         // do something
8 ~     } _ => {
9 |         // do something else
10~     }}
  |
@Imberflur Imberflur added the C-bug Category: This is a bug. label Feb 22, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 22, 2025
@Imberflur
Copy link
Author

Another case with Arc::get_mut.
Seems to only occur when the contained type implements Drop.

#![warn(if_let_rescope)]
#![allow(unused_variables)]
use std::sync::Arc;

pub struct Droppy;
impl Drop for Droppy {
    fn drop(&mut self) {}
}

pub struct Foo {
    field: Arc<Droppy>,
}
impl Foo {
    // Triggers lint
    pub fn foo(&mut self) {
        if let Some(value) = Arc::get_mut(&mut self.field) {
            // do something
        } else {
            // do something else
        }
    }
}

// Triggers lint
pub fn foo1(mut arc: Arc<Droppy>) {
    if let Some(value) = Arc::get_mut(&mut arc) {
        // do something
    } else {
        // do something else
    }
}

/// Does NOT trigger lint
pub fn foo2(arc: &mut Arc<Droppy>) {
    if let Some(value) = Arc::get_mut(arc) {
        // do something
    } else {
        // do something else
    }
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=cc979cdabe0e185c6b3464c26aadbebd

@jieyouxu jieyouxu added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. L-if_let_rescope Lint: if_let_rescope L-false-positive Lint: False positive (should not have fired). and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 22, 2025
@GnomedDev
Copy link
Contributor

GnomedDev commented Feb 22, 2025

It seems like this doesn't even need Arc/HashMap, although this might be a separate issue:

#![warn(if_let_rescope)]
#![allow(unused_variables, clippy::needless_else)]

pub struct SigDrop;
impl Drop for SigDrop {
    fn drop(&mut self) {}
}

pub fn with_option() {
    let sigdrop = Some(SigDrop);
    if let Some(sigdrop_ref) = &sigdrop { // Triggers here
    } else {
    }
}

#[allow(irrefutable_let_patterns)]
pub fn irrefutable_pattern() {
    let sigdrop = SigDrop;
    if let sigdrop_ref = &sigdrop { // Triggers here
    } else {
    }
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=19b83a57725cb634d73a36a7ae5aad25

This ends up triggering all over the https://github.com/serenity-rs/serenity codebase, making it hard to find a true positive.

@GnomedDev
Copy link
Contributor

GnomedDev commented Feb 22, 2025

Is this a duplicate of that? My irrefutable_pattern repro doesn't require enums at all? @traviscross

@theemathas
Copy link
Contributor

I don't think this is a duplicate. In this case, the value being rescoped doesn't even implement Drop. The other issue is about the lint not being able to reason that dropping None is a no-op.

@Noratrieb Noratrieb reopened this Feb 22, 2025
@theemathas
Copy link
Contributor

theemathas commented Feb 22, 2025

Another reproducer, which probably more clearly shows the cause of the issue:

#![warn(if_let_rescope)]

pub struct SigDrop;
impl Drop for SigDrop {
    fn drop(&mut self) {}
}

#[allow(irrefutable_let_patterns, dropping_references)]
pub fn irrefutable_pattern() {
    let sigdrop = SigDrop;
    if let () = drop(&sigdrop) { // Triggers here
    } else {
    }
}

It seems that merely creating a reference to a value that implements drop is enough to trigger the lint. In the hashmap example, it's presumably due to creating a reference to the hashmap itself. I don't know why the lint doesn't fire for the std hashmap though. (Maybe it's a may_dangle somewhere?)

@theemathas

This comment has been minimized.

@rustbot rustbot added the A-edition-2024 Area: The 2024 edition label Feb 22, 2025
@compiler-errors compiler-errors self-assigned this Feb 22, 2025
@steffahn
Copy link
Member

steffahn commented Feb 22, 2025

One more example for this issue:

#![warn(if_let_rescope)]

pub struct SigDrop;
impl Drop for SigDrop {
    fn drop(&mut self) {}
}

pub fn f() {
    let s: SigDrop; // uninitialized :-)
    if let _ = match s {
        _ => (),
    } {
    } else {
    }
}

(playground)

here, s is not even borrowed, heck, not even initialized.

It looks like it is just treating place expressions as if they’re values?

@compiler-errors
Copy link
Member

#137444 should fix this. It doesn't fix #137376, which would require the lint to be reworked further to be sensitive about match exhaustiveness.

@traviscross traviscross added the I-edition-triaged Issue: This issue has been reviewed and triaged by the Edition team. label Feb 25, 2025
@bors bors closed this as completed in 4e3edce Feb 25, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 25, 2025
Rollup merge of rust-lang#137444 - compiler-errors:drop-lint, r=oli-obk

Improve behavior of `IF_LET_RESCOPE` around temporaries and place expressions

Heavily reworks the `IF_LET_RESCOPE` to be more sensitive around 1. temporaries that get consumed/terminated and therefore should not trigger the lint, and 2. borrows of place expressions, which are not temporary values.

Fixes rust-lang#137411
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. I-edition-triaged Issue: This issue has been reviewed and triaged by the Edition team. L-false-positive Lint: False positive (should not have fired). L-if_let_rescope Lint: if_let_rescope T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants