Added eq_op and bad_bit_mask from the extra_lints project (mostly plain copy, need to refactor to integrate better)

This commit is contained in:
llogiq 2015-04-30 11:48:43 +02:00
parent 2935c31692
commit 441b55b328
8 changed files with 398 additions and 15 deletions

View file

@ -1,14 +1,14 @@
[package]
name = "clippy"
version = "0.0.1"
authors = ["Manish Goregaokar <manishsmail@gmail.com>"]
authors = [
"Manish Goregaokar <manishsmail@gmail.com>",
"Andre Bogus <bogusandre@gmail.com>"
]
[lib]
name = "clippy"
crate_type = ["dylib"]
[dev-dependencies.compiletest]
git = "https://github.com/laumann/compiletest-rs.git"

View file

@ -1,16 +1,18 @@
rust-clippy
===========
A collection of lints that give helpful tips to newbies.
A collection of lints that give helpful tips to newbies and catch oversights.
Lints included in this crate:
- `clippy_single_match`: Warns when a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used, and recommends `if let` instead.
- `clippy_box_vec`: Warns on usage of `Box<Vec<T>>`
- `clippy_dlist`: Warns on usage of `DList`
- `clippy_str_to_string`: Warns on usage of `str::to_string()`
- `clippy_toplevel_ref_arg`: Warns when a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not `fn foo((ref x, ref y): (u8, u8))`).
- `single_match`: Warns when a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used, and recommends `if let` instead.
- `box_vec`: Warns on usage of `Box<Vec<T>>`
- `dlist`: Warns on usage of `DList`
- `str_to_string`: Warns on usage of `str::to_string()`
- `toplevel_ref_arg`: Warns when a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not `fn foo((ref x, ref y): (u8, u8))`).
- `eq_op`: Warns on equal operands on both sides of a comparison or bitwise combination
- `bad_bit_mask`: Denies expressions of the form `_ & mask == select` that will only ever return `true` or `false` (because in the example `select` containing bits that `mask` doesn't have)
You can allow/warn/deny the whole set using the `clippy` lint group (`#[allow(clippy)]`, etc)

111
src/bit_mask.rs Normal file
View file

@ -0,0 +1,111 @@
//! Checks for incompatible bit masks in comparisons, e.g. `x & 1 == 2`. This cannot work because the bit that makes up
//! the value two was zeroed out by the bit-and with 1. So the formula for detecting if an expression of the type
//! `_ <bit_op> m <cmp_op> c` (where `<bit_op>` is one of {`&`, '|'} and `<cmp_op>` is one of {`!=`, `>=`, `>` ,`!=`, `>=`,
//! `>`}) can be determined from the following table:
//!
//! |Comparison |Bit-Op|Example |is always|Formula |
//! |------------|------|------------|---------|----------------------|
//! |`==` or `!=`| `&` |`x & 2 == 3`|`false` |`c & m != c` |
//! |`<` or `>=`| `&` |`x & 2 < 3` |`true` |`m < c` |
//! |`>` or `<=`| `&` |`x & 1 > 1` |`false` |`m <= c` |
//! |`==` or `!=`| `|` |`x | 1 == 0`|`false` |`c | m != c` |
//! |`<` or `>=`| `|` |`x | 1 < 1` |`false` |`m >= c` |
//! |`<=` or `>` | `|` |`x | 1 > 0` |`true` |`m > c` |
//!
//! *TODO*: There is the open question if things like `x | 1 > 1` should be caught by this lint, because it is basically
//! an obfuscated version of `x > 1`.
//!
//! This lint is **deny** by default
use rustc::plugin::Registry;
use rustc::lint::*;
use syntax::ast::*;
use syntax::ast_util::{is_comparison_binop, binop_to_string};
use syntax::ptr::P;
use syntax::codemap::Span;
declare_lint! {
pub BAD_BIT_MASK,
Deny,
"Deny the use of incompatible bit masks in comparisons, e.g. '(a & 1) == 2'"
}
#[derive(Copy,Clone)]
pub struct BitMask;
impl LintPass for BitMask {
fn get_lints(&self) -> LintArray {
lint_array!(BAD_BIT_MASK)
}
fn check_expr(&mut self, cx: &Context, e: &Expr) {
if let ExprBinary(ref cmp, ref left, ref right) = e.node {
if is_comparison_binop(cmp.node) {
fetch_int_literal(&right.node).map(|cmp_value| check_compare(cx, left, cmp.node, cmp_value, &e.span));
}
}
}
}
fn check_compare(cx: &Context, bit_op: &Expr, cmp_op: BinOp_, cmp_value: u64, span: &Span) {
match &bit_op.node {
&ExprParen(ref subexp) => check_compare(cx, subexp, cmp_op, cmp_value, span),
&ExprBinary(ref op, ref left, ref right) => {
if op.node != BiBitAnd && op.node != BiBitOr { return; }
if let Some(mask_value) = fetch_int_literal(&right.node) {
check_bit_mask(cx, op.node, cmp_op, mask_value, cmp_value, span);
} else if let Some(mask_value) = fetch_int_literal(&left.node) {
check_bit_mask(cx, op.node, cmp_op, mask_value, cmp_value, span);
}
},
_ => ()
}
}
fn check_bit_mask(cx: &Context, bit_op: BinOp_, cmp_op: BinOp_, mask_value: u64, cmp_value: u64, span: &Span) {
match cmp_op {
BiEq | BiNe => match bit_op {
BiBitAnd => if mask_value & cmp_value != mask_value {
cx.span_lint(BAD_BIT_MASK, *span, &format!("incompatible bit mask: _ & {} can never be equal to {}", mask_value,
cmp_value));
},
BiBitOr => if mask_value | cmp_value != cmp_value {
cx.span_lint(BAD_BIT_MASK, *span, &format!("incompatible bit mask: _ | {} can never be equal to {}", mask_value,
cmp_value));
},
_ => ()
},
BiLt | BiGe => match bit_op {
BiBitAnd => if mask_value < cmp_value {
cx.span_lint(BAD_BIT_MASK, *span, &format!("incompatible bit mask: _ & {} will always be lower than {}", mask_value,
cmp_value));
},
BiBitOr => if mask_value >= cmp_value {
cx.span_lint(BAD_BIT_MASK, *span, &format!("incompatible bit mask: _ | {} will never be lower than {}", mask_value,
cmp_value));
},
_ => ()
},
BiLe | BiGt => match bit_op {
BiBitAnd => if mask_value <= cmp_value {
cx.span_lint(BAD_BIT_MASK, *span, &format!("incompatible bit mask: _ & {} will never be higher than {}", mask_value,
cmp_value));
},
BiBitOr => if mask_value > cmp_value {
cx.span_lint(BAD_BIT_MASK, *span, &format!("incompatible bit mask: _ | {} will always be higher than {}", mask_value,
cmp_value));
},
_ => ()
},
_ => ()
}
}
fn fetch_int_literal(lit : &Expr_) -> Option<u64> {
if let &ExprLit(ref lit_ptr) = lit {
if let &LitInt(value, _) = &lit_ptr.node {
return Option::Some(value); //TODO: Handle sign
}
}
Option::None
}

214
src/eq_op.rs Normal file
View file

@ -0,0 +1,214 @@
use rustc::lint::*;
use syntax::ast::*;
use syntax::ast_util as ast_util;
use syntax::ptr::P;
use syntax::codemap as code;
declare_lint! {
pub EQ_OP,
Warn,
"warn about comparing equal expressions (e.g. x == x)"
}
#[derive(Copy,Clone)]
pub struct EqOp;
impl LintPass for EqOp {
fn get_lints(&self) -> LintArray {
lint_array!(EQ_OP)
}
fn check_expr(&mut self, cx: &Context, e: &Expr) {
if let ExprBinary(ref op, ref left, ref right) = e.node {
if is_cmp_or_bit(op) && is_exp_equal(left, right) {
cx.span_lint(EQ_OP, e.span, &format!("equal expressions as operands to {}", ast_util::binop_to_string(op.node)));
}
}
}
}
fn is_exp_equal(left : &Expr, right : &Expr) -> bool {
match (&left.node, &right.node) {
(&ExprBinary(ref lop, ref ll, ref lr), &ExprBinary(ref rop, ref rl, ref rr)) =>
lop.node == rop.node && is_exp_equal(ll, rl) && is_exp_equal(lr, rr),
(&ExprBox(ref lpl, ref lboxedpl), &ExprBox(ref rpl, ref rboxedpl)) =>
both(lpl, rpl, |l, r| is_exp_equal(l, r)) && is_exp_equal(lboxedpl, rboxedpl),
(&ExprCall(ref lcallee, ref largs), &ExprCall(ref rcallee, ref rargs)) =>
is_exp_equal(lcallee, rcallee) && is_exp_vec_equal(largs, rargs),
(&ExprCast(ref lcast, ref lty), &ExprCast(ref rcast, ref rty)) =>
is_ty_equal(lty, rty) && is_exp_equal(lcast, rcast),
(&ExprField(ref lfexp, ref lfident), &ExprField(ref rfexp, ref rfident)) =>
lfident.node == rfident.node && is_exp_equal(lfexp, rfexp),
(&ExprLit(ref llit), &ExprLit(ref rlit)) => llit.node == rlit.node,
(&ExprMethodCall(ref lident, ref lcty, ref lmargs), &ExprMethodCall(ref rident, ref rcty, ref rmargs)) =>
lident.node == rident.node && is_ty_vec_equal(lcty, rcty) && is_exp_vec_equal(lmargs, rmargs),
(&ExprParen(ref lparen), &ExprParen(ref rparen)) => is_exp_equal(lparen, rparen),
(&ExprParen(ref lparen), _) => is_exp_equal(lparen, right),
(_, &ExprParen(ref rparen)) => is_exp_equal(left, rparen),
(&ExprPath(ref lqself, ref lsubpath), &ExprPath(ref rqself, ref rsubpath)) =>
both(lqself, rqself, |l, r| is_qself_equal(l, r)) && is_path_equal(lsubpath, rsubpath),
(&ExprTup(ref ltup), &ExprTup(ref rtup)) => is_exp_vec_equal(ltup, rtup),
(&ExprUnary(lunop, ref lparam), &ExprUnary(runop, ref rparam)) => lunop == runop && is_exp_equal(lparam, rparam),
(&ExprVec(ref lvec), &ExprVec(ref rvec)) => is_exp_vec_equal(lvec, rvec),
_ => false
}
}
fn is_exp_vec_equal(left : &Vec<P<Expr>>, right : &Vec<P<Expr>>) -> bool {
over(left, right, |l, r| is_exp_equal(l, r))
}
fn is_path_equal(left : &Path, right : &Path) -> bool {
left.global == right.global && left.segments == right.segments
}
fn is_qself_equal(left : &QSelf, right : &QSelf) -> bool {
left.ty.node == right.ty.node && left.position == right.position
}
fn is_ty_equal(left : &Ty, right : &Ty) -> bool {
match (&left.node, &right.node) {
(&TyVec(ref lvec), &TyVec(ref rvec)) => is_ty_equal(lvec, rvec),
(&TyFixedLengthVec(ref lfvty, ref lfvexp), &TyFixedLengthVec(ref rfvty, ref rfvexp)) =>
is_ty_equal(lfvty, rfvty) && is_exp_equal(lfvexp, rfvexp),
(&TyPtr(ref lmut), &TyPtr(ref rmut)) => is_mut_ty_equal(lmut, rmut),
(&TyRptr(ref ltime, ref lrmut), &TyRptr(ref rtime, ref rrmut)) =>
both(ltime, rtime, is_lifetime_equal) && is_mut_ty_equal(lrmut, rrmut),
(&TyBareFn(ref lbare), &TyBareFn(ref rbare)) => is_bare_fn_ty_equal(lbare, rbare),
(&TyTup(ref ltup), &TyTup(ref rtup)) => is_ty_vec_equal(ltup, rtup),
(&TyPath(Option::None, ref lpath), &TyPath(Option::None, ref rpath)) => is_path_equal(lpath, rpath),
(&TyPath(Option::Some(ref lqself), ref lsubpath), &TyPath(Option::Some(ref rqself), ref rsubpath)) =>
is_qself_equal(lqself, rqself) && is_path_equal(lsubpath, rsubpath),
(&TyObjectSum(ref lsumty, ref lobounds), &TyObjectSum(ref rsumty, ref robounds)) =>
is_ty_equal(lsumty, rsumty) && is_param_bounds_equal(lobounds, robounds),
(&TyPolyTraitRef(ref ltbounds), &TyPolyTraitRef(ref rtbounds)) => is_param_bounds_equal(ltbounds, rtbounds),
(&TyParen(ref lty), &TyParen(ref rty)) => is_ty_equal(lty, rty),
(&TyTypeof(ref lof), &TyTypeof(ref rof)) => is_exp_equal(lof, rof),
(&TyInfer, &TyInfer) => true,
_ => false
}
}
fn is_param_bound_equal(left : &TyParamBound, right : &TyParamBound) -> bool {
match(left, right) {
(&TraitTyParamBound(ref lpoly, ref lmod), &TraitTyParamBound(ref rpoly, ref rmod)) =>
lmod == rmod && is_poly_traitref_equal(lpoly, rpoly),
(&RegionTyParamBound(ref ltime), &RegionTyParamBound(ref rtime)) => is_lifetime_equal(ltime, rtime),
_ => false
}
}
fn is_poly_traitref_equal(left : &PolyTraitRef, right : &PolyTraitRef) -> bool {
is_lifetimedef_vec_equal(&left.bound_lifetimes, &right.bound_lifetimes) &&
is_path_equal(&left.trait_ref.path, &right.trait_ref.path)
}
fn is_param_bounds_equal(left : &TyParamBounds, right : &TyParamBounds) -> bool {
over(left, right, is_param_bound_equal)
}
fn is_mut_ty_equal(left : &MutTy, right : &MutTy) -> bool {
left.mutbl == right.mutbl && is_ty_equal(&left.ty, &right.ty)
}
fn is_bare_fn_ty_equal(left : &BareFnTy, right : &BareFnTy) -> bool {
left.unsafety == right.unsafety && left.abi == right.abi &&
is_lifetimedef_vec_equal(&left.lifetimes, &right.lifetimes) && is_fndecl_equal(&left.decl, &right.decl)
}
fn is_fndecl_equal(left : &P<FnDecl>, right : &P<FnDecl>) -> bool {
left.variadic == right.variadic && is_arg_vec_equal(&left.inputs, &right.inputs) &&
is_fnret_ty_equal(&left.output, &right.output)
}
fn is_fnret_ty_equal(left : &FunctionRetTy, right : &FunctionRetTy) -> bool {
match (left, right) {
(&NoReturn(_), &NoReturn(_)) | (&DefaultReturn(_), &DefaultReturn(_)) => true,
(&Return(ref lty), &Return(ref rty)) => is_ty_equal(lty, rty),
_ => false
}
}
fn is_arg_equal(left : &Arg, right : &Arg) -> bool {
is_ty_equal(&left.ty, &right.ty) && is_pat_equal(&left.pat, &right.pat)
}
fn is_arg_vec_equal(left : &Vec<Arg>, right : &Vec<Arg>) -> bool {
over(left, right, is_arg_equal)
}
fn is_pat_equal(left : &Pat, right : &Pat) -> bool {
match(&left.node, &right.node) {
(&PatWild(lwild), &PatWild(rwild)) => lwild == rwild,
(&PatIdent(ref lmode, ref lident, Option::None), &PatIdent(ref rmode, ref rident, Option::None)) =>
lmode == rmode && is_ident_equal(&lident.node, &rident.node),
(&PatIdent(ref lmode, ref lident, Option::Some(ref lpat)),
&PatIdent(ref rmode, ref rident, Option::Some(ref rpat))) =>
lmode == rmode && is_ident_equal(&lident.node, &rident.node) && is_pat_equal(lpat, rpat),
(&PatEnum(ref lpath, Option::None), &PatEnum(ref rpath, Option::None)) => is_path_equal(lpath, rpath),
(&PatEnum(ref lpath, Option::Some(ref lenum)), &PatEnum(ref rpath, Option::Some(ref renum))) =>
is_path_equal(lpath, rpath) && is_pat_vec_equal(lenum, renum),
(&PatStruct(ref lpath, ref lfieldpat, lbool), &PatStruct(ref rpath, ref rfieldpat, rbool)) =>
lbool == rbool && is_path_equal(lpath, rpath) && is_spanned_fieldpat_vec_equal(lfieldpat, rfieldpat),
(&PatTup(ref ltup), &PatTup(ref rtup)) => is_pat_vec_equal(ltup, rtup),
(&PatBox(ref lboxed), &PatBox(ref rboxed)) => is_pat_equal(lboxed, rboxed),
(&PatRegion(ref lpat, ref lmut), &PatRegion(ref rpat, ref rmut)) => is_pat_equal(lpat, rpat) && lmut == rmut,
(&PatLit(ref llit), &PatLit(ref rlit)) => is_exp_equal(llit, rlit),
(&PatRange(ref lfrom, ref lto), &PatRange(ref rfrom, ref rto)) =>
is_exp_equal(lfrom, rfrom) && is_exp_equal(lto, rto),
(&PatVec(ref lfirst, Option::None, ref llast), &PatVec(ref rfirst, Option::None, ref rlast)) =>
is_pat_vec_equal(lfirst, rfirst) && is_pat_vec_equal(llast, rlast),
(&PatVec(ref lfirst, Option::Some(ref lpat), ref llast), &PatVec(ref rfirst, Option::Some(ref rpat), ref rlast)) =>
is_pat_vec_equal(lfirst, rfirst) && is_pat_equal(lpat, rpat) && is_pat_vec_equal(llast, rlast),
// I don't match macros for now, the code is slow enough as is ;-)
_ => false
}
}
fn is_spanned_fieldpat_vec_equal(left : &Vec<code::Spanned<FieldPat>>, right : &Vec<code::Spanned<FieldPat>>) -> bool {
over(left, right, |l, r| is_fieldpat_equal(&l.node, &r.node))
}
fn is_fieldpat_equal(left : &FieldPat, right : &FieldPat) -> bool {
left.is_shorthand == right.is_shorthand && is_ident_equal(&left.ident, &right.ident) &&
is_pat_equal(&left.pat, &right.pat)
}
fn is_ident_equal(left : &Ident, right : &Ident) -> bool {
&left.name == &right.name && left.ctxt == right.ctxt
}
fn is_pat_vec_equal(left : &Vec<P<Pat>>, right : &Vec<P<Pat>>) -> bool {
over(left, right, |l, r| is_pat_equal(l, r))
}
fn is_lifetimedef_equal(left : &LifetimeDef, right : &LifetimeDef) -> bool {
is_lifetime_equal(&left.lifetime, &right.lifetime) && over(&left.bounds, &right.bounds, is_lifetime_equal)
}
fn is_lifetimedef_vec_equal(left : &Vec<LifetimeDef>, right : &Vec<LifetimeDef>) -> bool {
over(left, right, is_lifetimedef_equal)
}
fn is_lifetime_equal(left : &Lifetime, right : &Lifetime) -> bool {
left.name == right.name
}
fn is_ty_vec_equal(left : &Vec<P<Ty>>, right : &Vec<P<Ty>>) -> bool {
over(left, right, |l, r| is_ty_equal(l, r))
}
fn over<X, F>(left: &[X], right: &[X], mut eq_fn: F) -> bool where F: FnMut(&X, &X) -> bool {
left.len() == right.len() && left.iter().zip(right).all(|(x, y)| eq_fn(x, y))
}
fn both<X, F>(l: &Option<X>, r: &Option<X>, mut eq_fn : F) -> bool where F: FnMut(&X, &X) -> bool {
if l.is_none() { r.is_none() } else { r.is_some() && eq_fn(l.as_ref().unwrap(), &r.as_ref().unwrap()) }
}
fn is_cmp_or_bit(op : &BinOp) -> bool {
match op.node {
BiEq | BiLt | BiLe | BiGt | BiGe | BiNe | BiAnd | BiOr | BiBitXor | BiBitAnd | BiBitOr => true,
_ => false
}
}

View file

@ -16,6 +16,8 @@ use rustc::lint::LintPassObject;
pub mod types;
pub mod misc;
pub mod eq_op;
pub mod bit_mask;
#[plugin_registrar]
pub fn plugin_registrar(reg: &mut Registry) {
@ -23,7 +25,10 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_lint_pass(box misc::MiscPass as LintPassObject);
reg.register_lint_pass(box misc::StrToStringPass as LintPassObject);
reg.register_lint_pass(box misc::TopLevelRefPass as LintPassObject);
reg.register_lint_pass(box eq_op::EqOp as LintPassObject);
reg.register_lint_pass(box bit_mask::BitMask as LintPassObject);
reg.register_lint_group("clippy", vec![types::BOX_VEC, types::LINKEDLIST,
misc::SINGLE_MATCH, misc::STR_TO_STRING,
misc::TOPLEVEL_REF_ARG]);
misc::TOPLEVEL_REF_ARG, eq_op::EQ_OP,
bit_mask::BAD_BIT_MASK]);
}

