Skip to content

Commit 4d67a1c

Browse files
Fix FP of identity_op when encountering Default::default() (#15028)
Fixes: #14932 changelog: Fix false positive of [`identity_op`] when encountering `Default::default()`.
2 parents 97815d0 + 2dfab75 commit 4d67a1c

File tree

4 files changed

+200
-6
lines changed

4 files changed

+200
-6
lines changed

clippy_lints/src/operators/identity_op.rs

Lines changed: 65 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
use clippy_utils::consts::{ConstEvalCtxt, Constant, FullInt};
22
use clippy_utils::diagnostics::span_lint_and_sugg;
33
use clippy_utils::source::snippet_with_applicability;
4-
use clippy_utils::{clip, peel_hir_expr_refs, unsext};
4+
use clippy_utils::{ExprUseNode, clip, expr_use_ctxt, peel_hir_expr_refs, unsext};
55
use rustc_errors::Applicability;
6-
use rustc_hir::{BinOpKind, Expr, ExprKind, Node};
6+
use rustc_hir::def::{DefKind, Res};
7+
use rustc_hir::{BinOpKind, Expr, ExprKind, Node, Path, QPath};
78
use rustc_lint::LateContext;
89
use rustc_middle::ty;
9-
use rustc_span::Span;
10+
use rustc_span::{Span, kw};
1011

1112
use super::IDENTITY_OP;
1213

@@ -17,7 +18,7 @@ pub(crate) fn check<'tcx>(
1718
left: &'tcx Expr<'_>,
1819
right: &'tcx Expr<'_>,
1920
) {
20-
if !is_allowed(cx, op, left, right) {
21+
if !is_allowed(cx, expr, op, left, right) {
2122
return;
2223
}
2324

@@ -165,7 +166,22 @@ fn needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, child: &Expr<'_>)
165166
Parens::Needed
166167
}
167168

168-
fn is_allowed(cx: &LateContext<'_>, cmp: BinOpKind, left: &Expr<'_>, right: &Expr<'_>) -> bool {
169+
fn is_allowed<'tcx>(
170+
cx: &LateContext<'tcx>,
171+
expr: &'tcx Expr<'tcx>,
172+
cmp: BinOpKind,
173+
left: &Expr<'tcx>,
174+
right: &Expr<'tcx>,
175+
) -> bool {
176+
// Exclude case where the left or right side is associated function call returns a type which is
177+
// `Self` that is not given explicitly, and the expression is not a let binding's init
178+
// expression and the let binding has a type annotation, or a function's return value.
179+
if (is_assoc_fn_without_type_instance(cx, left) || is_assoc_fn_without_type_instance(cx, right))
180+
&& !is_expr_used_with_type_annotation(cx, expr)
181+
{
182+
return false;
183+
}
184+
169185
// This lint applies to integers and their references
170186
cx.typeck_results().expr_ty(left).peel_refs().is_integral()
171187
&& cx.typeck_results().expr_ty(right).peel_refs().is_integral()
@@ -234,3 +250,47 @@ fn span_ineffective_operation(
234250
applicability,
235251
);
236252
}
253+
254+
fn is_expr_used_with_type_annotation<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
255+
match expr_use_ctxt(cx, expr).use_node(cx) {
256+
ExprUseNode::LetStmt(letstmt) => letstmt.ty.is_some(),
257+
ExprUseNode::Return(_) => true,
258+
_ => false,
259+
}
260+
}
261+
262+
/// Check if the expression is an associated function without a type instance.
263+
/// Example:
264+
/// ```
265+
/// trait Def {
266+
/// fn def() -> Self;
267+
/// }
268+
/// impl Def for usize {
269+
/// fn def() -> Self {
270+
/// 0
271+
/// }
272+
/// }
273+
/// fn test() {
274+
/// let _ = 0usize + &Default::default();
275+
/// let _ = 0usize + &Def::def();
276+
/// }
277+
/// ```
278+
fn is_assoc_fn_without_type_instance<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
279+
if let ExprKind::Call(func, _) = peel_hir_expr_refs(expr).0.kind
280+
&& let ExprKind::Path(QPath::Resolved(
281+
// If it's not None, don't need to go further.
282+
None,
283+
Path {
284+
res: Res::Def(DefKind::AssocFn, def_id),
285+
..
286+
},
287+
)) = func.kind
288+
&& let output_ty = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder().output()
289+
&& let ty::Param(ty::ParamTy {
290+
name: kw::SelfUpper, ..
291+
}) = output_ty.kind()
292+
{
293+
return true;
294+
}
295+
false
296+
}

