From 01d7a324dc66dfca20e1644419688c9d12b8f486 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Mon, 10 Feb 2025 13:09:31 +0100 Subject: [PATCH] `manual_strip`: use existing identifier instead of placeholder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- clippy_lints/src/manual_strip.rs | 73 +++++++++++++++++++++------- tests/ui/manual_strip.rs | 17 +++++++ tests/ui/manual_strip.stderr | 47 ++++++++++++++++-- tests/ui/manual_strip_fixable.fixed | 15 ++++++ tests/ui/manual_strip_fixable.rs | 17 +++++++ tests/ui/manual_strip_fixable.stderr | 40 +++++++++++++++ 6 files changed, 188 insertions(+), 21 deletions(-) create mode 100644 tests/ui/manual_strip_fixable.fixed create mode 100644 tests/ui/manual_strip_fixable.rs create mode 100644 tests/ui/manual_strip_fixable.stderr diff --git a/clippy_lints/src/manual_strip.rs b/clippy_lints/src/manual_strip.rs index d69384a2cb702..daed88b492e6b 100644 --- a/clippy_lints/src/manual_strip.rs +++ b/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,7 +97,7 @@ 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", @@ -103,10 +105,29 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip { }; 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 { + ("", 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() = {}.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, "".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 { + expr: &'tcx Expr<'tcx>, +) -> (Vec<&'tcx Expr<'tcx>>, FxHashMap) { struct StrippingFinder<'a, 'tcx> { cx: &'a LateContext<'tcx>, strip_kind: StripKind, target: Res, pattern: &'tcx Expr<'tcx>, - results: Vec, + results: Vec<&'tcx Expr<'tcx>>, + bindings: FxHashMap, } 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) } diff --git a/tests/ui/manual_strip.rs b/tests/ui/manual_strip.rs index ce53c99e6b507..086b75a398753 100644 --- a/tests/ui/manual_strip.rs +++ b/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"] diff --git a/tests/ui/manual_strip.stderr b/tests/ui/manual_strip.stderr index 41d15da610c4e..a323ef700e767 100644 --- a/tests/ui/manual_strip.stderr +++ b/tests/ui/manual_strip.stderr @@ -138,13 +138,54 @@ LL ~ .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() = s.strip_prefix("ab") { +LL ~ let stripped = ; +LL | +LL | let stripped = format!("{stripped}-"); +LL ~ println!("{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() = s.strip_prefix("ab") { +LL ~ let mut stripped = ; +LL | +LL | stripped = ""; +LL | let stripped = format!("{stripped}-"); +LL ~ println!("{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() = s.strip_prefix('a') { LL ~ .to_string(); | -error: aborting due to 8 previous errors +error: aborting due to 10 previous errors diff --git a/tests/ui/manual_strip_fixable.fixed b/tests/ui/manual_strip_fixable.fixed new file mode 100644 index 0000000000000..75a3f1645de33 --- /dev/null +++ b/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); + } +} diff --git a/tests/ui/manual_strip_fixable.rs b/tests/ui/manual_strip_fixable.rs new file mode 100644 index 0000000000000..5080068449e20 --- /dev/null +++ b/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()]); + } +} diff --git a/tests/ui/manual_strip_fixable.stderr b/tests/ui/manual_strip_fixable.stderr new file mode 100644 index 0000000000000..1c276e5d8fdfe --- /dev/null +++ b/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 +