Auto merge of #7948 - 5225225:castlosslessbool, r=llogiq

Lint for bool to integer casts in `cast_lossless`

The lint description says

> Checks for casts between *numerical* types that may be replaced by safe conversion functions.

Which is strictly speaking being violated here, but it seems within the spirit of the lint. I think it is still a useful lint to have, and having a different lint for just this feels excessive. Thoughts?

Fixes #7947

changelog: Lint for bool to integer casts in [`cast_lossless`]

Author: bors

clippy_lints/src/bool_assert_comparison.rs

@@ -73,7 +73,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
             if let Some(span) = is_direct_expn_of(expr.span, mac) {
                 if let Some(args) = higher::extract_assert_macro_args(expr) {
                     if let [a, b, ..] = args[..] {
-                        let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize;
+                        let nb_bool_args = usize::from(is_bool_lit(a)) + usize::from(is_bool_lit(b));
 
                         if nb_bool_args != 1 {
                             // If there are two boolean arguments, we definitely don't understand

clippy_lints/src/casts/cast_lossless.rs

@@ -1,16 +1,24 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::in_constant;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::is_isize_or_usize;
+use clippy_utils::{in_constant, meets_msrv, msrvs};
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind};
 use rustc_lint::LateContext;
 use rustc_middle::ty::{self, FloatTy, Ty};
+use rustc_semver::RustcVersion;
 
 use super::{utils, CAST_LOSSLESS};
 
-pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast_to: Ty<'_>) {
-    if !should_lint(cx, expr, cast_from, cast_to) {
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    cast_op: &Expr<'_>,
+    cast_from: Ty<'_>,
+    cast_to: Ty<'_>,
+    msrv: &Option<RustcVersion>,
+) {
+    if !should_lint(cx, expr, cast_from, cast_to, msrv) {
         return;
     }
 
@@ -32,21 +40,36 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_op: &Expr<'_>, c
         },
     );
 
+    let message = if cast_from.is_bool() {
+        format!(
+            "casting `{0:}` to `{1:}` is more cleanly stated with `{1:}::from(_)`",
+            cast_from, cast_to
+        )
+    } else {
+        format!(
+            "casting `{}` to `{}` may become silently lossy if you later change the type",
+            cast_from, cast_to
+        )
+    };
+
     span_lint_and_sugg(
         cx,
         CAST_LOSSLESS,
         expr.span,
-        &format!(
-            "casting `{}` to `{}` may become silently lossy if you later change the type",
-            cast_from, cast_to
-        ),
+        &message,
         "try",
         format!("{}::from({})", cast_to, sugg),
         applicability,
     );
 }
 
-fn should_lint(cx: &LateContext<'_>, expr: &Expr<'_>, cast_from: Ty<'_>, cast_to: Ty<'_>) -> bool {
+fn should_lint(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    cast_from: Ty<'_>,
+    cast_to: Ty<'_>,
+    msrv: &Option<RustcVersion>,
+) -> bool {
     // Do not suggest using From in consts/statics until it is valid to do so (see #2267).
     if in_constant(cx, expr.hir_id) {
         return false;
@@ -72,7 +95,7 @@ fn should_lint(cx: &LateContext<'_>, expr: &Expr<'_>, cast_from: Ty<'_>, cast_to
             };
             from_nbits < to_nbits
         },
-
+        (false, true) if matches!(cast_from.kind(), ty::Bool) && meets_msrv(msrv.as_ref(), &msrvs::FROM_BOOL) => true,
         (_, _) => {
             matches!(cast_from.kind(), ty::Float(FloatTy::F32)) && matches!(cast_to.kind(), ty::Float(FloatTy::F64))
         },

clippy_lints/src/casts/mod.rs

@@ -439,12 +439,16 @@ impl<'tcx> LateLintPass<'tcx> for Casts {
             fn_to_numeric_cast_any::check(cx, expr, cast_expr, cast_from, cast_to);
             fn_to_numeric_cast::check(cx, expr, cast_expr, cast_from, cast_to);
             fn_to_numeric_cast_with_truncation::check(cx, expr, cast_expr, cast_from, cast_to);
-            if cast_from.is_numeric() && cast_to.is_numeric() && !in_external_macro(cx.sess(), expr.span) {
-                cast_possible_truncation::check(cx, expr, cast_expr, cast_from, cast_to);
-                cast_possible_wrap::check(cx, expr, cast_from, cast_to);
-                cast_precision_loss::check(cx, expr, cast_from, cast_to);
-                cast_lossless::check(cx, expr, cast_expr, cast_from, cast_to);
-                cast_sign_loss::check(cx, expr, cast_expr, cast_from, cast_to);
+
+            if cast_to.is_numeric() && !in_external_macro(cx.sess(), expr.span) {
+                if cast_from.is_numeric() {
+                    cast_possible_truncation::check(cx, expr, cast_expr, cast_from, cast_to);
+                    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_lossless::check(cx, expr, cast_expr, cast_from, cast_to, &self.msrv);
             }
         }
 

clippy_utils/src/msrvs.rs

@@ -28,6 +28,7 @@ msrv_aliases! {
     1,35,0 { OPTION_COPIED, RANGE_CONTAINS }
     1,34,0 { TRY_FROM }
     1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES }
+    1,28,0 { FROM_BOOL }
     1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST }
     1,16,0 { STR_REPEAT }
 }

clippy_utils/src/source.rs

@@ -128,7 +128,7 @@ pub fn reindent_multiline(s: Cow<'_, str>, ignore_first: bool, indent: Option<us
 fn reindent_multiline_inner(s: &str, ignore_first: bool, indent: Option<usize>, ch: char) -> String {
     let x = s
         .lines()
-        .skip(ignore_first as usize)
+        .skip(usize::from(ignore_first))
         .filter_map(|l| {
             if l.is_empty() {
                 None

tests/ui/cast_lossless_bool.fixed

@@ -0,0 +1,42 @@
+// run-rustfix
+
+#![allow(dead_code)]
+#![warn(clippy::cast_lossless)]
+
+fn main() {
+    // Test clippy::cast_lossless with casts to integer types
+    let _ = u8::from(true);
+    let _ = u16::from(true);
+    let _ = u32::from(true);
+    let _ = u64::from(true);
+    let _ = u128::from(true);
+    let _ = usize::from(true);
+
+    let _ = i8::from(true);
+    let _ = i16::from(true);
+    let _ = i32::from(true);
+    let _ = i64::from(true);
+    let _ = i128::from(true);
+    let _ = isize::from(true);
+
+    // Test with an expression wrapped in parens
+    let _ = u16::from(true | false);
+}
+
+// The lint would suggest using `u32::from(input)` here but the `XX::from` function is not const,
+// so we skip the lint if the expression is in a const fn.
+// See #3656
+const fn abc(input: bool) -> u32 {
+    input as u32
+}
+
+// Same as the above issue. We can't suggest `::from` in const fns in impls
+mod cast_lossless_in_impl {
+    struct A;
+
+    impl A {
+        pub const fn convert(x: bool) -> u64 {
+            x as u64
+        }
+    }
+}

tests/ui/cast_lossless_bool.rs

@@ -0,0 +1,42 @@
+// run-rustfix
+
+#![allow(dead_code)]
+#![warn(clippy::cast_lossless)]
+
+fn main() {
+    // Test clippy::cast_lossless with casts to integer types
+    let _ = true as u8;
+    let _ = true as u16;
+    let _ = true as u32;
+    let _ = true as u64;
+    let _ = true as u128;
+    let _ = true as usize;
+
+    let _ = true as i8;
+    let _ = true as i16;
+    let _ = true as i32;
+    let _ = true as i64;
+    let _ = true as i128;
+    let _ = true as isize;
+
+    // Test with an expression wrapped in parens
+    let _ = (true | false) as u16;
+}
+
+// The lint would suggest using `u32::from(input)` here but the `XX::from` function is not const,
+// so we skip the lint if the expression is in a const fn.
+// See #3656
+const fn abc(input: bool) -> u32 {
+    input as u32
+}
+
+// Same as the above issue. We can't suggest `::from` in const fns in impls
+mod cast_lossless_in_impl {
+    struct A;
+
+    impl A {
+        pub const fn convert(x: bool) -> u64 {
+            x as u64
+        }
+    }
+}

tests/ui/cast_lossless_bool.stderr

@@ -0,0 +1,82 @@
+error: casting `bool` to `u8` is more cleanly stated with `u8::from(_)`
+  --> $DIR/cast_lossless_bool.rs:8:13
+   |
+LL |     let _ = true as u8;
+   |             ^^^^^^^^^^ help: try: `u8::from(true)`
+   |
+   = note: `-D clippy::cast-lossless` implied by `-D warnings`
+
+error: casting `bool` to `u16` is more cleanly stated with `u16::from(_)`
+  --> $DIR/cast_lossless_bool.rs:9:13
+   |
+LL |     let _ = true as u16;
+   |             ^^^^^^^^^^^ help: try: `u16::from(true)`
+
+error: casting `bool` to `u32` is more cleanly stated with `u32::from(_)`
+  --> $DIR/cast_lossless_bool.rs:10:13
+   |
+LL |     let _ = true as u32;
+   |             ^^^^^^^^^^^ help: try: `u32::from(true)`
+
+error: casting `bool` to `u64` is more cleanly stated with `u64::from(_)`
+  --> $DIR/cast_lossless_bool.rs:11:13
+   |
+LL |     let _ = true as u64;
+   |             ^^^^^^^^^^^ help: try: `u64::from(true)`
+
+error: casting `bool` to `u128` is more cleanly stated with `u128::from(_)`
+  --> $DIR/cast_lossless_bool.rs:12:13
+   |
+LL |     let _ = true as u128;
+   |             ^^^^^^^^^^^^ help: try: `u128::from(true)`
+
+error: casting `bool` to `usize` is more cleanly stated with `usize::from(_)`
+  --> $DIR/cast_lossless_bool.rs:13:13
+   |
+LL |     let _ = true as usize;
+   |             ^^^^^^^^^^^^^ help: try: `usize::from(true)`
+
+error: casting `bool` to `i8` is more cleanly stated with `i8::from(_)`
+  --> $DIR/cast_lossless_bool.rs:15:13
+   |
+LL |     let _ = true as i8;
+   |             ^^^^^^^^^^ help: try: `i8::from(true)`
+
+error: casting `bool` to `i16` is more cleanly stated with `i16::from(_)`
+  --> $DIR/cast_lossless_bool.rs:16:13
+   |
+LL |     let _ = true as i16;
+   |             ^^^^^^^^^^^ help: try: `i16::from(true)`
+
+error: casting `bool` to `i32` is more cleanly stated with `i32::from(_)`
+  --> $DIR/cast_lossless_bool.rs:17:13
+   |
+LL |     let _ = true as i32;
+   |             ^^^^^^^^^^^ help: try: `i32::from(true)`
+
+error: casting `bool` to `i64` is more cleanly stated with `i64::from(_)`
+  --> $DIR/cast_lossless_bool.rs:18:13
+   |
+LL |     let _ = true as i64;
+   |             ^^^^^^^^^^^ help: try: `i64::from(true)`
+
+error: casting `bool` to `i128` is more cleanly stated with `i128::from(_)`
+  --> $DIR/cast_lossless_bool.rs:19:13
+   |
+LL |     let _ = true as i128;
+   |             ^^^^^^^^^^^^ help: try: `i128::from(true)`
+
+error: casting `bool` to `isize` is more cleanly stated with `isize::from(_)`
+  --> $DIR/cast_lossless_bool.rs:20:13
+   |
+LL |     let _ = true as isize;
+   |             ^^^^^^^^^^^^^ help: try: `isize::from(true)`
+
+error: casting `bool` to `u16` is more cleanly stated with `u16::from(_)`
+  --> $DIR/cast_lossless_bool.rs:23:13
+   |
+LL |     let _ = (true | false) as u16;
+   |             ^^^^^^^^^^^^^^^^^^^^^ help: try: `u16::from(true | false)`
+
+error: aborting due to 13 previous errors
+

tests/ui/min_rust_version_attr.rs

@@ -140,6 +140,11 @@ fn unnest_or_patterns() {
 #[cfg_attr(rustfmt, rustfmt_skip)]
 fn deprecated_cfg_attr() {}
 
+#[warn(clippy::cast_lossless)]
+fn int_from_bool() -> u8 {
+    true as u8
+}
+
 fn main() {
     filter_map_next();
     checked_conversion();
@@ -156,6 +161,7 @@ fn main() {
     map_unwrap_or();
     missing_const_for_fn();
     unnest_or_patterns();
+    int_from_bool();
 }
 
 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:180:24
+  --> $DIR/min_rust_version_attr.rs:186: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:179:9
+  --> $DIR/min_rust_version_attr.rs:185: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:192:24
+  --> $DIR/min_rust_version_attr.rs:198:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
 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, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^