Fix `literal_string_with_formatting_args` lint emitted when it should not #13953

GuillaumeGomez · 2025-01-06T18:07:02Z

Problem was that I forgot to check whether or not the span was a "real" one. Because if not, then it starts pointing to pretty much only wrong content, hence the problems we saw with clippy linting on clippy.toml.

changelog: Fix literal_string_with_formatting_args lint emitted when it should not

r? @samueltardieu

samueltardieu

LGTM, with the addition of a non-formatting true positive macro test case.

You should reroll the dice as I don't have the privilege to approve this patch.

r?

samueltardieu · 2025-01-07T00:22:27Z

tests/ui/literal_string_with_formatting_arg.rs

+    );
+    let value = 0;
+    assert!(format!("{value}").is_ascii());
+}


Could you also add another true positive test such as

Suggested change

}

assert!("{value}".is_ascii());

}

so that we can check that the lint properly triggers in non-formatting-macro arguments?

rustbot · 2025-01-07T00:23:27Z

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

rustbot · 2025-01-07T00:23:28Z

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

samueltardieu · 2025-01-07T00:23:57Z

r? clippy

profetia · 2025-01-07T06:48:59Z

Can you check if this case works? I think just checking dummy is not enough, that is why I switched to checking the enclosing exprs in #13898

samueltardieu · 2025-01-07T07:48:17Z

Can you check if this case works? I think just checking dummy is not enough, that is why I switched to checking the enclosing exprs in #13898

It still triggers for me indeed:

warning: this looks like a formatting argument but it is not part of a formatting macro
 --> {value}.rs:7:6
  |
7 |     dbg!("something");
  |      ^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#literal_string_with_formatting_args
  = note: `#[warn(clippy::literal_string_with_formatting_args)]` on by default

… not

