-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Support user format-like macros #9948
base: master
Are you sure you want to change the base?
Conversation
r? @Alexendoo (rust-highfive has picked a reviewer for you, use r? to override) |
d9c9b8b
to
362a2c9
Compare
There are some reasonable situations where inline args can't be used, such as macro_rules! error {
($fmt:literal $(, $e:expr)* $(,)?) => { println!(concat!("ERROR: ", $fmt), $($e,)*) }
}
fn main() {
let x = 1;
error!("{}", x);
} Another thing is that it could break macro invocations themselves, initially I wrote that as |
362a2c9
to
725ca3f
Compare
@Alexendoo I just added your example to my unit tests - it passes just fine. Do you have a test that fails? |
6b83237
to
0bdf235
Compare
macro_rules! error {
($fmt:literal, $($e:expr),*) => { println!($fmt, $($e,)*) }
}
fn main() {
let x = 1;
error!("{}", x);
} |
0bdf235
to
60f8dcc
Compare
@Alexendoo that's an excellent catch, thank you! The issue here is with the bug in macro definition (granted that macros are a special beast and a "bug" could be someone's feature). TBH, I am surprised there is no lint that tries to catch this mistake. The bigger question though is if this is something we should or should not process? I.e. something that suggests a cleaner repeated macro arg definition. I would even make this as a That said, on one hand we have badly created user macros that probably should be fixed. On the other, there is a very common pattern with all sorts of log and other macros that use format strings and could really use this lint. Which of these takes priority? |
@Alexendoo I created a lint suggestion #9959 that would solve this types of issues - having both lints on at the same time would fix the issue i think |
It would be neat to be able to lint macro definitions, however as they are almost entirely represented as a big Another one to bring up is that a common way of defining things like this is two arms, so that lint wouldn't be generally applicable ($fmt:literal) => ...
($fmt:literal, $($e:expr),*) => ... e.g. the That would make the suggestion change the behaviour of the code, silently even if the printed variable is otherwise used My biggest concern is probably all the proc macros out there, the current implementation is not super robust against them. If a proc macro did an internal format!() but happened to use certain input spans we could be matching up a completely different format string than the one actually being expanded To that end it may be worth keeping an eye on rust-lang/rust#99012 / rust-lang/compiler-team#541 |
Another fun one: macro_rules! used_twice {
(
large = $large:literal,
small = $small:literal,
$val:expr,
) => {{
if $val < 5 {
println!($small, $val);
} else {
println!($large, $val);
}
}}
}
fn main() {
let x = 1;
used_twice! {
large = "large value: {}",
small = "small value: {}",
x,
}
} |
I have a better idea - let's disable Rust macros, as they are clearly a work of Cthulhu, designed to steer us away from the light... as this is clearly a case of "I miss Perl, so I will make things as interesting as possible". I don't think Clippy should care about such cases TBH as they probably represent 1% - when on the other hand we have 99% of the simple |
0b3e8b5
to
7239dd6
Compare
Please keep in mind that we deem false positives a worse outcome than false negatives. I'd be OK with having either a config that makes the lint apply everywhere instead of just format while making the suggestion |
@llogiq I think we should also take into account how often we get false positive vs false negative. I agree that if they are about the same, false positive is worse, but if we get 100x more false negatives, I would prefer get a few more false positives. I like the idea of an extra config param, something like |
I would fully support that argument for a With that said, as long as the user explicitly asks for it, I'm ok with changing this balance. |
☔ The latest upstream changes (presumably #9860) made this pull request unmergeable. Please resolve the merge conflicts. |
7239dd6
to
26b9957
Compare
☔ The latest upstream changes (presumably #9865) made this pull request unmergeable. Please resolve the merge conflicts. |
26b9957
to
d12944f
Compare
6249c11
to
b8ccdfe
Compare
Partial no-op refactoring of #9948 This contains some prep work for #9948 to keep that change to the minimum, and make it easier to review it. This should be a noop, but it has some tests from that PR discussion, and should help in the future with the corner case format handling. cc: `@Alexendoo` `@llogiq` `@xFrednet` as the 3 people who reviewed the parent PR
Partial no-op refactoring of #9948 This contains some prep work for #9948 to keep that change to the minimum, and make it easier to review it. This should be a noop, but it has some tests from that PR discussion, and should help in the future with the corner case format handling. cc: `@Alexendoo` `@llogiq` `@xFrednet` as the 3 people who reviewed the parent PR ---- changelog: none
This contains preparatory work for rust-lang#9948 to keep that change to the minimum, and make it easier to review it.
b6122b4
to
69259c0
Compare
Took a bit of a year+ pause on this one.. Still, seems there is plenty of interest in this, so now this lint is rebased and cleaned up a bit. The question still remains: how can we progress. Perhaps someone has a better idea after Clippy has improved so much over the last year? @Alexendoo had some comments back then - do we still want to go with a one-per-project allow-list of valid crates? Feels really messy. Maybe we should duplicate all format lints, e.g. create My original thoughts were posted in the log crate. automaticFully automatic - decide at the linting time if PROs: easiest to use everyone, and this approach can be used by many other types of lints to validate replacements instead of declaring them as attributeCreate an attribute that macro developers can use to indicate that their macro is format! compatible:
PROs: allows macro authors to declare a macro as OK for Clippy to process as a format-like macro for many lints configuration-basedadd a Clippy config param for end users to list all macros they will want to inline, plus provide a built-in list of all well-known format-like macros in popular crates. If the user wants to extend the built-in ones, they can just use the This may require more "magic" options: PROs: easy to configure by end user, can be applied to older crates |
☔ The latest upstream changes (presumably #12269) made this pull request unmergeable. Please resolve the merge conflicts. |
b41c598
to
7f29eff
Compare
My preferred solution would be the tool attribute applied to macro definitions, e.g. #[clippy::format_args_compatible]
macro_rules! log {
// ... Config based relies on assuming third party crate layouts to be widely useful which we're trying not to expand, I think putting it in the macro authors hands is preferable there If there's a good heuristic that works for automatic detection I would be fine with that, but I'm not sure if there is one |
@Alexendoo do you know of any example of an attribute set on a macro rule, and Clippy using that attribute while parsing the expansion of the macro? We could introduce a new |
I don't think we have any cases where it's used, but Linting macro definitions is considered too difficult because the only thing you get from unexpanded macros is a token stream |
☔ The latest upstream changes (presumably #12635) made this pull request unmergeable. Please resolve the merge conflicts. |
4b61a18
to
48385e7
Compare
Hey, this is a ping from triage. @Alexendoo can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using @rustbot ready |
☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts. |
7d46049
to
6fb3013
Compare
Ah sorry missed that notification @xFrednet, my position is still if we're doing this it should be through an attribute rather than configuration |
☔ The latest upstream changes (presumably #13173) made this pull request unmergeable. Please resolve the merge conflicts. |
e21624b
to
f1ea9ff
Compare
@Alexendoo i updated this PR to rely on |
f1ea9ff
to
0edfbef
Compare
Add support for `#[clippy::format_args]` attribute that can be attached to any macro to indicate that it functions the same as the built-in format macros like `format!`, `println!` and `write!`
0edfbef
to
58b6bd6
Compare
☔ The latest upstream changes (presumably #13440) made this pull request unmergeable. Please resolve the merge conflicts. |
Add support for
#[clippy::format_args]
attribute that can be attached to any macro to indicate that it functions the same as the built-in format macros likeformat!
,println!
andwrite!
changelog: Enhancement: [
format_in_format_args
], [recursive_format_impl
], [to_string_in_format_args
], [uninlined_format_args
], [unused_format_specs
]: Recognizes#[clippy::format_args]
to support custom 3rs party format macros.