From 3cf8c0b3b52f14bda9001855369b72eca9737c1d Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 31 Jan 2019 06:20:49 +0200 Subject: [PATCH] Fix `cast_sign_loss` false positive This checks if the value is a non-negative constant before linting about losing the sign. Because the `constant` function doesn't handle const functions, we check if the value is from a call to a `max_value` function directly. A utility method called `get_def_path` was added to make checking for the function paths easier. Fixes #2728 --- clippy_lints/src/types.rs | 55 ++++++++++++++++++++++++++++------- clippy_lints/src/utils/mod.rs | 6 ++++ tests/ui/cast.rs | 8 +++++ tests/ui/cast.stderr | 30 ++++++++----------- tests/ui/cast_size.stderr | 22 +------------- 5 files changed, 72 insertions(+), 49 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 94c83ed57..3c0171ce8 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -4,8 +4,8 @@ use crate::consts::{constant, Constant}; use crate::reexport::*; use crate::utils::paths; use crate::utils::{ - clip, comparisons, differing_macro_contexts, higher, in_constant, in_macro, int_bits, last_path_segment, - match_def_path, match_path, multispan_sugg, opt_def_id, same_tys, sext, snippet, snippet_opt, + clip, comparisons, differing_macro_contexts, get_def_path, higher, in_constant, in_macro, int_bits, + last_path_segment, match_def_path, match_path, multispan_sugg, opt_def_id, same_tys, sext, snippet, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then, unsext, AbsolutePathBuffer, }; @@ -1001,6 +1001,48 @@ enum ArchSuffix { None, } +fn check_loss_of_sign(cx: &LateContext<'_, '_>, expr: &Expr, op: &Expr, cast_from: Ty<'_>, cast_to: Ty<'_>) { + if !cast_from.is_signed() || cast_to.is_signed() { + return; + } + + // don't lint for positive constants + let const_val = constant(cx, &cx.tables, op); + if_chain! { + if let Some((const_val, _)) = const_val; + if let Constant::Int(n) = const_val; + if let ty::Int(ity) = cast_from.sty; + if sext(cx.tcx, n, ity) >= 0; + then { + return + } + } + + // don't lint for max_value const fns + if_chain! { + if let ExprKind::Call(callee, args) = &op.node; + if args.is_empty(); + if let ExprKind::Path(qpath) = &callee.node; + let def = cx.tables.qpath_def(qpath, callee.hir_id); + if let Some(def_id) = def.opt_def_id(); + let def_path = get_def_path(cx.tcx, def_id); + if let &["core", "num", impl_ty, "max_value"] = &def_path[..]; + then { + if let "" | "" | "" | + "" | "" = impl_ty { + return; + } + } + } + + span_lint( + cx, + CAST_SIGN_LOSS, + expr.span, + &format!("casting {} to {} may lose the sign of the value", cast_from, cast_to), + ); +} + fn check_truncation_and_wrapping(cx: &LateContext<'_, '_>, expr: &Expr, cast_from: Ty<'_>, cast_to: Ty<'_>) { let arch_64_suffix = " on targets with 64-bit wide pointers"; let arch_32_suffix = " on targets with 32-bit wide pointers"; @@ -1176,14 +1218,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CastPass { } }, (true, true) => { - if cast_from.is_signed() && !cast_to.is_signed() { - span_lint( - cx, - CAST_SIGN_LOSS, - expr.span, - &format!("casting {} to {} may lose the sign of the value", cast_from, cast_to), - ); - } + check_loss_of_sign(cx, expr, ex, cast_from, cast_to); check_truncation_and_wrapping(cx, expr, cast_from, cast_to); check_lossless(cx, expr, ex, cast_from, cast_to); }, diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index ee3356fdc..b9cde75d5 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -130,6 +130,12 @@ pub fn match_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId, path: &[&str]) -> apb.names.len() == path.len() && apb.names.into_iter().zip(path.iter()).all(|(a, &b)| *a == *b) } +pub fn get_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId) -> Vec<&'static str> { + let mut apb = AbsolutePathBuffer { names: vec![] }; + tcx.push_item_path(&mut apb, def_id, false); + apb.names.iter().map(|n| n.get()).collect() +} + /// Check if type is struct, enum or union type with given def path. pub fn match_type(cx: &LateContext<'_, '_>, ty: Ty<'_>, path: &[&str]) -> bool { match ty.sty { diff --git a/tests/ui/cast.rs b/tests/ui/cast.rs index 5f4de9894..c248b5bf5 100644 --- a/tests/ui/cast.rs +++ b/tests/ui/cast.rs @@ -34,7 +34,15 @@ fn main() { (1u8 + 1u8) as u16; // Test clippy::cast_sign_loss 1i32 as u32; + -1i32 as u32; 1isize as usize; + -1isize as usize; + 0i8 as u8; + i8::max_value() as u8; + i16::max_value() as u16; + i32::max_value() as u32; + i64::max_value() as u64; + i128::max_value() as u128; // Extra checks for *size // Test cast_unnecessary 1i32 as i32; diff --git a/tests/ui/cast.stderr b/tests/ui/cast.stderr index 92587312a..c01393793 100644 --- a/tests/ui/cast.stderr +++ b/tests/ui/cast.stderr @@ -70,12 +70,6 @@ error: casting i32 to i8 may truncate the value LL | 1i32 as i8; | ^^^^^^^^^^ -error: casting i32 to u8 may lose the sign of the value - --> $DIR/cast.rs:22:5 - | -LL | 1i32 as u8; - | ^^^^^^^^^^ - error: casting i32 to u8 may truncate the value --> $DIR/cast.rs:22:5 | @@ -147,19 +141,19 @@ LL | (1u8 + 1u8) as u16; | ^^^^^^^^^^^^^^^^^^ help: try: `u16::from(1u8 + 1u8)` error: casting i32 to u32 may lose the sign of the value - --> $DIR/cast.rs:36:5 - | -LL | 1i32 as u32; - | ^^^^^^^^^^^ - -error: casting isize to usize may lose the sign of the value --> $DIR/cast.rs:37:5 | -LL | 1isize as usize; - | ^^^^^^^^^^^^^^^ +LL | -1i32 as u32; + | ^^^^^^^^^^^^ + +error: casting isize to usize may lose the sign of the value + --> $DIR/cast.rs:39:5 + | +LL | -1isize as usize; + | ^^^^^^^^^^^^^^^^ error: casting to the same type is unnecessary (`i32` -> `i32`) - --> $DIR/cast.rs:40:5 + --> $DIR/cast.rs:48:5 | LL | 1i32 as i32; | ^^^^^^^^^^^ @@ -167,16 +161,16 @@ LL | 1i32 as i32; = note: `-D clippy::unnecessary-cast` implied by `-D warnings` error: casting to the same type is unnecessary (`f32` -> `f32`) - --> $DIR/cast.rs:41:5 + --> $DIR/cast.rs:49:5 | LL | 1f32 as f32; | ^^^^^^^^^^^ error: casting to the same type is unnecessary (`bool` -> `bool`) - --> $DIR/cast.rs:42:5 + --> $DIR/cast.rs:50:5 | LL | false as bool; | ^^^^^^^^^^^^^ -error: aborting due to 28 previous errors +error: aborting due to 27 previous errors diff --git a/tests/ui/cast_size.stderr b/tests/ui/cast_size.stderr index 9346deb19..a77aafaf1 100644 --- a/tests/ui/cast_size.stderr +++ b/tests/ui/cast_size.stderr @@ -38,14 +38,6 @@ error: casting isize to i32 may truncate the value on targets with 64-bit wide p LL | 1isize as i32; | ^^^^^^^^^^^^^ -error: casting isize to u32 may lose the sign of the value - --> $DIR/cast_size.rs:17:5 - | -LL | 1isize as u32; - | ^^^^^^^^^^^^^ - | - = note: `-D clippy::cast-sign-loss` implied by `-D warnings` - error: casting isize to u32 may truncate the value on targets with 64-bit wide pointers --> $DIR/cast_size.rs:17:5 | @@ -78,12 +70,6 @@ error: casting i64 to isize may truncate the value on targets with 32-bit wide p LL | 1i64 as isize; | ^^^^^^^^^^^^^ -error: casting i64 to usize may lose the sign of the value - --> $DIR/cast_size.rs:22:5 - | -LL | 1i64 as usize; - | ^^^^^^^^^^^^^ - error: casting i64 to usize may truncate the value on targets with 32-bit wide pointers --> $DIR/cast_size.rs:22:5 | @@ -114,11 +100,5 @@ error: casting u32 to isize may wrap around the value on targets with 32-bit wid LL | 1u32 as isize; | ^^^^^^^^^^^^^ -error: casting i32 to usize may lose the sign of the value - --> $DIR/cast_size.rs:28:5 - | -LL | 1i32 as usize; - | ^^^^^^^^^^^^^ - -error: aborting due to 19 previous errors +error: aborting due to 16 previous errors