Skip to content

Commit 2eb13d3

Browse files
borsbors
committed
Auto merge of #12056 - PartiallyTyped:12050, r=xFrednet
Fixes: #12050 - `identity_op` correctly suggests a deference for coerced references When `identity_op` identifies a `no_op`, provides a suggestion, it also checks the type of the type of the variable. If the variable is a reference that's been coerced into a value, e.g. ``` let x = &0i32; let _ = x + 0; ``` the suggestion will now use a derefence. This is done by identifying whether the variable is a reference to an integral value, and then whether it gets dereferenced. changelog: false positive: [`identity_op`]: corrected suggestion for reference coerced to value. fixes: #12050
2 parents 9eb2b22 + c2b3f5c commit 2eb13d3

File tree

4 files changed

+426
-124
lines changed

4 files changed

+426
-124
lines changed

‎clippy_lints/src/operators/identity_op.rs

Lines changed: 127 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -18,82 +18,118 @@ pub(crate) fn check<'tcx>(
1818
right: &'tcx Expr<'_>,
1919
) {
2020
if !is_allowed(cx, op, left, right) {
21-
match op {
22-
BinOpKind::Add | BinOpKind::BitOr | BinOpKind::BitXor => {
23-
check_op(
24-
cx,
25-
left,
26-
0,
27-
expr.span,
28-
peel_hir_expr_refs(right).0.span,
29-
needs_parenthesis(cx, expr, right),
30-
);
31-
check_op(
32-
cx,
33-
right,
34-
0,
35-
expr.span,
36-
peel_hir_expr_refs(left).0.span,
37-
Parens::Unneeded,
38-
);
39-
},
40-
BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => {
41-
check_op(
42-
cx,
43-
right,
44-
0,
45-
expr.span,
46-
peel_hir_expr_refs(left).0.span,
47-
Parens::Unneeded,
48-
);
49-
},
50-
BinOpKind::Mul => {
51-
check_op(
52-
cx,
53-
left,
54-
1,
55-
expr.span,
56-
peel_hir_expr_refs(right).0.span,
57-
needs_parenthesis(cx, expr, right),
58-
);
59-
check_op(
60-
cx,
61-
right,
62-
1,
63-
expr.span,
64-
peel_hir_expr_refs(left).0.span,
65-
Parens::Unneeded,
66-
);
67-
},
68-
BinOpKind::Div => check_op(
21+
return;
22+
}
23+
24+
// we need to know whether a ref is coerced to a value
25+
// if a ref is coerced, then the suggested lint must deref it
26+
// e.g. `let _: i32 = x+0` with `x: &i32` should be replaced with `let _: i32 = *x`.
27+
// we do this by checking the _kind_ of the type of the expression
28+
// if it's a ref, we then check whether it is erased, and that's it.
29+
let (peeled_left_span, left_is_coerced_to_value) = {
30+
let expr = peel_hir_expr_refs(left).0;
31+
let span = expr.span;
32+
let is_coerced = expr_is_erased_ref(cx, expr);
33+
(span, is_coerced)
34+
};
35+
36+
let (peeled_right_span, right_is_coerced_to_value) = {
37+
let expr = peel_hir_expr_refs(right).0;
38+
let span = expr.span;
39+
let is_coerced = expr_is_erased_ref(cx, expr);
40+
(span, is_coerced)
41+
};
42+
43+
match op {
44+
BinOpKind::Add | BinOpKind::BitOr | BinOpKind::BitXor => {
45+
check_op(
46+
cx,
47+
left,
48+
0,
49+
expr.span,
50+
peeled_right_span,
51+
needs_parenthesis(cx, expr, right),
52+
right_is_coerced_to_value,
53+
);
54+
check_op(
55+
cx,
56+
right,
57+
0,
58+
expr.span,
59+
peeled_left_span,
60+
Parens::Unneeded,
61+
left_is_coerced_to_value,
62+
);
63+
},
64+
BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => {
65+
check_op(
66+
cx,
67+
right,
68+
0,
69+
expr.span,
70+
peeled_left_span,
71+
Parens::Unneeded,
72+
left_is_coerced_to_value,
73+
);
74+
},
75+
BinOpKind::Mul => {
76+
check_op(
77+
cx,
78+
left,
79+
1,
80+
expr.span,
81+
peeled_right_span,
82+
needs_parenthesis(cx, expr, right),
83+
right_is_coerced_to_value,
84+
);
85+
check_op(
6986
cx,
7087
right,
7188
1,
7289
expr.span,
73-
peel_hir_expr_refs(left).0.span,
90+
peeled_left_span,
7491
Parens::Unneeded,
75-
),
76-
BinOpKind::BitAnd => {
77-
check_op(
78-
cx,
79-
left,
80-
-1,
81-
expr.span,
82-
peel_hir_expr_refs(right).0.span,
83-
needs_parenthesis(cx, expr, right),
84-
);
85-
check_op(
86-
cx,
87-
right,
88-
-1,
89-
expr.span,
90-
peel_hir_expr_refs(left).0.span,
91-
Parens::Unneeded,
92-
);
93-
},
94-
BinOpKind::Rem => check_remainder(cx, left, right, expr.span, left.span),
95-
_ => (),
96-
}
92+
left_is_coerced_to_value,
93+
);
94+
},
95+
BinOpKind::Div => check_op(
96+
cx,
97+
right,
98+
1,
99+
expr.span,
100+
peeled_left_span,
101+
Parens::Unneeded,
102+
left_is_coerced_to_value,
103+
),
104+
BinOpKind::BitAnd => {
105+
check_op(
106+
cx,
107+
left,
108+
-1,
109+
expr.span,
110+
peeled_right_span,
111+
needs_parenthesis(cx, expr, right),
112+
right_is_coerced_to_value,
113+
);
114+
check_op(
115+
cx,
116+
right,
117+
-1,
118+
expr.span,
119+
peeled_left_span,
120+
Parens::Unneeded,
121+
left_is_coerced_to_value,
122+
);
123+
},
124+
BinOpKind::Rem => check_remainder(cx, left, right, expr.span, left.span),
125+
_ => (),
126+
}
127+
}
128+
129+
fn expr_is_erased_ref(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
130+
match cx.typeck_results().expr_ty(expr).kind() {
131+
ty::Ref(r, ..) => r.is_erased(),
132+
_ => false,
97133
}
98134
}
99135

