manual_strip: use existing identifier instead of placeholder (#14188)

When the manually stripped entity receives a name as the first use
through a simple `let` statement, this name can be used in the generated
`if let Some(…)` expression instead of a placeholder.

Fix #14183

changelog: [`manual_strip`]: reuse existing identifier in suggestion
when possible

clippy_lints/src/manual_strip.rs

@@ -2,19 +2,21 @@ use clippy_config::Conf;
 use clippy_utils::consts::{ConstEvalCtxt, Constant};
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::msrvs::{self, Msrv};
-use clippy_utils::source::snippet;
+use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::usage::mutated_variables;
 use clippy_utils::{eq_expr_value, higher};
+use rustc_ast::BindingMode;
 use rustc_ast::ast::LitKind;
+use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
-use rustc_hir::intravisit::{Visitor, walk_expr};
-use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind};
-use rustc_lint::{LateContext, LateLintPass};
+use rustc_hir::intravisit::{Visitor, walk_expr, walk_pat};
+use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, Node, PatKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext as _};
 use rustc_middle::ty;
 use rustc_session::impl_lint_pass;
 use rustc_span::source_map::Spanned;
-use rustc_span::{Span, sym};
+use rustc_span::{Symbol, sym};
 use std::iter;
 
 declare_clippy_lint! {
@@ -95,18 +97,37 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip {
                 return;
             }
 
-            let strippings = find_stripping(cx, strip_kind, target_res, pattern, then);
+            let (strippings, bindings) = find_stripping(cx, strip_kind, target_res, pattern, then);
             if !strippings.is_empty() {
                 let kind_word = match strip_kind {
                     StripKind::Prefix => "prefix",
                     StripKind::Suffix => "suffix",
                 };
 
                 let test_span = expr.span.until(then.span);
+
+                // If the first use is a simple `let` statement, reuse its identifier in the `if let Some(…)` and
+                // remove the `let` statement as long as the identifier is never bound again within the lexical
+                // scope of interest.
+                let (ident_name, let_stmt_span, skip, mut app) = if let Node::LetStmt(let_stmt) =
+                    cx.tcx.parent_hir_node(strippings[0].hir_id)
+                    && let PatKind::Binding(BindingMode::NONE, _, ident, None) = &let_stmt.pat.kind
+                    && bindings.get(&ident.name) == Some(&1)
+                {
+                    (
+                        ident.name.as_str(),
+                        Some(cx.sess().source_map().span_extend_while_whitespace(let_stmt.span)),
+                        1,
+                        Applicability::MachineApplicable,
+                    )
+                } else {
+                    ("<stripped>", None, 0, Applicability::HasPlaceholders)
+                };
+
                 span_lint_and_then(
                     cx,
                     MANUAL_STRIP,
-                    strippings[0],
+                    strippings[0].span,
                     format!("stripping a {kind_word} manually"),
                     |diag| {
                         diag.span_note(test_span, format!("the {kind_word} was tested here"));
@@ -115,14 +136,20 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip {
                             iter::once((
                                 test_span,
                                 format!(
-                                    "if let Some(<stripped>) = {}.strip_{kind_word}({}) ",
-                                    snippet(cx, target_arg.span, ".."),
-                                    snippet(cx, pattern.span, "..")
+                                    "if let Some({ident_name}) = {}.strip_{kind_word}({}) ",
+                                    snippet_with_applicability(cx, target_arg.span, "_", &mut app),
+                                    snippet_with_applicability(cx, pattern.span, "_", &mut app)
                                 ),
                             ))
-                            .chain(strippings.into_iter().map(|span| (span, "<stripped>".into())))
+                            .chain(let_stmt_span.map(|span| (span, String::new())))
+                            .chain(
+                                strippings
+                                    .into_iter()
+                                    .skip(skip)
+                                    .map(|expr| (expr.span, ident_name.into())),
+                            )
                             .collect(),
-                            Applicability::HasPlaceholders,
+                            app,
                         );
                     },
                 );
@@ -188,19 +215,21 @@ fn peel_ref<'a>(expr: &'a Expr<'_>) -> &'a Expr<'a> {
 /// Find expressions where `target` is stripped using the length of `pattern`.
 /// We'll suggest replacing these expressions with the result of the `strip_{prefix,suffix}`
 /// method.
+/// Also, all bindings found during the visit are counted and returned.
 fn find_stripping<'tcx>(
     cx: &LateContext<'tcx>,
     strip_kind: StripKind,
     target: Res,
     pattern: &'tcx Expr<'_>,
-    expr: &'tcx Expr<'_>,
-) -> Vec<Span> {
+    expr: &'tcx Expr<'tcx>,
+) -> (Vec<&'tcx Expr<'tcx>>, FxHashMap<Symbol, usize>) {
     struct StrippingFinder<'a, 'tcx> {
         cx: &'a LateContext<'tcx>,
         strip_kind: StripKind,
         target: Res,
         pattern: &'tcx Expr<'tcx>,
-        results: Vec<Span>,
+        results: Vec<&'tcx Expr<'tcx>>,
+        bindings: FxHashMap<Symbol, usize>,
     }
 
     impl<'tcx> Visitor<'tcx> for StrippingFinder<'_, 'tcx> {
@@ -215,7 +244,7 @@ fn find_stripping<'tcx>(
                 match (self.strip_kind, start, end) {
                     (StripKind::Prefix, Some(start), None) => {
                         if eq_pattern_length(self.cx, self.pattern, start) {
-                            self.results.push(ex.span);
+                            self.results.push(ex);
                             return;
                         }
                     },
@@ -232,7 +261,7 @@ fn find_stripping<'tcx>(
                             && self.cx.qpath_res(left_path, left_arg.hir_id) == self.target
                             && eq_pattern_length(self.cx, self.pattern, right)
                         {
-                            self.results.push(ex.span);
+                            self.results.push(ex);
                             return;
                         }
                     },
@@ -242,6 +271,13 @@ fn find_stripping<'tcx>(
 
             walk_expr(self, ex);
         }
+
+        fn visit_pat(&mut self, pat: &'tcx rustc_hir::Pat<'tcx>) -> Self::Result {
+            if let PatKind::Binding(_, _, ident, _) = pat.kind {
+                *self.bindings.entry(ident.name).or_default() += 1;
+            }
+            walk_pat(self, pat);
+        }
     }
 
     let mut finder = StrippingFinder {
@@ -250,7 +286,8 @@ fn find_stripping<'tcx>(
         target,
         pattern,
         results: vec![],
+        bindings: FxHashMap::default(),
     };
     walk_expr(&mut finder, expr);
-    finder.results
+    (finder.results, finder.bindings)
 }

