Auto merge of #3725 - mikerite:fix-2728, r=phansch

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

Author: bors

clippy_lints/src/consts.rs

@@ -1,6 +1,7 @@
 #![allow(clippy::float_cmp)]
 
-use crate::utils::{clip, sext, unsext};
+use crate::utils::{clip, get_def_path, sext, unsext};
+use if_chain::if_chain;
 use rustc::hir::def::Def;
 use rustc::hir::*;
 use rustc::lint::LateContext;
@@ -234,6 +235,31 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
                 UnDeref => Some(o),
             }),
             ExprKind::Binary(op, ref left, ref right) => self.binop(op, left, right),
+            ExprKind::Call(ref callee, ref args) => {
+                // We only handle a few const functions for now
+                if_chain! {
+                    if args.is_empty();
+                    if let ExprKind::Path(qpath) = &callee.node;
+                    let def = self.tables.qpath_def(qpath, callee.hir_id);
+                    if let Some(def_id) = def.opt_def_id();
+                    let def_path = get_def_path(self.tcx, def_id);
+                    if let &["core", "num", impl_ty, "max_value"] = &def_path[..];
+                    then {
+                       let value = match impl_ty {
+                           "<impl i8>" => i8::max_value() as u128,
+                           "<impl i16>" => i16::max_value() as u128,
+                           "<impl i32>" => i32::max_value() as u128,
+                           "<impl i64>" => i64::max_value() as u128,
+                           "<impl i128>" => i128::max_value() as u128,
+                           _ => return None,
+                       };
+                       Some(Constant::Int(value))
+                    }
+                    else {
+                        None
+                    }
+                }
+            },
             // TODO: add other expressions
             _ => None,
         }

clippy_lints/src/types.rs

@@ -1000,6 +1000,31 @@ 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
+        }
+    }
+
+    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";
@@ -1175,14 +1200,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);
                     },

clippy_lints/src/utils/mod.rs

@@ -130,6 +130,21 @@ 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)
 }
 
+/// Get the absolute path of `def_id` as a vector of `&str`.
+///
+/// # Examples
+/// ```rust,ignore
+/// let def_path = get_def_path(tcx, def_id);
+/// if let &["core", "option", "Option"] = &def_path[..] {
+///     // The given `def_id` is that of an `Option` type
+/// };
+/// ```
+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 {

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;

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,36 +141,36 @@ 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
+  --> $DIR/cast.rs:37:5
    |
-LL |     1i32 as u32;
-   |     ^^^^^^^^^^^
+LL |     -1i32 as u32;
+   |     ^^^^^^^^^^^^
 
 error: casting isize to usize may lose the sign of the value
-  --> $DIR/cast.rs:37:5
+  --> $DIR/cast.rs:39:5
    |
-LL |     1isize as usize;
-   |     ^^^^^^^^^^^^^^^
+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;
    |     ^^^^^^^^^^^
    |
    = 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
 

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