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.

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

Open

literal_string_with_formatting_args false positive on test #13885

Open

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

Open

profetia mentioned this pull request Jan 17, 2025

A Compiler bug #14007

Open

samueltardieu mentioned this pull request Jan 17, 2025

change literal_string_with_formatting_args lint category to pedantic #14014

Open

Fix regression rust-lang#14007

277bf08

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

Fix literal_string_with_formatting_args lint emitted when it should not #13953

Are you sure you want to change the base?

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

samueltardieu Jan 7, 2025

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

y21 Jan 14, 2025

Choose a reason for hiding this comment

y21 Jan 14, 2025

Choose a reason for hiding this comment

GuillaumeGomez Jan 15, 2025

Choose a reason for hiding this comment

y21 Jan 14, 2025

Choose a reason for hiding this comment

y21 Jan 14, 2025

Choose a reason for hiding this comment

GuillaumeGomez Jan 15, 2025

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 15, 2025

samueltardieu commented Jan 16, 2025

GuillaumeGomez commented Jan 18, 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