Add new `literal_string_with_formatting_arg` lint #13410

GuillaumeGomez · 2024-09-17T22:11:13Z

changelog: Add new literal_string_with_formatting_arg lint

rustbot · 2024-09-17T22:11:18Z

rustbot has assigned @Jarcho.
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

clippy_lints/src/literal_string_with_formatting_arg.rs

GuillaumeGomez · 2024-09-17T22:13:12Z

clippy_lints/src/literal_string_with_formatting_arg.rs

+                })
+                .collect::<Vec<_>>();
+            if spans.len() == 1 {
+                span_lint(


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.

matthiaskrgr · 2024-09-17T22:24:14Z

Would like to see a test for malformed fmt attemps such as {y:?}} or "{y:?} y:?}" to make sure we don't lint there by accident

tests/ui/literal_string_with_formatting_arg.rs

emilk · 2024-09-18T07:40:43Z

I did a little bit of testing, and it seems to work like it should! ❤️ A few more unit tests would be nice though

GuillaumeGomez · 2024-09-18T16:13:19Z

I just found a much better idea: using the rustc format parser directly. :3

GuillaumeGomez · 2024-09-18T22:00:48Z

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.

y21 · 2024-09-18T22:28:09Z

What does the lint do for the empty JSON object {} which is also a valid formatting specifier? Would probably be a good to have as a negative test too.

emilk · 2024-09-19T07:24:10Z

clippy_lints/src/literal_string_with_formatting_arg.rs

+                    cx,
+                    LITERAL_STRING_WITH_FORMATTING_ARG,
+                    spans,
+                    "this is a formatting argument but it is not part of a formatting macro",


Maybe a less strong wording would be good, e.g. "this looks like a formatting argument…" ? Just in case of false positives

GuillaumeGomez · 2024-09-19T10:14:36Z

What does the lint do for the empty JSON object {} which is also a valid formatting specifier? Would probably be a good to have as a negative test too.

It lints (as expected 😄). There was a test for that with other characters but created a new one with just {} just in case.

Maybe a less strong wording would be good, e.g. "this looks like a formatting argument…" ? Just in case of false positives

Good idea! I changed the wording.

y21 · 2024-09-19T13:57:32Z

It lints (as expected 😄).

Realistically speaking, I feel like {} would run into more false positives than true positives if we decide to lint that.

{foo}/{foo:?} etc. all make sense since they can work with format! alone (that is, format!("{foo}") works), but you can't make that argument for just {}. Not only is it missing the format macro, but it's also missing a formatting argument.

If someone writes let foo = "{}";, it seems fairly unlikely to me that they actually meant to use a formatting macro there, because there is nothing to format.

GuillaumeGomez · 2024-09-19T14:01:53Z

Good point. Should I only lint against non-empty {} like {y}/{:?}/{y:?} and eventually we can add an option to look for everything?

GuillaumeGomez · 2024-09-19T20:53:29Z

I removed the check for {} for the time being.

bors · 2024-09-22T14:37:06Z

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

GuillaumeGomez · 2024-09-22T15:10:53Z

Fixed merge conflict.

samueltardieu · 2024-09-24T18:26:57Z

Quite a lot of hits in lintcheck, some crates may have to allow this crate-wise.

GuillaumeGomez · 2024-09-24T20:53:39Z

Should I move it to another lint level maybe?

samueltardieu · 2024-09-24T21:07:06Z

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.

GuillaumeGomez · 2024-09-25T09:32:05Z

Then keeping it as is.

Do you see other things that need to be changed @y21?

GuillaumeGomez · 2024-10-08T12:28:09Z

ping @y21 :)

GuillaumeGomez · 2024-10-29T13:34:24Z

r? @xFrednet

Jarcho · 2024-10-29T13:48:16Z