tests/ui/manual_strip.rs

@@ -74,6 +74,23 @@ fn main() {
     if s3.starts_with("ab") {
         s4[2..].to_string();
     }
+
+    // Don't propose to reuse the `stripped` identifier as it is overriden
+    if s.starts_with("ab") {
+        let stripped = &s["ab".len()..];
+        //~^ ERROR: stripping a prefix manually
+        let stripped = format!("{stripped}-");
+        println!("{stripped}{}", &s["ab".len()..]);
+    }
+
+    // Don't propose to reuse the `stripped` identifier as it is mutable
+    if s.starts_with("ab") {
+        let mut stripped = &s["ab".len()..];
+        //~^ ERROR: stripping a prefix manually
+        stripped = "";
+        let stripped = format!("{stripped}-");
+        println!("{stripped}{}", &s["ab".len()..]);
+    }
 }
 
 #[clippy::msrv = "1.44"]

tests/ui/manual_strip.stderr

@@ -138,13 +138,54 @@ LL ~         <stripped>.to_uppercase();
    |
 
 error: stripping a prefix manually
-  --> tests/ui/manual_strip.rs:91:9
+  --> tests/ui/manual_strip.rs:80:24
+   |
+LL |         let stripped = &s["ab".len()..];
+   |                        ^^^^^^^^^^^^^^^^
+   |
+note: the prefix was tested here
+  --> tests/ui/manual_strip.rs:79:5
+   |
+LL |     if s.starts_with("ab") {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+help: try using the `strip_prefix` method
+   |
+LL ~     if let Some(<stripped>) = s.strip_prefix("ab") {
+LL ~         let stripped = <stripped>;
+LL |
+LL |         let stripped = format!("{stripped}-");
+LL ~         println!("{stripped}{}", <stripped>);
+   |
+
+error: stripping a prefix manually
+  --> tests/ui/manual_strip.rs:88:28
+   |
+LL |         let mut stripped = &s["ab".len()..];
+   |                            ^^^^^^^^^^^^^^^^
+   |
+note: the prefix was tested here
+  --> tests/ui/manual_strip.rs:87:5
+   |
+LL |     if s.starts_with("ab") {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+help: try using the `strip_prefix` method
+   |
+LL ~     if let Some(<stripped>) = s.strip_prefix("ab") {
+LL ~         let mut stripped = <stripped>;
+LL |
+LL |         stripped = "";
+LL |         let stripped = format!("{stripped}-");
+LL ~         println!("{stripped}{}", <stripped>);
+   |
+
+error: stripping a prefix manually
+  --> tests/ui/manual_strip.rs:108:9
    |
 LL |         s[1..].to_string();
    |         ^^^^^^
    |
 note: the prefix was tested here
-  --> tests/ui/manual_strip.rs:90:5
+  --> tests/ui/manual_strip.rs:107:5
    |
 LL |     if s.starts_with('a') {
    |     ^^^^^^^^^^^^^^^^^^^^^^
@@ -154,5 +195,5 @@ LL ~     if let Some(<stripped>) = s.strip_prefix('a') {
 LL ~         <stripped>.to_string();
    |
 
-error: aborting due to 8 previous errors
+error: aborting due to 10 previous errors
 

tests/ui/manual_strip_fixable.fixed

@@ -0,0 +1,15 @@
+#![warn(clippy::manual_strip)]
+
+fn main() {
+    let s = "abc";
+
+    if let Some(stripped) = s.strip_prefix("ab") {
+        //~^ ERROR: stripping a prefix manually
+        println!("{stripped}{}", stripped);
+    }
+
+    if let Some(stripped) = s.strip_suffix("bc") {
+        //~^ ERROR: stripping a suffix manually
+        println!("{stripped}{}", stripped);
+    }
+}

tests/ui/manual_strip_fixable.rs

@@ -0,0 +1,17 @@
+#![warn(clippy::manual_strip)]
+
+fn main() {
+    let s = "abc";
+
+    if s.starts_with("ab") {
+        let stripped = &s["ab".len()..];
+        //~^ ERROR: stripping a prefix manually
+        println!("{stripped}{}", &s["ab".len()..]);
+    }
+
+    if s.ends_with("bc") {
+        let stripped = &s[..s.len() - "bc".len()];
+        //~^ ERROR: stripping a suffix manually
+        println!("{stripped}{}", &s[..s.len() - "bc".len()]);
+    }
+}

tests/ui/manual_strip_fixable.stderr

@@ -0,0 +1,40 @@
+error: stripping a prefix manually
+  --> tests/ui/manual_strip_fixable.rs:7:24
+   |
+LL |         let stripped = &s["ab".len()..];
+   |                        ^^^^^^^^^^^^^^^^
+   |
+note: the prefix was tested here
+  --> tests/ui/manual_strip_fixable.rs:6:5
+   |
+LL |     if s.starts_with("ab") {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+   = note: `-D clippy::manual-strip` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::manual_strip)]`
+help: try using the `strip_prefix` method
+   |
+LL ~     if let Some(stripped) = s.strip_prefix("ab") {
+LL ~
+LL ~         println!("{stripped}{}", stripped);
+   |
+
+error: stripping a suffix manually
+  --> tests/ui/manual_strip_fixable.rs:13:24
+   |
+LL |         let stripped = &s[..s.len() - "bc".len()];
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: the suffix was tested here
+  --> tests/ui/manual_strip_fixable.rs:12:5
+   |
+LL |     if s.ends_with("bc") {
+   |     ^^^^^^^^^^^^^^^^^^^^^
+help: try using the `strip_suffix` method
+   |
+LL ~     if let Some(stripped) = s.strip_suffix("bc") {
+LL ~
+LL ~         println!("{stripped}{}", stripped);
+   |
+
+error: aborting due to 2 previous errors
+