forked from rust-lang/rust
-
Notifications
You must be signed in to change notification settings - Fork 7
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of rust-lang#5427 - pmk21:implicit-sat-sub, r=flip1995
Add lint named implicit_saturating_sub Fixes: rust-lang#5399 I've made a basic skeleton of the lint, would love more feedback on how to make it better. changelog: Add lint [`implicit_saturating_sub`]
- Loading branch information
Showing
7 changed files
with
739 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
use crate::utils::{higher, in_macro, match_qpath, span_lint_and_sugg, SpanlessEq}; | ||
use if_chain::if_chain; | ||
use rustc_ast::ast::LitKind; | ||
use rustc_errors::Applicability; | ||
use rustc_hir::{BinOpKind, Expr, ExprKind, QPath, StmtKind}; | ||
use rustc_lint::{LateContext, LateLintPass}; | ||
use rustc_session::{declare_lint_pass, declare_tool_lint}; | ||
|
||
declare_clippy_lint! { | ||
/// **What it does:** Checks for implicit saturating subtraction. | ||
/// | ||
/// **Why is this bad?** Simplicity and readability. Instead we can easily use an builtin function. | ||
/// | ||
/// **Known problems:** None. | ||
/// | ||
/// **Example:** | ||
/// | ||
/// ```rust | ||
/// let end: u32 = 10; | ||
/// let start: u32 = 5; | ||
/// | ||
/// let mut i: u32 = end - start; | ||
/// | ||
/// // Bad | ||
/// if i != 0 { | ||
/// i -= 1; | ||
/// } | ||
/// ``` | ||
/// Use instead: | ||
/// ```rust | ||
/// let end: u32 = 10; | ||
/// let start: u32 = 5; | ||
/// | ||
/// let mut i: u32 = end - start; | ||
/// | ||
/// // Good | ||
/// i = i.saturating_sub(1); | ||
/// ``` | ||
pub IMPLICIT_SATURATING_SUB, | ||
pedantic, | ||
"Perform saturating subtraction instead of implicitly checking lower bound of data type" | ||
} | ||
|
||
declare_lint_pass!(ImplicitSaturatingSub => [IMPLICIT_SATURATING_SUB]); | ||
|
||
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitSaturatingSub { | ||
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) { | ||
if in_macro(expr.span) { | ||
return; | ||
} | ||
if_chain! { | ||
if let Some((ref cond, ref then, None)) = higher::if_block(&expr); | ||
|
||
// Check if the conditional expression is a binary operation | ||
if let ExprKind::Binary(ref cond_op, ref cond_left, ref cond_right) = cond.kind; | ||
|
||
// Ensure that the binary operator is >, != and < | ||
if BinOpKind::Ne == cond_op.node || BinOpKind::Gt == cond_op.node || BinOpKind::Lt == cond_op.node; | ||
|
||
// Check if the true condition block has only one statement | ||
if let ExprKind::Block(ref block, _) = then.kind; | ||
if block.stmts.len() == 1 && block.expr.is_none(); | ||
|
||
// Check if assign operation is done | ||
if let StmtKind::Semi(ref e) = block.stmts[0].kind; | ||
if let Some(target) = subtracts_one(cx, e); | ||
|
||
// Extracting out the variable name | ||
if let ExprKind::Path(ref assign_path) = target.kind; | ||
if let QPath::Resolved(_, ref ares_path) = assign_path; | ||
|
||
then { | ||
// 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; | ||
} | ||
} else { | ||
return; | ||
}; | ||
|
||
// Check if the variable in the condition statement is an integer | ||
if !cx.tables.expr_ty(cond_var).is_integral() { | ||
return; | ||
} | ||
|
||
// Get the variable name | ||
let var_name = ares_path.segments[0].ident.name.as_str(); | ||
const INT_TYPES: [&str; 5] = ["i8", "i16", "i32", "i64", "i128"]; | ||
|
||
match cond_num_val.kind { | ||
ExprKind::Lit(ref cond_lit) => { | ||
// Check if the constant is zero | ||
if let LitKind::Int(0, _) = cond_lit.node { | ||
if cx.tables.expr_ty(cond_left).is_signed() { | ||
} else { | ||
print_lint_and_sugg(cx, &var_name, expr); | ||
}; | ||
} | ||
}, | ||
ExprKind::Path(ref cond_num_path) => { | ||
if INT_TYPES.iter().any(|int_type| match_qpath(cond_num_path, &[int_type, "MIN"])) { | ||
print_lint_and_sugg(cx, &var_name, expr); | ||
}; | ||
}, | ||
ExprKind::Call(ref func, _) => { | ||
if let ExprKind::Path(ref cond_num_path) = func.kind { | ||
if INT_TYPES.iter().any(|int_type| match_qpath(cond_num_path, &[int_type, "min_value"])) { | ||
print_lint_and_sugg(cx, &var_name, expr); | ||
} | ||
}; | ||
}, | ||
_ => (), | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
fn subtracts_one<'a>(cx: &LateContext<'_, '_>, expr: &Expr<'a>) -> Option<&'a Expr<'a>> { | ||
match expr.kind { | ||
ExprKind::AssignOp(ref op1, ref target, ref value) => { | ||
if_chain! { | ||
if BinOpKind::Sub == op1.node; | ||
// Check if literal being subtracted is one | ||
if let ExprKind::Lit(ref lit1) = value.kind; | ||
if let LitKind::Int(1, _) = lit1.node; | ||
then { | ||
Some(target) | ||
} else { | ||
None | ||
} | ||
} | ||
}, | ||
ExprKind::Assign(ref target, ref value, _) => { | ||
if_chain! { | ||
if let ExprKind::Binary(ref op1, ref left1, ref right1) = value.kind; | ||
if BinOpKind::Sub == op1.node; | ||
|
||
if SpanlessEq::new(cx).eq_expr(left1, target); | ||
|
||
if let ExprKind::Lit(ref lit1) = right1.kind; | ||
if let LitKind::Int(1, _) = lit1.node; | ||
then { | ||
Some(target) | ||
} else { | ||
None | ||
} | ||
} | ||
}, | ||
_ => None, | ||
} | ||
} | ||
|
||
fn print_lint_and_sugg(cx: &LateContext<'_, '_>, var_name: &str, expr: &Expr<'_>) { | ||
span_lint_and_sugg( | ||
cx, | ||
IMPLICIT_SATURATING_SUB, | ||
expr.span, | ||
"Implicitly performing saturating subtraction", | ||
"try", | ||
format!("{} = {}.saturating_sub({});", var_name, var_name, 1.to_string()), | ||
Applicability::MachineApplicable, | ||
); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,160 @@ | ||
// run-rustfix | ||
#![allow(unused_assignments, unused_mut, clippy::assign_op_pattern)] | ||
#![warn(clippy::implicit_saturating_sub)] | ||
|
||
fn main() { | ||
// Tests for unsigned integers | ||
|
||
let end_8: u8 = 10; | ||
let start_8: u8 = 5; | ||
let mut u_8: u8 = end_8 - start_8; | ||
|
||
// Lint | ||
u_8 = u_8.saturating_sub(1); | ||
|
||
match end_8 { | ||
10 => { | ||
// Lint | ||
u_8 = u_8.saturating_sub(1); | ||
}, | ||
11 => u_8 += 1, | ||
_ => u_8 = 0, | ||
} | ||
|
||
let end_16: u16 = 35; | ||
let start_16: u16 = 40; | ||
|
||
let mut u_16: u16 = end_16 - start_16; | ||
|
||
// Lint | ||
u_16 = u_16.saturating_sub(1); | ||
|
||
let mut end_32: u32 = 7000; | ||
let mut start_32: u32 = 7010; | ||
|
||
let mut u_32: u32 = end_32 - start_32; | ||
|
||
// Lint | ||
u_32 = u_32.saturating_sub(1); | ||
|
||
// No Lint | ||
if u_32 > 0 { | ||
u_16 += 1; | ||
} | ||
|
||
// No Lint | ||
if u_32 != 0 { | ||
end_32 -= 1; | ||
start_32 += 1; | ||
} | ||
|
||
let mut end_64: u64 = 75001; | ||
let mut start_64: u64 = 75000; | ||
|
||
let mut u_64: u64 = end_64 - start_64; | ||
|
||
// Lint | ||
u_64 = u_64.saturating_sub(1); | ||
|
||
// Lint | ||
u_64 = u_64.saturating_sub(1); | ||
|
||
// Lint | ||
u_64 = u_64.saturating_sub(1); | ||
|
||
// No Lint | ||
if u_64 >= 1 { | ||
u_64 -= 1; | ||
} | ||
|
||
// No Lint | ||
if u_64 > 0 { | ||
end_64 -= 1; | ||
} | ||
|
||
// Tests for usize | ||
let end_usize: usize = 8054; | ||
let start_usize: usize = 8050; | ||
|
||
let mut u_usize: usize = end_usize - start_usize; | ||
|
||
// Lint | ||
u_usize = u_usize.saturating_sub(1); | ||
|
||
// Tests for signed integers | ||
|
||
let endi_8: i8 = 10; | ||
let starti_8: i8 = 50; | ||
|
||
let mut i_8: i8 = endi_8 - starti_8; | ||
|
||
// Lint | ||
i_8 = i_8.saturating_sub(1); | ||
|
||
// Lint | ||
i_8 = i_8.saturating_sub(1); | ||
|
||
// Lint | ||
i_8 = i_8.saturating_sub(1); | ||
|
||
// Lint | ||
i_8 = i_8.saturating_sub(1); | ||
|
||
let endi_16: i16 = 45; | ||
let starti_16: i16 = 44; | ||
|
||
let mut i_16: i16 = endi_16 - starti_16; | ||
|
||
// Lint | ||
i_16 = i_16.saturating_sub(1); | ||
|
||
// Lint | ||
i_16 = i_16.saturating_sub(1); | ||
|
||
// Lint | ||
i_16 = i_16.saturating_sub(1); | ||
|
||
// Lint | ||
i_16 = i_16.saturating_sub(1); | ||
|
||
let endi_32: i32 = 45; | ||
let starti_32: i32 = 44; | ||
|
||
let mut i_32: i32 = endi_32 - starti_32; | ||
|
||
// Lint | ||
i_32 = i_32.saturating_sub(1); | ||
|
||
// Lint | ||
i_32 = i_32.saturating_sub(1); | ||
|
||
// Lint | ||
i_32 = i_32.saturating_sub(1); | ||
|
||
// Lint | ||
i_32 = i_32.saturating_sub(1); | ||
|
||
let endi_64: i64 = 45; | ||
let starti_64: i64 = 44; | ||
|
||
let mut i_64: i64 = endi_64 - starti_64; | ||
|
||
// Lint | ||
i_64 = i_64.saturating_sub(1); | ||
|
||
// Lint | ||
i_64 = i_64.saturating_sub(1); | ||
|
||
// Lint | ||
i_64 = i_64.saturating_sub(1); | ||
|
||
// No Lint | ||
if i_64 > 0 { | ||
i_64 -= 1; | ||
} | ||
|
||
// No Lint | ||
if i_64 != 0 { | ||
i_64 -= 1; | ||
} | ||
} |
Oops, something went wrong.