Since this is currently in suspicious the goal should be to have no false-positives (they're acceptable, but should be rare). With that in mind This should only lint strings which would compile when used with format in the first place. Something like:

A valid format string (no parse errors).
Only inline arguments (or at least one).
All named arguments name real locals/globals (or a small edit distance away from one).
All arguments actually implement the required traits.

The goal here is a suspicious lint shouldn't be annoying. If all it does is end up linting on things which aren't intended as format strings it gets turned off.

GuillaumeGomez · 2024-10-29T14:21:02Z

All named arguments name real locals/globals (or a small edit distance away from one).

All arguments actually implement the required traits.

Sounds acceptable. Going to try to implement these extra requirements.

bors · 2024-10-31T10:14:00Z

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

xFrednet

Implementation generally looks good to me, but I haven't worked with the Parser before.

Mostly superficial comments and questions.

xFrednet · 2024-11-01T16:41:33Z

tests/ui/print_literal.rs

@@ -1,5 +1,5 @@
 #![warn(clippy::print_literal)]
-#![allow(clippy::uninlined_format_args)]
+#![allow(clippy::uninlined_format_args, clippy::literal_string_with_formatting_arg)]


Why does it have to be allowed in these files? Does it also lint strings in print! macros? 🤔

xFrednet · 2024-11-01T16:43:15Z

clippy_lints/src/literal_string_with_formatting_arg.rs

+    /// x.expect(&format!("{y:?}"));
+    /// ```
+    #[clippy::version = "1.83.0"]
+    pub LITERAL_STRING_WITH_FORMATTING_ARG,


I guess this should be plural, according to lint naming guidelines

Suggested change

pub LITERAL_STRING_WITH_FORMATTING_ARG,

pub LITERAL_STRINGS_WITH_FORMATTING_ARG,

xFrednet · 2024-11-01T16:45:37Z

clippy_lints/src/literal_string_with_formatting_arg.rs

+    /// ```
+    #[clippy::version = "1.83.0"]
+    pub LITERAL_STRING_WITH_FORMATTING_ARG,
+    suspicious,


Based on the number of needed allows this feels like the wrong category. Lintcheck also found 76 occurrences, so I don't think this should be warn by default. It feels more like a restriction lint or maybe pedantic but I'd be in favor of restriction

xFrednet · 2024-11-01T16:50:54Z

tests/ui/literal_string_with_formatting_arg.rs

+    x.expect(r"{y:?} y:?}"); //~ literal_string_with_formatting_arg
+    x.expect(r##" {y:?} {y:?} "##); //~ literal_string_with_formatting_arg
+    // Ensure that it doesn't try to go in the middle of a unicode character.
+    x.expect("———{:?}"); //~ literal_string_with_formatting_arg


I feel like this shouldn't be linted, for the simple reason, that the {:?} can't refer to anything. If someone mixed expect() up to think that it allows formatting, they would probably write expect("{:?}", obj) which would result in an error, since expect only takes one argument.

One option to make this line more accepting would be to restrict linting to {x:?} / {x} where x is available in the current scope 🤔

rustbot assigned Jarcho Sep 17, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 17, 2024

GuillaumeGomez changed the title ~~Literal string with formatting arg~~ Add new literal_string_with_formatting_arg lint Sep 17, 2024

GuillaumeGomez changed the title ~~Add new literal_string_with_formatting_arg lint~~ Add new literal_string_with_formatting_arg lint Sep 17, 2024

GuillaumeGomez commented Sep 17, 2024

View reviewed changes

clippy_lints/src/literal_string_with_formatting_arg.rs Outdated Show resolved Hide resolved

GuillaumeGomez commented Sep 17, 2024

View reviewed changes

GuillaumeGomez mentioned this pull request Sep 17, 2024

warn on strings that look like inline format strings but aren't #10195

Open

emilk reviewed Sep 18, 2024

View reviewed changes

tests/ui/literal_string_with_formatting_arg.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch 3 times, most recently from d720088 to 2356854 Compare September 18, 2024 21:59

emilk reviewed Sep 19, 2024

View reviewed changes

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch from 2356854 to c662397 Compare September 19, 2024 10:11

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch 2 times, most recently from 6a3f320 to c027b98 Compare September 19, 2024 13:55

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch 3 times, most recently from 2d7c05d to 852f1e3 Compare September 19, 2024 16:05

Add new literal_string_with_formatting_arg lint

c8e1086

GuillaumeGomez added 5 commits September 22, 2024 17:10

Add regression test for literal_string_with_formatting_arg

15a4074

Update new failing tests

70b7bf6

Allow literal_string_with_formatting_arg in some clippy crates

1dacc32

Disable checks on {}

cf40648

Don't emit if literal comes from macro expansion

d9c9ad4

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch from 852f1e3 to d9c9ad4 Compare September 22, 2024 15:10

GuillaumeGomez requested review from Jarcho and y21 October 28, 2024 12:52

rustbot assigned xFrednet and unassigned Jarcho Oct 29, 2024

xFrednet reviewed Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `literal_string_with_formatting_arg` lint #13410

Add new `literal_string_with_formatting_arg` lint #13410

GuillaumeGomez commented Sep 17, 2024

rustbot commented Sep 17, 2024

GuillaumeGomez Sep 17, 2024

matthiaskrgr commented Sep 17, 2024

emilk commented Sep 18, 2024

GuillaumeGomez commented Sep 18, 2024

GuillaumeGomez commented Sep 18, 2024

y21 commented Sep 18, 2024

emilk Sep 19, 2024

GuillaumeGomez commented Sep 19, 2024

y21 commented Sep 19, 2024

GuillaumeGomez commented Sep 19, 2024

GuillaumeGomez commented Sep 19, 2024

bors commented Sep 22, 2024

GuillaumeGomez commented Sep 22, 2024

samueltardieu commented Sep 24, 2024

GuillaumeGomez commented Sep 24, 2024

samueltardieu commented Sep 24, 2024

GuillaumeGomez commented Sep 25, 2024

GuillaumeGomez commented Oct 8, 2024

GuillaumeGomez commented Oct 29, 2024

Jarcho commented Oct 29, 2024

GuillaumeGomez commented Oct 29, 2024

bors commented Oct 31, 2024

xFrednet left a comment

xFrednet Nov 1, 2024

xFrednet Nov 1, 2024

xFrednet Nov 1, 2024

xFrednet Nov 1, 2024

	pub LITERAL_STRING_WITH_FORMATTING_ARG,
	pub LITERAL_STRINGS_WITH_FORMATTING_ARG,

Add new literal_string_with_formatting_arg lint #13410

Are you sure you want to change the base?

Add new literal_string_with_formatting_arg lint #13410

Conversation

GuillaumeGomez commented Sep 17, 2024

rustbot commented Sep 17, 2024

GuillaumeGomez Sep 17, 2024

Choose a reason for hiding this comment

matthiaskrgr commented Sep 17, 2024

emilk commented Sep 18, 2024

GuillaumeGomez commented Sep 18, 2024

GuillaumeGomez commented Sep 18, 2024

y21 commented Sep 18, 2024

emilk Sep 19, 2024

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 19, 2024

y21 commented Sep 19, 2024

GuillaumeGomez commented Sep 19, 2024

GuillaumeGomez commented Sep 19, 2024

bors commented Sep 22, 2024

GuillaumeGomez commented Sep 22, 2024

samueltardieu commented Sep 24, 2024

GuillaumeGomez commented Sep 24, 2024

samueltardieu commented Sep 24, 2024

GuillaumeGomez commented Sep 25, 2024

GuillaumeGomez commented Oct 8, 2024

GuillaumeGomez commented Oct 29, 2024

Jarcho commented Oct 29, 2024

GuillaumeGomez commented Oct 29, 2024

bors commented Oct 31, 2024

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Nov 1, 2024

Choose a reason for hiding this comment

xFrednet Nov 1, 2024

Choose a reason for hiding this comment

xFrednet Nov 1, 2024

Choose a reason for hiding this comment

xFrednet Nov 1, 2024

Choose a reason for hiding this comment

Add new `literal_string_with_formatting_arg` lint #13410

Add new `literal_string_with_formatting_arg` lint #13410