]>
Commit | Line | Data |
---|---|---|
cdc7bbd5 XL |
1 | use clippy_utils::diagnostics::span_lint_and_then; |
2 | use clippy_utils::source::snippet_opt; | |
3 | use clippy_utils::ty::implements_trait; | |
4 | use clippy_utils::{eq_expr_value, get_trait_def_id, trait_ref_of_method}; | |
5 | use clippy_utils::{higher, paths, sugg}; | |
f20569fa XL |
6 | use if_chain::if_chain; |
7 | use rustc_errors::Applicability; | |
8 | use rustc_hir as hir; | |
9 | use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; | |
10 | use rustc_lint::{LateContext, LateLintPass}; | |
11 | use rustc_middle::hir::map::Map; | |
12 | use rustc_session::{declare_lint_pass, declare_tool_lint}; | |
13 | ||
14 | declare_clippy_lint! { | |
15 | /// **What it does:** Checks for `a = a op b` or `a = b commutative_op a` | |
16 | /// patterns. | |
17 | /// | |
18 | /// **Why is this bad?** These can be written as the shorter `a op= b`. | |
19 | /// | |
20 | /// **Known problems:** While forbidden by the spec, `OpAssign` traits may have | |
21 | /// implementations that differ from the regular `Op` impl. | |
22 | /// | |
23 | /// **Example:** | |
24 | /// ```rust | |
25 | /// let mut a = 5; | |
26 | /// let b = 0; | |
27 | /// // ... | |
28 | /// // Bad | |
29 | /// a = a + b; | |
30 | /// | |
31 | /// // Good | |
32 | /// a += b; | |
33 | /// ``` | |
34 | pub ASSIGN_OP_PATTERN, | |
35 | style, | |
36 | "assigning the result of an operation on a variable to that same variable" | |
37 | } | |
38 | ||
39 | declare_clippy_lint! { | |
40 | /// **What it does:** Checks for `a op= a op b` or `a op= b op a` patterns. | |
41 | /// | |
42 | /// **Why is this bad?** Most likely these are bugs where one meant to write `a | |
43 | /// op= b`. | |
44 | /// | |
45 | /// **Known problems:** Clippy cannot know for sure if `a op= a op b` should have | |
46 | /// been `a = a op a op b` or `a = a op b`/`a op= b`. Therefore, it suggests both. | |
47 | /// If `a op= a op b` is really the correct behaviour it should be | |
48 | /// written as `a = a op a op b` as it's less confusing. | |
49 | /// | |
50 | /// **Example:** | |
51 | /// ```rust | |
52 | /// let mut a = 5; | |
53 | /// let b = 2; | |
54 | /// // ... | |
55 | /// a += a + b; | |
56 | /// ``` | |
57 | pub MISREFACTORED_ASSIGN_OP, | |
58 | complexity, | |
59 | "having a variable on both sides of an assign op" | |
60 | } | |
61 | ||
62 | declare_lint_pass!(AssignOps => [ASSIGN_OP_PATTERN, MISREFACTORED_ASSIGN_OP]); | |
63 | ||
64 | impl<'tcx> LateLintPass<'tcx> for AssignOps { | |
65 | #[allow(clippy::too_many_lines)] | |
66 | fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { | |
67 | match &expr.kind { | |
68 | hir::ExprKind::AssignOp(op, lhs, rhs) => { | |
69 | if let hir::ExprKind::Binary(binop, l, r) = &rhs.kind { | |
70 | if op.node != binop.node { | |
71 | return; | |
72 | } | |
73 | // lhs op= l op r | |
74 | if eq_expr_value(cx, lhs, l) { | |
75 | lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, r); | |
76 | } | |
77 | // lhs op= l commutative_op r | |
78 | if is_commutative(op.node) && eq_expr_value(cx, lhs, r) { | |
79 | lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, l); | |
80 | } | |
81 | } | |
82 | }, | |
83 | hir::ExprKind::Assign(assignee, e, _) => { | |
84 | if let hir::ExprKind::Binary(op, l, r) = &e.kind { | |
85 | let lint = |assignee: &hir::Expr<'_>, rhs: &hir::Expr<'_>| { | |
86 | let ty = cx.typeck_results().expr_ty(assignee); | |
87 | let rty = cx.typeck_results().expr_ty(rhs); | |
88 | macro_rules! ops { | |
89 | ($op:expr, | |
90 | $cx:expr, | |
91 | $ty:expr, | |
92 | $rty:expr, | |
93 | $($trait_name:ident),+) => { | |
94 | match $op { | |
95 | $(hir::BinOpKind::$trait_name => { | |
cdc7bbd5 | 96 | let [krate, module] = paths::OPS_MODULE; |
f20569fa XL |
97 | let path: [&str; 3] = [krate, module, concat!(stringify!($trait_name), "Assign")]; |
98 | let trait_id = if let Some(trait_id) = get_trait_def_id($cx, &path) { | |
99 | trait_id | |
100 | } else { | |
101 | return; // useless if the trait doesn't exist | |
102 | }; | |
103 | // check that we are not inside an `impl AssignOp` of this exact operation | |
104 | let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id); | |
105 | if_chain! { | |
106 | if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn); | |
107 | if trait_ref.path.res.def_id() == trait_id; | |
108 | then { return; } | |
109 | } | |
110 | implements_trait($cx, $ty, trait_id, &[$rty]) | |
111 | },)* | |
112 | _ => false, | |
113 | } | |
114 | } | |
115 | } | |
116 | if ops!( | |
117 | op.node, | |
118 | cx, | |
119 | ty, | |
120 | rty.into(), | |
121 | Add, | |
122 | Sub, | |
123 | Mul, | |
124 | Div, | |
125 | Rem, | |
126 | And, | |
127 | Or, | |
128 | BitAnd, | |
129 | BitOr, | |
130 | BitXor, | |
131 | Shr, | |
132 | Shl | |
133 | ) { | |
134 | span_lint_and_then( | |
135 | cx, | |
136 | ASSIGN_OP_PATTERN, | |
137 | expr.span, | |
138 | "manual implementation of an assign operation", | |
139 | |diag| { | |
140 | if let (Some(snip_a), Some(snip_r)) = | |
141 | (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span)) | |
142 | { | |
143 | diag.span_suggestion( | |
144 | expr.span, | |
145 | "replace it with", | |
146 | format!("{} {}= {}", snip_a, op.node.as_str(), snip_r), | |
147 | Applicability::MachineApplicable, | |
148 | ); | |
149 | } | |
150 | }, | |
151 | ); | |
152 | } | |
153 | }; | |
154 | ||
155 | let mut visitor = ExprVisitor { | |
156 | assignee, | |
157 | counter: 0, | |
158 | cx, | |
159 | }; | |
160 | ||
161 | walk_expr(&mut visitor, e); | |
162 | ||
163 | if visitor.counter == 1 { | |
164 | // a = a op b | |
165 | if eq_expr_value(cx, assignee, l) { | |
166 | lint(assignee, r); | |
167 | } | |
168 | // a = b commutative_op a | |
169 | // Limited to primitive type as these ops are know to be commutative | |
170 | if eq_expr_value(cx, assignee, r) && cx.typeck_results().expr_ty(assignee).is_primitive_ty() { | |
171 | match op.node { | |
172 | hir::BinOpKind::Add | |
173 | | hir::BinOpKind::Mul | |
174 | | hir::BinOpKind::And | |
175 | | hir::BinOpKind::Or | |
176 | | hir::BinOpKind::BitXor | |
177 | | hir::BinOpKind::BitAnd | |
178 | | hir::BinOpKind::BitOr => { | |
179 | lint(assignee, l); | |
180 | }, | |
181 | _ => {}, | |
182 | } | |
183 | } | |
184 | } | |
185 | } | |
186 | }, | |
187 | _ => {}, | |
188 | } | |
189 | } | |
190 | } | |
191 | ||
192 | fn lint_misrefactored_assign_op( | |
193 | cx: &LateContext<'_>, | |
194 | expr: &hir::Expr<'_>, | |
195 | op: hir::BinOp, | |
196 | rhs: &hir::Expr<'_>, | |
197 | assignee: &hir::Expr<'_>, | |
198 | rhs_other: &hir::Expr<'_>, | |
199 | ) { | |
200 | span_lint_and_then( | |
201 | cx, | |
202 | MISREFACTORED_ASSIGN_OP, | |
203 | expr.span, | |
204 | "variable appears on both sides of an assignment operation", | |
205 | |diag| { | |
206 | if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span)) { | |
207 | let a = &sugg::Sugg::hir(cx, assignee, ".."); | |
208 | let r = &sugg::Sugg::hir(cx, rhs, ".."); | |
209 | let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r)); | |
210 | diag.span_suggestion( | |
211 | expr.span, | |
212 | &format!( | |
213 | "did you mean `{} = {} {} {}` or `{}`? Consider replacing it with", | |
214 | snip_a, | |
215 | snip_a, | |
216 | op.node.as_str(), | |
217 | snip_r, | |
218 | long | |
219 | ), | |
220 | format!("{} {}= {}", snip_a, op.node.as_str(), snip_r), | |
221 | Applicability::MaybeIncorrect, | |
222 | ); | |
223 | diag.span_suggestion( | |
224 | expr.span, | |
225 | "or", | |
226 | long, | |
227 | Applicability::MaybeIncorrect, // snippet | |
228 | ); | |
229 | } | |
230 | }, | |
231 | ); | |
232 | } | |
233 | ||
234 | #[must_use] | |
235 | fn is_commutative(op: hir::BinOpKind) -> bool { | |
236 | use rustc_hir::BinOpKind::{ | |
237 | Add, And, BitAnd, BitOr, BitXor, Div, Eq, Ge, Gt, Le, Lt, Mul, Ne, Or, Rem, Shl, Shr, Sub, | |
238 | }; | |
239 | match op { | |
240 | Add | Mul | And | Or | BitXor | BitAnd | BitOr | Eq | Ne => true, | |
241 | Sub | Div | Rem | Shl | Shr | Lt | Le | Ge | Gt => false, | |
242 | } | |
243 | } | |
244 | ||
245 | struct ExprVisitor<'a, 'tcx> { | |
246 | assignee: &'a hir::Expr<'a>, | |
247 | counter: u8, | |
248 | cx: &'a LateContext<'tcx>, | |
249 | } | |
250 | ||
251 | impl<'a, 'tcx> Visitor<'tcx> for ExprVisitor<'a, 'tcx> { | |
252 | type Map = Map<'tcx>; | |
253 | ||
254 | fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) { | |
255 | if eq_expr_value(self.cx, self.assignee, expr) { | |
256 | self.counter += 1; | |
257 | } | |
258 | ||
259 | walk_expr(self, expr); | |
260 | } | |
261 | fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { | |
262 | NestedVisitorMap::None | |
263 | } | |
264 | } |