Open ijackson opened 6 months ago
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.
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
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...
often time an additional clippy_utils::is_from_proc_macro
check should suffice
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)
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 https://github.com/rust-lang/rust-clippy/issues/12627#issuecomment-2035068837 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 https://github.com/rust-lang/rust-clippy/issues/12627#issuecomment-2035120772
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
"
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.
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.
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:
The body expands to:
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:
true
andfalse
are involved. People who wrote those probably did so deliberately.Lint Name
nonminimal_bool
Reproducer
To reproduce
Actual output
Expected output
No complaint
Version
Additional Labels
No response