rust-lang / rust-lang/rust-clippy

`drop` on non-`Drop` types is useful defensive programming

Open
#11,650 10 comments 6 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

C-bug I-false-positive
Dominant language
Rust
Stars
13.5k
Forks
2.2k
Avg merge
2d 10h
Merged PRs (30d)
32

Description

Summary

If v is a value that is not Copy, then drop(v) ensures that v is
not used after that point. A later change to the code that accidentally
reuses that variable will lead to a compiler error, preventing bugs
especially when there are multiple variables of the same type in scope.

(Morally, this is similar to shadowing, and indeed a similar effect can
be obtained by replacing drop(v) with let () = v.)

However, if this technique is used when v does not have a nontrivial
destructor, then Clippy complains with the drop_non_drop lint. This is
bad, because accepting the lint's suggestion makes the code less robust
to future changes.

Lint Name

drop_non_drop

Reproducer

I tried this code:

#[derive(Debug, Clone, PartialEq)]
pub struct State {
    x: u64,
}

pub enum Op {
    Update(u64),
    Read,
}

// This function takes a complex set of parameters (here `Op`). It reads
// from `state` and, depending on its parameters, may or may not also mutate it.
pub fn operate(op: Op, state: &mut State) -> u64 {
    match op {
        Op::Update(y) => state.x += y,
        Op::Read => (),
    };
    state.x
}

pub fn do_multiple_operations(state: &mut State) -> u64 {
    let mut temp_state = state.clone();
    let a = operate(Op::Read, &mut temp_state);
    // Double-check that we didn't mutate the state, then discard the temp state
    // so that we don't accidentally use it instead of the real one.
    assert_eq!(&temp_state, state);
    drop(temp_state);

    // If we accidentally use `&mut temp_state` instead of `state` on the next
    // line, we get an error, because we dropped it above (good).
    let b = operate(Op::Update(77), state);
    a + b
}

I saw this happen:

warning: call to `std::mem::drop` with a value that does not implement `Drop`. Dropping such a type only extends its contained lifetimes
  --> src/lib.rs:27:5
   |
27 |     drop(temp_state);
   |     ^^^^^^^^^^^^^^^^
   |
note: argument has type `State`
  --> src/lib.rs:27:10
   |
27 |     drop(temp_state);
   |          ^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_non_drop
   = note: `#[warn(clippy::drop_non_drop)]` on by default

warning: `playground` (lib) generated 1 warning

I expected to see this happen:

No lint warning, because the code is useful and working as intended.

Version
rustc 1.70.0 (90c541806 2023-05-31)
binary: rustc
commit-hash: 90c541806f23a127002de5b4038be731ba1458ca
commit-date: 2023-05-31
host: x86_64-unknown-linux-gnu
release: 1.70.0
LLVM version: 16.0.2
Additional Labels

No response

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the drop_non_drop lint entry point and reproduce the warning using the Rust example in the issue. Trace how the lint decides that drop(temp_state) is problematic, then verify that defensive drops of non-Drop values no longer produce an inappropriate warning while genuine cases remain covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
devtools
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.