From 7cb7e28c4de7d7fc8338039a6cdf8228e71a546d Mon Sep 17 00:00:00 2001 From: relaxcn Date: Wed, 11 Jun 2025 01:04:11 +0800 Subject: [PATCH 1/3] Fix FP of `identity_op` when encountering `Default::default()` --- clippy_lints/src/operators/identity_op.rs | 65 +++++++++++++++++++++-- tests/ui/identity_op.fixed | 17 ++++++ tests/ui/identity_op.rs | 17 ++++++ tests/ui/identity_op.stderr | 20 ++++++- 4 files changed, 114 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/operators/identity_op.rs b/clippy_lints/src/operators/identity_op.rs index e1fd09549a4b..6460bf6d6729 100644 --- a/clippy_lints/src/operators/identity_op.rs +++ b/clippy_lints/src/operators/identity_op.rs @@ -3,10 +3,10 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; use clippy_utils::{clip, peel_hir_expr_refs, unsext}; use rustc_errors::Applicability; -use rustc_hir::{BinOpKind, Expr, ExprKind, Node}; +use rustc_hir::{BinOpKind, Expr, ExprKind, HirId, Item, ItemKind, Node, QPath}; use rustc_lint::LateContext; use rustc_middle::ty; -use rustc_span::Span; +use rustc_span::{Span, sym}; use super::IDENTITY_OP; @@ -17,7 +17,7 @@ pub(crate) fn check<'tcx>( left: &'tcx Expr<'_>, right: &'tcx Expr<'_>, ) { - if !is_allowed(cx, op, left, right) { + if !is_allowed(cx, expr, op, left, right) { return; } @@ -165,7 +165,14 @@ fn needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, child: &Expr<'_>) Parens::Needed } -fn is_allowed(cx: &LateContext<'_>, cmp: BinOpKind, left: &Expr<'_>, right: &Expr<'_>) -> bool { +fn is_allowed(cx: &LateContext<'_>, expr: &Expr<'_>, cmp: BinOpKind, left: &Expr<'_>, right: &Expr<'_>) -> bool { + // Exclude case where the left or right side is a call to `Default::default()` + // and the expression is not a let binding's init expression and the let binding has a type + // annotation, or a function's return value. + if (is_default_call(cx, left) || is_default_call(cx, right)) && !is_expr_with_type_annotation(cx, expr.hir_id) { + return false; + } + // This lint applies to integers and their references cx.typeck_results().expr_ty(left).peel_refs().is_integral() && cx.typeck_results().expr_ty(right).peel_refs().is_integral() @@ -175,6 +182,20 @@ fn is_allowed(cx: &LateContext<'_>, cmp: BinOpKind, left: &Expr<'_>, right: &Exp && ConstEvalCtxt::new(cx).eval_simple(left) == Some(Constant::Int(1))) } +/// Check if the expression is a call to `Default::default()` +fn is_default_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + if let ExprKind::Call(func, []) = peel_hir_expr_refs(expr).0.kind + && let ExprKind::Path(qpath) = func.kind + // Detect and ignore ::default() because these calls do explicitly name the type. + && let QPath::Resolved(None, _path) = qpath + && let Some(def_id) = cx.qpath_res(&qpath, func.hir_id).opt_def_id() + && cx.tcx.is_diagnostic_item(sym::default_fn, def_id) + { + return true; + } + false +} + fn check_remainder(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>, span: Span, arg: Span) { let ecx = ConstEvalCtxt::new(cx); if match (ecx.eval_full_int(left), ecx.eval_full_int(right)) { @@ -234,3 +255,39 @@ fn span_ineffective_operation( applicability, ); } + +/// Check if the expression is a let binding's init expression and the let binding has a type +/// annotation. Also check if the expression is a function's return value. +fn is_expr_with_type_annotation(cx: &LateContext<'_>, hir_id: HirId) -> bool { + // Get the parent node of the expression + if let Some((_, parent)) = cx.tcx.hir_parent_iter(hir_id).next() { + match parent { + Node::LetStmt(local) => { + // Check if this expression is the init expression of the let binding + if let Some(init) = local.init + && init.hir_id == hir_id + { + // Check if the let binding has an explicit type annotation + return local.ty.is_some(); + } + }, + Node::Block(block) => { + // If the parent node is a block, we can make sure the expression is the last expression in the + // block. + return is_expr_with_type_annotation(cx, block.hir_id); + }, + Node::Expr(expr) => { + return is_expr_with_type_annotation(cx, expr.hir_id); + }, + Node::Item(Item { + kind: ItemKind::Fn { .. }, + .. + }) => { + // Every function has a return type, so we can return true. + return true; + }, + _ => {}, + } + } + false +} diff --git a/tests/ui/identity_op.fixed b/tests/ui/identity_op.fixed index a1b556029987..2cc4a5d0c23b 100644 --- a/tests/ui/identity_op.fixed +++ b/tests/ui/identity_op.fixed @@ -312,3 +312,20 @@ fn issue_13470() { let _: u64 = 1u64 + ((x as i32 + y as i32) as u64); //~^ identity_op } + +fn issue_14932() { + let _ = 0usize + &Default::default(); // no error + + 0usize + &Default::default(); // no error + + let _ = usize::default(); + //~^ identity_op + + let _n: usize = Default::default(); + //~^ identity_op +} + +fn issue_14932_2() -> usize { + Default::default() + //~^ identity_op +} diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index f603e1078e4e..da0597c7abe4 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -312,3 +312,20 @@ fn issue_13470() { let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64); //~^ identity_op } + +fn issue_14932() { + let _ = 0usize + &Default::default(); // no error + + 0usize + &Default::default(); // no error + + let _ = 0usize + &usize::default(); + //~^ identity_op + + let _n: usize = 0usize + &Default::default(); + //~^ identity_op +} + +fn issue_14932_2() -> usize { + 0usize + &Default::default() + //~^ identity_op +} diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index 8f9c2b603c49..9c774a313ffc 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -379,5 +379,23 @@ error: this operation has no effect LL | let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((x as i32 + y as i32) as u64)` -error: aborting due to 63 previous errors +error: this operation has no effect + --> tests/ui/identity_op.rs:321:13 + | +LL | let _ = 0usize + &usize::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `usize::default()` + +error: this operation has no effect + --> tests/ui/identity_op.rs:324:21 + | +LL | let _n: usize = 0usize + &Default::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Default::default()` + +error: this operation has no effect + --> tests/ui/identity_op.rs:329:5 + | +LL | 0usize + &Default::default() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Default::default()` + +error: aborting due to 66 previous errors From bf1a276db3c265f1265ad1820c831386f2d5a31b Mon Sep 17 00:00:00 2001 From: relaxcn Date: Sat, 14 Jun 2025 04:58:47 +0800 Subject: [PATCH 2/3] make it support more cases --- clippy_lints/src/operators/identity_op.rs | 101 +++++++++++----------- tests/ui/identity_op.fixed | 23 +++++ tests/ui/identity_op.rs | 23 +++++ tests/ui/identity_op.stderr | 16 +++- 4 files changed, 109 insertions(+), 54 deletions(-) diff --git a/clippy_lints/src/operators/identity_op.rs b/clippy_lints/src/operators/identity_op.rs index 6460bf6d6729..be570c4a716a 100644 --- a/clippy_lints/src/operators/identity_op.rs +++ b/clippy_lints/src/operators/identity_op.rs @@ -1,12 +1,13 @@ use clippy_utils::consts::{ConstEvalCtxt, Constant, FullInt}; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::{clip, peel_hir_expr_refs, unsext}; +use clippy_utils::{ExprUseNode, clip, expr_use_ctxt, peel_hir_expr_refs, unsext}; use rustc_errors::Applicability; -use rustc_hir::{BinOpKind, Expr, ExprKind, HirId, Item, ItemKind, Node, QPath}; +use rustc_hir::def::{DefKind, Res}; +use rustc_hir::{BinOpKind, Expr, ExprKind, Node, Path, QPath}; use rustc_lint::LateContext; use rustc_middle::ty; -use rustc_span::{Span, sym}; +use rustc_span::{Span, kw}; use super::IDENTITY_OP; @@ -165,11 +166,19 @@ fn needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, child: &Expr<'_>) Parens::Needed } -fn is_allowed(cx: &LateContext<'_>, expr: &Expr<'_>, cmp: BinOpKind, left: &Expr<'_>, right: &Expr<'_>) -> bool { - // Exclude case where the left or right side is a call to `Default::default()` - // and the expression is not a let binding's init expression and the let binding has a type - // annotation, or a function's return value. - if (is_default_call(cx, left) || is_default_call(cx, right)) && !is_expr_with_type_annotation(cx, expr.hir_id) { +fn is_allowed<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + cmp: BinOpKind, + left: &Expr<'tcx>, + right: &Expr<'tcx>, +) -> bool { + // Exclude case where the left or right side is associated function call returns a type which is + // `Self` that is not given explicitly, and the expression is not a let binding's init + // expression and the let binding has a type annotation, or a function's return value. + if (is_assoc_fn_without_type_instance(cx, left) || is_assoc_fn_without_type_instance(cx, right)) + && !is_expr_used_with_type_annotation(cx, expr) + { return false; } @@ -182,20 +191,6 @@ fn is_allowed(cx: &LateContext<'_>, expr: &Expr<'_>, cmp: BinOpKind, left: &Expr && ConstEvalCtxt::new(cx).eval_simple(left) == Some(Constant::Int(1))) } -/// Check if the expression is a call to `Default::default()` -fn is_default_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if let ExprKind::Call(func, []) = peel_hir_expr_refs(expr).0.kind - && let ExprKind::Path(qpath) = func.kind - // Detect and ignore ::default() because these calls do explicitly name the type. - && let QPath::Resolved(None, _path) = qpath - && let Some(def_id) = cx.qpath_res(&qpath, func.hir_id).opt_def_id() - && cx.tcx.is_diagnostic_item(sym::default_fn, def_id) - { - return true; - } - false -} - fn check_remainder(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>, span: Span, arg: Span) { let ecx = ConstEvalCtxt::new(cx); if match (ecx.eval_full_int(left), ecx.eval_full_int(right)) { @@ -256,38 +251,40 @@ fn span_ineffective_operation( ); } -/// Check if the expression is a let binding's init expression and the let binding has a type -/// annotation. Also check if the expression is a function's return value. -fn is_expr_with_type_annotation(cx: &LateContext<'_>, hir_id: HirId) -> bool { - // Get the parent node of the expression - if let Some((_, parent)) = cx.tcx.hir_parent_iter(hir_id).next() { - match parent { - Node::LetStmt(local) => { - // Check if this expression is the init expression of the let binding - if let Some(init) = local.init - && init.hir_id == hir_id - { - // Check if the let binding has an explicit type annotation - return local.ty.is_some(); - } - }, - Node::Block(block) => { - // If the parent node is a block, we can make sure the expression is the last expression in the - // block. - return is_expr_with_type_annotation(cx, block.hir_id); - }, - Node::Expr(expr) => { - return is_expr_with_type_annotation(cx, expr.hir_id); - }, - Node::Item(Item { - kind: ItemKind::Fn { .. }, +fn is_expr_used_with_type_annotation<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + match expr_use_ctxt(cx, expr).use_node(cx) { + ExprUseNode::LetStmt(letstmt) => letstmt.ty.is_some(), + ExprUseNode::Return(_) => true, + _ => false, + } +} + +/// Check if the expression is an associated function without a type instance. +/// Example: +/// ``` +/// Default::default() +/// // Or +/// trait Def { +/// fn def() -> Self; +/// } +/// Def::def() +/// ``` +fn is_assoc_fn_without_type_instance<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool { + if let ExprKind::Call(func, _) = peel_hir_expr_refs(expr).0.kind + && let ExprKind::Path(QPath::Resolved( + // If it's not None, don't need to go further. + None, + Path { + res: Res::Def(DefKind::AssocFn, def_id), .. - }) => { - // Every function has a return type, so we can return true. - return true; }, - _ => {}, - } + )) = func.kind + && let output_ty = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder().output() + && let ty::Param(ty::ParamTy { + name: kw::SelfUpper, .. + }) = output_ty.kind() + { + return true; } false } diff --git a/tests/ui/identity_op.fixed b/tests/ui/identity_op.fixed index 2cc4a5d0c23b..20cb35633264 100644 --- a/tests/ui/identity_op.fixed +++ b/tests/ui/identity_op.fixed @@ -325,7 +325,30 @@ fn issue_14932() { //~^ identity_op } +// Expr's type can be inferred by the function's return type fn issue_14932_2() -> usize { Default::default() //~^ identity_op } + +trait Def { + fn def() -> Self; +} + +impl Def for usize { + fn def() -> Self { + 0 + } +} + +fn issue_14932_3() { + let _ = 0usize + &Def::def(); // no error + + 0usize + &Def::def(); // no error + + let _ = usize::def(); + //~^ identity_op + + let _n: usize = Def::def(); + //~^ identity_op +} diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index da0597c7abe4..18e300ef8a9e 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -325,7 +325,30 @@ fn issue_14932() { //~^ identity_op } +// Expr's type can be inferred by the function's return type fn issue_14932_2() -> usize { 0usize + &Default::default() //~^ identity_op } + +trait Def { + fn def() -> Self; +} + +impl Def for usize { + fn def() -> Self { + 0 + } +} + +fn issue_14932_3() { + let _ = 0usize + &Def::def(); // no error + + 0usize + &Def::def(); // no error + + let _ = 0usize + &usize::def(); + //~^ identity_op + + let _n: usize = 0usize + &Def::def(); + //~^ identity_op +} diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index 9c774a313ffc..1fa3dd104789 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -392,10 +392,22 @@ LL | let _n: usize = 0usize + &Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Default::default()` error: this operation has no effect - --> tests/ui/identity_op.rs:329:5 + --> tests/ui/identity_op.rs:330:5 | LL | 0usize + &Default::default() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Default::default()` -error: aborting due to 66 previous errors +error: this operation has no effect + --> tests/ui/identity_op.rs:349:13 + | +LL | let _ = 0usize + &usize::def(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `usize::def()` + +error: this operation has no effect + --> tests/ui/identity_op.rs:352:21 + | +LL | let _n: usize = 0usize + &Def::def(); + | ^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Def::def()` + +error: aborting due to 68 previous errors From 2dfab750e7b75eb0fd2dd81ddcc3b818924f2df6 Mon Sep 17 00:00:00 2001 From: relaxcn Date: Sat, 14 Jun 2025 17:24:09 +0800 Subject: [PATCH 3/3] fix ci/cd error --- clippy_lints/src/operators/identity_op.rs | 14 +++++++++---- tests/ui/identity_op.fixed | 6 ++++++ tests/ui/identity_op.rs | 6 ++++++ tests/ui/identity_op.stderr | 24 +++++++++++++++++------ 4 files changed, 40 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/operators/identity_op.rs b/clippy_lints/src/operators/identity_op.rs index be570c4a716a..7e515e83cc9d 100644 --- a/clippy_lints/src/operators/identity_op.rs +++ b/clippy_lints/src/operators/identity_op.rs @@ -262,12 +262,18 @@ fn is_expr_used_with_type_annotation<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx E /// Check if the expression is an associated function without a type instance. /// Example: /// ``` -/// Default::default() -/// // Or /// trait Def { -/// fn def() -> Self; +/// fn def() -> Self; +/// } +/// impl Def for usize { +/// fn def() -> Self { +/// 0 +/// } +/// } +/// fn test() { +/// let _ = 0usize + &Default::default(); +/// let _ = 0usize + &Def::def(); /// } -/// Def::def() /// ``` fn is_assoc_fn_without_type_instance<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool { if let ExprKind::Call(func, _) = peel_hir_expr_refs(expr).0.kind diff --git a/tests/ui/identity_op.fixed b/tests/ui/identity_op.fixed index 20cb35633264..4e14e1a5e33f 100644 --- a/tests/ui/identity_op.fixed +++ b/tests/ui/identity_op.fixed @@ -318,6 +318,9 @@ fn issue_14932() { 0usize + &Default::default(); // no error + ::default(); + //~^ identity_op + let _ = usize::default(); //~^ identity_op @@ -346,6 +349,9 @@ fn issue_14932_3() { 0usize + &Def::def(); // no error + ::def(); + //~^ identity_op + let _ = usize::def(); //~^ identity_op diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index 18e300ef8a9e..ebbef5723ffb 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -318,6 +318,9 @@ fn issue_14932() { 0usize + &Default::default(); // no error + 0usize + &::default(); + //~^ identity_op + let _ = 0usize + &usize::default(); //~^ identity_op @@ -346,6 +349,9 @@ fn issue_14932_3() { 0usize + &Def::def(); // no error + 0usize + &::def(); + //~^ identity_op + let _ = 0usize + &usize::def(); //~^ identity_op diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index 1fa3dd104789..24fa5db08ce5 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -380,34 +380,46 @@ LL | let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((x as i32 + y as i32) as u64)` error: this operation has no effect - --> tests/ui/identity_op.rs:321:13 + --> tests/ui/identity_op.rs:321:5 + | +LL | 0usize + &::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `::default()` + +error: this operation has no effect + --> tests/ui/identity_op.rs:324:13 | LL | let _ = 0usize + &usize::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `usize::default()` error: this operation has no effect - --> tests/ui/identity_op.rs:324:21 + --> tests/ui/identity_op.rs:327:21 | LL | let _n: usize = 0usize + &Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Default::default()` error: this operation has no effect - --> tests/ui/identity_op.rs:330:5 + --> tests/ui/identity_op.rs:333:5 | LL | 0usize + &Default::default() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Default::default()` error: this operation has no effect - --> tests/ui/identity_op.rs:349:13 + --> tests/ui/identity_op.rs:352:5 + | +LL | 0usize + &::def(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `::def()` + +error: this operation has no effect + --> tests/ui/identity_op.rs:355:13 | LL | let _ = 0usize + &usize::def(); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `usize::def()` error: this operation has no effect - --> tests/ui/identity_op.rs:352:21 + --> tests/ui/identity_op.rs:358:21 | LL | let _n: usize = 0usize + &Def::def(); | ^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Def::def()` -error: aborting due to 68 previous errors +error: aborting due to 70 previous errors