-
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
Add new literal_string_with_formatting_arg
lint
#13410
base: master
Are you sure you want to change the base?
Add new literal_string_with_formatting_arg
lint
#13410
Conversation
literal_string_with_formatting_arg
lint
}) | ||
.collect::<Vec<_>>(); | ||
if spans.len() == 1 { | ||
span_lint( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could emit a suggestion for both cases by wrapping the literal into &format!()
. Not sure if it's a good idea since it could be wrong in a lot of cases so for now I didn't do it.
Would like to see a test for malformed fmt attemps such as |
|
||
// Should not lint! | ||
format!("{y:?}"); | ||
println!("{y:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
println!("{y:?}"); | |
println!("{y:?}"); | |
let code = "fn main {\n\ | |
}"; |
I did a little bit of testing, and it seems to work like it should! ❤️ A few more unit tests would be nice though |
I just found a much better idea: using the rustc format parser directly. :3 |
d720088
to
2356854
Compare
So it now uses the rustc format parser, making things much better overall: no more regex, less false positive, simpler code. I also added all suggested test cases and some more. |
What does the lint do for the empty JSON object |
cx, | ||
LITERAL_STRING_WITH_FORMATTING_ARG, | ||
spans, | ||
"this is a formatting argument but it is not part of a formatting macro", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a less strong wording would be good, e.g. "this looks like a formatting argument…" ? Just in case of false positives
2356854
to
c662397
Compare
It lints (as expected 😄). There was a test for that with other characters but created a new one with just
Good idea! I changed the wording. |
6a3f320
to
c027b98
Compare
Realistically speaking, I feel like
If someone writes |
Good point. Should I only lint against non-empty |
2d7c05d
to
852f1e3
Compare
I removed the check for |
☔ The latest upstream changes (presumably #13322) made this pull request unmergeable. Please resolve the merge conflicts. |
852f1e3
to
d9c9ad4
Compare
Fixed merge conflict. |
Quite a lot of hits in lintcheck, some crates may have to allow this crate-wise. |
Should I move it to another lint level maybe? |
On the other hand, the affected crates are the expected ones: clap, cargo, rg, regex, that are crates that manipulate patterns. Those can disable the lint globally. |
Then keeping it as is. Do you see other things that need to be changed @y21? |
ping @y21 :) |
Fixes #10195.
changelog: Add new
literal_string_with_formatting_arg
lint