…args` lint

GuillaumeGomez · 2025-01-07T14:56:18Z

Indeed. Sadly, @profetia, your fix doesn't lint with:

assert!("{y}".is_ascii());

So I have a not so great middle-ground solution but I think it'll do the trick for the time being: I simply check if the string is part of the snippet in the user code. If not, then I skip the lint. What do you think?

y21 · 2025-01-10T17:40:37Z

Nominating for beta backport because the lint is on beta now and the false positive leads to pretty confusing warnings.

@rustbot label +beta-nominated

GuillaumeGomez · 2025-01-10T23:17:55Z

Changelog CI also passed. :)

GuillaumeGomez · 2025-01-14T14:54:27Z

r? @y21

y21 · 2025-01-14T16:57:13Z

tests/ui/{literal_string_with_formatting_args}.rs

+
+fn another_bad() {
+    let literal_string_with_formatting_args = 0;
+    dbg!("something");


I'm surprised that the expr.span.from_expansion() check doesn't already prevent this. Looking at -Zunpretty=hir-tree output it does seem like the span of the string with the file name is properly marked as being from the non-root context

Lit( Spanned { node: Str( "[/app/example.rs:2:5] \"something\" = ", Cooked, ), span: /rustc/eb54a50837ad4bcc9842924f27e7287ca66e294c/library/std/src/macros.rs:365:35: 365:58 (#4), }, ),

Odd

Ah, I see what's happening. Clippy sets -Zflatten_format_args=no, which changes the default behavior of how format args are lowered and only that one has this non-macro span. Adding the flag makes it repro on godbolt and shows a root context span for the file name string.

Lit( Spanned { node: Str( "/app/example.rs", Cooked, ), span: /app/example.rs:2:5: 2:22 (#0), }, )

Maybe add to the comment that clippy sets this flag and only with that flag does it have a non-macro span which gets around the from_expansion check, in case anyone wants to look into this in the future and doesn't have to go down this rabbit hole?

I looked into it but couldn't figure out why it didn't work. Thanks a lot for the detailed explanations! Adding it as a comment.

y21 · 2025-01-14T16:58:52Z

clippy_lints/src/literal_string_with_formatting_args.rs

+            let Some(snippet) = snippet_opt(cx, expr.span) else {
+                return;
+            };
            let fmt_str = symbol.as_str();
+            // If the literal has been generated by the macro, the snippet should not contain it,
+            // allowing us to skip it.
+            if !snippet.contains(fmt_str) {
+                return;
+            }


This will add false negatives for escaped strings (eg "{foo} \n bar") and similar, but as a temporary solution to fix FPs it's probably fine

It might be worth trying to use is_from_proc_macro here instead, because that also solves the same issue of a node not matching up with its snippet. It seems like it should also fix the FP while not running into this FN since it doesn't actually check the string contents.

(If we do go with the current approach, a small nit: it might be a good use for the newer SpanRange API with if !expr.span.check_source_text(|snippet| snippet.contains(fmt_str)) { return; }. Slightly simpler and avoids allocating a temporary string)

Not sure why is_from_proc_macro didn't work last time I tried but this time it behaves as expected, so switching back to it.

…the literal

GuillaumeGomez · 2025-01-15T18:56:36Z

@rustbot ready

samueltardieu · 2025-01-16T15:19:07Z

As it is currently, this does not fix #14007.

GuillaumeGomez · 2025-01-18T16:14:45Z

Fixed #14007 as well.

Alexendoo · 2025-01-19T12:01:54Z

Removing the beta nomination in favour of #14014 since backporting would no longer be urgent

y21 · 2025-01-23T03:14:01Z

clippy_lints/src/literal_string_with_formatting_args.rs

+                    let mut start = pos.start;
+                    while start < fmt_str.len() && !fmt_str.is_char_boundary(start) {
+                        start += 1;
+                    }
+                    let start = fmt_str[..start].rfind('{').unwrap_or(start);


Is this the same as fmt_str[..fmt_str.ceil_char_boundary(pos.start)].rfind('{')?

For what it's worth, it looks like the format parser offsets all spans by 1 (presumably because it's relative to the " quote), so that's what causes the index to be in the middle of a character sometimes. Just doing pos.start - 1 instead of pos.start on the old deleted line (to undo the parser's offset) should fix this too as far as I can tell, but this over-conservative approach should be fine too I guess

I'd prefer to keep it over-conservative if you don't mind. 😉

y21 · 2025-01-23T03:22:25Z

I've been reading through the surrounding code to get a better understanding of this lint and (somewhat tangentially related to the ICE) I'm wondering why we continue parsing after encountering parser errors? It seems to be adding a fair amount of complexity (remapping the spans, reconstructing parsers) while at the same time I'm not sure if it really makes sense to lint strings that failed to parse in the first place. It feels unlikely that the user meant to write a format string if it can't parse as one anyway? Maybe there is a good reason for this though and I don't know about it!

GuillaumeGomez · 2025-01-28T10:43:19Z

We need to remap the spans at every { we encounter, whether or not it's valid. For example if you have "{} bla {}", we still need to remap the spans after the first {}.

As for why continuing after an error, it's mainly because you can do typos and if this is the case, still expect formatting afterwards (like in panic) and not realizing that:

There is no formatting.
There is a typo.

Does my reasoning makes sense to you?

GuillaumeGomez · 2025-02-11T14:04:38Z

Is there anything else to be done here?

larseggert · 2025-02-22T18:11:42Z

Should this be fixed with 1.85.0? I still see this:

cargo clippy --all-targets --no-default-features -- -D warnings
...
error: this looks like a formatting argument but it is not part of a formatting macro
 --> /Users/lars/Documents/Code/neqo/.clippy.toml:2:68
  |
2 |     { path = "std::slice::from_raw_parts", reason = "see null_safe_slice" }
  |                                                                    ^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#literal_string_with_formatting_args
  = note: `-D clippy::literal-string-with-formatting-args` implied by `-D warnings`
  = help: to override `-D warnings` add `#[allow(clippy::literal_string_with_formatting_args)]`

error: could not compile `neqo-transport` (lib test) due to 1 previous error

with https://github.com/mozilla/neqo. (Example: https://github.com/mozilla/neqo/actions/runs/13468547315/job/37638898254?pr=2444#step:6:512)

y21 · 2025-02-22T18:19:58Z

This fix will be in 1.87.0. But there was another PR that downgraded this lint's category to nursery (#14014) which was backported and should be on stable, so you shouldn't really see this unless you explicitly enabled the lint.

Do you have something like #![warn(clippy::literal_string_with_formatting_args)] or #![warn(clippy::nursery)] in your crate?

larseggert · 2025-02-22T18:37:02Z

Yes, we have enabled nursery. Thanks for the pointer, will work around it!

rustbot assigned samueltardieu Jan 6, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 6, 2025

This was referenced Jan 6, 2025

fix literal_string_with_formatting_args FP on format inside assert #13898

Closed

literal_string_with_formatting_args false positive on test #13885

Closed

samueltardieu approved these changes Jan 7, 2025

View reviewed changes

rustbot assigned xFrednet and unassigned samueltardieu Jan 7, 2025

GuillaumeGomez added 2 commits January 7, 2025 15:35

Fix literal_string_with_formatting_args lint emitted when it should…

17f9344

… not

Correctly handle expanded macros for `literal_string_with_formatting_…

a7fb37c

…args` lint

GuillaumeGomez force-pushed the fix-literal_string_with_formatting_args branch from 9515896 to a7fb37c Compare January 7, 2025 14:55

