WIP format literal arg inlining #10740

nyurik · 2023-05-04T05:13:45Z

A rough draft of #10739 implementation. The goal is to allow any kind of literal string arguments to be inlined automatically.

format!("hello {}", "world")
// is replaced with
format!("hello world")

changelog: [`uninlined_format_args`]: support for inlining string literals

rustbot · 2023-05-04T05:13:50Z

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

llogiq

Looks good to me. I have but one request for clarification and I'd like to see a test with macros.

llogiq · 2023-05-04T20:52:19Z

clippy_lints/src/format_args.rs

+    } else if !matches!(arg.kind, FormatArgumentKind::Captured(_))
+        && let Some(FormatPlaceholder{span: Some(placeholder_span), ..}) = placeholder
+        && let rustc_ast::ExprKind::Lit(lit) = &arg.expr.kind
+        && lit.kind == LitKind::Str // FIXME: lit.kind must match the format string kind


This isn't clear to me. What other kinds would there be?

there are also raw strings

And possibly byte strings too? Not certain

llogiq · 2023-05-04T20:56:42Z

tests/ui/uninlined_format_args.fixed

@@ -262,3 +262,17 @@ fn tester2() {
        local_i32,
    };
 }
+
+fn literals() {


I worry about cases where the format string and literal value come from different macro expansions. We should have a test where that is the case.

Yeah, i will need tons more of tests for this - some already exist in

https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/write_literal.rs

https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/write_literal_2.rs

https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/write_with_newline.rs

Could you give an example of what specific macro expansion where you thinking about?

Alexendoo · 2023-05-04T23:18:55Z

I'd say this should be under a new lint name, although it's similar what it's doing is different enough to the existing uninlined_format_args

The existing implementation can also be used, it handles the various raw/non-raw permutations of the literal and format string as well as some other literals. You'd have to change the part where it checks the macro names before it's called + do a lint rename for both print_literal and write_literal to the new one

nyurik · 2023-05-04T23:56:25Z

@Alexendoo after some more thinking, it seems separating it into a new lint would create more problems than solve:

multiple lints operating on the same expression tend to cause issues, esp in the --fix mode
a format expression with numeric indexes would be ignored by both lints unless used together: format!("{0} {1}", var, "str") will be ignored by both the uninlined_format_args and the string inlining lint, but when used together it would suggest the format!("{var} str")
the original lint name uninlined_format_args fits perfectly for the string inlining as well, so wouldn't cause any confusion

So in short, we would not really gain anything besides problems from multiple lints... Not worth it IMO :)

Alexendoo · 2023-05-05T11:42:51Z

I don't see what problem that would cause here, overlapping suggestions would cause rustfix ui tests to fail (cargo fix can handle them though) but the suggestions here wouldn't have overlapping spans

The main thing is so that you can set the lint levels independently, e.g. print_literal and write_literal are style but uninlined_format_args is pedantic. It wouldn't be unusual to want to disable one independently if they were in the same group

It would also make the lint documentation more confusing to have to explain the two different kinds of inlining in one lint, and how allow-mixed-uninlined-format-args would only apply to one of those kinds

format!("{0} {1}", var, "str") doesn't seem likely enough to occur to make up for that

nyurik · 2023-05-05T18:30:36Z

@Alexendoo you do raise important config point. I think we can actually achieve both goals if we track the type of inlining needed -- e.g. if both are enabled, and both are needed (the "literal", var case above), we can show just one suggestion (under either of the names, or even add a note saying that both were involved). If only one is enabled, it won't even consider the other in the inlining.

Alexendoo · 2023-05-07T16:51:19Z

To me that sounds too complicated/novel considering the problem is relatively minor

llogiq · 2023-05-07T22:21:05Z

Agreed, let's not complicate the solution. As long as both lints don't step on each other's toes by issuing incompatible suggestions, having two lints that don't care about each other is fine.

bors · 2023-06-12T18:32:41Z

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

A rough draft of rust-lang#10739 implementation. The goal is to allow any kind of literal string arguments to be inlined automatically. ```rust format!("hello {}", "world") // is replaced with format!("hello world") ```

bors · 2024-02-19T09:29:41Z

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

xFrednet · 2024-03-29T11:54:39Z

Hey @nyurik, this is a ping from triage. Do you still plan to continue this PR? If you need help, you're always welcome to ask questions here or on Zulip :)

xFrednet · 2024-06-20T20:09:43Z

Hey this is triage, I'm closing this due to inactivity. If you want to continue this implementation, you're welcome to create a new PR. Thank you for the time, you already put into this!

Interested parties are welcome to pick this implementation up as well :)

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review

rustbot assigned llogiq May 4, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 4, 2023

llogiq reviewed May 4, 2023

View reviewed changes

nyurik force-pushed the inline-literal branch from 86cfa0b to de80373 Compare February 12, 2024 23:30

WIP format literal arg inlining

e50f944

A rough draft of rust-lang#10739 implementation. The goal is to allow any kind of literal string arguments to be inlined automatically. ```rust format!("hello {}", "world") // is replaced with format!("hello world") ```

nyurik force-pushed the inline-literal branch from de80373 to e50f944 Compare February 13, 2024 00:02

xFrednet closed this Jun 20, 2024

rustbot added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP format literal arg inlining #10740

WIP format literal arg inlining #10740

nyurik commented May 4, 2023 •

edited

Loading

rustbot commented May 4, 2023

llogiq left a comment

llogiq May 4, 2023

nyurik May 4, 2023

nyurik May 4, 2023

llogiq May 4, 2023

nyurik May 4, 2023

Alexendoo commented May 4, 2023

nyurik commented May 4, 2023

Alexendoo commented May 5, 2023

nyurik commented May 5, 2023 •

edited

Loading

Alexendoo commented May 7, 2023

llogiq commented May 7, 2023

bors commented Jun 12, 2023

bors commented Feb 19, 2024

xFrednet commented Mar 29, 2024

xFrednet commented Jun 20, 2024

WIP format literal arg inlining #10740

WIP format literal arg inlining #10740

Conversation

nyurik commented May 4, 2023 • edited Loading

rustbot commented May 4, 2023

llogiq left a comment

Choose a reason for hiding this comment

llogiq May 4, 2023

Choose a reason for hiding this comment

nyurik May 4, 2023

Choose a reason for hiding this comment

nyurik May 4, 2023

Choose a reason for hiding this comment

llogiq May 4, 2023

Choose a reason for hiding this comment

nyurik May 4, 2023

Choose a reason for hiding this comment

Alexendoo commented May 4, 2023

nyurik commented May 4, 2023

Alexendoo commented May 5, 2023

nyurik commented May 5, 2023 • edited Loading

Alexendoo commented May 7, 2023

llogiq commented May 7, 2023

bors commented Jun 12, 2023

bors commented Feb 19, 2024

xFrednet commented Mar 29, 2024

xFrednet commented Jun 20, 2024

nyurik commented May 4, 2023 •

edited

Loading

nyurik commented May 5, 2023 •

edited

Loading