Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint for bool to integer casts in cast_lossless #7948

Merged
merged 4 commits into from
Nov 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clippy_lints/src/bool_assert_comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
41 changes: 32 additions & 9 deletions clippy_lints/src/casts/cast_lossless.rs
Original file line number Diff line number Diff line change
@@ -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;
}

Expand All @@ -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;
Expand All @@ -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))
},
Expand Down
16 changes: 10 additions & 6 deletions clippy_lints/src/casts/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
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);
}
}

Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
}
2 changes: 1 addition & 1 deletion clippy_utils/src/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
42 changes: 42 additions & 0 deletions tests/ui/cast_lossless_bool.fixed
Original file line number Diff line number Diff line change
@@ -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
}
}
}
42 changes: 42 additions & 0 deletions tests/ui/cast_lossless_bool.rs
Original file line number Diff line number Diff line change
@@ -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
}
}
}
82 changes: 82 additions & 0 deletions tests/ui/cast_lossless_bool.stderr
Original file line number Diff line number Diff line change
@@ -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

6 changes: 6 additions & 0 deletions tests/ui/min_rust_version_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -156,6 +161,7 @@ fn main() {
map_unwrap_or();
missing_const_for_fn();
unnest_or_patterns();
int_from_bool();
}

mod just_under_msrv {
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/min_rust_version_attr.stderr
Original file line number Diff line number Diff line change
@@ -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, ") {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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, ") {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down