From 993239d33af2b91fcd5e6dbec30f3810c8178ae3 Mon Sep 17 00:00:00 2001 From: "R.Chavignat" Date: Thu, 20 Aug 2015 00:04:01 +0200 Subject: [PATCH] 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