Skip to content

Lint casts to u128 in cast_lossless #13146

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

Merged
merged 1 commit into from
Jul 24, 2024
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
111 changes: 47 additions & 64 deletions clippy_lints/src/casts/cast_lossless.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::in_constant;
use clippy_utils::source::{snippet_opt, snippet_with_applicability};
use clippy_utils::source::snippet_opt;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_isize_or_usize;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, QPath, TyKind};
use rustc_hir::{Expr, QPath, TyKind};
use rustc_lint::LateContext;
use rustc_middle::ty::{self, FloatTy, Ty, UintTy};
use rustc_middle::ty::{self, FloatTy, Ty};
use rustc_span::hygiene;

use super::{utils, CAST_LOSSLESS};

pub(super) fn check(
cx: &LateContext<'_>,
expr: &Expr<'_>,
cast_op: &Expr<'_>,
cast_from_expr: &Expr<'_>,
cast_from: Ty<'_>,
cast_to: Ty<'_>,
cast_to_hir: &rustc_hir::Ty<'_>,
Expand All @@ -23,64 +25,54 @@ pub(super) fn check(
return;
}

// The suggestion is to use a function call, so if the original expression
// has parens on the outside, they are no longer needed.
let mut app = Applicability::MachineApplicable;
let opt = snippet_opt(cx, cast_op.span.source_callsite());
let sugg = opt.as_ref().map_or_else(
|| {
app = Applicability::HasPlaceholders;
".."
},
|snip| {
if should_strip_parens(cast_op, snip) {
&snip[1..snip.len() - 1]
} else {
snip.as_str()
}
},
);

// Display the type alias instead of the aliased type. Fixes #11285
//
// FIXME: Once `lazy_type_alias` is stabilized(?) we should use `rustc_middle` types instead,
// this will allow us to display the right type with `cast_from` as well.
let cast_to_fmt = if let TyKind::Path(QPath::Resolved(None, path)) = cast_to_hir.kind
// It's a bit annoying but the turbofish is optional for types. A type in an `as` cast
// shouldn't have these if they're primitives, which are the only things we deal with.
//
// This could be removed for performance if this check is determined to have a pretty major
// effect.
&& path.segments.iter().all(|segment| segment.args.is_none())
{
snippet_with_applicability(cx, cast_to_hir.span, "..", &mut app)
} else {
cast_to.to_string().into()
};

let message = if cast_from.is_bool() {
format!("casting `{cast_from}` to `{cast_to_fmt}` is more cleanly stated with `{cast_to_fmt}::from(_)`")
} else {
format!("casting `{cast_from}` to `{cast_to_fmt}` may become silently lossy if you later change the type")
};

span_lint_and_sugg(
span_lint_and_then(
cx,
CAST_LOSSLESS,
expr.span,
message,
"try",
format!("{cast_to_fmt}::from({sugg})"),
app,
format!("casts from `{cast_from}` to `{cast_to}` can be expressed infallibly using `From`"),
|diag| {
diag.help("an `as` cast can become silently lossy if the types change in the future");
let mut applicability = Applicability::MachineApplicable;
let from_sugg = Sugg::hir_with_context(cx, cast_from_expr, expr.span.ctxt(), "<from>", &mut applicability);
let Some(ty) = snippet_opt(cx, hygiene::walk_chain(cast_to_hir.span, expr.span.ctxt())) else {
return;
};
match cast_to_hir.kind {
TyKind::Infer => {
diag.span_suggestion_verbose(
expr.span,
"use `Into::into` instead",
format!("{}.into()", from_sugg.maybe_par()),
applicability,
);
},
// Don't suggest `A<_>::B::From(x)` or `macro!()::from(x)`
kind if matches!(kind, TyKind::Path(QPath::Resolved(_, path)) if path.segments.iter().any(|s| s.args.is_some()))
|| !cast_to_hir.span.eq_ctxt(expr.span) =>
{
diag.span_suggestion_verbose(
expr.span,
format!("use `<{ty}>::from` instead"),
format!("<{ty}>::from({from_sugg})"),
applicability,
);
},
_ => {
diag.span_suggestion_verbose(
expr.span,
format!("use `{ty}::from` instead"),
format!("{ty}::from({from_sugg})"),
applicability,
);
},
}
},
);
}

fn should_lint(cx: &LateContext<'_>, expr: &Expr<'_>, cast_from: Ty<'_>, cast_to: Ty<'_>, msrv: &Msrv) -> bool {
// Do not suggest using From in consts/statics until it is valid to do so (see #2267).
//
// If destination is u128, do not lint because source type cannot be larger
// If source is bool, still lint due to the lint message differing (refers to style)
if in_constant(cx, expr.hir_id) || (!cast_from.is_bool() && matches!(cast_to.kind(), ty::Uint(UintTy::U128))) {
if in_constant(cx, expr.hir_id) {
return false;
}

Expand Down Expand Up @@ -110,12 +102,3 @@ fn should_lint(cx: &LateContext<'_>, expr: &Expr<'_>, cast_from: Ty<'_>, cast_to
},
}
}

fn should_strip_parens(cast_expr: &Expr<'_>, snip: &str) -> bool {
if let ExprKind::Binary(_, _, _) = cast_expr.kind {
if snip.starts_with('(') && snip.ends_with(')') {
return true;
}
}
false
}
36 changes: 18 additions & 18 deletions clippy_lints/src/casts/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -763,45 +763,45 @@ impl<'tcx> LateLintPass<'tcx> for Casts {
return;
}

if let ExprKind::Cast(cast_expr, cast_to_hir) = expr.kind {
if let ExprKind::Cast(cast_from_expr, cast_to_hir) = expr.kind {
if is_hir_ty_cfg_dependant(cx, cast_to_hir) {
return;
}
let (cast_from, cast_to) = (
cx.typeck_results().expr_ty(cast_expr),
cx.typeck_results().expr_ty(cast_from_expr),
cx.typeck_results().expr_ty(expr),
);

if !expr.span.from_expansion() && unnecessary_cast::check(cx, expr, cast_expr, cast_from, cast_to) {
if !expr.span.from_expansion() && unnecessary_cast::check(cx, expr, cast_from_expr, cast_from, cast_to) {
return;
}
cast_slice_from_raw_parts::check(cx, expr, cast_expr, cast_to, &self.msrv);
ptr_cast_constness::check(cx, expr, cast_expr, cast_from, cast_to, &self.msrv);
as_ptr_cast_mut::check(cx, expr, cast_expr, cast_to);
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);
zero_ptr::check(cx, expr, cast_expr, cast_to_hir);
cast_slice_from_raw_parts::check(cx, expr, cast_from_expr, cast_to, &self.msrv);
ptr_cast_constness::check(cx, expr, cast_from_expr, cast_from, cast_to, &self.msrv);
as_ptr_cast_mut::check(cx, expr, cast_from_expr, cast_to);
fn_to_numeric_cast_any::check(cx, expr, cast_from_expr, cast_from, cast_to);
fn_to_numeric_cast::check(cx, expr, cast_from_expr, cast_from, cast_to);
fn_to_numeric_cast_with_truncation::check(cx, expr, cast_from_expr, cast_from, cast_to);
zero_ptr::check(cx, expr, cast_from_expr, cast_to_hir);

if cast_to.is_numeric() {
cast_possible_truncation::check(cx, expr, cast_expr, cast_from, cast_to, cast_to_hir.span);
cast_possible_truncation::check(cx, expr, cast_from_expr, cast_from, cast_to, cast_to_hir.span);
if cast_from.is_numeric() {
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_nan_to_int::check(cx, expr, cast_expr, cast_from, cast_to);
cast_sign_loss::check(cx, expr, cast_from_expr, cast_from, cast_to);
cast_abs_to_unsigned::check(cx, expr, cast_from_expr, cast_from, cast_to, &self.msrv);
cast_nan_to_int::check(cx, expr, cast_from_expr, cast_from, cast_to);
}
cast_lossless::check(cx, expr, cast_expr, cast_from, cast_to, cast_to_hir, &self.msrv);
cast_enum_constructor::check(cx, expr, cast_expr, cast_from);
cast_lossless::check(cx, expr, cast_from_expr, cast_from, cast_to, cast_to_hir, &self.msrv);
cast_enum_constructor::check(cx, expr, cast_from_expr, cast_from);
}

as_underscore::check(cx, expr, cast_to_hir);

if self.msrv.meets(msrvs::PTR_FROM_REF) {
ref_as_ptr::check(cx, expr, cast_expr, cast_to_hir);
ref_as_ptr::check(cx, expr, cast_from_expr, cast_to_hir);
} else if self.msrv.meets(msrvs::BORROW_AS_PTR) {
borrow_as_ptr::check(cx, expr, cast_expr, cast_to_hir);
borrow_as_ptr::check(cx, expr, cast_from_expr, cast_to_hir);
}
}

Expand Down
9 changes: 6 additions & 3 deletions clippy_utils/src/sugg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ impl Display for Sugg<'_> {
impl<'a> Sugg<'a> {
/// Prepare a suggestion from an expression.
pub fn hir_opt(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<Self> {
let get_snippet = |span| snippet(cx, span, "");
let ctxt = expr.span.ctxt();
let get_snippet = |span| snippet_with_context(cx, span, ctxt, "", &mut Applicability::Unspecified).0;
snippet_opt(cx, expr.span).map(|_| Self::hir_from_snippet(expr, get_snippet))
}

Expand Down Expand Up @@ -100,7 +101,9 @@ impl<'a> Sugg<'a> {
applicability: &mut Applicability,
) -> Self {
if expr.span.ctxt() == ctxt {
Self::hir_from_snippet(expr, |span| snippet(cx, span, default))
Self::hir_from_snippet(expr, |span| {
snippet_with_context(cx, span, ctxt, default, applicability).0
})
} else {
let (snip, _) = snippet_with_context(cx, expr.span, ctxt, default, applicability);
Sugg::NonParen(snip)
Expand All @@ -109,7 +112,7 @@ impl<'a> Sugg<'a> {

/// Generate a suggestion for an expression with the given snippet. This is used by the `hir_*`
/// function variants of `Sugg`, since these use different snippet functions.
fn hir_from_snippet(expr: &hir::Expr<'_>, get_snippet: impl Fn(Span) -> Cow<'a, str>) -> Self {
fn hir_from_snippet(expr: &hir::Expr<'_>, mut get_snippet: impl FnMut(Span) -> Cow<'a, str>) -> Self {
if let Some(range) = higher::Range::hir(expr) {
let op = match range.limits {
ast::RangeLimits::HalfOpen => AssocOp::DotDot,
Expand Down
Loading