Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trigger modulo_one lint also on -1. #6360

Merged
merged 3 commits into from
Nov 28, 2020
Merged

Trigger modulo_one lint also on -1. #6360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82a7068
Trigger modulo_one lint also on -1.
mlegner Nov 21, 2020
e42a18f
Run `cargo dev fmt`.
mlegner Nov 21, 2020
b822632
Factor out `check_binary` from function `check_expr`.
mlegner Nov 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 83 additions & 61 deletions clippy_lints/src/misc.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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