Skip to content

Commit

Permalink
Handle symmetric if conditions
Browse files Browse the repository at this point in the history
  • Loading branch information
@pmk21
pmk21 committed Apr 9, 2020
1 parent d0f6d54 commit dd0fd99
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 32 deletions.
44 changes: 30 additions & 14 deletions clippy_lints/src/implicit_saturating_sub.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitSaturatingSub {
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 op, ref cond_left, ref cond_right) = cond.kind;
if let ExprKind::Binary(ref cond_op, ref cond_left, ref cond_right) = cond.kind;

// Check if the variable in the condition statement is an integer
if cx.tables.expr_ty(cond_left).is_integral();

// Ensure that the binary operator is > or !=
if BinOpKind::Ne == op.node || BinOpKind::Gt == op.node;
// 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;
Expand All @@ -70,9 +67,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitSaturatingSub {
if BinOpKind::Sub == op1.node;
if let ExprKind::Path(ref assign_path) = target.kind;

// Check if the variable in the condition and assignment statement are the same
if SpanlessEq::new(cx).eq_expr(cond_left, target);

// Extracting out the variable name
if let QPath::Resolved(_, ref ares_path) = assign_path;

Expand All @@ -81,10 +75,32 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitSaturatingSub {
if let LitKind::Int(1, _) = lit1.node;

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();

match cond_right.kind {
match cond_num_val.kind {
ExprKind::Lit(ref cond_lit) => {
// Check if the constant is zero
if let LitKind::Int(0, _) = cond_lit.node {
Expand All @@ -97,16 +113,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitSaturatingSub {
return;
}
},
ExprKind::Path(ref cond_right_path) => {
if match_qpath(cond_right_path, &["i8", "MIN"]) || match_qpath(cond_right_path, &["i16", "MIN"]) || match_qpath(cond_right_path, &["i32", "MIN"]) || match_qpath(cond_right_path, &["i64", "MIN"]) {
ExprKind::Path(ref cond_num_path) => {
if match_qpath(cond_num_path, &["i8", "MIN"]) || match_qpath(cond_num_path, &["i16", "MIN"]) || match_qpath(cond_num_path, &["i32", "MIN"]) || match_qpath(cond_num_path, &["i64", "MIN"]) {
print_lint_and_sugg(cx, &var_name, expr);
} else {
return;
}
},
ExprKind::Call(ref func, _) => {
if let ExprKind::Path(ref cond_right_path) = func.kind {
if match_qpath(cond_right_path, &["i8", "min_value"]) || match_qpath(cond_right_path, &["i16", "min_value"]) || match_qpath(cond_right_path, &["i32", "min_value"]) || match_qpath(cond_right_path, &["i64", "min_value"]) {
if let ExprKind::Path(ref cond_num_path) = func.kind {
if match_qpath(cond_num_path, &["i8", "min_value"]) || match_qpath(cond_num_path, &["i16", "min_value"]) || match_qpath(cond_num_path, &["i32", "min_value"]) || match_qpath(cond_num_path, &["i64", "min_value"]) {
print_lint_and_sugg(cx, &var_name, expr);
} else {
return;
Expand Down
30 changes: 30 additions & 0 deletions tests/ui/implicit_saturating_sub.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,16 @@ fn main() {
u_64 -= 1;
}

// Lint
if 0 < u_64 {
u_64 -= 1;
}

// Lint
if 0 != u_64 {
u_64 -= 1;
}

// No Lint
if u_64 >= 1 {
u_64 -= 1;
Expand Down Expand Up @@ -187,6 +197,26 @@ fn main() {
i_64 -= 1;
}

// Lint
if i64::min_value() != i_64 {
i_64 -= 1;
}

// Lint
if i64::min_value() < i_64 {
i_64 -= 1;
}

// Lint
if i64::MIN != i_64 {
i_64 -= 1;
}

// Lint
if i64::MIN < i_64 {
i_64 -= 1;
}

// No Lint
if i_64 > 0 {
i_64 -= 1;
Expand Down
84 changes: 66 additions & 18 deletions tests/ui/implicit_saturating_sub.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,140 +41,188 @@ LL | | }
| |_____^ help: try: `u_64 = u_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:84:5
--> $DIR/implicit_saturating_sub.rs:68:5
|
LL | / if 0 < u_64 {
LL | | u_64 -= 1;
LL | | }
| |_____^ help: try: `u_64 = u_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:73:5
|
LL | / if 0 != u_64 {
LL | | u_64 -= 1;
LL | | }
| |_____^ help: try: `u_64 = u_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:94:5
|
LL | / if u_usize > 0 {
LL | | u_usize -= 1;
LL | | }
| |_____^ help: try: `u_usize = u_usize.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:96:5
--> $DIR/implicit_saturating_sub.rs:106:5
|
LL | / if i_8 > i8::MIN {
LL | | i_8 -= 1;
LL | | }
| |_____^ help: try: `i_8 = i_8.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:101:5
--> $DIR/implicit_saturating_sub.rs:111:5
|
LL | / if i_8 > i8::min_value() {
LL | | i_8 -= 1;
LL | | }
| |_____^ help: try: `i_8 = i_8.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:106:5
--> $DIR/implicit_saturating_sub.rs:116:5
|
LL | / if i_8 != i8::MIN {
LL | | i_8 -= 1;
LL | | }
| |_____^ help: try: `i_8 = i_8.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:111:5
--> $DIR/implicit_saturating_sub.rs:121:5
|
LL | / if i_8 != i8::min_value() {
LL | | i_8 -= 1;
LL | | }
| |_____^ help: try: `i_8 = i_8.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:121:5
--> $DIR/implicit_saturating_sub.rs:131:5
|
LL | / if i_16 > i16::MIN {
LL | | i_16 -= 1;
LL | | }
| |_____^ help: try: `i_16 = i_16.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:126:5
--> $DIR/implicit_saturating_sub.rs:136:5
|
LL | / if i_16 > i16::min_value() {
LL | | i_16 -= 1;
LL | | }
| |_____^ help: try: `i_16 = i_16.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:131:5
--> $DIR/implicit_saturating_sub.rs:141:5
|
LL | / if i_16 != i16::MIN {
LL | | i_16 -= 1;
LL | | }
| |_____^ help: try: `i_16 = i_16.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:136:5
--> $DIR/implicit_saturating_sub.rs:146:5
|
LL | / if i_16 != i16::min_value() {
LL | | i_16 -= 1;
LL | | }
| |_____^ help: try: `i_16 = i_16.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:146:5
--> $DIR/implicit_saturating_sub.rs:156:5
|
LL | / if i_32 > i32::MIN {
LL | | i_32 -= 1;
LL | | }
| |_____^ help: try: `i_32 = i_32.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:151:5
--> $DIR/implicit_saturating_sub.rs:161:5
|
LL | / if i_32 > i32::min_value() {
LL | | i_32 -= 1;
LL | | }
| |_____^ help: try: `i_32 = i_32.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:156:5
--> $DIR/implicit_saturating_sub.rs:166:5
|
LL | / if i_32 != i32::MIN {
LL | | i_32 -= 1;
LL | | }
| |_____^ help: try: `i_32 = i_32.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:161:5
--> $DIR/implicit_saturating_sub.rs:171:5
|
LL | / if i_32 != i32::min_value() {
LL | | i_32 -= 1;
LL | | }
| |_____^ help: try: `i_32 = i_32.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:171:5
--> $DIR/implicit_saturating_sub.rs:181:5
|
LL | / if i_64 > i64::MIN {
LL | | i_64 -= 1;
LL | | }
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:176:5
--> $DIR/implicit_saturating_sub.rs:186:5
|
LL | / if i_64 > i64::min_value() {
LL | | i_64 -= 1;
LL | | }
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:181:5
--> $DIR/implicit_saturating_sub.rs:191:5
|
LL | / if i_64 != i64::MIN {
LL | | i_64 -= 1;
LL | | }
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:186:5
--> $DIR/implicit_saturating_sub.rs:196:5
|
LL | / if i_64 != i64::min_value() {
LL | | i_64 -= 1;
LL | | }
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`

error: aborting due to 22 previous errors
error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:201:5
|
LL | / if i64::min_value() != i_64 {
LL | | i_64 -= 1;
LL | | }
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:206:5
|
LL | / if i64::min_value() < i_64 {
LL | | i_64 -= 1;
LL | | }
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:211:5
|
LL | / if i64::MIN != i_64 {
LL | | i_64 -= 1;
LL | | }
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`

error: Implicitly performing saturating subtraction
--> $DIR/implicit_saturating_sub.rs:216:5
|
LL | / if i64::MIN < i_64 {
LL | | i_64 -= 1;
LL | | }
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`

error: aborting due to 28 previous errors

0 comments on commit dd0fd99

Please sign in to comment.