From 0478d26c8b32acb56df3d89cb21847974ff0f312 Mon Sep 17 00:00:00 2001 From: not-elm Date: Tue, 2 Apr 2024 22:24:17 +0900 Subject: [PATCH] FIX(12334): manual_swap auto fix Initialization expressions are now generated when index is bound to a variable. FIX: Check to see if variables are used after swap FIX: rename StmtKind::Local to StmtKind::Let --- clippy_lints/src/swap.rs | 174 +++++++++++++++++++++++++-- tests/ui/manual_swap_auto_fix.fixed | 57 +++++++++ tests/ui/manual_swap_auto_fix.rs | 72 +++++++++++ tests/ui/manual_swap_auto_fix.stderr | 88 ++++++++++++++ 4 files changed, 381 insertions(+), 10 deletions(-) create mode 100644 tests/ui/manual_swap_auto_fix.fixed create mode 100644 tests/ui/manual_swap_auto_fix.rs create mode 100644 tests/ui/manual_swap_auto_fix.stderr diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index 78c99e0c0a3..422fb6dfc4d 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -1,8 +1,14 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; -use clippy_utils::source::snippet_with_context; +use clippy_utils::source::{snippet_indent, snippet_with_context}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::is_type_diagnostic_item; + use clippy_utils::{can_mut_borrow_both, eq_expr_value, in_constant, std_or_core}; +use itertools::Itertools; + +use rustc_hir::intravisit::{walk_expr, Visitor}; + +use crate::FxHashSet; use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; @@ -80,7 +86,17 @@ impl<'tcx> LateLintPass<'tcx> for Swap { } } -fn generate_swap_warning(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>, span: Span, is_xor_based: bool) { +#[allow(clippy::too_many_arguments)] +fn generate_swap_warning<'tcx>( + block: &'tcx Block<'tcx>, + cx: &LateContext<'tcx>, + e1: &'tcx Expr<'tcx>, + e2: &'tcx Expr<'tcx>, + rhs1: &'tcx Expr<'tcx>, + rhs2: &'tcx Expr<'tcx>, + span: Span, + is_xor_based: bool, +) { let ctxt = span.ctxt(); let mut applicability = Applicability::MachineApplicable; @@ -99,6 +115,7 @@ fn generate_swap_warning(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>, spa || is_type_diagnostic_item(cx, ty, sym::VecDeque) { let slice = Sugg::hir_with_applicability(cx, lhs1, "", &mut applicability); + span_lint_and_sugg( cx, MANUAL_SWAP, @@ -106,7 +123,17 @@ fn generate_swap_warning(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>, spa format!("this looks like you are swapping elements of `{slice}` manually"), "try", format!( - "{}.swap({}, {});", + "{}{}.swap({}, {});", + IndexBinding { + block, + swap1_idx: idx1, + swap2_idx: idx2, + suggest_span: span, + cx, + ctxt, + applicability: &mut applicability, + } + .snippet_index_bindings(&[idx1, idx2, rhs1, rhs2]), slice.maybe_par(), snippet_with_context(cx, idx1.span, ctxt, "..", &mut applicability).0, snippet_with_context(cx, idx2.span, ctxt, "..", &mut applicability).0, @@ -142,7 +169,7 @@ fn generate_swap_warning(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>, spa } /// Implementation of the `MANUAL_SWAP` lint. -fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) { +fn check_manual_swap<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) { if in_constant(cx, block.hir_id) { return; } @@ -160,10 +187,10 @@ fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) { // bar() = t; && let StmtKind::Semi(second) = s3.kind && let ExprKind::Assign(lhs2, rhs2, _) = second.kind - && let ExprKind::Path(QPath::Resolved(None, rhs2)) = rhs2.kind - && rhs2.segments.len() == 1 + && let ExprKind::Path(QPath::Resolved(None, rhs2_path)) = rhs2.kind + && rhs2_path.segments.len() == 1 - && ident.name == rhs2.segments[0].ident.name + && ident.name == rhs2_path.segments[0].ident.name && eq_expr_value(cx, tmp_init, lhs1) && eq_expr_value(cx, rhs1, lhs2) @@ -174,7 +201,7 @@ fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) { && second.span.ctxt() == ctxt { let span = s1.span.to(s3.span); - generate_swap_warning(cx, lhs1, lhs2, span, false); + generate_swap_warning(block, cx, lhs1, lhs2, rhs1, rhs2, span, false); } } } @@ -254,7 +281,7 @@ fn parse<'a, 'hir>(stmt: &'a Stmt<'hir>) -> Option<(ExprOrIdent<'hir>, &'a Expr< } /// Implementation of the xor case for `MANUAL_SWAP` lint. -fn check_xor_swap(cx: &LateContext<'_>, block: &Block<'_>) { +fn check_xor_swap<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) { for [s1, s2, s3] in block.stmts.array_windows::<3>() { let ctxt = s1.span.ctxt(); if let Some((lhs0, rhs0)) = extract_sides_of_xor_assign(s1, ctxt) @@ -268,7 +295,7 @@ fn check_xor_swap(cx: &LateContext<'_>, block: &Block<'_>) { && s3.span.ctxt() == ctxt { let span = s1.span.to(s3.span); - generate_swap_warning(cx, lhs0, rhs0, span, true); + generate_swap_warning(block, cx, lhs0, rhs0, rhs1, rhs2, span, true); }; } } @@ -294,3 +321,130 @@ fn extract_sides_of_xor_assign<'a, 'hir>( None } } + +struct IndexBinding<'a, 'tcx> { + block: &'a Block<'a>, + swap1_idx: &'a Expr<'a>, + swap2_idx: &'a Expr<'a>, + suggest_span: Span, + cx: &'a LateContext<'tcx>, + ctxt: SyntaxContext, + applicability: &'a mut Applicability, +} + +impl<'a, 'tcx> IndexBinding<'a, 'tcx> { + fn snippet_index_bindings(&mut self, exprs: &[&'tcx Expr<'tcx>]) -> String { + let mut bindings = FxHashSet::default(); + for expr in exprs { + bindings.insert(self.snippet_index_binding(expr)); + } + bindings.into_iter().join("") + } + + fn snippet_index_binding(&mut self, expr: &'tcx Expr<'tcx>) -> String { + match expr.kind { + ExprKind::Binary(_, lhs, rhs) => { + if matches!(lhs.kind, ExprKind::Lit(_)) && matches!(rhs.kind, ExprKind::Lit(_)) { + return String::new(); + } + let lhs_snippet = self.snippet_index_binding(lhs); + let rhs_snippet = self.snippet_index_binding(rhs); + format!("{lhs_snippet}{rhs_snippet}") + }, + ExprKind::Path(QPath::Resolved(_, path)) => { + let init = self.cx.expr_or_init(expr); + + let Some(first_segment) = path.segments.first() else { + return String::new(); + }; + if !self.suggest_span.contains(init.span) || !self.is_used_other_than_swapping(first_segment.ident) { + return String::new(); + } + + let init_str = snippet_with_context(self.cx, init.span, self.ctxt, "", self.applicability) + .0 + .to_string(); + let indent_str = snippet_indent(self.cx, init.span); + let indent_str = indent_str.as_deref().unwrap_or(""); + + format!("let {} = {init_str};\n{indent_str}", first_segment.ident) + }, + _ => String::new(), + } + } + + fn is_used_other_than_swapping(&mut self, idx_ident: Ident) -> bool { + if Self::is_used_slice_indexed(self.swap1_idx, idx_ident) + || Self::is_used_slice_indexed(self.swap2_idx, idx_ident) + { + return true; + } + self.is_used_after_swap(idx_ident) + } + + fn is_used_after_swap(&mut self, idx_ident: Ident) -> bool { + let mut v = IndexBindingVisitor { + found_used: false, + suggest_span: self.suggest_span, + idx: idx_ident, + }; + + for stmt in self.block.stmts { + match stmt.kind { + StmtKind::Expr(expr) | StmtKind::Semi(expr) => v.visit_expr(expr), + StmtKind::Let(rustc_hir::Local { ref init, .. }) => { + if let Some(init) = init.as_ref() { + v.visit_expr(init); + } + }, + StmtKind::Item(_) => {}, + } + } + + v.found_used + } + + fn is_used_slice_indexed(swap_index: &Expr<'_>, idx_ident: Ident) -> bool { + match swap_index.kind { + ExprKind::Binary(_, lhs, rhs) => { + if matches!(lhs.kind, ExprKind::Lit(_)) && matches!(rhs.kind, ExprKind::Lit(_)) { + return false; + } + Self::is_used_slice_indexed(lhs, idx_ident) || Self::is_used_slice_indexed(rhs, idx_ident) + }, + ExprKind::Path(QPath::Resolved(_, path)) => { + path.segments.first().map_or(false, |idx| idx.ident == idx_ident) + }, + _ => false, + } + } +} + +struct IndexBindingVisitor { + idx: Ident, + suggest_span: Span, + found_used: bool, +} + +impl<'tcx> Visitor<'tcx> for IndexBindingVisitor { + fn visit_path_segment(&mut self, path_segment: &'tcx rustc_hir::PathSegment<'tcx>) -> Self::Result { + if path_segment.ident == self.idx { + self.found_used = true; + } + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) -> Self::Result { + if expr.span.hi() <= self.suggest_span.hi() { + return; + } + + match expr.kind { + ExprKind::Path(QPath::Resolved(_, path)) => { + for segment in path.segments { + self.visit_path_segment(segment); + } + }, + _ => walk_expr(self, expr), + } + } +} diff --git a/tests/ui/manual_swap_auto_fix.fixed b/tests/ui/manual_swap_auto_fix.fixed new file mode 100644 index 00000000000..28466ff3f9b --- /dev/null +++ b/tests/ui/manual_swap_auto_fix.fixed @@ -0,0 +1,57 @@ +#![warn(clippy::manual_swap)] +#![no_main] + +fn swap1() { + let mut v = [3, 2, 1, 0]; + let index = v[0]; + v.swap(0, index); +} + +fn swap2() { + let mut v = [3, 2, 1, 0]; + let tmp = v[0]; + v.swap(0, 1); + // check not found in this scope. + let _ = tmp; +} + +fn swap3() { + let mut v = [3, 2]; + let i1 = 0; + let i2 = 1; + v.swap(i1, i2); +} + +fn swap4() { + let mut v = [3, 2, 1]; + let i1 = 0; + let i2 = 1; + v.swap(i1, i2 + 1); +} + +fn swap5() { + let mut v = [0, 1, 2, 3]; + let i1 = 0; + let i2 = 1; + v.swap(i1, i2 + 1); +} + +fn swap6() { + let mut v = [0, 1, 2, 3]; + let index = v[0]; + v.swap(0, index + 1); +} + +fn swap7() { + let mut v = [0, 1, 2, 3]; + let i1 = 0; + let i2 = 6; + v.swap(i1 * 3, i2 / 2); +} + +fn swap8() { + let mut v = [1, 2, 3, 4]; + let i1 = 1; + let i2 = 1; + v.swap(i1 + i2, i2); +} diff --git a/tests/ui/manual_swap_auto_fix.rs b/tests/ui/manual_swap_auto_fix.rs new file mode 100644 index 00000000000..702a9e67d3d --- /dev/null +++ b/tests/ui/manual_swap_auto_fix.rs @@ -0,0 +1,72 @@ +#![warn(clippy::manual_swap)] +#![no_main] + +fn swap1() { + let mut v = [3, 2, 1, 0]; + let index = v[0]; + //~^ ERROR: this looks like you are swapping elements of `v` manually + v[0] = v[index]; + v[index] = index; +} + +fn swap2() { + let mut v = [3, 2, 1, 0]; + let tmp = v[0]; + v[0] = v[1]; + v[1] = tmp; + // check not found in this scope. + let _ = tmp; +} + +fn swap3() { + let mut v = [3, 2]; + let i1 = 0; + let i2 = 1; + let temp = v[i1]; + v[i1] = v[i2]; + v[i2] = temp; +} + +fn swap4() { + let mut v = [3, 2, 1]; + let i1 = 0; + let i2 = 1; + let temp = v[i1]; + v[i1] = v[i2 + 1]; + v[i2 + 1] = temp; +} + +fn swap5() { + let mut v = [0, 1, 2, 3]; + let i1 = 0; + let i2 = 1; + let temp = v[i1]; + v[i1] = v[i2 + 1]; + v[i2 + 1] = temp; +} + +fn swap6() { + let mut v = [0, 1, 2, 3]; + let index = v[0]; + //~^ ERROR: this looks like you are swapping elements of `v` manually + v[0] = v[index + 1]; + v[index + 1] = index; +} + +fn swap7() { + let mut v = [0, 1, 2, 3]; + let i1 = 0; + let i2 = 6; + let tmp = v[i1 * 3]; + v[i1 * 3] = v[i2 / 2]; + v[i2 / 2] = tmp; +} + +fn swap8() { + let mut v = [1, 2, 3, 4]; + let i1 = 1; + let i2 = 1; + let tmp = v[i1 + i2]; + v[i1 + i2] = v[i2]; + v[i2] = tmp; +} diff --git a/tests/ui/manual_swap_auto_fix.stderr b/tests/ui/manual_swap_auto_fix.stderr new file mode 100644 index 00000000000..eecfcd3977b --- /dev/null +++ b/tests/ui/manual_swap_auto_fix.stderr @@ -0,0 +1,88 @@ +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:6:5 + | +LL | / let index = v[0]; +LL | | +LL | | v[0] = v[index]; +LL | | v[index] = index; + | |_____________________^ + | + = note: `-D clippy::manual-swap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_swap)]` +help: try + | +LL ~ let index = v[0]; +LL + v.swap(0, index); + | + +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:14:5 + | +LL | / let tmp = v[0]; +LL | | v[0] = v[1]; +LL | | v[1] = tmp; + | |_______________^ + | +help: try + | +LL ~ let tmp = v[0]; +LL + v.swap(0, 1); + | + +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:25:5 + | +LL | / let temp = v[i1]; +LL | | v[i1] = v[i2]; +LL | | v[i2] = temp; + | |_________________^ help: try: `v.swap(i1, i2);` + +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:34:5 + | +LL | / let temp = v[i1]; +LL | | v[i1] = v[i2 + 1]; +LL | | v[i2 + 1] = temp; + | |_____________________^ help: try: `v.swap(i1, i2 + 1);` + +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:43:5 + | +LL | / let temp = v[i1]; +LL | | v[i1] = v[i2 + 1]; +LL | | v[i2 + 1] = temp; + | |_____________________^ help: try: `v.swap(i1, i2 + 1);` + +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:50:5 + | +LL | / let index = v[0]; +LL | | +LL | | v[0] = v[index + 1]; +LL | | v[index + 1] = index; + | |_________________________^ + | +help: try + | +LL ~ let index = v[0]; +LL + v.swap(0, index + 1); + | + +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:60:5 + | +LL | / let tmp = v[i1 * 3]; +LL | | v[i1 * 3] = v[i2 / 2]; +LL | | v[i2 / 2] = tmp; + | |____________________^ help: try: `v.swap(i1 * 3, i2 / 2);` + +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:69:5 + | +LL | / let tmp = v[i1 + i2]; +LL | | v[i1 + i2] = v[i2]; +LL | | v[i2] = tmp; + | |________________^ help: try: `v.swap(i1 + i2, i2);` + +error: aborting due to 8 previous errors +