Add new `useless_concat` lint #13829

GuillaumeGomez · 2024-12-14T22:42:39Z

Interestingly enough, to actually check that the macro call has at least two arguments, we need to use the rust lexer after getting the original source code snippet.

changelog: Add new useless_concat lint

rustbot · 2024-12-14T22:42:44Z

r? @dswij

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

GuillaumeGomez · 2024-12-15T21:10:52Z

Finally was able to only trigger the lint when appropriate.

GuillaumeGomez · 2024-12-25T20:53:03Z

r? clippy

clippy_lints/src/useless_concat.rs

GuillaumeGomez · 2024-12-29T20:35:14Z

Fixed dogfood. :)

@y21

I discovered that there were paths declared in `clippy_utils::paths` in #13829 so moving a few remaining hardcoded ones in one place. changelog: Move more def paths into `clippy_utils::paths` r? @y21

GuillaumeGomez · 2025-02-11T14:39:22Z

Fixed merge conflicts. If there is nothing else, should we start the FCP?

y21

I have a few more comments, but I'm going to open the FCP already because they shouldn't require major changes

tests/ui/useless_concat.rs

y21 · 2025-02-12T20:18:17Z

clippy_lints/src/useless_concat.rs

+            let literal = match literal {
+                Some(lit) => {
+                    // Literals can also be number, so we need to check this case too.
+                    if lit.starts_with('"') {


This also needs to handle r"" strings and character literals

You cannot have '', no?

y21 · 2025-02-12T20:57:12Z

clippy_lints/src/utils/internal_lints/invalid_paths.rs

+    // `builtin` macros don't seem to be found in `def_path_res`...
+    if path == ["core", "macros", "builtin", "concat"] {
+        return true;
+    }


I'm guessing this probably happens because it's a macro_rules! macro and gets resolved at core::concat by def_path_res. Anyway, usually we put #[expect(clippy::invalid_paths)] on the const in paths.rs instead of hardcoding it in the source here though. Can you do that instead?

y21 · 2025-02-12T21:06:20Z

FCP thread: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.3A.20useless_concat

GuillaumeGomez · 2025-05-02T15:34:07Z

Sorry for the (way too long) delay. Finally applied suggestions. Thanks a lot for them!

GuillaumeGomez · 2025-05-02T15:50:41Z

And fixed CI failure. :)

samueltardieu

That won't work properly with suffixed numeric literals: concat!(1f32) should be replaced by "1" as the builtin macro ignores the suffix, not by "1f32". However, the whole part before the suffix must be kept: concat!(1.000f32) must expand into "1.000".

Also, concat!('c') should be replaced by "c", not "'c'".

It also looks like concat!(true) and concat!(false) are not handled while they could.

Reference in the compiler

GuillaumeGomez · 2025-05-11T21:24:29Z

Thanks! Updated and added tests for each case.

samueltardieu · 2025-05-12T07:03:47Z

You'll have to rebase when #14784 is merged.

GuillaumeGomez · 2025-05-12T12:15:10Z

Rebased!

samueltardieu

This will fail in at least those two situations:

concat!('"') will suggest """ (escaping is missing)
concat!(1_f32) will suggest 1_ (underscore should be removed)

GuillaumeGomez · 2025-05-12T12:49:47Z

Good catch, thanks! Handled these cases as well.

samueltardieu

Yet another round of non-conforming substitution:

concat!(1__f32) suggests "1_" instead of the expected "1" (yes, you can use as many underscores as you like as part of the input)
concat!(0xf_f32) suggests "0xf" instead of "65530"
concat!(1_2) suggests "1_2" instead of "12"
concat!(1___2.00__100___f32) suggests "1___2.00__100__" instead of "12.00100"
As we're at it, concat!('\'') could suggest "'" instead of "'" (no need for escaping anymore)

Isn't there a way to invoke the compiler macro programmatically on the token stream once you've determined that there is only one argument? That would cover all those cases, and maybe even more, and simplify the code a lot.

GuillaumeGomez · 2025-05-13T05:42:46Z

That would be much better indeed... Let's ask today at the conference if someone has an idea. =D

samueltardieu · 2025-05-13T19:56:36Z

That would be much better indeed... Let's ask today at the conference if someone has an idea. =D

Now that I think about it… isn't the result of the expansion right under our noses in the HIR, even being the trigger for this lint? Checking the arguments, then issuing the string with proper escaping should be enough. WDYT? No need for replicating the macro call, it has been done by the compiler already.

samueltardieu

Other that the version and the reformatting, fine with me! You should also fix dogfood for the unlined format arg.

clippy_lints/src/useless_concat.rs

samueltardieu · 2025-05-19T14:51:33Z

@rustbot label S-blocked

I'd like to check something before it gets merged, it looks like I can trigger an ICE when I merge this with my snippet diagnostics patch, I'll remove the block ASAP.

samueltardieu · 2025-05-19T15:10:57Z

There is indeed a problem with obtaining the snippet for the declare_tool_lint!() macro source, but this PR handles this properly. I'll investigate the other problem separately. Thanks!

I'll merge it since I approved the FCP a long time ago, and since did many reviews of this code back and forth.

@rustbot review
r? @samueltardieu

rustbot assigned dswij Dec 14, 2024

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

GuillaumeGomez mentioned this pull request Dec 14, 2024

Add useless_concat #13793

Closed

GuillaumeGomez force-pushed the useless-concat branch 2 times, most recently from 4918abc to b92d91b Compare December 15, 2024 20:51

rustbot assigned y21 and unassigned dswij Dec 25, 2024

y21 reviewed Dec 27, 2024

View reviewed changes

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

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

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

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

GuillaumeGomez force-pushed the useless-concat branch from b92d91b to 1b635c0 Compare December 29, 2024 19:32

GuillaumeGomez mentioned this pull request Dec 29, 2024

Move more def paths into clippy_utils::paths #13903

Merged

GuillaumeGomez force-pushed the useless-concat branch 2 times, most recently from 87b000c to 8e6092c Compare December 29, 2024 20:24

GuillaumeGomez force-pushed the useless-concat branch from 8e6092c to 6e39181 Compare February 11, 2025 14:07

y21 reviewed Feb 12, 2025

View reviewed changes

y21 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 Feb 12, 2025

y21 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) labels Feb 22, 2025

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the useless-concat branch from 6e39181 to a56792a Compare May 2, 2025 15:33

GuillaumeGomez force-pushed the useless-concat branch from a56792a to 42f4fd6 Compare May 2, 2025 15:39

samueltardieu requested changes May 8, 2025

View reviewed changes

GuillaumeGomez force-pushed the useless-concat branch from 9e93fb3 to 6019bfc Compare May 11, 2025 21:23

GuillaumeGomez added 3 commits May 12, 2025 14:14

Add new useless_concat lint

a867981

Add ui test for useless_concat lint

126fb00

Move concat macro path into clippy_utils::paths

7b06d2b

GuillaumeGomez force-pushed the useless-concat branch from 6019bfc to b83678b Compare May 12, 2025 12:15

samueltardieu requested changes May 12, 2025

View reviewed changes

GuillaumeGomez force-pushed the useless-concat branch from b83678b to c1e2a7f Compare May 12, 2025 12:49

samueltardieu requested changes May 12, 2025

View reviewed changes

GuillaumeGomez force-pushed the useless-concat branch 2 times, most recently from 16049de to 3a4bfb4 Compare May 14, 2025 16:50

samueltardieu approved these changes May 14, 2025

View reviewed changes

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

Improve code and apply suggestions

0d25090

GuillaumeGomez force-pushed the useless-concat branch from 3a4bfb4 to 0d25090 Compare May 19, 2025 14:45

rustbot added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label May 19, 2025

rustbot assigned samueltardieu and unassigned y21 May 19, 2025

samueltardieu removed the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label May 19, 2025

samueltardieu added this pull request to the merge queue May 19, 2025

Merged via the queue into rust-lang:master with commit ebc2a68 May 19, 2025
11 checks passed

GuillaumeGomez deleted the useless-concat branch May 19, 2025 15:28

Add new useless_concat lint #13829

Add new useless_concat lint #13829

Uh oh!

Conversation

GuillaumeGomez commented Dec 14, 2024

Uh oh!

rustbot commented Dec 14, 2024

Uh oh!

GuillaumeGomez commented Dec 15, 2024

Uh oh!

GuillaumeGomez commented Dec 25, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Dec 29, 2024

Uh oh!

GuillaumeGomez commented Feb 11, 2025

Uh oh!

y21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

y21 Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez May 2, 2025

Choose a reason for hiding this comment

Uh oh!

y21 Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez May 2, 2025

Choose a reason for hiding this comment

Uh oh!

y21 commented Feb 12, 2025

Uh oh!

This comment has been minimized.

GuillaumeGomez commented May 2, 2025

Uh oh!

GuillaumeGomez commented May 2, 2025

Uh oh!

samueltardieu left a comment

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented May 11, 2025

Uh oh!

samueltardieu commented May 12, 2025

Uh oh!

GuillaumeGomez commented May 12, 2025

Uh oh!

samueltardieu left a comment

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented May 12, 2025

Uh oh!

samueltardieu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented May 13, 2025

Uh oh!

samueltardieu commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueltardieu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samueltardieu commented May 19, 2025

Uh oh!

samueltardieu commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

Add new `useless_concat` lint #13829

Add new `useless_concat` lint #13829

samueltardieu left a comment •

edited

Loading

samueltardieu commented May 13, 2025 •

edited

Loading

samueltardieu left a comment •

edited

Loading