GuillaumeGomez mentioned this pull request Jan 10, 2025

literal_string_with_formatting_args is trigger by Actix Web's wildcard route definitions #13928

Open

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 10, 2025

Icxolu mentioned this pull request Jan 11, 2025

release: 0.23.4 PyO3/pyo3#4835

Merged

rustbot assigned y21 and unassigned xFrednet Jan 14, 2025

y21 reviewed Jan 14, 2025

View reviewed changes

rbradford mentioned this pull request Jan 14, 2025

virtio-devices: iommu: allow limiting maximum address width in bits cloud-hypervisor/cloud-hypervisor#6900

Merged

GuillaumeGomez added 2 commits January 15, 2025 16:42

Add more detailed explanations for rust-lang#13885 regression test

00104a4

Use is_from_proc_macro instead of checking if the snippet contains …

e35330c

…the literal

Xuanwo mentioned this pull request Jan 17, 2025

False positive literal_string_with_formatting_args for non-static templete string expansion with replace. #13989

Closed

profetia mentioned this pull request Jan 17, 2025

A Compiler bug #14007

Closed

samueltardieu mentioned this pull request Jan 17, 2025

change literal_string_with_formatting_args lint category to nursery #14014

Merged

Fix regression rust-lang#14007

277bf08

Alexendoo removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 19, 2025

y21 reviewed Jan 23, 2025

View reviewed changes

joelwurtz mentioned this pull request Jan 29, 2025

chore(clippy): fix clippy errors HFQR/xitca-web#1188

Merged

y21 approved these changes Feb 13, 2025

View reviewed changes

y21 added this pull request to the merge queue Feb 13, 2025

Merged via the queue into rust-lang:master with commit b83762c Feb 13, 2025
11 of 13 checks passed

GuillaumeGomez deleted the fix-literal_string_with_formatting_args branch February 13, 2025 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `literal_string_with_formatting_args` lint emitted when it should not #13953

Fix `literal_string_with_formatting_args` lint emitted when it should not #13953

GuillaumeGomez commented Jan 6, 2025 •

edited

Loading

samueltardieu left a comment

samueltardieu Jan 7, 2025

rustbot commented Jan 7, 2025

rustbot commented Jan 7, 2025

samueltardieu commented Jan 7, 2025

profetia commented Jan 7, 2025

samueltardieu commented Jan 7, 2025

GuillaumeGomez commented Jan 7, 2025 •

edited

Loading

y21 commented Jan 10, 2025

GuillaumeGomez commented Jan 10, 2025

GuillaumeGomez commented Jan 14, 2025

y21 Jan 14, 2025

y21 Jan 14, 2025

GuillaumeGomez Jan 15, 2025

y21 Jan 14, 2025

y21 Jan 14, 2025

GuillaumeGomez Jan 15, 2025

GuillaumeGomez commented Jan 15, 2025

samueltardieu commented Jan 16, 2025

GuillaumeGomez commented Jan 18, 2025

Alexendoo commented Jan 19, 2025

y21 Jan 23, 2025

y21 Jan 23, 2025

GuillaumeGomez Jan 28, 2025

y21 commented Jan 23, 2025

GuillaumeGomez commented Jan 28, 2025

GuillaumeGomez commented Feb 11, 2025

larseggert commented Feb 22, 2025

y21 commented Feb 22, 2025

larseggert commented Feb 22, 2025

Fix literal_string_with_formatting_args lint emitted when it should not #13953

Fix literal_string_with_formatting_args lint emitted when it should not #13953

Conversation

GuillaumeGomez commented Jan 6, 2025 • edited Loading

samueltardieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Jan 7, 2025

rustbot commented Jan 7, 2025

samueltardieu commented Jan 7, 2025

profetia commented Jan 7, 2025

samueltardieu commented Jan 7, 2025

GuillaumeGomez commented Jan 7, 2025 • edited Loading

y21 commented Jan 10, 2025

GuillaumeGomez commented Jan 10, 2025

GuillaumeGomez commented Jan 14, 2025

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 Jan 15, 2025

samueltardieu commented Jan 16, 2025

GuillaumeGomez commented Jan 18, 2025

Alexendoo commented Jan 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 commented Jan 23, 2025

GuillaumeGomez commented Jan 28, 2025

GuillaumeGomez commented Feb 11, 2025

larseggert commented Feb 22, 2025

y21 commented Feb 22, 2025

larseggert commented Feb 22, 2025

Fix `literal_string_with_formatting_args` lint emitted when it should not #13953

Fix `literal_string_with_formatting_args` lint emitted when it should not #13953

GuillaumeGomez commented Jan 6, 2025 •

edited

Loading

GuillaumeGomez commented Jan 7, 2025 •

edited

Loading