Skip to content

Identity/erasing operation lints #2136

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.

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

Merged
merged 4 commits into from
Oct 23, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
63 changes: 63 additions & 0 deletions clippy_lints/src/erasing_op.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
use consts::{constant_simple, Constant};
use rustc::hir::*;
use rustc::lint::*;
use syntax::codemap::Span;
use utils::{in_macro, span_lint};

/// **What it does:** Checks for erasing operations, e.g. `x * 0`.
///
/// **Why is this bad?** The whole expression can be replaced by zero.
/// This is most likely not the intended outcome and should probably be
/// corrected
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// 0 / x; 0 * x; x & 0
/// ```
declare_lint! {
pub ERASING_OP,
Warn,
"using erasing operations, e.g. `x * 0` or `y & 0`"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also x | -1 (signed) / x | !0 (unsigned), though those return -1 and !0 respectively. It's ok if we extend this later, though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can add this lint. We might also change the lint name. Something about expression with a constant result, I'm not sure.

}

#[derive(Copy, Clone)]
pub struct ErasingOp;

impl LintPass for ErasingOp {
fn get_lints(&self) -> LintArray {
lint_array!(ERASING_OP)
}
}

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ErasingOp {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
if in_macro(e.span) {
return;
}
if let ExprBinary(ref cmp, ref left, ref right) = e.node {
match cmp.node {
BiMul | BiBitAnd => {
check(cx, left, e.span);
check(cx, right, e.span);
},
BiDiv => check(cx, left, e.span),
_ => (),
}
}
}
}

fn check(cx: &LateContext, e: &Expr, span: Span) {
if let Some(Constant::Int(v)) = constant_simple(cx, e) {
if v.to_u128_unchecked() == 0 {
span_lint(
cx,
ERASING_OP,
span,
"this operation will always return zero. This is likely not the intended outcome",
);
}
}
}
25 changes: 19 additions & 6 deletions clippy_lints/src/identity_op.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use consts::{constant_simple, Constant};
use rustc::lint::*;
use rustc::hir::*;
use rustc::lint::*;
use rustc_const_math::ConstInt;
use syntax::codemap::Span;
use utils::{in_macro, snippet, span_lint};
use syntax::attr::IntType::{SignedInt, UnsignedInt};

/// **What it does:** Checks for identity operations, e.g. `x + 0`.
///
Expand Down Expand Up @@ -58,15 +58,28 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IdentityOp {
}
}

fn all_ones(v: &ConstInt) -> bool {
match *v {
ConstInt::I8(i) => i == !0,
ConstInt::I16(i) => i == !0,
ConstInt::I32(i) => i == !0,
ConstInt::I64(i) => i == !0,
ConstInt::I128(i) => i == !0,
ConstInt::U8(i) => i == !0,
ConstInt::U16(i) => i == !0,
ConstInt::U32(i) => i == !0,
ConstInt::U64(i) => i == !0,
ConstInt::U128(i) => i == !0,
_ => false
}
}

#[allow(cast_possible_wrap)]
fn check(cx: &LateContext, e: &Expr, m: i8, span: Span, arg: Span) {
if let Some(Constant::Int(v)) = constant_simple(cx, e) {
if match m {
0 => v.to_u128_unchecked() == 0,
-1 => match v.int_type() {
SignedInt(_) => (v.to_u128_unchecked() as i128 == -1),
UnsignedInt(_) => false,
},
-1 => all_ones(&v),
1 => v.to_u128_unchecked() == 1,
_ => unreachable!(),
} {
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ pub mod entry;
pub mod enum_clike;
pub mod enum_glob_use;
pub mod enum_variants;
pub mod erasing_op;
pub mod eq_op;
pub mod escape;
pub mod eta_reduction;
Expand Down Expand Up @@ -246,6 +247,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_early_lint_pass(box needless_continue::NeedlessContinue);
reg.register_late_lint_pass(box eta_reduction::EtaPass);
reg.register_late_lint_pass(box identity_op::IdentityOp);
reg.register_late_lint_pass(box erasing_op::ErasingOp);
reg.register_early_lint_pass(box items_after_statements::ItemsAfterStatements);
reg.register_late_lint_pass(box mut_mut::MutMut);
reg.register_late_lint_pass(box mut_reference::UnnecessaryMutPassed);
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/bit_masks.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@ error: &-masking with zero
|
= note: `-D bad-bit-mask` implied by `-D warnings`

error: this operation will always return zero. This is likely not the intended outcome
--> $DIR/bit_masks.rs:12:5
|
12 | x & 0 == 0;
| ^^^^^
|
= note: `-D erasing-op` implied by `-D warnings`

error: incompatible bit mask: `_ & 2` can never be equal to `1`
--> $DIR/bit_masks.rs:15:5
|
Expand Down Expand Up @@ -48,6 +56,12 @@ error: &-masking with zero
35 | 0 & x == 0;
| ^^^^^^^^^^

error: this operation will always return zero. This is likely not the intended outcome
--> $DIR/bit_masks.rs:35:5
|
35 | 0 & x == 0;
| ^^^^^

error: incompatible bit mask: `_ | 2` will always be higher than `1`
--> $DIR/bit_masks.rs:39:5
|
Expand Down
12 changes: 12 additions & 0 deletions tests/ui/erasing_op.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@



#[allow(no_effect)]
#[warn(erasing_op)]
fn main() {
let x: u8 = 0;

x * 0;
0 & x;
0 / x;
}
20 changes: 20 additions & 0 deletions tests/ui/erasing_op.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
error: this operation will always return zero. This is likely not the intended outcome
--> $DIR/erasing_op.rs:9:5
|
9 | x * 0;
| ^^^^^
|
= note: `-D erasing-op` implied by `-D warnings`

error: this operation will always return zero. This is likely not the intended outcome
--> $DIR/erasing_op.rs:10:5
|
10 | 0 & x;
| ^^^^^

error: this operation will always return zero. This is likely not the intended outcome
--> $DIR/erasing_op.rs:11:5
|
11 | 0 / x;
| ^^^^^

3 changes: 3 additions & 0 deletions tests/ui/identity_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,7 @@ fn main() {

x & NEG_ONE; //no error, as we skip lookups (for now)
-1 & x;

let u : u8 = 0;
u & 255;
}
6 changes: 6 additions & 0 deletions tests/ui/identity_op.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,9 @@ error: the operation is ineffective. Consider reducing it to `x`
29 | -1 & x;
| ^^^^^^

error: the operation is ineffective. Consider reducing it to `u`
--> $DIR/identity_op.rs:32:5
|
32 | u & 255;
| ^^^^^^^