Skip to content

Commit 5ae57c5

Browse files
oli-obkoli-obk
authored
Merge pull request #1671 from Manishearth/eq_op
Fix op_ref false positives
2 parents 0432f1e + 7b61116 commit 5ae57c5

13 files changed

+408
-159
lines changed

‎clippy_lints/src/eq_op.rs

Lines changed: 79 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use rustc::hir::*;
22
use rustc::lint::*;
3-
use utils::{SpanlessEq, span_lint, span_lint_and_then, multispan_sugg, snippet};
3+
use utils::{SpanlessEq, span_lint, span_lint_and_then, multispan_sugg, snippet, implements_trait};
44
use utils::sugg::Sugg;
55

66
/// **What it does:** Checks for equal operands to comparison, logical and
@@ -60,53 +60,88 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EqOp {
6060
e.span,
6161
&format!("equal expressions as operands to `{}`", op.node.as_str()));
6262
} else {
63-
match (&left.node, &right.node) {
64-
(&ExprAddrOf(_, ref l), &ExprAddrOf(_, ref r)) => {
65-
span_lint_and_then(cx,
66-
OP_REF,
67-
e.span,
68-
"taken reference of both operands, which is done automatically by the operator anyway",
69-
|db| {
70-
let lsnip = snippet(cx, l.span, "...").to_string();
71-
let rsnip = snippet(cx, r.span, "...").to_string();
72-
multispan_sugg(db,
73-
"use the values directly".to_string(),
74-
vec![(left.span, lsnip),
75-
(right.span, rsnip)]);
63+
let trait_id = match op.node {
64+
BiAdd => cx.tcx.lang_items.add_trait(),
65+
BiSub => cx.tcx.lang_items.sub_trait(),
66+
BiMul => cx.tcx.lang_items.mul_trait(),
67+
BiDiv => cx.tcx.lang_items.div_trait(),
68+
BiRem => cx.tcx.lang_items.rem_trait(),
69+
BiAnd |
70+
BiOr => None,
71+
BiBitXor => cx.tcx.lang_items.bitxor_trait(),
72+
BiBitAnd => cx.tcx.lang_items.bitand_trait(),
73+
BiBitOr => cx.tcx.lang_items.bitor_trait(),
74+
BiShl => cx.tcx.lang_items.shl_trait(),
75+
BiShr => cx.tcx.lang_items.shr_trait(),
76+
BiNe |
77+
BiEq => cx.tcx.lang_items.eq_trait(),
78+
BiLt |
79+
BiLe |
80+
BiGe |
81+
BiGt => cx.tcx.lang_items.ord_trait(),
82+
};
83+
if let Some(trait_id) = trait_id {
84+
#[allow(match_same_arms)]
85+
match (&left.node, &right.node) {
86+
// do not suggest to dereference literals
87+
(&ExprLit(..), _) |
88+
(_, &ExprLit(..)) => {},
89+
// &foo == &bar
90+
(&ExprAddrOf(_, ref l), &ExprAddrOf(_, ref r)) => {
91+
if implements_trait(cx, cx.tables.expr_ty(l), trait_id, &[cx.tables.expr_ty(r)], None) {
92+
span_lint_and_then(cx,
93+
OP_REF,
94+
e.span,
95+
"taken reference of both operands, which is done automatically by the operator anyway",
96+
|db| {
97+
let lsnip = snippet(cx, l.span, "...").to_string();
98+
let rsnip = snippet(cx, r.span, "...").to_string();
99+
multispan_sugg(db,
100+
"use the values directly".to_string(),
101+
vec![(left.span, lsnip),
102+
(right.span, rsnip)]);
103+
}
104+
)
76105
}
77-
)
78-
}
79-
(&ExprAddrOf(_, ref l), _) => {
80-
span_lint_and_then(cx,
81-
OP_REF,
82-
e.span,
83-
"taken reference of left operand",
84-
|db| {
85-
let lsnip = snippet(cx, l.span, "...").to_string();
86-
let rsnip = Sugg::hir(cx, right, "...").deref().to_string();
87-
multispan_sugg(db,
88-
"dereference the right operand instead".to_string(),
89-
vec![(left.span, lsnip),
90-
(right.span, rsnip)]);
106+
}
107+
// &foo == bar
108+
(&ExprAddrOf(_, ref l), _) => {
109+
if implements_trait(cx, cx.tables.expr_ty(l), trait_id, &[cx.tables.expr_ty(right)], None) {
110+
span_lint_and_then(cx,
111+
OP_REF,
112+
e.span,
113+
"taken reference of left operand",
114+
|db| {
115+
let lsnip = snippet(cx, l.span, "...").to_string();
116+
let rsnip = Sugg::hir(cx, right, "...").deref().to_string();
117+
multispan_sugg(db,
118+
"dereference the right operand instead".to_string(),
119+
vec![(left.span, lsnip),
120+
(right.span, rsnip)]);
121+
}
122+
)
91123
}
92-
)
93-
}
94-
(_, &ExprAddrOf(_, ref r)) => {
95-
span_lint_and_then(cx,
96-
OP_REF,
97-
e.span,
98-
"taken reference of right operand",
99-
|db| {
100-
let lsnip = Sugg::hir(cx, left, "...").deref().to_string();
101-
let rsnip = snippet(cx, r.span, "...").to_string();
102-
multispan_sugg(db,
103-
"dereference the left operand instead".to_string(),
104-
vec![(left.span, lsnip),
105-
(right.span, rsnip)]);
124+
}
125+
// foo == &bar
126+
(_, &ExprAddrOf(_, ref r)) => {
127+
if implements_trait(cx, cx.tables.expr_ty(left), trait_id, &[cx.tables.expr_ty(r)], None) {
128+
span_lint_and_then(cx,
129+
OP_REF,
130+
e.span,
131+
"taken reference of right operand",
132+
|db| {
133+
let lsnip = Sugg::hir(cx, left, "...").deref().to_string();
134+
let rsnip = snippet(cx, r.span, "...").to_string();
135+
multispan_sugg(db,
136+
"dereference the left operand instead".to_string(),
137+
vec![(left.span, lsnip),
138+
(right.span, rsnip)]);
139+
}
140+
)
106141
}
107-
)
142+
}
143+
_ => {}
108144
}
109-
_ => {}
110145
}
111146
}
112147
}

‎clippy_lints/src/format.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ pub fn get_argument_fmtstr_parts<'a, 'b>(cx: &LateContext<'a, 'b>, expr: &'a Exp
9999

100100
/// Checks if the expressions matches
101101
/// ```rust
102-
/// { static __STATIC_FMTSTR: … = &["…", "…", …]; __STATIC_FMTSTR }
102+
/// { static __STATIC_FMTSTR: s = &["a", "b", c]; __STATIC_FMTSTR }
103103
/// ```
104104
fn check_static_str(cx: &LateContext, expr: &Expr) -> bool {
105105
if let Some(expr) = get_argument_fmtstr_parts(cx, expr) {
@@ -110,10 +110,10 @@ fn check_static_str(cx: &LateContext, expr: &Expr) -> bool {
110110
}
111111

112112
/// Checks if the expressions matches
113-
/// ```rust
113+
/// ```rust,ignore
114114
/// &match (&42,) {
115115
/// (__arg0,) => [::std::fmt::ArgumentV1::new(__arg0, ::std::fmt::Display::fmt)],
116-
/// })
116+
/// }
117117
/// ```
118118
fn check_arg_is_display(cx: &LateContext, expr: &Expr) -> bool {
119119
if_let_chain! {[

‎tests/ui/collapsible_if.stderr

Lines changed: 53 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,7 @@ error: this if statement can be collapsed
66
9 | |
77
10 | |
88
11 | |
9-
12 | | if y == "world" {
10-
13 | | println!("Hello world!");
9+
... |
1110
14 | | }
1211
15 | | }
1312
| |_____^ ...ending here
@@ -30,8 +29,7 @@ error: this if statement can be collapsed
3029
18 | |
3130
19 | |
3231
20 | |
33-
21 | | if y == "world" || y == "hello" {
34-
22 | | println!("Hello world!");
32+
... |
3533
23 | | }
3634
24 | | }
3735
| |_____^ ...ending here
@@ -49,8 +47,7 @@ error: this if statement can be collapsed
4947
27 | |
5048
28 | |
5149
29 | |
52-
30 | | if y == "world" || y == "hello" {
53-
31 | | println!("Hello world!");
50+
... |
5451
32 | | }
5552
33 | | }
5653
| |_____^ ...ending here
@@ -68,8 +65,7 @@ error: this if statement can be collapsed
6865
36 | |
6966
37 | |
7067
38 | |
71-
39 | | if y == "world" && y == "hello" {
72-
40 | | println!("Hello world!");
68+
... |
7369
41 | | }
7470
42 | | }
7571
| |_____^ ...ending here
@@ -87,8 +83,7 @@ error: this if statement can be collapsed
8783
45 | |
8884
46 | |
8985
47 | |
90-
48 | | if y == "world" && y == "hello" {
91-
49 | | println!("Hello world!");
86+
... |
9287
50 | | }
9388
51 | | }
9489
| |_____^ ...ending here
@@ -106,8 +101,7 @@ error: this if statement can be collapsed
106101
54 | |
107102
55 | |
108103
56 | |
109-
57 | | if 'a' != 'A' {
110-
58 | | println!("world!")
104+
... |
111105
59 | | }
112106
60 | | }
113107
| |_____^ ...ending here
@@ -125,8 +119,7 @@ error: this `else { if .. }` block can be collapsed
125119
66 | |
126120
67 | |
127121
68 | |
128-
69 | | if y == "world" {
129-
70 | | println!("world!")
122+
... |
130123
71 | | }
131124
72 | | }
132125
| |_____^ ...ending here
@@ -144,8 +137,7 @@ error: this `else { if .. }` block can be collapsed
144137
77 | |
145138
78 | |
146139
79 | |
147-
80 | | if let Some(42) = Some(42) {
148-
81 | | println!("world!")
140+
... |
149141
82 | | }
150142
83 | | }
151143
| |_____^ ...ending here
@@ -158,8 +150,15 @@ help: try
158150
error: this `else { if .. }` block can be collapsed
159151
--> $DIR/collapsible_if.rs:87:12
160152
|
161-
87 | } else {
162-
| ^
153+
87 | } else {
154+
| ____________^ starting here...
155+
88 | |
156+
89 | |
157+
90 | |
158+
... |
159+
96 | | }
160+
97 | | }
161+
| |_____^ ...ending here
163162
|
164163
help: try
165164
| } else if y == "world" {
@@ -172,8 +171,15 @@ help: try
172171
error: this `else { if .. }` block can be collapsed
173172
--> $DIR/collapsible_if.rs:101:12
174173
|
175-
101 | } else {
176-
| ^
174+
101 | } else {
175+
| ____________^ starting here...
176+
102 | |
177+
103 | |
178+
104 | |
179+
... |
180+
110 | | }
181+
111 | | }
182+
| |_____^ ...ending here
177183
|
178184
help: try
179185
| } else if let Some(42) = Some(42) {
@@ -186,8 +192,15 @@ help: try
186192
error: this `else { if .. }` block can be collapsed
187193
--> $DIR/collapsible_if.rs:115:12
188194
|
189-
115 | } else {
190-
| ^
195+
115 | } else {
196+
| ____________^ starting here...
197+
116 | |
198+
117 | |
199+
118 | |
200+
... |
201+
124 | | }
202+
125 | | }
203+
| |_____^ ...ending here
191204
|
192205
help: try
193206
| } else if let Some(42) = Some(42) {
@@ -200,8 +213,15 @@ help: try
200213
error: this `else { if .. }` block can be collapsed
201214
--> $DIR/collapsible_if.rs:129:12
202215
|
203-
129 | } else {
204-
| ^
216+
129 | } else {
217+
| ____________^ starting here...
218+
130 | |
219+
131 | |
220+
132 | |
221+
... |
222+
138 | | }
223+
139 | | }
224+
| |_____^ ...ending here
205225
|
206226
help: try
207227
| } else if x == "hello" {
@@ -214,8 +234,15 @@ help: try
214234
error: this `else { if .. }` block can be collapsed
215235
--> $DIR/collapsible_if.rs:143:12
216236
|
217-
143 | } else {
218-
| ^
237+
143 | } else {
238+
| ____________^ starting here...
239+
144 | |
240+
145 | |
241+
146 | |
242+
... |
243+
152 | | }
244+
153 | | }
245+
| |_____^ ...ending here
219246
|
220247
help: try
221248
| } else if let Some(42) = Some(42) {

0 commit comments

Comments
 (0)