drop_copy should not trigger when drop is used in a match branch

[ia0]

Summary

The lint description says:

drop does nothing for types that implement Copy

This is wrong, drop can be used to forget that a function returns a non-unit type when the result value is sometimes useless.

Lint Name

drop_copy

Reproducer

I tried this code:

fn foo() -> u8 {
    println!("doing foo");
    0 // result is not always useful, the side-effect matters
}
fn bar() {
    println!("doing bar");
}

fn main() {
    match 42 {
        0 => drop(foo()),  // drop is needed because we only care about side-effects
        1 => bar(),
        _ => (),  // doing nothing (no side-effects needed here)
    }
}

I saw this happen:

error: calls to `std::mem::drop` with a value that implements `Copy`. Dropping a copy leaves the original intact
  --> src/main.rs:11:14
   |
11 |         0 => drop(foo()),  // drop is needed because we only care about side-effects
   |              ^^^^^^^^^^^
   |
   = note: `#[deny(clippy::drop_copy)]` on by default
note: argument has type `u8`
  --> src/main.rs:11:19
   |
11 |         0 => drop(foo()),  // drop is needed because we only care about side-effects
   |                   ^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_copy

I expected to see this happen:

No error

Version

rustc 1.63.0 (4b91a6ea7 2022-08-08)
binary: rustc
commit-hash: 4b91a6ea7258a947e59c6522cd5898e7c0a6a88f
commit-date: 2022-08-08
host: x86_64-unknown-linux-gnu
release: 1.63.0
LLVM version: 14.0.5

Additional Labels

No response

[kraktus]

I don't think this use of drop is idiomatic, do you have any reference or example in "canonical" code? You would do => {foo();} or let _ = foo(); which the common way to discard unused return values

[ia0]

I don't think this use of drop is idiomatic

That's a fair point. See below for a solution if we go in that direction.

do you have any reference or example in "canonical" code

• wasmtime
• rust-analyzer (the types are not copy, but drop is used to unify the different types of each branch to unit which is the point I'm making: "drop is used for typing")
• cargo (discarding a Result)
• clippy (discarding a Result)

You would do => {foo();} or let _ = foo(); which the common way to discard unused return values

This hurts readability:

match x with {
    A => { // 3 lines
        foo();
    }
    B => { // 3 lines
        let _ = foo();
    }
    C => drop(foo()),
}

If the problem is that clippy wants to argue that drop should only be used for dropping and not for the additional type conversion it provides, then one solution I would see would be for the standard library to provide the following function like OCaml does:

pub fn ignore<T>(_x: T) {} // This is the exact same implementation as drop.

Until this function is provided, clippy should not complain about drop being used instead of ignore.

Once ignore is provided, clippy should complain about the following:

• drop being used for type conversion only (should suggest to use ignore instead)
• ignore being used to drop (should suggest to use drop instead)

Here is how I would compare both solutions:

• "Disabling drop_copy lint when there's no auto-ignore (i.e. everywhere but when expressions are used as statements or inside a let _ = which is similar to an expression being used as a statement)"
  • Pro: No need to extend the standard library.
  • Pro: Introduces less noise in case of migration (e.g. a type being suddenly copy)
  • Con: Doesn't catch mistakes where the user thought dropping would do something
• "Introducing ignore and an associated ignore_drop lint"
  • Pro: Clear dissociation between drop (runtime behavior) and ignore (compile-time behavior)
  • Pro: Prevents mistakes where the user has wrong expectations
  • Con: May introduce some noise in case of migration (e.g. a drop usage doing both runtime and compile-time behavior suddenly doing only compile-time behavior, or an ignore usage suddenly starting to do runtime behavior)

[kraktus]

Thanks for the examples. So it seems drop is preferred over the let _ pattern in expression for brevity. I think that by not linting it when it's an match branch we would allow this pattern while keeping false-negative low enough

[ia0]

Sounds good to me. I also think that silencing the lint at top-level of match branches (i.e. <pat> => drop(<expr>), pattern) should be enough indeed.