View file

@ -0,0 +1,19 @@
#![feature(plugin)]
#![plugin(clippy)]
#![deny(bad_bit_mask)]
fn main() {
let x = 5;
x & 1 == 1; //ok, distinguishes bit 0
x & 2 == 1; //~ERROR
x | 1 == 3; //ok, equals x == 2 || x == 3
x | 3 == 3; //ok, equals x <= 3
x | 3 == 2; //~ERROR
x & 1 > 1; //~ERROR
x & 2 > 1; // ok, distinguishes x & 2 == 2 from x & 2 == 0
x & 2 < 1; // ok, distinguishes x & 2 == 2 from x & 2 == 0
x | 1 > 1; // ok (if a bit silly), equals x > 1
x | 2 > 1; //~ERROR
x | 2 <= 2; // ok (if a bit silly), equals x <= 2
}

View file

@ -0,0 +1,36 @@
#![feature(plugin)]
#![plugin(clippy)]
fn id<X>(x: X) -> X {
x
}
#[deny(eq_op)]
fn main() {
// simple values and comparisons
1 == 1; //~ERROR
"no" == "no"; //~ERROR
// even though I agree that no means no ;-)
false != false; //~ERROR
1.5 < 1.5; //~ERROR
1u64 >= 1u64; //~ERROR
// casts, methods, parenthesis
(1 as u64) & (1 as u64); //~ERROR
1 ^ ((((((1)))))); //~ERROR
id((1)) | id(1); //~ERROR
// unary and binary operators
(-(2) < -(2)); //~ERROR
((1 + 1) & (1 + 1) == (1 + 1) & (1 + 1));
//~^ ERROR
//~^^ ERROR
//~^^^ ERROR
(1 * 2) + (3 * 4) == 1 * 2 + 3 * 4; //~ERROR
// various other things
([1] != [1]); //~ERROR
((1, 2) != (1, 2)); //~ERROR
[1].len() == [1].len(); //~ERROR
vec![1, 2, 3] == vec![1, 2, 3]; //no error yet, as we don't match macros
}

View file

@ -1,11 +1,8 @@
extern crate compiletest;
use std::env;
use std::process::Command;
use std::path::PathBuf;
fn run_mode(mode: &'static str) {
let mut config = compiletest::default_config();
let cfg_mode = mode.parse().ok().expect("Invalid mode");
config.target_rustcflags = Some("-L target/debug/".to_string());
@ -19,5 +16,4 @@ fn run_mode(mode: &'static str) {
#[test]
fn compile_test() {
run_mode("compile-fail");
// run_mode("run-pass");
}