@@ -144,11 +180,11 @@ fn needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, right: &Expr<'_>)
144180
}
145181

146182
fn is_allowed(cx: &LateContext<'_>, cmp: BinOpKind, left: &Expr<'_>, right: &Expr<'_>) -> bool {
147-
// This lint applies to integers
148-
!cx.typeck_results().expr_ty(left).peel_refs().is_integral()
149-
|| !cx.typeck_results().expr_ty(right).peel_refs().is_integral()
183+
// This lint applies to integers and their references
184+
cx.typeck_results().expr_ty(left).peel_refs().is_integral()
185+
&& cx.typeck_results().expr_ty(right).peel_refs().is_integral()
150186
// `1 << 0` is a common pattern in bit manipulation code
151-
|| (cmp == BinOpKind::Shl
187+
&& !(cmp == BinOpKind::Shl
152188
&& constant_simple(cx, cx.typeck_results(), right) == Some(Constant::Int(0))
153189
&& constant_simple(cx, cx.typeck_results(), left) == Some(Constant::Int(1)))
154190
}
@@ -161,11 +197,11 @@ fn check_remainder(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>, span
161197
(Some(FullInt::U(lv)), Some(FullInt::U(rv))) => lv < rv,
162198
_ => return,
163199
} {
164-
span_ineffective_operation(cx, span, arg, Parens::Unneeded);
200+
span_ineffective_operation(cx, span, arg, Parens::Unneeded, false);
165201
}
166202
}
167203

168-
fn check_op(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span, parens: Parens) {
204+
fn check_op(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span, parens: Parens, is_erased: bool) {
169205
if let Some(Constant::Int(v)) = constant_simple(cx, cx.typeck_results(), e).map(Constant::peel_refs) {
170206
let check = match *cx.typeck_results().expr_ty(e).peel_refs().kind() {
171207
ty::Int(ity) => unsext(cx.tcx, -1_i128, ity),
@@ -178,18 +214,28 @@ fn check_op(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span, pa
178214
1 => v == 1,
179215
_ => unreachable!(),
180216
} {
181-
span_ineffective_operation(cx, span, arg, parens);
217+
span_ineffective_operation(cx, span, arg, parens, is_erased);
182218
}
183219
}
184220
}
185221

186-
fn span_ineffective_operation(cx: &LateContext<'_>, span: Span, arg: Span, parens: Parens) {
222+
fn span_ineffective_operation(
223+
cx: &LateContext<'_>,
224+
span: Span,
225+
arg: Span,
226+
parens: Parens,
227+
is_ref_coerced_to_val: bool,
228+
) {
187229
let mut applicability = Applicability::MachineApplicable;
188230
let expr_snippet = snippet_with_applicability(cx, arg, "..", &mut applicability);
189-
231+
let expr_snippet = if is_ref_coerced_to_val {
232+
format!("*{expr_snippet}")
233+
} else {
234+
expr_snippet.into_owned()
235+
};
190236
let suggestion = match parens {
191237
Parens::Needed => format!("({expr_snippet})"),
192-
Parens::Unneeded => expr_snippet.into_owned(),
238+
Parens::Unneeded => expr_snippet,
193239
};
194240

195241
span_lint_and_sugg(

0 commit comments

Comments
 (0)