Turn declare_clippy_lint into a declarative macro #13359

blyxyas · 2024-09-06T11:57:30Z

Ease of development, and hopefully compile times (the dependencies are still there because of ui-test). The procedural macro was doing just some very basic processing (like assigning a lint level to each category), so it didn't have a reason to stay IMO

changelog: None

rustbot · 2024-09-06T11:57:34Z

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

xFrednet · 2024-09-06T12:31:57Z

Maybe:
r? @Alexendoo since I believe that you developed the macro originally?

Alexendoo · 2024-09-06T13:33:46Z

Recompile time seems pretty similar, maybe try ${concat} instead of paste but my impression is proc macros aren't really slow

flip1995 · 2024-09-06T14:01:28Z

clippy_utils/src/declare_clippy_lint.rs

This shouldn't be in the clippy_utils crate, as this is not something users of that crate should use.

So, should it be in clippy_lints/lib.rs? Or somewhere else maybe

In clippy_lints probably yeah. But not necessarily in lib.rs. But lib.rs should at least re-export it.

Moving it from clippy_utils to clippy_lints would need prepending all lint files with use crate::declare_clippy_lint. It can be done a simple bash script, but is it what we want?

I'm not sure why this isn't the case anymore, but when the macro was in clippy_utils just auto-imported it into the lint modules.

It's because of #[macro_use]

rust-clippy/clippy_lints/src/lib.rs

Lines 61 to 62 in 78bdd45

#[macro_use]

extern crate clippy_utils;

To do the same for a local macro you can either #[macro_export] it or put it in a #[macro_use] module

I know it's because of the #[macro_use], but giving it to another module is not working. (I've tried both with all combinations of #[macro_export] on the macro and #[macro_use] on the module)

I've tried both in clippy_lints/lib.rs, in a separate file and in the clippy_lints::utils module. Still, not working :/

Oh there's also that it needs to be defined above the lint modules that use the macro for some reason

blyxyas · 2024-09-06T14:57:44Z

maybe try ${concat} instead of paste

Before using paste, I tried with concat_idents, we cannot declare a value with static and concat_idents (nor do any macro magic for this to happen AFAIK)

proc macros aren't really slow

In the common nightly the diference isn't all that great, I've just tried with the parallel compiler (-Z threads=8) and the difference is 23 seconds, both from cargo clean.

Alexendoo · 2024-09-06T15:25:39Z

${concat} does not have the same problems concat_idents does

blyxyas · 2024-09-06T15:52:46Z

Fixed, I didn't know about ${concat}, seems very useful (and possibly a general replacement for paste!)

blyxyas · 2024-09-13T18:29:12Z

Oh, something I forgot to mention. The main motivator behind this change is rust-lang/rust#125116. I need to mark some lints as always evaluated (so that, even if they are allow by default and are not manually triggered, they still run).

The proc macro API is hard to work with, and I wasn't getting to the point of accepting "[eval_always: true]" into the proc macro optionally. That's the reason I translated it, and most of the reason.

bors · 2024-09-22T19:12:51Z

☔ The latest upstream changes (presumably #13440) made this pull request unmergeable. Please resolve the merge conflicts.

Alexendoo · 2024-09-22T19:37:39Z