diff --git a/Cargo.toml b/Cargo.toml index 6a599adcc..27ff93982 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,14 +1,14 @@ [package] - name = "clippy" version = "0.0.1" -authors = ["Manish Goregaokar "] +authors = [ + "Manish Goregaokar ", + "Andre Bogus " +] [lib] name = "clippy" crate_type = ["dylib"] - - [dev-dependencies.compiletest] git = "https://github.com/laumann/compiletest-rs.git" diff --git a/README.md b/README.md index 12f7557a8..577de498d 100644 --- a/README.md +++ b/README.md @@ -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>` - - `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>` + - `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) diff --git a/src/bit_mask.rs b/src/bit_mask.rs new file mode 100644 index 000000000..313bfb1ac --- /dev/null +++ b/src/bit_mask.rs @@ -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 +//! `_ m c` (where `` is one of {`&`, '|'} and `` 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 { + if let &ExprLit(ref lit_ptr) = lit { + if let &LitInt(value, _) = &lit_ptr.node { + return Option::Some(value); //TODO: Handle sign + } + } + Option::None +} diff --git a/src/eq_op.rs b/src/eq_op.rs new file mode 100644 index 000000000..e0b722a0a --- /dev/null +++ b/src/eq_op.rs @@ -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>, right : &Vec>) -> 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, right : &P) -> 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, right : &Vec) -> 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>, right : &Vec>) -> 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>, right : &Vec>) -> 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, right : &Vec) -> 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>, right : &Vec>) -> bool { + over(left, right, |l, r| is_ty_equal(l, r)) +} + +fn over(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(l: &Option, r: &Option, 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 + } +} diff --git a/src/lib.rs b/src/lib.rs index ee137230f..ad477a210 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -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]); } diff --git a/tests/compile-fail/bit_masks.rs b/tests/compile-fail/bit_masks.rs new file mode 100644 index 000000000..0c1fd3482 --- /dev/null +++ b/tests/compile-fail/bit_masks.rs @@ -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 +} diff --git a/tests/compile-fail/eq_op.rs b/tests/compile-fail/eq_op.rs new file mode 100644 index 000000000..910b5e848 --- /dev/null +++ b/tests/compile-fail/eq_op.rs @@ -0,0 +1,36 @@ +#![feature(plugin)] +#![plugin(clippy)] + +fn id(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 +} diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 3fd219dfb..5008c2aa7 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -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"); }