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

nonminimal_bool complains about asserts in boolean expressions #11932

Closed
TomFryersMidsummer opened this issue Dec 6, 2023 · 2 comments · Fixed by #12083
Closed

nonminimal_bool complains about asserts in boolean expressions #11932

TomFryersMidsummer opened this issue Dec 6, 2023 · 2 comments · Fixed by #12083
Assignees
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

@TomFryersMidsummer
Copy link

TomFryersMidsummer commented Dec 6, 2023

Summary

nonminimal_bool seems to get confused when assert and debug_assert crop up in boolean expressions: see the example. The issue disappears if x % 2 == 0 || is removed.

It's possible I'm missing something, here, and there is a simpler way to write this. In that case, it'd be nice if the error message was a bit clearer what this was.

Lint Name

nonminimal_bool

Reproducer

I tried this code:

fn f(x: i32) -> bool {
    x % 2 == 0 || {
        assert!(x > 0);
        x % 3 == 0
    }
}

I saw this happen:

warning: this boolean expression can be simplified
 --> foo.rs:3:9
  |
3 |         assert!(x > 0);
  |         ^^^^^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
  = note: `#[warn(clippy::nonminimal_bool)]` on by default
  = note: this warning originates in the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

I expected to see this happen: [no error]

Version

rustc 1.74.0 (79e9716c9 2023-11-13)
binary: rustc
commit-hash: 79e9716c980570bfd1f666e3b16ac583f0168962
commit-date: 2023-11-13
host: x86_64-unknown-linux-gnu
release: 1.74.0
LLVM version: 17.0.4

Additional Labels

No response

@TomFryersMidsummer TomFryersMidsummer 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 Dec 6, 2023
@asquared31415
Copy link
Contributor

asquared31415 commented Dec 7, 2023

That code expands to

fn f(x: i32) -> bool {
    x % 2 == 0 ||
        {
            if !(x > 0) {
                    ::core::panicking::panic("assertion failed: x > 0")
                };
            x % 3 == 0
        }
}

and what clippy is seeing is that if !(x > 0), which could indeed be simplified.
If you run the expanded code through clippy, that is indeed where it's warning:

warning: this boolean expression can be simplified
  --> src/lib.rs:10:16
   |
10 |             if !(x > 0) {
   |                ^^^^^^^^ help: try: `x <= 0`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
   = note: `#[warn(clippy::nonminimal_bool)]` on by default

I think that it would be worth clippy special casing the assert family of macros to ignore them, because they are always constructed in this way that will never be minimal, to simplify the implementation of the macro. (Perhaps special casing all macros that start with assert_ because they will likely be similar? I'm not sure and don't feel strongly either way on that.)

@cocodery
Copy link
Contributor

cocodery commented Jan 3, 2024

@rustbot claim

bors added a commit that referenced this issue Jan 27, 2024
Fix/Issue11932: assert* in multi-condition after unrolling will cause lint `nonminimal_bool` emit warning

fixes [Issue#11932](#11932)

After `assert`, `assert_eq`, `assert_ne`, etc, assert family marcos unrolling in multi-condition expressions, lint `nonminimal_bool` will recognize whole expression as a entirety, analyze each simple condition expr of them, and check whether can simplify them.

But `assert` itself is a entirety to programmers, we don't need to lint on `assert`. This commit add check whether lint snippet contains `assert` when try to warning to an expression.

changelog: [`nonminimal_bool`] add check for condition expression
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
3 participants