From 29492da34bccad34ec1532e2825e43fba8aac4ff Mon Sep 17 00:00:00 2001 From: relaxcn Date: Thu, 5 Jun 2025 02:20:28 +0800 Subject: [PATCH 1/2] Fix suggestion-causes-error of `manual_swap` --- clippy_lints/src/swap.rs | 35 ++++++++++++++++++++++++---- tests/ui/manual_swap_auto_fix.fixed | 11 +++++++++ tests/ui/manual_swap_auto_fix.rs | 14 +++++++++++ tests/ui/manual_swap_auto_fix.stderr | 11 ++++++++- 4 files changed, 66 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index e3ecd6508bf9..dfc63ae7f6af 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -10,7 +10,7 @@ use rustc_data_structures::fx::FxIndexSet; use rustc_hir::intravisit::{Visitor, walk_expr}; use rustc_errors::Applicability; -use rustc_hir::{AssignOpKind, Block, Expr, ExprKind, LetStmt, PatKind, QPath, Stmt, StmtKind}; +use rustc_hir::{AssignOpKind, Block, Expr, ExprKind, LetStmt, PatKind, Path, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::ty; use rustc_session::declare_lint_pass; @@ -350,12 +350,24 @@ impl<'tcx> IndexBinding<'_, 'tcx> { 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) { + + let init = self.cx.expr_or_init(expr); + + // We skip suggesting a variable binding in any of these cases: + // 1. Variable initialization is outside the suggestion span + // 2. Variable initialization is inside the suggestion span but the variable is not used as an index + // or elsewhere later + // 3. Variable initialization is inside the suggestion span and the variable is used as an + // index/elsewhere later, but its declaration is outside the suggestion span + if !self.suggest_span.contains(init.span) + || !self.is_used_other_than_swapping(first_segment.ident) + || self + .get_res_span(expr) + .is_some_and(|span| !self.suggest_span.contains(span)) + { return String::new(); } @@ -371,6 +383,21 @@ impl<'tcx> IndexBinding<'_, 'tcx> { } } + fn get_res_span(&self, expr: &'tcx Expr<'tcx>) -> Option { + if let ExprKind::Path(QPath::Resolved( + _, + Path { + res: rustc_hir::def::Res::Local(hir_id), + .. + }, + )) = expr.kind + { + Some(self.cx.tcx.hir_span(*hir_id)) + } else { + None + } + } + 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) diff --git a/tests/ui/manual_swap_auto_fix.fixed b/tests/ui/manual_swap_auto_fix.fixed index 28466ff3f9b4..6cd81bafce8b 100644 --- a/tests/ui/manual_swap_auto_fix.fixed +++ b/tests/ui/manual_swap_auto_fix.fixed @@ -55,3 +55,14 @@ fn swap8() { let i2 = 1; v.swap(i1 + i2, i2); } + +fn issue_14931() { + let mut v = [1, 2, 3, 4]; + + let mut i1 = 0; + for i2 in 0..4 { + v.swap(i1, i2); + + i1 += 2; + } +} diff --git a/tests/ui/manual_swap_auto_fix.rs b/tests/ui/manual_swap_auto_fix.rs index c9880e651cd7..19dabfd833f6 100644 --- a/tests/ui/manual_swap_auto_fix.rs +++ b/tests/ui/manual_swap_auto_fix.rs @@ -78,3 +78,17 @@ fn swap8() { v[i1 + i2] = v[i2]; v[i2] = tmp; } + +fn issue_14931() { + let mut v = [1, 2, 3, 4]; + + let mut i1 = 0; + for i2 in 0..4 { + let tmp = v[i1]; + //~^ manual_swap + v[i1] = v[i2]; + v[i2] = tmp; + + i1 += 2; + } +} diff --git a/tests/ui/manual_swap_auto_fix.stderr b/tests/ui/manual_swap_auto_fix.stderr index 7ab898fcc726..a0bb32233e23 100644 --- a/tests/ui/manual_swap_auto_fix.stderr +++ b/tests/ui/manual_swap_auto_fix.stderr @@ -92,5 +92,14 @@ LL | | v[i1 + i2] = v[i2]; LL | | v[i2] = tmp; | |________________^ help: try: `v.swap(i1 + i2, i2);` -error: aborting due to 8 previous errors +error: this looks like you are swapping elements of `v` manually + --> tests/ui/manual_swap_auto_fix.rs:87:9 + | +LL | / let tmp = v[i1]; +LL | | +LL | | v[i1] = v[i2]; +LL | | v[i2] = tmp; + | |____________________^ help: try: `v.swap(i1, i2);` + +error: aborting due to 9 previous errors From 4389e177ebb36e6f7ea9dfa496cca46e0c6f3f8a Mon Sep 17 00:00:00 2001 From: Boot0x7c00 Date: Mon, 9 Jun 2025 03:05:28 +0000 Subject: [PATCH 2/2] optimize codes --- clippy_lints/src/swap.rs | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index dfc63ae7f6af..5ecbb56925ec 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -3,14 +3,14 @@ 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, is_in_const_context, std_or_core}; +use clippy_utils::{can_mut_borrow_both, eq_expr_value, is_in_const_context, path_to_local, std_or_core}; use itertools::Itertools; use rustc_data_structures::fx::FxIndexSet; use rustc_hir::intravisit::{Visitor, walk_expr}; use rustc_errors::Applicability; -use rustc_hir::{AssignOpKind, Block, Expr, ExprKind, LetStmt, PatKind, Path, QPath, Stmt, StmtKind}; +use rustc_hir::{AssignOpKind, Block, Expr, ExprKind, LetStmt, PatKind, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::ty; use rustc_session::declare_lint_pass; @@ -357,16 +357,13 @@ impl<'tcx> IndexBinding<'_, 'tcx> { let init = self.cx.expr_or_init(expr); // We skip suggesting a variable binding in any of these cases: - // 1. Variable initialization is outside the suggestion span - // 2. Variable initialization is inside the suggestion span but the variable is not used as an index - // or elsewhere later - // 3. Variable initialization is inside the suggestion span and the variable is used as an - // index/elsewhere later, but its declaration is outside the suggestion span + // - Variable initialization is outside the suggestion span + // - Variable declaration is outside the suggestion span + // - Variable is not used as an index or elsewhere later if !self.suggest_span.contains(init.span) + || path_to_local(expr) + .is_some_and(|hir_id| !self.suggest_span.contains(self.cx.tcx.hir_span(hir_id))) || !self.is_used_other_than_swapping(first_segment.ident) - || self - .get_res_span(expr) - .is_some_and(|span| !self.suggest_span.contains(span)) { return String::new(); } @@ -383,21 +380,6 @@ impl<'tcx> IndexBinding<'_, 'tcx> { } } - fn get_res_span(&self, expr: &'tcx Expr<'tcx>) -> Option { - if let ExprKind::Path(QPath::Resolved( - _, - Path { - res: rustc_hir::def::Res::Local(hir_id), - .. - }, - )) = expr.kind - { - Some(self.cx.tcx.hir_span(*hir_id)) - } else { - None - } - } - 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)