tests/ui/identity_op.fixed

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,3 +312,49 @@ fn issue_13470() {
312312
let _: u64 = 1u64 + ((x as i32 + y as i32) as u64);
313313
//~^ identity_op
314314
}
315+
316+
fn issue_14932() {
317+
let _ = 0usize + &Default::default(); // no error
318+
319+
0usize + &Default::default(); // no error
320+
321+
<usize as Default>::default();
322+
//~^ identity_op
323+
324+
let _ = usize::default();
325+
//~^ identity_op
326+
327+
let _n: usize = Default::default();
328+
//~^ identity_op
329+
}
330+
331+
// Expr's type can be inferred by the function's return type
332+
fn issue_14932_2() -> usize {
333+
Default::default()
334+
//~^ identity_op
335+
}
336+
337+
trait Def {
338+
fn def() -> Self;
339+
}
340+
341+
impl Def for usize {
342+
fn def() -> Self {
343+
0
344+
}
345+
}
346+
347+
fn issue_14932_3() {
348+
let _ = 0usize + &Def::def(); // no error
349+
350+
0usize + &Def::def(); // no error
351+
352+
<usize as Def>::def();
353+
//~^ identity_op
354+
355+
let _ = usize::def();
356+
//~^ identity_op
357+
358+
let _n: usize = Def::def();
359+
//~^ identity_op
360+
}

tests/ui/identity_op.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,3 +312,49 @@ fn issue_13470() {
312312
let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64);
313313
//~^ identity_op
314314
}
315+
316+
fn issue_14932() {
317+
let _ = 0usize + &Default::default(); // no error
318+
319+
0usize + &Default::default(); // no error
320+
321+
0usize + &<usize as Default>::default();
322+
//~^ identity_op
323+
324+
let _ = 0usize + &usize::default();
325+
//~^ identity_op
326+
327+
let _n: usize = 0usize + &Default::default();
328+
//~^ identity_op
329+
}
330+
331+
// Expr's type can be inferred by the function's return type
332+
fn issue_14932_2() -> usize {
333+
0usize + &Default::default()
334+
//~^ identity_op
335+
}
336+
337+
trait Def {
338+
fn def() -> Self;
339+
}
340+
341+
impl Def for usize {
342+
fn def() -> Self {
343+
0
344+
}
345+
}
346+
347+
fn issue_14932_3() {
348+
let _ = 0usize + &Def::def(); // no error
349+
350+
0usize + &Def::def(); // no error
351+
352+
0usize + &<usize as Def>::def();
353+
//~^ identity_op
354+
355+
let _ = 0usize + &usize::def();
356+
//~^ identity_op
357+
358+
let _n: usize = 0usize + &Def::def();
359+
//~^ identity_op
360+
}

tests/ui/identity_op.stderr

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,5 +379,47 @@ error: this operation has no effect
379379
LL | let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64);
380380
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((x as i32 + y as i32) as u64)`
381381

382-
error: aborting due to 63 previous errors
382+
error: this operation has no effect
383+
--> tests/ui/identity_op.rs:321:5
384+
|
385+
LL | 0usize + &<usize as Default>::default();
386+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `<usize as Default>::default()`
387+
388+
error: this operation has no effect
389+
--> tests/ui/identity_op.rs:324:13
390+
|
391+
LL | let _ = 0usize + &usize::default();
392+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `usize::default()`
393+
394+
error: this operation has no effect
395+
--> tests/ui/identity_op.rs:327:21
396+
|
397+
LL | let _n: usize = 0usize + &Default::default();
398+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Default::default()`
399+
400+
error: this operation has no effect
401+
--> tests/ui/identity_op.rs:333:5
402+
|
403+
LL | 0usize + &Default::default()
404+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Default::default()`
405+
406+
error: this operation has no effect
407+
--> tests/ui/identity_op.rs:352:5
408+
|
409+
LL | 0usize + &<usize as Def>::def();
410+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `<usize as Def>::def()`
411+
412+
error: this operation has no effect
413+
--> tests/ui/identity_op.rs:355:13
414+
|
415+
LL | let _ = 0usize + &usize::def();
416+
| ^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `usize::def()`
417+
418+
error: this operation has no effect
419+
--> tests/ui/identity_op.rs:358:21
420+
|
421+
LL | let _n: usize = 0usize + &Def::def();
422+
| ^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `Def::def()`
423+
424+
error: aborting due to 70 previous errors
383425

0 commit comments

Comments
 (0)