Skip to content

Commit

Permalink
Auto merge of #6360 - mlegner:mod_one, r=llogiq
Browse files Browse the repository at this point in the history 
Trigger modulo_one lint also on -1.

Fixes #6321.

changelog: trigger `modulo_one` lint also on `-1`
  • Loading branch information
@bors
bors committed Nov 28, 2020
2 parents 84ba08f + b822632 commit a644930
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 67 deletions.
144 changes: 83 additions & 61 deletions clippy_lints/src/misc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::utils::sugg::Sugg;
use crate::utils::{
get_item_name, get_parent_expr, higher, implements_trait, in_constant, is_integer_const, iter_input_pats,
last_path_segment, match_qpath, match_trait_method, paths, snippet, snippet_opt, span_lint, span_lint_and_sugg,
span_lint_and_then, span_lint_hir_and_then, SpanlessEq,
span_lint_and_then, span_lint_hir_and_then, unsext, SpanlessEq,
};

declare_clippy_lint! {
Expand Down Expand Up @@ -139,23 +139,26 @@ declare_clippy_lint! {
}

declare_clippy_lint! {
/// **What it does:** Checks for getting the remainder of a division by one.
/// **What it does:** Checks for getting the remainder of a division by one or minus
/// one.
///
/// **Why is this bad?** The result can only ever be zero. No one will write
/// such code deliberately, unless trying to win an Underhanded Rust
/// Contest. Even for that contest, it's probably a bad idea. Use something more
/// underhanded.
/// **Why is this bad?** The result for a divisor of one can only ever be zero; for
/// minus one it can cause panic/overflow (if the left operand is the minimal value of
/// the respective integer type) or results in zero. No one will write such code
/// deliberately, unless trying to win an Underhanded Rust Contest. Even for that
/// contest, it's probably a bad idea. Use something more underhanded.
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// # let x = 1;
/// let a = x % 1;
/// let a = x % -1;
/// ```
pub MODULO_ONE,
correctness,
"taking a number modulo 1, which always returns 0"
"taking a number modulo +/-1, which can either panic/overflow or always returns 0"
}

declare_clippy_lint! {
Expand Down Expand Up @@ -378,60 +381,8 @@ impl<'tcx> LateLintPass<'tcx> for MiscLints {
return;
},
ExprKind::Binary(ref cmp, ref left, ref right) => {
let op = cmp.node;
if op.is_comparison() {
check_nan(cx, left, expr);
check_nan(cx, right, expr);
check_to_owned(cx, left, right, true);
check_to_owned(cx, right, left, false);
}
if (op == BinOpKind::Eq || op == BinOpKind::Ne) && (is_float(cx, left) || is_float(cx, right)) {
if is_allowed(cx, left) || is_allowed(cx, right) {
return;
}

// Allow comparing the results of signum()
if is_signum(cx, left) && is_signum(cx, right) {
return;
}

if let Some(name) = get_item_name(cx, expr) {
let name = name.as_str();
if name == "eq"
|| name == "ne"
|| name == "is_nan"
|| name.starts_with("eq_")
|| name.ends_with("_eq")
{
return;
}
}
let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
let (lint, msg) = get_lint_and_message(
is_named_constant(cx, left) || is_named_constant(cx, right),
is_comparing_arrays,
);
span_lint_and_then(cx, lint, expr.span, msg, |diag| {
let lhs = Sugg::hir(cx, left, "..");
let rhs = Sugg::hir(cx, right, "..");

if !is_comparing_arrays {
diag.span_suggestion(
expr.span,
"consider comparing them within some margin of error",
format!(
"({}).abs() {} error_margin",
lhs - rhs,
if op == BinOpKind::Eq { '<' } else { '>' }
),
Applicability::HasPlaceholders, // snippet
);
}
diag.note("`f32::EPSILON` and `f64::EPSILON` are available for the `error_margin`");
});
} else if op == BinOpKind::Rem && is_integer_const(cx, right, 1) {
span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0");
}
check_binary(cx, expr, cmp, left, right);
return;
},
_ => {},
}
Expand Down Expand Up @@ -744,3 +695,74 @@ fn check_cast(cx: &LateContext<'_>, span: Span, e: &Expr<'_>, ty: &hir::Ty<'_>)
}
}
}

