Auto merge of #8635 - pbor:unsigned-abs, r=giraffate

Add a lint to detect cast to unsigned for abs() and suggest unsigned_…

…abs()

changelog: Add a [`cast_abs_to_unsigned`] that checks for uses of `abs()` that are cast to the corresponding unsigned integer type and suggest to replace them with `unsigned_abs()`.

Author: bors

CHANGELOG.md

@@ -3201,6 +3201,7 @@ Released 2018-09-13
 [`bytes_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#bytes_nth
 [`cargo_common_metadata`]: https://rust-lang.github.io/rust-clippy/master/index.html#cargo_common_metadata
 [`case_sensitive_file_extension_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#case_sensitive_file_extension_comparisons
+[`cast_abs_to_unsigned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_abs_to_unsigned
 [`cast_enum_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_enum_constructor
 [`cast_enum_truncation`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_enum_truncation
 [`cast_lossless`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_lossless

clippy_lints/src/casts/cast_abs_to_unsigned.rs

@@ -0,0 +1,42 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::sugg::Sugg;
+use clippy_utils::{meets_msrv, msrvs};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_middle::ty::Ty;
+use rustc_semver::RustcVersion;
+
+use super::CAST_ABS_TO_UNSIGNED;
+
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    cast_expr: &Expr<'_>,
+    cast_from: Ty<'_>,
+    cast_to: Ty<'_>,
+    msrv: &Option<RustcVersion>,
+) {
+    if_chain! {
+        if meets_msrv(msrv.as_ref(), &msrvs::UNSIGNED_ABS);
+        if cast_from.is_integral();
+        if cast_to.is_integral();
+        if cast_from.is_signed();
+        if !cast_to.is_signed();
+        if let ExprKind::MethodCall(method_path, args, _) = cast_expr.kind;
+        if let method_name = method_path.ident.name.as_str();
+        if method_name == "abs";
+        then {
+            span_lint_and_sugg(
+                cx,
+                CAST_ABS_TO_UNSIGNED,
+                expr.span,
+                &format!("casting the result of `{}::{}()` to {}", cast_from, method_name, cast_to),
+                "replace with",
+                format!("{}.unsigned_abs()", Sugg::hir(cx, &args[0], "..")),
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}

clippy_lints/src/casts/mod.rs

@@ -1,3 +1,4 @@
+mod cast_abs_to_unsigned;
 mod cast_enum_constructor;
 mod cast_lossless;
 mod cast_possible_truncation;
@@ -473,6 +474,28 @@ declare_clippy_lint! {
     "casts from an enum tuple constructor to an integer"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for uses of the `abs()` method that cast the result to unsigned.
+    ///
+    /// ### Why is this bad?
+    /// The `unsigned_abs()` method avoids panic when called on the MIN value.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let x: i32 = -42;
+    /// let y: u32 = x.abs() as u32;
+    /// ```
+    /// Use instead:
+    /// let x: i32 = -42;
+    /// let y: u32 = x.unsigned_abs();
+    /// ```
+    #[clippy::version = "1.61.0"]
+    pub CAST_ABS_TO_UNSIGNED,
+    suspicious,
+    "casting the result of `abs()` to an unsigned integer can panic"
+}
+
 pub struct Casts {
     msrv: Option<RustcVersion>,
 }
@@ -500,7 +523,8 @@ impl_lint_pass!(Casts => [
     CHAR_LIT_AS_U8,
     PTR_AS_PTR,
     CAST_ENUM_TRUNCATION,
-    CAST_ENUM_CONSTRUCTOR
+    CAST_ENUM_CONSTRUCTOR,
+    CAST_ABS_TO_UNSIGNED
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Casts {
@@ -536,6 +560,7 @@ impl<'tcx> LateLintPass<'tcx> for Casts {
                     cast_possible_wrap::check(cx, expr, cast_from, cast_to);
                     cast_precision_loss::check(cx, expr, cast_from, cast_to);
                     cast_sign_loss::check(cx, expr, cast_expr, cast_from, cast_to);
+                    cast_abs_to_unsigned::check(cx, expr, cast_expr, cast_from, cast_to, &self.msrv);
                 }
                 cast_lossless::check(cx, expr, cast_expr, cast_from, cast_to, &self.msrv);
                 cast_enum_constructor::check(cx, expr, cast_expr, cast_from);

clippy_lints/src/lib.register_all.rs

@@ -23,6 +23,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
     LintId::of(booleans::LOGIC_BUG),
     LintId::of(booleans::NONMINIMAL_BOOL),
+    LintId::of(casts::CAST_ABS_TO_UNSIGNED),
     LintId::of(casts::CAST_ENUM_CONSTRUCTOR),
     LintId::of(casts::CAST_ENUM_TRUNCATION),
     LintId::of(casts::CAST_REF_TO_MUT),

clippy_lints/src/lib.register_lints.rs

@@ -70,6 +70,7 @@ store.register_lints(&[
     cargo::REDUNDANT_FEATURE_NAMES,
     cargo::WILDCARD_DEPENDENCIES,
     case_sensitive_file_extension_comparisons::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS,
+    casts::CAST_ABS_TO_UNSIGNED,
     casts::CAST_ENUM_CONSTRUCTOR,
     casts::CAST_ENUM_TRUNCATION,
     casts::CAST_LOSSLESS,

clippy_lints/src/lib.register_suspicious.rs

@@ -7,6 +7,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
     LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS),
     LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
     LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
+    LintId::of(casts::CAST_ABS_TO_UNSIGNED),
     LintId::of(casts::CAST_ENUM_CONSTRUCTOR),
     LintId::of(casts::CAST_ENUM_TRUNCATION),
     LintId::of(crate_in_macro_def::CRATE_IN_MACRO_DEF),

clippy_lints/src/utils/conf.rs

@@ -156,7 +156,7 @@ define_Conf! {
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),
-    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, EXPECT_ERR.
+    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED.
     ///
     /// The minimum rust version that the project supports
     (msrv: Option<String> = None),

clippy_utils/src/msrvs.rs

@@ -14,7 +14,7 @@ macro_rules! msrv_aliases {
 msrv_aliases! {
     1,53,0 { OR_PATTERNS, MANUAL_BITS }
     1,52,0 { STR_SPLIT_ONCE }
-    1,51,0 { BORROW_AS_PTR }
+    1,51,0 { BORROW_AS_PTR, UNSIGNED_ABS }
     1,50,0 { BOOL_THEN }
     1,47,0 { TAU }
     1,46,0 { CONST_IF_MATCH }

tests/ui/cast.rs

@@ -7,7 +7,7 @@
     clippy::cast_sign_loss,
     clippy::cast_possible_wrap
 )]
-#[allow(clippy::no_effect, clippy::unnecessary_operation)]
+#[allow(clippy::cast_abs_to_unsigned, clippy::no_effect, clippy::unnecessary_operation)]
 fn main() {
     // Test clippy::cast_precision_loss
     let x0 = 1i32;

tests/ui/cast_abs_to_unsigned.fixed

@@ -0,0 +1,8 @@
+// run-rustfix
+#![warn(clippy::cast_abs_to_unsigned)]
+
+fn main() {
+    let x: i32 = -42;
+    let y: u32 = x.unsigned_abs();
+    println!("The absolute value of {} is {}", x, y);
+}

tests/ui/cast_abs_to_unsigned.rs

@@ -0,0 +1,8 @@
+// run-rustfix
+#![warn(clippy::cast_abs_to_unsigned)]
+
+fn main() {
+    let x: i32 = -42;
+    let y: u32 = x.abs() as u32;
+    println!("The absolute value of {} is {}", x, y);
+}

tests/ui/cast_abs_to_unsigned.stderr

@@ -0,0 +1,10 @@
+error: casting the result of `i32::abs()` to u32
+  --> $DIR/cast_abs_to_unsigned.rs:6:18
+   |
+LL |     let y: u32 = x.abs() as u32;
+   |                  ^^^^^^^^^^^^^^ help: replace with: `x.unsigned_abs()`
+   |
+   = note: `-D clippy::cast-abs-to-unsigned` implied by `-D warnings`
+
+error: aborting due to previous error
+

tests/ui/min_rust_version_attr.rs

@@ -150,6 +150,11 @@ fn err_expect() {
     x.err().expect("Testing expect_err");
 }
 
+fn cast_abs_to_unsigned() {
+    let x: i32 = 10;
+    assert_eq!(10u32, x.abs() as u32);
+}
+
 fn main() {
     filter_map_next();
     checked_conversion();
@@ -168,6 +173,7 @@ fn main() {
     unnest_or_patterns();
     int_from_bool();
     err_expect();
+    cast_abs_to_unsigned();
 }
 
 mod just_under_msrv {

tests/ui/min_rust_version_attr.stderr

@@ -1,12 +1,12 @@
 error: stripping a prefix manually
-  --> $DIR/min_rust_version_attr.rs:192:24
+  --> $DIR/min_rust_version_attr.rs:198:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::manual-strip` implied by `-D warnings`
 note: the prefix was tested here
-  --> $DIR/min_rust_version_attr.rs:191:9
+  --> $DIR/min_rust_version_attr.rs:197:9
    |
 LL |         if s.starts_with("hello, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -17,13 +17,13 @@ LL ~             assert_eq!(<stripped>.to_uppercase(), "WORLD!");
    |
 
 error: stripping a prefix manually
-  --> $DIR/min_rust_version_attr.rs:204:24
+  --> $DIR/min_rust_version_attr.rs:210:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
 note: the prefix was tested here
-  --> $DIR/min_rust_version_attr.rs:203:9
+  --> $DIR/min_rust_version_attr.rs:209:9
    |
 LL |         if s.starts_with("hello, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^