nonminimal_bool and macros

[ijackson]

Summary

When writing macros which expand to boolean expressions, it is sometimes best to write a nonminimal expression.

For example, in an MR in Arti I have this code:

    #[allow(clippy::nonminimal_bool)]
    fn same_relay_ids<T: HasRelayIds + ?Sized>(&self, other: &T) -> bool {
        derive_deftly_adhoc! {
            RelayIdType:
            $(
                self.identity($vtype) == other.identity($vtype) &&
            )
                true
        }
    }

The body expands to:

                self.identity(RelayIdType::Ed22519) == other.identity(RelayIdType::Ed22519) &&
                self.identity(RelayIdType::Rsa) == other.identity(RelayIdType::Rsa) &&
                true

This triggers the lint. But there isn't a better way to write it.

This is with derive-deftly. I think similar situations could arise with macro_rules! macros.

I suggest either or both of:

• Don't lint when boolean literals true and false are involved. People who wrote those probably did so deliberately.
• Don't lint when the expression is generated from a proc macro.

Lint Name

nonminimal_bool

Reproducer

To reproduce

git clone https://gitlab.torproject.org/Diziet/arti
cd arti
git checkout relay-id-perf-2-clippy-repro
cargo clippy --locked -p tor-linkspec

Actual output

warning: this boolean expression can be simplified
   --> crates/tor-linkspec/src/traits.rs:118:17
    |
118 | /                 self.identity($vtype) == other.identity($vtype) &&
119 | |             )
120 | |                 true
    | |____________________^ help: try: `self.identity($vtype) == other.identity( && self.identity($vtype) == other.identity(`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
note: the lint level is defined here
   --> crates/tor-linkspec/src/lib.rs:9:9
    |
9   | #![warn(clippy::all)]
    |         ^^^^^^^^^^^
    = note: `#[warn(clippy::nonminimal_bool)]` implied by `#[warn(clippy::all)]`

warning: `tor-linkspec` (lib) generated 1 warning

Expected output

No complaint

Version

rustc 1.77.0-beta.5 (f2043422f 2024-02-17)
binary: rustc
commit-hash: f2043422f7b161a2fc1a00589a8c4956db963450
commit-date: 2024-02-17
host: x86_64-unknown-linux-gnu
release: 1.77.0-beta.5
LLVM version: 17.0.6

Additional Labels

No response

[ijackson]

I tried to make a repro with macro_rules! but https://play.rust-lang.org/?version=beta&mode=debug&edition=2021&gist=0763e09ef35c45901bcabb35177fecb6 doesn't trigger the lint. I don't know why not.

[y21]

The lint already sort of tries to ignore macros if the span is marked to come from one IIRC, but proc macros tend to return their tokens with the span of the call site, tricking clippy into thinking that it's what the user wrote. So proc macros often need some special hacks handling. That may be what's going on there

[ijackson]

proc macros tend to return their tokens with the span of the call site,

Right. (As it happens, I'm the principal author of derive-deftly, and I can confirm that the output tokens typically, including in this case, will have spans from the input.)

So proc macros often need some special hacks handling. That may be what's going on there

That sounds very plausible. I'm kind of hoping there is a standard approach to this in clippy? It doesn't seem like this should be a new problem...

[J-ZhengLi]

often time an additional clippy_utils::is_from_proc_macro check should suffice

[antonilol]

isnt it common to just turn off specific lints on generated code (with decl/proc macros)?

when using proc macros it should be easy to add some extra code to avoid this

when using a declarative macro (syntax is different that what you got there), this can be done to avoid the lint. i think you can trust the optimizer with this:
[$(self.identity($vtype) == other.identity($vtype)),*].into_iter().all(|b| b)

[ijackson]

isnt it common to just turn off specific lints on generated code (with decl/proc macros)?

when using proc macros it should be easy to add some extra code to avoid this

Suppressing a lint with #[allow] can only be done on whole functions. Attributes on blocks and expressions are unstable. There isn't any difference in what proc macros and macro_rules macros can do here.

So a proc macro that expands to an expression cannot disable this lint for its output. Neither could a macro_rules macro, but according to #12627 (comment) this particular lint is already inherently disabled in proc_macro output.

when using a declarative macro (syntax is different that what you got there), this can be done to avoid the lint. i think you can trust the optimizer with this: [$(self.identity($vtype) == other.identity($vtype)),*].into_iter().all(|b| b)

Such a workaround is indeed possible in this case. (Hence "it is sometimes best", in my original report, recognising that there are other ways of writing it, but they're worse.) It might be a sensible tactic to adopt for a particular project. But:

The purpose of these kind of clippy lints is to encourage people to write code which is clearer, more readable, and more normal. If clippy complains about straightforward code, and demands the user write complex or unnatural code to placate it, clippy is defeating its own objective.

Ie, the possibility of a clumsy workaround is helpful - but it's not a reason to close a false positive bug report like this one.

I think the right answer is to disable this lint for proc_macro output, as it already is for macro_rules output. Probably, as proposed in #12627 (comment)

[antonilol]

Suppressing a lint with #[allow] can only be done on whole functions. Attributes on blocks and expressions are unstable. There isn't any difference in what proc macros and macro_rules macros can do here.

apparently that does not hold for all expressions, the following code works on stable:

fn main() {
    #[allow(clippy::nonminimal_bool)]
    {
        // no warning
        let _ = true && false;
    }

    // warning
    let _ = true && false;
}

i agree with the rest, the workaround i gave can some time in the future also get a lint, something like "[].into_iter().all(|b| b) may be replaced with true"

[ijackson]

apparently that does not hold for all expressions, the following code works on stable:

So it does! I think I have been misled by a compiler message, possibly at some point in the past. That's a much more palatable workaround.

[lochetti]

often time an additional clippy_utils::is_from_proc_macro check should suffice

If we want to add this check, the change is pretty simple and I would be happy to do. Tbh, I am just not so sure if we need/want this, right now.