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

Consider deleting FORMAT_MACRO_DIAG_ITEMS, and tag the macros in stdlib #14267

Open
nyurik opened this issue Feb 21, 2025 · 5 comments
Open

Consider deleting FORMAT_MACRO_DIAG_ITEMS, and tag the macros in stdlib #14267

nyurik opened this issue Feb 21, 2025 · 5 comments

Comments

@nyurik
Copy link
Contributor

nyurik commented Feb 21, 2025

Now that #[clippy::format_args] has been released, should we delete the FORMAT_MACRO_DIAG_ITEMS list, and instead tag all relevant macros with the new attribute in the stdlib?

Benefits

  • Keep each macro's metadata next to the declaration.
  • No need to track when new relevant macros are written
  • Improves discoverability of the #[clippy::format_args], as any developer writing format-like macro will see it in the declaration
@Centri3
Copy link
Member

Centri3 commented Feb 21, 2025

Personally think this is a good idea 👍

nyurik added a commit to nyurik/rust that referenced this issue Feb 21, 2025
Since 1.85, Clippy [supports](See https://doc.rust-lang.org/nightly/clippy/attribs.html#clippyformat_args) attribute-based discovery of the `format!`-compatible macros, i.e. macros whose trailing arguments can be passed to `format_args!` as is.

Tagging core library with the same attribute will allow clippy to stop maintaining a separate list of format-like macros - allowing Clippy to validate macro usage for all lints that work on format arguments. See also rust-lang/rust-clippy#14267
nyurik added a commit to nyurik/rust that referenced this issue Feb 21, 2025
Since 1.85, Clippy [supports](See https://doc.rust-lang.org/nightly/clippy/attribs.html#clippyformat_args) attribute-based discovery of the `format!`-compatible macros, i.e. macros whose trailing arguments can be passed to `format_args!` as is.

Tagging core library with the same attribute will allow clippy to stop maintaining a separate list of format-like macros - allowing Clippy to validate macro usage for all lints that work on format arguments. See also rust-lang/rust-clippy#14267
@blyxyas
Copy link
Member

blyxyas commented Mar 10, 2025

Do we have a certain method that returns all items with certain method?
Benchmarking rust#137364 is taking more effort than I think it's worth, as I don't have the workflow for benchmarking rust changes from Clippy.

I can absolutely see the value in not having to maintain the independent list and the ergonomics in adding new macros, but I don't think it's worth it compared to all macros going to the not happy path which is much more complex.

I strongly think that it's not worth it, we can nominate the issue for the next meeting. Or if someone wants to chime in and do the benchmarks for me, that would be great!

@nyurik
Copy link
Contributor Author

nyurik commented Mar 10, 2025

@blyxyas, in the rust-lang/rust#137364 - how were you able to work around the rust-lang/rust#98291 compilation issue? Are there any compiler experts who may know how to fix it?

Note that the current implementation in Clippy utils always does the macro check - i.e. it first chacks if the macro is in the "known list", and if it is not, and still checks if that macro has the #[clippy::format_args] attribute. So switching this to "always check" might be slightly slower in cases of format-using macros, but slightly faster for all other macros as they will skip the first check.

@blyxyas
Copy link
Member

blyxyas commented Mar 10, 2025

The lint detection code checks for expansion != LocalExpnId::ROOT before reporting the bug. It could be that the attribute macro is getting your macro_exported macro a new LocalExpnID that isn't root and isn't caused by the macro_export itself (which I don't think expands to anything, it's just a marker)

@blyxyas
Copy link
Member

blyxyas commented Mar 10, 2025

By the way, I was not able to work around it :( the main culprit of the difficulties for benchmarking rust from Clippy is that you can either:

  1. Inject Clippy into a commit-specific rust repository, and try to make that work.
  2. Use a commit-specific Rust into Clippy with lintcheck --perf it involves building rust, linking rustc_driver.so with a newly built Clippy version also from the Rust repo...

After mentalizing about what was the game plan (each PR to benchmark is different, much more if it's from rustc), and seeing the not-compiling warning... Well, it was not great.

I'm sure that in the future apart from flying cars we'll have a more ergonomic Rust-into-Clippy and backwards linking workflow. (I hope)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants