From 269b913bfd7da288ca3a1658833686b4b3c518ee Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 17 Mar 2025 16:10:34 +0100 Subject: [PATCH] Better handle blocks with just one expression --- clippy_lints/src/strings.rs | 24 +++++++++++++++++------- tests/ui/string_to_string.rs | 1 - tests/ui/string_to_string.stderr | 2 +- tests/ui/string_to_string_in_map.fixed | 3 +++ tests/ui/string_to_string_in_map.rs | 3 +++ tests/ui/string_to_string_in_map.stderr | 12 +++++++++--- 6 files changed, 33 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index 247b5e270f82..27c548bed9f6 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -14,6 +14,8 @@ use rustc_session::declare_lint_pass; use rustc_span::source_map::Spanned; use rustc_span::sym; +use std::ops::ControlFlow; + declare_clippy_lint! { /// ### What it does /// Checks for string appends of the form `x = x + y` (without @@ -454,13 +456,21 @@ fn is_parent_map_like(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option, expr: &Expr<'_>) -> Option { - let parent = get_parent_expr(cx, expr)?; - - if matches!(parent.kind, ExprKind::Closure(_)) { - is_parent_map_like(cx, parent) - } else { - None - } + // Look for a closure as parent of `expr`, discarding simple blocks + let parent_closure = cx + .tcx + .hir_parent_iter(expr.hir_id) + .try_fold(expr.hir_id, |child_hir_id, (_, node)| match node { + // Check that the child expression is the only expression in the block + Node::Block(block) if block.stmts.is_empty() && block.expr.map(|e| e.hir_id) == Some(child_hir_id) => { + ControlFlow::Continue(block.hir_id) + }, + Node::Expr(expr) if matches!(expr.kind, ExprKind::Block(..)) => ControlFlow::Continue(expr.hir_id), + Node::Expr(expr) if matches!(expr.kind, ExprKind::Closure(_)) => ControlFlow::Break(Some(expr)), + _ => ControlFlow::Break(None), + }) + .break_value()?; + is_parent_map_like(cx, parent_closure?) } fn suggest_cloned_string_to_string(cx: &LateContext<'_>, span: rustc_span::Span) { diff --git a/tests/ui/string_to_string.rs b/tests/ui/string_to_string.rs index 8c97d8c16f93..7c5bd8a897ba 100644 --- a/tests/ui/string_to_string.rs +++ b/tests/ui/string_to_string.rs @@ -15,7 +15,6 @@ fn main() { }); //~^^ string_to_string - // Should not lint. let x = Some(String::new()); let _ = x.unwrap_or_else(|| v.to_string()); //~^ string_to_string diff --git a/tests/ui/string_to_string.stderr b/tests/ui/string_to_string.stderr index c0e2e75f51a7..99eea06f18eb 100644 --- a/tests/ui/string_to_string.stderr +++ b/tests/ui/string_to_string.stderr @@ -17,7 +17,7 @@ LL | x.to_string() = help: consider using `.clone()` error: `to_string()` called on a `String` - --> tests/ui/string_to_string.rs:20:33 + --> tests/ui/string_to_string.rs:19:33 | LL | let _ = x.unwrap_or_else(|| v.to_string()); | ^^^^^^^^^^^^^ diff --git a/tests/ui/string_to_string_in_map.fixed b/tests/ui/string_to_string_in_map.fixed index b004fd736adf..efc085539f15 100644 --- a/tests/ui/string_to_string_in_map.fixed +++ b/tests/ui/string_to_string_in_map.fixed @@ -9,6 +9,9 @@ fn main() { //~^ string_to_string let _ = variable2.cloned(); //~^ string_to_string + #[rustfmt::skip] + let _ = variable2.cloned(); + //~^ string_to_string let _ = vec![String::new()].iter().cloned().collect::>(); //~^ string_to_string diff --git a/tests/ui/string_to_string_in_map.rs b/tests/ui/string_to_string_in_map.rs index 387abe84205d..5bf1d7ba5a2e 100644 --- a/tests/ui/string_to_string_in_map.rs +++ b/tests/ui/string_to_string_in_map.rs @@ -9,6 +9,9 @@ fn main() { //~^ string_to_string let _ = variable2.map(|x| x.to_string()); //~^ string_to_string + #[rustfmt::skip] + let _ = variable2.map(|x| { x.to_string() }); + //~^ string_to_string let _ = vec![String::new()].iter().map(String::to_string).collect::>(); //~^ string_to_string diff --git a/tests/ui/string_to_string_in_map.stderr b/tests/ui/string_to_string_in_map.stderr index 44e2119c1002..35aeed656eed 100644 --- a/tests/ui/string_to_string_in_map.stderr +++ b/tests/ui/string_to_string_in_map.stderr @@ -17,16 +17,22 @@ LL | let _ = variable2.map(|x| x.to_string()); | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` error: `to_string()` called on a `String` - --> tests/ui/string_to_string_in_map.rs:13:40 + --> tests/ui/string_to_string_in_map.rs:13:23 + | +LL | let _ = variable2.map(|x| { x.to_string() }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` + +error: `to_string()` called on a `String` + --> tests/ui/string_to_string_in_map.rs:16:40 | LL | let _ = vec![String::new()].iter().map(String::to_string).collect::>(); | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` error: `to_string()` called on a `String` - --> tests/ui/string_to_string_in_map.rs:15:40 + --> tests/ui/string_to_string_in_map.rs:18:40 | LL | let _ = vec![String::new()].iter().map(|x| x.to_string()).collect::>(); | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors