From 5d1d1825da2955432b00f2e2c8d62f72a9f6239e Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 16 Feb 2025 22:42:52 +0000 Subject: [PATCH 01/26] Add new lint [] init --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/manual_checked_sub.rs | 219 +++++++++++++++++++++++++ clippy_utils/src/msrvs.rs | 2 +- tests/ui/manual_checked_sub.fixed | 102 ++++++++++++ tests/ui/manual_checked_sub.rs | 103 ++++++++++++ tests/ui/manual_checked_sub.stderr | 41 +++++ 8 files changed, 470 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/manual_checked_sub.rs create mode 100644 tests/ui/manual_checked_sub.fixed create mode 100644 tests/ui/manual_checked_sub.rs create mode 100644 tests/ui/manual_checked_sub.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b62c9a59aa5..27b5854cc223 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5847,6 +5847,7 @@ Released 2018-09-13 [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn [`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits [`manual_c_str_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_c_str_literals +[`manual_checked_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_checked_sub [`manual_clamp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp [`manual_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_contains [`manual_dangling_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_dangling_ptr diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2cccd6ba2702..c2aa079ea090 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -289,6 +289,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::manual_assert::MANUAL_ASSERT_INFO, crate::manual_async_fn::MANUAL_ASYNC_FN_INFO, crate::manual_bits::MANUAL_BITS_INFO, + crate::manual_checked_sub::MANUAL_CHECKED_SUB_INFO, crate::manual_clamp::MANUAL_CLAMP_INFO, crate::manual_div_ceil::MANUAL_DIV_CEIL_INFO, crate::manual_float_methods::MANUAL_IS_FINITE_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5fa8f6f4bf3d..dc843eddcbce 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -208,6 +208,7 @@ mod manual_abs_diff; mod manual_assert; mod manual_async_fn; mod manual_bits; +mod manual_checked_sub; mod manual_clamp; mod manual_div_ceil; mod manual_float_methods; @@ -943,5 +944,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(manual_option_as_slice::ManualOptionAsSlice::new(conf))); store.register_late_pass(|_| Box::new(single_option_map::SingleOptionMap)); store.register_late_pass(move |_| Box::new(redundant_test_prefix::RedundantTestPrefix)); + store.register_late_pass(move |_| Box::new(manual_checked_sub::ManualCheckedSub::new(conf))); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs new file mode 100644 index 000000000000..09793b5d85b8 --- /dev/null +++ b/clippy_lints/src/manual_checked_sub.rs @@ -0,0 +1,219 @@ +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::source::snippet_with_applicability; +use rustc_ast::{BinOpKind, LitIntType, LitKind}; +use rustc_data_structures::packed::Pu128; +use rustc_errors::Applicability; +use rustc_hir::def::Res; +use rustc_hir::intravisit::{Visitor, walk_expr}; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self}; +use rustc_session::impl_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for manual re-implementations of checked subtraction. + /// + /// ### Why is this bad? + /// Manually re-implementing checked subtraction can be error-prone and less readable. + /// Using the standard library method `.checked_sub()` is clearer and less likely to contain bugs. + /// + /// ### Example + /// ```no_run + /// if a >= b { + /// a - b + /// } + /// ``` + /// Use instead: + /// ```no_run + /// a.checked_sub(b) + /// ``` + #[clippy::version = "1.86.0"] + pub MANUAL_CHECKED_SUB, + complexity, + "default lint description" +} + +pub struct ManualCheckedSub { + msrv: Msrv, +} + +impl ManualCheckedSub { + #[must_use] + pub fn new(conf: &'static Conf) -> Self { + Self { + msrv: conf.msrv.clone(), + } + } +} + +impl_lint_pass!(ManualCheckedSub => [MANUAL_CHECKED_SUB]); + +impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if !self.msrv.meets(msrvs::MANUAL_CHECKED_SUB) { + return; + } + + let applicability = Applicability::MachineApplicable; + + if let ExprKind::If(drop_temp_expr, body_block, _) = expr.kind + && let ExprKind::DropTemps(condition_expr) = drop_temp_expr.kind + && let ExprKind::Binary(op, lhs, rhs) = condition_expr.kind + && is_unsigned_int(cx, &lhs) + && is_unsigned_int(cx, &rhs) + { + if let BinOpKind::Ge = op.node { + SubExprVisitor { + cx, + applicability, + condition_lhs: &lhs, + condition_rhs: &rhs, + } + .visit_expr(body_block) + } + + if let BinOpKind::Gt = op.node { + if let ExprKind::Lit(lit) = &rhs.kind + && let LitKind::Int(Pu128(0), _) = lit.node + { + SubExprVisitor { + cx, + applicability, + condition_lhs: &lhs, + condition_rhs: &rhs, + } + .visit_expr(body_block) + } + } + } + } +} + +struct SubExprVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + applicability: Applicability, + condition_lhs: &'tcx Expr<'tcx>, + condition_rhs: &'tcx Expr<'tcx>, +} + +impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if let ExprKind::Binary(op, sub_lhs, sub_rhs) = expr.kind + && let BinOpKind::Sub = op.node + { + if let ExprKind::Lit(lit) = self.condition_rhs.kind + && let LitKind::Int(Pu128(0), _) = lit.node + && (is_referencing_same_variable(&sub_lhs, self.condition_lhs) + || are_literals_equal(&sub_lhs, self.condition_lhs)) + { + self.build_suggession(expr); + } else { + if (is_referencing_same_variable(&sub_lhs, self.condition_lhs) + || are_literals_equal(&sub_lhs, self.condition_lhs)) + && (is_referencing_same_variable(&sub_rhs, self.condition_rhs) + || are_literals_equal(&sub_rhs, self.condition_rhs)) + { + self.build_suggession(expr); + } + } + } + + walk_expr(self, expr); + } +} + +impl<'a, 'tcx> SubExprVisitor<'a, 'tcx> { + fn build_suggession(&mut self, expr: &Expr<'tcx>) { + let lhs_snippet = snippet_with_applicability(self.cx, self.condition_lhs.span, "..", &mut self.applicability); + let rhs_snippet = snippet_with_applicability(self.cx, self.condition_rhs.span, "..", &mut self.applicability); + + let lhs_needs_parens = matches!(self.condition_lhs.kind, ExprKind::Cast(..)); + + let suggestion = if lhs_needs_parens { + format!("({}).checked_sub({})", lhs_snippet, rhs_snippet) + } else { + format!("{}.checked_sub({})", lhs_snippet, rhs_snippet) + }; + + span_lint_and_sugg( + self.cx, + MANUAL_CHECKED_SUB, + expr.span, + "manual re-implementation of checked subtraction", + "consider using `.checked_sub()`", + suggestion, + self.applicability, + ); + } +} + +fn is_unsigned_int<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> bool { + let expr_type = cx.typeck_results().expr_ty(expr).peel_refs(); + if matches!(expr_type.kind(), ty::Uint(_)) { + return true; + } + + if let ExprKind::Lit(lit) = &expr.kind { + if let LitKind::Int(_, suffix) = &lit.node { + if let LitIntType::Unsuffixed = suffix { + return true; + } + } + } + false +} + +fn are_literals_equal<'tcx>(expr1: &Expr<'tcx>, expr2: &Expr<'tcx>) -> bool { + if let (ExprKind::Lit(lit1), ExprKind::Lit(lit2)) = (&expr1.kind, &expr2.kind) { + if let (LitKind::Int(val1, suffix1), LitKind::Int(val2, suffix2)) = (&lit1.node, &lit2.node) { + return val1 == val2 + && suffix1 == suffix2 + && *suffix1 != LitIntType::Unsuffixed + && *suffix2 != LitIntType::Unsuffixed; + } + } + false +} +fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<'_>) -> bool { + if let ExprKind::Cast(cast_expr1, _) = &expr1.kind { + if let Some(cast_expr1_path) = get_resolved_path_id(cast_expr1) { + if let ExprKind::Cast(cast_expr2, _) = &expr2.kind { + if let Some(cast_expr2_path) = get_resolved_path_id(cast_expr2) { + return cast_expr1_path == cast_expr2_path; + } + } else { + if let Some(expr2_path) = get_resolved_path_id(expr2) { + return cast_expr1_path == expr2_path; + }; + return false; + } + } + } else if let ExprKind::Cast(cast_expr2, _) = &expr2.kind { + if let Some(cast_expr2_path) = get_resolved_path_id(cast_expr2) { + if let Some(expr1_path) = get_resolved_path_id(expr1) { + return expr1_path == cast_expr2_path; + }; + return false; + } + } + + let expr1_path = get_resolved_path_id(expr1); + let expr2_path = get_resolved_path_id(expr2); + + if let (Some(expr1_path), Some(expr2_path)) = (expr1_path, expr2_path) { + expr1_path == expr2_path + } else { + false + } +} + +fn get_resolved_path_id<'tcx>(expr: &'tcx Expr<'_>) -> Option { + if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { + path.segments.first().map(|segment| segment.res) + } else { + None + } +} diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 19061b574ff8..1d7177dbb7f9 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -51,7 +51,7 @@ msrv_aliases! { 1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST } 1,51,0 { BORROW_AS_PTR, SEEK_FROM_CURRENT, UNSIGNED_ABS } 1,50,0 { BOOL_THEN, CLAMP, SLICE_FILL } - 1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST } + 1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, MANUAL_CHECKED_SUB } 1,46,0 { CONST_IF_MATCH } 1,45,0 { STR_STRIP_PREFIX } 1,43,0 { LOG2_10, LOG10_2, NUMERIC_ASSOCIATED_CONSTANTS } diff --git a/tests/ui/manual_checked_sub.fixed b/tests/ui/manual_checked_sub.fixed new file mode 100644 index 000000000000..48f4d6bf7423 --- /dev/null +++ b/tests/ui/manual_checked_sub.fixed @@ -0,0 +1,102 @@ +#![allow(clippy::collapsible_if)] +#![warn(clippy::manual_checked_sub)] + +//Positive test, lint +fn positive_tests() { + let a: u32 = 10; + let b: u32 = 5; + + // Case 1: a - b inside an if a >= b + if a >= b { + let _ = a.checked_sub(b); + } + + // Case 2: a - 1 inside an if a > 0 + if a > 0 { + let _ = a.checked_sub(0); + } + + // Case 3: Both suffixed, should lint + if 10u32 >= 5u32 { + let _ = 10u32.checked_sub(5u32); + } + + let x: i32 = 10; + let y: i32 = 5; + + // Case 4: casted variables inside an if, should lint + if x as u32 >= y as u32 { + let _ = (x as u32).checked_sub(y as u32); + } + + // Case 5: casted variable and literal inside an if, should lint + if x as u32 >= 5u32 { + let _ = (x as u32).checked_sub(5u32); + } +} + +// Negative tests, no lint +fn negative_tests() { + let a: u32 = 10; + let b: u32 = 5; + + // Case 1: Uses checked_sub() correctly, no lint + let _ = a.checked_sub(b); + + // Case 2: Works with signed integers (i32), no lint + let x: i32 = 10; + let y: i32 = 5; + if x >= y { + let _ = x - y; + } + + // Case 3: If condition doesn't match, no lint + if a == b { + let _ = a - b; + } + + // Case 4: a - b inside an if a > b + if a > b { + let _ = a - b; + } + + // Case 5: Unsuffixed literals, no lint + if 10 >= 5 { + let _ = 10 - 5; + } + + // Case 6: Suffixed lhs, unsuffixed rhs, no lint + if 10u32 >= 5 { + let _ = 10u32 - 5; + } + + // Case 7: Unsuffixed lhs, suffixed rhs, no lint + if 10 >= 5u32 { + let _ = 10 - 5u32; + } +} + +fn edge_cases() { + let a: u32 = 10; + let b: u32 = 5; + let c = 3; + + // Edge Case 1: Subtraction inside a nested if + if a >= b { + if c > 0 { + let _ = a.checked_sub(b); + } + } + + if a > b { + if a - b > c { + let _ = a - b; // This subtraction is safe because `a > b` is checked + } + } +} + +fn main() { + positive_tests(); + negative_tests(); + edge_cases(); +} diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs new file mode 100644 index 000000000000..4107966ee758 --- /dev/null +++ b/tests/ui/manual_checked_sub.rs @@ -0,0 +1,103 @@ +#![allow(clippy::collapsible_if)] +#![warn(clippy::manual_checked_sub)] + +//Positive test, lint +fn positive_tests() { + let a: u32 = 10; + let b: u32 = 5; + + // Case 1: a - b inside an if a >= b + if a >= b { + let _ = a - b; + } + + // Case 2: a - 1 inside an if a > 0 + if a > 0 { + let _ = a - 1; + } + + // Case 3: Both suffixed, should lint + if 10u32 >= 5u32 { + let _ = 10u32 - 5u32; + } + + let x: i32 = 10; + let y: i32 = 5; + + // Case 4: casted variables inside an if, should lint + if x as u32 >= y as u32 { + let _ = x as u32 - y as u32; + } + + // Case 5: casted variable and literal inside an if, should lint + if x as u32 >= 5u32 { + let _ = x as u32 - 5u32; + } +} + +// Negative tests, no lint +fn negative_tests() { + let a: u32 = 10; + let b: u32 = 5; + + // Case 1: Uses checked_sub() correctly, no lint + let _ = a.checked_sub(b); + + // Case 2: Works with signed integers (i32), no lint + let x: i32 = 10; + let y: i32 = 5; + if x >= y { + let _ = x - y; + } + + // Case 3: If condition doesn't match, no lint + if a == b { + let _ = a - b; + } + + // Case 4: a - b inside an if a > b + if a > b { + let _ = a - b; + } + + // Case 5: Unsuffixed literals, no lint + if 10 >= 5 { + let _ = 10 - 5; + } + + // Case 6: Suffixed lhs, unsuffixed rhs, no lint + if 10u32 >= 5 { + let _ = 10u32 - 5; + } + + // Case 7: Unsuffixed lhs, suffixed rhs, no lint + if 10 >= 5u32 { + let _ = 10 - 5u32; + } +} + +fn edge_cases() { + let a: u32 = 10; + let b: u32 = 5; + let c = 3; + + // Edge Case 1: Subtraction inside a nested if, should lint + if a >= b { + if c > 0 { + let _ = a - b; + } + } + + // no lint + if a > b { + if a - b > c { + let _ = a - b; + } + } +} + +fn main() { + positive_tests(); + negative_tests(); + edge_cases(); +} diff --git a/tests/ui/manual_checked_sub.stderr b/tests/ui/manual_checked_sub.stderr new file mode 100644 index 000000000000..3168a75511de --- /dev/null +++ b/tests/ui/manual_checked_sub.stderr @@ -0,0 +1,41 @@ +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:11:17 + | +LL | let _ = a - b; + | ^^^^^ help: consider using `.checked_sub()`: `a.checked_sub(b)` + | + = note: `-D clippy::manual-checked-sub` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_checked_sub)]` + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:16:17 + | +LL | let _ = a - 1; + | ^^^^^ help: consider using `.checked_sub()`: `a.checked_sub(0)` + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:21:17 + | +LL | let _ = 10u32 - 5u32; + | ^^^^^^^^^^^^ help: consider using `.checked_sub()`: `10u32.checked_sub(5u32)` + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:29:17 + | +LL | let _ = x as u32 - y as u32; + | ^^^^^^^^^^^^^^^^^^^ help: consider using `.checked_sub()`: `(x as u32).checked_sub(y as u32)` + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:34:17 + | +LL | let _ = x as u32 - 5u32; + | ^^^^^^^^^^^^^^^ help: consider using `.checked_sub()`: `(x as u32).checked_sub(5u32)` + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:87:21 + | +LL | let _ = a - b; + | ^^^^^ help: consider using `.checked_sub()`: `a.checked_sub(b)` + +error: aborting due to 6 previous errors + From 0b9eba2220b7d8be12e314f59cad6d94a8c72332 Mon Sep 17 00:00:00 2001 From: benacq Date: Sat, 1 Mar 2025 22:48:55 +0000 Subject: [PATCH 02/26] Lint test case false positives corrected --- clippy_lints/src/manual_checked_sub.rs | 45 +++++- tests/ui/implicit_saturating_sub.rs | 7 +- tests/ui/manual_checked_sub.fixed | 121 ++++++++++----- tests/ui/manual_checked_sub.rs | 104 +++++++++---- tests/ui/manual_checked_sub.stderr | 204 ++++++++++++++++++++++--- 5 files changed, 380 insertions(+), 101 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 09793b5d85b8..ccb15259f1b6 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -33,7 +33,7 @@ declare_clippy_lint! { #[clippy::version = "1.86.0"] pub MANUAL_CHECKED_SUB, complexity, - "default lint description" + "Checks for manual re-implementations of checked subtraction." } pub struct ManualCheckedSub { @@ -59,7 +59,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { let applicability = Applicability::MachineApplicable; - if let ExprKind::If(drop_temp_expr, body_block, _) = expr.kind + if let ExprKind::If(drop_temp_expr, body_block, else_block) = expr.kind && let ExprKind::DropTemps(condition_expr) = drop_temp_expr.kind && let ExprKind::Binary(op, lhs, rhs) = condition_expr.kind && is_unsigned_int(cx, &lhs) @@ -69,6 +69,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { SubExprVisitor { cx, applicability, + if_expr: expr, + if_body_block: body_block, + else_block, condition_lhs: &lhs, condition_rhs: &rhs, } @@ -82,6 +85,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { SubExprVisitor { cx, applicability, + if_expr: expr, + if_body_block: body_block, + else_block, condition_lhs: &lhs, condition_rhs: &rhs, } @@ -94,6 +100,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { struct SubExprVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, + if_expr: &'tcx Expr<'tcx>, + if_body_block: &'tcx Expr<'tcx>, + else_block: Option<&'tcx Expr<'tcx>>, applicability: Applicability, condition_lhs: &'tcx Expr<'tcx>, condition_rhs: &'tcx Expr<'tcx>, @@ -127,21 +136,43 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { impl<'a, 'tcx> SubExprVisitor<'a, 'tcx> { fn build_suggession(&mut self, expr: &Expr<'tcx>) { + // if let Some(else_expr) = self.else_block { + // println!("ELSE BLOCK in suggestion:::: {:#?}", else_expr); + // } + + let body_snippet = snippet_with_applicability(self.cx, self.if_body_block.span, "..", &mut self.applicability); + + let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut self.applicability); + let updated_usage_context_snippet = body_snippet.as_ref().replace(binary_expr_snippet.as_ref(), "result"); + // let else_snippet = snippet_with_applicability(self.cx, self.if_else_block.span, "..", &mut + // self.applicability); + let lhs_snippet = snippet_with_applicability(self.cx, self.condition_lhs.span, "..", &mut self.applicability); let rhs_snippet = snippet_with_applicability(self.cx, self.condition_rhs.span, "..", &mut self.applicability); let lhs_needs_parens = matches!(self.condition_lhs.kind, ExprKind::Cast(..)); - let suggestion = if lhs_needs_parens { - format!("({}).checked_sub({})", lhs_snippet, rhs_snippet) + let mut suggestion = if lhs_needs_parens { + format!( + "if let Some(result) = ({}).checked_sub({}) {}", + lhs_snippet, rhs_snippet, updated_usage_context_snippet + ) } else { - format!("{}.checked_sub({})", lhs_snippet, rhs_snippet) + format!( + "if let Some(result) = {}.checked_sub({}) {}", + lhs_snippet, rhs_snippet, updated_usage_context_snippet + ) }; + if let Some(else_expr) = self.else_block { + let else_snippet = snippet_with_applicability(self.cx, else_expr.span, "..", &mut self.applicability); + suggestion.push_str(&format!(" else {}", else_snippet)); + } + span_lint_and_sugg( self.cx, MANUAL_CHECKED_SUB, - expr.span, + self.if_expr.span, "manual re-implementation of checked subtraction", "consider using `.checked_sub()`", suggestion, @@ -212,7 +243,7 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<' fn get_resolved_path_id<'tcx>(expr: &'tcx Expr<'_>) -> Option { if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { - path.segments.first().map(|segment| segment.res) + path.segments.first().and_then(|segment| Some(segment.res)) } else { None } diff --git a/tests/ui/implicit_saturating_sub.rs b/tests/ui/implicit_saturating_sub.rs index 7ca57a2902db..5d774127cb62 100644 --- a/tests/ui/implicit_saturating_sub.rs +++ b/tests/ui/implicit_saturating_sub.rs @@ -1,4 +1,9 @@ -#![allow(unused_assignments, unused_mut, clippy::assign_op_pattern)] +#![allow( + unused_assignments, + unused_mut, + clippy::assign_op_pattern, + clippy::manual_checked_sub +)] #![warn(clippy::implicit_saturating_sub)] use std::cmp::PartialEq; diff --git a/tests/ui/manual_checked_sub.fixed b/tests/ui/manual_checked_sub.fixed index 48f4d6bf7423..86c381df5758 100644 --- a/tests/ui/manual_checked_sub.fixed +++ b/tests/ui/manual_checked_sub.fixed @@ -1,102 +1,141 @@ #![allow(clippy::collapsible_if)] #![warn(clippy::manual_checked_sub)] -//Positive test, lint fn positive_tests() { let a: u32 = 10; let b: u32 = 5; + let c = 3; - // Case 1: a - b inside an if a >= b - if a >= b { - let _ = a.checked_sub(b); + // Basic subtraction inside an if + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + let c = result; + } + + // Basic subtraction inside an if with an else block + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + let c = result; + } else { + let c = a + b; } - // Case 2: a - 1 inside an if a > 0 - if a > 0 { - let _ = a.checked_sub(0); + // Decrementing inside an if condition + if let Some(result) = a.checked_sub(0) { + //~^ manual_checked_sub + let _ = result; } - // Case 3: Both suffixed, should lint - if 10u32 >= 5u32 { - let _ = 10u32.checked_sub(5u32); + // Variable subtraction used in a function call + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + some_function(result); + } + + // Subtracting two suffixed literals + if let Some(result) = 10u32.checked_sub(5u32) { + //~^ manual_checked_sub + let _ = result; } let x: i32 = 10; let y: i32 = 5; - // Case 4: casted variables inside an if, should lint - if x as u32 >= y as u32 { - let _ = (x as u32).checked_sub(y as u32); + // Casted variables inside an if + if let Some(result) = (x as u32).checked_sub(y as u32) { + //~^ manual_checked_sub + let _ = result; + } + + // Casted variable and literal inside an if + if let Some(result) = (x as u32).checked_sub(5u32) { + //~^ manual_checked_sub + let _ = result; + } + + // Casting subtraction result + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + let _ = result as u64; + } + + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + macro_rules! subtract { + () => { + result + }; + } + let _ = subtract!(); + } + + struct Example { + value: u32, + } + + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + let _ = Example { value: result }; } - // Case 5: casted variable and literal inside an if, should lint - if x as u32 >= 5u32 { - let _ = (x as u32).checked_sub(5u32); + // Subtraction inside a nested if, should lint + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + if c > 0 { + let _ = result; + } } } -// Negative tests, no lint +fn some_function(a: u32) {} + fn negative_tests() { let a: u32 = 10; let b: u32 = 5; - // Case 1: Uses checked_sub() correctly, no lint + // Uses `.checked_sub()`, should not trigger let _ = a.checked_sub(b); - // Case 2: Works with signed integers (i32), no lint + // Works with signed integers (i32), should not trigger let x: i32 = 10; let y: i32 = 5; + if x >= y { let _ = x - y; } - // Case 3: If condition doesn't match, no lint + // If condition does not match subtraction variables if a == b { let _ = a - b; } - // Case 4: a - b inside an if a > b + // a - b inside an if a > b (shouldn't lint) if a > b { let _ = a - b; } - // Case 5: Unsuffixed literals, no lint + // Unsuffixed literals (shouldn't lint) if 10 >= 5 { let _ = 10 - 5; } - // Case 6: Suffixed lhs, unsuffixed rhs, no lint + // Suffixed LHS, unsuffixed RHS (shouldn't lint) if 10u32 >= 5 { let _ = 10u32 - 5; } - // Case 7: Unsuffixed lhs, suffixed rhs, no lint + // Unsuffixed LHS, suffixed RHS (shouldn't lint) if 10 >= 5u32 { let _ = 10 - 5u32; } -} - -fn edge_cases() { - let a: u32 = 10; - let b: u32 = 5; - let c = 3; - // Edge Case 1: Subtraction inside a nested if + // Using `.wrapping_sub()`, should not trigger if a >= b { - if c > 0 { - let _ = a.checked_sub(b); - } - } - - if a > b { - if a - b > c { - let _ = a - b; // This subtraction is safe because `a > b` is checked - } + let _ = a.wrapping_sub(b); } } fn main() { positive_tests(); negative_tests(); - edge_cases(); } diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs index 4107966ee758..cf05e3417927 100644 --- a/tests/ui/manual_checked_sub.rs +++ b/tests/ui/manual_checked_sub.rs @@ -1,103 +1,141 @@ #![allow(clippy::collapsible_if)] #![warn(clippy::manual_checked_sub)] -//Positive test, lint fn positive_tests() { let a: u32 = 10; let b: u32 = 5; + let c = 3; - // Case 1: a - b inside an if a >= b + // Basic subtraction inside an if if a >= b { - let _ = a - b; + //~^ manual_checked_sub + let c = a - b; + } + + // Basic subtraction inside an if with an else block + if a >= b { + //~^ manual_checked_sub + let c = a - b; + } else { + let c = a + b; } - // Case 2: a - 1 inside an if a > 0 + // Decrementing inside an if condition if a > 0 { + //~^ manual_checked_sub let _ = a - 1; } - // Case 3: Both suffixed, should lint + // Variable subtraction used in a function call + if a >= b { + //~^ manual_checked_sub + some_function(a - b); + } + + // Subtracting two suffixed literals if 10u32 >= 5u32 { + //~^ manual_checked_sub let _ = 10u32 - 5u32; } let x: i32 = 10; let y: i32 = 5; - // Case 4: casted variables inside an if, should lint + // Casted variables inside an if if x as u32 >= y as u32 { + //~^ manual_checked_sub let _ = x as u32 - y as u32; } - // Case 5: casted variable and literal inside an if, should lint + // Casted variable and literal inside an if if x as u32 >= 5u32 { + //~^ manual_checked_sub let _ = x as u32 - 5u32; } + + // Casting subtraction result + if a >= b { + //~^ manual_checked_sub + let _ = (a - b) as u64; + } + + if a >= b { + //~^ manual_checked_sub + macro_rules! subtract { + () => { + a - b + }; + } + let _ = subtract!(); + } + + struct Example { + value: u32, + } + + if a >= b { + //~^ manual_checked_sub + let _ = Example { value: a - b }; + } + + // Subtraction inside a nested if, should lint + if a >= b { + //~^ manual_checked_sub + if c > 0 { + let _ = a - b; + } + } } -// Negative tests, no lint +fn some_function(a: u32) {} + fn negative_tests() { let a: u32 = 10; let b: u32 = 5; - // Case 1: Uses checked_sub() correctly, no lint + // Uses `.checked_sub()`, should not trigger let _ = a.checked_sub(b); - // Case 2: Works with signed integers (i32), no lint + // Works with signed integers (i32), should not trigger let x: i32 = 10; let y: i32 = 5; + if x >= y { let _ = x - y; } - // Case 3: If condition doesn't match, no lint + // If condition does not match subtraction variables if a == b { let _ = a - b; } - // Case 4: a - b inside an if a > b + // a - b inside an if a > b (shouldn't lint) if a > b { let _ = a - b; } - // Case 5: Unsuffixed literals, no lint + // Unsuffixed literals (shouldn't lint) if 10 >= 5 { let _ = 10 - 5; } - // Case 6: Suffixed lhs, unsuffixed rhs, no lint + // Suffixed LHS, unsuffixed RHS (shouldn't lint) if 10u32 >= 5 { let _ = 10u32 - 5; } - // Case 7: Unsuffixed lhs, suffixed rhs, no lint + // Unsuffixed LHS, suffixed RHS (shouldn't lint) if 10 >= 5u32 { let _ = 10 - 5u32; } -} - -fn edge_cases() { - let a: u32 = 10; - let b: u32 = 5; - let c = 3; - // Edge Case 1: Subtraction inside a nested if, should lint + // Using `.wrapping_sub()`, should not trigger if a >= b { - if c > 0 { - let _ = a - b; - } - } - - // no lint - if a > b { - if a - b > c { - let _ = a - b; - } + let _ = a.wrapping_sub(b); } } fn main() { positive_tests(); negative_tests(); - edge_cases(); } diff --git a/tests/ui/manual_checked_sub.stderr b/tests/ui/manual_checked_sub.stderr index 3168a75511de..615095cfa6ca 100644 --- a/tests/ui/manual_checked_sub.stderr +++ b/tests/ui/manual_checked_sub.stderr @@ -1,41 +1,207 @@ error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:11:17 + --> tests/ui/manual_checked_sub.rs:10:5 | -LL | let _ = a - b; - | ^^^^^ help: consider using `.checked_sub()`: `a.checked_sub(b)` +LL | / if a >= b { +LL | | +LL | | let c = a - b; +LL | | } + | |_____^ | = note: `-D clippy::manual-checked-sub` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_checked_sub)]` +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + let c = result; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:16:5 + | +LL | / if a >= b { +LL | | +LL | | let c = a - b; +LL | | } else { +LL | | let c = a + b; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + let c = result; +LL + } else { +LL + let c = a + b; +LL + } + | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:16:17 + --> tests/ui/manual_checked_sub.rs:24:5 + | +LL | / if a > 0 { +LL | | +LL | | let _ = a - 1; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(0) { +LL + +LL + let _ = result; +LL + } | -LL | let _ = a - 1; - | ^^^^^ help: consider using `.checked_sub()`: `a.checked_sub(0)` error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:21:17 + --> tests/ui/manual_checked_sub.rs:30:5 + | +LL | / if a >= b { +LL | | +LL | | some_function(a - b); +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + some_function(result); +LL + } | -LL | let _ = 10u32 - 5u32; - | ^^^^^^^^^^^^ help: consider using `.checked_sub()`: `10u32.checked_sub(5u32)` error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:29:17 + --> tests/ui/manual_checked_sub.rs:36:5 + | +LL | / if 10u32 >= 5u32 { +LL | | +LL | | let _ = 10u32 - 5u32; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = 10u32.checked_sub(5u32) { +LL + +LL + let _ = result; +LL + } | -LL | let _ = x as u32 - y as u32; - | ^^^^^^^^^^^^^^^^^^^ help: consider using `.checked_sub()`: `(x as u32).checked_sub(y as u32)` error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:34:17 + --> tests/ui/manual_checked_sub.rs:45:5 + | +LL | / if x as u32 >= y as u32 { +LL | | +LL | | let _ = x as u32 - y as u32; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = (x as u32).checked_sub(y as u32) { +LL + +LL + let _ = result; +LL + } | -LL | let _ = x as u32 - 5u32; - | ^^^^^^^^^^^^^^^ help: consider using `.checked_sub()`: `(x as u32).checked_sub(5u32)` error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:87:21 + --> tests/ui/manual_checked_sub.rs:51:5 + | +LL | / if x as u32 >= 5u32 { +LL | | +LL | | let _ = x as u32 - 5u32; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = (x as u32).checked_sub(5u32) { +LL + +LL + let _ = result; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:57:5 + | +LL | / if a >= b { +LL | | +LL | | let _ = (a - b) as u64; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + let _ = result as u64; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:62:5 + | +LL | / if a >= b { +LL | | +LL | | macro_rules! subtract { +LL | | () => { +... | +LL | | let _ = subtract!(); +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + macro_rules! subtract { +LL + () => { +LL + result +LL + }; +LL + } +LL + let _ = subtract!(); +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:76:5 + | +LL | / if a >= b { +LL | | +LL | | let _ = Example { value: a - b }; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + let _ = Example { value: result }; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:82:5 + | +LL | / if a >= b { +LL | | +LL | | if c > 0 { +LL | | let _ = a - b; +LL | | } +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + if c > 0 { +LL + let _ = result; +LL + } +LL + } | -LL | let _ = a - b; - | ^^^^^ help: consider using `.checked_sub()`: `a.checked_sub(b)` -error: aborting due to 6 previous errors +error: aborting due to 11 previous errors From 392c3b4297cdc73ceecbe573c9ba16225e9e7471 Mon Sep 17 00:00:00 2001 From: benacq Date: Sat, 1 Mar 2025 23:19:47 +0000 Subject: [PATCH 03/26] Lint test case false positives corrected --- clippy_lints/src/manual_checked_sub.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index ccb15259f1b6..c25551683ef9 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -53,7 +53,7 @@ impl_lint_pass!(ManualCheckedSub => [MANUAL_CHECKED_SUB]); impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if !self.msrv.meets(msrvs::MANUAL_CHECKED_SUB) { + if !self.msrv.meets(cx, msrvs::MANUAL_CHECKED_SUB) { return; } From f7c2e5a16aa5a7076a4f6e81168f49c5a414eebd Mon Sep 17 00:00:00 2001 From: benacq Date: Sat, 1 Mar 2025 23:30:37 +0000 Subject: [PATCH 04/26] Lint test case false positives corrected --- tests/ui/implicit_saturating_sub.fixed | 7 +++- tests/ui/implicit_saturating_sub.stderr | 54 ++++++++++++------------- 2 files changed, 33 insertions(+), 28 deletions(-) diff --git a/tests/ui/implicit_saturating_sub.fixed b/tests/ui/implicit_saturating_sub.fixed index 1aab6c54407e..aa62b7ffa4c0 100644 --- a/tests/ui/implicit_saturating_sub.fixed +++ b/tests/ui/implicit_saturating_sub.fixed @@ -1,4 +1,9 @@ -#![allow(unused_assignments, unused_mut, clippy::assign_op_pattern)] +#![allow( + unused_assignments, + unused_mut, + clippy::assign_op_pattern, + clippy::manual_checked_sub +)] #![warn(clippy::implicit_saturating_sub)] use std::cmp::PartialEq; diff --git a/tests/ui/implicit_saturating_sub.stderr b/tests/ui/implicit_saturating_sub.stderr index 0c225856fd07..a8d433799efc 100644 --- a/tests/ui/implicit_saturating_sub.stderr +++ b/tests/ui/implicit_saturating_sub.stderr @@ -1,5 +1,5 @@ error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:27:5 + --> tests/ui/implicit_saturating_sub.rs:32:5 | LL | / if u_8 > 0 { LL | | @@ -11,7 +11,7 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::implicit_saturating_sub)]` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:35:13 + --> tests/ui/implicit_saturating_sub.rs:40:13 | LL | / if u_8 > 0 { LL | | @@ -20,7 +20,7 @@ LL | | } | |_____________^ help: try: `u_8 = u_8.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:50:5 + --> tests/ui/implicit_saturating_sub.rs:55:5 | LL | / if u_16 > 0 { LL | | @@ -29,7 +29,7 @@ LL | | } | |_____^ help: try: `u_16 = u_16.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:61:5 + --> tests/ui/implicit_saturating_sub.rs:66:5 | LL | / if u_32 != 0 { LL | | @@ -38,7 +38,7 @@ LL | | } | |_____^ help: try: `u_32 = u_32.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:83:5 + --> tests/ui/implicit_saturating_sub.rs:88:5 | LL | / if u_64 > 0 { LL | | @@ -47,7 +47,7 @@ LL | | } | |_____^ help: try: `u_64 = u_64.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:89:5 + --> tests/ui/implicit_saturating_sub.rs:94:5 | LL | / if 0 < u_64 { LL | | @@ -56,7 +56,7 @@ LL | | } | |_____^ help: try: `u_64 = u_64.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:95:5 + --> tests/ui/implicit_saturating_sub.rs:100:5 | LL | / if 0 != u_64 { LL | | @@ -65,7 +65,7 @@ LL | | } | |_____^ help: try: `u_64 = u_64.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:117:5 + --> tests/ui/implicit_saturating_sub.rs:122:5 | LL | / if u_usize > 0 { LL | | @@ -74,7 +74,7 @@ LL | | } | |_____^ help: try: `u_usize = u_usize.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:130:5 + --> tests/ui/implicit_saturating_sub.rs:135:5 | LL | / if i_8 > i8::MIN { LL | | @@ -83,7 +83,7 @@ LL | | } | |_____^ help: try: `i_8 = i_8.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:136:5 + --> tests/ui/implicit_saturating_sub.rs:141:5 | LL | / if i_8 > i8::MIN { LL | | @@ -92,7 +92,7 @@ LL | | } | |_____^ help: try: `i_8 = i_8.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:142:5 + --> tests/ui/implicit_saturating_sub.rs:147:5 | LL | / if i_8 != i8::MIN { LL | | @@ -101,7 +101,7 @@ LL | | } | |_____^ help: try: `i_8 = i_8.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:148:5 + --> tests/ui/implicit_saturating_sub.rs:153:5 | LL | / if i_8 != i8::MIN { LL | | @@ -110,7 +110,7 @@ LL | | } | |_____^ help: try: `i_8 = i_8.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:159:5 + --> tests/ui/implicit_saturating_sub.rs:164:5 | LL | / if i_16 > i16::MIN { LL | | @@ -119,7 +119,7 @@ LL | | } | |_____^ help: try: `i_16 = i_16.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:165:5 + --> tests/ui/implicit_saturating_sub.rs:170:5 | LL | / if i_16 > i16::MIN { LL | | @@ -128,7 +128,7 @@ LL | | } | |_____^ help: try: `i_16 = i_16.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:171:5 + --> tests/ui/implicit_saturating_sub.rs:176:5 | LL | / if i_16 != i16::MIN { LL | | @@ -137,7 +137,7 @@ LL | | } | |_____^ help: try: `i_16 = i_16.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:177:5 + --> tests/ui/implicit_saturating_sub.rs:182:5 | LL | / if i_16 != i16::MIN { LL | | @@ -146,7 +146,7 @@ LL | | } | |_____^ help: try: `i_16 = i_16.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:188:5 + --> tests/ui/implicit_saturating_sub.rs:193:5 | LL | / if i_32 > i32::MIN { LL | | @@ -155,7 +155,7 @@ LL | | } | |_____^ help: try: `i_32 = i_32.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:194:5 + --> tests/ui/implicit_saturating_sub.rs:199:5 | LL | / if i_32 > i32::MIN { LL | | @@ -164,7 +164,7 @@ LL | | } | |_____^ help: try: `i_32 = i_32.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:200:5 + --> tests/ui/implicit_saturating_sub.rs:205:5 | LL | / if i_32 != i32::MIN { LL | | @@ -173,7 +173,7 @@ LL | | } | |_____^ help: try: `i_32 = i_32.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:206:5 + --> tests/ui/implicit_saturating_sub.rs:211:5 | LL | / if i_32 != i32::MIN { LL | | @@ -182,7 +182,7 @@ LL | | } | |_____^ help: try: `i_32 = i_32.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:217:5 + --> tests/ui/implicit_saturating_sub.rs:222:5 | LL | / if i64::MIN < i_64 { LL | | @@ -191,7 +191,7 @@ LL | | } | |_____^ help: try: `i_64 = i_64.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:223:5 + --> tests/ui/implicit_saturating_sub.rs:228:5 | LL | / if i64::MIN != i_64 { LL | | @@ -200,7 +200,7 @@ LL | | } | |_____^ help: try: `i_64 = i_64.saturating_sub(1);` error: implicitly performing saturating subtraction - --> tests/ui/implicit_saturating_sub.rs:229:5 + --> tests/ui/implicit_saturating_sub.rs:234:5 | LL | / if i64::MIN < i_64 { LL | | @@ -209,7 +209,7 @@ LL | | } | |_____^ help: try: `i_64 = i_64.saturating_sub(1);` error: manual arithmetic check found - --> tests/ui/implicit_saturating_sub.rs:298:12 + --> tests/ui/implicit_saturating_sub.rs:303:12 | LL | } else if a >= b { | ____________^ @@ -221,19 +221,19 @@ LL | | } | |_____^ help: replace it with: `{ b.saturating_sub(a) }` error: manual arithmetic check found - --> tests/ui/implicit_saturating_sub.rs:314:13 + --> tests/ui/implicit_saturating_sub.rs:319:13 | LL | let _ = if a * 2 > b { a * 2 - b } else { 0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `(a * 2).saturating_sub(b)` error: manual arithmetic check found - --> tests/ui/implicit_saturating_sub.rs:317:13 + --> tests/ui/implicit_saturating_sub.rs:322:13 | LL | let _ = if a > b * 2 { a - b * 2 } else { 0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b * 2)` error: manual arithmetic check found - --> tests/ui/implicit_saturating_sub.rs:320:13 + --> tests/ui/implicit_saturating_sub.rs:325:13 | LL | let _ = if a < b * 2 { 0 } else { a - b * 2 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b * 2)` From 60b5c16ffb595f6896603b284b9ed6dacdfd05ce Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 2 Mar 2025 00:28:04 +0000 Subject: [PATCH 05/26] Lint test case false positives corrected --- clippy_lints/src/manual_checked_sub.rs | 74 ++++++++++---------------- 1 file changed, 29 insertions(+), 45 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index c25551683ef9..a7442dc73fa3 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -43,9 +43,7 @@ pub struct ManualCheckedSub { impl ManualCheckedSub { #[must_use] pub fn new(conf: &'static Conf) -> Self { - Self { - msrv: conf.msrv.clone(), - } + Self { msrv: conf.msrv } } } @@ -62,20 +60,20 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { if let ExprKind::If(drop_temp_expr, body_block, else_block) = expr.kind && let ExprKind::DropTemps(condition_expr) = drop_temp_expr.kind && let ExprKind::Binary(op, lhs, rhs) = condition_expr.kind - && is_unsigned_int(cx, &lhs) - && is_unsigned_int(cx, &rhs) + && is_unsigned_int(cx, lhs) + && is_unsigned_int(cx, rhs) { if let BinOpKind::Ge = op.node { SubExprVisitor { cx, - applicability, if_expr: expr, if_body_block: body_block, else_block, - condition_lhs: &lhs, - condition_rhs: &rhs, + applicability, + condition_lhs: lhs, + condition_rhs: rhs, } - .visit_expr(body_block) + .visit_expr(body_block); } if let BinOpKind::Gt = op.node { @@ -84,14 +82,14 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { { SubExprVisitor { cx, - applicability, if_expr: expr, if_body_block: body_block, else_block, - condition_lhs: &lhs, - condition_rhs: &rhs, + applicability, + condition_lhs: lhs, + condition_rhs: rhs, } - .visit_expr(body_block) + .visit_expr(body_block); } } } @@ -115,18 +113,16 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { { if let ExprKind::Lit(lit) = self.condition_rhs.kind && let LitKind::Int(Pu128(0), _) = lit.node - && (is_referencing_same_variable(&sub_lhs, self.condition_lhs) - || are_literals_equal(&sub_lhs, self.condition_lhs)) + && (is_referencing_same_variable(sub_lhs, self.condition_lhs) + || are_literals_equal(sub_lhs, self.condition_lhs)) + { + self.build_suggession(expr); + } else if (is_referencing_same_variable(sub_lhs, self.condition_lhs) + || are_literals_equal(sub_lhs, self.condition_lhs)) + && (is_referencing_same_variable(sub_rhs, self.condition_rhs) + || are_literals_equal(sub_rhs, self.condition_rhs)) { self.build_suggession(expr); - } else { - if (is_referencing_same_variable(&sub_lhs, self.condition_lhs) - || are_literals_equal(&sub_lhs, self.condition_lhs)) - && (is_referencing_same_variable(&sub_rhs, self.condition_rhs) - || are_literals_equal(&sub_rhs, self.condition_rhs)) - { - self.build_suggession(expr); - } } } @@ -134,12 +130,8 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { } } -impl<'a, 'tcx> SubExprVisitor<'a, 'tcx> { +impl<'tcx> SubExprVisitor<'_, 'tcx> { fn build_suggession(&mut self, expr: &Expr<'tcx>) { - // if let Some(else_expr) = self.else_block { - // println!("ELSE BLOCK in suggestion:::: {:#?}", else_expr); - // } - let body_snippet = snippet_with_applicability(self.cx, self.if_body_block.span, "..", &mut self.applicability); let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut self.applicability); @@ -153,20 +145,14 @@ impl<'a, 'tcx> SubExprVisitor<'a, 'tcx> { let lhs_needs_parens = matches!(self.condition_lhs.kind, ExprKind::Cast(..)); let mut suggestion = if lhs_needs_parens { - format!( - "if let Some(result) = ({}).checked_sub({}) {}", - lhs_snippet, rhs_snippet, updated_usage_context_snippet - ) + format!("if let Some(result) = ({lhs_snippet}).checked_sub({rhs_snippet}) {updated_usage_context_snippet}") } else { - format!( - "if let Some(result) = {}.checked_sub({}) {}", - lhs_snippet, rhs_snippet, updated_usage_context_snippet - ) + format!("if let Some(result) = {lhs_snippet}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}") }; if let Some(else_expr) = self.else_block { let else_snippet = snippet_with_applicability(self.cx, else_expr.span, "..", &mut self.applicability); - suggestion.push_str(&format!(" else {}", else_snippet)); + suggestion.push_str(&format!(" else {else_snippet}")); } span_lint_and_sugg( @@ -181,17 +167,15 @@ impl<'a, 'tcx> SubExprVisitor<'a, 'tcx> { } } -fn is_unsigned_int<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> bool { +fn is_unsigned_int<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool { let expr_type = cx.typeck_results().expr_ty(expr).peel_refs(); if matches!(expr_type.kind(), ty::Uint(_)) { return true; } if let ExprKind::Lit(lit) = &expr.kind { - if let LitKind::Int(_, suffix) = &lit.node { - if let LitIntType::Unsuffixed = suffix { - return true; - } + if let LitKind::Int(_, LitIntType::Unsuffixed) = &lit.node { + return true; } } false @@ -218,7 +202,7 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<' } else { if let Some(expr2_path) = get_resolved_path_id(expr2) { return cast_expr1_path == expr2_path; - }; + } return false; } } @@ -226,7 +210,7 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<' if let Some(cast_expr2_path) = get_resolved_path_id(cast_expr2) { if let Some(expr1_path) = get_resolved_path_id(expr1) { return expr1_path == cast_expr2_path; - }; + } return false; } } @@ -241,7 +225,7 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<' } } -fn get_resolved_path_id<'tcx>(expr: &'tcx Expr<'_>) -> Option { +fn get_resolved_path_id(expr: &Expr<'_>) -> Option { if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { path.segments.first().and_then(|segment| Some(segment.res)) } else { From bdf9ac8d32e8a364896420172a608bd131bb617a Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 2 Mar 2025 08:46:27 +0000 Subject: [PATCH 06/26] Lint test case false positives corrected --- clippy_lints/src/dereference.rs | 4 +- clippy_lints/src/manual_checked_sub.rs | 7 +++- .../map_with_unused_argument_over_ranges.rs | 6 +-- tests/ui/manual_checked_sub.fixed | 8 ++++ tests/ui/manual_checked_sub.rs | 8 ++++ tests/ui/manual_checked_sub.stderr | 39 ++++++++++++++----- 6 files changed, 56 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 7da5a530eaa3..507c2da7a83d 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -433,12 +433,12 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { (2, deref_msg) }; - if deref_count >= required_refs { + if let Some(result) = deref_count.checked_sub(required_refs) { self.state = Some(( State::DerefedBorrow(DerefedBorrow { // One of the required refs is for the current borrow expression, the remaining ones // can't be removed without breaking the code. See earlier comment. - count: deref_count - required_refs, + count: result, msg, stability, for_field_access: if let ExprUseNode::FieldAccess(name) = use_node diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index a7442dc73fa3..a6c2e6c5c740 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -11,6 +11,7 @@ use rustc_hir::{Expr, ExprKind, QPath}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self}; use rustc_session::impl_lint_pass; +use std::fmt::Write; declare_clippy_lint! { /// ### What it does @@ -152,7 +153,8 @@ impl<'tcx> SubExprVisitor<'_, 'tcx> { if let Some(else_expr) = self.else_block { let else_snippet = snippet_with_applicability(self.cx, else_expr.span, "..", &mut self.applicability); - suggestion.push_str(&format!(" else {else_snippet}")); + // suggestion.push_str(&format!(" else {else_snippet}")); + write!(suggestion, " else {else_snippet}").unwrap(); } span_lint_and_sugg( @@ -227,7 +229,8 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<' fn get_resolved_path_id(expr: &Expr<'_>) -> Option { if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { - path.segments.first().and_then(|segment| Some(segment.res)) + // path.segments.first().and_then(|segment| Some(segment.res)) + path.segments.first().map(|segment| segment.res) } else { None } diff --git a/clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs b/clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs index a2a522a60687..13715b823aa6 100644 --- a/clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs +++ b/clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs @@ -30,10 +30,10 @@ fn extract_count_with_applicability( && let LitKind::Int(Pu128(upper_bound), _) = lit.node { // Here we can explicitly calculate the number of iterations - let count = if upper_bound >= lower_bound { + let count = if let Some(result) = upper_bound.checked_sub(lower_bound) { match range.limits { - RangeLimits::HalfOpen => upper_bound - lower_bound, - RangeLimits::Closed => (upper_bound - lower_bound).checked_add(1)?, + RangeLimits::HalfOpen => result, + RangeLimits::Closed => (result).checked_add(1)?, } } else { 0 diff --git a/tests/ui/manual_checked_sub.fixed b/tests/ui/manual_checked_sub.fixed index 86c381df5758..2dcaf23e44b8 100644 --- a/tests/ui/manual_checked_sub.fixed +++ b/tests/ui/manual_checked_sub.fixed @@ -20,6 +20,14 @@ fn positive_tests() { let c = a + b; } + // Basic subtraction inside an if with an else-if block + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + let c = result; + } else if a < b { + let c = a + b; + } + // Decrementing inside an if condition if let Some(result) = a.checked_sub(0) { //~^ manual_checked_sub diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs index cf05e3417927..36c9948e289a 100644 --- a/tests/ui/manual_checked_sub.rs +++ b/tests/ui/manual_checked_sub.rs @@ -20,6 +20,14 @@ fn positive_tests() { let c = a + b; } + // Basic subtraction inside an if with an else-if block + if a >= b { + //~^ manual_checked_sub + let c = a - b; + } else if a < b { + let c = a + b; + } + // Decrementing inside an if condition if a > 0 { //~^ manual_checked_sub diff --git a/tests/ui/manual_checked_sub.stderr b/tests/ui/manual_checked_sub.stderr index 615095cfa6ca..5847d1e2d3d4 100644 --- a/tests/ui/manual_checked_sub.stderr +++ b/tests/ui/manual_checked_sub.stderr @@ -41,6 +41,27 @@ LL + } error: manual re-implementation of checked subtraction --> tests/ui/manual_checked_sub.rs:24:5 | +LL | / if a >= b { +LL | | +LL | | let c = a - b; +LL | | } else if a < b { +LL | | let c = a + b; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + let c = result; +LL + } else if a < b { +LL + let c = a + b; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:32:5 + | LL | / if a > 0 { LL | | LL | | let _ = a - 1; @@ -56,7 +77,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:30:5 + --> tests/ui/manual_checked_sub.rs:38:5 | LL | / if a >= b { LL | | @@ -73,7 +94,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:36:5 + --> tests/ui/manual_checked_sub.rs:44:5 | LL | / if 10u32 >= 5u32 { LL | | @@ -90,7 +111,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:45:5 + --> tests/ui/manual_checked_sub.rs:53:5 | LL | / if x as u32 >= y as u32 { LL | | @@ -107,7 +128,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:51:5 + --> tests/ui/manual_checked_sub.rs:59:5 | LL | / if x as u32 >= 5u32 { LL | | @@ -124,7 +145,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:57:5 + --> tests/ui/manual_checked_sub.rs:65:5 | LL | / if a >= b { LL | | @@ -141,7 +162,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:62:5 + --> tests/ui/manual_checked_sub.rs:70:5 | LL | / if a >= b { LL | | @@ -166,7 +187,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:76:5 + --> tests/ui/manual_checked_sub.rs:84:5 | LL | / if a >= b { LL | | @@ -183,7 +204,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:82:5 + --> tests/ui/manual_checked_sub.rs:90:5 | LL | / if a >= b { LL | | @@ -203,5 +224,5 @@ LL + } LL + } | -error: aborting due to 11 previous errors +error: aborting due to 12 previous errors From 2e85d1a9551fd7253a3b32f0d0b9077edf0dc817 Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 2 Mar 2025 08:58:38 +0000 Subject: [PATCH 07/26] Lint test case false positives corrected --- clippy_lints/src/manual_checked_sub.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index a6c2e6c5c740..7fa1a76e18b0 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -23,13 +23,13 @@ declare_clippy_lint! { /// /// ### Example /// ```no_run - /// if a >= b { - /// a - b + /// if 10u32 >= 4u32 { + /// 10u32 - 4u32 /// } /// ``` /// Use instead: /// ```no_run - /// a.checked_sub(b) + /// if let Some(result) = 10u32.checked_sub(4u32); /// ``` #[clippy::version = "1.86.0"] pub MANUAL_CHECKED_SUB, From e0b131a038f3a55467c2fcff95d4a4197d572cde Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 2 Mar 2025 09:05:46 +0000 Subject: [PATCH 08/26] Lint test case false positives corrected --- clippy_lints/src/manual_checked_sub.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 7fa1a76e18b0..685f35f83006 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -24,7 +24,7 @@ declare_clippy_lint! { /// ### Example /// ```no_run /// if 10u32 >= 4u32 { - /// 10u32 - 4u32 + /// 10u32 - 4u32; /// } /// ``` /// Use instead: From 797075a811e0521adfa9e5392117116f99fd577b Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 2 Mar 2025 09:14:55 +0000 Subject: [PATCH 09/26] Lint test case false positives corrected --- clippy_lints/src/manual_checked_sub.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 685f35f83006..8b9a485d0d1c 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -24,12 +24,14 @@ declare_clippy_lint! { /// ### Example /// ```no_run /// if 10u32 >= 4u32 { - /// 10u32 - 4u32; + /// let c = 10u32 - 4u32; /// } /// ``` /// Use instead: /// ```no_run - /// if let Some(result) = 10u32.checked_sub(4u32); + /// if let Some(result) = 10u32.checked_sub(4u32){ + /// c = result; + /// }; /// ``` #[clippy::version = "1.86.0"] pub MANUAL_CHECKED_SUB, From 2669fc936f311c2dde923515b3c9baf07a030112 Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 2 Mar 2025 09:23:33 +0000 Subject: [PATCH 10/26] Lint test case false positives corrected --- clippy_lints/src/manual_checked_sub.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 8b9a485d0d1c..b13ee0857f4d 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -30,7 +30,7 @@ declare_clippy_lint! { /// Use instead: /// ```no_run /// if let Some(result) = 10u32.checked_sub(4u32){ - /// c = result; + /// result; /// }; /// ``` #[clippy::version = "1.86.0"] From 06383da1825d540b35e86bdb0c49ec709e76f3d3 Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 2 Mar 2025 09:42:34 +0000 Subject: [PATCH 11/26] Lint test case false positives corrected --- tests/ui/manual_checked_sub.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs index 36c9948e289a..f9ddaa45cbf9 100644 --- a/tests/ui/manual_checked_sub.rs +++ b/tests/ui/manual_checked_sub.rs @@ -136,11 +136,6 @@ fn negative_tests() { if 10 >= 5u32 { let _ = 10 - 5u32; } - - // Using `.wrapping_sub()`, should not trigger - if a >= b { - let _ = a.wrapping_sub(b); - } } fn main() { From 6b034a7b93b1441a716d8126b8a1136b22153a60 Mon Sep 17 00:00:00 2001 From: benacq Date: Sun, 2 Mar 2025 11:45:05 +0000 Subject: [PATCH 12/26] Lint test case false positives corrected --- tests/ui/manual_checked_sub.fixed | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/ui/manual_checked_sub.fixed b/tests/ui/manual_checked_sub.fixed index 2dcaf23e44b8..5318005f4762 100644 --- a/tests/ui/manual_checked_sub.fixed +++ b/tests/ui/manual_checked_sub.fixed @@ -136,11 +136,6 @@ fn negative_tests() { if 10 >= 5u32 { let _ = 10 - 5u32; } - - // Using `.wrapping_sub()`, should not trigger - if a >= b { - let _ = a.wrapping_sub(b); - } } fn main() { From 0e66fd9c0278e1313fd23bc483a862190c7f9d83 Mon Sep 17 00:00:00 2001 From: benacq Date: Fri, 28 Mar 2025 17:41:16 +0000 Subject: [PATCH 13/26] In progress --- clippy_lints/src/manual_checked_sub.rs | 160 +++++++++---------------- clippy_utils/src/msrvs.rs | 2 +- tests/ui/manual_checked_sub.fixed | 75 ++++-------- tests/ui/manual_checked_sub.rs | 43 ++----- tests/ui/manual_checked_sub.stderr | 108 ++++------------- 5 files changed, 110 insertions(+), 278 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index b13ee0857f4d..7ce3614cfeab 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -1,13 +1,14 @@ use clippy_config::Conf; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::path_to_local; use clippy_utils::source::snippet_with_applicability; +use clippy_utils::sugg::Sugg; use rustc_ast::{BinOpKind, LitIntType, LitKind}; use rustc_data_structures::packed::Pu128; use rustc_errors::Applicability; -use rustc_hir::def::Res; use rustc_hir::intravisit::{Visitor, walk_expr}; -use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self}; use rustc_session::impl_lint_pass; @@ -54,45 +55,27 @@ impl_lint_pass!(ManualCheckedSub => [MANUAL_CHECKED_SUB]); impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if !self.msrv.meets(cx, msrvs::MANUAL_CHECKED_SUB) { + if !self.msrv.meets(cx, msrvs::CHECKED_SUB) { return; } - let applicability = Applicability::MachineApplicable; - - if let ExprKind::If(drop_temp_expr, body_block, else_block) = expr.kind - && let ExprKind::DropTemps(condition_expr) = drop_temp_expr.kind - && let ExprKind::Binary(op, lhs, rhs) = condition_expr.kind - && is_unsigned_int(cx, lhs) - && is_unsigned_int(cx, rhs) - { - if let BinOpKind::Ge = op.node { - SubExprVisitor { - cx, - if_expr: expr, - if_body_block: body_block, - else_block, - applicability, - condition_lhs: lhs, - condition_rhs: rhs, - } - .visit_expr(body_block); - } - - if let BinOpKind::Gt = op.node { - if let ExprKind::Lit(lit) = &rhs.kind - && let LitKind::Int(Pu128(0), _) = lit.node - { + if let Some(if_expr) = clippy_utils::higher::If::hir(expr) { + if let ExprKind::Binary(op, lhs, rhs) = if_expr.cond.kind + && !(matches!(lhs.kind, ExprKind::Lit(_)) && matches!(rhs.kind, ExprKind::Lit(_))) + && is_unsigned_int(cx, lhs) + && is_unsigned_int(cx, rhs) + { + if let BinOpKind::Ge | BinOpKind::Gt = op.node { SubExprVisitor { cx, if_expr: expr, - if_body_block: body_block, - else_block, - applicability, + if_body_block: if_expr.then, + else_block: if_expr.r#else, condition_lhs: lhs, condition_rhs: rhs, + condition_op: op.node, } - .visit_expr(body_block); + .visit_expr(if_expr.then); } } } @@ -104,9 +87,9 @@ struct SubExprVisitor<'a, 'tcx> { if_expr: &'tcx Expr<'tcx>, if_body_block: &'tcx Expr<'tcx>, else_block: Option<&'tcx Expr<'tcx>>, - applicability: Applicability, condition_lhs: &'tcx Expr<'tcx>, condition_rhs: &'tcx Expr<'tcx>, + condition_op: BinOpKind, } impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { @@ -115,17 +98,18 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { && let BinOpKind::Sub = op.node { if let ExprKind::Lit(lit) = self.condition_rhs.kind + && self.condition_op == BinOpKind::Gt && let LitKind::Int(Pu128(0), _) = lit.node + && (is_referencing_same_variable(sub_lhs, self.condition_lhs)) + { + self.emit_lint(expr, Some(sub_rhs)); + } else if self.condition_op == BinOpKind::Ge && (is_referencing_same_variable(sub_lhs, self.condition_lhs) || are_literals_equal(sub_lhs, self.condition_lhs)) - { - self.build_suggession(expr); - } else if (is_referencing_same_variable(sub_lhs, self.condition_lhs) - || are_literals_equal(sub_lhs, self.condition_lhs)) && (is_referencing_same_variable(sub_rhs, self.condition_rhs) || are_literals_equal(sub_rhs, self.condition_rhs)) { - self.build_suggession(expr); + self.emit_lint(expr, None); } } @@ -134,28 +118,35 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { } impl<'tcx> SubExprVisitor<'_, 'tcx> { - fn build_suggession(&mut self, expr: &Expr<'tcx>) { - let body_snippet = snippet_with_applicability(self.cx, self.if_body_block.span, "..", &mut self.applicability); + // fn is_name_in_scope(&self, name: &str, id: HirId) -> bool { + // let def_id = self.cx.tcx.hir().local_def_id(id); + // let resolver = self.cx.tcx.resolver_for_lowering(); + + // // Check if the name is already defined in the current scope + // resolver.is_name_defined(name) + // } - let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut self.applicability); - let updated_usage_context_snippet = body_snippet.as_ref().replace(binary_expr_snippet.as_ref(), "result"); - // let else_snippet = snippet_with_applicability(self.cx, self.if_else_block.span, "..", &mut - // self.applicability); + fn emit_lint(&mut self, expr: &Expr<'tcx>, sub_rhs_expr: Option<&'tcx Expr<'tcx>>) { + let mut applicability = Applicability::MachineApplicable; - let lhs_snippet = snippet_with_applicability(self.cx, self.condition_lhs.span, "..", &mut self.applicability); - let rhs_snippet = snippet_with_applicability(self.cx, self.condition_rhs.span, "..", &mut self.applicability); + let body_snippet = snippet_with_applicability(self.cx, self.if_body_block.span, "..", &mut applicability); - let lhs_needs_parens = matches!(self.condition_lhs.kind, ExprKind::Cast(..)); + let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut applicability); + let updated_usage_context_snippet = body_snippet.as_ref().replace(binary_expr_snippet.as_ref(), "diff"); - let mut suggestion = if lhs_needs_parens { - format!("if let Some(result) = ({lhs_snippet}).checked_sub({rhs_snippet}) {updated_usage_context_snippet}") - } else { - format!("if let Some(result) = {lhs_snippet}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}") + let rhs_snippet = match sub_rhs_expr { + Some(sub_rhs) => Sugg::hir(self.cx, sub_rhs, "..").maybe_par(), + None => Sugg::hir(self.cx, self.condition_rhs, "..").maybe_par(), }; + // TODO: Check diff variable is not already in scope + let mut suggestion = format!( + "if let Some(diff) = {}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}", + Sugg::hir(self.cx, self.condition_lhs, "..").maybe_par() + ); + if let Some(else_expr) = self.else_block { - let else_snippet = snippet_with_applicability(self.cx, else_expr.span, "..", &mut self.applicability); - // suggestion.push_str(&format!(" else {else_snippet}")); + let else_snippet = snippet_with_applicability(self.cx, else_expr.span, "..", &mut applicability); write!(suggestion, " else {else_snippet}").unwrap(); } @@ -166,7 +157,7 @@ impl<'tcx> SubExprVisitor<'_, 'tcx> { "manual re-implementation of checked subtraction", "consider using `.checked_sub()`", suggestion, - self.applicability, + applicability, ); } } @@ -177,63 +168,26 @@ fn is_unsigned_int<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool { return true; } - if let ExprKind::Lit(lit) = &expr.kind { - if let LitKind::Int(_, LitIntType::Unsuffixed) = &lit.node { - return true; - } - } false } fn are_literals_equal<'tcx>(expr1: &Expr<'tcx>, expr2: &Expr<'tcx>) -> bool { - if let (ExprKind::Lit(lit1), ExprKind::Lit(lit2)) = (&expr1.kind, &expr2.kind) { - if let (LitKind::Int(val1, suffix1), LitKind::Int(val2, suffix2)) = (&lit1.node, &lit2.node) { - return val1 == val2 - && suffix1 == suffix2 - && *suffix1 != LitIntType::Unsuffixed - && *suffix2 != LitIntType::Unsuffixed; - } + if let (ExprKind::Lit(lit1), ExprKind::Lit(lit2)) = (&expr1.kind, &expr2.kind) + && let (LitKind::Int(val1, suffix1), LitKind::Int(val2, suffix2)) = (&lit1.node, &lit2.node) + { + return val1 == val2 + && suffix1 == suffix2 + && *suffix1 != LitIntType::Unsuffixed + && *suffix2 != LitIntType::Unsuffixed; } + false } -fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<'_>) -> bool { - if let ExprKind::Cast(cast_expr1, _) = &expr1.kind { - if let Some(cast_expr1_path) = get_resolved_path_id(cast_expr1) { - if let ExprKind::Cast(cast_expr2, _) = &expr2.kind { - if let Some(cast_expr2_path) = get_resolved_path_id(cast_expr2) { - return cast_expr1_path == cast_expr2_path; - } - } else { - if let Some(expr2_path) = get_resolved_path_id(expr2) { - return cast_expr1_path == expr2_path; - } - return false; - } - } - } else if let ExprKind::Cast(cast_expr2, _) = &expr2.kind { - if let Some(cast_expr2_path) = get_resolved_path_id(cast_expr2) { - if let Some(expr1_path) = get_resolved_path_id(expr1) { - return expr1_path == cast_expr2_path; - } - return false; - } - } - let expr1_path = get_resolved_path_id(expr1); - let expr2_path = get_resolved_path_id(expr2); - - if let (Some(expr1_path), Some(expr2_path)) = (expr1_path, expr2_path) { - expr1_path == expr2_path - } else { - false +fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<'_>) -> bool { + if let (Some(id1), Some(id2)) = (path_to_local(expr1), path_to_local(expr2)) { + return id1 == id2; } -} -fn get_resolved_path_id(expr: &Expr<'_>) -> Option { - if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { - // path.segments.first().and_then(|segment| Some(segment.res)) - path.segments.first().map(|segment| segment.res) - } else { - None - } + false } diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 1d7177dbb7f9..b444e2185838 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -51,7 +51,7 @@ msrv_aliases! { 1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST } 1,51,0 { BORROW_AS_PTR, SEEK_FROM_CURRENT, UNSIGNED_ABS } 1,50,0 { BOOL_THEN, CLAMP, SLICE_FILL } - 1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, MANUAL_CHECKED_SUB } + 1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, CHECKED_SUB } 1,46,0 { CONST_IF_MATCH } 1,45,0 { STR_STRIP_PREFIX } 1,43,0 { LOG2_10, LOG10_2, NUMERIC_ASSOCIATED_CONSTANTS } diff --git a/tests/ui/manual_checked_sub.fixed b/tests/ui/manual_checked_sub.fixed index 5318005f4762..49cb3a477c8e 100644 --- a/tests/ui/manual_checked_sub.fixed +++ b/tests/ui/manual_checked_sub.fixed @@ -7,71 +7,44 @@ fn positive_tests() { let c = 3; // Basic subtraction inside an if - if let Some(result) = a.checked_sub(b) { + if let Some(diff) = a.checked_sub(b) { //~^ manual_checked_sub - let c = result; + let c = diff; } // Basic subtraction inside an if with an else block - if let Some(result) = a.checked_sub(b) { + if let Some(diff) = a.checked_sub(b) { //~^ manual_checked_sub - let c = result; + let c = diff; } else { let c = a + b; } // Basic subtraction inside an if with an else-if block - if let Some(result) = a.checked_sub(b) { + if let Some(diff) = a.checked_sub(b) { //~^ manual_checked_sub - let c = result; + let c = diff; } else if a < b { let c = a + b; } // Decrementing inside an if condition - if let Some(result) = a.checked_sub(0) { + if let Some(diff) = a.checked_sub(1) { //~^ manual_checked_sub - let _ = result; + let _ = diff; } // Variable subtraction used in a function call - if let Some(result) = a.checked_sub(b) { + if let Some(diff) = a.checked_sub(b) { //~^ manual_checked_sub - some_function(result); + some_function(diff); } - // Subtracting two suffixed literals - if let Some(result) = 10u32.checked_sub(5u32) { - //~^ manual_checked_sub - let _ = result; - } - - let x: i32 = 10; - let y: i32 = 5; - - // Casted variables inside an if - if let Some(result) = (x as u32).checked_sub(y as u32) { - //~^ manual_checked_sub - let _ = result; - } - - // Casted variable and literal inside an if - if let Some(result) = (x as u32).checked_sub(5u32) { - //~^ manual_checked_sub - let _ = result; - } - - // Casting subtraction result - if let Some(result) = a.checked_sub(b) { - //~^ manual_checked_sub - let _ = result as u64; - } - - if let Some(result) = a.checked_sub(b) { + if let Some(diff) = a.checked_sub(b) { //~^ manual_checked_sub macro_rules! subtract { () => { - result + diff }; } let _ = subtract!(); @@ -81,16 +54,16 @@ fn positive_tests() { value: u32, } - if let Some(result) = a.checked_sub(b) { + if let Some(diff) = a.checked_sub(b) { //~^ manual_checked_sub - let _ = Example { value: result }; + let _ = Example { value: diff }; } // Subtraction inside a nested if, should lint - if let Some(result) = a.checked_sub(b) { + if let Some(diff) = a.checked_sub(b) { //~^ manual_checked_sub if c > 0 { - let _ = result; + let _ = diff; } } } @@ -127,15 +100,15 @@ fn negative_tests() { let _ = 10 - 5; } - // Suffixed LHS, unsuffixed RHS (shouldn't lint) - if 10u32 >= 5 { - let _ = 10u32 - 5; - } + // // Suffixed LHS, unsuffixed RHS (shouldn't lint) + // if 10u32 >= 5 { + // let _ = 10u32 - 5; + // } - // Unsuffixed LHS, suffixed RHS (shouldn't lint) - if 10 >= 5u32 { - let _ = 10 - 5u32; - } + // // Unsuffixed LHS, suffixed RHS (shouldn't lint) + // if 10 >= 5u32 { + // let _ = 10 - 5u32; + // } } fn main() { diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs index f9ddaa45cbf9..284d9e0d54d4 100644 --- a/tests/ui/manual_checked_sub.rs +++ b/tests/ui/manual_checked_sub.rs @@ -40,33 +40,6 @@ fn positive_tests() { some_function(a - b); } - // Subtracting two suffixed literals - if 10u32 >= 5u32 { - //~^ manual_checked_sub - let _ = 10u32 - 5u32; - } - - let x: i32 = 10; - let y: i32 = 5; - - // Casted variables inside an if - if x as u32 >= y as u32 { - //~^ manual_checked_sub - let _ = x as u32 - y as u32; - } - - // Casted variable and literal inside an if - if x as u32 >= 5u32 { - //~^ manual_checked_sub - let _ = x as u32 - 5u32; - } - - // Casting subtraction result - if a >= b { - //~^ manual_checked_sub - let _ = (a - b) as u64; - } - if a >= b { //~^ manual_checked_sub macro_rules! subtract { @@ -127,15 +100,15 @@ fn negative_tests() { let _ = 10 - 5; } - // Suffixed LHS, unsuffixed RHS (shouldn't lint) - if 10u32 >= 5 { - let _ = 10u32 - 5; - } + // // Suffixed LHS, unsuffixed RHS (shouldn't lint) + // if 10u32 >= 5 { + // let _ = 10u32 - 5; + // } - // Unsuffixed LHS, suffixed RHS (shouldn't lint) - if 10 >= 5u32 { - let _ = 10 - 5u32; - } + // // Unsuffixed LHS, suffixed RHS (shouldn't lint) + // if 10 >= 5u32 { + // let _ = 10 - 5u32; + // } } fn main() { diff --git a/tests/ui/manual_checked_sub.stderr b/tests/ui/manual_checked_sub.stderr index 5847d1e2d3d4..7d170051e9c5 100644 --- a/tests/ui/manual_checked_sub.stderr +++ b/tests/ui/manual_checked_sub.stderr @@ -11,9 +11,9 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::manual_checked_sub)]` help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(diff) = a.checked_sub(b) { LL + -LL + let c = result; +LL + let c = diff; LL + } | @@ -30,9 +30,9 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(diff) = a.checked_sub(b) { LL + -LL + let c = result; +LL + let c = diff; LL + } else { LL + let c = a + b; LL + } @@ -51,9 +51,9 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(diff) = a.checked_sub(b) { LL + -LL + let c = result; +LL + let c = diff; LL + } else if a < b { LL + let c = a + b; LL + } @@ -70,9 +70,9 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(0) { +LL ~ if let Some(diff) = a.checked_sub(1) { LL + -LL + let _ = result; +LL + let _ = diff; LL + } | @@ -87,82 +87,14 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(diff) = a.checked_sub(b) { LL + -LL + some_function(result); +LL + some_function(diff); LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:44:5 - | -LL | / if 10u32 >= 5u32 { -LL | | -LL | | let _ = 10u32 - 5u32; -LL | | } - | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(result) = 10u32.checked_sub(5u32) { -LL + -LL + let _ = result; -LL + } - | - -error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:53:5 - | -LL | / if x as u32 >= y as u32 { -LL | | -LL | | let _ = x as u32 - y as u32; -LL | | } - | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(result) = (x as u32).checked_sub(y as u32) { -LL + -LL + let _ = result; -LL + } - | - -error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:59:5 - | -LL | / if x as u32 >= 5u32 { -LL | | -LL | | let _ = x as u32 - 5u32; -LL | | } - | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(result) = (x as u32).checked_sub(5u32) { -LL + -LL + let _ = result; -LL + } - | - -error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:65:5 - | -LL | / if a >= b { -LL | | -LL | | let _ = (a - b) as u64; -LL | | } - | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(result) = a.checked_sub(b) { -LL + -LL + let _ = result as u64; -LL + } - | - -error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:70:5 + --> tests/ui/manual_checked_sub.rs:43:5 | LL | / if a >= b { LL | | @@ -175,11 +107,11 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(diff) = a.checked_sub(b) { LL + LL + macro_rules! subtract { LL + () => { -LL + result +LL + diff LL + }; LL + } LL + let _ = subtract!(); @@ -187,7 +119,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:84:5 + --> tests/ui/manual_checked_sub.rs:57:5 | LL | / if a >= b { LL | | @@ -197,14 +129,14 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(diff) = a.checked_sub(b) { LL + -LL + let _ = Example { value: result }; +LL + let _ = Example { value: diff }; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:90:5 + --> tests/ui/manual_checked_sub.rs:63:5 | LL | / if a >= b { LL | | @@ -216,13 +148,13 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(diff) = a.checked_sub(b) { LL + LL + if c > 0 { -LL + let _ = result; +LL + let _ = diff; LL + } LL + } | -error: aborting due to 12 previous errors +error: aborting due to 8 previous errors From 99be19d94672c2e3c57f5187ca50fcb9edfd638b Mon Sep 17 00:00:00 2001 From: benacq Date: Mon, 7 Apr 2025 17:38:47 +0000 Subject: [PATCH 14/26] manual_checked_sub implementation review feedback fixes --- clippy_lints/.DS_Store | Bin 0 -> 6148 bytes clippy_lints/src/.DS_Store | Bin 0 -> 8196 bytes clippy_lints/src/manual_checked_sub.rs | 69 +++++++++++---- tests/ui/manual_checked_sub.fixed | 90 +++++++++++--------- tests/ui/manual_checked_sub.rs | 64 +++++++------- tests/ui/manual_checked_sub.stderr | 113 ++++++++++++++++++------- 6 files changed, 217 insertions(+), 119 deletions(-) create mode 100644 clippy_lints/.DS_Store create mode 100644 clippy_lints/src/.DS_Store diff --git a/clippy_lints/.DS_Store b/clippy_lints/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..4b3523353e0328eddbb93f59dd9440ca93c37d49 GIT binary patch literal 6148 zcmeHKyH3ME5ZomxEYYN-ykAI1X@dEJNYp5a1Vt=ILG|A-~#x?zCOgthU_>nd<4`^!)YZ+0IX1+iiZ>-Q5gMy?g|XMg^z<6`%rC;9o0% zo^4hg2QpFtDnJE36tM3@fg9F|eV~6j5PSpx+HbrYo_&@87E1u@#6A!imW`S4A-a2|Y>$L@b1%EfxdO3o(VxYHTEUXou4C;zKW4})91D%e%(}DaEFkNU= I;5QU_148yG-v9sr literal 0 HcmV?d00001 diff --git a/clippy_lints/src/.DS_Store b/clippy_lints/src/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..88c8a6f0cc7f9e51144fdba7e0c9bdb0a153a08e GIT binary patch literal 8196 zcmeHMJ!=#}7=GtINIVM#n}E$WHl{J1ottp>LO>9~eB52`4(_r?K7@2vr%|xfF8%$_lDl#pQ6Zmbz1N7*R9aw`;N*pU|*IH}mia1IUx2l5W+wR@G8Xak?M?E3x1 zM_JoX>h~(ZCT6tudXou zaehBZ`TYpNPY+Z6bvPjRFVxdlV}4W1`Qy(Seuqf;{RqKd8fgA9H7TKn`0eV_r?iIi z?!5du^4;a}-<+=%d}>kioud`nq*b`SL0vGfW_~vv?)p(&$Bpm1BORXmaZSe~r9Rgm zdQg?nAfv1Fs103J8rO0?&tLD2^aw=S9)5)2FPrw5puTw`8q}dyMnm~t9U3wSuH(w| z{zyj-m1-$Q2|bkvWQMJUw(7u>W=2mfewIgYJx}O>>tXldZB&dBdM+3}HTct|b@ZPF z_@kfP9&L%$!dT=SYu0q&oZv$sdCN{YQ^dU8qhtwEsC8@WoNhhLL^u|4=%R5$8%+vMW+Xy1%W=f LateLintPass<'tcx> for ManualCheckedSub { && is_unsigned_int(cx, lhs) && is_unsigned_int(cx, rhs) { - if let BinOpKind::Ge | BinOpKind::Gt = op.node { + // Skip if either non-literal operand is mutable + if (!matches!(lhs.kind, ExprKind::Lit(_)) && is_mutable(cx, lhs)) + || (!matches!(rhs.kind, ExprKind::Lit(_)) && is_mutable(cx, rhs)) + { + return; + } + + if let BinOpKind::Ge | BinOpKind::Gt | BinOpKind::Le | BinOpKind::Lt = op.node { SubExprVisitor { cx, if_expr: expr, @@ -97,13 +104,23 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { if let ExprKind::Binary(op, sub_lhs, sub_rhs) = expr.kind && let BinOpKind::Sub = op.node { + if let ExprKind::Lit(lit) = self.condition_lhs.kind + && self.condition_op == BinOpKind::Lt + && let LitKind::Int(Pu128(0), _) = lit.node + && (is_referencing_same_variable(sub_lhs, self.condition_rhs)) + { + self.emit_lint(expr, Some(sub_rhs)); + } + if let ExprKind::Lit(lit) = self.condition_rhs.kind && self.condition_op == BinOpKind::Gt && let LitKind::Int(Pu128(0), _) = lit.node && (is_referencing_same_variable(sub_lhs, self.condition_lhs)) { self.emit_lint(expr, Some(sub_rhs)); - } else if self.condition_op == BinOpKind::Ge + } + + if self.condition_op == BinOpKind::Ge && (is_referencing_same_variable(sub_lhs, self.condition_lhs) || are_literals_equal(sub_lhs, self.condition_lhs)) && (is_referencing_same_variable(sub_rhs, self.condition_rhs) @@ -111,6 +128,15 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { { self.emit_lint(expr, None); } + + if self.condition_op == BinOpKind::Le + && (is_referencing_same_variable(sub_lhs, self.condition_rhs) + || are_literals_equal(sub_lhs, self.condition_rhs)) + && (is_referencing_same_variable(sub_rhs, self.condition_lhs) + || are_literals_equal(sub_rhs, self.condition_lhs)) + { + self.emit_lint(expr, None); + } } walk_expr(self, expr); @@ -118,32 +144,41 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { } impl<'tcx> SubExprVisitor<'_, 'tcx> { - // fn is_name_in_scope(&self, name: &str, id: HirId) -> bool { - // let def_id = self.cx.tcx.hir().local_def_id(id); - // let resolver = self.cx.tcx.resolver_for_lowering(); - - // // Check if the name is already defined in the current scope - // resolver.is_name_defined(name) - // } - fn emit_lint(&mut self, expr: &Expr<'tcx>, sub_rhs_expr: Option<&'tcx Expr<'tcx>>) { let mut applicability = Applicability::MachineApplicable; let body_snippet = snippet_with_applicability(self.cx, self.if_body_block.span, "..", &mut applicability); let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut applicability); - let updated_usage_context_snippet = body_snippet.as_ref().replace(binary_expr_snippet.as_ref(), "diff"); + let updated_usage_context_snippet = body_snippet.as_ref().replace(binary_expr_snippet.as_ref(), "result"); + + let lhs_snippet = Sugg::hir(self.cx, self.condition_lhs, "..").maybe_par(); let rhs_snippet = match sub_rhs_expr { Some(sub_rhs) => Sugg::hir(self.cx, sub_rhs, "..").maybe_par(), None => Sugg::hir(self.cx, self.condition_rhs, "..").maybe_par(), }; - // TODO: Check diff variable is not already in scope - let mut suggestion = format!( - "if let Some(diff) = {}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}", - Sugg::hir(self.cx, self.condition_lhs, "..").maybe_par() - ); + // TODO: Check result variable is not already in scope + let mut suggestion = match self.condition_op { + BinOpKind::Le => { + format!( + "if let Some(result) = {rhs_snippet}.checked_sub({lhs_snippet}) {updated_usage_context_snippet}" + ) + }, + + BinOpKind::Lt => { + format!( + "if let Some(result) = {}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}", + Sugg::hir(self.cx, self.condition_rhs, "..").maybe_par() + ) + }, + _ => { + format!( + "if let Some(result) = {lhs_snippet}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}" + ) + }, + }; if let Some(else_expr) = self.else_block { let else_snippet = snippet_with_applicability(self.cx, else_expr.span, "..", &mut applicability); diff --git a/tests/ui/manual_checked_sub.fixed b/tests/ui/manual_checked_sub.fixed index 49cb3a477c8e..6861aeb21b47 100644 --- a/tests/ui/manual_checked_sub.fixed +++ b/tests/ui/manual_checked_sub.fixed @@ -4,47 +4,52 @@ fn positive_tests() { let a: u32 = 10; let b: u32 = 5; - let c = 3; + let c: u32 = 3; - // Basic subtraction inside an if - if let Some(diff) = a.checked_sub(b) { + if let Some(result) = a.checked_sub(b) { //~^ manual_checked_sub - let c = diff; + let d = result; } - // Basic subtraction inside an if with an else block - if let Some(diff) = a.checked_sub(b) { + if let Some(result) = a.checked_sub(b) { //~^ manual_checked_sub - let c = diff; + let d = result; + } + + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + let d = result; } else { - let c = a + b; + let d = a + b; } - // Basic subtraction inside an if with an else-if block - if let Some(diff) = a.checked_sub(b) { + if let Some(result) = a.checked_sub(b) { //~^ manual_checked_sub - let c = diff; + let d = result; } else if a < b { - let c = a + b; + let d = a + b; } - // Decrementing inside an if condition - if let Some(diff) = a.checked_sub(1) { + if let Some(result) = a.checked_sub(1) { //~^ manual_checked_sub - let _ = diff; + let _ = result; } - // Variable subtraction used in a function call - if let Some(diff) = a.checked_sub(b) { + if let Some(result) = a.checked_sub(1) { //~^ manual_checked_sub - some_function(diff); + let _ = result; } - if let Some(diff) = a.checked_sub(b) { + if let Some(result) = a.checked_sub(b) { + //~^ manual_checked_sub + some_function(result); + } + + if let Some(result) = a.checked_sub(b) { //~^ manual_checked_sub macro_rules! subtract { () => { - diff + result }; } let _ = subtract!(); @@ -54,16 +59,22 @@ fn positive_tests() { value: u32, } - if let Some(diff) = a.checked_sub(b) { + if let Some(result) = a.checked_sub(b) { //~^ manual_checked_sub - let _ = Example { value: diff }; + let _ = Example { value: result }; } - // Subtraction inside a nested if, should lint - if let Some(diff) = a.checked_sub(b) { + if let Some(result) = a.checked_sub(b) { //~^ manual_checked_sub if c > 0 { - let _ = diff; + let _ = result; + } + } + + if a >= b { + if let Some(result) = c.checked_sub(1) { + //~^ manual_checked_sub + let _ = result; } } } @@ -74,41 +85,36 @@ fn negative_tests() { let a: u32 = 10; let b: u32 = 5; - // Uses `.checked_sub()`, should not trigger - let _ = a.checked_sub(b); - - // Works with signed integers (i32), should not trigger let x: i32 = 10; let y: i32 = 5; + let mut a_mut = 10; + + if a_mut >= b { + a_mut *= 2; + let c = a_mut - b; + } + + if a_mut > 0 { + a_mut *= 2; + let c = a_mut - 1; + } + if x >= y { let _ = x - y; } - // If condition does not match subtraction variables if a == b { let _ = a - b; } - // a - b inside an if a > b (shouldn't lint) if a > b { let _ = a - b; } - // Unsuffixed literals (shouldn't lint) if 10 >= 5 { let _ = 10 - 5; } - - // // Suffixed LHS, unsuffixed RHS (shouldn't lint) - // if 10u32 >= 5 { - // let _ = 10u32 - 5; - // } - - // // Unsuffixed LHS, suffixed RHS (shouldn't lint) - // if 10 >= 5u32 { - // let _ = 10 - 5u32; - // } } fn main() { diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs index 284d9e0d54d4..b55496c5aa9b 100644 --- a/tests/ui/manual_checked_sub.rs +++ b/tests/ui/manual_checked_sub.rs @@ -4,37 +4,42 @@ fn positive_tests() { let a: u32 = 10; let b: u32 = 5; - let c = 3; + let c: u32 = 3; - // Basic subtraction inside an if if a >= b { //~^ manual_checked_sub - let c = a - b; + let d = a - b; + } + + if b <= a { + //~^ manual_checked_sub + let d = a - b; } - // Basic subtraction inside an if with an else block if a >= b { //~^ manual_checked_sub - let c = a - b; + let d = a - b; } else { - let c = a + b; + let d = a + b; } - // Basic subtraction inside an if with an else-if block if a >= b { //~^ manual_checked_sub - let c = a - b; + let d = a - b; } else if a < b { - let c = a + b; + let d = a + b; } - // Decrementing inside an if condition if a > 0 { //~^ manual_checked_sub let _ = a - 1; } - // Variable subtraction used in a function call + if 0 < a { + //~^ manual_checked_sub + let _ = a - 1; + } + if a >= b { //~^ manual_checked_sub some_function(a - b); @@ -59,13 +64,19 @@ fn positive_tests() { let _ = Example { value: a - b }; } - // Subtraction inside a nested if, should lint if a >= b { //~^ manual_checked_sub if c > 0 { let _ = a - b; } } + + if a >= b { + if c > 0 { + //~^ manual_checked_sub + let _ = c - 1; + } + } } fn some_function(a: u32) {} @@ -74,41 +85,36 @@ fn negative_tests() { let a: u32 = 10; let b: u32 = 5; - // Uses `.checked_sub()`, should not trigger - let _ = a.checked_sub(b); - - // Works with signed integers (i32), should not trigger let x: i32 = 10; let y: i32 = 5; + let mut a_mut = 10; + + if a_mut >= b { + a_mut *= 2; + let c = a_mut - b; + } + + if a_mut > 0 { + a_mut *= 2; + let c = a_mut - 1; + } + if x >= y { let _ = x - y; } - // If condition does not match subtraction variables if a == b { let _ = a - b; } - // a - b inside an if a > b (shouldn't lint) if a > b { let _ = a - b; } - // Unsuffixed literals (shouldn't lint) if 10 >= 5 { let _ = 10 - 5; } - - // // Suffixed LHS, unsuffixed RHS (shouldn't lint) - // if 10u32 >= 5 { - // let _ = 10u32 - 5; - // } - - // // Unsuffixed LHS, suffixed RHS (shouldn't lint) - // if 10 >= 5u32 { - // let _ = 10 - 5u32; - // } } fn main() { diff --git a/tests/ui/manual_checked_sub.stderr b/tests/ui/manual_checked_sub.stderr index 7d170051e9c5..d2f0cd9dbbbd 100644 --- a/tests/ui/manual_checked_sub.stderr +++ b/tests/ui/manual_checked_sub.stderr @@ -1,9 +1,9 @@ error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:10:5 + --> tests/ui/manual_checked_sub.rs:9:5 | LL | / if a >= b { LL | | -LL | | let c = a - b; +LL | | let d = a - b; LL | | } | |_____^ | @@ -11,56 +11,73 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::manual_checked_sub)]` help: consider using `.checked_sub()` | -LL ~ if let Some(diff) = a.checked_sub(b) { +LL ~ if let Some(result) = a.checked_sub(b) { LL + -LL + let c = diff; +LL + let d = result; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:16:5 + --> tests/ui/manual_checked_sub.rs:14:5 + | +LL | / if b <= a { +LL | | +LL | | let d = a - b; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(b) { +LL + +LL + let d = result; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:19:5 | LL | / if a >= b { LL | | -LL | | let c = a - b; +LL | | let d = a - b; LL | | } else { -LL | | let c = a + b; +LL | | let d = a + b; LL | | } | |_____^ | help: consider using `.checked_sub()` | -LL ~ if let Some(diff) = a.checked_sub(b) { +LL ~ if let Some(result) = a.checked_sub(b) { LL + -LL + let c = diff; +LL + let d = result; LL + } else { -LL + let c = a + b; +LL + let d = a + b; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:24:5 + --> tests/ui/manual_checked_sub.rs:26:5 | LL | / if a >= b { LL | | -LL | | let c = a - b; +LL | | let d = a - b; LL | | } else if a < b { -LL | | let c = a + b; +LL | | let d = a + b; LL | | } | |_____^ | help: consider using `.checked_sub()` | -LL ~ if let Some(diff) = a.checked_sub(b) { +LL ~ if let Some(result) = a.checked_sub(b) { LL + -LL + let c = diff; +LL + let d = result; LL + } else if a < b { -LL + let c = a + b; +LL + let d = a + b; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:32:5 + --> tests/ui/manual_checked_sub.rs:33:5 | LL | / if a > 0 { LL | | @@ -70,15 +87,32 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(diff) = a.checked_sub(1) { +LL ~ if let Some(result) = a.checked_sub(1) { LL + -LL + let _ = diff; +LL + let _ = result; LL + } | error: manual re-implementation of checked subtraction --> tests/ui/manual_checked_sub.rs:38:5 | +LL | / if 0 < a { +LL | | +LL | | let _ = a - 1; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = a.checked_sub(1) { +LL + +LL + let _ = result; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:43:5 + | LL | / if a >= b { LL | | LL | | some_function(a - b); @@ -87,14 +121,14 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(diff) = a.checked_sub(b) { +LL ~ if let Some(result) = a.checked_sub(b) { LL + -LL + some_function(diff); +LL + some_function(result); LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:43:5 + --> tests/ui/manual_checked_sub.rs:48:5 | LL | / if a >= b { LL | | @@ -107,11 +141,11 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(diff) = a.checked_sub(b) { +LL ~ if let Some(result) = a.checked_sub(b) { LL + LL + macro_rules! subtract { LL + () => { -LL + diff +LL + result LL + }; LL + } LL + let _ = subtract!(); @@ -119,7 +153,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:57:5 + --> tests/ui/manual_checked_sub.rs:62:5 | LL | / if a >= b { LL | | @@ -129,14 +163,14 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(diff) = a.checked_sub(b) { +LL ~ if let Some(result) = a.checked_sub(b) { LL + -LL + let _ = Example { value: diff }; +LL + let _ = Example { value: result }; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:63:5 + --> tests/ui/manual_checked_sub.rs:67:5 | LL | / if a >= b { LL | | @@ -148,13 +182,30 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(diff) = a.checked_sub(b) { +LL ~ if let Some(result) = a.checked_sub(b) { LL + LL + if c > 0 { -LL + let _ = diff; +LL + let _ = result; LL + } LL + } | -error: aborting due to 8 previous errors +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:75:9 + | +LL | / if c > 0 { +LL | | +LL | | let _ = c - 1; +LL | | } + | |_________^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(result) = c.checked_sub(1) { +LL + +LL + let _ = result; +LL + } + | + +error: aborting due to 11 previous errors From e3b02622138efda698185892bfefaca1168ab2b2 Mon Sep 17 00:00:00 2001 From: benacq Date: Mon, 7 Apr 2025 17:49:55 +0000 Subject: [PATCH 15/26] manual_checked_sub implementation review feedback fixes --- clippy_lints/src/manual_checked_sub.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index e1f09b91c920..2efd15647d17 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -152,11 +152,11 @@ impl<'tcx> SubExprVisitor<'_, 'tcx> { let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut applicability); let updated_usage_context_snippet = body_snippet.as_ref().replace(binary_expr_snippet.as_ref(), "result"); - let lhs_snippet = Sugg::hir(self.cx, self.condition_lhs, "..").maybe_par(); + let lhs_snippet = Sugg::hir(self.cx, self.condition_lhs, "..").maybe_paren(); let rhs_snippet = match sub_rhs_expr { - Some(sub_rhs) => Sugg::hir(self.cx, sub_rhs, "..").maybe_par(), - None => Sugg::hir(self.cx, self.condition_rhs, "..").maybe_par(), + Some(sub_rhs) => Sugg::hir(self.cx, sub_rhs, "..").maybe_paren(), + None => Sugg::hir(self.cx, self.condition_rhs, "..").maybe_paren(), }; // TODO: Check result variable is not already in scope @@ -170,7 +170,7 @@ impl<'tcx> SubExprVisitor<'_, 'tcx> { BinOpKind::Lt => { format!( "if let Some(result) = {}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}", - Sugg::hir(self.cx, self.condition_rhs, "..").maybe_par() + Sugg::hir(self.cx, self.condition_rhs, "..").maybe_paren() ) }, _ => { From cc58db7bdf6fd4e40f1c7342e697db009473246e Mon Sep 17 00:00:00 2001 From: benacq Date: Mon, 7 Apr 2025 17:56:50 +0000 Subject: [PATCH 16/26] manual_checked_sub implementation review feedback fixes --- clippy_lints/src/manual_checked_sub.rs | 35 ++++++++++++++++++++------ 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 2efd15647d17..9ebfa6c77bc9 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -23,16 +23,35 @@ declare_clippy_lint! { /// Using the standard library method `.checked_sub()` is clearer and less likely to contain bugs. /// /// ### Example - /// ```no_run - /// if 10u32 >= 4u32 { - /// let c = 10u32 - 4u32; + /// ```rust + /// // Before + /// fn calculate_remaining(a: u32, b: u32) -> Option { + /// if a >= b { + /// Some(a - b) + /// } else { + /// None + /// } + /// } + /// + /// // After + /// fn calculate_remaining(a: u32, b: u32) -> Option { + /// a.checked_sub(b) /// } /// ``` - /// Use instead: - /// ```no_run - /// if let Some(result) = 10u32.checked_sub(4u32){ - /// result; - /// }; + /// + /// ### Real-world cases it catches + /// ```rust + /// // Buffer position calculation + /// if current_pos >= offset { + /// let new_pos = current_pos - offset; + /// // ... use new_pos + /// } + /// + /// // Inventory management + /// if available >= requested { + /// dispatch_items(requested); + /// remaining = available - requested; + /// } /// ``` #[clippy::version = "1.86.0"] pub MANUAL_CHECKED_SUB, From 9d2189313acccbda09bfbd550c0d9cb72f205501 Mon Sep 17 00:00:00 2001 From: benacq Date: Mon, 7 Apr 2025 20:01:01 +0000 Subject: [PATCH 17/26] manual_checked_sub implementation review feedback fixes - Improved rustdoc example --- clippy_lints/src/manual_checked_sub.rs | 43 +++++++++++++------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 9ebfa6c77bc9..0644cb3197b1 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -78,31 +78,30 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { return; } - if let Some(if_expr) = clippy_utils::higher::If::hir(expr) { - if let ExprKind::Binary(op, lhs, rhs) = if_expr.cond.kind - && !(matches!(lhs.kind, ExprKind::Lit(_)) && matches!(rhs.kind, ExprKind::Lit(_))) - && is_unsigned_int(cx, lhs) - && is_unsigned_int(cx, rhs) + if let Some(if_expr) = clippy_utils::higher::If::hir(expr) + && let ExprKind::Binary(op, lhs, rhs) = if_expr.cond.kind + && !(matches!(lhs.kind, ExprKind::Lit(_)) && matches!(rhs.kind, ExprKind::Lit(_))) + && is_unsigned_int(cx, lhs) + && is_unsigned_int(cx, rhs) + { + // Skip if either non-literal operand is mutable + if (!matches!(lhs.kind, ExprKind::Lit(_)) && is_mutable(cx, lhs)) + || (!matches!(rhs.kind, ExprKind::Lit(_)) && is_mutable(cx, rhs)) { - // Skip if either non-literal operand is mutable - if (!matches!(lhs.kind, ExprKind::Lit(_)) && is_mutable(cx, lhs)) - || (!matches!(rhs.kind, ExprKind::Lit(_)) && is_mutable(cx, rhs)) - { - return; - } + return; + } - if let BinOpKind::Ge | BinOpKind::Gt | BinOpKind::Le | BinOpKind::Lt = op.node { - SubExprVisitor { - cx, - if_expr: expr, - if_body_block: if_expr.then, - else_block: if_expr.r#else, - condition_lhs: lhs, - condition_rhs: rhs, - condition_op: op.node, - } - .visit_expr(if_expr.then); + if let BinOpKind::Ge | BinOpKind::Gt | BinOpKind::Le | BinOpKind::Lt = op.node { + SubExprVisitor { + cx, + if_expr: expr, + if_body_block: if_expr.then, + else_block: if_expr.r#else, + condition_lhs: lhs, + condition_rhs: rhs, + condition_op: op.node, } + .visit_expr(if_expr.then); } } } From 46d448f562196e83e3dd8821d9189b9eef5e3ff3 Mon Sep 17 00:00:00 2001 From: benacq Date: Mon, 7 Apr 2025 20:21:17 +0000 Subject: [PATCH 18/26] manual_checked_sub implementation review feedback fixes - Improved rustdoc example --- clippy_lints/src/manual_checked_sub.rs | 28 ++++++++------------------ 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 0644cb3197b1..7a20d54c1fbb 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -24,33 +24,21 @@ declare_clippy_lint! { /// /// ### Example /// ```rust - /// // Before - /// fn calculate_remaining(a: u32, b: u32) -> Option { - /// if a >= b { - /// Some(a - b) + /// // Bad: Manual implementation of checked subtraction + /// fn get_remaining_items(total: u32, used: u32) -> Option { + /// if total >= used { + /// Some(total - used) /// } else { /// None /// } /// } - /// - /// // After - /// fn calculate_remaining(a: u32, b: u32) -> Option { - /// a.checked_sub(b) - /// } /// ``` /// - /// ### Real-world cases it catches + /// Use instead: /// ```rust - /// // Buffer position calculation - /// if current_pos >= offset { - /// let new_pos = current_pos - offset; - /// // ... use new_pos - /// } - /// - /// // Inventory management - /// if available >= requested { - /// dispatch_items(requested); - /// remaining = available - requested; + /// // Good: Using the standard library's checked_sub + /// fn get_remaining_items(total: u32, used: u32) -> Option { + /// total.checked_sub(used) /// } /// ``` #[clippy::version = "1.86.0"] From 31aca94b0d95f81ebdc304a0dab9bd9c69f7de9b Mon Sep 17 00:00:00 2001 From: benacq Date: Tue, 8 Apr 2025 07:24:51 +0000 Subject: [PATCH 19/26] manual_checked_sub implementation review feedback fixes - Variable generator function --- clippy_lints/src/manual_checked_sub.rs | 117 ++++++++++++++++++++++--- tests/ui/manual_checked_sub.fixed | 58 +++++++----- tests/ui/manual_checked_sub.rs | 14 +++ tests/ui/manual_checked_sub.stderr | 106 +++++++++++++++------- 4 files changed, 228 insertions(+), 67 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 7a20d54c1fbb..6ba282618da7 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -7,8 +7,8 @@ use clippy_utils::{is_mutable, path_to_local}; use rustc_ast::{BinOpKind, LitIntType, LitKind}; use rustc_data_structures::packed::Pu128; use rustc_errors::Applicability; -use rustc_hir::intravisit::{Visitor, walk_expr}; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::intravisit::{Visitor, walk_expr, walk_stmt}; +use rustc_hir::{Expr, ExprKind, PatKind, QPath}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self}; use rustc_session::impl_lint_pass; @@ -95,8 +95,8 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { } } -struct SubExprVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, +struct SubExprVisitor<'cx, 'tcx> { + cx: &'cx LateContext<'tcx>, if_expr: &'tcx Expr<'tcx>, if_body_block: &'tcx Expr<'tcx>, else_block: Option<&'tcx Expr<'tcx>>, @@ -155,8 +155,15 @@ impl<'tcx> SubExprVisitor<'_, 'tcx> { let body_snippet = snippet_with_applicability(self.cx, self.if_body_block.span, "..", &mut applicability); + let checked_sub_result_var_name = match self.condition_op { + BinOpKind::Lt | BinOpKind::Gt => generate_unique_var_name("decremented", self.if_body_block), + _ => generate_unique_var_name("difference", self.if_body_block), + }; + let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut applicability); - let updated_usage_context_snippet = body_snippet.as_ref().replace(binary_expr_snippet.as_ref(), "result"); + let updated_usage_context_snippet = body_snippet + .as_ref() + .replace(binary_expr_snippet.as_ref(), &checked_sub_result_var_name); let lhs_snippet = Sugg::hir(self.cx, self.condition_lhs, "..").maybe_paren(); @@ -169,19 +176,19 @@ impl<'tcx> SubExprVisitor<'_, 'tcx> { let mut suggestion = match self.condition_op { BinOpKind::Le => { format!( - "if let Some(result) = {rhs_snippet}.checked_sub({lhs_snippet}) {updated_usage_context_snippet}" + "if let Some({checked_sub_result_var_name}) = {rhs_snippet}.checked_sub({lhs_snippet}) {updated_usage_context_snippet}" ) }, BinOpKind::Lt => { format!( - "if let Some(result) = {}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}", + "if let Some({checked_sub_result_var_name}) = {}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}", Sugg::hir(self.cx, self.condition_rhs, "..").maybe_paren() ) }, _ => { format!( - "if let Some(result) = {lhs_snippet}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}" + "if let Some({checked_sub_result_var_name}) = {lhs_snippet}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}" ) }, }; @@ -216,10 +223,7 @@ fn are_literals_equal<'tcx>(expr1: &Expr<'tcx>, expr2: &Expr<'tcx>) -> bool { if let (ExprKind::Lit(lit1), ExprKind::Lit(lit2)) = (&expr1.kind, &expr2.kind) && let (LitKind::Int(val1, suffix1), LitKind::Int(val2, suffix2)) = (&lit1.node, &lit2.node) { - return val1 == val2 - && suffix1 == suffix2 - && *suffix1 != LitIntType::Unsuffixed - && *suffix2 != LitIntType::Unsuffixed; + return val1 == val2 && suffix1 == suffix2 && *suffix1 != LitIntType::Unsuffixed; } false @@ -232,3 +236,92 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<' false } +/// Generates a unique variable name based on the provided base name. +/// If the base name already exists in the scope, appends a number to make it unique. +fn generate_unique_var_name<'tcx>(base_name: &str, scope_expr: &'tcx Expr<'tcx>) -> String { + struct VarNameVisitor<'cx> { + base_name: &'cx str, + is_name_in_scope: bool, + } + + impl<'tcx> Visitor<'tcx> for VarNameVisitor<'_> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { + if let Some(segment) = path.segments.last() { + let name = segment.ident.name.to_string(); + if name == self.base_name { + self.is_name_in_scope = true; + } + } + } + + walk_expr(self, expr); + } + + fn visit_stmt(&mut self, stmt: &'tcx rustc_hir::Stmt<'_>) { + if let rustc_hir::StmtKind::Let(let_stmt) = &stmt.kind { + if let PatKind::Binding(_, _, ident, _) = &let_stmt.pat.kind { + let name = ident.name.to_string(); + if name == self.base_name { + self.is_name_in_scope = true; + } + } + } + + walk_stmt(self, stmt); + } + } + + let mut visitor = VarNameVisitor { + base_name, + is_name_in_scope: false, + }; + + if let ExprKind::Block(block, _) = &scope_expr.kind { + for stmt in block.stmts { + visitor.visit_stmt(stmt); + } + + // Visit the optional expression at the end of the block + if let Some(expr) = block.expr { + visitor.visit_expr(expr); + } + } else { + visitor.visit_expr(scope_expr); + } + + if !visitor.is_name_in_scope { + return base_name.to_string(); + } + + // If the base name is in scope, append a number + // Keep incrementing until we find a name that's not in scope + let mut counter = 1; + loop { + let candidate = format!("{}_{}", base_name, counter); + + let mut candidate_visitor = VarNameVisitor { + base_name: &candidate, + is_name_in_scope: false, + }; + + // Check if the candidate name is in scope + if let ExprKind::Block(block, _) = &scope_expr.kind { + for stmt in block.stmts { + candidate_visitor.visit_stmt(stmt); + } + + if let Some(expr) = block.expr { + candidate_visitor.visit_expr(expr); + } + } else { + candidate_visitor.visit_expr(scope_expr); + } + + if !candidate_visitor.is_name_in_scope { + return candidate; + } + + counter += 1; + } +} diff --git a/tests/ui/manual_checked_sub.fixed b/tests/ui/manual_checked_sub.fixed index 6861aeb21b47..efe146050f66 100644 --- a/tests/ui/manual_checked_sub.fixed +++ b/tests/ui/manual_checked_sub.fixed @@ -6,50 +6,64 @@ fn positive_tests() { let b: u32 = 5; let c: u32 = 3; - if let Some(result) = a.checked_sub(b) { + if let Some(difference_1) = a.checked_sub(b) { //~^ manual_checked_sub - let d = result; + let difference = "some test"; + let d = difference_1; } - if let Some(result) = a.checked_sub(b) { + if let Some(difference) = a.checked_sub(b) { //~^ manual_checked_sub - let d = result; + let d = difference; } - if let Some(result) = a.checked_sub(b) { + if let Some(difference) = a.checked_sub(b) { //~^ manual_checked_sub - let d = result; + let d = difference; } else { let d = a + b; } - if let Some(result) = a.checked_sub(b) { + if let Some(difference) = a.checked_sub(b) { //~^ manual_checked_sub - let d = result; + let d = difference; } else if a < b { let d = a + b; } - if let Some(result) = a.checked_sub(1) { + if let Some(decremented) = a.checked_sub(1) { //~^ manual_checked_sub - let _ = result; + let _ = decremented; } - if let Some(result) = a.checked_sub(1) { + if let Some(decremented) = a.checked_sub(1) { //~^ manual_checked_sub - let _ = result; + let _ = decremented; } - if let Some(result) = a.checked_sub(b) { + let mut difference = "variable initialized outside the if scope"; + if let Some(difference_1) = a.checked_sub(b) { //~^ manual_checked_sub - some_function(result); + let some_reference = difference; + let d = difference_1; } - if let Some(result) = a.checked_sub(b) { + if let Some(decremented_1) = a.checked_sub(b) { + //~^ manual_checked_sub + let decremented = "variable initialized inside the if scope"; + let d = decremented_1; + } + + if let Some(difference) = a.checked_sub(b) { + //~^ manual_checked_sub + some_function(difference); + } + + if let Some(difference) = a.checked_sub(b) { //~^ manual_checked_sub macro_rules! subtract { () => { - result + difference }; } let _ = subtract!(); @@ -59,22 +73,22 @@ fn positive_tests() { value: u32, } - if let Some(result) = a.checked_sub(b) { + if let Some(difference) = a.checked_sub(b) { //~^ manual_checked_sub - let _ = Example { value: result }; + let _ = Example { value: difference }; } - if let Some(result) = a.checked_sub(b) { + if let Some(difference) = a.checked_sub(b) { //~^ manual_checked_sub if c > 0 { - let _ = result; + let _ = difference; } } if a >= b { - if let Some(result) = c.checked_sub(1) { + if let Some(decremented) = c.checked_sub(1) { //~^ manual_checked_sub - let _ = result; + let _ = decremented; } } } diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs index b55496c5aa9b..938485957c57 100644 --- a/tests/ui/manual_checked_sub.rs +++ b/tests/ui/manual_checked_sub.rs @@ -8,6 +8,7 @@ fn positive_tests() { if a >= b { //~^ manual_checked_sub + let difference = "some test"; let d = a - b; } @@ -40,6 +41,19 @@ fn positive_tests() { let _ = a - 1; } + let mut difference = "variable initialized outside the if scope"; + if a >= b { + //~^ manual_checked_sub + let some_reference = difference; + let d = a - b; + } + + if a > 0 { + //~^ manual_checked_sub + let decremented = "variable initialized inside the if scope"; + let d = a - b; + } + if a >= b { //~^ manual_checked_sub some_function(a - b); diff --git a/tests/ui/manual_checked_sub.stderr b/tests/ui/manual_checked_sub.stderr index d2f0cd9dbbbd..73eadfe7d328 100644 --- a/tests/ui/manual_checked_sub.stderr +++ b/tests/ui/manual_checked_sub.stderr @@ -3,6 +3,7 @@ error: manual re-implementation of checked subtraction | LL | / if a >= b { LL | | +LL | | let difference = "some test"; LL | | let d = a - b; LL | | } | |_____^ @@ -11,14 +12,15 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::manual_checked_sub)]` help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(difference_1) = a.checked_sub(b) { LL + -LL + let d = result; +LL + let difference = "some test"; +LL + let d = difference_1; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:14:5 + --> tests/ui/manual_checked_sub.rs:15:5 | LL | / if b <= a { LL | | @@ -28,14 +30,14 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(difference) = a.checked_sub(b) { LL + -LL + let d = result; +LL + let d = difference; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:19:5 + --> tests/ui/manual_checked_sub.rs:20:5 | LL | / if a >= b { LL | | @@ -47,16 +49,16 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(difference) = a.checked_sub(b) { LL + -LL + let d = result; +LL + let d = difference; LL + } else { LL + let d = a + b; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:26:5 + --> tests/ui/manual_checked_sub.rs:27:5 | LL | / if a >= b { LL | | @@ -68,16 +70,16 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(difference) = a.checked_sub(b) { LL + -LL + let d = result; +LL + let d = difference; LL + } else if a < b { LL + let d = a + b; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:33:5 + --> tests/ui/manual_checked_sub.rs:34:5 | LL | / if a > 0 { LL | | @@ -87,14 +89,14 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(1) { +LL ~ if let Some(decremented) = a.checked_sub(1) { LL + -LL + let _ = result; +LL + let _ = decremented; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:38:5 + --> tests/ui/manual_checked_sub.rs:39:5 | LL | / if 0 < a { LL | | @@ -104,14 +106,52 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(1) { +LL ~ if let Some(decremented) = a.checked_sub(1) { LL + -LL + let _ = result; +LL + let _ = decremented; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:43:5 + --> tests/ui/manual_checked_sub.rs:45:5 + | +LL | / if a >= b { +LL | | +LL | | let some_reference = difference; +LL | | let d = a - b; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(difference_1) = a.checked_sub(b) { +LL + +LL + let some_reference = difference; +LL + let d = difference_1; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:51:5 + | +LL | / if a > 0 { +LL | | +LL | | let decremented = "variable initialized inside the if scope"; +LL | | let d = a - b; +LL | | } + | |_____^ + | +help: consider using `.checked_sub()` + | +LL ~ if let Some(decremented_1) = a.checked_sub(b) { +LL + +LL + let decremented = "variable initialized inside the if scope"; +LL + let d = decremented_1; +LL + } + | + +error: manual re-implementation of checked subtraction + --> tests/ui/manual_checked_sub.rs:57:5 | LL | / if a >= b { LL | | @@ -121,14 +161,14 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(difference) = a.checked_sub(b) { LL + -LL + some_function(result); +LL + some_function(difference); LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:48:5 + --> tests/ui/manual_checked_sub.rs:62:5 | LL | / if a >= b { LL | | @@ -141,11 +181,11 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(difference) = a.checked_sub(b) { LL + LL + macro_rules! subtract { LL + () => { -LL + result +LL + difference LL + }; LL + } LL + let _ = subtract!(); @@ -153,7 +193,7 @@ LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:62:5 + --> tests/ui/manual_checked_sub.rs:76:5 | LL | / if a >= b { LL | | @@ -163,14 +203,14 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(difference) = a.checked_sub(b) { LL + -LL + let _ = Example { value: result }; +LL + let _ = Example { value: difference }; LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:67:5 + --> tests/ui/manual_checked_sub.rs:81:5 | LL | / if a >= b { LL | | @@ -182,16 +222,16 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = a.checked_sub(b) { +LL ~ if let Some(difference) = a.checked_sub(b) { LL + LL + if c > 0 { -LL + let _ = result; +LL + let _ = difference; LL + } LL + } | error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:75:9 + --> tests/ui/manual_checked_sub.rs:89:9 | LL | / if c > 0 { LL | | @@ -201,11 +241,11 @@ LL | | } | help: consider using `.checked_sub()` | -LL ~ if let Some(result) = c.checked_sub(1) { +LL ~ if let Some(decremented) = c.checked_sub(1) { LL + -LL + let _ = result; +LL + let _ = decremented; LL + } | -error: aborting due to 11 previous errors +error: aborting due to 13 previous errors From 510d8474c83b97ca1ddff2caf5fa12f031f3c591 Mon Sep 17 00:00:00 2001 From: benacq Date: Tue, 8 Apr 2025 07:31:48 +0000 Subject: [PATCH 20/26] manual_checked_sub implementation review feedback fixes - Variable generator function --- clippy_lints/src/manual_checked_sub.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 6ba282618da7..7ed9967f51f4 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -298,7 +298,7 @@ fn generate_unique_var_name<'tcx>(base_name: &str, scope_expr: &'tcx Expr<'tcx>) // Keep incrementing until we find a name that's not in scope let mut counter = 1; loop { - let candidate = format!("{}_{}", base_name, counter); + let candidate = format!("{base_name}_{counter}"); let mut candidate_visitor = VarNameVisitor { base_name: &candidate, From 8d8dece3cefa177d46cde691fd71d14744b22d29 Mon Sep 17 00:00:00 2001 From: benacq Date: Tue, 8 Apr 2025 07:32:46 +0000 Subject: [PATCH 21/26] manual_checked_sub implementation review feedback fixes - Variable generator function --- clippy_lints/src/manual_checked_sub.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 7ed9967f51f4..513fae22679b 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -259,12 +259,12 @@ fn generate_unique_var_name<'tcx>(base_name: &str, scope_expr: &'tcx Expr<'tcx>) } fn visit_stmt(&mut self, stmt: &'tcx rustc_hir::Stmt<'_>) { - if let rustc_hir::StmtKind::Let(let_stmt) = &stmt.kind { - if let PatKind::Binding(_, _, ident, _) = &let_stmt.pat.kind { - let name = ident.name.to_string(); - if name == self.base_name { - self.is_name_in_scope = true; - } + if let rustc_hir::StmtKind::Let(let_stmt) = &stmt.kind + && let PatKind::Binding(_, _, ident, _) = &let_stmt.pat.kind + { + let name = ident.name.to_string(); + if name == self.base_name { + self.is_name_in_scope = true; } } From 3f42120c624e15c88d165d2cc6ac614090153691 Mon Sep 17 00:00:00 2001 From: benacq Date: Tue, 8 Apr 2025 07:33:20 +0000 Subject: [PATCH 22/26] manual_checked_sub implementation review feedback fixes - Variable generator function --- clippy_lints/src/manual_checked_sub.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index 513fae22679b..d7dd98698130 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -236,6 +236,7 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<' false } + /// Generates a unique variable name based on the provided base name. /// If the base name already exists in the scope, appends a number to make it unique. fn generate_unique_var_name<'tcx>(base_name: &str, scope_expr: &'tcx Expr<'tcx>) -> String { @@ -246,12 +247,12 @@ fn generate_unique_var_name<'tcx>(base_name: &str, scope_expr: &'tcx Expr<'tcx>) impl<'tcx> Visitor<'tcx> for VarNameVisitor<'_> { fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { - if let Some(segment) = path.segments.last() { - let name = segment.ident.name.to_string(); - if name == self.base_name { - self.is_name_in_scope = true; - } + if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind + && let Some(segment) = path.segments.last() + { + let name = segment.ident.name.to_string(); + if name == self.base_name { + self.is_name_in_scope = true; } } From 3cb4e861ba68ace6f6e31dc54016086a830addf5 Mon Sep 17 00:00:00 2001 From: benacq Date: Wed, 23 Apr 2025 20:10:05 +0000 Subject: [PATCH 23/26] Macro call guard and lint suggestion refactor --- clippy_lints/src/manual_checked_sub.rs | 181 ++++--------------------- tests/ui/manual_checked_sub.fixed | 137 ------------------- tests/ui/manual_checked_sub.rs | 32 +++-- tests/ui/manual_checked_sub.stderr | 161 +++------------------- 4 files changed, 61 insertions(+), 450 deletions(-) delete mode 100644 tests/ui/manual_checked_sub.fixed diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index d7dd98698130..df311c45f40e 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -1,18 +1,15 @@ use clippy_config::Conf; -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::span_lint; use clippy_utils::msrvs::{self, Msrv}; -use clippy_utils::source::snippet_with_applicability; -use clippy_utils::sugg::Sugg; + use clippy_utils::{is_mutable, path_to_local}; use rustc_ast::{BinOpKind, LitIntType, LitKind}; use rustc_data_structures::packed::Pu128; -use rustc_errors::Applicability; -use rustc_hir::intravisit::{Visitor, walk_expr, walk_stmt}; -use rustc_hir::{Expr, ExprKind, PatKind, QPath}; +use rustc_hir::intravisit::{Visitor, walk_expr}; +use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self}; use rustc_session::impl_lint_pass; -use std::fmt::Write; declare_clippy_lint! { /// ### What it does @@ -79,12 +76,16 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { return; } + // Skip if either lhs or rhs is a macro call + if lhs.span.from_expansion() || rhs.span.from_expansion() { + return; + } + if let BinOpKind::Ge | BinOpKind::Gt | BinOpKind::Le | BinOpKind::Lt = op.node { SubExprVisitor { cx, if_expr: expr, - if_body_block: if_expr.then, - else_block: if_expr.r#else, + condition_lhs: lhs, condition_rhs: rhs, condition_op: op.node, @@ -98,8 +99,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub { struct SubExprVisitor<'cx, 'tcx> { cx: &'cx LateContext<'tcx>, if_expr: &'tcx Expr<'tcx>, - if_body_block: &'tcx Expr<'tcx>, - else_block: Option<&'tcx Expr<'tcx>>, + condition_lhs: &'tcx Expr<'tcx>, condition_rhs: &'tcx Expr<'tcx>, condition_op: BinOpKind, @@ -110,12 +110,17 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { if let ExprKind::Binary(op, sub_lhs, sub_rhs) = expr.kind && let BinOpKind::Sub = op.node { + // // Skip if either sub_lhs or sub_rhs is a macro call + if sub_lhs.span.from_expansion() || sub_rhs.span.from_expansion() { + return; + } + if let ExprKind::Lit(lit) = self.condition_lhs.kind && self.condition_op == BinOpKind::Lt && let LitKind::Int(Pu128(0), _) = lit.node && (is_referencing_same_variable(sub_lhs, self.condition_rhs)) { - self.emit_lint(expr, Some(sub_rhs)); + self.emit_lint(); } if let ExprKind::Lit(lit) = self.condition_rhs.kind @@ -123,7 +128,7 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { && let LitKind::Int(Pu128(0), _) = lit.node && (is_referencing_same_variable(sub_lhs, self.condition_lhs)) { - self.emit_lint(expr, Some(sub_rhs)); + self.emit_lint(); } if self.condition_op == BinOpKind::Ge @@ -132,7 +137,7 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { && (is_referencing_same_variable(sub_rhs, self.condition_rhs) || are_literals_equal(sub_rhs, self.condition_rhs)) { - self.emit_lint(expr, None); + self.emit_lint(); } if self.condition_op == BinOpKind::Le @@ -141,7 +146,7 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { && (is_referencing_same_variable(sub_rhs, self.condition_lhs) || are_literals_equal(sub_rhs, self.condition_lhs)) { - self.emit_lint(expr, None); + self.emit_lint(); } } @@ -150,62 +155,12 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { } impl<'tcx> SubExprVisitor<'_, 'tcx> { - fn emit_lint(&mut self, expr: &Expr<'tcx>, sub_rhs_expr: Option<&'tcx Expr<'tcx>>) { - let mut applicability = Applicability::MachineApplicable; - - let body_snippet = snippet_with_applicability(self.cx, self.if_body_block.span, "..", &mut applicability); - - let checked_sub_result_var_name = match self.condition_op { - BinOpKind::Lt | BinOpKind::Gt => generate_unique_var_name("decremented", self.if_body_block), - _ => generate_unique_var_name("difference", self.if_body_block), - }; - - let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut applicability); - let updated_usage_context_snippet = body_snippet - .as_ref() - .replace(binary_expr_snippet.as_ref(), &checked_sub_result_var_name); - - let lhs_snippet = Sugg::hir(self.cx, self.condition_lhs, "..").maybe_paren(); - - let rhs_snippet = match sub_rhs_expr { - Some(sub_rhs) => Sugg::hir(self.cx, sub_rhs, "..").maybe_paren(), - None => Sugg::hir(self.cx, self.condition_rhs, "..").maybe_paren(), - }; - - // TODO: Check result variable is not already in scope - let mut suggestion = match self.condition_op { - BinOpKind::Le => { - format!( - "if let Some({checked_sub_result_var_name}) = {rhs_snippet}.checked_sub({lhs_snippet}) {updated_usage_context_snippet}" - ) - }, - - BinOpKind::Lt => { - format!( - "if let Some({checked_sub_result_var_name}) = {}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}", - Sugg::hir(self.cx, self.condition_rhs, "..").maybe_paren() - ) - }, - _ => { - format!( - "if let Some({checked_sub_result_var_name}) = {lhs_snippet}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}" - ) - }, - }; - - if let Some(else_expr) = self.else_block { - let else_snippet = snippet_with_applicability(self.cx, else_expr.span, "..", &mut applicability); - write!(suggestion, " else {else_snippet}").unwrap(); - } - - span_lint_and_sugg( + fn emit_lint(&mut self) { + span_lint( self.cx, MANUAL_CHECKED_SUB, self.if_expr.span, - "manual re-implementation of checked subtraction", - "consider using `.checked_sub()`", - suggestion, - applicability, + "manual re-implementation of checked subtraction - consider using `.checked_sub()`", ); } } @@ -236,93 +191,3 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<' false } - -/// Generates a unique variable name based on the provided base name. -/// If the base name already exists in the scope, appends a number to make it unique. -fn generate_unique_var_name<'tcx>(base_name: &str, scope_expr: &'tcx Expr<'tcx>) -> String { - struct VarNameVisitor<'cx> { - base_name: &'cx str, - is_name_in_scope: bool, - } - - impl<'tcx> Visitor<'tcx> for VarNameVisitor<'_> { - fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind - && let Some(segment) = path.segments.last() - { - let name = segment.ident.name.to_string(); - if name == self.base_name { - self.is_name_in_scope = true; - } - } - - walk_expr(self, expr); - } - - fn visit_stmt(&mut self, stmt: &'tcx rustc_hir::Stmt<'_>) { - if let rustc_hir::StmtKind::Let(let_stmt) = &stmt.kind - && let PatKind::Binding(_, _, ident, _) = &let_stmt.pat.kind - { - let name = ident.name.to_string(); - if name == self.base_name { - self.is_name_in_scope = true; - } - } - - walk_stmt(self, stmt); - } - } - - let mut visitor = VarNameVisitor { - base_name, - is_name_in_scope: false, - }; - - if let ExprKind::Block(block, _) = &scope_expr.kind { - for stmt in block.stmts { - visitor.visit_stmt(stmt); - } - - // Visit the optional expression at the end of the block - if let Some(expr) = block.expr { - visitor.visit_expr(expr); - } - } else { - visitor.visit_expr(scope_expr); - } - - if !visitor.is_name_in_scope { - return base_name.to_string(); - } - - // If the base name is in scope, append a number - // Keep incrementing until we find a name that's not in scope - let mut counter = 1; - loop { - let candidate = format!("{base_name}_{counter}"); - - let mut candidate_visitor = VarNameVisitor { - base_name: &candidate, - is_name_in_scope: false, - }; - - // Check if the candidate name is in scope - if let ExprKind::Block(block, _) = &scope_expr.kind { - for stmt in block.stmts { - candidate_visitor.visit_stmt(stmt); - } - - if let Some(expr) = block.expr { - candidate_visitor.visit_expr(expr); - } - } else { - candidate_visitor.visit_expr(scope_expr); - } - - if !candidate_visitor.is_name_in_scope { - return candidate; - } - - counter += 1; - } -} diff --git a/tests/ui/manual_checked_sub.fixed b/tests/ui/manual_checked_sub.fixed deleted file mode 100644 index efe146050f66..000000000000 --- a/tests/ui/manual_checked_sub.fixed +++ /dev/null @@ -1,137 +0,0 @@ -#![allow(clippy::collapsible_if)] -#![warn(clippy::manual_checked_sub)] - -fn positive_tests() { - let a: u32 = 10; - let b: u32 = 5; - let c: u32 = 3; - - if let Some(difference_1) = a.checked_sub(b) { - //~^ manual_checked_sub - let difference = "some test"; - let d = difference_1; - } - - if let Some(difference) = a.checked_sub(b) { - //~^ manual_checked_sub - let d = difference; - } - - if let Some(difference) = a.checked_sub(b) { - //~^ manual_checked_sub - let d = difference; - } else { - let d = a + b; - } - - if let Some(difference) = a.checked_sub(b) { - //~^ manual_checked_sub - let d = difference; - } else if a < b { - let d = a + b; - } - - if let Some(decremented) = a.checked_sub(1) { - //~^ manual_checked_sub - let _ = decremented; - } - - if let Some(decremented) = a.checked_sub(1) { - //~^ manual_checked_sub - let _ = decremented; - } - - let mut difference = "variable initialized outside the if scope"; - if let Some(difference_1) = a.checked_sub(b) { - //~^ manual_checked_sub - let some_reference = difference; - let d = difference_1; - } - - if let Some(decremented_1) = a.checked_sub(b) { - //~^ manual_checked_sub - let decremented = "variable initialized inside the if scope"; - let d = decremented_1; - } - - if let Some(difference) = a.checked_sub(b) { - //~^ manual_checked_sub - some_function(difference); - } - - if let Some(difference) = a.checked_sub(b) { - //~^ manual_checked_sub - macro_rules! subtract { - () => { - difference - }; - } - let _ = subtract!(); - } - - struct Example { - value: u32, - } - - if let Some(difference) = a.checked_sub(b) { - //~^ manual_checked_sub - let _ = Example { value: difference }; - } - - if let Some(difference) = a.checked_sub(b) { - //~^ manual_checked_sub - if c > 0 { - let _ = difference; - } - } - - if a >= b { - if let Some(decremented) = c.checked_sub(1) { - //~^ manual_checked_sub - let _ = decremented; - } - } -} - -fn some_function(a: u32) {} - -fn negative_tests() { - let a: u32 = 10; - let b: u32 = 5; - - let x: i32 = 10; - let y: i32 = 5; - - let mut a_mut = 10; - - if a_mut >= b { - a_mut *= 2; - let c = a_mut - b; - } - - if a_mut > 0 { - a_mut *= 2; - let c = a_mut - 1; - } - - if x >= y { - let _ = x - y; - } - - if a == b { - let _ = a - b; - } - - if a > b { - let _ = a - b; - } - - if 10 >= 5 { - let _ = 10 - 5; - } -} - -fn main() { - positive_tests(); - negative_tests(); -} diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs index 938485957c57..7742d4fd8a2d 100644 --- a/tests/ui/manual_checked_sub.rs +++ b/tests/ui/manual_checked_sub.rs @@ -59,16 +59,6 @@ fn positive_tests() { some_function(a - b); } - if a >= b { - //~^ manual_checked_sub - macro_rules! subtract { - () => { - a - b - }; - } - let _ = subtract!(); - } - struct Example { value: u32, } @@ -129,6 +119,28 @@ fn negative_tests() { if 10 >= 5 { let _ = 10 - 5; } + + macro_rules! id { + ($e:ident) => { + $e + }; + } + if id!(a) >= id!(b) { + let d = id!(a) - id!(b); + } + + if a >= b { + let d = id!(a) - id!(b); + } + + if a >= b { + macro_rules! subtract { + () => { + a - b + }; + } + let _ = subtract!(); + } } fn main() { diff --git a/tests/ui/manual_checked_sub.stderr b/tests/ui/manual_checked_sub.stderr index 73eadfe7d328..8dba0c8109c8 100644 --- a/tests/ui/manual_checked_sub.stderr +++ b/tests/ui/manual_checked_sub.stderr @@ -1,4 +1,4 @@ -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:9:5 | LL | / if a >= b { @@ -10,16 +10,8 @@ LL | | } | = note: `-D clippy::manual-checked-sub` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_checked_sub)]` -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference_1) = a.checked_sub(b) { -LL + -LL + let difference = "some test"; -LL + let d = difference_1; -LL + } - | -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:15:5 | LL | / if b <= a { @@ -27,16 +19,8 @@ LL | | LL | | let d = a - b; LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference) = a.checked_sub(b) { -LL + -LL + let d = difference; -LL + } - | -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:20:5 | LL | / if a >= b { @@ -46,18 +30,8 @@ LL | | } else { LL | | let d = a + b; LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference) = a.checked_sub(b) { -LL + -LL + let d = difference; -LL + } else { -LL + let d = a + b; -LL + } - | -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:27:5 | LL | / if a >= b { @@ -67,18 +41,8 @@ LL | | } else if a < b { LL | | let d = a + b; LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference) = a.checked_sub(b) { -LL + -LL + let d = difference; -LL + } else if a < b { -LL + let d = a + b; -LL + } - | -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:34:5 | LL | / if a > 0 { @@ -86,16 +50,8 @@ LL | | LL | | let _ = a - 1; LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(decremented) = a.checked_sub(1) { -LL + -LL + let _ = decremented; -LL + } - | -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:39:5 | LL | / if 0 < a { @@ -103,16 +59,8 @@ LL | | LL | | let _ = a - 1; LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(decremented) = a.checked_sub(1) { -LL + -LL + let _ = decremented; -LL + } - | -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:45:5 | LL | / if a >= b { @@ -121,17 +69,8 @@ LL | | let some_reference = difference; LL | | let d = a - b; LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference_1) = a.checked_sub(b) { -LL + -LL + let some_reference = difference; -LL + let d = difference_1; -LL + } - | -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:51:5 | LL | / if a > 0 { @@ -140,17 +79,8 @@ LL | | let decremented = "variable initialized inside the if scope"; LL | | let d = a - b; LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(decremented_1) = a.checked_sub(b) { -LL + -LL + let decremented = "variable initialized inside the if scope"; -LL + let d = decremented_1; -LL + } - | -error: manual re-implementation of checked subtraction +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` --> tests/ui/manual_checked_sub.rs:57:5 | LL | / if a >= b { @@ -158,59 +88,18 @@ LL | | LL | | some_function(a - b); LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference) = a.checked_sub(b) { -LL + -LL + some_function(difference); -LL + } - | - -error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:62:5 - | -LL | / if a >= b { -LL | | -LL | | macro_rules! subtract { -LL | | () => { -... | -LL | | let _ = subtract!(); -LL | | } - | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference) = a.checked_sub(b) { -LL + -LL + macro_rules! subtract { -LL + () => { -LL + difference -LL + }; -LL + } -LL + let _ = subtract!(); -LL + } - | -error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:76:5 +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` + --> tests/ui/manual_checked_sub.rs:66:5 | LL | / if a >= b { LL | | LL | | let _ = Example { value: a - b }; LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference) = a.checked_sub(b) { -LL + -LL + let _ = Example { value: difference }; -LL + } - | -error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:81:5 +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` + --> tests/ui/manual_checked_sub.rs:71:5 | LL | / if a >= b { LL | | @@ -219,33 +108,15 @@ LL | | let _ = a - b; LL | | } LL | | } | |_____^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(difference) = a.checked_sub(b) { -LL + -LL + if c > 0 { -LL + let _ = difference; -LL + } -LL + } - | -error: manual re-implementation of checked subtraction - --> tests/ui/manual_checked_sub.rs:89:9 +error: manual re-implementation of checked subtraction - consider using `.checked_sub()` + --> tests/ui/manual_checked_sub.rs:79:9 | LL | / if c > 0 { LL | | LL | | let _ = c - 1; LL | | } | |_________^ - | -help: consider using `.checked_sub()` - | -LL ~ if let Some(decremented) = c.checked_sub(1) { -LL + -LL + let _ = decremented; -LL + } - | -error: aborting due to 13 previous errors +error: aborting due to 12 previous errors From b1f33407144c8fceb603b37d0625a6b88b762387 Mon Sep 17 00:00:00 2001 From: benacq Date: Thu, 24 Apr 2025 08:05:30 +0000 Subject: [PATCH 24/26] Revert "Macro guard implementation and lint suggestion refector" This reverts commit ae4d4939eab3c85772a09df458a88b579966eeaa. --- clippy_lints/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index dc843eddcbce..c856f0187412 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -943,7 +943,8 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(non_std_lazy_statics::NonStdLazyStatic::new(conf))); store.register_late_pass(|_| Box::new(manual_option_as_slice::ManualOptionAsSlice::new(conf))); store.register_late_pass(|_| Box::new(single_option_map::SingleOptionMap)); - store.register_late_pass(move |_| Box::new(redundant_test_prefix::RedundantTestPrefix)); store.register_late_pass(move |_| Box::new(manual_checked_sub::ManualCheckedSub::new(conf))); + store.register_late_pass(move |_| Box::new(redundant_test_prefix::RedundantTestPrefix)); + // add lints here, do not remove this comment, it's used in `new_lint` } From e61986c04abdd43175e5e5b30211524e55eddbb1 Mon Sep 17 00:00:00 2001 From: benacq Date: Thu, 24 Apr 2025 09:17:11 +0000 Subject: [PATCH 25/26] Macro guard implementation and lint suggestion refactor --- tests/ui/manual_abs_diff.fixed | 2 +- tests/ui/manual_abs_diff.rs | 2 +- tests/ui/manual_checked_sub.rs | 1 + tests/ui/manual_checked_sub.stderr | 24 ++++++++++++------------ 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/tests/ui/manual_abs_diff.fixed b/tests/ui/manual_abs_diff.fixed index f1b1278ea6d2..57eb303f0977 100644 --- a/tests/ui/manual_abs_diff.fixed +++ b/tests/ui/manual_abs_diff.fixed @@ -1,5 +1,5 @@ +#![allow(clippy::manual_checked_sub)] #![warn(clippy::manual_abs_diff)] - use std::time::Duration; fn main() { diff --git a/tests/ui/manual_abs_diff.rs b/tests/ui/manual_abs_diff.rs index 60ef819c12d3..ee9c8298ab98 100644 --- a/tests/ui/manual_abs_diff.rs +++ b/tests/ui/manual_abs_diff.rs @@ -1,5 +1,5 @@ +#![allow(clippy::manual_checked_sub)] #![warn(clippy::manual_abs_diff)] - use std::time::Duration; fn main() { diff --git a/tests/ui/manual_checked_sub.rs b/tests/ui/manual_checked_sub.rs index 7742d4fd8a2d..167a8655f7ee 100644 --- a/tests/ui/manual_checked_sub.rs +++ b/tests/ui/manual_checked_sub.rs @@ -1,4 +1,5 @@ #![allow(clippy::collapsible_if)] +// #![allow(clippy::manual_abs_diff)] #![warn(clippy::manual_checked_sub)] fn positive_tests() { diff --git a/tests/ui/manual_checked_sub.stderr b/tests/ui/manual_checked_sub.stderr index 8dba0c8109c8..a487ec7dbd9c 100644 --- a/tests/ui/manual_checked_sub.stderr +++ b/tests/ui/manual_checked_sub.stderr @@ -1,5 +1,5 @@ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:9:5 + --> tests/ui/manual_checked_sub.rs:10:5 | LL | / if a >= b { LL | | @@ -12,7 +12,7 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::manual_checked_sub)]` error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:15:5 + --> tests/ui/manual_checked_sub.rs:16:5 | LL | / if b <= a { LL | | @@ -21,7 +21,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:20:5 + --> tests/ui/manual_checked_sub.rs:21:5 | LL | / if a >= b { LL | | @@ -32,7 +32,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:27:5 + --> tests/ui/manual_checked_sub.rs:28:5 | LL | / if a >= b { LL | | @@ -43,7 +43,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:34:5 + --> tests/ui/manual_checked_sub.rs:35:5 | LL | / if a > 0 { LL | | @@ -52,7 +52,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:39:5 + --> tests/ui/manual_checked_sub.rs:40:5 | LL | / if 0 < a { LL | | @@ -61,7 +61,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:45:5 + --> tests/ui/manual_checked_sub.rs:46:5 | LL | / if a >= b { LL | | @@ -71,7 +71,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:51:5 + --> tests/ui/manual_checked_sub.rs:52:5 | LL | / if a > 0 { LL | | @@ -81,7 +81,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:57:5 + --> tests/ui/manual_checked_sub.rs:58:5 | LL | / if a >= b { LL | | @@ -90,7 +90,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:66:5 + --> tests/ui/manual_checked_sub.rs:67:5 | LL | / if a >= b { LL | | @@ -99,7 +99,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:71:5 + --> tests/ui/manual_checked_sub.rs:72:5 | LL | / if a >= b { LL | | @@ -110,7 +110,7 @@ LL | | } | |_____^ error: manual re-implementation of checked subtraction - consider using `.checked_sub()` - --> tests/ui/manual_checked_sub.rs:79:9 + --> tests/ui/manual_checked_sub.rs:80:9 | LL | / if c > 0 { LL | | From 090496edef50e58a5e2476bb5c4f9a101194e66a Mon Sep 17 00:00:00 2001 From: benacq Date: Thu, 24 Apr 2025 12:17:06 +0000 Subject: [PATCH 26/26] Macro guard implementation and lint suggestion refactor --- clippy_lints/src/manual_checked_sub.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_checked_sub.rs b/clippy_lints/src/manual_checked_sub.rs index df311c45f40e..a862b01a9225 100644 --- a/clippy_lints/src/manual_checked_sub.rs +++ b/clippy_lints/src/manual_checked_sub.rs @@ -154,7 +154,7 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> { } } -impl<'tcx> SubExprVisitor<'_, 'tcx> { +impl SubExprVisitor<'_, '_> { fn emit_lint(&mut self) { span_lint( self.cx,