debug_assert_with_mut_call: Multiple kinds of false positives in a default-deny lint

[adlerd]

debug_assert_with_mut_call is a "correctness" lint with major false positives! I'm not sure what the policy is on correctness lints, but the readme defines it like this: "code that is just outright wrong or very very useless". Instances of debug_assert_with_mut_call are often neither of these.

I believe the spirit of this lint is to prevent side-effects escaping debug_assert. In that light, I have identified two issues with this lint, can be seen with the following code:

pub fn example1() {
    use std::sync::Mutex;

    let mut mutex = Mutex::new(10);

    // get_mut is used to avoid a lock, not for mutation: there is no equivalent get().
    // The function requires &mut self to guarantee uniqueness.
    debug_assert_eq!(*mutex.get_mut().unwrap(), 10);
}

pub fn example1b() {
    use std::cell::Cell;
    let cell = Cell::new(10);

    debug_assert!({
        cell.set(42);
        true
    });
}

pub fn example2() {
    let vec = vec![3, 1, 7, 5];

    debug_assert_eq!(
        {
            let mut sorted = vec.clone();
            // sort_unstable() does mutate something,
            // but its side-effects do not propagate outside of the debug_assert.
            sorted.sort_unstable();
            sorted
        },
        [1, 3, 5, 7]
    );

    drop(vec); // avoid a different uninteresting lint
}

Running clippy 0.0.212 (69f99e7 2019-12-14) with default settings yields

error: do not call a function with mutable arguments inside of `debug_assert_eq!`
 --> src/lib.rs:8:23
  |
8 |     debug_assert_eq!(*mutex.get_mut().unwrap(), 10);
  |                       ^^^^^^^^^^^^^^^
  |
  = note: `#[deny(clippy::debug_assert_with_mut_call)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#debug_assert_with_mut_call

error: do not call a function with mutable arguments inside of `debug_assert_eq!`
  --> src/lib.rs:29:13
   |
30 |             sorted.sort_unstable();
   |             ^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#debug_assert_with_mut_call

error: aborting due to 2 previous errors

error: could not compile `badlint`.

example1 and the corresponding lint output demonstrate the first problem with this lint: it assumes that &mut has something to do with mutation. This is a common misconception discussed here and here. There is no such thing as "a function with mutable arguments", or at least, not in the way the lint means it. example1 is perfectly fine code which is not "outright wrong or very very useless". example1b, on the other hand, does meet that definition and falls directly within both the spirit and letter of this lint, but is not caught. Yikes!

example2 demonstrates the other problem, which is in some ways I think worse because it encourages bad practice. This example has code with side-effects which don't escape from debug_assert; the idea is that there's no need to perform those side-effects if their result won't be used. With this lint, though, devs are likely to move those side-effects out of the debug assert, slowing down release code by adding dead code that often can't be eliminated because of those very side-effects.

I hit both of these issues in real-life code which did not have any actual bugs in the linted code; nor could it be trivially refactored without introducing new lints or new bugs. Therefore, while the intention behind the lint is good, I believe it needs at a minimum to be downgraded to default-warn until these issues are resolved. FWIW, there have been other recent issues with this lint as well: #4737 and #5105.

[flip1995]

Yeah, we should move this to nursery. The & mut check was a heuristic. But not a good one.
I'll move it in #5106

[flip1995]

We move this lint to nursery (allow-by-default) and backport it to the current beta, so that this bug is gone in 1.42.0. In the mean time, you should just #![allow(clippy::debug_assert_with_mut_call)] this lint in lib.rs/main.rs.

Sorry for the inconvenience.

[phimuemue]

This is another case which - in my opinion - is a false positive:

fn main() {
    let v1 = vec![0,1];
    debug_assert_eq!(
        (0..2).fold(Vec::new(), |mut v, n| {v.push(n); v}),
        v1
    );
}