fn check_binary(
cx: &LateContext<'a>,
expr: &Expr<'_>,
cmp: &rustc_span::source_map::Spanned<rustc_hir::BinOpKind>,
left: &'a Expr<'_>,
right: &'a Expr<'_>,
) {
let op = cmp.node;
if op.is_comparison() {
check_nan(cx, left, expr);
check_nan(cx, right, expr);
check_to_owned(cx, left, right, true);
check_to_owned(cx, right, left, false);
}
if (op == BinOpKind::Eq || op == BinOpKind::Ne) && (is_float(cx, left) || is_float(cx, right)) {
if is_allowed(cx, left) || is_allowed(cx, right) {
return;
}

// Allow comparing the results of signum()
if is_signum(cx, left) && is_signum(cx, right) {
return;
}

if let Some(name) = get_item_name(cx, expr) {
let name = name.as_str();
if name == "eq" || name == "ne" || name == "is_nan" || name.starts_with("eq_") || name.ends_with("_eq") {
return;
}
}
let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
let (lint, msg) = get_lint_and_message(
is_named_constant(cx, left) || is_named_constant(cx, right),
is_comparing_arrays,
);
span_lint_and_then(cx, lint, expr.span, msg, |diag| {
let lhs = Sugg::hir(cx, left, "..");
let rhs = Sugg::hir(cx, right, "..");

if !is_comparing_arrays {
diag.span_suggestion(
expr.span,
"consider comparing them within some margin of error",
format!(
"({}).abs() {} error_margin",
lhs - rhs,
if op == BinOpKind::Eq { '<' } else { '>' }
),
Applicability::HasPlaceholders, // snippet
);
}
diag.note("`f32::EPSILON` and `f64::EPSILON` are available for the `error_margin`");
});
} else if op == BinOpKind::Rem {
if is_integer_const(cx, right, 1) {
span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0");
}

if let ty::Int(ity) = cx.typeck_results().expr_ty(right).kind() {
if is_integer_const(cx, right, unsext(cx.tcx, -1, *ity)) {
span_lint(
cx,
MODULO_ONE,
expr.span,
"any number modulo -1 will panic/overflow or result in 0",
);
}
};
}
}
11 changes: 10 additions & 1 deletion tests/ui/modulo_one.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,22 @@
#![allow(clippy::no_effect, clippy::unnecessary_operation)]

static STATIC_ONE: usize = 2 - 1;
static STATIC_NEG_ONE: i64 = 1 - 2;

fn main() {
10 % 1;
10 % -1;
10 % 2;
i32::MIN % (-1); // also caught by rustc

const ONE: u32 = 1 * 1;
const NEG_ONE: i64 = 1 - 2;
const INT_MIN: i64 = i64::MIN;

2 % ONE;
5 % STATIC_ONE;
5 % STATIC_ONE; // NOT caught by lint
2 % NEG_ONE;
5 % STATIC_NEG_ONE; // NOT caught by lint
INT_MIN % NEG_ONE; // also caught by rustc
INT_MIN % STATIC_NEG_ONE; // ONLY caught by rustc
}
54 changes: 49 additions & 5 deletions tests/ui/modulo_one.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,30 +1,74 @@
error: this arithmetic operation will overflow
--> $DIR/modulo_one.rs:11:5
|
LL | i32::MIN % (-1); // also caught by rustc
| ^^^^^^^^^^^^^^^ attempt to compute the remainder of `i32::MIN % -1_i32`, which would overflow
|
= note: `#[deny(arithmetic_overflow)]` on by default

error: this arithmetic operation will overflow
--> $DIR/modulo_one.rs:21:5
|
LL | INT_MIN % NEG_ONE; // also caught by rustc
| ^^^^^^^^^^^^^^^^^ attempt to compute the remainder of `i64::MIN % -1_i64`, which would overflow

error: this arithmetic operation will overflow
--> $DIR/modulo_one.rs:22:5
|
LL | INT_MIN % STATIC_NEG_ONE; // ONLY caught by rustc
| ^^^^^^^^^^^^^^^^^^^^^^^^ attempt to compute the remainder of `i64::MIN % -1_i64`, which would overflow

error: any number modulo 1 will be 0
--> $DIR/modulo_one.rs:7:5
--> $DIR/modulo_one.rs:8:5
|
LL | 10 % 1;
| ^^^^^^
|
= note: `-D clippy::modulo-one` implied by `-D warnings`

error: any number modulo -1 will panic/overflow or result in 0
--> $DIR/modulo_one.rs:9:5
|
LL | 10 % -1;
| ^^^^^^^

error: any number modulo -1 will panic/overflow or result in 0
--> $DIR/modulo_one.rs:11:5
|
LL | i32::MIN % (-1); // also caught by rustc
| ^^^^^^^^^^^^^^^

error: the operation is ineffective. Consider reducing it to `1`
--> $DIR/modulo_one.rs:10:22
--> $DIR/modulo_one.rs:13:22
|
LL | const ONE: u32 = 1 * 1;
| ^^^^^
|
= note: `-D clippy::identity-op` implied by `-D warnings`

error: the operation is ineffective. Consider reducing it to `1`
--> $DIR/modulo_one.rs:10:22
--> $DIR/modulo_one.rs:13:22
|
LL | const ONE: u32 = 1 * 1;
| ^^^^^

error: any number modulo 1 will be 0
--> $DIR/modulo_one.rs:12:5
--> $DIR/modulo_one.rs:17:5
|
LL | 2 % ONE;
| ^^^^^^^

error: aborting due to 4 previous errors
error: any number modulo -1 will panic/overflow or result in 0
--> $DIR/modulo_one.rs:19:5
|
LL | 2 % NEG_ONE;
| ^^^^^^^^^^^

error: any number modulo -1 will panic/overflow or result in 0
--> $DIR/modulo_one.rs:21:5
|
LL | INT_MIN % NEG_ONE; // also caught by rustc
| ^^^^^^^^^^^^^^^^^

error: aborting due to 11 previous errors

0 comments on commit a644930

Please sign in to comment.