From 33822012ec905a589ef16041a051095154b995ca Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Mon, 8 Nov 2021 20:44:50 +0000 Subject: [PATCH 1/4] Lint for bool to integer casts in `cast_lossless` --- clippy_lints/src/bool_assert_comparison.rs | 2 +- clippy_lints/src/casts/cast_lossless.rs | 2 +- clippy_lints/src/casts/mod.rs | 14 ++-- tests/ui/cast_lossless_bool.fixed | 42 +++++++++++ tests/ui/cast_lossless_bool.rs | 42 +++++++++++ tests/ui/cast_lossless_bool.stderr | 82 ++++++++++++++++++++++ 6 files changed, 177 insertions(+), 7 deletions(-) create mode 100644 tests/ui/cast_lossless_bool.fixed create mode 100644 tests/ui/cast_lossless_bool.rs create mode 100644 tests/ui/cast_lossless_bool.stderr diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index cdc192a47e48a..2e061267ff5b4 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -72,7 +72,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 diff --git a/clippy_lints/src/casts/cast_lossless.rs b/clippy_lints/src/casts/cast_lossless.rs index 869deecfbd53a..75dc909803385 100644 --- a/clippy_lints/src/casts/cast_lossless.rs +++ b/clippy_lints/src/casts/cast_lossless.rs @@ -72,7 +72,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) => true, (_, _) => { matches!(cast_from.kind(), ty::Float(FloatTy::F32)) && matches!(cast_to.kind(), ty::Float(FloatTy::F64)) }, diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs index 233abd178943e..108f6fbc92457 100644 --- a/clippy_lints/src/casts/mod.rs +++ b/clippy_lints/src/casts/mod.rs @@ -426,12 +426,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); + + 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); - cast_sign_loss::check(cx, expr, cast_expr, cast_from, cast_to); } } diff --git a/tests/ui/cast_lossless_bool.fixed b/tests/ui/cast_lossless_bool.fixed new file mode 100644 index 0000000000000..9e2da45c37858 --- /dev/null +++ b/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 + } + } +} diff --git a/tests/ui/cast_lossless_bool.rs b/tests/ui/cast_lossless_bool.rs new file mode 100644 index 0000000000000..b6f6c59a01f95 --- /dev/null +++ b/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 + } + } +} diff --git a/tests/ui/cast_lossless_bool.stderr b/tests/ui/cast_lossless_bool.stderr new file mode 100644 index 0000000000000..a0d11f08acf65 --- /dev/null +++ b/tests/ui/cast_lossless_bool.stderr @@ -0,0 +1,82 @@ +error: casting `bool` to `u8` may become silently lossy if you later change the type + --> $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` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:9:13 + | +LL | let _ = true as u16; + | ^^^^^^^^^^^ help: try: `u16::from(true)` + +error: casting `bool` to `u32` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:10:13 + | +LL | let _ = true as u32; + | ^^^^^^^^^^^ help: try: `u32::from(true)` + +error: casting `bool` to `u64` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:11:13 + | +LL | let _ = true as u64; + | ^^^^^^^^^^^ help: try: `u64::from(true)` + +error: casting `bool` to `u128` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:12:13 + | +LL | let _ = true as u128; + | ^^^^^^^^^^^^ help: try: `u128::from(true)` + +error: casting `bool` to `usize` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:13:13 + | +LL | let _ = true as usize; + | ^^^^^^^^^^^^^ help: try: `usize::from(true)` + +error: casting `bool` to `i8` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:15:13 + | +LL | let _ = true as i8; + | ^^^^^^^^^^ help: try: `i8::from(true)` + +error: casting `bool` to `i16` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:16:13 + | +LL | let _ = true as i16; + | ^^^^^^^^^^^ help: try: `i16::from(true)` + +error: casting `bool` to `i32` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:17:13 + | +LL | let _ = true as i32; + | ^^^^^^^^^^^ help: try: `i32::from(true)` + +error: casting `bool` to `i64` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:18:13 + | +LL | let _ = true as i64; + | ^^^^^^^^^^^ help: try: `i64::from(true)` + +error: casting `bool` to `i128` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:19:13 + | +LL | let _ = true as i128; + | ^^^^^^^^^^^^ help: try: `i128::from(true)` + +error: casting `bool` to `isize` may become silently lossy if you later change the type + --> $DIR/cast_lossless_bool.rs:20:13 + | +LL | let _ = true as isize; + | ^^^^^^^^^^^^^ help: try: `isize::from(true)` + +error: casting `bool` to `u16` may become silently lossy if you later change the type + --> $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 + From 96db1d6bea254b23bcda76206683d7452ff45c48 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Mon, 8 Nov 2021 22:03:06 +0000 Subject: [PATCH 2/4] fix dogfood lint on clippy_utils --- clippy_utils/src/source.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index 7f68cc388eb01..d928317259da0 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -128,7 +128,7 @@ pub fn reindent_multiline(s: Cow<'_, str>, ignore_first: bool, indent: Option, 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 From 6e84f00045e20cbd9f9d2695f67e9fc0852134d4 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Tue, 9 Nov 2021 17:35:35 +0000 Subject: [PATCH 3/4] Check MSRV for bool to int from impl --- clippy_lints/src/casts/cast_lossless.rs | 24 +++++++++++++++++++----- clippy_lints/src/casts/mod.rs | 2 +- clippy_utils/src/msrvs.rs | 1 + tests/ui/min_rust_version_attr.rs | 6 ++++++ tests/ui/min_rust_version_attr.stderr | 8 ++++---- 5 files changed, 31 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/casts/cast_lossless.rs b/clippy_lints/src/casts/cast_lossless.rs index 75dc909803385..cc360fc9c760c 100644 --- a/clippy_lints/src/casts/cast_lossless.rs +++ b/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, +) { + if !should_lint(cx, expr, cast_from, cast_to, msrv) { return; } @@ -46,7 +54,13 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_op: &Expr<'_>, c ); } -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, +) -> 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 +86,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) => true, + (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)) }, diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs index 108f6fbc92457..822ac4c242e0a 100644 --- a/clippy_lints/src/casts/mod.rs +++ b/clippy_lints/src/casts/mod.rs @@ -435,7 +435,7 @@ impl<'tcx> LateLintPass<'tcx> for Casts { cast_sign_loss::check(cx, expr, cast_expr, cast_from, cast_to); } - cast_lossless::check(cx, expr, cast_expr, cast_from, cast_to); + cast_lossless::check(cx, expr, cast_expr, cast_from, cast_to, &self.msrv); } } diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index f3913203f4b4f..fb11928dad6ce 100644 --- a/clippy_utils/src/msrvs.rs +++ b/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 } } diff --git a/tests/ui/min_rust_version_attr.rs b/tests/ui/min_rust_version_attr.rs index 57ed453da10b1..c5f221220ece7 100644 --- a/tests/ui/min_rust_version_attr.rs +++ b/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 { diff --git a/tests/ui/min_rust_version_attr.stderr b/tests/ui/min_rust_version_attr.stderr index 4388b78f832a3..6b3fdb0844b49 100644 --- a/tests/ui/min_rust_version_attr.stderr +++ b/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!(.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, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From d4c8cb63a474c2365664a68e35153adef3cdd9b9 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Fri, 12 Nov 2021 17:01:35 +0000 Subject: [PATCH 4/4] Change cast_lossless message for bools only --- clippy_lints/src/casts/cast_lossless.rs | 17 ++++++++++++---- tests/ui/cast_lossless_bool.stderr | 26 ++++++++++++------------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/casts/cast_lossless.rs b/clippy_lints/src/casts/cast_lossless.rs index cc360fc9c760c..4a95bed1148dc 100644 --- a/clippy_lints/src/casts/cast_lossless.rs +++ b/clippy_lints/src/casts/cast_lossless.rs @@ -40,14 +40,23 @@ pub(super) fn check( }, ); + 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, diff --git a/tests/ui/cast_lossless_bool.stderr b/tests/ui/cast_lossless_bool.stderr index a0d11f08acf65..6b148336011d5 100644 --- a/tests/ui/cast_lossless_bool.stderr +++ b/tests/ui/cast_lossless_bool.stderr @@ -1,4 +1,4 @@ -error: casting `bool` to `u8` may become silently lossy if you later change the type +error: casting `bool` to `u8` is more cleanly stated with `u8::from(_)` --> $DIR/cast_lossless_bool.rs:8:13 | LL | let _ = true as u8; @@ -6,73 +6,73 @@ LL | let _ = true as u8; | = note: `-D clippy::cast-lossless` implied by `-D warnings` -error: casting `bool` to `u16` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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` may become silently lossy if you later change the type +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;