rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.42k stars 1.54k forks source link

allow_attributes lint triggers inside derive macro #13349

Closed RuairidhWilliamson closed 3 days ago

RuairidhWilliamson commented 1 month ago

Summary

I tried using the new clippy lint allow_attributes on one of my projects and found many warnings for structs deriving bytemuck::Pod.

I am using bytemuck and checking the output of cargo expand there is an #[allow(clippy::missing_const_for_fn)] in the generated code.

Of course using cargo clippy --fix doesn't work in this case too because it tries to replace the #[repr(C)] with expect.

Reproducer

I tried this code:

#![expect(unused)]
#![warn(clippy::allow_attributes)]

fn main() {}

#[repr(C)]
#[derive(Clone, Copy, bytemuck::Pod, bytemuck::Zeroable)]
struct Foo {
    a: u32,
}

I expected to see this happen: No clippy lints

Instead, this happened:

warning: #[allow] attribute found
 --> src/main.rs:4:1
  |
4 | #[repr(C)]
  | ^ help: replace it with: `expect`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#allow_attributes
note: the lint level is defined here
 --> src/main.rs:2:9
  |
2 | #![warn(clippy::allow_attributes)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^^

warning: `clippy_experiment` (bin "clippy_experiment") generated 1 warning (run `cargo clippy --fix --bin "clippy_experiment"` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.52s

Version

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-unknown-linux-gnu
release: 1.81.0
LLVM version: 18.1.7

Additional Labels

No response

alex-semenyuk commented 1 month ago

@rustbot label I-false-positive

domenicquirl commented 1 month ago

Having the same issue with a derive macro we have in our own codebase. It's basically impossible to use expect inside the generated code, because the usual reason that macros output allow attributes (that is different from regular, non-macro generated code) is that the macro shouldn't produce redundant warnings for things like deprecated fields or user-defined functions. However, the macro cannot know whether any particular field will actually be deprecated and fire the lint, so it has to apply allow to the whole thing. If the lint doesn't fire, then using expect will achieve exactly the opposite of the intended effect by emitting an "lint expectation was not fulfilled" warning for seemingly no reason (from a user's perspective). Since you also cannot allow that, you'd have to expect it, which doesn't work when the original lint does fire and is caught by the generated expect.

What's the expected way for macro authors to take this into consideration for their macros? Or can this only be fixed in clippy? I tried adding #[expect(clippy::allow_attributes)] to the macro definition in desperation, but of course that doesn't help either.

GnomedDev commented 2 weeks ago

allow_attributes is already checking correctly for proc macro tokens, from what I can see in the code, but this is a macro-specific problem. Macros should not be associating output tokens with input tokens if clippy should not lint those output tokens as-if the user can change the output.

This specifically will be fixed by https://github.com/Lokathor/bytemuck/pull/279. For other macro authors, audit your span outputs to avoid confusing clippy's in_external_macro and is_from_proc_macro checks.

danielhenrymantilla commented 2 weeks ago

Macros should not be associating output tokens with input tokens if clippy should not lint those output tokens as-if the user can change the output.

I don't see this being even remotely sustainable for the ecosystem: spans can be used for many things, be it hygiene, or to relocate diagnostics w.r.t. certain errors.

What about:

#[expect(clippy::allow_attributes)]
#[allow(warnings, clippy::all)] // <- or more fine-grained ones for the most maintained macros

If the above code does not prevent triggering the lint, then the lint is too restrictive.

EDIT: it looks like this works too 🙂

y21 commented 2 weeks ago

Clippy's is_from_proc_macro (which this lint already uses) was created specifically for this type of issue (detecting proc macros returning an input span which is not detected by Span::from_expansion()), so while fixing these issues directly in proc macros is good, part of this also sounds like a bug in our is_from_proc_macro function.

domenicquirl commented 1 week ago

What about:

[expect(clippy::allow_attributes)]

[allow(warnings, clippy::all)] // <- or more fine-grained ones for the most maintained macros

If the above code does not prevent triggering the lint, then the lint is too restrictive.

EDIT: it looks like this works too 🙂

@danielhenrymantilla I just tried this on some of our macros and couldn't get it to silence the lint on the #[allow], it still fired. Am I holding it wrong? How did you apply the attributes in your testing?

danielhenrymantilla commented 1 week ago

@domenicquirl Hmm, I tried with a basic snippet with everything inline, no macros involved. So I probably got ahead of myself when I said that "it looks like it works": maybe the #[expect…]/#[allow…] is, itself, being ignored when emitted from a macro?

domenicquirl commented 1 week ago

@danielhenrymantilla the issue only occurs if the #[expect] is emitted from a macro. Here's an example playground where the same function is written with the code inline and generated by a macro. The lint only fires on the #[allow] inside the macro-generated code, the same #[allow] in the inline function is not linted.

danielhenrymantilla commented 1 week ago

That's what I was fearing from what you had mentioned. Then that is, indeed, a bug with clippy, good catch.

RuairidhWilliamson commented 1 week ago

@rustbot claim

blyxyas commented 1 week ago

EDIT: it does not seem to reproduce on beta?

For me, it still reproduces on nightly, beta and stable.