Add new `literal_string_with_formatting_args` lint #13410

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

changelog: Added new [literal_string_with_formatting_args] pedantic lint
#13410

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-11-19T18:06:08Z

Implemented the limitation as asked. The ui test file is a good summary of what's emitting the lint or not. Please tell me if you need more restrictions.

xFrednet

Superficial NITs the rest looks good :D

xFrednet · 2024-11-22T12:35:08Z

tests/ui/literal_string_with_formatting_arg.stderr

   |
 LL |     x.expect(r##" {y:?} {y:?} "##);
   |                   ^^^^^ ^^^^^

 error: this looks like a formatting argument but it is not part of a formatting macro
-  --> tests/ui/literal_string_with_formatting_args.rs:17:18
+  --> tests/ui/literal_string_with_formatting_arg.rs:18:18
   |
 LL |     x.expect("———{:?}");


Why is this still being linted after restricting the lint to idents in the current scope?

As long as there is a valid formatter with :, then it's highly likely that it's actually something the user intended. I don't know any language where {:?} or {:#} or {:.2} would make sense (doesn't mean they don't exist) so I think it's pretty safe to keep these cases. I can remove them if preferred but I think it's not cases that could trigger issues.

That makes sense, I'm happy to keep it like this :D

xFrednet · 2024-11-22T12:37:06Z

clippy_lints/src/literal_string_with_formatting_args.rs

+                    }
+
+                    if fmt_str[start + 1..].trim_start().starts_with('}') {
+                        // For now, we ignore `{}`.


NIT: The "For now" sounds like an open TODO

Suggested change

// For now, we ignore `{}`.

We ignore `{}` and `{:?}` since they can't refer to anything

xFrednet · 2024-11-22T12:39:20Z

tests/ui/single_component_path_imports.rs

@@ -1,5 +1,5 @@
 #![warn(clippy::single_component_path_imports)]
-#![allow(unused_imports)]
+#![allow(unused_imports, clippy::literal_string_with_formatting_args)]


I believe this is no longer needed since 4 in {4} isn't a valid identifier

Same for other tests. Could you remove them to make sure those FPs are resolved?

Sure, let me go through them. 👍

GuillaumeGomez · 2024-11-22T13:55:53Z

tests/ui/uninlined_format_args_panic.rs

@@ -3,6 +3,7 @@
 //@[edition2021] edition:2021

 #![warn(clippy::uninlined_format_args)]
+#![allow(clippy::literal_string_with_formatting_args)]


This one uses panic!("p4 {var}");, so we need to allow the lint.

…_formatting_args`

…formatting argument is passed

GuillaumeGomez · 2024-11-22T14:26:50Z

Removed the allow attributes where possible. I still think that things like {:?} should be linted but if you prefer, I can remove it as well (and open a follow-up so it can be discussed if it wasn't already 😆 ).

xFrednet

This version looks good to me. I've created an FCP: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.20.60literal_string_with_formatting_args.60.20rust-clippy.2313410

GuillaumeGomez · 2024-11-23T13:38:00Z

Thanks!

xFrednet · 2024-12-01T11:46:13Z

Alright, let's get this merged.

Roses are {:#?}
Violets are {color2}
this would be horrible formatting,
lets lint this

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

Closed

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

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

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch 5 times, most recently from ffce1c1 to 076d9ca Compare November 19, 2024 17:51

xFrednet reviewed Nov 22, 2024

View reviewed changes

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch 2 times, most recently from ade93b2 to 3c30d61 Compare November 22, 2024 13:55

GuillaumeGomez commented Nov 22, 2024

View reviewed changes

GuillaumeGomez added 5 commits November 22, 2024 14:57

Update new failing tests

05705b6

Allow literal_string_with_formatting_arg in some clippy crates

8d5a4e9

Disable checks on {}

9ea769a

Don't emit if literal comes from macro expansion

b20b75a

Rename literal_string_with_formatting_arg into `literal_string_with…

607a3f6

…_formatting_args`

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch from 3c30d61 to 0930c94 Compare November 22, 2024 13:57

Limit literal_string_with_formatting_args to known variables if no …

cfc6444

…formatting argument is passed

GuillaumeGomez force-pushed the literal_string_with_formatting_arg branch from 0930c94 to cfc6444 Compare November 22, 2024 13:59

xFrednet changed the title ~~Add new literal_string_with_formatting_arg lint~~ Add new literal_string_with_formatting_args lint Nov 23, 2024

xFrednet approved these changes Nov 23, 2024

View reviewed changes

xFrednet added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 23, 2024

xFrednet removed the S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) label Dec 1, 2024

xFrednet added this pull request to the merge queue Dec 1, 2024

Merged via the queue into rust-lang:master with commit 1f966e9 Dec 1, 2024
9 checks passed

GuillaumeGomez deleted the literal_string_with_formatting_arg branch December 1, 2024 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `literal_string_with_formatting_args` lint #13410

Add new `literal_string_with_formatting_args` lint #13410

GuillaumeGomez commented Sep 17, 2024 •

edited

Loading

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 Nov 19, 2024

xFrednet left a comment

xFrednet Nov 22, 2024

GuillaumeGomez Nov 22, 2024

xFrednet Nov 23, 2024

GuillaumeGomez Nov 23, 2024

xFrednet Nov 22, 2024

GuillaumeGomez Nov 22, 2024

xFrednet Nov 22, 2024

GuillaumeGomez Nov 22, 2024

GuillaumeGomez Nov 22, 2024

GuillaumeGomez commented Nov 22, 2024

xFrednet left a comment

GuillaumeGomez commented Nov 23, 2024

xFrednet commented Dec 1, 2024

	// For now, we ignore `{}`.
	We ignore `{}` and `{:?}` since they can't refer to anything

Add new literal_string_with_formatting_args lint #13410

Add new literal_string_with_formatting_args lint #13410

Conversation

GuillaumeGomez commented Sep 17, 2024 • edited Loading

rustbot commented 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

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 Nov 19, 2024

xFrednet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 22, 2024

xFrednet left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 23, 2024

xFrednet commented Dec 1, 2024

Add new `literal_string_with_formatting_args` lint #13410

Add new `literal_string_with_formatting_args` lint #13410

GuillaumeGomez commented Sep 17, 2024 •

edited

Loading