Skip to content

Commit b59f804

Browse files
samueltardieusamueltardieu
authored
fix(useless_format): fire on wrapped in a block-producing macro (#17060)
## Summary Fixes a false negative in `useless_format`: the lint silently failed to fire on `format!("{}", s)` when the call is the **tail expression of a block produced by another macro**. This affects rustc's entire `define_helper!`-generated family (`with_forced_trimmed_paths!`, `with_no_trimmed_paths!`, `with_no_queries!`, etc). Spotted during review of #17058: > Even as a macro argument, using `format!("{}", some_string)` should trigger it. ## Reproducer ```rust #![feature(decl_macro)] #![warn(clippy::useless_format)] macro_rules! plain_mr { ($e:expr) => { $e }; } macro_rules! block_mr { ($e:expr) => { { let _g: i32 = 0; $e } }; } pub macro plain_dm($e:expr) { $e } pub macro block_dm($e:expr) { { let _g: i32 = 0; $e } } fn s() -> String { String::from("x") } fn main() { let _ = format!("{}", s()); // (1) bare => lints let _ = plain_mr!(format!("{}", s())); // (2) mr, no block => lints let _ = block_mr!(format!("{}", s())); // (3) mr + block => MISS (bug) let _ = plain_dm!(format!("{}", s())); // (4) dm, no block => lints let _ = block_dm!(format!("{}", s())); // (5) dm + block => MISS (bug) } ``` Before this PR, cases (3) and (5) are silently missed. After this PR all five fire as expected. Note that the discriminator is block-wrapping, not `macro_rules` vs `decl_macro`. ## Root cause `clippy_lints/src/format.rs` previously used `root_macro_call_first_node`, which requires `first_node_in_macro == Some(ExpnId::root())`. For `block_dm!(format!(...))`, the `format!` HIR parent is the `Block` emitted by `block_dm`, whose span lives in `block_dm`'s expansion (sibling to `format!`'s). `first_node_in_macro` returns `Some(block_dm.expn)` rather than `Some(root)`, and the lint bails. ## Fix Replace the gate with: ```rust if let Some(macro_call) = matching_root_macro_call(cx, expr.span, sym::format_macro) && first_node_in_macro(cx, expr).is_some_and(|p_expn| p_expn != macro_call.expn) ``` - `matching_root_macro_call` preserves hygiene. The outermost macro on `expr.span`'s backtrace must be `format!`, so a `format!` written inside another macro's body (where the rewrite would target the macro definition) is still ignored. - `first_node_in_macro(..).is_some_and(|p| p != macro_call.expn)` preserves single-firing. `expr` must be the outermost node of `format!`'s expansion. Without `p != macro_call.expn`, deeper nodes whose parent is also in `format!`'s expansion (including internal `format_args!` invocations) would slip through and the lint would fire multiple times per call. ## Test changes - `tests/ui/format.{rs,fixed,stderr}`: added `#![feature(decl_macro)]` and a `block_wrap` module covering all four pass-through and block-wrap combinations across `macro_rules!` and `decl_macro`, as regression coverage. - `tests/ui/unused_format_specs.{rs,1.fixed,2.fixed}`: added `clippy::useless_format` to the allow-list. The relaxed gate also starts firing on `println!("{:.3}", format!("abcde"))`-style cases, which appear in this test's `.fixed` outputs (after `unused_format_specs` suggests `format_args!` to `format!`). This is a latent true positive previously masked by the strict gate. The test is scoped to `unused_format_specs` and should not be entangled with another lint's coverage. The same pattern is already used by `tests/ui/format.rs` itself, which allows other unrelated lints. ## Verification - `cargo test --test compile-test`: 1764 UI tests + 188 fixed checks + 46 ui-cargo tests pass. 0 duplicate diagnostics. - `cargo test --test dogfood`: catches the same false negative in clippy's own `unnecessary_literal_unwrap.rs` (the line that #17059 is fixing manually), demonstrating the fix on real-world code. ## Related - Motivating discussion: #17058. - Companion manual fix that this PR's lint now catches automatically: #17059. - Test file: `tests/ui/format.rs`, see the new `mod block_wrap` at the bottom. changelog: [`useless_format`] no longer misses `format!` calls that are the tail expression of a block produced by another macro (e.g. rustc's `with_forced_trimmed_paths!`).
2 parents 7019104 + c31d970 commit b59f804

8 files changed

Lines changed: 130 additions & 23 deletions

File tree

‎clippy_lints/src/format.rs‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2-
use clippy_utils::macros::{FormatArgsStorage, find_format_arg_expr, root_macro_call_first_node};
2+
use clippy_utils::macros::{FormatArgsStorage, find_format_arg_expr, first_node_in_macro, matching_root_macro_call};
33
use clippy_utils::source::{SpanRangeExt, snippet_with_context};
44
use clippy_utils::sugg::Sugg;
55
use rustc_ast::{FormatArgsPiece, FormatOptions, FormatTrait};
@@ -53,8 +53,13 @@ impl UselessFormat {
5353

5454
impl<'tcx> LateLintPass<'tcx> for UselessFormat {
5555
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
56-
if let Some(macro_call) = root_macro_call_first_node(cx, expr)
57-
&& cx.tcx.is_diagnostic_item(sym::format_macro, macro_call.def_id)
56+
// Loosened from `root_macro_call_first_node` so the lint also fires when `format!` is
57+
// the tail of a block emitted by another macro. The `!= macro_call.expn` check filters
58+
// HIR nodes inside `format!`'s own expansion (its outer block's tail, its nested
59+
// `format_args!`), which would otherwise also pass `first_node_in_macro` and cause the
60+
// lint to fire multiple times per call.
61+
if let Some(macro_call) = matching_root_macro_call(cx, expr.span, sym::format_macro)
62+
&& first_node_in_macro(cx, expr).is_some_and(|p_expn| p_expn != macro_call.expn)
5863
&& let Some(format_args) = self.format_args.get(cx, expr, macro_call.expn)
5964
{
6065
let mut applicability = Applicability::MachineApplicable;

‎clippy_lints/src/methods/unnecessary_literal_unwrap.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ pub(super) fn check(
9292
(sym::None, sym::unwrap_or_default, _) => {
9393
let ty = cx.typeck_results().expr_ty(expr);
9494
let default_ty_string = if let ty::Adt(def, ..) = ty.kind() {
95-
with_forced_trimmed_paths!(format!("{}", cx.tcx.def_path_str(def.did())))
95+
with_forced_trimmed_paths!(cx.tcx.def_path_str(def.did()))
9696
} else {
9797
"Default".to_string()
9898
};

‎tests/ui/format.fixed‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![feature(decl_macro)]
12
#![warn(clippy::useless_format)]
23
#![allow(
34
clippy::print_literal,
@@ -103,3 +104,41 @@ fn main() {
103104
let _ = xx.to_string();
104105
//~^ useless_format
105106
}
107+
108+
// `format!` as the tail expression of a block emitted by another macro
109+
// (e.g. rustc's `with_forced_trimmed_paths!`). The lint must still fire.
110+
mod block_wrap {
111+
macro_rules! plain_mr {
112+
($e:expr) => {
113+
$e
114+
};
115+
}
116+
macro_rules! block_mr {
117+
($e:expr) => {{
118+
let _g: i32 = 0;
119+
$e
120+
}};
121+
}
122+
pub macro plain_dm($e:expr) {
123+
$e
124+
}
125+
pub macro block_dm($e:expr) {{
126+
let _g: i32 = 0;
127+
$e
128+
}}
129+
130+
fn s() -> String {
131+
String::from("x")
132+
}
133+
134+
pub fn check() {
135+
let _ = plain_mr!(s().to_string());
136+
//~^ useless_format
137+
let _ = block_mr!(s().to_string());
138+
//~^ useless_format
139+
let _ = plain_dm!(s().to_string());
140+
//~^ useless_format
141+
let _ = block_dm!(s().to_string());
142+
//~^ useless_format
143+
}
144+
}

‎tests/ui/format.rs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![feature(decl_macro)]
12
#![warn(clippy::useless_format)]
23
#![allow(
34
clippy::print_literal,
@@ -106,3 +107,41 @@ fn main() {
106107
let _ = format!("{xx}");
107108
//~^ useless_format
108109
}
110+
111+
// `format!` as the tail expression of a block emitted by another macro
112+
// (e.g. rustc's `with_forced_trimmed_paths!`). The lint must still fire.
113+
mod block_wrap {
114+
macro_rules! plain_mr {
115+
($e:expr) => {
116+
$e
117+
};
118+
}
119+
macro_rules! block_mr {
120+
($e:expr) => {{
121+
let _g: i32 = 0;
122+
$e
123+
}};
124+
}
125+
pub macro plain_dm($e:expr) {
126+
$e
127+
}
128+
pub macro block_dm($e:expr) {{
129+
let _g: i32 = 0;
130+
$e
131+
}}
132+
133+
fn s() -> String {
134+
String::from("x")
135+
}
136+
137+
pub fn check() {
138+
let _ = plain_mr!(format!("{}", s()));
139+
//~^ useless_format
140+
let _ = block_mr!(format!("{}", s()));
141+
//~^ useless_format
142+
let _ = plain_dm!(format!("{}", s()));
143+
//~^ useless_format
144+
let _ = block_dm!(format!("{}", s()));
145+
//~^ useless_format
146+
}
147+
}

‎tests/ui/format.stderr‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: useless use of `format!`
2-
--> tests/ui/format.rs:20:5
2+
--> tests/ui/format.rs:21:5
33
|
44
LL | format!("foo");
55
| ^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"foo".to_string()`
@@ -8,19 +8,19 @@ LL | format!("foo");
88
= help: to override `-D warnings` add `#[allow(clippy::useless_format)]`
99

1010
error: useless use of `format!`
11-
--> tests/ui/format.rs:22:5
11+
--> tests/ui/format.rs:23:5
1212
|
1313
LL | format!("{{}}");
1414
| ^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"{}".to_string()`
1515

1616
error: useless use of `format!`
17-
--> tests/ui/format.rs:24:5
17+
--> tests/ui/format.rs:25:5
1818
|
1919
LL | format!("{{}} abc {{}}");
2020
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"{} abc {}".to_string()`
2121

2222
error: useless use of `format!`
23-
--> tests/ui/format.rs:26:5
23+
--> tests/ui/format.rs:27:5
2424
|
2525
LL | / format!(
2626
LL | |
@@ -36,70 +36,94 @@ LL ~ " bar"##.to_string();
3636
|
3737

3838
error: useless use of `format!`
39-
--> tests/ui/format.rs:32:13
39+
--> tests/ui/format.rs:33:13
4040
|
4141
LL | let _ = format!("");
4242
| ^^^^^^^^^^^ help: consider using `String::new()`: `String::new()`
4343

4444
error: useless use of `format!`
45-
--> tests/ui/format.rs:35:5
45+
--> tests/ui/format.rs:36:5
4646
|
4747
LL | format!("{}", "foo");
4848
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"foo".to_string()`
4949

5050
error: useless use of `format!`
51-
--> tests/ui/format.rs:44:5
51+
--> tests/ui/format.rs:45:5
5252
|
5353
LL | format!("{}", arg);
5454
| ^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `arg.to_string()`
5555

5656
error: useless use of `format!`
57-
--> tests/ui/format.rs:75:5
57+
--> tests/ui/format.rs:76:5
5858
|
5959
LL | format!("{}", 42.to_string());
6060
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `42.to_string()`
6161

6262
error: useless use of `format!`
63-
--> tests/ui/format.rs:78:5
63+
--> tests/ui/format.rs:79:5
6464
|
6565
LL | format!("{}", x.display().to_string());
6666
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `x.display().to_string()`
6767

6868
error: useless use of `format!`
69-
--> tests/ui/format.rs:83:18
69+
--> tests/ui/format.rs:84:18
7070
|
7171
LL | let _ = Some(format!("{}", a + "bar"));
7272
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `a + "bar"`
7373

7474
error: useless use of `format!`
75-
--> tests/ui/format.rs:88:22
75+
--> tests/ui/format.rs:89:22
7676
|
7777
LL | let _s: String = format!("{}", &*v.join("\n"));
7878
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `(&*v.join("\n")).to_string()`
7979

8080
error: useless use of `format!`
81-
--> tests/ui/format.rs:95:13
81+
--> tests/ui/format.rs:96:13
8282
|
8383
LL | let _ = format!("{x}");
8484
| ^^^^^^^^^^^^^^ help: consider using `.to_string()`: `x.to_string()`
8585

8686
error: useless use of `format!`
87-
--> tests/ui/format.rs:98:13
87+
--> tests/ui/format.rs:99:13
8888
|
8989
LL | let _ = format!("{y}", y = x);
9090
| ^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `x.to_string()`
9191

9292
error: useless use of `format!`
93-
--> tests/ui/format.rs:103:13
93+
--> tests/ui/format.rs:104:13
9494
|
9595
LL | let _ = format!("{abc}");
9696
| ^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `abc.to_string()`
9797

9898
error: useless use of `format!`
99-
--> tests/ui/format.rs:106:13
99+
--> tests/ui/format.rs:107:13
100100
|
101101
LL | let _ = format!("{xx}");
102102
| ^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `xx.to_string()`
103103

104-
error: aborting due to 15 previous errors
104+
error: useless use of `format!`
105+
--> tests/ui/format.rs:138:27
106+
|
107+
LL | let _ = plain_mr!(format!("{}", s()));
108+
| ^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `s().to_string()`
109+
110+
error: useless use of `format!`
111+
--> tests/ui/format.rs:140:27
112+
|
113+
LL | let _ = block_mr!(format!("{}", s()));
114+
| ^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `s().to_string()`
115+
116+
error: useless use of `format!`
117+
--> tests/ui/format.rs:142:27
118+
|
119+
LL | let _ = plain_dm!(format!("{}", s()));
120+
| ^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `s().to_string()`
121+
122+
error: useless use of `format!`
123+
--> tests/ui/format.rs:144:27
124+
|
125+
LL | let _ = block_dm!(format!("{}", s()));
126+
| ^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `s().to_string()`
127+
128+
error: aborting due to 19 previous errors
105129

‎tests/ui/unused_format_specs.1.fixed‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![warn(clippy::unused_format_specs)]
2-
#![allow(unused)]
2+
#![allow(clippy::useless_format)]
33

44
macro_rules! format_args_from_macro {
55
() => {

‎tests/ui/unused_format_specs.2.fixed‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![warn(clippy::unused_format_specs)]
2-
#![allow(unused)]
2+
#![allow(clippy::useless_format)]
33

44
macro_rules! format_args_from_macro {
55
() => {

‎tests/ui/unused_format_specs.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![warn(clippy::unused_format_specs)]
2-
#![allow(unused)]
2+
#![allow(clippy::useless_format)]
33

44
macro_rules! format_args_from_macro {
55
() => {

0 commit comments

Comments
 (0)