Auto merge of #12476 - GuillaumeGomez:add-manual_arithmetic_check, r=y21

Extend `implicit_saturating_sub` lint

Fixes #10070.

It can serve as base if we want to add equivalent checks for other arithmetic operations.

Also one important note: when writing this lint, I realized that I could check for wrong conditions performed beforehand on subtraction and added another part in the lint. Considering they both rely on the same checks, I kept both in the same place. Not sure if it makes sense though...

changelog: Extend `implicit_saturating_sub` lint
This commit is contained in:
bors 2024-08-31 16:03:42 +00:00
commit ac914d3457
12 changed files with 511 additions and 73 deletions

View file

@ -5500,6 +5500,7 @@ Released 2018-09-13
[`invalid_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_regex
[`invalid_upcast_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_upcast_comparisons
[`invalid_utf8_in_unchecked`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_utf8_in_unchecked
[`inverted_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#inverted_saturating_sub
[`invisible_characters`]: https://rust-lang.github.io/rust-clippy/master/index.html#invisible_characters
[`is_digit_ascii_radix`]: https://rust-lang.github.io/rust-clippy/master/index.html#is_digit_ascii_radix
[`items_after_statements`]: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_statements

View file

@ -36,7 +36,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 }
1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN }
1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST }
1,46,0 { CONST_IF_MATCH }
1,45,0 { STR_STRIP_PREFIX }
1,43,0 { LOG2_10, LOG10_2, NUMERIC_ASSOCIATED_CONSTANTS }

View file

@ -217,6 +217,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::implicit_return::IMPLICIT_RETURN_INFO,
crate::implicit_saturating_add::IMPLICIT_SATURATING_ADD_INFO,
crate::implicit_saturating_sub::IMPLICIT_SATURATING_SUB_INFO,
crate::implicit_saturating_sub::INVERTED_SATURATING_SUB_INFO,
crate::implied_bounds_in_impls::IMPLIED_BOUNDS_IN_IMPLS_INFO,
crate::incompatible_msrv::INCOMPATIBLE_MSRV_INFO,
crate::inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR_INFO,

View file

@ -1,11 +1,17 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::{higher, is_integer_literal, peel_blocks_with_stmt, SpanlessEq};
use clippy_config::msrvs::{self, Msrv};
use clippy_config::Conf;
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::snippet_opt;
use clippy_utils::{
higher, is_in_const_context, is_integer_literal, path_to_local, peel_blocks, peel_blocks_with_stmt, SpanlessEq,
};
use rustc_ast::ast::LitKind;
use rustc_data_structures::packed::Pu128;
use rustc_errors::Applicability;
use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
use rustc_hir::{BinOp, BinOpKind, Expr, ExprKind, HirId, QPath};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::declare_lint_pass;
use rustc_session::impl_lint_pass;
use rustc_span::Span;
declare_clippy_lint! {
/// ### What it does
@ -39,7 +45,49 @@ declare_clippy_lint! {
"Perform saturating subtraction instead of implicitly checking lower bound of data type"
}
declare_lint_pass!(ImplicitSaturatingSub => [IMPLICIT_SATURATING_SUB]);
declare_clippy_lint! {
/// ### What it does
/// Checks for comparisons between integers, followed by subtracting the greater value from the
/// lower one.
///
/// ### Why is this bad?
/// This could result in an underflow and is most likely not what the user wants. If this was
/// intended to be a saturated subtraction, consider using the `saturating_sub` method directly.
///
/// ### Example
/// ```no_run
/// let a = 12u32;
/// let b = 13u32;
///
/// let result = if a > b { b - a } else { 0 };
/// ```
///
/// Use instead:
/// ```no_run
/// let a = 12u32;
/// let b = 13u32;
///
/// let result = a.saturating_sub(b);
/// ```
#[clippy::version = "1.44.0"]
pub INVERTED_SATURATING_SUB,
correctness,
"Check if a variable is smaller than another one and still subtract from it even if smaller"
}
pub struct ImplicitSaturatingSub {
msrv: Msrv,
}
impl_lint_pass!(ImplicitSaturatingSub => [IMPLICIT_SATURATING_SUB, INVERTED_SATURATING_SUB]);
impl ImplicitSaturatingSub {
pub fn new(conf: &'static Conf) -> Self {
Self {
msrv: conf.msrv.clone(),
}
}
}
impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingSub {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
@ -50,73 +98,260 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingSub {
// Check if the conditional expression is a binary operation
&& let ExprKind::Binary(ref cond_op, cond_left, cond_right) = cond.kind
// Ensure that the binary operator is >, !=, or <
&& (BinOpKind::Ne == cond_op.node || BinOpKind::Gt == cond_op.node || BinOpKind::Lt == cond_op.node)
// Check if assign operation is done
&& let Some(target) = subtracts_one(cx, then)
// Extracting out the variable name
&& let ExprKind::Path(QPath::Resolved(_, ares_path)) = target.kind
{
// Handle symmetric conditions in the if statement
let (cond_var, cond_num_val) = if SpanlessEq::new(cx).eq_expr(cond_left, target) {
if BinOpKind::Gt == cond_op.node || BinOpKind::Ne == cond_op.node {
(cond_left, cond_right)
} else {
return;
}
} else if SpanlessEq::new(cx).eq_expr(cond_right, target) {
if BinOpKind::Lt == cond_op.node || BinOpKind::Ne == cond_op.node {
(cond_right, cond_left)
} else {
return;
}
check_with_condition(cx, expr, cond_op.node, cond_left, cond_right, then);
} else if let Some(higher::If {
cond,
then: if_block,
r#else: Some(else_block),
}) = higher::If::hir(expr)
&& let ExprKind::Binary(ref cond_op, cond_left, cond_right) = cond.kind
{
check_manual_check(
cx, expr, cond_op, cond_left, cond_right, if_block, else_block, &self.msrv,
);
}
}
extract_msrv_attr!(LateContext);
}
#[allow(clippy::too_many_arguments)]
fn check_manual_check<'tcx>(
cx: &LateContext<'tcx>,
expr: &Expr<'tcx>,
condition: &BinOp,
left_hand: &Expr<'tcx>,
right_hand: &Expr<'tcx>,
if_block: &Expr<'tcx>,
else_block: &Expr<'tcx>,
msrv: &Msrv,
) {
let ty = cx.typeck_results().expr_ty(left_hand);
if ty.is_numeric() && !ty.is_signed() {
match condition.node {
BinOpKind::Gt | BinOpKind::Ge => check_gt(
cx,
condition.span,
expr.span,
left_hand,
right_hand,
if_block,
else_block,
msrv,
),
BinOpKind::Lt | BinOpKind::Le => check_gt(
cx,
condition.span,
expr.span,
right_hand,
left_hand,
if_block,
else_block,
msrv,
),
_ => {},
}
}
}
#[allow(clippy::too_many_arguments)]
fn check_gt(
cx: &LateContext<'_>,
condition_span: Span,
expr_span: Span,
big_var: &Expr<'_>,
little_var: &Expr<'_>,
if_block: &Expr<'_>,
else_block: &Expr<'_>,
msrv: &Msrv,
) {
if let Some(big_var) = Var::new(big_var)
&& let Some(little_var) = Var::new(little_var)
{
check_subtraction(
cx,
condition_span,
expr_span,
big_var,
little_var,
if_block,
else_block,
msrv,
);
}
}
struct Var {
span: Span,
hir_id: HirId,
}
impl Var {
fn new(expr: &Expr<'_>) -> Option<Self> {
path_to_local(expr).map(|hir_id| Self {
span: expr.span,
hir_id,
})
}
}
#[allow(clippy::too_many_arguments)]
fn check_subtraction(
cx: &LateContext<'_>,
condition_span: Span,
expr_span: Span,
big_var: Var,
little_var: Var,
if_block: &Expr<'_>,
else_block: &Expr<'_>,
msrv: &Msrv,
) {
let if_block = peel_blocks(if_block);
let else_block = peel_blocks(else_block);
if is_integer_literal(if_block, 0) {
// We need to check this case as well to prevent infinite recursion.
if is_integer_literal(else_block, 0) {
// Well, seems weird but who knows?
return;
}
// If the subtraction is done in the `else` block, then we need to also revert the two
// variables as it means that the check was reverted too.
check_subtraction(
cx,
condition_span,
expr_span,
little_var,
big_var,
else_block,
if_block,
msrv,
);
return;
}
if is_integer_literal(else_block, 0)
&& let ExprKind::Binary(op, left, right) = if_block.kind
&& let BinOpKind::Sub = op.node
{
let local_left = path_to_local(left);
let local_right = path_to_local(right);
if Some(big_var.hir_id) == local_left && Some(little_var.hir_id) == local_right {
// This part of the condition is voluntarily split from the one before to ensure that
// if `snippet_opt` fails, it won't try the next conditions.
if let Some(big_var_snippet) = snippet_opt(cx, big_var.span)
&& let Some(little_var_snippet) = snippet_opt(cx, little_var.span)
&& (!is_in_const_context(cx) || msrv.meets(msrvs::SATURATING_SUB_CONST))
{
span_lint_and_sugg(
cx,
IMPLICIT_SATURATING_SUB,
expr_span,
"manual arithmetic check found",
"replace it with",
format!("{big_var_snippet}.saturating_sub({little_var_snippet})"),
Applicability::MachineApplicable,
);
}
} else if Some(little_var.hir_id) == local_left
&& Some(big_var.hir_id) == local_right
&& let Some(big_var_snippet) = snippet_opt(cx, big_var.span)
&& let Some(little_var_snippet) = snippet_opt(cx, little_var.span)
{
span_lint_and_then(
cx,
INVERTED_SATURATING_SUB,
condition_span,
"inverted arithmetic check before subtraction",
|diag| {
diag.span_note(
if_block.span,
format!("this subtraction underflows when `{little_var_snippet} < {big_var_snippet}`"),
);
diag.span_suggestion(
if_block.span,
"try replacing it with",
format!("{big_var_snippet} - {little_var_snippet}"),
Applicability::MaybeIncorrect,
);
},
);
}
}
}
fn check_with_condition<'tcx>(
cx: &LateContext<'tcx>,
expr: &Expr<'tcx>,
cond_op: BinOpKind,
cond_left: &Expr<'tcx>,
cond_right: &Expr<'tcx>,
then: &Expr<'tcx>,
) {
// Ensure that the binary operator is >, !=, or <
if (BinOpKind::Ne == cond_op || BinOpKind::Gt == cond_op || BinOpKind::Lt == cond_op)
// Check if assign operation is done
&& let Some(target) = subtracts_one(cx, then)
// Extracting out the variable name
&& let ExprKind::Path(QPath::Resolved(_, ares_path)) = target.kind
{
// Handle symmetric conditions in the if statement
let (cond_var, cond_num_val) = if SpanlessEq::new(cx).eq_expr(cond_left, target) {
if BinOpKind::Gt == cond_op || BinOpKind::Ne == cond_op {
(cond_left, cond_right)
} else {
return;
};
// Check if the variable in the condition statement is an integer
if !cx.typeck_results().expr_ty(cond_var).is_integral() {
}
} else if SpanlessEq::new(cx).eq_expr(cond_right, target) {
if BinOpKind::Lt == cond_op || BinOpKind::Ne == cond_op {
(cond_right, cond_left)
} else {
return;
}
} else {
return;
};
// Get the variable name
let var_name = ares_path.segments[0].ident.name.as_str();
match cond_num_val.kind {
ExprKind::Lit(cond_lit) => {
// Check if the constant is zero
if let LitKind::Int(Pu128(0), _) = cond_lit.node {
if cx.typeck_results().expr_ty(cond_left).is_signed() {
} else {
print_lint_and_sugg(cx, var_name, expr);
};
}
},
ExprKind::Path(QPath::TypeRelative(_, name)) => {
if name.ident.as_str() == "MIN"
&& let Some(const_id) = cx.typeck_results().type_dependent_def_id(cond_num_val.hir_id)
&& let Some(impl_id) = cx.tcx.impl_of_method(const_id)
&& let None = cx.tcx.impl_trait_ref(impl_id) // An inherent impl
&& cx.tcx.type_of(impl_id).instantiate_identity().is_integral()
{
// Check if the variable in the condition statement is an integer
if !cx.typeck_results().expr_ty(cond_var).is_integral() {
return;
}
// Get the variable name
let var_name = ares_path.segments[0].ident.name.as_str();
match cond_num_val.kind {
ExprKind::Lit(cond_lit) => {
// Check if the constant is zero
if let LitKind::Int(Pu128(0), _) = cond_lit.node {
if cx.typeck_results().expr_ty(cond_left).is_signed() {
} else {
print_lint_and_sugg(cx, var_name, expr);
}
},
ExprKind::Call(func, []) => {
if let ExprKind::Path(QPath::TypeRelative(_, name)) = func.kind
&& name.ident.as_str() == "min_value"
&& let Some(func_id) = cx.typeck_results().type_dependent_def_id(func.hir_id)
&& let Some(impl_id) = cx.tcx.impl_of_method(func_id)
&& let None = cx.tcx.impl_trait_ref(impl_id) // An inherent impl
&& cx.tcx.type_of(impl_id).instantiate_identity().is_integral()
{
print_lint_and_sugg(cx, var_name, expr);
}
},
_ => (),
}
};
}
},
ExprKind::Path(QPath::TypeRelative(_, name)) => {
if name.ident.as_str() == "MIN"
&& let Some(const_id) = cx.typeck_results().type_dependent_def_id(cond_num_val.hir_id)
&& let Some(impl_id) = cx.tcx.impl_of_method(const_id)
&& let None = cx.tcx.impl_trait_ref(impl_id) // An inherent impl
&& cx.tcx.type_of(impl_id).instantiate_identity().is_integral()
{
print_lint_and_sugg(cx, var_name, expr);
}
},
ExprKind::Call(func, []) => {
if let ExprKind::Path(QPath::TypeRelative(_, name)) = func.kind
&& name.ident.as_str() == "min_value"
&& let Some(func_id) = cx.typeck_results().type_dependent_def_id(func.hir_id)
&& let Some(impl_id) = cx.tcx.impl_of_method(func_id)
&& let None = cx.tcx.impl_trait_ref(impl_id) // An inherent impl
&& cx.tcx.type_of(impl_id).instantiate_identity().is_integral()
{
print_lint_and_sugg(cx, var_name, expr);
}
},
_ => (),
}
}
}

View file

@ -624,7 +624,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(|_| Box::new(unit_return_expecting_ord::UnitReturnExpectingOrd));
store.register_late_pass(|_| Box::new(strings::StringAdd));
store.register_late_pass(|_| Box::new(implicit_return::ImplicitReturn));
store.register_late_pass(|_| Box::new(implicit_saturating_sub::ImplicitSaturatingSub));
store.register_late_pass(move |_| Box::new(implicit_saturating_sub::ImplicitSaturatingSub::new(conf)));
store.register_late_pass(|_| Box::new(default_numeric_fallback::DefaultNumericFallback));
store.register_late_pass(|_| Box::new(inconsistent_struct_constructor::InconsistentStructConstructor));
store.register_late_pass(|_| Box::new(non_octal_unix_permissions::NonOctalUnixPermissions));

View file

@ -184,18 +184,18 @@ fn main() {
let mut m = Mock;
let mut u_32 = 3000;
let a = 200;
let mut _b = 8;
let mut b = 8;
if m != 0 {
m -= 1;
}
if a > 0 {
_b -= 1;
b -= 1;
}
if 0 > a {
_b -= 1;
b -= 1;
}
if u_32 > 0 {
@ -214,4 +214,11 @@ fn main() {
} else if u_32 > 0 {
u_32 -= 1;
}
let result = if a < b {
println!("we shouldn't remove this");
0
} else {
a - b
};
}

View file

@ -230,18 +230,18 @@ fn main() {
let mut m = Mock;
let mut u_32 = 3000;
let a = 200;
let mut _b = 8;
let mut b = 8;
if m != 0 {
m -= 1;
}
if a > 0 {
_b -= 1;
b -= 1;
}
if 0 > a {
_b -= 1;
b -= 1;
}
if u_32 > 0 {
@ -260,4 +260,11 @@ fn main() {
} else if u_32 > 0 {
u_32 -= 1;
}
let result = if a < b {
println!("we shouldn't remove this");
0
} else {
a - b
};
}

View file

@ -0,0 +1,35 @@
//@no-rustfix
#![warn(clippy::implicit_saturating_sub)]
#![allow(arithmetic_overflow)]
fn main() {
let a = 12u32;
let b = 13u32;
let result = if a > b { b - a } else { 0 };
//~^ ERROR: inverted arithmetic check before subtraction
let result = if b < a { b - a } else { 0 };
//~^ ERROR: inverted arithmetic check before subtraction
let result = if a > b { 0 } else { a - b };
//~^ ERROR: inverted arithmetic check before subtraction
let result = if a >= b { 0 } else { a - b };
//~^ ERROR: inverted arithmetic check before subtraction
let result = if b < a { 0 } else { a - b };
//~^ ERROR: inverted arithmetic check before subtraction
let result = if b <= a { 0 } else { a - b };
//~^ ERROR: inverted arithmetic check before subtraction
let af = 12f32;
let bf = 13f32;
// Should not lint!
let result = if bf < af { 0. } else { af - bf };
// Should not lint!
let result = if a < b {
println!("we shouldn't remove this");
0
} else {
a - b
};
}

View file

@ -0,0 +1,75 @@
error: inverted arithmetic check before subtraction
--> tests/ui/manual_arithmetic_check-2.rs:9:23
|
LL | let result = if a > b { b - a } else { 0 };
| ^ ----- help: try replacing it with: `a - b`
|
note: this subtraction underflows when `b < a`
--> tests/ui/manual_arithmetic_check-2.rs:9:29
|
LL | let result = if a > b { b - a } else { 0 };
| ^^^^^
= note: `#[deny(clippy::inverted_saturating_sub)]` on by default
error: inverted arithmetic check before subtraction
--> tests/ui/manual_arithmetic_check-2.rs:11:23
|
LL | let result = if b < a { b - a } else { 0 };
| ^ ----- help: try replacing it with: `a - b`
|
note: this subtraction underflows when `b < a`
--> tests/ui/manual_arithmetic_check-2.rs:11:29
|
LL | let result = if b < a { b - a } else { 0 };
| ^^^^^
error: inverted arithmetic check before subtraction
--> tests/ui/manual_arithmetic_check-2.rs:14:23
|
LL | let result = if a > b { 0 } else { a - b };
| ^ ----- help: try replacing it with: `b - a`
|
note: this subtraction underflows when `a < b`
--> tests/ui/manual_arithmetic_check-2.rs:14:40
|
LL | let result = if a > b { 0 } else { a - b };
| ^^^^^
error: inverted arithmetic check before subtraction
--> tests/ui/manual_arithmetic_check-2.rs:16:23
|
LL | let result = if a >= b { 0 } else { a - b };
| ^^ ----- help: try replacing it with: `b - a`
|
note: this subtraction underflows when `a < b`
--> tests/ui/manual_arithmetic_check-2.rs:16:41
|
LL | let result = if a >= b { 0 } else { a - b };
| ^^^^^
error: inverted arithmetic check before subtraction
--> tests/ui/manual_arithmetic_check-2.rs:18:23
|
LL | let result = if b < a { 0 } else { a - b };
| ^ ----- help: try replacing it with: `b - a`
|
note: this subtraction underflows when `a < b`
--> tests/ui/manual_arithmetic_check-2.rs:18:40
|
LL | let result = if b < a { 0 } else { a - b };
| ^^^^^
error: inverted arithmetic check before subtraction
--> tests/ui/manual_arithmetic_check-2.rs:20:23
|
LL | let result = if b <= a { 0 } else { a - b };
| ^^ ----- help: try replacing it with: `b - a`
|
note: this subtraction underflows when `a < b`
--> tests/ui/manual_arithmetic_check-2.rs:20:41
|
LL | let result = if b <= a { 0 } else { a - b };
| ^^^^^
error: aborting due to 6 previous errors

View file

@ -0,0 +1,24 @@
#![warn(clippy::implicit_saturating_sub, clippy::inverted_saturating_sub)]
#![allow(clippy::if_same_then_else)]
fn main() {
let a = 12u32;
let b = 13u32;
let c = 8u32;
let result = a.saturating_sub(b);
//~^ ERROR: manual arithmetic check found
let result = a.saturating_sub(b);
//~^ ERROR: manual arithmetic check found
let result = a.saturating_sub(b);
//~^ ERROR: manual arithmetic check found
let result = a.saturating_sub(b);
//~^ ERROR: manual arithmetic check found
// Should not warn!
let result = if a > b { a - b } else { a - c };
// Just to check it won't break clippy.
let result = if b > a { 0 } else { 0 };
}

View file

@ -0,0 +1,24 @@
#![warn(clippy::implicit_saturating_sub, clippy::inverted_saturating_sub)]
#![allow(clippy::if_same_then_else)]
fn main() {
let a = 12u32;
let b = 13u32;
let c = 8u32;
let result = if a > b { a - b } else { 0 };
//~^ ERROR: manual arithmetic check found
let result = if b < a { a - b } else { 0 };
//~^ ERROR: manual arithmetic check found
let result = if a < b { 0 } else { a - b };
//~^ ERROR: manual arithmetic check found
let result = if b > a { 0 } else { a - b };
//~^ ERROR: manual arithmetic check found
// Should not warn!
let result = if a > b { a - b } else { a - c };
// Just to check it won't break clippy.
let result = if b > a { 0 } else { 0 };
}

View file

@ -0,0 +1,29 @@
error: manual arithmetic check found
--> tests/ui/manual_arithmetic_check.rs:9:18
|
LL | let result = if a > b { a - b } else { 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)`
|
= note: `-D clippy::implicit-saturating-sub` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::implicit_saturating_sub)]`
error: manual arithmetic check found
--> tests/ui/manual_arithmetic_check.rs:11:18
|
LL | let result = if b < a { a - b } else { 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)`
error: manual arithmetic check found
--> tests/ui/manual_arithmetic_check.rs:14:18
|
LL | let result = if a < b { 0 } else { a - b };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)`
error: manual arithmetic check found
--> tests/ui/manual_arithmetic_check.rs:16:18
|
LL | let result = if b > a { 0 } else { a - b };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)`
error: aborting due to 4 previous errors