diff --git a/README.md b/README.md index e6a4d1be514..be7154c8c62 100644 --- a/README.md +++ b/README.md @@ -4,56 +4,55 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -Lints included in this crate: +There are 45 lints included in this crate: -name | default | meaning ----------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- -approx_constant | warn | the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found; suggests to use the constant -bad_bit_mask | deny | 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) -box_vec | warn | usage of `Box>`, vector elements are already on the heap -cmp_nan | deny | comparisons to NAN (which will always return false, which is probably not intended) -cmp_owned | warn | creating owned instances for comparing with others, e.g. `x == "foo".to_string()` -collapsible_if | warn | two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }` -eq_op | warn | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`) -explicit_iter_loop | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do -float_cmp | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) -identity_op | warn | using identity operations, e.g. `x + 0` or `y / 1` -ineffective_bit_mask | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` -inline_always | warn | `#[inline(always)]` is a bad idea in most cases -iter_next_loop | warn | for-looping over `_.next()` which is probably not intended -len_without_is_empty | warn | traits and impls that have `.len()` but not `.is_empty()` -len_zero | warn | checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` could be used instead -let_and_return | warn | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a function -let_unit_value | warn | creating a let binding to a value of unit type, which usually can't be used afterwards -linkedlist | warn | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a RingBuf -modulo_one | warn | taking a number modulo 1, which always returns 0 -mut_mut | warn | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references) -needless_bool | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` -needless_lifetimes | warn | using explicit lifetimes for references in function arguments when elision rules would allow omitting them -needless_range_loop | warn | for-looping over a range of indices where an iterator over items would do -needless_return | warn | using a return statement like `return expr;` where an expression would suffice -non_ascii_literal | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead -option_unwrap_used | allow | using `Option.unwrap()`, which should at least get a better message using `expect()` -precedence | warn | expressions where precedence may trip up the unwary reader of the source; suggests adding parentheses, e.g. `x << 2 + y` will be parsed as `x << (2 + y)` -ptr_arg | allow | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively -range_step_by_zero | warn | using Range::step_by(0), which produces an infinite iterator -redundant_closure | warn | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`) -result_unwrap_used | allow | using `Result.unwrap()`, which might be better handled -single_match | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead -str_to_string | warn | using `to_string()` on a str, which should be `to_owned()` -string_add | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead -string_add_assign | allow | using `x = x + ..` where x is a `String`; suggests using `push_str()` instead -string_to_string | warn | calling `String.to_string()` which is a no-op -toplevel_ref_arg | warn | a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not `fn foo((ref x, ref y): (u8, u8))`) -unit_cmp | warn | comparing unit values (which is always `true` or `false`, respectively) -zero_width_space | deny | using a zero-width space in a string literal, which is confusing - -To use, add the following lines to your Cargo.toml: - -``` -[dependencies] -clippy = "*" -``` +name | default | meaning +-------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- +approx_constant | warn | the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found; suggests to use the constant +bad_bit_mask | deny | 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) +box_vec | warn | usage of `Box>`, vector elements are already on the heap +cast_possible_truncation | allow | casts that may cause truncation of the value, e.g `x as u8` where `x: u32`, or `x as i32` where `x: f32` +cast_possible_wrap | allow | casts that may cause wrapping around the value, e.g `x as i32` where `x: u32` and `x > i32::MAX` +cast_precision_loss | allow | casts that cause loss of precision, e.g `x as f32` where `x: u64` +cast_sign_loss | allow | casts from signed types to unsigned types, e.g `x as u32` where `x: i32` +cmp_nan | deny | comparisons to NAN (which will always return false, which is probably not intended) +cmp_owned | warn | creating owned instances for comparing with others, e.g. `x == "foo".to_string()` +collapsible_if | warn | two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }` +eq_op | warn | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`) +explicit_iter_loop | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do +float_cmp | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) +identity_op | warn | using identity operations, e.g. `x + 0` or `y / 1` +ineffective_bit_mask | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` +inline_always | warn | `#[inline(always)]` is a bad idea in most cases +iter_next_loop | warn | for-looping over `_.next()` which is probably not intended +len_without_is_empty | warn | traits and impls that have `.len()` but not `.is_empty()` +len_zero | warn | checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` could be used instead +let_and_return | warn | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a function +let_unit_value | warn | creating a let binding to a value of unit type, which usually can't be used afterwards +linkedlist | warn | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a RingBuf +match_ref_pats | warn | a match has all arms prefixed with `&`; the match expression can be dereferenced instead +modulo_one | warn | taking a number modulo 1, which always returns 0 +mut_mut | warn | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references) +needless_bool | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` +needless_lifetimes | warn | using explicit lifetimes for references in function arguments when elision rules would allow omitting them +needless_range_loop | warn | for-looping over a range of indices where an iterator over items would do +needless_return | warn | using a return statement like `return expr;` where an expression would suffice +non_ascii_literal | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead +option_unwrap_used | allow | using `Option.unwrap()`, which should at least get a better message using `expect()` +precedence | warn | expressions where precedence may trip up the unwary reader of the source; suggests adding parentheses, e.g. `x << 2 + y` will be parsed as `x << (2 + y)` +ptr_arg | allow | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively +range_step_by_zero | warn | using Range::step_by(0), which produces an infinite iterator +redundant_closure | warn | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`) +result_unwrap_used | allow | using `Result.unwrap()`, which might be better handled +single_match | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead +str_to_string | warn | using `to_string()` on a str, which should be `to_owned()` +string_add | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead +string_add_assign | allow | using `x = x + ..` where x is a `String`; suggests using `push_str()` instead +string_to_string | warn | calling `String.to_string()` which is a no-op +toplevel_ref_arg | warn | a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not `fn foo((ref x, ref y): (u8, u8))`) +type_complexity | warn | usage of very complex types; recommends factoring out parts into `type` definitions +unit_cmp | warn | comparing unit values (which is always `true` or `false`, respectively) +zero_width_space | deny | using a zero-width space in a string literal, which is confusing More to come, please [file an issue](https://github.com/Manishearth/rust-clippy/issues) if you have ideas! @@ -63,8 +62,8 @@ Compiler plugins are highly unstable and will only work with a nightly Rust for Add in your `Cargo.toml`: ```toml -[dependencies.clippy] -git = "https://github.com/Manishearth/rust-clippy" +[dependencies] +clippy = "*" ``` Sample `main.rs`: diff --git a/src/approx_const.rs b/src/approx_const.rs index cfd646765c9..3e0ba4eb669 100644 --- a/src/approx_const.rs +++ b/src/approx_const.rs @@ -37,10 +37,10 @@ impl LintPass for ApproxConstant { } fn check_lit(cx: &Context, lit: &Lit, span: Span) { - match &lit.node { - &LitFloat(ref str, TyF32) => check_known_consts(cx, span, str, "f32"), - &LitFloat(ref str, TyF64) => check_known_consts(cx, span, str, "f64"), - &LitFloatUnsuffixed(ref str) => check_known_consts(cx, span, str, "f{32, 64}"), + match lit.node { + LitFloat(ref str, TyF32) => check_known_consts(cx, span, str, "f32"), + LitFloat(ref str, TyF64) => check_known_consts(cx, span, str, "f64"), + LitFloatUnsuffixed(ref str) => check_known_consts(cx, span, str, "f{32, 64}"), _ => () } } diff --git a/src/attrs.rs b/src/attrs.rs index 3e451ac5eda..ad021f28a4d 100644 --- a/src/attrs.rs +++ b/src/attrs.rs @@ -71,14 +71,14 @@ fn is_relevant_block(block: &Block) -> bool { _ => () } } - block.expr.as_ref().map_or(false, |e| is_relevant_expr(&*e)) + block.expr.as_ref().map_or(false, |e| is_relevant_expr(e)) } fn is_relevant_expr(expr: &Expr) -> bool { match expr.node { ExprBlock(ref block) => is_relevant_block(block), ExprRet(Some(ref e)) | ExprParen(ref e) => - is_relevant_expr(&*e), + is_relevant_expr(e), ExprRet(None) | ExprBreak(_) | ExprMac(_) => false, ExprCall(ref path_expr, _) => { if let ExprPath(_, ref path) = path_expr.node { diff --git a/src/bit_mask.rs b/src/bit_mask.rs index 7789381da23..6537fcf4c1a 100644 --- a/src/bit_mask.rs +++ b/src/bit_mask.rs @@ -82,9 +82,9 @@ fn invert_cmp(cmp : BinOp_) -> BinOp_ { 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) => { + 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; } fetch_int_literal(cx, right).or_else(|| fetch_int_literal( cx, left)).map_or((), |mask| check_bit_mask(cx, op.node, @@ -182,13 +182,13 @@ fn check_ineffective_gt(cx: &Context, span: Span, m: u64, c: u64, op: &str) { } fn fetch_int_literal(cx: &Context, lit : &Expr) -> Option { - match &lit.node { - &ExprLit(ref lit_ptr) => { + match lit.node { + ExprLit(ref lit_ptr) => { if let &LitInt(value, _) = &lit_ptr.node { Option::Some(value) //TODO: Handle sign } else { Option::None } }, - &ExprPath(_, _) => { + ExprPath(_, _) => { // Important to let the borrow expire before the const lookup to avoid double // borrowing. let def_map = cx.tcx.def_map.borrow(); diff --git a/src/collapsible_if.rs b/src/collapsible_if.rs index 0b6dfc19e6b..e0b25b7283b 100644 --- a/src/collapsible_if.rs +++ b/src/collapsible_if.rs @@ -48,7 +48,6 @@ fn check_expr_expd(cx: &Context, e: &Expr, info: Option<&ExpnInfo>) { if e.span.expn_id != sp.expn_id { return; } - cx.sess().note(&format!("{:?} -- {:?}", e.span, sp)); span_help_and_lint(cx, COLLAPSIBLE_IF, e.span, "this if statement can be collapsed", &format!("try\nif {} && {} {}", @@ -80,7 +79,7 @@ fn single_stmt_of_block(block: &Block) -> Option<&Expr> { } else { None } } else { if block.stmts.is_empty() { - if let Some(ref p) = block.expr { Some(&*p) } else { None } + if let Some(ref p) = block.expr { Some(p) } else { None } } else { None } } } diff --git a/src/consts.rs b/src/consts.rs index 5056cc27a54..1a828317fc2 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -67,15 +67,16 @@ impl Constant { } /// convert this constant to a f64, if possible - pub fn as_float(&self) -> Option { - match *self { - ConstantByte(b) => Some(b as f64), - ConstantFloat(ref s, _) => s.parse().ok(), - ConstantInt(i, ty) => Some(if is_negative(ty) { - -(i as f64) } else { i as f64 }), - _ => None - } - } + #[allow(cast_precision_loss)] + pub fn as_float(&self) -> Option { + match *self { + ConstantByte(b) => Some(b as f64), + ConstantFloat(ref s, _) => s.parse().ok(), + ConstantInt(i, ty) => Some(if is_negative(ty) { + -(i as f64) } else { i as f64 }), + _ => None + } + } } impl PartialEq for Constant { @@ -221,7 +222,7 @@ fn neg_float_str(s: String) -> String { if s.starts_with('-') { s[1..].to_owned() } else { - format!("-{}", &*s) + format!("-{}", s) } } @@ -298,7 +299,7 @@ impl<'c, 'cc> ConstEvalContext<'c, 'cc> { ExprPath(_, _) => self.fetch_path(e), ExprBlock(ref block) => self.block(block), ExprIf(ref cond, ref then, ref otherwise) => - self.ifthenelse(&*cond, &*then, &*otherwise), + self.ifthenelse(cond, then, otherwise), ExprLit(ref lit) => Some(lit_to_constant(&lit.node)), ExprVec(ref vec) => self.multi(vec).map(ConstantVec), ExprTup(ref tup) => self.multi(tup).map(ConstantTuple), @@ -329,8 +330,13 @@ impl<'c, 'cc> ConstEvalContext<'c, 'cc> { /// lookup a possibly constant expression from a ExprPath fn fetch_path(&mut self, e: &Expr) -> Option { if let Some(lcx) = self.lcx { + let mut maybe_id = None; if let Some(&PathResolution { base_def: DefConst(id), ..}) = lcx.tcx.def_map.borrow().get(&e.id) { + maybe_id = Some(id); + } + // separate if lets to avoid doubleborrowing the defmap + if let Some(id) = maybe_id { if let Some(const_expr) = lookup_const_by_id(lcx.tcx, id, None) { let ret = self.expr(const_expr); if ret.is_some() { @@ -356,7 +362,7 @@ impl<'c, 'cc> ConstEvalContext<'c, 'cc> { if b { self.block(then) } else { - otherwise.as_ref().and_then(|ref expr| self.expr(expr)) + otherwise.as_ref().and_then(|expr| self.expr(expr)) } } else { None } } diff --git a/src/eq_op.rs b/src/eq_op.rs index 50b61e23356..ebc6aa17100 100644 --- a/src/eq_op.rs +++ b/src/eq_op.rs @@ -4,6 +4,7 @@ use syntax::ast_util as ast_util; use syntax::ptr::P; use syntax::codemap as code; +use consts::constant; use utils::span_lint; declare_lint! { @@ -22,7 +23,7 @@ impl LintPass for EqOp { 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) { + if is_cmp_or_bit(op) && is_exp_equal(cx, left, right) { span_lint(cx, EQ_OP, e.span, &format!( "equal expressions as operands to {}", ast_util::binop_to_string(op.node))); @@ -31,45 +32,49 @@ impl LintPass for EqOp { } } -pub fn is_exp_equal(left : &Expr, right : &Expr) -> bool { - match (&left.node, &right.node) { +pub fn is_exp_equal(cx: &Context, left : &Expr, right : &Expr) -> bool { + if 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), + is_exp_equal(cx, ll, rl) && is_exp_equal(cx, lr, rr), (&ExprBox(ref lpl, ref lbox), &ExprBox(ref rpl, ref rbox)) => - both(lpl, rpl, |l, r| is_exp_equal(l, r)) && - is_exp_equal(lbox, rbox), + both(lpl, rpl, |l, r| is_exp_equal(cx, l, r)) && + is_exp_equal(cx, lbox, rbox), (&ExprCall(ref lcallee, ref largs), - &ExprCall(ref rcallee, ref rargs)) => is_exp_equal(lcallee, - rcallee) && is_exps_equal(largs, rargs), + &ExprCall(ref rcallee, ref rargs)) => is_exp_equal(cx, lcallee, + rcallee) && is_exps_equal(cx, largs, rargs), (&ExprCast(ref lc, ref lty), &ExprCast(ref rc, ref rty)) => - is_ty_equal(lty, rty) && is_exp_equal(lc, rc), + is_ty_equal(cx, lty, rty) && is_exp_equal(cx, lc, rc), (&ExprField(ref lfexp, ref lfident), &ExprField(ref rfexp, ref rfident)) => - lfident.node == rfident.node && is_exp_equal(lfexp, rfexp), + lfident.node == rfident.node && is_exp_equal(cx, lfexp, rfexp), (&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node, (&ExprMethodCall(ref lident, ref lcty, ref lmargs), &ExprMethodCall(ref rident, ref rcty, ref rmargs)) => - lident.node == rident.node && is_tys_equal(lcty, rcty) && - is_exps_equal(lmargs, rmargs), - (&ExprParen(ref lparen), _) => is_exp_equal(lparen, right), - (_, &ExprParen(ref rparen)) => is_exp_equal(left, rparen), + lident.node == rident.node && is_tys_equal(cx, lcty, rcty) && + is_exps_equal(cx, lmargs, rmargs), + (&ExprParen(ref lparen), _) => is_exp_equal(cx, lparen, right), + (_, &ExprParen(ref rparen)) => is_exp_equal(cx, 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_exps_equal(ltup, rtup), + is_exps_equal(cx, ltup, rtup), (&ExprUnary(lunop, ref l), &ExprUnary(runop, ref r)) => - lunop == runop && is_exp_equal(l, r), - (&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(l, r), + lunop == runop && is_exp_equal(cx, l, r), + (&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r), + _ => false + } { return true; } + match (constant(cx, left), constant(cx, right)) { + (Some(l), Some(r)) => l == r, _ => false } } -fn is_exps_equal(left : &[P], right : &[P]) -> bool { - over(left, right, |l, r| is_exp_equal(l, r)) +fn is_exps_equal(cx: &Context, left : &[P], right : &[P]) -> bool { + over(left, right, |l, r| is_exp_equal(cx, l, r)) } fn is_path_equal(left : &Path, right : &Path) -> bool { @@ -85,29 +90,29 @@ 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 { +fn is_ty_equal(cx: &Context, left : &Ty, right : &Ty) -> bool { match (&left.node, &right.node) { - (&TyVec(ref lvec), &TyVec(ref rvec)) => is_ty_equal(lvec, rvec), + (&TyVec(ref lvec), &TyVec(ref rvec)) => is_ty_equal(cx, 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), + is_ty_equal(cx, lfvty, rfvty) && is_exp_equal(cx, lfvexp, rfvexp), + (&TyPtr(ref lmut), &TyPtr(ref rmut)) => is_mut_ty_equal(cx, lmut, rmut), (&TyRptr(ref ltime, ref lrmut), &TyRptr(ref rtime, ref rrmut)) => both(ltime, rtime, is_lifetime_equal) && - is_mut_ty_equal(lrmut, rrmut), + is_mut_ty_equal(cx, lrmut, rrmut), (&TyBareFn(ref lbare), &TyBareFn(ref rbare)) => - is_bare_fn_ty_equal(lbare, rbare), - (&TyTup(ref ltup), &TyTup(ref rtup)) => is_tys_equal(ltup, rtup), + is_bare_fn_ty_equal(cx, lbare, rbare), + (&TyTup(ref ltup), &TyTup(ref rtup)) => is_tys_equal(cx, ltup, rtup), (&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) => both(lq, rq, is_qself_equal) && is_path_equal(lpath, rpath), (&TyObjectSum(ref lsumty, ref lobounds), &TyObjectSum(ref rsumty, ref robounds)) => - is_ty_equal(lsumty, rsumty) && + is_ty_equal(cx, 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), + (&TyParen(ref lty), &TyParen(ref rty)) => is_ty_equal(cx, lty, rty), + (&TyTypeof(ref lof), &TyTypeof(ref rof)) => is_exp_equal(cx, lof, rof), (&TyInfer, &TyInfer) => true, _ => false } @@ -136,41 +141,41 @@ fn is_param_bounds_equal(left : &TyParamBounds, right : &TyParamBounds) 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_mut_ty_equal(cx: &Context, left : &MutTy, right : &MutTy) -> bool { + left.mutbl == right.mutbl && is_ty_equal(cx, &left.ty, &right.ty) } -fn is_bare_fn_ty_equal(left : &BareFnTy, right : &BareFnTy) -> bool { +fn is_bare_fn_ty_equal(cx: &Context, left : &BareFnTy, right : &BareFnTy) -> bool { left.unsafety == right.unsafety && left.abi == right.abi && is_lifetimedefs_equal(&left.lifetimes, &right.lifetimes) && - is_fndecl_equal(&left.decl, &right.decl) + is_fndecl_equal(cx, &left.decl, &right.decl) } -fn is_fndecl_equal(left : &P, right : &P) -> bool { +fn is_fndecl_equal(cx: &Context, left : &P, right : &P) -> bool { left.variadic == right.variadic && - is_args_equal(&left.inputs, &right.inputs) && - is_fnret_ty_equal(&left.output, &right.output) + is_args_equal(cx, &left.inputs, &right.inputs) && + is_fnret_ty_equal(cx, &left.output, &right.output) } -fn is_fnret_ty_equal(left : &FunctionRetTy, right : &FunctionRetTy) - -> bool { +fn is_fnret_ty_equal(cx: &Context, left : &FunctionRetTy, + right : &FunctionRetTy) -> bool { match (left, right) { (&NoReturn(_), &NoReturn(_)) | (&DefaultReturn(_), &DefaultReturn(_)) => true, - (&Return(ref lty), &Return(ref rty)) => is_ty_equal(lty, rty), + (&Return(ref lty), &Return(ref rty)) => is_ty_equal(cx, lty, rty), _ => false } } -fn is_arg_equal(l: &Arg, r : &Arg) -> bool { - is_ty_equal(&l.ty, &r.ty) && is_pat_equal(&l.pat, &r.pat) +fn is_arg_equal(cx: &Context, l: &Arg, r : &Arg) -> bool { + is_ty_equal(cx, &l.ty, &r.ty) && is_pat_equal(cx, &l.pat, &r.pat) } -fn is_args_equal(left : &[Arg], right : &[Arg]) -> bool { - over(left, right, is_arg_equal) +fn is_args_equal(cx: &Context, left : &[Arg], right : &[Arg]) -> bool { + over(left, right, |l, r| is_arg_equal(cx, l, r)) } -fn is_pat_equal(left : &Pat, right : &Pat) -> bool { +fn is_pat_equal(cx: &Context, left : &Pat, right : &Pat) -> bool { match(&left.node, &right.node) { (&PatWild(lwild), &PatWild(rwild)) => lwild == rwild, (&PatIdent(ref lmode, ref lident, Option::None), @@ -179,51 +184,51 @@ fn is_pat_equal(left : &Pat, right : &Pat) -> bool { (&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), + is_pat_equal(cx, lpat, rpat), (&PatEnum(ref lpath, ref lenum), &PatEnum(ref rpath, ref renum)) => is_path_equal(lpath, rpath) && both(lenum, renum, |l, r| - is_pats_equal(l, r)), + is_pats_equal(cx, l, r)), (&PatStruct(ref lpath, ref lfieldpat, lbool), &PatStruct(ref rpath, ref rfieldpat, rbool)) => lbool == rbool && is_path_equal(lpath, rpath) && - is_spanned_fieldpats_equal(lfieldpat, rfieldpat), - (&PatTup(ref ltup), &PatTup(ref rtup)) => is_pats_equal(ltup, rtup), + is_spanned_fieldpats_equal(cx, lfieldpat, rfieldpat), + (&PatTup(ref ltup), &PatTup(ref rtup)) => is_pats_equal(cx, ltup, rtup), (&PatBox(ref lboxed), &PatBox(ref rboxed)) => - is_pat_equal(lboxed, rboxed), + is_pat_equal(cx, 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), + is_pat_equal(cx, lpat, rpat) && lmut == rmut, + (&PatLit(ref llit), &PatLit(ref rlit)) => is_exp_equal(cx, llit, rlit), (&PatRange(ref lfrom, ref lto), &PatRange(ref rfrom, ref rto)) => - is_exp_equal(lfrom, rfrom) && is_exp_equal(lto, rto), + is_exp_equal(cx, lfrom, rfrom) && is_exp_equal(cx, lto, rto), (&PatVec(ref lfirst, Option::None, ref llast), &PatVec(ref rfirst, Option::None, ref rlast)) => - is_pats_equal(lfirst, rfirst) && is_pats_equal(llast, rlast), + is_pats_equal(cx, lfirst, rfirst) && is_pats_equal(cx, llast, rlast), (&PatVec(ref lfirst, Option::Some(ref lpat), ref llast), &PatVec(ref rfirst, Option::Some(ref rpat), ref rlast)) => - is_pats_equal(lfirst, rfirst) && is_pat_equal(lpat, rpat) && - is_pats_equal(llast, rlast), + is_pats_equal(cx, lfirst, rfirst) && is_pat_equal(cx, lpat, rpat) && + is_pats_equal(cx, llast, rlast), // I don't match macros for now, the code is slow enough as is ;-) _ => false } } -fn is_spanned_fieldpats_equal(left : &[code::Spanned], +fn is_spanned_fieldpats_equal(cx: &Context, left : &[code::Spanned], right : &[code::Spanned]) -> bool { - over(left, right, |l, r| is_fieldpat_equal(&l.node, &r.node)) + over(left, right, |l, r| is_fieldpat_equal(cx, &l.node, &r.node)) } -fn is_fieldpat_equal(left : &FieldPat, right : &FieldPat) -> bool { +fn is_fieldpat_equal(cx: &Context, 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) + is_pat_equal(cx, &left.pat, &right.pat) } fn is_ident_equal(left : &Ident, right : &Ident) -> bool { &left.name == &right.name && left.ctxt == right.ctxt } -fn is_pats_equal(left : &[P], right : &[P]) -> bool { - over(left, right, |l, r| is_pat_equal(l, r)) +fn is_pats_equal(cx: &Context, left : &[P], right : &[P]) -> bool { + over(left, right, |l, r| is_pat_equal(cx, l, r)) } fn is_lifetimedef_equal(left : &LifetimeDef, right : &LifetimeDef) @@ -241,8 +246,8 @@ fn is_lifetime_equal(left : &Lifetime, right : &Lifetime) -> bool { left.name == right.name } -fn is_tys_equal(left : &[P], right : &[P]) -> bool { - over(left, right, |l, r| is_ty_equal(l, r)) +fn is_tys_equal(cx: &Context, left : &[P], right : &[P]) -> bool { + over(left, right, |l, r| is_ty_equal(cx, l, r)) } fn over(left: &[X], right: &[X], mut eq_fn: F) -> bool diff --git a/src/eta_reduction.rs b/src/eta_reduction.rs index 6712e787278..481512abc62 100644 --- a/src/eta_reduction.rs +++ b/src/eta_reduction.rs @@ -18,11 +18,11 @@ impl LintPass for EtaPass { } fn check_expr(&mut self, cx: &Context, expr: &Expr) { - match &expr.node { - &ExprCall(_, ref args) | - &ExprMethodCall(_, _, ref args) => { + match expr.node { + ExprCall(_, ref args) | + ExprMethodCall(_, _, ref args) => { for arg in args { - check_closure(cx, &*arg) + check_closure(cx, arg) } }, _ => (), diff --git a/src/len_zero.rs b/src/len_zero.rs index 073dcea582d..ca3ce51bf7c 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -22,10 +22,10 @@ impl LintPass for LenZero { } fn check_item(&mut self, cx: &Context, item: &Item) { - match &item.node { - &ItemTrait(_, _, _, ref trait_items) => + match item.node { + ItemTrait(_, _, _, ref trait_items) => check_trait_items(cx, item, trait_items), - &ItemImpl(_, _, _, None, _, ref impl_items) => // only non-trait + ItemImpl(_, _, _, None, _, ref impl_items) => // only non-trait check_impl_items(cx, item, impl_items), _ => () } @@ -100,9 +100,9 @@ fn check_cmp(cx: &Context, span: Span, left: &Expr, right: &Expr, op: &str) { fn check_len_zero(cx: &Context, span: Span, method: &SpannedIdent, args: &[P], lit: &Lit, op: &str) { - if let &Spanned{node: LitInt(0, _), ..} = lit { + if let Spanned{node: LitInt(0, _), ..} = *lit { if method.node.name == "len" && args.len() == 1 && - has_is_empty(cx, &*args[0]) { + has_is_empty(cx, &args[0]) { span_lint(cx, LEN_ZERO, span, &format!( "consider replacing the len comparison with `{}{}.is_empty()`", op, snippet(cx, args[0].span, "_"))) diff --git a/src/lib.rs b/src/lib.rs index 50ebbd6d9fd..863ba2624dd 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,7 @@ #![feature(plugin_registrar, box_syntax)] #![feature(rustc_private, core, collections)] -#![feature(str_split_at)] +#![feature(str_split_at, num_bits_bytes)] +#![allow(unknown_lints)] #[macro_use] extern crate syntax; @@ -37,11 +38,11 @@ pub mod returns; pub mod lifetimes; pub mod loops; pub mod ranges; +pub mod matches; #[plugin_registrar] pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box types::TypePass as LintPassObject); - reg.register_lint_pass(box misc::MiscPass as LintPassObject); reg.register_lint_pass(box misc::TopLevelRefPass as LintPassObject); reg.register_lint_pass(box misc::CmpNan as LintPassObject); reg.register_lint_pass(box eq_op::EqOp as LintPassObject); @@ -68,6 +69,9 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box loops::LoopsPass as LintPassObject); reg.register_lint_pass(box lifetimes::LifetimePass as LintPassObject); reg.register_lint_pass(box ranges::StepByZero as LintPassObject); + reg.register_lint_pass(box types::CastPass as LintPassObject); + reg.register_lint_pass(box types::TypeComplexityPass as LintPassObject); + reg.register_lint_pass(box matches::MatchPass as LintPassObject); reg.register_lint_group("clippy", vec![ approx_const::APPROX_CONSTANT, @@ -84,6 +88,8 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::EXPLICIT_ITER_LOOP, loops::ITER_NEXT_LOOP, loops::NEEDLESS_RANGE_LOOP, + matches::MATCH_REF_PATS, + matches::SINGLE_MATCH, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, methods::STR_TO_STRING, @@ -93,7 +99,6 @@ pub fn plugin_registrar(reg: &mut Registry) { misc::FLOAT_CMP, misc::MODULO_ONE, misc::PRECEDENCE, - misc::SINGLE_MATCH, misc::TOPLEVEL_REF_ARG, mut_mut::MUT_MUT, needless_bool::NEEDLESS_BOOL, @@ -104,8 +109,13 @@ pub fn plugin_registrar(reg: &mut Registry) { strings::STRING_ADD, strings::STRING_ADD_ASSIGN, types::BOX_VEC, + types::CAST_POSSIBLE_TRUNCATION, + types::CAST_POSSIBLE_WRAP, + types::CAST_PRECISION_LOSS, + types::CAST_SIGN_LOSS, types::LET_UNIT_VALUE, types::LINKEDLIST, + types::TYPE_COMPLEXITY, types::UNIT_CMP, unicode::NON_ASCII_LITERAL, unicode::ZERO_WIDTH_SPACE, diff --git a/src/lifetimes.rs b/src/lifetimes.rs index 9d07df4a3ed..660d68535bd 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -26,14 +26,14 @@ impl LintPass for LifetimePass { fn check_impl_item(&mut self, cx: &Context, item: &ImplItem) { if let MethodImplItem(ref sig, _) = item.node { - check_fn_inner(cx, &*sig.decl, Some(&sig.explicit_self), + check_fn_inner(cx, &sig.decl, Some(&sig.explicit_self), &sig.generics.lifetimes, item.span); } } fn check_trait_item(&mut self, cx: &Context, item: &TraitItem) { if let MethodTraitItem(ref sig, _) = item.node { - check_fn_inner(cx, &*sig.decl, Some(&sig.explicit_self), + check_fn_inner(cx, &sig.decl, Some(&sig.explicit_self), &sig.generics.lifetimes, item.span); } } @@ -92,7 +92,7 @@ fn could_use_elision(func: &FnDecl, slf: Option<&ExplicitSelf>, } // extract lifetimes in input argument types for arg in &func.inputs { - walk_ty(&mut input_visitor, &*arg.ty); + walk_ty(&mut input_visitor, &arg.ty); } // extract lifetimes in output type if let Return(ref ty) = func.output { diff --git a/src/loops.rs b/src/loops.rs index 36bab550b67..ca8d3990fc5 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -1,10 +1,9 @@ use rustc::lint::*; use syntax::ast::*; use syntax::visit::{Visitor, walk_expr}; -use rustc::middle::ty; use std::collections::HashSet; -use utils::{snippet, span_lint, get_parent_expr, match_def_path}; +use utils::{snippet, span_lint, get_parent_expr, match_trait_method}; declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn, "for-looping over a range of indices where an iterator over items would do" } @@ -68,16 +67,10 @@ impl LintPass for LoopsPass { object, object)); // check for looping over Iterator::next() which is not what you want } else if method_name == "next" { - let method_call = ty::MethodCall::expr(arg.id); - let trt_id = cx.tcx.tables - .borrow().method_map.get(&method_call) - .and_then(|callee| cx.tcx.trait_of_item(callee.def_id)); - if let Some(trt_id) = trt_id { - if match_def_path(cx, trt_id, &["core", "iter", "Iterator"]) { - span_lint(cx, ITER_NEXT_LOOP, expr.span, - "you are iterating over `Iterator::next()` which is an Option; \ - this will compile but is probably not what you want"); - } + if match_trait_method(cx, arg, &["core", "iter", "Iterator"]) { + span_lint(cx, ITER_NEXT_LOOP, expr.span, + "you are iterating over `Iterator::next()` which is an Option; \ + this will compile but is probably not what you want"); } } } @@ -102,9 +95,9 @@ fn recover_for_loop(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> { let PatEnum(_, Some(ref somepats)) = innerarms[0].pats[0].node, somepats.len() == 1 ], { - return Some((&*somepats[0], - &*iterargs[0], - &*innerarms[0].body)); + return Some((&somepats[0], + &iterargs[0], + &innerarms[0].body)); } } None diff --git a/src/matches.rs b/src/matches.rs new file mode 100644 index 00000000000..d1c74daf2cd --- /dev/null +++ b/src/matches.rs @@ -0,0 +1,86 @@ +use rustc::lint::*; +use syntax::ast; +use syntax::ast::*; +use std::borrow::Cow; + +use utils::{snippet, snippet_block, span_lint, span_help_and_lint}; + +declare_lint!(pub SINGLE_MATCH, Warn, + "a match statement with a single nontrivial arm (i.e, where the other arm \ + is `_ => {}`) is used; recommends `if let` instead"); +declare_lint!(pub MATCH_REF_PATS, Warn, + "a match has all arms prefixed with `&`; the match expression can be \ + dereferenced instead"); + +#[allow(missing_copy_implementations)] +pub struct MatchPass; + +impl LintPass for MatchPass { + fn get_lints(&self) -> LintArray { + lint_array!(SINGLE_MATCH, MATCH_REF_PATS) + } + + fn check_expr(&mut self, cx: &Context, expr: &Expr) { + if let ExprMatch(ref ex, ref arms, ast::MatchSource::Normal) = expr.node { + // check preconditions for SINGLE_MATCH + // only two arms + if arms.len() == 2 && + // both of the arms have a single pattern and no guard + arms[0].pats.len() == 1 && arms[0].guard.is_none() && + arms[1].pats.len() == 1 && arms[1].guard.is_none() && + // and the second pattern is a `_` wildcard: this is not strictly necessary, + // since the exhaustiveness check will ensure the last one is a catch-all, + // but in some cases, an explicit match is preferred to catch situations + // when an enum is extended, so we don't consider these cases + arms[1].pats[0].node == PatWild(PatWildSingle) && + // finally, we don't want any content in the second arm (unit or empty block) + is_unit_expr(&arms[1].body) + { + let body_code = snippet_block(cx, arms[0].body.span, ".."); + let body_code = if let ExprBlock(_) = arms[0].body.node { + body_code + } else { + Cow::Owned(format!("{{ {} }}", body_code)) + }; + span_help_and_lint(cx, SINGLE_MATCH, expr.span, + "you seem to be trying to use match for \ + destructuring a single pattern. Did you mean to \ + use `if let`?", + &format!("try\nif let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, ex.span, ".."), + body_code) + ); + } + + // check preconditions for MATCH_REF_PATS + if has_only_ref_pats(arms) { + if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { + span_lint(cx, MATCH_REF_PATS, expr.span, &format!( + "you don't need to add `&` to both the expression to match \ + and the patterns: use `match {} {{ ...`", snippet(cx, inner.span, ".."))); + } else { + span_lint(cx, MATCH_REF_PATS, expr.span, &format!( + "instead of prefixing all patterns with `&`, you can dereference the \ + expression to match: `match *{} {{ ...`", snippet(cx, ex.span, ".."))); + } + } + } + } +} + +fn is_unit_expr(expr: &Expr) -> bool { + match expr.node { + ExprTup(ref v) if v.is_empty() => true, + ExprBlock(ref b) if b.stmts.is_empty() && b.expr.is_none() => true, + _ => false, + } +} + +fn has_only_ref_pats(arms: &[Arm]) -> bool { + arms.iter().flat_map(|a| &a.pats).all(|p| match p.node { + PatRegion(..) => true, // &-patterns + PatWild(..) => true, // an "anything" wildcard is also fine + _ => false, + }) +} diff --git a/src/methods.rs b/src/methods.rs index f2df736bebc..07693e11d99 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -2,7 +2,8 @@ use syntax::ast::*; use rustc::lint::*; use rustc::middle::ty; -use utils::{span_lint, match_def_path, walk_ptrs_ty}; +use utils::{span_lint, match_type, walk_ptrs_ty}; +use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; #[derive(Copy,Clone)] pub struct MethodsPass; @@ -16,45 +17,32 @@ declare_lint!(pub STR_TO_STRING, Warn, declare_lint!(pub STRING_TO_STRING, Warn, "calling `String.to_string()` which is a no-op"); -#[allow(unused_imports)] impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { - { - // In case stuff gets moved around - use core::option::Option; - use core::result::Result; - use collections::string::String; - } if let ExprMethodCall(ref ident, _, ref args) = expr.node { - let ref obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])).sty; + let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(&args[0])); if ident.node.name == "unwrap" { - if let ty::TyEnum(did, _) = *obj_ty { - if match_def_path(cx, did.did, &["core", "option", "Option"]) { - span_lint(cx, OPTION_UNWRAP_USED, expr.span, - "used unwrap() on an Option value. If you don't want \ - to handle the None case gracefully, consider using - expect() to provide a better panic message"); - } - else if match_def_path(cx, did.did, &["core", "result", "Result"]) { - span_lint(cx, RESULT_UNWRAP_USED, expr.span, - "used unwrap() on a Result value. Graceful handling \ - of Err values is preferred"); - } + if match_type(cx, obj_ty, &OPTION_PATH) { + span_lint(cx, OPTION_UNWRAP_USED, expr.span, + "used unwrap() on an Option value. If you don't want \ + to handle the None case gracefully, consider using \ + expect() to provide a better panic message"); + } else if match_type(cx, obj_ty, &RESULT_PATH) { + span_lint(cx, RESULT_UNWRAP_USED, expr.span, + "used unwrap() on a Result value. Graceful handling \ + of Err values is preferred"); } } else if ident.node.name == "to_string" { - if let ty::TyStr = *obj_ty { + if obj_ty.sty == ty::TyStr { span_lint(cx, STR_TO_STRING, expr.span, "`str.to_owned()` is faster"); - } - else if let ty::TyStruct(did, _) = *obj_ty { - if match_def_path(cx, did.did, &["collections", "string", "String"]) { - span_lint(cx, STRING_TO_STRING, expr.span, - "`String.to_string()` is a no-op") - } + } else if match_type(cx, obj_ty, &STRING_PATH) { + span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op; use \ + `clone()` to make a copy"); } } } diff --git a/src/misc.rs b/src/misc.rs index aca849931bb..2290af38bb5 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -1,75 +1,14 @@ use rustc::lint::*; use syntax::ptr::P; -use syntax::ast; use syntax::ast::*; use syntax::ast_util::{is_comparison_binop, binop_to_string}; use syntax::codemap::{Span, Spanned}; use syntax::visit::FnKind; use rustc::middle::ty; -use std::borrow::Cow; -use utils::{match_path, snippet, snippet_block, span_lint, span_help_and_lint, walk_ptrs_ty}; +use utils::{match_path, snippet, span_lint, walk_ptrs_ty}; use consts::constant; -/// Handles uncategorized lints -/// Currently handles linting of if-let-able matches -#[allow(missing_copy_implementations)] -pub struct MiscPass; - - -declare_lint!(pub SINGLE_MATCH, Warn, - "a match statement with a single nontrivial arm (i.e, where the other arm \ - is `_ => {}`) is used; recommends `if let` instead"); - -impl LintPass for MiscPass { - fn get_lints(&self) -> LintArray { - lint_array!(SINGLE_MATCH) - } - - fn check_expr(&mut self, cx: &Context, expr: &Expr) { - if let ExprMatch(ref ex, ref arms, ast::MatchSource::Normal) = expr.node { - // check preconditions: only two arms - if arms.len() == 2 && - // both of the arms have a single pattern and no guard - arms[0].pats.len() == 1 && arms[0].guard.is_none() && - arms[1].pats.len() == 1 && arms[1].guard.is_none() && - // and the second pattern is a `_` wildcard: this is not strictly necessary, - // since the exhaustiveness check will ensure the last one is a catch-all, - // but in some cases, an explicit match is preferred to catch situations - // when an enum is extended, so we don't consider these cases - arms[1].pats[0].node == PatWild(PatWildSingle) && - // finally, we don't want any content in the second arm (unit or empty block) - is_unit_expr(&*arms[1].body) - { - let body_code = snippet_block(cx, arms[0].body.span, ".."); - let body_code = if let ExprBlock(_) = arms[0].body.node { - body_code - } else { - Cow::Owned(format!("{{ {} }}", body_code)) - }; - span_help_and_lint(cx, SINGLE_MATCH, expr.span, - "you seem to be trying to use match for \ - destructuring a single pattern. Did you mean to \ - use `if let`?", - &*format!("try\nif let {} = {} {}", - snippet(cx, arms[0].pats[0].span, ".."), - snippet(cx, ex.span, ".."), - body_code) - ); - } - } - } -} - -fn is_unit_expr(expr: &Expr) -> bool { - match expr.node { - ExprTup(ref v) if v.is_empty() => true, - ExprBlock(ref b) if b.stmts.is_empty() && b.expr.is_none() => true, - _ => false, - } -} - - declare_lint!(pub TOPLEVEL_REF_ARG, Warn, "a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not \ `fn foo((ref x, ref y): (u8, u8))`)"); @@ -236,8 +175,8 @@ impl LintPass for CmpOwned { } fn check_to_owned(cx: &Context, expr: &Expr, other_span: Span) { - match &expr.node { - &ExprMethodCall(Spanned{node: ref ident, ..}, _, ref args) => { + match expr.node { + ExprMethodCall(Spanned{node: ref ident, ..}, _, ref args) => { let name = ident.name; if name == "to_string" || name == "to_owned" && is_str_arg(cx, args) { @@ -247,7 +186,7 @@ fn check_to_owned(cx: &Context, expr: &Expr, other_span: Span) { snippet(cx, other_span, ".."))) } }, - &ExprCall(ref path, _) => { + ExprCall(ref path, _) => { if let &ExprPath(None, ref path) = &path.node { if match_path(path, &["String", "from_str"]) || match_path(path, &["String", "from"]) { @@ -264,7 +203,7 @@ fn check_to_owned(cx: &Context, expr: &Expr, other_span: Span) { fn is_str_arg(cx: &Context, args: &[P]) -> bool { args.len() == 1 && if let ty::TyStr = - walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])).sty { true } else { false } + walk_ptrs_ty(cx.tcx.expr_ty(&args[0])).sty { true } else { false } } declare_lint!(pub MODULO_ONE, Warn, "taking a number modulo 1, which always returns 0"); diff --git a/src/needless_bool.rs b/src/needless_bool.rs index 18d98f1f063..0fe52c44189 100644 --- a/src/needless_bool.rs +++ b/src/needless_bool.rs @@ -5,7 +5,7 @@ use rustc::lint::*; use syntax::ast::*; -use utils::{de_p, span_lint, snippet}; +use utils::{span_lint, snippet}; declare_lint! { pub NEEDLESS_BOOL, @@ -55,14 +55,14 @@ impl LintPass for NeedlessBool { fn fetch_bool_block(block: &Block) -> Option { if block.stmts.is_empty() { - block.expr.as_ref().map(de_p).and_then(fetch_bool_expr) + block.expr.as_ref().and_then(|e| fetch_bool_expr(e)) } else { None } } fn fetch_bool_expr(expr: &Expr) -> Option { - match &expr.node { - &ExprBlock(ref block) => fetch_bool_block(block), - &ExprLit(ref lit_ptr) => if let &LitBool(value) = &lit_ptr.node { + match expr.node { + ExprBlock(ref block) => fetch_bool_block(block), + ExprLit(ref lit_ptr) => if let LitBool(value) = lit_ptr.node { Some(value) } else { None }, _ => None } diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index 2748d187a4e..bcbd8dad68a 100644 --- a/src/ptr_arg.rs +++ b/src/ptr_arg.rs @@ -4,10 +4,10 @@ use rustc::lint::*; use syntax::ast::*; -use syntax::codemap::Span; +use rustc::middle::ty; -use types::match_ty_unwrap; -use utils::span_lint; +use utils::{span_lint, match_type}; +use utils::{STRING_PATH, VEC_PATH}; declare_lint! { pub PTR_ARG, @@ -45,22 +45,19 @@ impl LintPass for PtrArg { fn check_fn(cx: &Context, decl: &FnDecl) { for arg in &decl.inputs { - match &arg.ty.node { - &TyPtr(ref p) | &TyRptr(_, ref p) => - check_ptr_subtype(cx, arg.ty.span, &p.ty), - _ => () + if let Some(pat_ty) = cx.tcx.pat_ty_opt(&arg.pat) { + if let ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = pat_ty.sty { + if match_type(cx, ty, &VEC_PATH) { + span_lint(cx, PTR_ARG, arg.ty.span, + "writing `&Vec<_>` instead of `&[_]` involves one more reference \ + and cannot be used with non-Vec-based slices. Consider changing \ + the type to `&[...]`"); + } else if match_type(cx, ty, &STRING_PATH) { + span_lint(cx, PTR_ARG, arg.ty.span, + "writing `&String` instead of `&str` involves a new object \ + where a slice will do. Consider changing the type to `&str`"); + } + } } } } - -fn check_ptr_subtype(cx: &Context, span: Span, ty: &Ty) { - match_ty_unwrap(ty, &["Vec"]).map_or_else(|| match_ty_unwrap(ty, - &["String"]).map_or((), |_| { - span_lint(cx, PTR_ARG, span, - "writing `&String` instead of `&str` involves a new object \ - where a slice will do. Consider changing the type to `&str`") - }), |_| span_lint(cx, PTR_ARG, span, - "writing `&Vec<_>` instead of \ - `&[_]` involves one more reference and cannot be used with \ - non-Vec-based slices. Consider changing the type to `&[...]`")) -} diff --git a/src/ranges.rs b/src/ranges.rs index bbe65285d58..914b4daa6be 100644 --- a/src/ranges.rs +++ b/src/ranges.rs @@ -1,8 +1,7 @@ use rustc::lint::{Context, LintArray, LintPass}; -use rustc::middle::ty::TypeVariants::TyStruct; use syntax::ast::*; use syntax::codemap::Spanned; -use utils::{match_def_path}; +use utils::match_type; declare_lint! { pub RANGE_STEP_BY_ZERO, Warn, @@ -21,7 +20,7 @@ impl LintPass for StepByZero { if let ExprMethodCall(Spanned { node: ref ident, .. }, _, ref args) = expr.node { // Only warn on literal ranges. - if ident.name.as_str() == "step_by" && args.len() == 2 && + if ident.name == "step_by" && args.len() == 2 && is_range(cx, &args[0]) && is_lit_zero(&args[1]) { cx.span_lint(RANGE_STEP_BY_ZERO, expr.span, "Range::step_by(0) produces an infinite iterator. \ @@ -34,11 +33,9 @@ impl LintPass for StepByZero { fn is_range(cx: &Context, expr: &Expr) -> bool { // No need for walk_ptrs_ty here because step_by moves self, so it // can't be called on a borrowed range. - if let TyStruct(did, _) = cx.tcx.expr_ty(expr).sty { - // Note: RangeTo and RangeFull don't have step_by - match_def_path(cx, did.did, &["core", "ops", "Range"]) || - match_def_path(cx, did.did, &["core", "ops", "RangeFrom"]) - } else { false } + let ty = cx.tcx.expr_ty(expr); + // Note: RangeTo and RangeFull don't have step_by + match_type(cx, ty, &["core", "ops", "Range"]) || match_type(cx, ty, &["core", "ops", "RangeFrom"]) } fn is_lit_zero(expr: &Expr) -> bool { diff --git a/src/returns.rs b/src/returns.rs index df0b93f301e..301072f7912 100644 --- a/src/returns.rs +++ b/src/returns.rs @@ -50,7 +50,7 @@ impl ReturnPass { // a match expr, check all arms ExprMatch(_, ref arms, _) => { for arm in arms { - self.check_final_expr(cx, &*arm.body); + self.check_final_expr(cx, &arm.body); } } _ => { } @@ -76,7 +76,7 @@ impl ReturnPass { let PatIdent(_, Spanned { node: id, .. }, _) = local.pat.node, let Some(ref retexpr) = block.expr, let ExprPath(_, ref path) = retexpr.node, - match_path(path, &[&*id.name.as_str()]) + match_path(path, &[&id.name.as_str()]) ], { self.emit_let_lint(cx, retexpr.span, initexpr.span); } diff --git a/src/strings.rs b/src/strings.rs index 7981b785850..d03f4d53c60 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -4,12 +4,12 @@ //! disable the subsumed lint unless it has a higher level use rustc::lint::*; -use rustc::middle::ty::TypeVariants::TyStruct; use syntax::ast::*; use syntax::codemap::Spanned; use eq_op::is_exp_equal; -use utils::{match_def_path, span_lint, walk_ptrs_ty, get_parent_expr}; +use utils::{match_type, span_lint, walk_ptrs_ty, get_parent_expr}; +use utils::STRING_PATH; declare_lint! { pub STRING_ADD_ASSIGN, @@ -41,7 +41,7 @@ impl LintPass for StringAdd { if let Some(ref p) = parent { if let &ExprAssign(ref target, _) = &p.node { // avoid duplicate matches - if is_exp_equal(target, left) { return; } + if is_exp_equal(cx, target, left) { return; } } } } @@ -51,7 +51,7 @@ impl LintPass for StringAdd { Consider using `String::push_str()` instead") } } else if let &ExprAssign(ref target, ref src) = &e.node { - if is_string(cx, target) && is_add(src, target) { + if is_string(cx, target) && is_add(cx, src, target) { span_lint(cx, STRING_ADD_ASSIGN, e.span, "you assigned the result of adding something to this string. \ Consider using `String::push_str()` instead") @@ -61,19 +61,17 @@ impl LintPass for StringAdd { } fn is_string(cx: &Context, e: &Expr) -> bool { - let ty = walk_ptrs_ty(cx.tcx.expr_ty(e)); - if let TyStruct(did, _) = ty.sty { - match_def_path(cx, did.did, &["collections", "string", "String"]) - } else { false } + match_type(cx, walk_ptrs_ty(cx.tcx.expr_ty(e)), &STRING_PATH) } -fn is_add(src: &Expr, target: &Expr) -> bool { - match &src.node { - &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => - is_exp_equal(target, left), - &ExprBlock(ref block) => block.stmts.is_empty() && - block.expr.as_ref().map_or(false, |expr| is_add(&*expr, target)), - &ExprParen(ref expr) => is_add(&*expr, target), +fn is_add(cx: &Context, src: &Expr, target: &Expr) -> bool { + match src.node { + ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => + is_exp_equal(cx, target, left), + ExprBlock(ref block) => block.stmts.is_empty() && + block.expr.as_ref().map_or(false, + |expr| is_add(cx, expr, target)), + ExprParen(ref expr) => is_add(cx, expr, target), _ => false } } diff --git a/src/types.rs b/src/types.rs index 617c51fd961..7479a65b6ee 100644 --- a/src/types.rs +++ b/src/types.rs @@ -2,11 +2,13 @@ use rustc::lint::*; use syntax::ast; use syntax::ast::*; use syntax::ast_util::{is_comparison_binop, binop_to_string}; -use syntax::ptr::P; +use syntax::codemap::Span; +use syntax::visit::{FnKind, Visitor, walk_ty}; use rustc::middle::ty; use syntax::codemap::ExpnInfo; -use utils::{in_macro, snippet, span_lint, span_help_and_lint}; +use utils::{in_macro, match_type, snippet, span_lint, span_help_and_lint, in_external_macro}; +use utils::{LL_PATH, VEC_PATH}; /// Handles all the linting of funky types #[allow(missing_copy_implementations)] @@ -18,61 +20,26 @@ declare_lint!(pub LINKEDLIST, Warn, "usage of LinkedList, usually a vector is faster, or a more specialized data \ structure like a RingBuf"); -/// Matches a type with a provided string, and returns its type parameters if successful -pub fn match_ty_unwrap<'a>(ty: &'a Ty, segments: &[&str]) -> Option<&'a [P]> { - match ty.node { - TyPath(_, Path {segments: ref seg, ..}) => { - // So ast::Path isn't the full path, just the tokens that were provided. - // I could muck around with the maps and find the full path - // however the more efficient way is to simply reverse the iterators and zip them - // which will compare them in reverse until one of them runs out of segments - if seg.iter().rev().zip(segments.iter().rev()).all(|(a,b)| a.identifier.name == b) { - match seg[..].last() { - Some(&PathSegment {parameters: AngleBracketedParameters(ref a), ..}) => { - Some(&a.types[..]) - } - _ => None - } - } else { - None - } - }, - _ => None - } -} - -#[allow(unused_imports)] impl LintPass for TypePass { fn get_lints(&self) -> LintArray { lint_array!(BOX_VEC, LINKEDLIST) } - fn check_ty(&mut self, cx: &Context, ty: &ast::Ty) { - { - // In case stuff gets moved around - use std::boxed::Box; - use std::vec::Vec; - } - match_ty_unwrap(ty, &["std", "boxed", "Box"]).and_then(|t| t.first()) - .and_then(|t| match_ty_unwrap(&**t, &["std", "vec", "Vec"])) - .map(|_| { - span_help_and_lint(cx, BOX_VEC, ty.span, - "you seem to be trying to use `Box>`. Did you mean to use `Vec`?", - "`Vec` is already on the heap, `Box>` makes an extra allocation"); - }); - { - // In case stuff gets moved around - use collections::linked_list::LinkedList as DL1; - use std::collections::linked_list::LinkedList as DL2; - } - let dlists = [vec!["std","collections","linked_list","LinkedList"], - vec!["collections","linked_list","LinkedList"]]; - for path in &dlists { - if match_ty_unwrap(ty, &path[..]).is_some() { - span_help_and_lint(cx, LINKEDLIST, ty.span, - "I see you're using a LinkedList! Perhaps you meant some other data structure?", - "a RingBuf might work"); - return; + fn check_ty(&mut self, cx: &Context, ast_ty: &ast::Ty) { + if let Some(ty) = cx.tcx.ast_ty_to_ty_cache.borrow().get(&ast_ty.id) { + if let ty::TyBox(ref inner) = ty.sty { + if match_type(cx, inner, &VEC_PATH) { + span_help_and_lint( + cx, BOX_VEC, ast_ty.span, + "you seem to be trying to use `Box>`. Did you mean to use `Vec`?", + "`Vec` is already on the heap, `Box>` makes an extra allocation"); + } + } + else if match_type(cx, ty, &LL_PATH) { + span_help_and_lint( + cx, LINKEDLIST, ast_ty.span, + "I see you're using a LinkedList! Perhaps you meant some other data structure?", + "a RingBuf might work"); } } } @@ -88,7 +55,7 @@ declare_lint!(pub LET_UNIT_VALUE, Warn, fn check_let_unit(cx: &Context, decl: &Decl, info: Option<&ExpnInfo>) { if in_macro(cx, info) { return; } if let DeclLocal(ref local) = decl.node { - let bindtype = &cx.tcx.pat_ty(&*local.pat).sty; + let bindtype = &cx.tcx.pat_ty(&local.pat).sty; if *bindtype == ty::TyTuple(vec![]) { span_lint(cx, LET_UNIT_VALUE, decl.span, &format!( "this let-binding has unit value. Consider omitting `let {} =`", @@ -136,3 +103,270 @@ impl LintPass for UnitCmp { } } } + +pub struct CastPass; + +declare_lint!(pub CAST_PRECISION_LOSS, Allow, + "casts that cause loss of precision, e.g `x as f32` where `x: u64`"); +declare_lint!(pub CAST_SIGN_LOSS, Allow, + "casts from signed types to unsigned types, e.g `x as u32` where `x: i32`"); +declare_lint!(pub CAST_POSSIBLE_TRUNCATION, Allow, + "casts that may cause truncation of the value, e.g `x as u8` where `x: u32`, or `x as i32` where `x: f32`"); +declare_lint!(pub CAST_POSSIBLE_WRAP, Allow, + "casts that may cause wrapping around the value, e.g `x as i32` where `x: u32` and `x > i32::MAX`"); + +/// Returns the size in bits of an integral type. +/// Will return 0 if the type is not an int or uint variant +fn int_ty_to_nbits(typ: &ty::TyS) -> usize { + let n = match typ.sty { + ty::TyInt(i) => 4 << (i as usize), + ty::TyUint(u) => 4 << (u as usize), + _ => 0 + }; + // n == 4 is the usize/isize case + if n == 4 { ::std::usize::BITS } else { n } +} + +fn is_isize_or_usize(typ: &ty::TyS) -> bool { + match typ.sty { + ty::TyInt(ast::TyIs) | ty::TyUint(ast::TyUs) => true, + _ => false + } +} + +fn span_precision_loss_lint(cx: &Context, expr: &Expr, cast_from: &ty::TyS, cast_to_f64: bool) { + let mantissa_nbits = if cast_to_f64 {52} else {23}; + let arch_dependent = is_isize_or_usize(cast_from) && cast_to_f64; + let arch_dependent_str = "on targets with 64-bit wide pointers "; + let from_nbits_str = if arch_dependent {"64".to_owned()} + else if is_isize_or_usize(cast_from) {"32 or 64".to_owned()} + else {int_ty_to_nbits(cast_from).to_string()}; + span_lint(cx, CAST_PRECISION_LOSS, expr.span, + &format!("casting {0} to {1} causes a loss of precision {2}\ + ({0} is {3} bits wide, but {1}'s mantissa is only {4} bits wide)", + cast_from, if cast_to_f64 {"f64"} else {"f32"}, + if arch_dependent {arch_dependent_str} else {""}, + from_nbits_str, + mantissa_nbits)); +} + +enum ArchSuffix { + _32, _64, None +} + +fn check_truncation_and_wrapping(cx: &Context, expr: &Expr, cast_from: &ty::TyS, cast_to: &ty::TyS) { + let arch_64_suffix = " on targets with 64-bit wide pointers"; + let arch_32_suffix = " on targets with 32-bit wide pointers"; + let cast_unsigned_to_signed = !cast_from.is_signed() && cast_to.is_signed(); + let (from_nbits, to_nbits) = (int_ty_to_nbits(cast_from), int_ty_to_nbits(cast_to)); + let (span_truncation, suffix_truncation, span_wrap, suffix_wrap) = + match (is_isize_or_usize(cast_from), is_isize_or_usize(cast_to)) { + (true, true) | (false, false) => ( + to_nbits < from_nbits, + ArchSuffix::None, + to_nbits == from_nbits && cast_unsigned_to_signed, + ArchSuffix::None + ), + (true, false) => ( + to_nbits <= 32, + if to_nbits == 32 {ArchSuffix::_64} else {ArchSuffix::None}, + to_nbits <= 32 && cast_unsigned_to_signed, + ArchSuffix::_32 + ), + (false, true) => ( + from_nbits == 64, + ArchSuffix::_32, + cast_unsigned_to_signed, + if from_nbits == 64 {ArchSuffix::_64} else {ArchSuffix::_32} + ), + }; + if span_truncation { + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may truncate the value{}", + cast_from, cast_to, + match suffix_truncation { + ArchSuffix::_32 => arch_32_suffix, + ArchSuffix::_64 => arch_64_suffix, + ArchSuffix::None => "" })); + } + if span_wrap { + span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, + &format!("casting {} to {} may wrap around the value{}", + cast_from, cast_to, + match suffix_wrap { + ArchSuffix::_32 => arch_32_suffix, + ArchSuffix::_64 => arch_64_suffix, + ArchSuffix::None => "" })); + } +} + +impl LintPass for CastPass { + fn get_lints(&self) -> LintArray { + lint_array!(CAST_PRECISION_LOSS, + CAST_SIGN_LOSS, + CAST_POSSIBLE_TRUNCATION, + CAST_POSSIBLE_WRAP) + } + + fn check_expr(&mut self, cx: &Context, expr: &Expr) { + if let ExprCast(ref ex, _) = expr.node { + let (cast_from, cast_to) = (cx.tcx.expr_ty(ex), cx.tcx.expr_ty(expr)); + if cast_from.is_numeric() && cast_to.is_numeric() && !in_external_macro(cx, expr.span) { + match (cast_from.is_integral(), cast_to.is_integral()) { + (true, false) => { + let from_nbits = int_ty_to_nbits(cast_from); + let to_nbits = if let ty::TyFloat(ast::TyF32) = cast_to.sty {32} else {64}; + if is_isize_or_usize(cast_from) || from_nbits >= to_nbits { + span_precision_loss_lint(cx, expr, cast_from, to_nbits == 64); + } + }, + (false, true) => { + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may truncate the value", cast_from, cast_to)); + if !cast_to.is_signed() { + span_lint(cx, CAST_SIGN_LOSS, expr.span, + &format!("casting {} to {} may lose the sign of the value", cast_from, cast_to)); + } + }, + (true, true) => { + if cast_from.is_signed() && !cast_to.is_signed() { + span_lint(cx, CAST_SIGN_LOSS, expr.span, + &format!("casting {} to {} may lose the sign of the value", cast_from, cast_to)); + } + check_truncation_and_wrapping(cx, expr, cast_from, cast_to); + } + (false, false) => { + if let (&ty::TyFloat(ast::TyF64), + &ty::TyFloat(ast::TyF32)) = (&cast_from.sty, &cast_to.sty) { + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, "casting f64 to f32 may truncate the value"); + } + } + } + } + } + } +} + +declare_lint!(pub TYPE_COMPLEXITY, Warn, + "usage of very complex types; recommends factoring out parts into `type` definitions"); + +#[allow(missing_copy_implementations)] +pub struct TypeComplexityPass; + +impl LintPass for TypeComplexityPass { + fn get_lints(&self) -> LintArray { + lint_array!(TYPE_COMPLEXITY) + } + + fn check_fn(&mut self, cx: &Context, _: FnKind, decl: &FnDecl, _: &Block, _: Span, _: NodeId) { + check_fndecl(cx, decl); + } + + fn check_struct_field(&mut self, cx: &Context, field: &StructField) { + check_type(cx, &field.node.ty); + } + + fn check_variant(&mut self, cx: &Context, var: &Variant, _: &Generics) { + // StructVariant is covered by check_struct_field + if let TupleVariantKind(ref args) = var.node.kind { + for arg in args { + check_type(cx, &arg.ty); + } + } + } + + fn check_item(&mut self, cx: &Context, item: &Item) { + match item.node { + ItemStatic(ref ty, _, _) | + ItemConst(ref ty, _) => check_type(cx, ty), + // functions, enums, structs, impls and traits are covered + _ => () + } + } + + fn check_trait_item(&mut self, cx: &Context, item: &TraitItem) { + match item.node { + ConstTraitItem(ref ty, _) | + TypeTraitItem(_, Some(ref ty)) => check_type(cx, ty), + MethodTraitItem(MethodSig { ref decl, .. }, None) => check_fndecl(cx, decl), + // methods with default impl are covered by check_fn + _ => () + } + } + + fn check_impl_item(&mut self, cx: &Context, item: &ImplItem) { + match item.node { + ConstImplItem(ref ty, _) | + TypeImplItem(ref ty) => check_type(cx, ty), + // methods are covered by check_fn + _ => () + } + } + + fn check_local(&mut self, cx: &Context, local: &Local) { + if let Some(ref ty) = local.ty { + check_type(cx, ty); + } + } +} + +fn check_fndecl(cx: &Context, decl: &FnDecl) { + for arg in &decl.inputs { + check_type(cx, &arg.ty); + } + if let Return(ref ty) = decl.output { + check_type(cx, ty); + } +} + +fn check_type(cx: &Context, ty: &ast::Ty) { + if in_external_macro(cx, ty.span) { return; } + let score = { + let mut visitor = TypeComplexityVisitor { score: 0, nest: 1 }; + visitor.visit_ty(ty); + visitor.score + }; + // println!("{:?} --> {}", ty, score); + if score > 250 { + span_lint(cx, TYPE_COMPLEXITY, ty.span, &format!( + "very complex type used. Consider factoring parts into `type` definitions")); + } +} + +/// Walks a type and assigns a complexity score to it. +struct TypeComplexityVisitor { + /// total complexity score of the type + score: u32, + /// current nesting level + nest: u32, +} + +impl<'v> Visitor<'v> for TypeComplexityVisitor { + fn visit_ty(&mut self, ty: &'v ast::Ty) { + let (add_score, sub_nest) = match ty.node { + // _, &x and *x have only small overhead; don't mess with nesting level + TyInfer | + TyPtr(..) | + TyRptr(..) => (1, 0), + + // the "normal" components of a type: named types, arrays/tuples + TyPath(..) | + TyVec(..) | + TyTup(..) | + TyFixedLengthVec(..) => (10 * self.nest, 1), + + // "Sum" of trait bounds + TyObjectSum(..) => (20 * self.nest, 0), + + // function types and "for<...>" bring a lot of overhead + TyBareFn(..) | + TyPolyTraitRef(..) => (50 * self.nest, 1), + + _ => (0, 0) + }; + self.score += add_score; + self.nest += sub_nest; + walk_ty(self, ty); + self.nest -= sub_nest; + } +} diff --git a/src/unicode.rs b/src/unicode.rs index ab48fd1bef2..8a64f612666 100644 --- a/src/unicode.rs +++ b/src/unicode.rs @@ -40,6 +40,7 @@ fn check_str(cx: &Context, string: &str, span: Span) { } } +#[allow(cast_possible_truncation)] fn str_pos_lint(cx: &Context, lint: &'static Lint, span: Span, index: usize, msg: &str) { span_lint(cx, lint, Span { lo: span.lo + BytePos((1 + index) as u32), hi: span.lo + BytePos((1 + index) as u32), diff --git a/src/utils.rs b/src/utils.rs index 47e3a3456d6..394204bedfc 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,11 +1,17 @@ use rustc::lint::*; use syntax::ast::*; use syntax::codemap::{ExpnInfo, Span}; -use syntax::ptr::P; use rustc::ast_map::Node::NodeExpr; use rustc::middle::ty; use std::borrow::Cow; +// module DefPaths for certain structs/enums we check for +pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"]; +pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"]; +pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"]; +pub const VEC_PATH: [&'static str; 3] = ["collections", "vec", "Vec"]; +pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedList"]; + /// returns true if the macro that expanded the crate was outside of /// the current crate or was a compiler plugin pub fn in_macro(cx: &Context, opt_info: Option<&ExpnInfo>) -> bool { @@ -37,6 +43,31 @@ pub fn match_def_path(cx: &Context, def_id: DefId, path: &[&str]) -> bool { .zip(path.iter()).all(|(nm, p)| nm == p)) } +/// check if type is struct or enum type with given def path +pub fn match_type(cx: &Context, ty: ty::Ty, path: &[&str]) -> bool { + match ty.sty { + ty::TyEnum(ref adt, _) | ty::TyStruct(ref adt, _) => { + match_def_path(cx, adt.did, path) + } + _ => { + false + } + } +} + +/// check if method call given in "expr" belongs to given trait +pub fn match_trait_method(cx: &Context, expr: &Expr, path: &[&str]) -> bool { + let method_call = ty::MethodCall::expr(expr.id); + let trt_id = cx.tcx.tables + .borrow().method_map.get(&method_call) + .and_then(|callee| cx.tcx.trait_of_item(callee.def_id)); + if let Some(trt_id) = trt_id { + match_def_path(cx, trt_id, path) + } else { + false + } +} + /// match a Path against a slice of segment string literals, e.g. /// `match_path(path, &["std", "rt", "begin_unwind"])` pub fn match_path(path: &Path, segments: &[&str]) -> bool { @@ -98,9 +129,6 @@ pub fn get_parent_expr<'c>(cx: &'c Context, e: &Expr) -> Option<&'c Expr> { if let NodeExpr(parent) = node { Some(parent) } else { None } ) } -/// dereference a P and return a ref on the result -pub fn de_p(p: &P) -> &T { &*p } - #[cfg(not(feature="structured_logging"))] pub fn span_lint(cx: &Context, lint: &'static Lint, sp: Span, msg: &str) { cx.span_lint(lint, sp, msg); diff --git a/tests/compile-fail/cast.rs b/tests/compile-fail/cast.rs new file mode 100644 index 00000000000..b17f5de841b --- /dev/null +++ b/tests/compile-fail/cast.rs @@ -0,0 +1,63 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[deny(cast_precision_loss, cast_possible_truncation, cast_sign_loss, cast_possible_wrap)] +fn main() { + // Test cast_precision_loss + 1i32 as f32; //~ERROR casting i32 to f32 causes a loss of precision (i32 is 32 bits wide, but f32's mantissa is only 23 bits wide) + 1i64 as f32; //~ERROR casting i64 to f32 causes a loss of precision (i64 is 64 bits wide, but f32's mantissa is only 23 bits wide) + 1i64 as f64; //~ERROR casting i64 to f64 causes a loss of precision (i64 is 64 bits wide, but f64's mantissa is only 52 bits wide) + 1u32 as f32; //~ERROR casting u32 to f32 causes a loss of precision (u32 is 32 bits wide, but f32's mantissa is only 23 bits wide) + 1u64 as f32; //~ERROR casting u64 to f32 causes a loss of precision (u64 is 64 bits wide, but f32's mantissa is only 23 bits wide) + 1u64 as f64; //~ERROR casting u64 to f64 causes a loss of precision (u64 is 64 bits wide, but f64's mantissa is only 52 bits wide) + 1i32 as f64; // Should not trigger the lint + 1u32 as f64; // Should not trigger the lint + + // Test cast_possible_truncation + 1f32 as i32; //~ERROR casting f32 to i32 may truncate the value + 1f32 as u32; //~ERROR casting f32 to u32 may truncate the value + //~^ERROR casting f32 to u32 may lose the sign of the value + 1f64 as f32; //~ERROR casting f64 to f32 may truncate the value + 1i32 as i8; //~ERROR casting i32 to i8 may truncate the value + 1i32 as u8; //~ERROR casting i32 to u8 may truncate the value + //~^ERROR casting i32 to u8 may lose the sign of the value + 1f64 as isize; //~ERROR casting f64 to isize may truncate the value + 1f64 as usize; //~ERROR casting f64 to usize may truncate the value + //~^ERROR casting f64 to usize may lose the sign of the value + + // Test cast_possible_wrap + 1u8 as i8; //~ERROR casting u8 to i8 may wrap around the value + 1u16 as i16; //~ERROR casting u16 to i16 may wrap around the value + 1u32 as i32; //~ERROR casting u32 to i32 may wrap around the value + 1u64 as i64; //~ERROR casting u64 to i64 may wrap around the value + 1usize as isize; //~ERROR casting usize to isize may wrap around the value + + // Test cast_sign_loss + 1i32 as u32; //~ERROR casting i32 to u32 may lose the sign of the value + 1isize as usize; //~ERROR casting isize to usize may lose the sign of the value + + // Extra checks for *size + // Casting from *size + 1isize as i8; //~ERROR casting isize to i8 may truncate the value + 1isize as f64; //~ERROR casting isize to f64 causes a loss of precision on targets with 64-bit wide pointers (isize is 64 bits wide, but f64's mantissa is only 52 bits wide) + 1usize as f64; //~ERROR casting usize to f64 causes a loss of precision on targets with 64-bit wide pointers (usize is 64 bits wide, but f64's mantissa is only 52 bits wide) + 1isize as f32; //~ERROR casting isize to f32 causes a loss of precision (isize is 32 or 64 bits wide, but f32's mantissa is only 23 bits wide) + 1usize as f32; //~ERROR casting usize to f32 causes a loss of precision (usize is 32 or 64 bits wide, but f32's mantissa is only 23 bits wide) + 1isize as i32; //~ERROR casting isize to i32 may truncate the value on targets with 64-bit wide pointers + 1isize as u32; //~ERROR casting isize to u32 may lose the sign of the value + //~^ERROR casting isize to u32 may truncate the value on targets with 64-bit wide pointers + 1usize as u32; //~ERROR casting usize to u32 may truncate the value on targets with 64-bit wide pointers + 1usize as i32; //~ERROR casting usize to i32 may truncate the value on targets with 64-bit wide pointers + //~^ERROR casting usize to i32 may wrap around the value on targets with 32-bit wide pointers + // Casting to *size + 1i64 as isize; //~ERROR casting i64 to isize may truncate the value on targets with 32-bit wide pointers + 1i64 as usize; //~ERROR casting i64 to usize may truncate the value on targets with 32-bit wide pointers + //~^ERROR casting i64 to usize may lose the sign of the value + 1u64 as isize; //~ERROR casting u64 to isize may truncate the value on targets with 32-bit wide pointers + //~^ERROR casting u64 to isize may wrap around the value on targets with 64-bit wide pointers + 1u64 as usize; //~ERROR casting u64 to usize may truncate the value on targets with 32-bit wide pointers + 1u32 as isize; //~ERROR casting u32 to isize may wrap around the value on targets with 32-bit wide pointers + 1u32 as usize; // Should not trigger any lint + 1i32 as isize; // Neither should this + 1i32 as usize; //~ERROR casting i32 to usize may lose the sign of the value +} \ No newline at end of file diff --git a/tests/compile-fail/complex_types.rs b/tests/compile-fail/complex_types.rs new file mode 100755 index 00000000000..995132ba88c --- /dev/null +++ b/tests/compile-fail/complex_types.rs @@ -0,0 +1,44 @@ +#![feature(plugin)] +#![plugin(clippy)] +#![deny(clippy)] +#![allow(unused)] +#![feature(associated_consts, associated_type_defaults)] + +type Alias = Vec>>; // no warning here + +const CST: (u32, (u32, (u32, (u32, u32)))) = (0, (0, (0, (0, 0)))); //~ERROR very complex type +static ST: (u32, (u32, (u32, (u32, u32)))) = (0, (0, (0, (0, 0)))); //~ERROR very complex type + +struct S { + f: Vec>>, //~ERROR very complex type +} + +struct TS(Vec>>); //~ERROR very complex type + +enum E { + V1(Vec>>), //~ERROR very complex type + V2 { f: Vec>> }, //~ERROR very complex type +} + +impl S { + const A: (u32, (u32, (u32, (u32, u32)))) = (0, (0, (0, (0, 0)))); //~ERROR very complex type + fn impl_method(&self, p: Vec>>) { } //~ERROR very complex type +} + +trait T { + const A: Vec>>; //~ERROR very complex type + type B = Vec>>; //~ERROR very complex type + fn method(&self, p: Vec>>); //~ERROR very complex type + fn def_method(&self, p: Vec>>) { } //~ERROR very complex type +} + +fn test1() -> Vec>> { vec![] } //~ERROR very complex type + +fn test2(_x: Vec>>) { } //~ERROR very complex type + +fn test3() { + let _y: Vec>> = vec![]; //~ERROR very complex type +} + +fn main() { +} diff --git a/tests/compile-fail/dlist.rs b/tests/compile-fail/dlist.rs old mode 100644 new mode 100755 index a2343c339ad..a800c045a50 --- a/tests/compile-fail/dlist.rs +++ b/tests/compile-fail/dlist.rs @@ -12,4 +12,4 @@ pub fn test(foo: LinkedList) { //~ ERROR I see you're using a LinkedList! fn main(){ test(LinkedList::new()); -} \ No newline at end of file +} diff --git a/tests/compile-fail/eq_op.rs b/tests/compile-fail/eq_op.rs index 8f61a11aa08..a1183629344 100755 --- a/tests/compile-fail/eq_op.rs +++ b/tests/compile-fail/eq_op.rs @@ -34,4 +34,8 @@ fn main() { ((1, 2) != (1, 2)); //~ERROR equal expressions [1].len() == [1].len(); //~ERROR equal expressions vec![1, 2, 3] == vec![1, 2, 3]; //no error yet, as we don't match macros + + // const folding + 1 + 1 == 2; //~ERROR equal expressions + 1 - 1 == 0; //~ERROR equal expressions } diff --git a/tests/compile-fail/match_if_let.rs b/tests/compile-fail/matches.rs similarity index 50% rename from tests/compile-fail/match_if_let.rs rename to tests/compile-fail/matches.rs index bf2e7e43a52..3cc540992c9 100755 --- a/tests/compile-fail/match_if_let.rs +++ b/tests/compile-fail/matches.rs @@ -2,8 +2,9 @@ #![plugin(clippy)] #![deny(clippy)] +#![allow(unused)] -fn main(){ +fn single_match(){ let x = Some(1u8); match x { //~ ERROR you seem to be trying to use match //~^ HELP try @@ -36,3 +37,29 @@ fn main(){ _ => println!("nope"), } } + +fn ref_pats() { + let ref v = Some(0); + match v { //~ERROR instead of prefixing all patterns with `&` + &Some(v) => println!("{:?}", v), + &None => println!("none"), + } + match v { // this doesn't trigger, we have a different pattern + &Some(v) => println!("some"), + other => println!("other"), + } + let ref tup = (1, 2); + match tup { //~ERROR instead of prefixing all patterns with `&` + &(v, 1) => println!("{}", v), + _ => println!("none"), + } + // special case: using & both in expr and pats + let w = Some(0); + match &w { //~ERROR you don't need to add `&` to both + &Some(v) => println!("{:?}", v), + &None => println!("none"), + } +} + +fn main() { +} diff --git a/tests/compile-fail/ptr_arg.rs b/tests/compile-fail/ptr_arg.rs index d4b6d22608f..d0615be492b 100755 --- a/tests/compile-fail/ptr_arg.rs +++ b/tests/compile-fail/ptr_arg.rs @@ -1,20 +1,23 @@ #![feature(plugin)] #![plugin(clippy)] +#![allow(unused)] +#![deny(ptr_arg)] -#[deny(ptr_arg)] -#[allow(unused)] fn do_vec(x: &Vec) { //~ERROR writing `&Vec<_>` instead of `&[_]` //Nothing here } -#[deny(ptr_arg)] -#[allow(unused)] +fn do_vec_mut(x: &mut Vec) { // no error here + //Nothing here +} + fn do_str(x: &String) { //~ERROR writing `&String` instead of `&str` //Nothing here either } -fn main() { - let x = vec![1i64, 2, 3]; - do_vec(&x); - do_str(&"hello".to_owned()); +fn do_str_mut(x: &mut String) { // no error here + //Nothing here either +} + +fn main() { } diff --git a/util/update_lints.py b/util/update_lints.py index ed26637059f..940899d4ebb 100755 --- a/util/update_lints.py +++ b/util/update_lints.py @@ -113,6 +113,11 @@ def main(print_only=False, check=False): lambda: gen_table(lints), write_back=not check) + changed |= replace_region('README.md', + r'^There are \d+ lints included in this crate:', "", + lambda: ['There are %d lints included in this crate:\n' % len(lints)], + write_back=not check) + # same for "clippy" lint collection changed |= replace_region('src/lib.rs', r'reg.register_lint_group\("clippy"', r'\]\);', lambda: gen_group(lints), replace_start=False,