From 51295960bf499368e48e9da2eafcdc9dbb7e2918 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 2 Mar 2017 10:41:20 +0100 Subject: [PATCH] Fix invalid_upcast_comparisons lint for same size sign casts --- clippy_lints/src/identity_op.rs | 2 +- clippy_lints/src/large_enum_variant.rs | 38 ++--- clippy_lints/src/types.rs | 12 +- clippy_lints/src/utils/mod.rs | 7 + tests/ui/invalid_upcast_comparisons.rs | 90 ++++++++--- tests/ui/invalid_upcast_comparisons.stderr | 178 +++++++++++++++++---- 6 files changed, 243 insertions(+), 84 deletions(-) diff --git a/clippy_lints/src/identity_op.rs b/clippy_lints/src/identity_op.rs index a0a61a0ee1e..82db13b1025 100644 --- a/clippy_lints/src/identity_op.rs +++ b/clippy_lints/src/identity_op.rs @@ -64,7 +64,7 @@ fn check(cx: &LateContext, e: &Expr, m: i8, span: Span, arg: Span) { if match m { 0 => v.to_u128_unchecked() == 0, -1 => match v.int_type() { - SignedInt(_) => (v.to_u128_unchecked() as i128 == -1), + SignedInt(_) => #[allow(cast_possible_wrap)] (v.to_u128_unchecked() as i128 == -1), UnsignedInt(_) => false }, 1 => v.to_u128_unchecked() == 1, diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs index f656d513b51..bfd17d995c8 100644 --- a/clippy_lints/src/large_enum_variant.rs +++ b/clippy_lints/src/large_enum_variant.rs @@ -2,10 +2,8 @@ use rustc::lint::*; use rustc::hir::*; -use utils::{span_lint_and_then, snippet_opt}; -use rustc::ty::layout::TargetDataLayout; +use utils::{span_lint_and_then, snippet_opt, type_size}; use rustc::ty::TypeFoldable; -use rustc::traits::Reveal; /// **What it does:** Checks for large size differences between variants on `enum`s. /// @@ -55,28 +53,22 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LargeEnumVariant { let mut largest_variant: Option<(_, _)> = None; for (i, variant) in adt.variants.iter().enumerate() { - let data_layout = TargetDataLayout::parse(cx.sess()); - cx.tcx.infer_ctxt((), Reveal::All).enter(|infcx| { - let size: u64 = variant.fields - .iter() - .map(|f| { - let ty = cx.tcx.item_type(f.did); - if ty.needs_subst() { - 0 // we can't reason about generics, so we treat them as zero sized - } else { - ty.layout(&infcx) - .expect("layout should be computable for concrete type") - .size(&data_layout) - .bytes() - } - }) - .sum(); + let size: u64 = variant.fields + .iter() + .map(|f| { + let ty = cx.tcx.item_type(f.did); + if ty.needs_subst() { + 0 // we can't reason about generics, so we treat them as zero sized + } else { + type_size(cx, ty).expect("size should be computable for concrete type") + } + }) + .sum(); - let grouped = (size, (i, variant)); + let grouped = (size, (i, variant)); - update_if(&mut smallest_variant, grouped, |a, b| b.0 <= a.0); - update_if(&mut largest_variant, grouped, |a, b| b.0 >= a.0); - }); + update_if(&mut smallest_variant, grouped, |a, b| b.0 <= a.0); + update_if(&mut largest_variant, grouped, |a, b| b.0 >= a.0); } if let (Some(smallest), Some(largest)) = (smallest_variant, largest_variant) { diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 4521ce426b5..c93ba00218a 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -8,7 +8,7 @@ use syntax::ast::{IntTy, UintTy, FloatTy}; use syntax::attr::IntType; use syntax::codemap::Span; use utils::{comparisons, higher, in_external_macro, in_macro, match_def_path, snippet, span_help_and_lint, span_lint, - opt_def_id, last_path_segment}; + opt_def_id, last_path_segment, type_size}; use utils::paths; /// Handles all the linting of funky types @@ -1077,7 +1077,13 @@ fn numeric_cast_precast_bounds<'a>(cx: &LateContext, expr: &'a Expr) -> Option<( use std::*; if let ExprCast(ref cast_exp, _) = expr.node { - match cx.tables.expr_ty(cast_exp).sty { + let pre_cast_ty = cx.tables.expr_ty(cast_exp); + let cast_ty = cx.tables.expr_ty(expr); + // if it's a cast from i32 to u32 wrapping will invalidate all these checks + if type_size(cx, pre_cast_ty) == type_size(cx, cast_ty) { + return None; + } + match pre_cast_ty.sty { TyInt(int_ty) => { Some(match int_ty { IntTy::I8 => (FullInt::S(i8::min_value() as i128), FullInt::S(i8::max_value() as i128)), @@ -1113,7 +1119,7 @@ fn node_as_const_fullint(cx: &LateContext, expr: &Expr) -> Option { Ok(val) => { if let Integral(const_int) = val { match const_int.int_type() { - IntType::SignedInt(_) => Some(FullInt::S(const_int.to_u128_unchecked() as i128)), + IntType::SignedInt(_) => #[allow(cast_possible_wrap)] Some(FullInt::S(const_int.to_u128_unchecked() as i128)), IntType::UnsignedInt(_) => Some(FullInt::U(const_int.to_u128_unchecked())), } } else { diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 9b888d531d0..18864d42df2 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -9,6 +9,7 @@ use rustc::traits::Reveal; use rustc::traits; use rustc::ty::subst::Subst; use rustc::ty; +use rustc::ty::layout::TargetDataLayout; use rustc_errors; use std::borrow::Cow; use std::env; @@ -972,3 +973,9 @@ pub fn is_try(expr: &Expr) -> Option<&Expr> { None } + +pub fn type_size<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: ty::Ty<'tcx>) -> Option { + cx.tcx.infer_ctxt((), Reveal::All).enter(|infcx| + ty.layout(&infcx).ok().map(|lay| lay.size(&TargetDataLayout::parse(cx.sess())).bytes()) + ) +} diff --git a/tests/ui/invalid_upcast_comparisons.rs b/tests/ui/invalid_upcast_comparisons.rs index 952cf1e4361..c59f04d4f38 100644 --- a/tests/ui/invalid_upcast_comparisons.rs +++ b/tests/ui/invalid_upcast_comparisons.rs @@ -3,33 +3,79 @@ #![deny(invalid_upcast_comparisons)] #![allow(unused, eq_op, no_effect, unnecessary_operation)] + +fn mk_value() -> T { unimplemented!() } + fn main() { - let zero: u32 = 0; - let u8_max: u8 = 255; + let u32: u32 = mk_value(); + let u8: u8 = mk_value(); + let i32: i32 = mk_value(); + let i8: i8 = mk_value(); - (u8_max as u32) > 300; - (u8_max as u32) > 20; + // always false, since no u8 can be > 300 + (u8 as u32) > 300; + (u8 as i32) > 300; + (u8 as u32) == 300; + (u8 as i32) == 300; + 300 < (u8 as u32); + 300 < (u8 as i32); + 300 == (u8 as u32); + 300 == (u8 as i32); + // inverted of the above + (u8 as u32) <= 300; + (u8 as i32) <= 300; + (u8 as u32) != 300; + (u8 as i32) != 300; + 300 >= (u8 as u32); + 300 >= (u8 as i32); + 300 != (u8 as u32); + 300 != (u8 as i32); - (zero as i32) < -5; - (zero as i32) < 10; + // always false, since u8 -> i32 doesn't wrap + (u8 as i32) < 0; + -5 != (u8 as i32); + // inverted of the above + (u8 as i32) >= 0; + -5 == (u8 as i32); - -5 < (zero as i32); - 0 <= (zero as i32); - 0 < (zero as i32); + // always false, since no u8 can be 1337 + 1337 == (u8 as i32); + 1337 == (u8 as u32); + // inverted of the above + 1337 != (u8 as i32); + 1337 != (u8 as u32); - -5 > (zero as i32); - -5 >= (u8_max as i32); - 1337 == (u8_max as i32); - - -5 == (zero as i32); - -5 != (u8_max as i32); // Those are Ok: - 42 == (u8_max as i32); - 42 != (u8_max as i32); - 42 > (u8_max as i32); - (u8_max as i32) == 42; - (u8_max as i32) != 42; - (u8_max as i32) > 42; - (u8_max as i32) < 42; + (u8 as u32) > 20; + 42 == (u8 as i32); + 42 != (u8 as i32); + 42 > (u8 as i32); + (u8 as i32) == 42; + (u8 as i32) != 42; + (u8 as i32) > 42; + (u8 as i32) < 42; + + (u8 as i8) == -1; + (u8 as i8) != -1; + (u8 as i32) > -1; + (u8 as i32) < -1; + (u32 as i32) < -5; + (u32 as i32) < 10; + + (i8 as u8) == 1; + (i8 as u8) != 1; + (i8 as u8) < 1; + (i8 as u8) > 1; + (i32 as u32) < 5; + (i32 as u32) < 10; + + -5 < (u32 as i32); + 0 <= (u32 as i32); + 0 < (u32 as i32); + + -5 > (u32 as i32); + -5 >= (u8 as i32); + + -5 == (u32 as i32); } diff --git a/tests/ui/invalid_upcast_comparisons.stderr b/tests/ui/invalid_upcast_comparisons.stderr index e7eda752f38..874081c22ab 100644 --- a/tests/ui/invalid_upcast_comparisons.stderr +++ b/tests/ui/invalid_upcast_comparisons.stderr @@ -1,8 +1,8 @@ -error: because of the numeric bounds on `u8_max` prior to casting, this expression is always false - --> $DIR/invalid_upcast_comparisons.rs:10:5 +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:16:5 | -10 | (u8_max as u32) > 300; - | ^^^^^^^^^^^^^^^^^^^^^ +16 | (u8 as u32) > 300; + | ^^^^^^^^^^^^^^^^^ | note: lint level defined here --> $DIR/invalid_upcast_comparisons.rs:4:9 @@ -10,53 +10,161 @@ note: lint level defined here 4 | #![deny(invalid_upcast_comparisons)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: because of the numeric bounds on `zero` prior to casting, this expression is always false - --> $DIR/invalid_upcast_comparisons.rs:13:5 - | -13 | (zero as i32) < -5; - | ^^^^^^^^^^^^^^^^^^ - -error: because of the numeric bounds on `zero` prior to casting, this expression is always true - --> $DIR/invalid_upcast_comparisons.rs:16:5 - | -16 | -5 < (zero as i32); - | ^^^^^^^^^^^^^^^^^^ - -error: because of the numeric bounds on `zero` prior to casting, this expression is always true +error: because of the numeric bounds on `u8` prior to casting, this expression is always false --> $DIR/invalid_upcast_comparisons.rs:17:5 | -17 | 0 <= (zero as i32); +17 | (u8 as i32) > 300; + | ^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:18:5 + | +18 | (u8 as u32) == 300; | ^^^^^^^^^^^^^^^^^^ -error: because of the numeric bounds on `zero` prior to casting, this expression is always false +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:19:5 + | +19 | (u8 as i32) == 300; + | ^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always false --> $DIR/invalid_upcast_comparisons.rs:20:5 | -20 | -5 > (zero as i32); - | ^^^^^^^^^^^^^^^^^^ +20 | 300 < (u8 as u32); + | ^^^^^^^^^^^^^^^^^ -error: because of the numeric bounds on `u8_max` prior to casting, this expression is always false +error: because of the numeric bounds on `u8` prior to casting, this expression is always false --> $DIR/invalid_upcast_comparisons.rs:21:5 | -21 | -5 >= (u8_max as i32); - | ^^^^^^^^^^^^^^^^^^^^^ +21 | 300 < (u8 as i32); + | ^^^^^^^^^^^^^^^^^ -error: because of the numeric bounds on `u8_max` prior to casting, this expression is always false +error: because of the numeric bounds on `u8` prior to casting, this expression is always false --> $DIR/invalid_upcast_comparisons.rs:22:5 | -22 | 1337 == (u8_max as i32); - | ^^^^^^^^^^^^^^^^^^^^^^^ +22 | 300 == (u8 as u32); + | ^^^^^^^^^^^^^^^^^^ -error: because of the numeric bounds on `zero` prior to casting, this expression is always false - --> $DIR/invalid_upcast_comparisons.rs:24:5 +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:23:5 | -24 | -5 == (zero as i32); - | ^^^^^^^^^^^^^^^^^^^ +23 | 300 == (u8 as i32); + | ^^^^^^^^^^^^^^^^^^ -error: because of the numeric bounds on `u8_max` prior to casting, this expression is always true +error: because of the numeric bounds on `u8` prior to casting, this expression is always true --> $DIR/invalid_upcast_comparisons.rs:25:5 | -25 | -5 != (u8_max as i32); - | ^^^^^^^^^^^^^^^^^^^^^ +25 | (u8 as u32) <= 300; + | ^^^^^^^^^^^^^^^^^^ -error: aborting due to 9 previous errors +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:26:5 + | +26 | (u8 as i32) <= 300; + | ^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:27:5 + | +27 | (u8 as u32) != 300; + | ^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:28:5 + | +28 | (u8 as i32) != 300; + | ^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:29:5 + | +29 | 300 >= (u8 as u32); + | ^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:30:5 + | +30 | 300 >= (u8 as i32); + | ^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:31:5 + | +31 | 300 != (u8 as u32); + | ^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:32:5 + | +32 | 300 != (u8 as i32); + | ^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:35:5 + | +35 | (u8 as i32) < 0; + | ^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:36:5 + | +36 | -5 != (u8 as i32); + | ^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:38:5 + | +38 | (u8 as i32) >= 0; + | ^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:39:5 + | +39 | -5 == (u8 as i32); + | ^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:42:5 + | +42 | 1337 == (u8 as i32); + | ^^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:43:5 + | +43 | 1337 == (u8 as u32); + | ^^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:45:5 + | +45 | 1337 != (u8 as i32); + | ^^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:46:5 + | +46 | 1337 != (u8 as u32); + | ^^^^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always true + --> $DIR/invalid_upcast_comparisons.rs:61:5 + | +61 | (u8 as i32) > -1; + | ^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:62:5 + | +62 | (u8 as i32) < -1; + | ^^^^^^^^^^^^^^^^ + +error: because of the numeric bounds on `u8` prior to casting, this expression is always false + --> $DIR/invalid_upcast_comparisons.rs:78:5 + | +78 | -5 >= (u8 as i32); + | ^^^^^^^^^^^^^^^^^ + +error: aborting due to 27 previous errors