From 993239d33af2b91fcd5e6dbec30f3810c8178ae3 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 00:04:01 +0200 Subject: [PATCH 01/33] Initial implementation of lossy cast lints. Introduces 3 lints : cast_possible_overflow cast_precision_loss cast_sign_loss Add a compile-test test case. Fix errors spotted by dogfood script. --- README.md | 85 ++++++++++++----------- src/consts.rs | 19 +++--- src/lib.rs | 4 ++ src/types.rs | 135 ++++++++++++++++++++++++++++++++++++- tests/compile-fail/cast.rs | 31 +++++++++ 5 files changed, 223 insertions(+), 51 deletions(-) create mode 100644 tests/compile-fail/cast.rs diff --git a/README.md b/README.md index e6a4d1be514..df097ca9e3f 100644 --- a/README.md +++ b/README.md @@ -6,47 +6,50 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints 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 +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_overflow | allow | casts that may cause overflow +cast_precision_loss | allow | casts that cause loss of precision +cast_sign_loss | allow | casts from signed types to unsigned types +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: diff --git a/src/consts.rs b/src/consts.rs index 5056cc27a54..c033888e360 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(unknown_lints,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 { diff --git a/src/lib.rs b/src/lib.rs index 50ebbd6d9fd..1b4d77dacca 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -68,6 +68,7 @@ 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_group("clippy", vec![ approx_const::APPROX_CONSTANT, @@ -104,6 +105,9 @@ pub fn plugin_registrar(reg: &mut Registry) { strings::STRING_ADD, strings::STRING_ADD_ASSIGN, types::BOX_VEC, + types::CAST_POSSIBLE_OVERFLOW, + types::CAST_PRECISION_LOSS, + types::CAST_SIGN_LOSS, types::LET_UNIT_VALUE, types::LINKEDLIST, types::UNIT_CMP, diff --git a/src/types.rs b/src/types.rs index 617c51fd961..17ebb791c3c 100644 --- a/src/types.rs +++ b/src/types.rs @@ -6,7 +6,7 @@ use syntax::ptr::P; use rustc::middle::ty; use syntax::codemap::ExpnInfo; -use utils::{in_macro, snippet, span_lint, span_help_and_lint}; +use utils::{in_macro, snippet, span_lint, span_help_and_lint, in_external_macro}; /// Handles all the linting of funky types #[allow(missing_copy_implementations)] @@ -136,3 +136,136 @@ impl LintPass for UnitCmp { } } } + +pub struct CastPass; + +declare_lint!(pub CAST_PRECISION_LOSS, Allow, + "casts that cause loss of precision"); +declare_lint!(pub CAST_SIGN_LOSS, Allow, + "casts from signed types to unsigned types"); +declare_lint!(pub CAST_POSSIBLE_OVERFLOW, Allow, + "casts that may cause overflow"); + +impl LintPass for CastPass { + fn get_lints(&self) -> LintArray { + lint_array!(CAST_PRECISION_LOSS, + CAST_SIGN_LOSS, + CAST_POSSIBLE_OVERFLOW) + } + + 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() && !in_external_macro(cx, expr.span) { + match (cast_from.is_integral(), cast_to.is_integral()) { + (true, false) => { + match (&cast_from.sty, &cast_to.sty) { + (&ty::TypeVariants::TyInt(i), &ty::TypeVariants::TyFloat(f)) => { + match (i, f) { + (ast::IntTy::TyI32, ast::FloatTy::TyF32) | + (ast::IntTy::TyI64, ast::FloatTy::TyF32) | + (ast::IntTy::TyI64, ast::FloatTy::TyF64) => { + span_lint(cx, CAST_PRECISION_LOSS, expr.span, + &format!("converting from {} to {}, which causes a loss of precision", + i, f)); + }, + _ => () + } + } + (&ty::TypeVariants::TyUint(u), &ty::TypeVariants::TyFloat(f)) => { + match (u, f) { + (ast::UintTy::TyU32, ast::FloatTy::TyF32) | + (ast::UintTy::TyU64, ast::FloatTy::TyF32) | + (ast::UintTy::TyU64, ast::FloatTy::TyF64) => { + span_lint(cx, CAST_PRECISION_LOSS, expr.span, + &format!("converting from {} to {}, which causes a loss of precision", + u, f)); + }, + _ => () + } + }, + _ => () + } + }, + (false, true) => { + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + &format!("the contents of a {} may overflow a {}", cast_from, cast_to)); + if !cx.tcx.expr_ty(expr).is_signed() { + span_lint(cx, CAST_SIGN_LOSS, expr.span, + &format!("casting from {} to {} loses the sign of the value", cast_from, cast_to)); + } + }, + (true, true) => { + match (&cast_from.sty, &cast_to.sty) { + (&ty::TypeVariants::TyInt(i1), &ty::TypeVariants::TyInt(i2)) => { + match (i1, i2) { + (ast::IntTy::TyI64, ast::IntTy::TyI32) | + (ast::IntTy::TyI64, ast::IntTy::TyI16) | + (ast::IntTy::TyI64, ast::IntTy::TyI8) | + (ast::IntTy::TyI32, ast::IntTy::TyI16) | + (ast::IntTy::TyI32, ast::IntTy::TyI8) | + (ast::IntTy::TyI16, ast::IntTy::TyI8) => + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + &format!("the contents of a {} may overflow a {}", i1, i2)), + _ => () + } + }, + (&ty::TypeVariants::TyInt(i), &ty::TypeVariants::TyUint(u)) => { + span_lint(cx, CAST_SIGN_LOSS, expr.span, + &format!("casting from {} to {} loses the sign of the value", i, u)); + match (i, u) { + (ast::IntTy::TyI64, ast::UintTy::TyU32) | + (ast::IntTy::TyI64, ast::UintTy::TyU16) | + (ast::IntTy::TyI64, ast::UintTy::TyU8) | + (ast::IntTy::TyI32, ast::UintTy::TyU16) | + (ast::IntTy::TyI32, ast::UintTy::TyU8) | + (ast::IntTy::TyI16, ast::UintTy::TyU8) => + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + &format!("the contents of a {} may overflow a {}", i, u)), + _ => () + } + }, + (&ty::TypeVariants::TyUint(u), &ty::TypeVariants::TyInt(i)) => { + match (u, i) { + (ast::UintTy::TyU64, ast::IntTy::TyI32) | + (ast::UintTy::TyU64, ast::IntTy::TyI64) | + (ast::UintTy::TyU64, ast::IntTy::TyI16) | + (ast::UintTy::TyU64, ast::IntTy::TyI8) | + (ast::UintTy::TyU32, ast::IntTy::TyI32) | + (ast::UintTy::TyU32, ast::IntTy::TyI16) | + (ast::UintTy::TyU32, ast::IntTy::TyI8) | + (ast::UintTy::TyU16, ast::IntTy::TyI16) | + (ast::UintTy::TyU16, ast::IntTy::TyI8) | + (ast::UintTy::TyU8, ast::IntTy::TyI8) => + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + &format!("the contents of a {} may overflow a {}", u, i)), + _ => () + } + }, + (&ty::TypeVariants::TyUint(u1), &ty::TypeVariants::TyUint(u2)) => { + match (u1, u2) { + (ast::UintTy::TyU64, ast::UintTy::TyU32) | + (ast::UintTy::TyU64, ast::UintTy::TyU16) | + (ast::UintTy::TyU64, ast::UintTy::TyU8) | + (ast::UintTy::TyU32, ast::UintTy::TyU16) | + (ast::UintTy::TyU32, ast::UintTy::TyU8) | + (ast::UintTy::TyU16, ast::UintTy::TyU8) => + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + &format!("the contents of a {} may overflow a {}", u1, u2)), + _ => () + } + }, + _ => () + } + } + (false, false) => { + if let (&ty::TypeVariants::TyFloat(ast::FloatTy::TyF64), + &ty::TypeVariants::TyFloat(ast::FloatTy::TyF32)) = (&cast_from.sty, &cast_to.sty) { + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, "the contents of a f64 may overflow a f32"); + } + } + } + } + } + } +} diff --git a/tests/compile-fail/cast.rs b/tests/compile-fail/cast.rs new file mode 100644 index 00000000000..a51ea62a7b8 --- /dev/null +++ b/tests/compile-fail/cast.rs @@ -0,0 +1,31 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[deny(cast_precision_loss, cast_possible_overflow, cast_sign_loss)] +fn main() { + let i : i32 = 42; + let u : u32 = 42; + let f : f32 = 42.0; + + // Test cast_precision_loss + i as f32; //~ERROR converting from i32 to f32, which causes a loss of precision + (i as i64) as f32; //~ERROR converting from i64 to f32, which causes a loss of precision + (i as i64) as f64; //~ERROR converting from i64 to f64, which causes a loss of precision + u as f32; //~ERROR converting from u32 to f32, which causes a loss of precision + (u as u64) as f32; //~ERROR converting from u64 to f32, which causes a loss of precision + (u as u64) as f64; //~ERROR converting from u64 to f64, which causes a loss of precision + i as f64; // Should not trigger the lint + u as f64; // Should not trigger the lint + + // Test cast_possible_overflow + f as i32; //~ERROR the contents of a f32 may overflow a i32 + f as u32; //~ERROR the contents of a f32 may overflow a u32 + //~^ERROR casting from f32 to u32 loses the sign of the value + i as u8; //~ERROR the contents of a i32 may overflow a u8 + //~^ERROR casting from i32 to u8 loses the sign of the value + (f as f64) as f32; //~ERROR the contents of a f64 may overflow a f32 + i as i8; //~ERROR the contents of a i32 may overflow a i8 + + // Test cast_sign_loss + i as u32; //~ERROR casting from i32 to u32 loses the sign of the value +} \ No newline at end of file From 1846581baace12af85dfda3fffc219b9ceadc7e8 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 14:24:26 +0200 Subject: [PATCH 02/33] Added examples to lint descriptions. --- README.md | 6 +++--- src/types.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index df097ca9e3f..6a264034773 100644 --- a/README.md +++ b/README.md @@ -11,9 +11,9 @@ 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_overflow | allow | casts that may cause overflow -cast_precision_loss | allow | casts that cause loss of precision -cast_sign_loss | allow | casts from signed types to unsigned types +cast_possible_overflow | allow | casts that may cause overflow, e.g `x as u8` where `x: u32`, or `x as i32` where `x: f32` +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() }` diff --git a/src/types.rs b/src/types.rs index 17ebb791c3c..d6081153f01 100644 --- a/src/types.rs +++ b/src/types.rs @@ -140,11 +140,11 @@ impl LintPass for UnitCmp { pub struct CastPass; declare_lint!(pub CAST_PRECISION_LOSS, Allow, - "casts that cause loss of precision"); + "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"); + "casts from signed types to unsigned types, e.g `x as u32` where `x: i32`"); declare_lint!(pub CAST_POSSIBLE_OVERFLOW, Allow, - "casts that may cause overflow"); + "casts that may cause overflow, e.g `x as u8` where `x: u32`, or `x as i32` where `x: f32`"); impl LintPass for CastPass { fn get_lints(&self) -> LintArray { From 93d9249f769f27e2864d5b2d53dfc94a933412a4 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 14:25:08 +0200 Subject: [PATCH 03/33] Moved allow(unknown_lints) to crate level. --- src/consts.rs | 2 +- src/lib.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/consts.rs b/src/consts.rs index c033888e360..70d5ff4bc17 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -67,7 +67,7 @@ impl Constant { } /// convert this constant to a f64, if possible - #[allow(unknown_lints,cast_precision_loss)] + #[allow(cast_precision_loss)] pub fn as_float(&self) -> Option { match *self { ConstantByte(b) => Some(b as f64), diff --git a/src/lib.rs b/src/lib.rs index 1b4d77dacca..f4e3ed54c60 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)] +#![allow(unknown_lints)] #[macro_use] extern crate syntax; From b417f01ed82332cd81954ac6d9f5bad615db2bfc Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 14:36:26 +0200 Subject: [PATCH 04/33] Also test that the CastExpr's right arm is numeric. --- src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types.rs b/src/types.rs index d6081153f01..3f7012f6ca8 100644 --- a/src/types.rs +++ b/src/types.rs @@ -156,7 +156,7 @@ impl LintPass for CastPass { 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() && !in_external_macro(cx, expr.span) { + 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) => { match (&cast_from.sty, &cast_to.sty) { From 14528d433af176f36aab475318691676c3de4464 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 14:37:35 +0200 Subject: [PATCH 05/33] Simplified reexported ast::* type paths. Also removed trailing whitespaces. --- src/types.rs | 100 +++++++++++++++++++++++++-------------------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/src/types.rs b/src/types.rs index 3f7012f6ca8..2c4d81e361d 100644 --- a/src/types.rs +++ b/src/types.rs @@ -160,11 +160,11 @@ impl LintPass for CastPass { match (cast_from.is_integral(), cast_to.is_integral()) { (true, false) => { match (&cast_from.sty, &cast_to.sty) { - (&ty::TypeVariants::TyInt(i), &ty::TypeVariants::TyFloat(f)) => { + (&ty::TyInt(i), &ty::TyFloat(f)) => { match (i, f) { - (ast::IntTy::TyI32, ast::FloatTy::TyF32) | - (ast::IntTy::TyI64, ast::FloatTy::TyF32) | - (ast::IntTy::TyI64, ast::FloatTy::TyF64) => { + (ast::TyI32, ast::TyF32) | + (ast::TyI64, ast::TyF32) | + (ast::TyI64, ast::TyF64) => { span_lint(cx, CAST_PRECISION_LOSS, expr.span, &format!("converting from {} to {}, which causes a loss of precision", i, f)); @@ -172,11 +172,11 @@ impl LintPass for CastPass { _ => () } } - (&ty::TypeVariants::TyUint(u), &ty::TypeVariants::TyFloat(f)) => { + (&ty::TyUint(u), &ty::TyFloat(f)) => { match (u, f) { - (ast::UintTy::TyU32, ast::FloatTy::TyF32) | - (ast::UintTy::TyU64, ast::FloatTy::TyF32) | - (ast::UintTy::TyU64, ast::FloatTy::TyF64) => { + (ast::TyU32, ast::TyF32) | + (ast::TyU64, ast::TyF32) | + (ast::TyU64, ast::TyF64) => { span_lint(cx, CAST_PRECISION_LOSS, expr.span, &format!("converting from {} to {}, which causes a loss of precision", u, f)); @@ -188,69 +188,69 @@ impl LintPass for CastPass { } }, (false, true) => { - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, &format!("the contents of a {} may overflow a {}", cast_from, cast_to)); if !cx.tcx.expr_ty(expr).is_signed() { - span_lint(cx, CAST_SIGN_LOSS, expr.span, + span_lint(cx, CAST_SIGN_LOSS, expr.span, &format!("casting from {} to {} loses the sign of the value", cast_from, cast_to)); } }, (true, true) => { match (&cast_from.sty, &cast_to.sty) { - (&ty::TypeVariants::TyInt(i1), &ty::TypeVariants::TyInt(i2)) => { + (&ty::TyInt(i1), &ty::TyInt(i2)) => { match (i1, i2) { - (ast::IntTy::TyI64, ast::IntTy::TyI32) | - (ast::IntTy::TyI64, ast::IntTy::TyI16) | - (ast::IntTy::TyI64, ast::IntTy::TyI8) | - (ast::IntTy::TyI32, ast::IntTy::TyI16) | - (ast::IntTy::TyI32, ast::IntTy::TyI8) | - (ast::IntTy::TyI16, ast::IntTy::TyI8) => - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + (ast::TyI64, ast::TyI32) | + (ast::TyI64, ast::TyI16) | + (ast::TyI64, ast::TyI8) | + (ast::TyI32, ast::TyI16) | + (ast::TyI32, ast::TyI8) | + (ast::TyI16, ast::TyI8) => + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, &format!("the contents of a {} may overflow a {}", i1, i2)), _ => () } }, - (&ty::TypeVariants::TyInt(i), &ty::TypeVariants::TyUint(u)) => { - span_lint(cx, CAST_SIGN_LOSS, expr.span, + (&ty::TyInt(i), &ty::TyUint(u)) => { + span_lint(cx, CAST_SIGN_LOSS, expr.span, &format!("casting from {} to {} loses the sign of the value", i, u)); match (i, u) { - (ast::IntTy::TyI64, ast::UintTy::TyU32) | - (ast::IntTy::TyI64, ast::UintTy::TyU16) | - (ast::IntTy::TyI64, ast::UintTy::TyU8) | - (ast::IntTy::TyI32, ast::UintTy::TyU16) | - (ast::IntTy::TyI32, ast::UintTy::TyU8) | - (ast::IntTy::TyI16, ast::UintTy::TyU8) => - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + (ast::TyI64, ast::TyU32) | + (ast::TyI64, ast::TyU16) | + (ast::TyI64, ast::TyU8) | + (ast::TyI32, ast::TyU16) | + (ast::TyI32, ast::TyU8) | + (ast::TyI16, ast::TyU8) => + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, &format!("the contents of a {} may overflow a {}", i, u)), _ => () } }, - (&ty::TypeVariants::TyUint(u), &ty::TypeVariants::TyInt(i)) => { + (&ty::TyUint(u), &ty::TyInt(i)) => { match (u, i) { - (ast::UintTy::TyU64, ast::IntTy::TyI32) | - (ast::UintTy::TyU64, ast::IntTy::TyI64) | - (ast::UintTy::TyU64, ast::IntTy::TyI16) | - (ast::UintTy::TyU64, ast::IntTy::TyI8) | - (ast::UintTy::TyU32, ast::IntTy::TyI32) | - (ast::UintTy::TyU32, ast::IntTy::TyI16) | - (ast::UintTy::TyU32, ast::IntTy::TyI8) | - (ast::UintTy::TyU16, ast::IntTy::TyI16) | - (ast::UintTy::TyU16, ast::IntTy::TyI8) | - (ast::UintTy::TyU8, ast::IntTy::TyI8) => - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + (ast::TyU64, ast::TyI32) | + (ast::TyU64, ast::TyI64) | + (ast::TyU64, ast::TyI16) | + (ast::TyU64, ast::TyI8) | + (ast::TyU32, ast::TyI32) | + (ast::TyU32, ast::TyI16) | + (ast::TyU32, ast::TyI8) | + (ast::TyU16, ast::TyI16) | + (ast::TyU16, ast::TyI8) | + (ast::TyU8, ast::TyI8) => + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, &format!("the contents of a {} may overflow a {}", u, i)), _ => () } }, - (&ty::TypeVariants::TyUint(u1), &ty::TypeVariants::TyUint(u2)) => { + (&ty::TyUint(u1), &ty::TyUint(u2)) => { match (u1, u2) { - (ast::UintTy::TyU64, ast::UintTy::TyU32) | - (ast::UintTy::TyU64, ast::UintTy::TyU16) | - (ast::UintTy::TyU64, ast::UintTy::TyU8) | - (ast::UintTy::TyU32, ast::UintTy::TyU16) | - (ast::UintTy::TyU32, ast::UintTy::TyU8) | - (ast::UintTy::TyU16, ast::UintTy::TyU8) => - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + (ast::TyU64, ast::TyU32) | + (ast::TyU64, ast::TyU16) | + (ast::TyU64, ast::TyU8) | + (ast::TyU32, ast::TyU16) | + (ast::TyU32, ast::TyU8) | + (ast::TyU16, ast::TyU8) => + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, &format!("the contents of a {} may overflow a {}", u1, u2)), _ => () } @@ -259,13 +259,13 @@ impl LintPass for CastPass { } } (false, false) => { - if let (&ty::TypeVariants::TyFloat(ast::FloatTy::TyF64), - &ty::TypeVariants::TyFloat(ast::FloatTy::TyF32)) = (&cast_from.sty, &cast_to.sty) { + if let (&ty::TyFloat(ast::TyF64), + &ty::TyFloat(ast::TyF32)) = (&cast_from.sty, &cast_to.sty) { span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, "the contents of a f64 may overflow a f32"); } } } } } - } + } } From ff28dd324ec404285dc61c6a29ceadbf3985d7e0 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 14:50:26 +0200 Subject: [PATCH 06/33] Fixed a little oversight. --- src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types.rs b/src/types.rs index 2c4d81e361d..2950a03a036 100644 --- a/src/types.rs +++ b/src/types.rs @@ -190,7 +190,7 @@ impl LintPass for CastPass { (false, true) => { span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, &format!("the contents of a {} may overflow a {}", cast_from, cast_to)); - if !cx.tcx.expr_ty(expr).is_signed() { + if !cast_to.is_signed() { span_lint(cx, CAST_SIGN_LOSS, expr.span, &format!("casting from {} to {} loses the sign of the value", cast_from, cast_to)); } From ab481e5cb1cb54005a19044709ac9bebabd74aae Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 21:37:37 +0200 Subject: [PATCH 07/33] Refactored the CastPass lints. --- src/types.rs | 121 +++++++++++++++------------------------------------ 1 file changed, 35 insertions(+), 86 deletions(-) diff --git a/src/types.rs b/src/types.rs index 2950a03a036..ea0416e512c 100644 --- a/src/types.rs +++ b/src/types.rs @@ -158,33 +158,24 @@ impl LintPass for CastPass { 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) => { - match (&cast_from.sty, &cast_to.sty) { - (&ty::TyInt(i), &ty::TyFloat(f)) => { - match (i, f) { - (ast::TyI32, ast::TyF32) | - (ast::TyI64, ast::TyF32) | - (ast::TyI64, ast::TyF64) => { - span_lint(cx, CAST_PRECISION_LOSS, expr.span, - &format!("converting from {} to {}, which causes a loss of precision", - i, f)); - }, - _ => () - } + (true, false) => { + let from_nbits = match &cast_from.sty { + &ty::TyInt(i) => 4 << (i as usize), + &ty::TyUint(u) => 4 << (u as usize), + _ => 0 + }; + let to_nbits : usize = match &cast_to.sty { + &ty::TyFloat(ast::TyF32) => 32, + &ty::TyFloat(ast::TyF64) => 64, + _ => 0 + }; + if from_nbits != 4 { + // Handle TyIs/TyUs separately (size is arch dependant) + if from_nbits >= to_nbits { + span_lint(cx, CAST_PRECISION_LOSS, expr.span, + &format!("converting from {} to {}, which causes a loss of precision", + cast_from, cast_to)); } - (&ty::TyUint(u), &ty::TyFloat(f)) => { - match (u, f) { - (ast::TyU32, ast::TyF32) | - (ast::TyU64, ast::TyF32) | - (ast::TyU64, ast::TyF64) => { - span_lint(cx, CAST_PRECISION_LOSS, expr.span, - &format!("converting from {} to {}, which causes a loss of precision", - u, f)); - }, - _ => () - } - }, - _ => () } }, (false, true) => { @@ -196,66 +187,24 @@ impl LintPass for CastPass { } }, (true, true) => { - match (&cast_from.sty, &cast_to.sty) { - (&ty::TyInt(i1), &ty::TyInt(i2)) => { - match (i1, i2) { - (ast::TyI64, ast::TyI32) | - (ast::TyI64, ast::TyI16) | - (ast::TyI64, ast::TyI8) | - (ast::TyI32, ast::TyI16) | - (ast::TyI32, ast::TyI8) | - (ast::TyI16, ast::TyI8) => - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, - &format!("the contents of a {} may overflow a {}", i1, i2)), - _ => () - } - }, - (&ty::TyInt(i), &ty::TyUint(u)) => { - span_lint(cx, CAST_SIGN_LOSS, expr.span, - &format!("casting from {} to {} loses the sign of the value", i, u)); - match (i, u) { - (ast::TyI64, ast::TyU32) | - (ast::TyI64, ast::TyU16) | - (ast::TyI64, ast::TyU8) | - (ast::TyI32, ast::TyU16) | - (ast::TyI32, ast::TyU8) | - (ast::TyI16, ast::TyU8) => - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, - &format!("the contents of a {} may overflow a {}", i, u)), - _ => () - } - }, - (&ty::TyUint(u), &ty::TyInt(i)) => { - match (u, i) { - (ast::TyU64, ast::TyI32) | - (ast::TyU64, ast::TyI64) | - (ast::TyU64, ast::TyI16) | - (ast::TyU64, ast::TyI8) | - (ast::TyU32, ast::TyI32) | - (ast::TyU32, ast::TyI16) | - (ast::TyU32, ast::TyI8) | - (ast::TyU16, ast::TyI16) | - (ast::TyU16, ast::TyI8) | - (ast::TyU8, ast::TyI8) => - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, - &format!("the contents of a {} may overflow a {}", u, i)), - _ => () - } - }, - (&ty::TyUint(u1), &ty::TyUint(u2)) => { - match (u1, u2) { - (ast::TyU64, ast::TyU32) | - (ast::TyU64, ast::TyU16) | - (ast::TyU64, ast::TyU8) | - (ast::TyU32, ast::TyU16) | - (ast::TyU32, ast::TyU8) | - (ast::TyU16, ast::TyU8) => - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, - &format!("the contents of a {} may overflow a {}", u1, u2)), - _ => () - } - }, - _ => () + if cast_from.is_signed() && !cast_to.is_signed() { + span_lint(cx, CAST_SIGN_LOSS, expr.span, + &format!("casting from {} to {} loses the sign of the value", cast_from, cast_to)); + } + let from_nbits = match &cast_from.sty { + &ty::TyInt(i) => 4 << (i as usize), + &ty::TyUint(u) => 4 << (u as usize), + _ => 0 + }; + let to_nbits = match &cast_to.sty { + &ty::TyInt(i) => 4 << (i as usize), + &ty::TyUint(u) => 4 << (u as usize), + _ => 0 + }; + if to_nbits < from_nbits || + (!cast_from.is_signed() && cast_to.is_signed() && to_nbits <= from_nbits) { + span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, + &format!("the contents of a {} may overflow a {}", cast_from, cast_to)); } } (false, false) => { From dbc9b7f46eb95d2583fc60c09220c0803ba541ee Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 22:44:40 +0200 Subject: [PATCH 08/33] Reworked the error messages for more heplfulness. Renamed the cast_possible_overflow lint to cast_possible_truncation, and updated the error message, readme and crate root accordingly. Added some more information to the message for the cast_precision_loss lint. Updated the test case to reflect changes. --- README.md | 88 +++++++++++++++++++------------------- src/lib.rs | 2 +- src/types.rs | 23 +++++----- tests/compile-fail/cast.rs | 26 +++++------ 4 files changed, 70 insertions(+), 69 deletions(-) diff --git a/README.md b/README.md index 6a264034773..fbb16fcb170 100644 --- a/README.md +++ b/README.md @@ -6,50 +6,50 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints 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 -cast_possible_overflow | allow | casts that may cause overflow, e.g `x as u8` where `x: u32`, or `x as i32` where `x: f32` -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 -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 +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_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 +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: diff --git a/src/lib.rs b/src/lib.rs index f4e3ed54c60..bdb3cb3471a 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -106,7 +106,7 @@ pub fn plugin_registrar(reg: &mut Registry) { strings::STRING_ADD, strings::STRING_ADD_ASSIGN, types::BOX_VEC, - types::CAST_POSSIBLE_OVERFLOW, + types::CAST_POSSIBLE_TRUNCATION, types::CAST_PRECISION_LOSS, types::CAST_SIGN_LOSS, types::LET_UNIT_VALUE, diff --git a/src/types.rs b/src/types.rs index ea0416e512c..f9949a7b563 100644 --- a/src/types.rs +++ b/src/types.rs @@ -143,14 +143,14 @@ 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_OVERFLOW, Allow, - "casts that may cause overflow, e.g `x as u8` where `x: u32`, or `x as i32` where `x: f32`"); +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`"); impl LintPass for CastPass { fn get_lints(&self) -> LintArray { lint_array!(CAST_PRECISION_LOSS, CAST_SIGN_LOSS, - CAST_POSSIBLE_OVERFLOW) + CAST_POSSIBLE_TRUNCATION) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -170,17 +170,18 @@ impl LintPass for CastPass { _ => 0 }; if from_nbits != 4 { - // Handle TyIs/TyUs separately (size is arch dependant) + // Handle TyIs/TyUs separately (pointer size is arch dependant) if from_nbits >= to_nbits { span_lint(cx, CAST_PRECISION_LOSS, expr.span, - &format!("converting from {} to {}, which causes a loss of precision", - cast_from, cast_to)); + &format!("converting from {0} to {1}, which causes a loss of precision \ + ({0} is {2} bits wide, but {1}'s mantissa is only {3} bits wide)", + cast_from, cast_to, from_nbits, if to_nbits == 64 {52} else {23} )); } } }, (false, true) => { - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, - &format!("the contents of a {} may overflow a {}", cast_from, cast_to)); + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may cause truncation of the value", cast_from, cast_to)); if !cast_to.is_signed() { span_lint(cx, CAST_SIGN_LOSS, expr.span, &format!("casting from {} to {} loses the sign of the value", cast_from, cast_to)); @@ -203,14 +204,14 @@ impl LintPass for CastPass { }; if to_nbits < from_nbits || (!cast_from.is_signed() && cast_to.is_signed() && to_nbits <= from_nbits) { - span_lint(cx, CAST_POSSIBLE_OVERFLOW, expr.span, - &format!("the contents of a {} may overflow a {}", cast_from, cast_to)); + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may cause truncation of the value", 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_OVERFLOW, expr.span, "the contents of a f64 may overflow a f32"); + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, "casting f64 to f32 may cause truncation of the value"); } } } diff --git a/tests/compile-fail/cast.rs b/tests/compile-fail/cast.rs index a51ea62a7b8..af6e6089fbf 100644 --- a/tests/compile-fail/cast.rs +++ b/tests/compile-fail/cast.rs @@ -1,30 +1,30 @@ #![feature(plugin)] #![plugin(clippy)] -#[deny(cast_precision_loss, cast_possible_overflow, cast_sign_loss)] +#[deny(cast_precision_loss, cast_possible_truncation, cast_sign_loss)] fn main() { let i : i32 = 42; let u : u32 = 42; let f : f32 = 42.0; // Test cast_precision_loss - i as f32; //~ERROR converting from i32 to f32, which causes a loss of precision - (i as i64) as f32; //~ERROR converting from i64 to f32, which causes a loss of precision - (i as i64) as f64; //~ERROR converting from i64 to f64, which causes a loss of precision - u as f32; //~ERROR converting from u32 to f32, which causes a loss of precision - (u as u64) as f32; //~ERROR converting from u64 to f32, which causes a loss of precision - (u as u64) as f64; //~ERROR converting from u64 to f64, which causes a loss of precision + i as f32; //~ERROR converting from i32 to f32, which causes a loss of precision (i32 is 32 bits wide, but f32's mantissa is only 23 bits wide) + (i as i64) as f32; //~ERROR converting from i64 to f32, which causes a loss of precision (i64 is 64 bits wide, but f32's mantissa is only 23 bits wide) + (i as i64) as f64; //~ERROR converting from i64 to f64, which causes a loss of precision (i64 is 64 bits wide, but f64's mantissa is only 52 bits wide) + u as f32; //~ERROR converting from u32 to f32, which causes a loss of precision (u32 is 32 bits wide, but f32's mantissa is only 23 bits wide) + (u as u64) as f32; //~ERROR converting from u64 to f32, which causes a loss of precision (u64 is 64 bits wide, but f32's mantissa is only 23 bits wide) + (u as u64) as f64; //~ERROR converting from u64 to f64, which causes a loss of precision (u64 is 64 bits wide, but f64's mantissa is only 52 bits wide) i as f64; // Should not trigger the lint u as f64; // Should not trigger the lint - // Test cast_possible_overflow - f as i32; //~ERROR the contents of a f32 may overflow a i32 - f as u32; //~ERROR the contents of a f32 may overflow a u32 + // Test cast_possible_truncation + f as i32; //~ERROR casting f32 to i32 may cause truncation of the value + f as u32; //~ERROR casting f32 to u32 may cause truncation of the value //~^ERROR casting from f32 to u32 loses the sign of the value - i as u8; //~ERROR the contents of a i32 may overflow a u8 + i as u8; //~ERROR casting i32 to u8 may cause truncation of the value //~^ERROR casting from i32 to u8 loses the sign of the value - (f as f64) as f32; //~ERROR the contents of a f64 may overflow a f32 - i as i8; //~ERROR the contents of a i32 may overflow a i8 + (f as f64) as f32; //~ERROR casting f64 to f32 may cause truncation of the value + i as i8; //~ERROR casting i32 to i8 may cause truncation of the value // Test cast_sign_loss i as u32; //~ERROR casting from i32 to u32 loses the sign of the value From ad0bc66402eb528aedb1d14235fed9a70bcc0a34 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Fri, 21 Aug 2015 03:03:37 +0200 Subject: [PATCH 09/33] Added support for isize/usize in the CastPass lint pass. Extracted the match that determines an integer types's size in a utility function and implemented support for usize/isize. Added a needed feature to the crate root. Added some tests to cover those cases, and a test I previously forgot. Silenced two errors signaled by dogfood.sh in unicode.rs. --- src/lib.rs | 2 +- src/types.rs | 33 ++++++++++++++++----------------- src/unicode.rs | 1 + tests/compile-fail/cast.rs | 30 ++++++++++++++++++++++++++++-- 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index bdb3cb3471a..9ea1efeed5f 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,6 @@ #![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] diff --git a/src/types.rs b/src/types.rs index f9949a7b563..915ffb15fe5 100644 --- a/src/types.rs +++ b/src/types.rs @@ -146,6 +146,18 @@ declare_lint!(pub CAST_SIGN_LOSS, Allow, 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`"); +/// 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 } +} + impl LintPass for CastPass { fn get_lints(&self) -> LintArray { lint_array!(CAST_PRECISION_LOSS, @@ -159,18 +171,13 @@ impl LintPass for CastPass { 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 = match &cast_from.sty { - &ty::TyInt(i) => 4 << (i as usize), - &ty::TyUint(u) => 4 << (u as usize), - _ => 0 - }; + let from_nbits = int_ty_to_nbits(cast_from); let to_nbits : usize = match &cast_to.sty { &ty::TyFloat(ast::TyF32) => 32, &ty::TyFloat(ast::TyF64) => 64, _ => 0 }; - if from_nbits != 4 { - // Handle TyIs/TyUs separately (pointer size is arch dependant) + if from_nbits != 0 { if from_nbits >= to_nbits { span_lint(cx, CAST_PRECISION_LOSS, expr.span, &format!("converting from {0} to {1}, which causes a loss of precision \ @@ -192,16 +199,8 @@ impl LintPass for CastPass { span_lint(cx, CAST_SIGN_LOSS, expr.span, &format!("casting from {} to {} loses the sign of the value", cast_from, cast_to)); } - let from_nbits = match &cast_from.sty { - &ty::TyInt(i) => 4 << (i as usize), - &ty::TyUint(u) => 4 << (u as usize), - _ => 0 - }; - let to_nbits = match &cast_to.sty { - &ty::TyInt(i) => 4 << (i as usize), - &ty::TyUint(u) => 4 << (u as usize), - _ => 0 - }; + let from_nbits = int_ty_to_nbits(cast_from); + let to_nbits = int_ty_to_nbits(cast_to); if to_nbits < from_nbits || (!cast_from.is_signed() && cast_to.is_signed() && to_nbits <= from_nbits) { span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, 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/tests/compile-fail/cast.rs b/tests/compile-fail/cast.rs index af6e6089fbf..0fa402b3bf7 100644 --- a/tests/compile-fail/cast.rs +++ b/tests/compile-fail/cast.rs @@ -2,6 +2,7 @@ #![plugin(clippy)] #[deny(cast_precision_loss, cast_possible_truncation, cast_sign_loss)] +#[allow(dead_code)] fn main() { let i : i32 = 42; let u : u32 = 42; @@ -16,7 +17,7 @@ fn main() { (u as u64) as f64; //~ERROR converting from u64 to f64, which causes a loss of precision (u64 is 64 bits wide, but f64's mantissa is only 52 bits wide) i as f64; // Should not trigger the lint u as f64; // Should not trigger the lint - + // Test cast_possible_truncation f as i32; //~ERROR casting f32 to i32 may cause truncation of the value f as u32; //~ERROR casting f32 to u32 may cause truncation of the value @@ -25,7 +26,32 @@ fn main() { //~^ERROR casting from i32 to u8 loses the sign of the value (f as f64) as f32; //~ERROR casting f64 to f32 may cause truncation of the value i as i8; //~ERROR casting i32 to i8 may cause truncation of the value - + u as i32; //~ERROR casting u32 to i32 may cause truncation of the value + // Test cast_sign_loss i as u32; //~ERROR casting from i32 to u32 loses the sign of the value + + // Extra checks for usize/isize + let is : isize = -42; + is as usize; //~ERROR casting from isize to usize loses the sign of the value + is as i8; //~ERROR casting isize to i8 may cause truncation of the value + + // FIXME : enable these checks when we figure out a way to make compiletest deal with conditional compilation + /* + #[cfg(target_pointer_width = "64")] + fn check_64() { + let is : isize = -42; + let us : usize = 42; + is as f32; //ERROR converting from isize to f32, which causes a loss of precision (isize is 64 bits wide, but f32's mantissa is only 23 bits wide) + us as u32; //ERROR casting usize to u32 may cause truncation of the value + us as u64; // Should not trigger any lint + } + #[cfg(target_pointer_width = "32")] + fn check_32() { + let is : isize = -42; + let us : usize = 42; + is as f32; //ERROR converting from isize to f32, which causes a loss of precision (isize is 32 bits wide, but f32's mantissa is only 23 bits wide) + us as u32; // Should not trigger any lint + us as u64; // Should not trigger any lint + }*/ } \ No newline at end of file From 4dcbad1b086368beb97dc9d20b154a634c5c84af Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 21 Aug 2015 12:19:07 +0200 Subject: [PATCH 10/33] const folding for eq_op --- src/eq_op.rs | 128 +++++++++++++++++++++++++------------------------ src/strings.rs | 13 ++--- 2 files changed, 73 insertions(+), 68 deletions(-) diff --git a/src/eq_op.rs b/src/eq_op.rs index 50b61e23356..6202dcc3670 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,48 @@ impl LintPass for EqOp { } } -pub fn is_exp_equal(left : &Expr, right : &Expr) -> bool { +pub fn is_exp_equal(cx: &Context, 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), + 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 + } || 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 +89,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 +140,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 +183,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 +245,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/strings.rs b/src/strings.rs index 7981b785850..b1da93d71e3 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -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") @@ -67,13 +67,14 @@ fn is_string(cx: &Context, e: &Expr) -> bool { } else { false } } -fn is_add(src: &Expr, target: &Expr) -> bool { +fn is_add(cx: &Context, src: &Expr, target: &Expr) -> bool { match &src.node { &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => - is_exp_equal(target, left), + is_exp_equal(cx, 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), + block.expr.as_ref().map_or(false, + |expr| is_add(cx, &*expr, target)), + &ExprParen(ref expr) => is_add(cx, &*expr, target), _ => false } } From a22b3cdceecb85bf55a21e15959d639d98f6da5e Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 21 Aug 2015 12:26:03 +0200 Subject: [PATCH 11/33] const folding for eq_op --- src/eq_op.rs | 5 +++-- tests/compile-fail/eq_op.rs | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/eq_op.rs b/src/eq_op.rs index 6202dcc3670..ebc6aa17100 100644 --- a/src/eq_op.rs +++ b/src/eq_op.rs @@ -33,7 +33,7 @@ impl LintPass for EqOp { } pub fn is_exp_equal(cx: &Context, left : &Expr, right : &Expr) -> bool { - match (&left.node, &right.node) { + if match (&left.node, &right.node) { (&ExprBinary(ref lop, ref ll, ref lr), &ExprBinary(ref rop, ref rl, ref rr)) => lop.node == rop.node && @@ -66,7 +66,8 @@ pub fn is_exp_equal(cx: &Context, left : &Expr, right : &Expr) -> bool { lunop == runop && is_exp_equal(cx, l, r), (&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r), _ => false - } || match (constant(cx, left), constant(cx, right)) { + } { return true; } + match (constant(cx, left), constant(cx, right)) { (Some(l), Some(r)) => l == r, _ => false } 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 } From b2df15d65a7baf1f8d9b8b776027a34dd6e1db10 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 18:28:17 +0200 Subject: [PATCH 12/33] ptr_arg improvements (fixes #214) * do not trigger on mutable references * use "real" type from ty, not AST type --- src/ptr_arg.rs | 45 ++++++++++++++++++++--------------- tests/compile-fail/ptr_arg.rs | 19 ++++++++------- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index 2748d187a4e..35c61b10266 100644 --- a/src/ptr_arg.rs +++ b/src/ptr_arg.rs @@ -4,10 +4,9 @@ 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_def_path}; declare_lint! { pub PTR_ARG, @@ -43,24 +42,32 @@ impl LintPass for PtrArg { } } +#[allow(unused_imports)] fn check_fn(cx: &Context, decl: &FnDecl) { + { + // In case stuff gets moved around + use collections::vec::Vec; + use collections::string::String; + } 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 arg.ty.node == TyInfer { // "self" arguments + continue; + } + let ref sty = cx.tcx.pat_ty(&*arg.pat).sty; + if let &ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = sty { + if let ty::TyStruct(did, _) = ty.sty { + if match_def_path(cx, did.did, &["collections", "vec", "Vec"]) { + 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_def_path(cx, did.did, &["collections", "string", "String"]) { + 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/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() { } From 707e95f2e5b9f6e20998508d99959dd1642d26ec Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 18:40:36 +0200 Subject: [PATCH 13/33] types: use middle::ty types instead of ast types This gets rid of the match_ty_unwrap function. --- src/types.rs | 71 ++++++++++++------------------------- tests/compile-fail/dlist.rs | 2 +- 2 files changed, 24 insertions(+), 49 deletions(-) mode change 100644 => 100755 tests/compile-fail/dlist.rs diff --git a/src/types.rs b/src/types.rs index 915ffb15fe5..986fb1016ed 100644 --- a/src/types.rs +++ b/src/types.rs @@ -2,11 +2,10 @@ use rustc::lint::*; use syntax::ast; use syntax::ast::*; use syntax::ast_util::{is_comparison_binop, binop_to_string}; -use syntax::ptr::P; use rustc::middle::ty; use syntax::codemap::ExpnInfo; -use utils::{in_macro, snippet, span_lint, span_help_and_lint, in_external_macro}; +use utils::{in_macro, match_def_path, snippet, span_lint, span_help_and_lint, in_external_macro}; /// Handles all the linting of funky types #[allow(missing_copy_implementations)] @@ -18,61 +17,37 @@ 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) { + fn check_ty(&mut self, cx: &Context, ast_ty: &ast::Ty) { { // In case stuff gets moved around - use std::boxed::Box; - use std::vec::Vec; + use collections::vec::Vec; + use collections::linked_list::LinkedList; } - 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; + 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 let ty::TyStruct(did, _) = inner.sty { + if match_def_path(cx, did.did, &["collections", "vec", "Vec"]) { + 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"); + } + } + } + if let ty::TyStruct(did, _) = ty.sty { + if match_def_path(cx, did.did, &["collections", "linked_list", "LinkedList"]) { + 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"); + return; + } } } } 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 +} From a437936d49081faa82227c7d216ff3b0d6363ed3 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 18:48:36 +0200 Subject: [PATCH 14/33] all: put often used DefPaths into utils as consts Also remove the "use xxx;" blocks to ensure import paths don't change. They don't work anyway since stuff may still be re-exported at the old location, while we need the "canonical" location for the type checks. Plus, the test suite catches all these cases. --- src/methods.rs | 14 ++++---------- src/ptr_arg.rs | 11 +++-------- src/strings.rs | 3 ++- src/types.rs | 11 +++-------- src/utils.rs | 7 +++++++ 5 files changed, 19 insertions(+), 27 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index f2df736bebc..70cf32e5093 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -3,6 +3,7 @@ use rustc::lint::*; use rustc::middle::ty; use utils::{span_lint, match_def_path, walk_ptrs_ty}; +use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; #[derive(Copy,Clone)] pub struct MethodsPass; @@ -16,30 +17,23 @@ 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; if ident.node.name == "unwrap" { if let ty::TyEnum(did, _) = *obj_ty { - if match_def_path(cx, did.did, &["core", "option", "Option"]) { + if match_def_path(cx, did.did, &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_def_path(cx, did.did, &["core", "result", "Result"]) { + else if match_def_path(cx, did.did, &RESULT_PATH) { span_lint(cx, RESULT_UNWRAP_USED, expr.span, "used unwrap() on a Result value. Graceful handling \ of Err values is preferred"); @@ -51,7 +45,7 @@ impl LintPass for MethodsPass { 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"]) { + if match_def_path(cx, did.did, &STRING_PATH) { span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op") } diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index 35c61b10266..cdf4ecb48e5 100644 --- a/src/ptr_arg.rs +++ b/src/ptr_arg.rs @@ -7,6 +7,7 @@ use syntax::ast::*; use rustc::middle::ty; use utils::{span_lint, match_def_path}; +use utils::{STRING_PATH, VEC_PATH}; declare_lint! { pub PTR_ARG, @@ -42,13 +43,7 @@ impl LintPass for PtrArg { } } -#[allow(unused_imports)] fn check_fn(cx: &Context, decl: &FnDecl) { - { - // In case stuff gets moved around - use collections::vec::Vec; - use collections::string::String; - } for arg in &decl.inputs { if arg.ty.node == TyInfer { // "self" arguments continue; @@ -56,13 +51,13 @@ fn check_fn(cx: &Context, decl: &FnDecl) { let ref sty = cx.tcx.pat_ty(&*arg.pat).sty; if let &ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = sty { if let ty::TyStruct(did, _) = ty.sty { - if match_def_path(cx, did.did, &["collections", "vec", "Vec"]) { + if match_def_path(cx, did.did, &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_def_path(cx, did.did, &["collections", "string", "String"]) { + else if match_def_path(cx, did.did, &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`"); diff --git a/src/strings.rs b/src/strings.rs index b1da93d71e3..c4fbca9344f 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -10,6 +10,7 @@ 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::STRING_PATH; declare_lint! { pub STRING_ADD_ASSIGN, @@ -63,7 +64,7 @@ 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"]) + match_def_path(cx, did.did, &STRING_PATH) } else { false } } diff --git a/src/types.rs b/src/types.rs index 986fb1016ed..57649adee8b 100644 --- a/src/types.rs +++ b/src/types.rs @@ -6,6 +6,7 @@ use rustc::middle::ty; use syntax::codemap::ExpnInfo; use utils::{in_macro, match_def_path, 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)] @@ -17,22 +18,16 @@ declare_lint!(pub LINKEDLIST, Warn, "usage of LinkedList, usually a vector is faster, or a more specialized data \ structure like a RingBuf"); -#[allow(unused_imports)] impl LintPass for TypePass { fn get_lints(&self) -> LintArray { lint_array!(BOX_VEC, LINKEDLIST) } fn check_ty(&mut self, cx: &Context, ast_ty: &ast::Ty) { - { - // In case stuff gets moved around - use collections::vec::Vec; - use collections::linked_list::LinkedList; - } 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 let ty::TyStruct(did, _) = inner.sty { - if match_def_path(cx, did.did, &["collections", "vec", "Vec"]) { + if match_def_path(cx, did.did, &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`?", @@ -41,7 +36,7 @@ impl LintPass for TypePass { } } if let ty::TyStruct(did, _) = ty.sty { - if match_def_path(cx, did.did, &["collections", "linked_list", "LinkedList"]) { + if match_def_path(cx, did.did, &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?", diff --git a/src/utils.rs b/src/utils.rs index 47e3a3456d6..fe5c1433c84 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -6,6 +6,13 @@ 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 { From 8a10440641f7ec89236b1f40129738426ad4d702 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 19:00:33 +0200 Subject: [PATCH 15/33] utils: add match_type() helper function which saves one level of matching when checking for type paths --- src/methods.rs | 35 ++++++++++++++--------------------- src/ptr_arg.rs | 23 ++++++++++------------- src/ranges.rs | 11 ++++------- src/strings.rs | 8 ++------ src/types.rs | 27 +++++++++++---------------- src/utils.rs | 12 ++++++++++++ 6 files changed, 53 insertions(+), 63 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index 70cf32e5093..df8e35d98fb 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -2,7 +2,7 @@ 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)] @@ -24,31 +24,24 @@ impl LintPass for MethodsPass { fn check_expr(&mut self, cx: &Context, expr: &Expr) { 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, &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_def_path(cx, did.did, &RESULT_PATH) { - 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, &STRING_PATH) { - 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"); } } } diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index cdf4ecb48e5..f0a0592f5e2 100644 --- a/src/ptr_arg.rs +++ b/src/ptr_arg.rs @@ -6,7 +6,7 @@ use rustc::lint::*; use syntax::ast::*; use rustc::middle::ty; -use utils::{span_lint, match_def_path}; +use utils::{span_lint, match_type}; use utils::{STRING_PATH, VEC_PATH}; declare_lint! { @@ -50,18 +50,15 @@ fn check_fn(cx: &Context, decl: &FnDecl) { } let ref sty = cx.tcx.pat_ty(&*arg.pat).sty; if let &ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = sty { - if let ty::TyStruct(did, _) = ty.sty { - if match_def_path(cx, did.did, &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_def_path(cx, did.did, &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`"); - } + 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`"); } } } diff --git a/src/ranges.rs b/src/ranges.rs index bbe65285d58..d1a0a7e702e 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, @@ -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/strings.rs b/src/strings.rs index c4fbca9344f..64d18eeb26d 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -4,12 +4,11 @@ //! 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! { @@ -62,10 +61,7 @@ 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, &STRING_PATH) - } else { false } + match_type(cx, walk_ptrs_ty(cx.tcx.expr_ty(e)), &STRING_PATH) } fn is_add(cx: &Context, src: &Expr, target: &Expr) -> bool { diff --git a/src/types.rs b/src/types.rs index 57649adee8b..622f733f812 100644 --- a/src/types.rs +++ b/src/types.rs @@ -5,7 +5,7 @@ use syntax::ast_util::{is_comparison_binop, binop_to_string}; use rustc::middle::ty; use syntax::codemap::ExpnInfo; -use utils::{in_macro, match_def_path, snippet, span_lint, span_help_and_lint, in_external_macro}; +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 @@ -26,23 +26,18 @@ impl LintPass for TypePass { 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 let ty::TyStruct(did, _) = inner.sty { - if match_def_path(cx, did.did, &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"); - } + 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"); } } - if let ty::TyStruct(did, _) = ty.sty { - if match_def_path(cx, did.did, &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"); - return; - } + 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"); } } } diff --git a/src/utils.rs b/src/utils.rs index fe5c1433c84..4fd36fb91d4 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -44,6 +44,18 @@ 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 + } + } +} + /// 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 { From f1255d5f5d9c9d23d039ee3798af4e164563226f Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Sat, 22 Aug 2015 02:44:05 +0200 Subject: [PATCH 16/33] Casts : work in progress handling *size separately --- src/types.rs | 37 ++++++++++++++++++++++++++++--------- tests/compile-fail/cast.rs | 28 +++++++--------------------- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/src/types.rs b/src/types.rs index 915ffb15fe5..e8da11ecbe0 100644 --- a/src/types.rs +++ b/src/types.rs @@ -150,14 +150,21 @@ declare_lint!(pub CAST_POSSIBLE_TRUNCATION, Allow, /// 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 + &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 + } +} + impl LintPass for CastPass { fn get_lints(&self) -> LintArray { lint_array!(CAST_PRECISION_LOSS, @@ -178,7 +185,14 @@ impl LintPass for CastPass { _ => 0 }; if from_nbits != 0 { - if from_nbits >= to_nbits { + // When casting to f32, precision loss would occur regardless of the arch + if is_isize_or_usize(cast_from) && to_nbits == 64 { + span_lint(cx, CAST_PRECISION_LOSS, expr.span, + &format!("converting from {0} to f64, which causes a loss of precision on 64-bit architectures \ + ({0} is 64 bits wide, but f64's mantissa is only 52 bits wide)", + cast_from)); + } + else if from_nbits >= to_nbits { span_lint(cx, CAST_PRECISION_LOSS, expr.span, &format!("converting from {0} to {1}, which causes a loss of precision \ ({0} is {2} bits wide, but {1}'s mantissa is only {3} bits wide)", @@ -186,7 +200,7 @@ impl LintPass for CastPass { } } }, - (false, true) => { + (false, true) => { // Nothing to add there span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, &format!("casting {} to {} may cause truncation of the value", cast_from, cast_to)); if !cast_to.is_signed() { @@ -201,10 +215,15 @@ impl LintPass for CastPass { } let from_nbits = int_ty_to_nbits(cast_from); let to_nbits = int_ty_to_nbits(cast_to); - if to_nbits < from_nbits || - (!cast_from.is_signed() && cast_to.is_signed() && to_nbits <= from_nbits) { - span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, - &format!("casting {} to {} may cause truncation of the value", cast_from, cast_to)); + match (is_isize_or_usize(cast_from), is_isize_or_usize(cast_to)) { + (true, true) | (false, false) => + if to_nbits < from_nbits || + (!cast_from.is_signed() && cast_to.is_signed() && to_nbits <= from_nbits) { + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may cause truncation of the value", cast_from, cast_to)); + }, + (true, false) => (), // TODO + (false, true) => () // TODO } } (false, false) => { diff --git a/tests/compile-fail/cast.rs b/tests/compile-fail/cast.rs index 0fa402b3bf7..8e854fb21f9 100644 --- a/tests/compile-fail/cast.rs +++ b/tests/compile-fail/cast.rs @@ -32,26 +32,12 @@ fn main() { i as u32; //~ERROR casting from i32 to u32 loses the sign of the value // Extra checks for usize/isize - let is : isize = -42; - is as usize; //~ERROR casting from isize to usize loses the sign of the value - is as i8; //~ERROR casting isize to i8 may cause truncation of the value - - // FIXME : enable these checks when we figure out a way to make compiletest deal with conditional compilation /* - #[cfg(target_pointer_width = "64")] - fn check_64() { - let is : isize = -42; - let us : usize = 42; - is as f32; //ERROR converting from isize to f32, which causes a loss of precision (isize is 64 bits wide, but f32's mantissa is only 23 bits wide) - us as u32; //ERROR casting usize to u32 may cause truncation of the value - us as u64; // Should not trigger any lint - } - #[cfg(target_pointer_width = "32")] - fn check_32() { - let is : isize = -42; - let us : usize = 42; - is as f32; //ERROR converting from isize to f32, which causes a loss of precision (isize is 32 bits wide, but f32's mantissa is only 23 bits wide) - us as u32; // Should not trigger any lint - us as u64; // Should not trigger any lint - }*/ + let is : isize = -42; + let us : usize = 42; + is as usize; //ERROR casting from isize to usize loses the sign of the value + is as i8; //ERROR casting isize to i8 may cause truncation of the value + is as f64; //ERROR converting from isize to f64, which causes a loss of precision on 64-bit architectures (isize is 64 bits wide, but f64's mantissa is only 52 bits wide) + us as f64; //ERROR converting from usize to f64, which causes a loss of precision on 64-bit architectures (usize is 64 bits wide, but f64's mantissa is only 52 bits wide) + */ } \ No newline at end of file From e92bf84a535b77c65127fadc2b82d762cff8ab66 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Sat, 22 Aug 2015 08:57:05 +0200 Subject: [PATCH 17/33] ptr_arg: fix panic when pattern type is not in tcx --- src/ptr_arg.rs | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index f0a0592f5e2..2d09fcbcca9 100644 --- a/src/ptr_arg.rs +++ b/src/ptr_arg.rs @@ -45,20 +45,18 @@ impl LintPass for PtrArg { fn check_fn(cx: &Context, decl: &FnDecl) { for arg in &decl.inputs { - if arg.ty.node == TyInfer { // "self" arguments - continue; - } - let ref sty = cx.tcx.pat_ty(&*arg.pat).sty; - if let &ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = 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`"); + 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`"); + } } } } From 630bb76f960c49d559efde55030b9a3ccb8b5704 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 22:53:47 +0200 Subject: [PATCH 18/33] new lint: type complexity (fixes #93) Still very naive, but it's a start. --- README.md | 1 + src/lib.rs | 2 + src/types.rs | 125 ++++++++++++++++++++++++++++ tests/compile-fail/complex_types.rs | 44 ++++++++++ 4 files changed, 172 insertions(+) create mode 100755 tests/compile-fail/complex_types.rs diff --git a/README.md b/README.md index fbb16fcb170..55976104058 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,7 @@ string_add | allow | using `x + ..` where x is a `String`; sugge 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 diff --git a/src/lib.rs b/src/lib.rs index 9ea1efeed5f..b0f46ae45ab 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -70,6 +70,7 @@ pub fn plugin_registrar(reg: &mut Registry) { 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_group("clippy", vec![ approx_const::APPROX_CONSTANT, @@ -111,6 +112,7 @@ pub fn plugin_registrar(reg: &mut Registry) { 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/types.rs b/src/types.rs index 622f733f812..54c36535286 100644 --- a/src/types.rs +++ b/src/types.rs @@ -2,6 +2,8 @@ use rustc::lint::*; use syntax::ast; use syntax::ast::*; use syntax::ast_util::{is_comparison_binop, binop_to_string}; +use syntax::codemap::Span; +use syntax::visit::{FnKind, Visitor, walk_ty}; use rustc::middle::ty; use syntax::codemap::ExpnInfo; @@ -183,3 +185,126 @@ impl LintPass for CastPass { } } } + +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) { + 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/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() { +} From 1334f2ceaea5e012bba02b6afc0371bf92e976e7 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sat, 22 Aug 2015 13:01:54 +0530 Subject: [PATCH 19/33] Fix doubleborrow of refcell in consts.rs --- src/consts.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/consts.rs b/src/consts.rs index 70d5ff4bc17..e54ac77b599 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -330,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() { From 1587256dc4651bbc53793fb461f1a22c6f65fc5c Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Sat, 22 Aug 2015 14:30:53 +0200 Subject: [PATCH 20/33] types: check for macros in type complexity check --- src/types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/types.rs b/src/types.rs index 54c36535286..cd7a5dc3729 100644 --- a/src/types.rs +++ b/src/types.rs @@ -259,6 +259,7 @@ fn check_fndecl(cx: &Context, decl: &FnDecl) { } 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); From 5403e826818a3c669f476909b24b4470e6e3749c Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 19:32:21 +0200 Subject: [PATCH 21/33] matches: new module, move single_match lint there --- src/lib.rs | 5 ++-- src/matches.rs | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ src/misc.rs | 63 +------------------------------------------------- 3 files changed, 65 insertions(+), 64 deletions(-) create mode 100644 src/matches.rs diff --git a/src/lib.rs b/src/lib.rs index b0f46ae45ab..fbeebb210fe 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -38,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); @@ -71,6 +71,7 @@ pub fn plugin_registrar(reg: &mut Registry) { 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, @@ -87,6 +88,7 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::EXPLICIT_ITER_LOOP, loops::ITER_NEXT_LOOP, loops::NEEDLESS_RANGE_LOOP, + matches::SINGLE_MATCH, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, methods::STR_TO_STRING, @@ -96,7 +98,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, diff --git a/src/matches.rs b/src/matches.rs new file mode 100644 index 00000000000..b9f6cbc4326 --- /dev/null +++ b/src/matches.rs @@ -0,0 +1,61 @@ +use rustc::lint::*; +use syntax::ast; +use syntax::ast::*; +use std::borrow::Cow; + +use utils::{snippet, snippet_block, 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"); + +#[allow(missing_copy_implementations)] +pub struct MatchPass; + +impl LintPass for MatchPass { + 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, + } +} diff --git a/src/misc.rs b/src/misc.rs index aca849931bb..49324de8de9 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))`)"); From 017dac23017e2dcf8fe350b66821a9e50d39bbd1 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 19:49:00 +0200 Subject: [PATCH 22/33] new lint: using &Ref patterns instead of matching on *expr (fixes #187) --- README.md | 1 + src/lib.rs | 1 + src/matches.rs | 30 +++++++++++++++++-- .../{match_if_let.rs => matches.rs} | 23 +++++++++++++- 4 files changed, 51 insertions(+), 4 deletions(-) rename tests/compile-fail/{match_if_let.rs => matches.rs} (58%) diff --git a/README.md b/README.md index 55976104058..72ce27392a7 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,7 @@ len_zero | warn | checking `.len() == 0` or `.len() > 0` (or 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 }` diff --git a/src/lib.rs b/src/lib.rs index fbeebb210fe..26af063c9a5 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -88,6 +88,7 @@ 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, diff --git a/src/matches.rs b/src/matches.rs index b9f6cbc4326..b704a2b47bb 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -3,23 +3,27 @@ use syntax::ast; use syntax::ast::*; use std::borrow::Cow; -use utils::{snippet, snippet_block, span_help_and_lint}; +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) + 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: only two arms + // 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() && @@ -48,6 +52,13 @@ impl LintPass for MatchPass { body_code) ); } + + // check preconditions for MATCH_REF_PATS + if has_only_ref_pats(arms) { + 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, ".."))); + } } } } @@ -59,3 +70,16 @@ fn is_unit_expr(expr: &Expr) -> bool { _ => false, } } + +fn has_only_ref_pats(arms: &[Arm]) -> bool { + for arm in arms { + for pat in &arm.pats { + match pat.node { + PatRegion(..) => (), // &-patterns + PatWild(..) => (), // an "anything" wildcard is also fine + _ => return false, + } + } + } + true +} diff --git a/tests/compile-fail/match_if_let.rs b/tests/compile-fail/matches.rs similarity index 58% rename from tests/compile-fail/match_if_let.rs rename to tests/compile-fail/matches.rs index bf2e7e43a52..43cf43b68df 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,23 @@ 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"), + } +} + +fn main() { +} From 8f1a2374938d77e3ecb713d57241d8209578ed0d Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 20:44:48 +0200 Subject: [PATCH 23/33] &-matches: dogfood fixes! --- src/approx_const.rs | 8 ++++---- src/bit_mask.rs | 12 ++++++------ src/eta_reduction.rs | 6 +++--- src/len_zero.rs | 8 ++++---- src/misc.rs | 6 +++--- src/needless_bool.rs | 6 +++--- src/strings.rs | 8 ++++---- src/types.rs | 16 ++++++++-------- 8 files changed, 35 insertions(+), 35 deletions(-) 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/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/eta_reduction.rs b/src/eta_reduction.rs index 6712e787278..25e967b07e5 100644 --- a/src/eta_reduction.rs +++ b/src/eta_reduction.rs @@ -18,9 +18,9 @@ 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) } diff --git a/src/len_zero.rs b/src/len_zero.rs index 073dcea582d..5eaa0256402 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,7 +100,7 @@ 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]) { span_lint(cx, LEN_ZERO, span, &format!( diff --git a/src/misc.rs b/src/misc.rs index 49324de8de9..81b03db5e14 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -175,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) { @@ -186,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"]) { diff --git a/src/needless_bool.rs b/src/needless_bool.rs index 18d98f1f063..7671d63a35d 100644 --- a/src/needless_bool.rs +++ b/src/needless_bool.rs @@ -60,9 +60,9 @@ fn fetch_bool_block(block: &Block) -> Option { } 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/strings.rs b/src/strings.rs index 64d18eeb26d..b24ea345244 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -65,13 +65,13 @@ fn is_string(cx: &Context, e: &Expr) -> bool { } fn is_add(cx: &Context, src: &Expr, target: &Expr) -> bool { - match &src.node { - &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => + match src.node { + ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => is_exp_equal(cx, target, left), - &ExprBlock(ref block) => block.stmts.is_empty() && + 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), + ExprParen(ref expr) => is_add(cx, &*expr, target), _ => false } } diff --git a/src/types.rs b/src/types.rs index 54c36535286..cb85fd6e0c6 100644 --- a/src/types.rs +++ b/src/types.rs @@ -116,10 +116,10 @@ declare_lint!(pub CAST_POSSIBLE_TRUNCATION, Allow, /// 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 + 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 } @@ -139,16 +139,16 @@ impl LintPass for CastPass { match (cast_from.is_integral(), cast_to.is_integral()) { (true, false) => { let from_nbits = int_ty_to_nbits(cast_from); - let to_nbits : usize = match &cast_to.sty { - &ty::TyFloat(ast::TyF32) => 32, - &ty::TyFloat(ast::TyF64) => 64, + let to_nbits : usize = match cast_to.sty { + ty::TyFloat(ast::TyF32) => 32, + ty::TyFloat(ast::TyF64) => 64, _ => 0 }; if from_nbits != 0 { if from_nbits >= to_nbits { span_lint(cx, CAST_PRECISION_LOSS, expr.span, &format!("converting from {0} to {1}, which causes a loss of precision \ - ({0} is {2} bits wide, but {1}'s mantissa is only {3} bits wide)", + ({0} is {2} bits wide, but {1}'s mantissa is only {3} bits wide)", cast_from, cast_to, from_nbits, if to_nbits == 64 {52} else {23} )); } } From 7580da306e338089b1cffedb09a71cb11debddf5 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 21 Aug 2015 20:49:59 +0200 Subject: [PATCH 24/33] matches: special message for this case match &e { &Pat1 => {}, &Pat2 => {}, ... } (inspired by dogfood fixes) --- src/matches.rs | 27 ++++++++++++++------------- tests/compile-fail/matches.rs | 6 ++++++ 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/matches.rs b/src/matches.rs index b704a2b47bb..002da07f50b 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -55,9 +55,15 @@ impl LintPass for MatchPass { // check preconditions for MATCH_REF_PATS if has_only_ref_pats(arms) { - 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, ".."))); + 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, ".."))); + } } } } @@ -72,14 +78,9 @@ fn is_unit_expr(expr: &Expr) -> bool { } fn has_only_ref_pats(arms: &[Arm]) -> bool { - for arm in arms { - for pat in &arm.pats { - match pat.node { - PatRegion(..) => (), // &-patterns - PatWild(..) => (), // an "anything" wildcard is also fine - _ => return false, - } - } - } - true + 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/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index 43cf43b68df..3cc540992c9 100755 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -53,6 +53,12 @@ fn ref_pats() { &(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() { From 807dab943bc35b8f579cc082f385bdd5a6a98c63 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Sat, 22 Aug 2015 21:36:54 +0200 Subject: [PATCH 25/33] Updated test case for cast lints. Also improved readability and reworded the messages. --- tests/compile-fail/cast.rs | 78 ++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 32 deletions(-) diff --git a/tests/compile-fail/cast.rs b/tests/compile-fail/cast.rs index 8e854fb21f9..9be0d501198 100644 --- a/tests/compile-fail/cast.rs +++ b/tests/compile-fail/cast.rs @@ -1,43 +1,57 @@ #![feature(plugin)] #![plugin(clippy)] -#[deny(cast_precision_loss, cast_possible_truncation, cast_sign_loss)] -#[allow(dead_code)] +#[deny(cast_precision_loss, cast_possible_truncation, cast_sign_loss, cast_possible_wrap)] fn main() { - let i : i32 = 42; - let u : u32 = 42; - let f : f32 = 42.0; - // Test cast_precision_loss - i as f32; //~ERROR converting from i32 to f32, which causes a loss of precision (i32 is 32 bits wide, but f32's mantissa is only 23 bits wide) - (i as i64) as f32; //~ERROR converting from i64 to f32, which causes a loss of precision (i64 is 64 bits wide, but f32's mantissa is only 23 bits wide) - (i as i64) as f64; //~ERROR converting from i64 to f64, which causes a loss of precision (i64 is 64 bits wide, but f64's mantissa is only 52 bits wide) - u as f32; //~ERROR converting from u32 to f32, which causes a loss of precision (u32 is 32 bits wide, but f32's mantissa is only 23 bits wide) - (u as u64) as f32; //~ERROR converting from u64 to f32, which causes a loss of precision (u64 is 64 bits wide, but f32's mantissa is only 23 bits wide) - (u as u64) as f64; //~ERROR converting from u64 to f64, which causes a loss of precision (u64 is 64 bits wide, but f64's mantissa is only 52 bits wide) - i as f64; // Should not trigger the lint - u as f64; // Should not trigger the lint + 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 - f as i32; //~ERROR casting f32 to i32 may cause truncation of the value - f as u32; //~ERROR casting f32 to u32 may cause truncation of the value - //~^ERROR casting from f32 to u32 loses the sign of the value - i as u8; //~ERROR casting i32 to u8 may cause truncation of the value - //~^ERROR casting from i32 to u8 loses the sign of the value - (f as f64) as f32; //~ERROR casting f64 to f32 may cause truncation of the value - i as i8; //~ERROR casting i32 to i8 may cause truncation of the value - u as i32; //~ERROR casting u32 to i32 may cause truncation of the value + 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 - i as u32; //~ERROR casting from i32 to u32 loses the sign of the value + 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 usize/isize - /* - let is : isize = -42; - let us : usize = 42; - is as usize; //ERROR casting from isize to usize loses the sign of the value - is as i8; //ERROR casting isize to i8 may cause truncation of the value - is as f64; //ERROR converting from isize to f64, which causes a loss of precision on 64-bit architectures (isize is 64 bits wide, but f64's mantissa is only 52 bits wide) - us as f64; //ERROR converting from usize to f64, which causes a loss of precision on 64-bit architectures (usize is 64 bits wide, but f64's mantissa is only 52 bits wide) - */ + // 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 + // 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 } \ No newline at end of file From 79ef13592e617e58b84146ba568c43bad81e77bf Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Sat, 22 Aug 2015 23:49:03 +0200 Subject: [PATCH 26/33] Completed the implementation of *size handling. Added some more cases to the test, and implemented a new lint, cast_possible_wrap, triggered when casting from an unsigned type to a signed type of the same size. --- README.md | 1 + src/lib.rs | 1 + src/types.rs | 88 +++++++++++++++++++++++++++++--------- tests/compile-fail/cast.rs | 8 +++- 4 files changed, 76 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index fbb16fcb170..7ac5388e0fe 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,7 @@ approx_constant | warn | the approximate of a known float 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) diff --git a/src/lib.rs b/src/lib.rs index 9ea1efeed5f..19a62dd214b 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -107,6 +107,7 @@ pub fn plugin_registrar(reg: &mut Registry) { 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, diff --git a/src/types.rs b/src/types.rs index e8da11ecbe0..579527cfff4 100644 --- a/src/types.rs +++ b/src/types.rs @@ -145,6 +145,8 @@ 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 @@ -169,7 +171,8 @@ impl LintPass for CastPass { fn get_lints(&self) -> LintArray { lint_array!(CAST_PRECISION_LOSS, CAST_SIGN_LOSS, - CAST_POSSIBLE_TRUNCATION) + CAST_POSSIBLE_TRUNCATION, + CAST_POSSIBLE_WRAP) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -186,50 +189,93 @@ impl LintPass for CastPass { }; if from_nbits != 0 { // When casting to f32, precision loss would occur regardless of the arch - if is_isize_or_usize(cast_from) && to_nbits == 64 { - span_lint(cx, CAST_PRECISION_LOSS, expr.span, - &format!("converting from {0} to f64, which causes a loss of precision on 64-bit architectures \ - ({0} is 64 bits wide, but f64's mantissa is only 52 bits wide)", - cast_from)); + if is_isize_or_usize(cast_from) { + if to_nbits == 64 { + span_lint(cx, CAST_PRECISION_LOSS, expr.span, + &format!("casting {0} to f64 causes a loss of precision on targets with 64-bit wide pointers \ + ({0} is 64 bits wide, but f64's mantissa is only 52 bits wide)", + cast_from)); + } + else { + span_lint(cx, CAST_PRECISION_LOSS, expr.span, + &format!("casting {0} to f32 causes a loss of precision \ + ({0} is 32 or 64 bits wide, but f32's mantissa is only 23 bits wide)", + cast_from)); + } } else if from_nbits >= to_nbits { span_lint(cx, CAST_PRECISION_LOSS, expr.span, - &format!("converting from {0} to {1}, which causes a loss of precision \ + &format!("casting {0} to {1} causes a loss of precision \ ({0} is {2} bits wide, but {1}'s mantissa is only {3} bits wide)", cast_from, cast_to, from_nbits, if to_nbits == 64 {52} else {23} )); } } }, - (false, true) => { // Nothing to add there + (false, true) => { + // Nothing to add there as long as UB in involved when the cast overflows span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, - &format!("casting {} to {} may cause truncation of the value", cast_from, cast_to)); + &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 from {} to {} loses the sign of the value", cast_from, cast_to)); + &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 from {} to {} loses the sign of the value", cast_from, cast_to)); - } let from_nbits = int_ty_to_nbits(cast_from); let to_nbits = int_ty_to_nbits(cast_to); + 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)); + } match (is_isize_or_usize(cast_from), is_isize_or_usize(cast_to)) { (true, true) | (false, false) => - if to_nbits < from_nbits || - (!cast_from.is_signed() && cast_to.is_signed() && to_nbits <= from_nbits) { - span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, - &format!("casting {} to {} may cause truncation of the value", cast_from, cast_to)); + if to_nbits < from_nbits { + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may truncate the value", cast_from, cast_to)); + } + else if !cast_from.is_signed() && cast_to.is_signed() && to_nbits == from_nbits { + span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, + &format!("casting {} to {} may wrap around the value", cast_from, cast_to)); }, - (true, false) => (), // TODO - (false, true) => () // TODO + (true, false) => + if to_nbits == 32 { + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may truncate the value on targets with 64-bit wide pointers", + cast_from, cast_to)); + if !cast_from.is_signed() && cast_to.is_signed() { + span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, + &format!("casting {} to {} may wrap around the value on targets with 32-bit wide pointers", + cast_from, cast_to)); + } + } + else if to_nbits < 32 { + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may truncate the value", cast_from, cast_to)); + }, + (false, true) => + if from_nbits == 64 { + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, + &format!("casting {} to {} may truncate the value on targets with 32-bit wide pointers", + cast_from, cast_to)); + if !cast_from.is_signed() && cast_to.is_signed() { + span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, + &format!("casting {} to {} may wrap around the value on targets with 64-bit wide pointers", + cast_from, cast_to)); + } + } + else { + if !cast_from.is_signed() && cast_to.is_signed() { + span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, + &format!("casting {} to {} may wrap around the value on targets with 32-bit wide pointers", + 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 cause truncation of the value"); + span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, "casting f64 to f32 may truncate the value"); } } } diff --git a/tests/compile-fail/cast.rs b/tests/compile-fail/cast.rs index 9be0d501198..b17f5de841b 100644 --- a/tests/compile-fail/cast.rs +++ b/tests/compile-fail/cast.rs @@ -45,8 +45,10 @@ fn main() { 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 + //~^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 @@ -54,4 +56,8 @@ fn main() { 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 From 3af2e3ba857630214d9bc81756be1c07ae305fc4 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Sun, 23 Aug 2015 01:06:31 +0200 Subject: [PATCH 27/33] Refactored CastPass. --- src/types.rs | 143 +++++++++++++++++++++++++-------------------------- 1 file changed, 70 insertions(+), 73 deletions(-) diff --git a/src/types.rs b/src/types.rs index 47ee51e98ef..4e9dd133ac8 100644 --- a/src/types.rs +++ b/src/types.rs @@ -134,6 +134,72 @@ fn is_isize_or_usize(typ: &ty::TyS) -> bool { } } +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, @@ -149,33 +215,9 @@ impl LintPass for CastPass { match (cast_from.is_integral(), cast_to.is_integral()) { (true, false) => { let from_nbits = int_ty_to_nbits(cast_from); - let to_nbits : usize = match cast_to.sty { - ty::TyFloat(ast::TyF32) => 32, - ty::TyFloat(ast::TyF64) => 64, - _ => 0 - }; - if from_nbits != 0 { - // When casting to f32, precision loss would occur regardless of the arch - if is_isize_or_usize(cast_from) { - if to_nbits == 64 { - span_lint(cx, CAST_PRECISION_LOSS, expr.span, - &format!("casting {0} to f64 causes a loss of precision on targets with 64-bit wide pointers \ - ({0} is 64 bits wide, but f64's mantissa is only 52 bits wide)", - cast_from)); - } - else { - span_lint(cx, CAST_PRECISION_LOSS, expr.span, - &format!("casting {0} to f32 causes a loss of precision \ - ({0} is 32 or 64 bits wide, but f32's mantissa is only 23 bits wide)", - cast_from)); - } - } - else if from_nbits >= to_nbits { - span_lint(cx, CAST_PRECISION_LOSS, expr.span, - &format!("casting {0} to {1} causes a loss of precision \ - ({0} is {2} bits wide, but {1}'s mantissa is only {3} bits wide)", - cast_from, cast_to, from_nbits, if to_nbits == 64 {52} else {23} )); - } + 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) => { @@ -187,56 +229,11 @@ impl LintPass for CastPass { } }, (true, true) => { - let from_nbits = int_ty_to_nbits(cast_from); - let to_nbits = int_ty_to_nbits(cast_to); 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)); } - match (is_isize_or_usize(cast_from), is_isize_or_usize(cast_to)) { - (true, true) | (false, false) => - if to_nbits < from_nbits { - span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, - &format!("casting {} to {} may truncate the value", cast_from, cast_to)); - } - else if !cast_from.is_signed() && cast_to.is_signed() && to_nbits == from_nbits { - span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, - &format!("casting {} to {} may wrap around the value", cast_from, cast_to)); - }, - (true, false) => - if to_nbits == 32 { - span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, - &format!("casting {} to {} may truncate the value on targets with 64-bit wide pointers", - cast_from, cast_to)); - if !cast_from.is_signed() && cast_to.is_signed() { - span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, - &format!("casting {} to {} may wrap around the value on targets with 32-bit wide pointers", - cast_from, cast_to)); - } - } - else if to_nbits < 32 { - span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, - &format!("casting {} to {} may truncate the value", cast_from, cast_to)); - }, - (false, true) => - if from_nbits == 64 { - span_lint(cx, CAST_POSSIBLE_TRUNCATION, expr.span, - &format!("casting {} to {} may truncate the value on targets with 32-bit wide pointers", - cast_from, cast_to)); - if !cast_from.is_signed() && cast_to.is_signed() { - span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, - &format!("casting {} to {} may wrap around the value on targets with 64-bit wide pointers", - cast_from, cast_to)); - } - } - else { - if !cast_from.is_signed() && cast_to.is_signed() { - span_lint(cx, CAST_POSSIBLE_WRAP, expr.span, - &format!("casting {} to {} may wrap around the value on targets with 32-bit wide pointers", - cast_from, cast_to)); - } - } - } + check_truncation_and_wrapping(cx, expr, cast_from, cast_to); } (false, false) => { if let (&ty::TyFloat(ast::TyF64), From c8a2e848ab249758f79ca048a5b06b8fef56cd87 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Sun, 23 Aug 2015 16:32:50 +0200 Subject: [PATCH 28/33] utils: extract utility method for matching trait method calls from loops --- src/loops.rs | 17 +++++------------ src/utils.rs | 13 +++++++++++++ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/loops.rs b/src/loops.rs index 36bab550b67..5f18439eafe 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"); } } } diff --git a/src/utils.rs b/src/utils.rs index 4fd36fb91d4..5e7c63e85d9 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -56,6 +56,19 @@ pub fn match_type(cx: &Context, ty: ty::Ty, path: &[&str]) -> bool { } } +/// 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 { From cc8f33d9152c38ca59cb79ccb89aca70fb3a7420 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Sun, 23 Aug 2015 16:34:23 +0200 Subject: [PATCH 29/33] ranges: remove unneeded as_str() --- src/ranges.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ranges.rs b/src/ranges.rs index d1a0a7e702e..914b4daa6be 100644 --- a/src/ranges.rs +++ b/src/ranges.rs @@ -20,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. \ From 380e41a914133f8e1c527e1f18db835dc4feb3f1 Mon Sep 17 00:00:00 2001 From: llogiq Date: Mon, 24 Aug 2015 16:23:05 +0200 Subject: [PATCH 30/33] improved README, added lint counter --- README.md | 13 +++---------- util/update_lints.py | 5 +++++ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 66881b52290..be7154c8c62 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ 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 -------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -54,13 +54,6 @@ type_complexity | warn | usage of very complex types; recommends fac 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 = "*" -``` - More to come, please [file an issue](https://github.com/Manishearth/rust-clippy/issues) if you have ideas! ##Usage @@ -69,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/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, From 81ef3da03cfb00eb16bd01d884fdc38835d9dfe0 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 25 Aug 2015 12:45:52 +0200 Subject: [PATCH 31/33] methods: people might be using to_string() to make a copy; add a hint for that --- src/methods.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/methods.rs b/src/methods.rs index df8e35d98fb..40043be109a 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -41,7 +41,8 @@ impl LintPass for MethodsPass { if obj_ty.sty == ty::TyStr { span_lint(cx, STR_TO_STRING, expr.span, "`str.to_owned()` is faster"); } else if match_type(cx, obj_ty, &STRING_PATH) { - span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op"); + span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op; use \ + `clone()` to make a copy"); } } } From d5c808acd05d5f91b2358984b0c4c97c33cfc1f9 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 25 Aug 2015 13:26:20 +0200 Subject: [PATCH 32/33] collapsible_if: remove extraneous note output This was probably a debug addition. --- src/collapsible_if.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/collapsible_if.rs b/src/collapsible_if.rs index 0b6dfc19e6b..7d654b43f2f 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 {} && {} {}", From 92a3394065e601f6f4ace7f374f5ce782d7b211d Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 25 Aug 2015 14:41:35 +0200 Subject: [PATCH 33/33] all: remove unneeded deref and/or ref operations --- src/attrs.rs | 4 ++-- src/collapsible_if.rs | 2 +- src/consts.rs | 6 +++--- src/eta_reduction.rs | 2 +- src/len_zero.rs | 2 +- src/lifetimes.rs | 6 +++--- src/loops.rs | 6 +++--- src/matches.rs | 10 +++++----- src/methods.rs | 2 +- src/misc.rs | 2 +- src/needless_bool.rs | 4 ++-- src/ptr_arg.rs | 2 +- src/returns.rs | 4 ++-- src/strings.rs | 4 ++-- src/types.rs | 10 +++++----- src/utils.rs | 4 ---- 16 files changed, 33 insertions(+), 37 deletions(-) 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/collapsible_if.rs b/src/collapsible_if.rs index 7d654b43f2f..e0b25b7283b 100644 --- a/src/collapsible_if.rs +++ b/src/collapsible_if.rs @@ -79,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 e54ac77b599..1a828317fc2 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -222,7 +222,7 @@ fn neg_float_str(s: String) -> String { if s.starts_with('-') { s[1..].to_owned() } else { - format!("-{}", &*s) + format!("-{}", s) } } @@ -299,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), @@ -362,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/eta_reduction.rs b/src/eta_reduction.rs index 25e967b07e5..481512abc62 100644 --- a/src/eta_reduction.rs +++ b/src/eta_reduction.rs @@ -22,7 +22,7 @@ impl LintPass for EtaPass { 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 5eaa0256402..ca3ce51bf7c 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -102,7 +102,7 @@ fn check_len_zero(cx: &Context, span: Span, method: &SpannedIdent, args: &[P], lit: &Lit, op: &str) { 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/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 5f18439eafe..ca8d3990fc5 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -95,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 index 002da07f50b..d1c74daf2cd 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -34,7 +34,7 @@ impl LintPass for MatchPass { // 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) + 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 { @@ -46,10 +46,10 @@ impl LintPass for MatchPass { "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) + &format!("try\nif let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, ex.span, ".."), + body_code) ); } diff --git a/src/methods.rs b/src/methods.rs index 40043be109a..07693e11d99 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -24,7 +24,7 @@ impl LintPass for MethodsPass { fn check_expr(&mut self, cx: &Context, expr: &Expr) { if let ExprMethodCall(ref ident, _, ref args) = expr.node { - let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])); + let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(&args[0])); if ident.node.name == "unwrap" { if match_type(cx, obj_ty, &OPTION_PATH) { span_lint(cx, OPTION_UNWRAP_USED, expr.span, diff --git a/src/misc.rs b/src/misc.rs index 81b03db5e14..2290af38bb5 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -203,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 7671d63a35d..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,7 +55,7 @@ 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 } } diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index 2d09fcbcca9..bcbd8dad68a 100644 --- a/src/ptr_arg.rs +++ b/src/ptr_arg.rs @@ -45,7 +45,7 @@ impl LintPass for PtrArg { fn check_fn(cx: &Context, decl: &FnDecl) { for arg in &decl.inputs { - if let Some(pat_ty) = cx.tcx.pat_ty_opt(&*arg.pat) { + 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, 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 b24ea345244..d03f4d53c60 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -70,8 +70,8 @@ fn is_add(cx: &Context, src: &Expr, target: &Expr) -> bool { 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), + |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 4e9dd133ac8..7479a65b6ee 100644 --- a/src/types.rs +++ b/src/types.rs @@ -55,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 {} =`", @@ -210,7 +210,7 @@ impl LintPass for CastPass { 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)); + 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) => { @@ -263,14 +263,14 @@ impl LintPass for TypeComplexityPass { } fn check_struct_field(&mut self, cx: &Context, field: &StructField) { - check_type(cx, &*field.node.ty); + 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); + check_type(cx, &arg.ty); } } } @@ -312,7 +312,7 @@ impl LintPass for TypeComplexityPass { fn check_fndecl(cx: &Context, decl: &FnDecl) { for arg in &decl.inputs { - check_type(cx, &*arg.ty); + check_type(cx, &arg.ty); } if let Return(ref ty) = decl.output { check_type(cx, ty); diff --git a/src/utils.rs b/src/utils.rs index 5e7c63e85d9..394204bedfc 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,7 +1,6 @@ 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; @@ -130,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);