From 7d2abed7ecfea8157e395aa56a8a342e6f8117eb Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Fri, 28 Mar 2025 16:07:41 +0100 Subject: [PATCH 1/4] Mark unsuffixed type literals are type uncertain by default However, if they appear within the context of a function argument, they will be marked as certain, as they might eventually be considered `i32` by default if the context is not more specific. Also, in the case of binary expressions, if one of the side is uncertain, then the certainty will come from the other side. --- clippy_utils/src/ty/type_certainty/mod.rs | 53 +++++++++++++++++------ 1 file changed, 40 insertions(+), 13 deletions(-) diff --git a/clippy_utils/src/ty/type_certainty/mod.rs b/clippy_utils/src/ty/type_certainty/mod.rs index 3398ff8af2f5..6d8ba65c500f 100644 --- a/clippy_utils/src/ty/type_certainty/mod.rs +++ b/clippy_utils/src/ty/type_certainty/mod.rs @@ -12,6 +12,7 @@ //! be considered a bug. use crate::def_path_res; +use rustc_ast::{LitFloatType, LitIntType, LitKind}; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::DefId; use rustc_hir::intravisit::{InferKind, Visitor, VisitorExt, walk_qpath, walk_ty}; @@ -24,22 +25,24 @@ mod certainty; use certainty::{Certainty, Meet, join, meet}; pub fn expr_type_is_certain(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - expr_type_certainty(cx, expr).is_certain() + expr_type_certainty(cx, expr, false).is_certain() } -fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty { +/// Determine the type certainty of `expr`. `in_arg` indicates that the expression happens within +/// the evaluation of a function or method call argument. +fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>, in_arg: bool) -> Certainty { let certainty = match &expr.kind { ExprKind::Unary(_, expr) | ExprKind::Field(expr, _) | ExprKind::Index(expr, _, _) - | ExprKind::AddrOf(_, _, expr) => expr_type_certainty(cx, expr), + | ExprKind::AddrOf(_, _, expr) => expr_type_certainty(cx, expr, in_arg), - ExprKind::Array(exprs) => join(exprs.iter().map(|expr| expr_type_certainty(cx, expr))), + ExprKind::Array(exprs) => join(exprs.iter().map(|expr| expr_type_certainty(cx, expr, in_arg))), ExprKind::Call(callee, args) => { - let lhs = expr_type_certainty(cx, callee); + let lhs = expr_type_certainty(cx, callee, false); let rhs = if type_is_inferable_from_arguments(cx, expr) { - meet(args.iter().map(|arg| expr_type_certainty(cx, arg))) + meet(args.iter().map(|arg| expr_type_certainty(cx, arg, true))) } else { Certainty::Uncertain }; @@ -47,7 +50,7 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty { }, ExprKind::MethodCall(method, receiver, args, _) => { - let mut receiver_type_certainty = expr_type_certainty(cx, receiver); + let mut receiver_type_certainty = expr_type_certainty(cx, receiver, false); // Even if `receiver_type_certainty` is `Certain(Some(..))`, the `Self` type in the method // identified by `type_dependent_def_id(..)` can differ. This can happen as a result of a `deref`, // for example. So update the `DefId` in `receiver_type_certainty` (if any). @@ -59,7 +62,8 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty { let lhs = path_segment_certainty(cx, receiver_type_certainty, method, false); let rhs = if type_is_inferable_from_arguments(cx, expr) { meet( - std::iter::once(receiver_type_certainty).chain(args.iter().map(|arg| expr_type_certainty(cx, arg))), + std::iter::once(receiver_type_certainty) + .chain(args.iter().map(|arg| expr_type_certainty(cx, arg, true))), ) } else { Certainty::Uncertain @@ -67,16 +71,39 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty { lhs.join(rhs) }, - ExprKind::Tup(exprs) => meet(exprs.iter().map(|expr| expr_type_certainty(cx, expr))), + ExprKind::Tup(exprs) => meet(exprs.iter().map(|expr| expr_type_certainty(cx, expr, in_arg))), - ExprKind::Binary(_, lhs, rhs) => expr_type_certainty(cx, lhs).meet(expr_type_certainty(cx, rhs)), + ExprKind::Binary(_, lhs, rhs) => { + // If one of the side of the expression is uncertain, the certainty will come from the other side, + // with no information on the type. + match ( + expr_type_certainty(cx, lhs, in_arg), + expr_type_certainty(cx, rhs, in_arg), + ) { + (Certainty::Uncertain, Certainty::Certain(_)) | (Certainty::Certain(_), Certainty::Uncertain) => { + Certainty::Certain(None) + }, + (l, r) => l.meet(r), + } + }, - ExprKind::Lit(_) => Certainty::Certain(None), + ExprKind::Lit(lit) => { + if !in_arg + && matches!( + lit.node, + LitKind::Int(_, LitIntType::Unsuffixed) | LitKind::Float(_, LitFloatType::Unsuffixed) + ) + { + Certainty::Uncertain + } else { + Certainty::Certain(None) + } + }, ExprKind::Cast(_, ty) => type_certainty(cx, ty), ExprKind::If(_, if_expr, Some(else_expr)) => { - expr_type_certainty(cx, if_expr).join(expr_type_certainty(cx, else_expr)) + expr_type_certainty(cx, if_expr, in_arg).join(expr_type_certainty(cx, else_expr, in_arg)) }, ExprKind::Path(qpath) => qpath_certainty(cx, qpath, false), @@ -248,7 +275,7 @@ fn path_segment_certainty( let lhs = local.ty.map_or(Certainty::Uncertain, |ty| type_certainty(cx, ty)); let rhs = local .init - .map_or(Certainty::Uncertain, |init| expr_type_certainty(cx, init)); + .map_or(Certainty::Uncertain, |init| expr_type_certainty(cx, init, false)); let certainty = lhs.join(rhs); if resolves_to_type { certainty From 1727363f5aecf9a04141cfb773ce81bc556b8756 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Fri, 28 Mar 2025 16:37:38 +0100 Subject: [PATCH 2/4] Do not remove cast if it would make the expression type uncertain --- clippy_lints/src/casts/unnecessary_cast.rs | 6 +- tests/ui/unnecessary_cast.fixed | 21 +++++ tests/ui/unnecessary_cast.rs | 21 +++++ tests/ui/unnecessary_cast.stderr | 96 ++++++++++++---------- 4 files changed, 101 insertions(+), 43 deletions(-) diff --git a/clippy_lints/src/casts/unnecessary_cast.rs b/clippy_lints/src/casts/unnecessary_cast.rs index 96c5da2cf73e..2c9307219d39 100644 --- a/clippy_lints/src/casts/unnecessary_cast.rs +++ b/clippy_lints/src/casts/unnecessary_cast.rs @@ -1,6 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::numeric_literal::NumericLiteral; use clippy_utils::source::{SpanRangeExt, snippet_opt}; +use clippy_utils::ty::expr_type_is_certain; use clippy_utils::visitors::{Visitable, for_each_expr_without_closures}; use clippy_utils::{get_parent_expr, is_hir_ty_cfg_dependant, is_ty_alias, path_to_local}; use rustc_ast::{LitFloatType, LitIntType, LitKind}; @@ -141,7 +142,10 @@ pub(super) fn check<'tcx>( } } - if cast_from.kind() == cast_to.kind() && !expr.span.in_external_macro(cx.sess().source_map()) { + if cast_from.kind() == cast_to.kind() + && !expr.span.in_external_macro(cx.sess().source_map()) + && expr_type_is_certain(cx, cast_expr) + { if let Some(id) = path_to_local(cast_expr) && !cx.tcx.hir().span(id).eq_ctxt(cast_expr.span) { diff --git a/tests/ui/unnecessary_cast.fixed b/tests/ui/unnecessary_cast.fixed index ba167e79a308..bf81bfe7ff93 100644 --- a/tests/ui/unnecessary_cast.fixed +++ b/tests/ui/unnecessary_cast.fixed @@ -6,6 +6,7 @@ clippy::no_effect, clippy::nonstandard_macro_braces, clippy::unnecessary_operation, + clippy::double_parens, nonstandard_style, unused )] @@ -269,4 +270,24 @@ mod fixable { { *x }.pow(2) //~^ unnecessary_cast } + + fn issue_14366(i: u32) { + // Do not remove the cast if it helps determining the type + let _ = ((1.0 / 8.0) as f64).powf(i as f64); + + // But remove useless casts anyway + let _ = ((1.0 / 8.0) as f64).powf(i as f64); + //~^ unnecessary_cast + } + + fn ambiguity() { + pub trait T {} + impl T for u32 {} + impl T for String {} + fn f(_: impl T) {} + + f((1 + 2) as u32); + f((1 + 2u32)); + //~^ unnecessary_cast + } } diff --git a/tests/ui/unnecessary_cast.rs b/tests/ui/unnecessary_cast.rs index 0f90a8b05965..920d3f264c09 100644 --- a/tests/ui/unnecessary_cast.rs +++ b/tests/ui/unnecessary_cast.rs @@ -6,6 +6,7 @@ clippy::no_effect, clippy::nonstandard_macro_braces, clippy::unnecessary_operation, + clippy::double_parens, nonstandard_style, unused )] @@ -269,4 +270,24 @@ mod fixable { (*x as usize).pow(2) //~^ unnecessary_cast } + + fn issue_14366(i: u32) { + // Do not remove the cast if it helps determining the type + let _ = ((1.0 / 8.0) as f64).powf(i as f64); + + // But remove useless casts anyway + let _ = (((1.0 / 8.0) as f64) as f64).powf(i as f64); + //~^ unnecessary_cast + } + + fn ambiguity() { + pub trait T {} + impl T for u32 {} + impl T for String {} + fn f(_: impl T) {} + + f((1 + 2) as u32); + f((1 + 2u32) as u32); + //~^ unnecessary_cast + } } diff --git a/tests/ui/unnecessary_cast.stderr b/tests/ui/unnecessary_cast.stderr index c83770c1a299..2ed1b5bb7e14 100644 --- a/tests/ui/unnecessary_cast.stderr +++ b/tests/ui/unnecessary_cast.stderr @@ -1,5 +1,5 @@ error: casting raw pointers to the same type and constness is unnecessary (`*const T` -> `*const T`) - --> tests/ui/unnecessary_cast.rs:19:5 + --> tests/ui/unnecessary_cast.rs:20:5 | LL | ptr as *const T | ^^^^^^^^^^^^^^^ help: try: `ptr` @@ -8,244 +8,256 @@ LL | ptr as *const T = help: to override `-D warnings` add `#[allow(clippy::unnecessary_cast)]` error: casting integer literal to `i32` is unnecessary - --> tests/ui/unnecessary_cast.rs:55:5 + --> tests/ui/unnecessary_cast.rs:56:5 | LL | 1i32 as i32; | ^^^^^^^^^^^ help: try: `1_i32` error: casting float literal to `f32` is unnecessary - --> tests/ui/unnecessary_cast.rs:57:5 + --> tests/ui/unnecessary_cast.rs:58:5 | LL | 1f32 as f32; | ^^^^^^^^^^^ help: try: `1_f32` error: casting to the same type is unnecessary (`bool` -> `bool`) - --> tests/ui/unnecessary_cast.rs:59:5 + --> tests/ui/unnecessary_cast.rs:60:5 | LL | false as bool; | ^^^^^^^^^^^^^ help: try: `false` error: casting integer literal to `i32` is unnecessary - --> tests/ui/unnecessary_cast.rs:63:5 + --> tests/ui/unnecessary_cast.rs:64:5 | LL | -1_i32 as i32; | ^^^^^^^^^^^^^ help: try: `-1_i32` error: casting integer literal to `i32` is unnecessary - --> tests/ui/unnecessary_cast.rs:65:5 + --> tests/ui/unnecessary_cast.rs:66:5 | LL | - 1_i32 as i32; | ^^^^^^^^^^^^^^ help: try: `- 1_i32` error: casting float literal to `f32` is unnecessary - --> tests/ui/unnecessary_cast.rs:67:5 + --> tests/ui/unnecessary_cast.rs:68:5 | LL | -1f32 as f32; | ^^^^^^^^^^^^ help: try: `-1_f32` error: casting integer literal to `i32` is unnecessary - --> tests/ui/unnecessary_cast.rs:69:5 + --> tests/ui/unnecessary_cast.rs:70:5 | LL | 1_i32 as i32; | ^^^^^^^^^^^^ help: try: `1_i32` error: casting float literal to `f32` is unnecessary - --> tests/ui/unnecessary_cast.rs:71:5 + --> tests/ui/unnecessary_cast.rs:72:5 | LL | 1_f32 as f32; | ^^^^^^^^^^^^ help: try: `1_f32` error: casting raw pointers to the same type and constness is unnecessary (`*const u8` -> `*const u8`) - --> tests/ui/unnecessary_cast.rs:74:22 + --> tests/ui/unnecessary_cast.rs:75:22 | LL | let _: *mut u8 = [1u8, 2].as_ptr() as *const u8 as *mut u8; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `[1u8, 2].as_ptr()` error: casting raw pointers to the same type and constness is unnecessary (`*const u8` -> `*const u8`) - --> tests/ui/unnecessary_cast.rs:77:5 + --> tests/ui/unnecessary_cast.rs:78:5 | LL | [1u8, 2].as_ptr() as *const u8; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `[1u8, 2].as_ptr()` error: casting raw pointers to the same type and constness is unnecessary (`*mut u8` -> `*mut u8`) - --> tests/ui/unnecessary_cast.rs:80:5 + --> tests/ui/unnecessary_cast.rs:81:5 | LL | [1u8, 2].as_mut_ptr() as *mut u8; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `[1u8, 2].as_mut_ptr()` error: casting raw pointers to the same type and constness is unnecessary (`*const u32` -> `*const u32`) - --> tests/ui/unnecessary_cast.rs:92:5 + --> tests/ui/unnecessary_cast.rs:93:5 | LL | owo::([1u32].as_ptr()) as *const u32; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `owo::([1u32].as_ptr())` error: casting raw pointers to the same type and constness is unnecessary (`*const u8` -> `*const u8`) - --> tests/ui/unnecessary_cast.rs:94:5 + --> tests/ui/unnecessary_cast.rs:95:5 | LL | uwu::([1u32].as_ptr()) as *const u8; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `uwu::([1u32].as_ptr())` error: casting raw pointers to the same type and constness is unnecessary (`*const u32` -> `*const u32`) - --> tests/ui/unnecessary_cast.rs:97:5 + --> tests/ui/unnecessary_cast.rs:98:5 | LL | uwu::([1u32].as_ptr()) as *const u32; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `uwu::([1u32].as_ptr())` error: casting to the same type is unnecessary (`u32` -> `u32`) - --> tests/ui/unnecessary_cast.rs:133:5 + --> tests/ui/unnecessary_cast.rs:134:5 | LL | aaa() as u32; | ^^^^^^^^^^^^ help: try: `aaa()` error: casting to the same type is unnecessary (`u32` -> `u32`) - --> tests/ui/unnecessary_cast.rs:136:5 + --> tests/ui/unnecessary_cast.rs:137:5 | LL | aaa() as u32; | ^^^^^^^^^^^^ help: try: `aaa()` error: casting integer literal to `f32` is unnecessary - --> tests/ui/unnecessary_cast.rs:173:9 + --> tests/ui/unnecessary_cast.rs:174:9 | LL | 100 as f32; | ^^^^^^^^^^ help: try: `100_f32` error: casting integer literal to `f64` is unnecessary - --> tests/ui/unnecessary_cast.rs:175:9 + --> tests/ui/unnecessary_cast.rs:176:9 | LL | 100 as f64; | ^^^^^^^^^^ help: try: `100_f64` error: casting integer literal to `f64` is unnecessary - --> tests/ui/unnecessary_cast.rs:177:9 + --> tests/ui/unnecessary_cast.rs:178:9 | LL | 100_i32 as f64; | ^^^^^^^^^^^^^^ help: try: `100_f64` error: casting integer literal to `f32` is unnecessary - --> tests/ui/unnecessary_cast.rs:179:17 + --> tests/ui/unnecessary_cast.rs:180:17 | LL | let _ = -100 as f32; | ^^^^^^^^^^^ help: try: `-100_f32` error: casting integer literal to `f64` is unnecessary - --> tests/ui/unnecessary_cast.rs:181:17 + --> tests/ui/unnecessary_cast.rs:182:17 | LL | let _ = -100 as f64; | ^^^^^^^^^^^ help: try: `-100_f64` error: casting integer literal to `f64` is unnecessary - --> tests/ui/unnecessary_cast.rs:183:17 + --> tests/ui/unnecessary_cast.rs:184:17 | LL | let _ = -100_i32 as f64; | ^^^^^^^^^^^^^^^ help: try: `-100_f64` error: casting float literal to `f32` is unnecessary - --> tests/ui/unnecessary_cast.rs:185:9 + --> tests/ui/unnecessary_cast.rs:186:9 | LL | 100. as f32; | ^^^^^^^^^^^ help: try: `100_f32` error: casting float literal to `f64` is unnecessary - --> tests/ui/unnecessary_cast.rs:187:9 + --> tests/ui/unnecessary_cast.rs:188:9 | LL | 100. as f64; | ^^^^^^^^^^^ help: try: `100_f64` error: casting integer literal to `u32` is unnecessary - --> tests/ui/unnecessary_cast.rs:200:9 + --> tests/ui/unnecessary_cast.rs:201:9 | LL | 1 as u32; | ^^^^^^^^ help: try: `1_u32` error: casting integer literal to `i32` is unnecessary - --> tests/ui/unnecessary_cast.rs:202:9 + --> tests/ui/unnecessary_cast.rs:203:9 | LL | 0x10 as i32; | ^^^^^^^^^^^ help: try: `0x10_i32` error: casting integer literal to `usize` is unnecessary - --> tests/ui/unnecessary_cast.rs:204:9 + --> tests/ui/unnecessary_cast.rs:205:9 | LL | 0b10 as usize; | ^^^^^^^^^^^^^ help: try: `0b10_usize` error: casting integer literal to `u16` is unnecessary - --> tests/ui/unnecessary_cast.rs:206:9 + --> tests/ui/unnecessary_cast.rs:207:9 | LL | 0o73 as u16; | ^^^^^^^^^^^ help: try: `0o73_u16` error: casting integer literal to `u32` is unnecessary - --> tests/ui/unnecessary_cast.rs:208:9 + --> tests/ui/unnecessary_cast.rs:209:9 | LL | 1_000_000_000 as u32; | ^^^^^^^^^^^^^^^^^^^^ help: try: `1_000_000_000_u32` error: casting float literal to `f64` is unnecessary - --> tests/ui/unnecessary_cast.rs:211:9 + --> tests/ui/unnecessary_cast.rs:212:9 | LL | 1.0 as f64; | ^^^^^^^^^^ help: try: `1.0_f64` error: casting float literal to `f32` is unnecessary - --> tests/ui/unnecessary_cast.rs:213:9 + --> tests/ui/unnecessary_cast.rs:214:9 | LL | 0.5 as f32; | ^^^^^^^^^^ help: try: `0.5_f32` error: casting integer literal to `i32` is unnecessary - --> tests/ui/unnecessary_cast.rs:218:17 + --> tests/ui/unnecessary_cast.rs:219:17 | LL | let _ = -1 as i32; | ^^^^^^^^^ help: try: `-1_i32` error: casting float literal to `f32` is unnecessary - --> tests/ui/unnecessary_cast.rs:220:17 + --> tests/ui/unnecessary_cast.rs:221:17 | LL | let _ = -1.0 as f32; | ^^^^^^^^^^^ help: try: `-1.0_f32` error: casting to the same type is unnecessary (`i32` -> `i32`) - --> tests/ui/unnecessary_cast.rs:227:18 + --> tests/ui/unnecessary_cast.rs:228:18 | LL | let _ = &(x as i32); | ^^^^^^^^^^ help: try: `{ x }` error: casting integer literal to `i32` is unnecessary - --> tests/ui/unnecessary_cast.rs:234:22 + --> tests/ui/unnecessary_cast.rs:235:22 | LL | let _: i32 = -(1) as i32; | ^^^^^^^^^^^ help: try: `-1_i32` error: casting integer literal to `i64` is unnecessary - --> tests/ui/unnecessary_cast.rs:237:22 + --> tests/ui/unnecessary_cast.rs:238:22 | LL | let _: i64 = -(1) as i64; | ^^^^^^^^^^^ help: try: `-1_i64` error: casting float literal to `f64` is unnecessary - --> tests/ui/unnecessary_cast.rs:245:22 + --> tests/ui/unnecessary_cast.rs:246:22 | LL | let _: f64 = (-8.0 as f64).exp(); | ^^^^^^^^^^^^^ help: try: `(-8.0_f64)` error: casting float literal to `f64` is unnecessary - --> tests/ui/unnecessary_cast.rs:248:23 + --> tests/ui/unnecessary_cast.rs:249:23 | LL | let _: f64 = -(8.0 as f64).exp(); // should suggest `-8.0_f64.exp()` here not to change code behavior | ^^^^^^^^^^^^ help: try: `8.0_f64` error: casting to the same type is unnecessary (`f32` -> `f32`) - --> tests/ui/unnecessary_cast.rs:258:20 + --> tests/ui/unnecessary_cast.rs:259:20 | LL | let _num = foo() as f32; | ^^^^^^^^^^^^ help: try: `foo()` error: casting to the same type is unnecessary (`usize` -> `usize`) - --> tests/ui/unnecessary_cast.rs:269:9 + --> tests/ui/unnecessary_cast.rs:270:9 | LL | (*x as usize).pow(2) | ^^^^^^^^^^^^^ help: try: `{ *x }` -error: aborting due to 41 previous errors +error: casting to the same type is unnecessary (`f64` -> `f64`) + --> tests/ui/unnecessary_cast.rs:279:17 + | +LL | let _ = (((1.0 / 8.0) as f64) as f64).powf(i as f64); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `((1.0 / 8.0) as f64)` + +error: casting to the same type is unnecessary (`u32` -> `u32`) + --> tests/ui/unnecessary_cast.rs:290:11 + | +LL | f((1 + 2u32) as u32); + | ^^^^^^^^^^^^^^^^^ help: try: `(1 + 2u32)` + +error: aborting due to 43 previous errors From 2a8d79eee708b586dca82f2ce754245c0d8c3db3 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Sat, 29 Mar 2025 00:27:53 +0100 Subject: [PATCH 3/4] Add `ExprKind::Block` to type certainty algorithm --- clippy_utils/src/ty/type_certainty/mod.rs | 4 ++++ tests/ui/unnecessary_cast.fixed | 6 ++++++ tests/ui/unnecessary_cast.rs | 6 ++++++ tests/ui/unnecessary_cast.stderr | 8 +++++++- 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/clippy_utils/src/ty/type_certainty/mod.rs b/clippy_utils/src/ty/type_certainty/mod.rs index 6d8ba65c500f..cb34a46c8a59 100644 --- a/clippy_utils/src/ty/type_certainty/mod.rs +++ b/clippy_utils/src/ty/type_certainty/mod.rs @@ -110,6 +110,10 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>, in_arg: bool) -> C ExprKind::Struct(qpath, _, _) => qpath_certainty(cx, qpath, true), + ExprKind::Block(block, _) => block + .expr + .map_or(Certainty::Certain(None), |expr| expr_type_certainty(cx, expr, false)), + _ => Certainty::Uncertain, }; diff --git a/tests/ui/unnecessary_cast.fixed b/tests/ui/unnecessary_cast.fixed index bf81bfe7ff93..37d6cba6be96 100644 --- a/tests/ui/unnecessary_cast.fixed +++ b/tests/ui/unnecessary_cast.fixed @@ -290,4 +290,10 @@ mod fixable { f((1 + 2u32)); //~^ unnecessary_cast } + + fn with_blocks(a: i64, b: i64, c: u64) { + let threshold = if c < 10 { a } else { b }; + let _ = threshold; + //~^ unnecessary_cast + } } diff --git a/tests/ui/unnecessary_cast.rs b/tests/ui/unnecessary_cast.rs index 920d3f264c09..c70f75b5777e 100644 --- a/tests/ui/unnecessary_cast.rs +++ b/tests/ui/unnecessary_cast.rs @@ -290,4 +290,10 @@ mod fixable { f((1 + 2u32) as u32); //~^ unnecessary_cast } + + fn with_blocks(a: i64, b: i64, c: u64) { + let threshold = if c < 10 { a } else { b }; + let _ = threshold as i64; + //~^ unnecessary_cast + } } diff --git a/tests/ui/unnecessary_cast.stderr b/tests/ui/unnecessary_cast.stderr index 2ed1b5bb7e14..dd4246f17a4e 100644 --- a/tests/ui/unnecessary_cast.stderr +++ b/tests/ui/unnecessary_cast.stderr @@ -259,5 +259,11 @@ error: casting to the same type is unnecessary (`u32` -> `u32`) LL | f((1 + 2u32) as u32); | ^^^^^^^^^^^^^^^^^ help: try: `(1 + 2u32)` -error: aborting due to 43 previous errors +error: casting to the same type is unnecessary (`i64` -> `i64`) + --> tests/ui/unnecessary_cast.rs:296:17 + | +LL | let _ = threshold as i64; + | ^^^^^^^^^^^^^^^^ help: try: `threshold` + +error: aborting due to 44 previous errors From 8ad3b0ff283709535dafb5bd19f04bf178bd2b73 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Sat, 29 Mar 2025 00:27:53 +0100 Subject: [PATCH 4/4] Properly resolve associated constants of primitive types Associated constants of primitive types are resolved as `Res::Err` in the HIR. To force a resolution, the primitive type must be recognized as certain. Since they are not ADT, they don't have a `DefId`. By allowing the `Certainty` type to embed a `TypeKind` which is either a `PrimTy` or a `DefId`, we allow primitives types to be resolved, and the associated constants as well. Also, in method calls, if the receiver is not certain, consider that the overall call is not either. --- clippy_utils/src/lib.rs | 1 + .../src/ty/type_certainty/certainty.rs | 62 +++++++++++++++---- clippy_utils/src/ty/type_certainty/mod.rs | 38 +++++++----- tests/ui/or_fun_call.fixed | 12 ++-- tests/ui/or_fun_call.rs | 4 ++ tests/ui/or_fun_call.stderr | 56 ++++++++++++----- tests/ui/unnecessary_cast.fixed | 13 ++++ tests/ui/unnecessary_cast.rs | 13 ++++ tests/ui/unnecessary_cast.stderr | 8 ++- 9 files changed, 158 insertions(+), 49 deletions(-) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index e8ec42e0662b..3b23c68908c0 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -10,6 +10,7 @@ #![feature(assert_matches)] #![feature(unwrap_infallible)] #![feature(array_windows)] +#![feature(try_trait_v2)] #![recursion_limit = "512"] #![allow( clippy::missing_errors_doc, diff --git a/clippy_utils/src/ty/type_certainty/certainty.rs b/clippy_utils/src/ty/type_certainty/certainty.rs index 0e69ffa2212d..b2f5d857b8f4 100644 --- a/clippy_utils/src/ty/type_certainty/certainty.rs +++ b/clippy_utils/src/ty/type_certainty/certainty.rs @@ -1,5 +1,12 @@ use rustc_hir::def_id::DefId; use std::fmt::Debug; +use std::ops::{ControlFlow, FromResidual, Try}; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum TypeKind { + PrimTy, + AdtDef(DefId), +} #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum Certainty { @@ -7,9 +14,9 @@ pub enum Certainty { Uncertain, /// The type can be determined purely from subexpressions. If the argument is `Some(..)`, the - /// specific `DefId` is known. Such arguments are needed to handle path segments whose `res` is - /// `Res::Err`. - Certain(Option), + /// specific primitive type or `DefId` is known. Such arguments are needed to handle path + /// segments whose `res` is `Res::Err`. + Certain(Option), /// The heuristic believes that more than one `DefId` applies to a type---this is a bug. Contradiction, @@ -23,7 +30,7 @@ pub trait TryJoin: Sized { fn try_join(self, other: Self) -> Option; } -impl Meet for Option { +impl Meet for Option { fn meet(self, other: Self) -> Self { match (self, other) { (None, _) | (_, None) => None, @@ -32,11 +39,11 @@ impl Meet for Option { } } -impl TryJoin for Option { +impl TryJoin for Option { fn try_join(self, other: Self) -> Option { match (self, other) { (Some(lhs), Some(rhs)) => (lhs == rhs).then_some(Some(lhs)), - (Some(def_id), _) | (_, Some(def_id)) => Some(Some(def_id)), + (Some(ty_kind), _) | (_, Some(ty_kind)) => Some(Some(ty_kind)), (None, None) => Some(None), } } @@ -79,11 +86,11 @@ impl Certainty { /// Join two `Certainty`s after clearing their `DefId`s. This method should be used when `self` /// or `other` do not necessarily refer to types, e.g., when they are aggregations of other /// `Certainty`s. - pub fn join_clearing_def_ids(self, other: Self) -> Self { - self.clear_def_id().join(other.clear_def_id()) + pub fn join_clearing_types(self, other: Self) -> Self { + self.clear_type().join(other.clear_type()) } - pub fn clear_def_id(self) -> Certainty { + pub fn clear_type(self) -> Certainty { if matches!(self, Certainty::Certain(_)) { Certainty::Certain(None) } else { @@ -91,9 +98,17 @@ impl Certainty { } } + pub fn with_prim_ty(self) -> Certainty { + if matches!(self, Certainty::Certain(_)) { + Certainty::Certain(Some(TypeKind::PrimTy)) + } else { + self + } + } + pub fn with_def_id(self, def_id: DefId) -> Certainty { if matches!(self, Certainty::Certain(_)) { - Certainty::Certain(Some(def_id)) + Certainty::Certain(Some(TypeKind::AdtDef(def_id))) } else { self } @@ -101,7 +116,7 @@ impl Certainty { pub fn to_def_id(self) -> Option { match self { - Certainty::Certain(inner) => inner, + Certainty::Certain(Some(TypeKind::AdtDef(def_id))) => Some(def_id), _ => None, } } @@ -120,3 +135,28 @@ pub fn meet(iter: impl Iterator) -> Certainty { pub fn join(iter: impl Iterator) -> Certainty { iter.fold(Certainty::Uncertain, Certainty::join) } + +pub struct NoCertainty(Certainty); + +impl FromResidual for Certainty { + fn from_residual(residual: NoCertainty) -> Self { + residual.0 + } +} + +impl Try for Certainty { + type Output = Certainty; + + type Residual = NoCertainty; + + fn from_output(output: Self::Output) -> Self { + output + } + + fn branch(self) -> ControlFlow { + match self { + Certainty::Certain(_) => ControlFlow::Continue(self), + _ => ControlFlow::Break(NoCertainty(self)), + } + } +} diff --git a/clippy_utils/src/ty/type_certainty/mod.rs b/clippy_utils/src/ty/type_certainty/mod.rs index cb34a46c8a59..f18ad0c2fefa 100644 --- a/clippy_utils/src/ty/type_certainty/mod.rs +++ b/clippy_utils/src/ty/type_certainty/mod.rs @@ -22,7 +22,7 @@ use rustc_middle::ty::{self, AdtDef, GenericArgKind, Ty}; use rustc_span::{Span, Symbol}; mod certainty; -use certainty::{Certainty, Meet, join, meet}; +use certainty::{Certainty, Meet, TypeKind, join, meet}; pub fn expr_type_is_certain(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { expr_type_certainty(cx, expr, false).is_certain() @@ -46,11 +46,12 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>, in_arg: bool) -> C } else { Certainty::Uncertain }; - lhs.join_clearing_def_ids(rhs) + lhs.join_clearing_types(rhs) }, ExprKind::MethodCall(method, receiver, args, _) => { - let mut receiver_type_certainty = expr_type_certainty(cx, receiver, false); + let mut receiver_type_certainty = expr_type_certainty(cx, receiver, false)?; + // Even if `receiver_type_certainty` is `Certain(Some(..))`, the `Self` type in the method // identified by `type_dependent_def_id(..)` can differ. This can happen as a result of a `deref`, // for example. So update the `DefId` in `receiver_type_certainty` (if any). @@ -66,7 +67,7 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>, in_arg: bool) -> C .chain(args.iter().map(|arg| expr_type_certainty(cx, arg, true))), ) } else { - Certainty::Uncertain + return Certainty::Uncertain; }; lhs.join(rhs) }, @@ -117,12 +118,13 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>, in_arg: bool) -> C _ => Certainty::Uncertain, }; - let expr_ty = cx.typeck_results().expr_ty(expr); - if let Some(def_id) = adt_def_id(expr_ty) { - certainty.with_def_id(def_id) - } else { - certainty.clear_def_id() - } + let result = match cx.typeck_results().expr_ty(expr).kind() { + ty::Adt(adt_def, _) => certainty.with_def_id(adt_def.did()), + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) => certainty.with_prim_ty(), + _ => certainty.clear_type(), + }; + + result } struct CertaintyVisitor<'cx, 'tcx> { @@ -209,7 +211,11 @@ fn qpath_certainty(cx: &LateContext<'_>, qpath: &QPath<'_>, resolves_to_type: bo .map_or(Certainty::Uncertain, |def_id| { let generics = cx.tcx.generics_of(def_id); if generics.is_empty() { - Certainty::Certain(if resolves_to_type { Some(def_id) } else { None }) + Certainty::Certain(if resolves_to_type { + Some(TypeKind::AdtDef(def_id)) + } else { + None + }) } else { Certainty::Uncertain } @@ -249,7 +255,7 @@ fn path_segment_certainty( .args .map_or(Certainty::Uncertain, |args| generic_args_certainty(cx, args)); // See the comment preceding `qpath_certainty`. `def_id` could refer to a type or a value. - let certainty = lhs.join_clearing_def_ids(rhs); + let certainty = lhs.join_clearing_types(rhs); if resolves_to_type { if let DefKind::TyAlias = cx.tcx.def_kind(def_id) { adt_def_id(cx.tcx.type_of(def_id).instantiate_identity()) @@ -265,9 +271,7 @@ fn path_segment_certainty( } }, - Res::PrimTy(_) | Res::SelfTyParam { .. } | Res::SelfTyAlias { .. } | Res::SelfCtor(_) => { - Certainty::Certain(None) - }, + Res::PrimTy(_) => Certainty::Certain(Some(TypeKind::PrimTy)), // `get_parent` because `hir_id` refers to a `Pat`, and we're interested in the node containing the `Pat`. Res::Local(hir_id) => match cx.tcx.parent_hir_node(hir_id) { @@ -284,13 +288,13 @@ fn path_segment_certainty( if resolves_to_type { certainty } else { - certainty.clear_def_id() + certainty.clear_type() } }, _ => Certainty::Uncertain, }, - _ => Certainty::Uncertain, + _ => Certainty::Certain(None), }; debug_assert!(resolves_to_type || certainty.to_def_id().is_none()); certainty diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed index 1794ac57fe5b..4181c28043c8 100644 --- a/tests/ui/or_fun_call.fixed +++ b/tests/ui/or_fun_call.fixed @@ -212,7 +212,8 @@ mod issue8239 { acc.push_str(&f); acc }) - .unwrap_or(String::new()); + .unwrap_or_default(); + //~^ unwrap_or_default } fn more_to_max_suggestion_highest_lines_1() { @@ -225,7 +226,8 @@ mod issue8239 { acc.push_str(&f); acc }) - .unwrap_or(String::new()); + .unwrap_or_default(); + //~^ unwrap_or_default } fn equal_to_max_suggestion_highest_lines() { @@ -237,7 +239,8 @@ mod issue8239 { acc.push_str(&f); acc }) - .unwrap_or(String::new()); + .unwrap_or_default(); + //~^ unwrap_or_default } fn less_than_max_suggestion_highest_lines() { @@ -248,7 +251,8 @@ mod issue8239 { acc.push_str(&f); acc }) - .unwrap_or(String::new()); + .unwrap_or_default(); + //~^ unwrap_or_default } } diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs index 256db343c057..62dc7bc5e795 100644 --- a/tests/ui/or_fun_call.rs +++ b/tests/ui/or_fun_call.rs @@ -213,6 +213,7 @@ mod issue8239 { acc }) .unwrap_or(String::new()); + //~^ unwrap_or_default } fn more_to_max_suggestion_highest_lines_1() { @@ -226,6 +227,7 @@ mod issue8239 { acc }) .unwrap_or(String::new()); + //~^ unwrap_or_default } fn equal_to_max_suggestion_highest_lines() { @@ -238,6 +240,7 @@ mod issue8239 { acc }) .unwrap_or(String::new()); + //~^ unwrap_or_default } fn less_than_max_suggestion_highest_lines() { @@ -249,6 +252,7 @@ mod issue8239 { acc }) .unwrap_or(String::new()); + //~^ unwrap_or_default } } diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr index 93c87b2f12cd..664350b8c651 100644 --- a/tests/ui/or_fun_call.stderr +++ b/tests/ui/or_fun_call.stderr @@ -142,74 +142,98 @@ error: function call inside of `unwrap_or` LL | None.unwrap_or( unsafe { ptr_to_ref(s) } ); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| unsafe { ptr_to_ref(s) })` +error: use of `unwrap_or` to construct default value + --> tests/ui/or_fun_call.rs:215:14 + | +LL | .unwrap_or(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` + +error: use of `unwrap_or` to construct default value + --> tests/ui/or_fun_call.rs:229:14 + | +LL | .unwrap_or(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` + +error: use of `unwrap_or` to construct default value + --> tests/ui/or_fun_call.rs:242:14 + | +LL | .unwrap_or(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` + +error: use of `unwrap_or` to construct default value + --> tests/ui/or_fun_call.rs:254:10 + | +LL | .unwrap_or(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` + error: function call inside of `map_or` - --> tests/ui/or_fun_call.rs:276:25 + --> tests/ui/or_fun_call.rs:280:25 | LL | let _ = Some(4).map_or(g(), |v| v); | ^^^^^^^^^^^^^^^^^^ help: try: `map_or_else(g, |v| v)` error: function call inside of `map_or` - --> tests/ui/or_fun_call.rs:278:25 + --> tests/ui/or_fun_call.rs:282:25 | LL | let _ = Some(4).map_or(g(), f); | ^^^^^^^^^^^^^^ help: try: `map_or_else(g, f)` error: use of `unwrap_or_else` to construct default value - --> tests/ui/or_fun_call.rs:310:18 + --> tests/ui/or_fun_call.rs:314:18 | LL | with_new.unwrap_or_else(Vec::new); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` error: use of `unwrap_or_else` to construct default value - --> tests/ui/or_fun_call.rs:314:28 + --> tests/ui/or_fun_call.rs:318:28 | LL | with_default_trait.unwrap_or_else(Default::default); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` error: use of `unwrap_or_else` to construct default value - --> tests/ui/or_fun_call.rs:318:27 + --> tests/ui/or_fun_call.rs:322:27 | LL | with_default_type.unwrap_or_else(u64::default); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` error: use of `unwrap_or_else` to construct default value - --> tests/ui/or_fun_call.rs:322:22 + --> tests/ui/or_fun_call.rs:326:22 | LL | real_default.unwrap_or_else(::default); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` error: use of `or_insert_with` to construct default value - --> tests/ui/or_fun_call.rs:326:23 + --> tests/ui/or_fun_call.rs:330:23 | LL | map.entry(42).or_insert_with(String::new); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `or_default()` error: use of `or_insert_with` to construct default value - --> tests/ui/or_fun_call.rs:330:25 + --> tests/ui/or_fun_call.rs:334:25 | LL | btree.entry(42).or_insert_with(String::new); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `or_default()` error: use of `unwrap_or_else` to construct default value - --> tests/ui/or_fun_call.rs:334:25 + --> tests/ui/or_fun_call.rs:338:25 | LL | let _ = stringy.unwrap_or_else(String::new); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` error: function call inside of `unwrap_or` - --> tests/ui/or_fun_call.rs:376:17 + --> tests/ui/or_fun_call.rs:380:17 | LL | let _ = opt.unwrap_or({ f() }); // suggest `.unwrap_or_else(f)` | ^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(f)` error: function call inside of `unwrap_or` - --> tests/ui/or_fun_call.rs:381:17 + --> tests/ui/or_fun_call.rs:385:17 | LL | let _ = opt.unwrap_or(f() + 1); // suggest `.unwrap_or_else(|| f() + 1)` | ^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| f() + 1)` error: function call inside of `unwrap_or` - --> tests/ui/or_fun_call.rs:386:17 + --> tests/ui/or_fun_call.rs:390:17 | LL | let _ = opt.unwrap_or({ | _________________^ @@ -229,22 +253,22 @@ LL ~ }); | error: function call inside of `map_or` - --> tests/ui/or_fun_call.rs:392:17 + --> tests/ui/or_fun_call.rs:396:17 | LL | let _ = opt.map_or(f() + 1, |v| v); // suggest `.map_or_else(|| f() + 1, |v| v)` | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `map_or_else(|| f() + 1, |v| v)` error: use of `unwrap_or` to construct default value - --> tests/ui/or_fun_call.rs:397:17 + --> tests/ui/or_fun_call.rs:401:17 | LL | let _ = opt.unwrap_or({ i32::default() }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` error: function call inside of `unwrap_or` - --> tests/ui/or_fun_call.rs:404:21 + --> tests/ui/or_fun_call.rs:408:21 | LL | let _ = opt_foo.unwrap_or(Foo { val: String::default() }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| Foo { val: String::default() })` -error: aborting due to 38 previous errors +error: aborting due to 42 previous errors diff --git a/tests/ui/unnecessary_cast.fixed b/tests/ui/unnecessary_cast.fixed index 37d6cba6be96..14dc53633fab 100644 --- a/tests/ui/unnecessary_cast.fixed +++ b/tests/ui/unnecessary_cast.fixed @@ -296,4 +296,17 @@ mod fixable { let _ = threshold; //~^ unnecessary_cast } + + fn with_prim_ty() { + let threshold = 20; + let threshold = if threshold == 0 { + i64::MAX + } else if threshold <= 60 { + 10 + } else { + 0 + }; + let _ = threshold; + //~^ unnecessary_cast + } } diff --git a/tests/ui/unnecessary_cast.rs b/tests/ui/unnecessary_cast.rs index c70f75b5777e..2fcd0f80d5dc 100644 --- a/tests/ui/unnecessary_cast.rs +++ b/tests/ui/unnecessary_cast.rs @@ -296,4 +296,17 @@ mod fixable { let _ = threshold as i64; //~^ unnecessary_cast } + + fn with_prim_ty() { + let threshold = 20; + let threshold = if threshold == 0 { + i64::MAX + } else if threshold <= 60 { + 10 + } else { + 0 + }; + let _ = threshold as i64; + //~^ unnecessary_cast + } } diff --git a/tests/ui/unnecessary_cast.stderr b/tests/ui/unnecessary_cast.stderr index dd4246f17a4e..b365a7647ca0 100644 --- a/tests/ui/unnecessary_cast.stderr +++ b/tests/ui/unnecessary_cast.stderr @@ -265,5 +265,11 @@ error: casting to the same type is unnecessary (`i64` -> `i64`) LL | let _ = threshold as i64; | ^^^^^^^^^^^^^^^^ help: try: `threshold` -error: aborting due to 44 previous errors +error: casting to the same type is unnecessary (`i64` -> `i64`) + --> tests/ui/unnecessary_cast.rs:309:17 + | +LL | let _ = threshold as i64; + | ^^^^^^^^^^^^^^^^ help: try: `threshold` + +error: aborting due to 45 previous errors