From 31dab972361c1f632ec9ce3e79faf5654acd6ec3 Mon Sep 17 00:00:00 2001 From: Gernot Ohner Date: Mon, 13 Nov 2023 15:01:36 -0700 Subject: [PATCH 1/6] create "cast_integer" lint with cargo dev new_lint --- CHANGELOG.md | 1 + clippy_lints/src/casts/cast_integer.rs | 8 ++++++++ clippy_lints/src/casts/mod.rs | 21 +++++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + tests/ui/cast_integer.rs | 5 +++++ 5 files changed, 36 insertions(+) create mode 100644 clippy_lints/src/casts/cast_integer.rs create mode 100644 tests/ui/cast_integer.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 05e76c0d0234..19353ab0d530 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4968,6 +4968,7 @@ Released 2018-09-13 [`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_integer`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_integer [`cast_lossless`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_lossless [`cast_nan_to_int`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_nan_to_int [`cast_possible_truncation`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_possible_truncation diff --git a/clippy_lints/src/casts/cast_integer.rs b/clippy_lints/src/casts/cast_integer.rs new file mode 100644 index 000000000000..61762440cc93 --- /dev/null +++ b/clippy_lints/src/casts/cast_integer.rs @@ -0,0 +1,8 @@ +use rustc_lint::{LateContext, LintContext}; + +use super::CAST_INTEGER; + +// TODO: Adjust the parameters as necessary +pub(super) fn check(cx: &LateContext<'_>) { + todo!(); +} diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs index e05b8f66d861..21b7651c7199 100644 --- a/clippy_lints/src/casts/mod.rs +++ b/clippy_lints/src/casts/mod.rs @@ -3,6 +3,7 @@ mod as_underscore; mod borrow_as_ptr; mod cast_abs_to_unsigned; mod cast_enum_constructor; +mod cast_integer; mod cast_lossless; mod cast_nan_to_int; mod cast_possible_truncation; @@ -689,6 +690,25 @@ declare_clippy_lint! { "using `0 as *{const, mut} T`" } +declare_clippy_lint! { + /// ### What it does + /// + /// ### Why is this bad? + /// + /// ### Example + /// ```no_run + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```no_run + /// // example code which does not raise clippy warning + /// ``` + #[clippy::version = "1.75.0"] + pub CAST_INTEGER, + pedantic, + "default lint description" +} + pub struct Casts { msrv: Msrv, } @@ -724,6 +744,7 @@ impl_lint_pass!(Casts => [ AS_PTR_CAST_MUT, CAST_NAN_TO_INT, ZERO_PTR, + CAST_INTEGER, ]); impl<'tcx> LateLintPass<'tcx> for Casts { diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index a9ae22e8d43a..12a4627db57d 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -81,6 +81,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::casts::CAST_ABS_TO_UNSIGNED_INFO, crate::casts::CAST_ENUM_CONSTRUCTOR_INFO, crate::casts::CAST_ENUM_TRUNCATION_INFO, + crate::casts::CAST_INTEGER_INFO, crate::casts::CAST_LOSSLESS_INFO, crate::casts::CAST_NAN_TO_INT_INFO, crate::casts::CAST_POSSIBLE_TRUNCATION_INFO, diff --git a/tests/ui/cast_integer.rs b/tests/ui/cast_integer.rs new file mode 100644 index 000000000000..28121b0848cb --- /dev/null +++ b/tests/ui/cast_integer.rs @@ -0,0 +1,5 @@ +#![warn(clippy::cast_integer)] + +fn main() { + // test code goes here +} From ec51e3d0dbd366b4ef28d305d080c888c36c20ac Mon Sep 17 00:00:00 2001 From: Gernot Ohner Date: Mon, 11 Dec 2023 19:42:57 -0700 Subject: [PATCH 2/6] Add `cast_integer` lint implementation and test --- clippy_lints/src/casts/cast_integer.rs | 38 +++++- clippy_lints/src/casts/mod.rs | 1 + tests/ui/cast_integer.rs | 67 ++++++++- tests/ui/cast_integer.stderr | 180 +++++++++++++++++++++++++ 4 files changed, 282 insertions(+), 4 deletions(-) create mode 100644 tests/ui/cast_integer.stderr diff --git a/clippy_lints/src/casts/cast_integer.rs b/clippy_lints/src/casts/cast_integer.rs index 61762440cc93..9be510fbad61 100644 --- a/clippy_lints/src/casts/cast_integer.rs +++ b/clippy_lints/src/casts/cast_integer.rs @@ -1,8 +1,40 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LintContext}; +use rustc_middle::ty::Ty; +use clippy_utils::in_constant; use super::CAST_INTEGER; -// TODO: Adjust the parameters as necessary -pub(super) fn check(cx: &LateContext<'_>) { - todo!(); +pub(super) fn check( + cx: &LateContext<'_>, + expr: &Expr<'_>, + from_ty: Ty<'_>, + to_ty: Ty<'_> +) { + + if !should_lint(cx, expr, from_ty, to_ty) { + return; + } + + span_lint_and_help( + cx, + CAST_INTEGER, + expr.span, + "Integer casts can introduce subtle surprises and should be done with From/TryFrom.", + None, + "Try T::from(_) or T::try_from(_) instead", + ) } + +fn should_lint(cx: &LateContext<'_>, expr: &Expr<'_>, cast_from: Ty<'_>, cast_to: Ty<'_>) -> 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; + } + + match (cast_from.is_integral(), cast_to.is_integral()) { + (true, true) => true, + (_, _) => false, + } +} \ No newline at end of file diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs index 21b7651c7199..559c8c7d3dd6 100644 --- a/clippy_lints/src/casts/mod.rs +++ b/clippy_lints/src/casts/mod.rs @@ -785,6 +785,7 @@ impl<'tcx> LateLintPass<'tcx> for Casts { 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_nan_to_int::check(cx, expr, cast_expr, cast_from, cast_to); + cast_integer::check(cx, expr, cast_from, cast_to); } cast_lossless::check(cx, expr, cast_expr, cast_from, cast_to, &self.msrv); cast_enum_constructor::check(cx, expr, cast_expr, cast_from); diff --git a/tests/ui/cast_integer.rs b/tests/ui/cast_integer.rs index 28121b0848cb..1c9e6a7a04bb 100644 --- a/tests/ui/cast_integer.rs +++ b/tests/ui/cast_integer.rs @@ -1,5 +1,70 @@ #![warn(clippy::cast_integer)] fn main() { - // test code goes here + let u = 1usize; + let _ = u as u64; + let _ = u64::try_from(u); + + // unsigned + let u64: u64 = 1; + let _ = u64 as u128; + let _ = u128::from(u64); + + let u32: u32 = 1; + let _ = u32 as u128; + let _ = u128::from(u32); + let _ = u32 as u64; + let _ = u64::from(u32); + + let u16: u16 = 1; + let _ = u16 as u128; + let _ = u128::from(u16); + let _ = u16 as u64; + let _ = u64::from(u16); + let _ = u16 as u32; + let _ = u32::from(u16); + + let u8: u8 = 1; + let _ = u8 as u128; + let _ = u128::from(u8); + let _ = u8 as u64; + let _ = u64::from(u8); + let _ = u8 as u32; + let _ = u32::from(u8); + let _ = u8 as u16; + let _ = u16::from(u8); + + // signed + + let isize: isize = 1; + let _ = isize as i64; + let _ = i64::try_from(isize); + + let i64: i64 = 1; + let _ = i64 as i128; + let _ = i128::from(i64); + + let i32 = 1; + let _ = i32 as i128; + let _ = i128::from(i32); + let _ = i32 as i64; + let _ = i64::from(i32); + + let i16: i16 = 1; + let _ = i16 as i128; + let _ = i128::from(i16); + let _ = i16 as i64; + let _ = i64::from(i16); + let _ = i16 as i32; + let _ = i32::from(i16); + + let i8: i8 = 1; + let _ = i8 as i128; + let _ = i128::from(i8); + let _ = i8 as i64; + let _ = i64::from(i8); + let _ = i8 as i32; + let _ = i32::from(i8); + let _ = i8 as i16; + let _ = i16::from(i8); } diff --git a/tests/ui/cast_integer.stderr b/tests/ui/cast_integer.stderr new file mode 100644 index 000000000000..bf4d66a89049 --- /dev/null +++ b/tests/ui/cast_integer.stderr @@ -0,0 +1,180 @@ +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:5:13 + | +LL | let _ = u as u64; + | ^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + = note: `-D clippy::cast-integer` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::cast_integer)]` + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:10:13 + | +LL | let _ = u64 as u128; + | ^^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:14:13 + | +LL | let _ = u32 as u128; + | ^^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:16:13 + | +LL | let _ = u32 as u64; + | ^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:20:13 + | +LL | let _ = u16 as u128; + | ^^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:22:13 + | +LL | let _ = u16 as u64; + | ^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:24:13 + | +LL | let _ = u16 as u32; + | ^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:28:13 + | +LL | let _ = u8 as u128; + | ^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:30:13 + | +LL | let _ = u8 as u64; + | ^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:32:13 + | +LL | let _ = u8 as u32; + | ^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:34:13 + | +LL | let _ = u8 as u16; + | ^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:40:13 + | +LL | let _ = isize as i64; + | ^^^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:44:13 + | +LL | let _ = i64 as i128; + | ^^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:48:13 + | +LL | let _ = i32 as i128; + | ^^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:50:13 + | +LL | let _ = i32 as i64; + | ^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:54:13 + | +LL | let _ = i16 as i128; + | ^^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:56:13 + | +LL | let _ = i16 as i64; + | ^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:58:13 + | +LL | let _ = i16 as i32; + | ^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:62:13 + | +LL | let _ = i8 as i128; + | ^^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:64:13 + | +LL | let _ = i8 as i64; + | ^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:66:13 + | +LL | let _ = i8 as i32; + | ^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:68:13 + | +LL | let _ = i8 as i16; + | ^^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: aborting due to 22 previous errors + From 7d91b207ef4a8aa871f43ed8f0dc8dc79de8176e Mon Sep 17 00:00:00 2001 From: Gernot Ohner Date: Mon, 11 Dec 2023 19:47:51 -0700 Subject: [PATCH 3/6] Add tests for const functions --- tests/ui/cast_integer.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/ui/cast_integer.rs b/tests/ui/cast_integer.rs index 1c9e6a7a04bb..1a52c00b424f 100644 --- a/tests/ui/cast_integer.rs +++ b/tests/ui/cast_integer.rs @@ -68,3 +68,21 @@ fn main() { let _ = i8 as i16; let _ = i16::from(i8); } + +// 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 const_function(input: u32) -> u64 { + input as u64 +} + +// Same as the above issue. We can't suggest `::from` in const fns in impls +mod cast_integer_in_impl { + struct A; + + impl A { + pub const fn convert(x: u32) -> u64 { + x as u64 + } + } +} \ No newline at end of file From 1b81ff2144689361fddda7ccb138974d82bffeac Mon Sep 17 00:00:00 2001 From: Gernot Ohner Date: Mon, 11 Dec 2023 19:50:07 -0700 Subject: [PATCH 4/6] Make code more idiomatic --- tests/ui/cast_integer.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/ui/cast_integer.rs b/tests/ui/cast_integer.rs index 1a52c00b424f..4adf4efc4241 100644 --- a/tests/ui/cast_integer.rs +++ b/tests/ui/cast_integer.rs @@ -6,17 +6,17 @@ fn main() { let _ = u64::try_from(u); // unsigned - let u64: u64 = 1; + let u64= 1u64; let _ = u64 as u128; let _ = u128::from(u64); - let u32: u32 = 1; + let u32 = 1u32; let _ = u32 as u128; let _ = u128::from(u32); let _ = u32 as u64; let _ = u64::from(u32); - let u16: u16 = 1; + let u16= 1u16; let _ = u16 as u128; let _ = u128::from(u16); let _ = u16 as u64; @@ -24,7 +24,7 @@ fn main() { let _ = u16 as u32; let _ = u32::from(u16); - let u8: u8 = 1; + let u8 = 1u8; let _ = u8 as u128; let _ = u128::from(u8); let _ = u8 as u64; @@ -36,21 +36,21 @@ fn main() { // signed - let isize: isize = 1; + let isize= 1isize; let _ = isize as i64; let _ = i64::try_from(isize); - let i64: i64 = 1; + let i64 = 1i64; let _ = i64 as i128; let _ = i128::from(i64); - let i32 = 1; + let i32 = 1i32; let _ = i32 as i128; let _ = i128::from(i32); let _ = i32 as i64; let _ = i64::from(i32); - let i16: i16 = 1; + let i16 = 1i16; let _ = i16 as i128; let _ = i128::from(i16); let _ = i16 as i64; @@ -58,7 +58,7 @@ fn main() { let _ = i16 as i32; let _ = i32::from(i16); - let i8: i8 = 1; + let i8 = 1i8; let _ = i8 as i128; let _ = i128::from(i8); let _ = i8 as i64; From 5cb06ea01d2690d535b4fd9337e65e7d15f7da7b Mon Sep 17 00:00:00 2001 From: Gernot Ohner Date: Mon, 11 Dec 2023 19:53:42 -0700 Subject: [PATCH 5/6] Add test for cast in function --- tests/ui/cast_integer.rs | 2 ++ tests/ui/cast_integer.stderr | 10 +++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/ui/cast_integer.rs b/tests/ui/cast_integer.rs index 4adf4efc4241..b9a7af9e2bf4 100644 --- a/tests/ui/cast_integer.rs +++ b/tests/ui/cast_integer.rs @@ -69,6 +69,8 @@ fn main() { let _ = i16::from(i8); } +pub fn non_const_function(x: usize) -> u64 { x as u64 } + // 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 diff --git a/tests/ui/cast_integer.stderr b/tests/ui/cast_integer.stderr index bf4d66a89049..6e3316d0839d 100644 --- a/tests/ui/cast_integer.stderr +++ b/tests/ui/cast_integer.stderr @@ -176,5 +176,13 @@ LL | let _ = i8 as i16; | = help: Try T::from(_) or T::try_from(_) instead -error: aborting due to 22 previous errors +error: Integer casts can introduce subtle surprises and should be done with From/TryFrom. + --> $DIR/cast_integer.rs:72:46 + | +LL | pub fn non_const_function(x: usize) -> u64 { x as u64 } + | ^^^^^^^^ + | + = help: Try T::from(_) or T::try_from(_) instead + +error: aborting due to 23 previous errors From 8dd730b9fa10894e663508eee83a891e82ce00ff Mon Sep 17 00:00:00 2001 From: Gernot Ohner Date: Mon, 11 Dec 2023 21:20:20 -0700 Subject: [PATCH 6/6] Update mod.rs for cast_integer --- clippy_lints/src/casts/mod.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs index 559c8c7d3dd6..bd2a71df8c55 100644 --- a/clippy_lints/src/casts/mod.rs +++ b/clippy_lints/src/casts/mod.rs @@ -692,21 +692,25 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does + /// Trigger on every integer cast using `as`, recommending From/TryFrom instead /// /// ### Why is this bad? + /// Because integer casts might introduce subtle unintended changes /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning + /// let u = 1usize; + /// let _ = u as u64; /// ``` /// Use instead: /// ```no_run - /// // example code which does not raise clippy warning + /// let u = 1usize; + /// let _ = u64::try_from(u); /// ``` #[clippy::version = "1.75.0"] pub CAST_INTEGER, pedantic, - "default lint description" + "casting integer types using `as` instead of From/TryFrom" } pub struct Casts {