]>
Commit | Line | Data |
---|---|---|
cdc7bbd5 XL |
1 | use clippy_utils::diagnostics::{span_lint, span_lint_and_note}; |
2 | use clippy_utils::{get_parent_expr, path_to_local, path_to_local_id}; | |
17df50a5 | 3 | use if_chain::if_chain; |
f20569fa XL |
4 | use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; |
5 | use rustc_hir::{BinOpKind, Block, Expr, ExprKind, Guard, HirId, Local, Node, Stmt, StmtKind}; | |
6 | use rustc_lint::{LateContext, LateLintPass}; | |
7 | use rustc_middle::hir::map::Map; | |
8 | use rustc_middle::ty; | |
9 | use rustc_session::{declare_lint_pass, declare_tool_lint}; | |
10 | ||
11 | declare_clippy_lint! { | |
12 | /// **What it does:** Checks for a read and a write to the same variable where | |
13 | /// whether the read occurs before or after the write depends on the evaluation | |
14 | /// order of sub-expressions. | |
15 | /// | |
16 | /// **Why is this bad?** It is often confusing to read. In addition, the | |
17 | /// sub-expression evaluation order for Rust is not well documented. | |
18 | /// | |
19 | /// **Known problems:** Code which intentionally depends on the evaluation | |
20 | /// order, or which is correct for any evaluation order. | |
21 | /// | |
22 | /// **Example:** | |
23 | /// ```rust | |
24 | /// let mut x = 0; | |
25 | /// | |
26 | /// // Bad | |
27 | /// let a = { | |
28 | /// x = 1; | |
29 | /// 1 | |
30 | /// } + x; | |
31 | /// // Unclear whether a is 1 or 2. | |
32 | /// | |
33 | /// // Good | |
34 | /// let tmp = { | |
35 | /// x = 1; | |
36 | /// 1 | |
37 | /// }; | |
38 | /// let a = tmp + x; | |
39 | /// ``` | |
40 | pub EVAL_ORDER_DEPENDENCE, | |
136023e0 | 41 | suspicious, |
f20569fa XL |
42 | "whether a variable read occurs before a write depends on sub-expression evaluation order" |
43 | } | |
44 | ||
45 | declare_clippy_lint! { | |
46 | /// **What it does:** Checks for diverging calls that are not match arms or | |
47 | /// statements. | |
48 | /// | |
49 | /// **Why is this bad?** It is often confusing to read. In addition, the | |
50 | /// sub-expression evaluation order for Rust is not well documented. | |
51 | /// | |
52 | /// **Known problems:** Someone might want to use `some_bool || panic!()` as a | |
53 | /// shorthand. | |
54 | /// | |
55 | /// **Example:** | |
56 | /// ```rust,no_run | |
57 | /// # fn b() -> bool { true } | |
58 | /// # fn c() -> bool { true } | |
59 | /// let a = b() || panic!() || c(); | |
60 | /// // `c()` is dead, `panic!()` is only called if `b()` returns `false` | |
61 | /// let x = (a, b, c, panic!()); | |
62 | /// // can simply be replaced by `panic!()` | |
63 | /// ``` | |
64 | pub DIVERGING_SUB_EXPRESSION, | |
65 | complexity, | |
66 | "whether an expression contains a diverging sub expression" | |
67 | } | |
68 | ||
69 | declare_lint_pass!(EvalOrderDependence => [EVAL_ORDER_DEPENDENCE, DIVERGING_SUB_EXPRESSION]); | |
70 | ||
71 | impl<'tcx> LateLintPass<'tcx> for EvalOrderDependence { | |
72 | fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { | |
73 | // Find a write to a local variable. | |
17df50a5 XL |
74 | let var = if_chain! { |
75 | if let ExprKind::Assign(lhs, ..) | ExprKind::AssignOp(_, lhs, _) = expr.kind; | |
76 | if let Some(var) = path_to_local(lhs); | |
77 | if expr.span.desugaring_kind().is_none(); | |
78 | then { var } else { return; } | |
79 | }; | |
80 | let mut visitor = ReadVisitor { | |
81 | cx, | |
82 | var, | |
83 | write_expr: expr, | |
84 | last_expr: expr, | |
85 | }; | |
86 | check_for_unsequenced_reads(&mut visitor); | |
f20569fa XL |
87 | } |
88 | fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { | |
89 | match stmt.kind { | |
cdc7bbd5 XL |
90 | StmtKind::Local(local) => { |
91 | if let Local { init: Some(e), .. } = local { | |
f20569fa XL |
92 | DivergenceVisitor { cx }.visit_expr(e); |
93 | } | |
94 | }, | |
cdc7bbd5 | 95 | StmtKind::Expr(e) | StmtKind::Semi(e) => DivergenceVisitor { cx }.maybe_walk_expr(e), |
f20569fa XL |
96 | StmtKind::Item(..) => {}, |
97 | } | |
98 | } | |
99 | } | |
100 | ||
101 | struct DivergenceVisitor<'a, 'tcx> { | |
102 | cx: &'a LateContext<'tcx>, | |
103 | } | |
104 | ||
105 | impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { | |
106 | fn maybe_walk_expr(&mut self, e: &'tcx Expr<'_>) { | |
107 | match e.kind { | |
108 | ExprKind::Closure(..) => {}, | |
cdc7bbd5 | 109 | ExprKind::Match(e, arms, _) => { |
f20569fa XL |
110 | self.visit_expr(e); |
111 | for arm in arms { | |
112 | if let Some(Guard::If(if_expr)) = arm.guard { | |
17df50a5 | 113 | self.visit_expr(if_expr); |
f20569fa XL |
114 | } |
115 | // make sure top level arm expressions aren't linted | |
116 | self.maybe_walk_expr(&*arm.body); | |
117 | } | |
118 | }, | |
119 | _ => walk_expr(self, e), | |
120 | } | |
121 | } | |
122 | fn report_diverging_sub_expr(&mut self, e: &Expr<'_>) { | |
123 | span_lint(self.cx, DIVERGING_SUB_EXPRESSION, e.span, "sub-expression diverges"); | |
124 | } | |
125 | } | |
126 | ||
127 | impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { | |
128 | type Map = Map<'tcx>; | |
129 | ||
130 | fn visit_expr(&mut self, e: &'tcx Expr<'_>) { | |
131 | match e.kind { | |
132 | ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => self.report_diverging_sub_expr(e), | |
cdc7bbd5 | 133 | ExprKind::Call(func, _) => { |
f20569fa XL |
134 | let typ = self.cx.typeck_results().expr_ty(func); |
135 | match typ.kind() { | |
136 | ty::FnDef(..) | ty::FnPtr(_) => { | |
137 | let sig = typ.fn_sig(self.cx.tcx); | |
138 | if let ty::Never = self.cx.tcx.erase_late_bound_regions(sig).output().kind() { | |
139 | self.report_diverging_sub_expr(e); | |
140 | } | |
141 | }, | |
142 | _ => {}, | |
143 | } | |
144 | }, | |
145 | ExprKind::MethodCall(..) => { | |
146 | let borrowed_table = self.cx.typeck_results(); | |
147 | if borrowed_table.expr_ty(e).is_never() { | |
148 | self.report_diverging_sub_expr(e); | |
149 | } | |
150 | }, | |
151 | _ => { | |
152 | // do not lint expressions referencing objects of type `!`, as that required a | |
153 | // diverging expression | |
154 | // to begin with | |
155 | }, | |
156 | } | |
157 | self.maybe_walk_expr(e); | |
158 | } | |
159 | fn visit_block(&mut self, _: &'tcx Block<'_>) { | |
160 | // don't continue over blocks, LateLintPass already does that | |
161 | } | |
162 | fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { | |
163 | NestedVisitorMap::None | |
164 | } | |
165 | } | |
166 | ||
167 | /// Walks up the AST from the given write expression (`vis.write_expr`) looking | |
168 | /// for reads to the same variable that are unsequenced relative to the write. | |
169 | /// | |
170 | /// This means reads for which there is a common ancestor between the read and | |
171 | /// the write such that | |
172 | /// | |
173 | /// * evaluating the ancestor necessarily evaluates both the read and the write (for example, `&x` | |
174 | /// and `|| x = 1` don't necessarily evaluate `x`), and | |
175 | /// | |
176 | /// * which one is evaluated first depends on the order of sub-expression evaluation. Blocks, `if`s, | |
177 | /// loops, `match`es, and the short-circuiting logical operators are considered to have a defined | |
178 | /// evaluation order. | |
179 | /// | |
180 | /// When such a read is found, the lint is triggered. | |
181 | fn check_for_unsequenced_reads(vis: &mut ReadVisitor<'_, '_>) { | |
182 | let map = &vis.cx.tcx.hir(); | |
183 | let mut cur_id = vis.write_expr.hir_id; | |
184 | loop { | |
185 | let parent_id = map.get_parent_node(cur_id); | |
186 | if parent_id == cur_id { | |
187 | break; | |
188 | } | |
189 | let parent_node = match map.find(parent_id) { | |
190 | Some(parent) => parent, | |
191 | None => break, | |
192 | }; | |
193 | ||
194 | let stop_early = match parent_node { | |
195 | Node::Expr(expr) => check_expr(vis, expr), | |
196 | Node::Stmt(stmt) => check_stmt(vis, stmt), | |
197 | Node::Item(_) => { | |
198 | // We reached the top of the function, stop. | |
199 | break; | |
200 | }, | |
201 | _ => StopEarly::KeepGoing, | |
202 | }; | |
203 | match stop_early { | |
204 | StopEarly::Stop => break, | |
205 | StopEarly::KeepGoing => {}, | |
206 | } | |
207 | ||
208 | cur_id = parent_id; | |
209 | } | |
210 | } | |
211 | ||
212 | /// Whether to stop early for the loop in `check_for_unsequenced_reads`. (If | |
213 | /// `check_expr` weren't an independent function, this would be unnecessary and | |
214 | /// we could just use `break`). | |
215 | enum StopEarly { | |
216 | KeepGoing, | |
217 | Stop, | |
218 | } | |
219 | ||
220 | fn check_expr<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, expr: &'tcx Expr<'_>) -> StopEarly { | |
221 | if expr.hir_id == vis.last_expr.hir_id { | |
222 | return StopEarly::KeepGoing; | |
223 | } | |
224 | ||
225 | match expr.kind { | |
226 | ExprKind::Array(_) | |
227 | | ExprKind::Tup(_) | |
228 | | ExprKind::MethodCall(..) | |
229 | | ExprKind::Call(_, _) | |
230 | | ExprKind::Assign(..) | |
231 | | ExprKind::Index(_, _) | |
232 | | ExprKind::Repeat(_, _) | |
233 | | ExprKind::Struct(_, _, _) => { | |
234 | walk_expr(vis, expr); | |
235 | }, | |
236 | ExprKind::Binary(op, _, _) | ExprKind::AssignOp(op, _, _) => { | |
237 | if op.node == BinOpKind::And || op.node == BinOpKind::Or { | |
238 | // x && y and x || y always evaluate x first, so these are | |
239 | // strictly sequenced. | |
240 | } else { | |
241 | walk_expr(vis, expr); | |
242 | } | |
243 | }, | |
244 | ExprKind::Closure(_, _, _, _, _) => { | |
245 | // Either | |
246 | // | |
247 | // * `var` is defined in the closure body, in which case we've reached the top of the enclosing | |
248 | // function and can stop, or | |
249 | // | |
250 | // * `var` is captured by the closure, in which case, because evaluating a closure does not evaluate | |
251 | // its body, we don't necessarily have a write, so we need to stop to avoid generating false | |
252 | // positives. | |
253 | // | |
254 | // This is also the only place we need to stop early (grrr). | |
255 | return StopEarly::Stop; | |
256 | }, | |
257 | // All other expressions either have only one child or strictly | |
258 | // sequence the evaluation order of their sub-expressions. | |
259 | _ => {}, | |
260 | } | |
261 | ||
262 | vis.last_expr = expr; | |
263 | ||
264 | StopEarly::KeepGoing | |
265 | } | |
266 | ||
267 | fn check_stmt<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, stmt: &'tcx Stmt<'_>) -> StopEarly { | |
268 | match stmt.kind { | |
cdc7bbd5 | 269 | StmtKind::Expr(expr) | StmtKind::Semi(expr) => check_expr(vis, expr), |
f20569fa XL |
270 | // If the declaration is of a local variable, check its initializer |
271 | // expression if it has one. Otherwise, keep going. | |
cdc7bbd5 | 272 | StmtKind::Local(local) => local |
f20569fa XL |
273 | .init |
274 | .as_ref() | |
275 | .map_or(StopEarly::KeepGoing, |expr| check_expr(vis, expr)), | |
cdc7bbd5 | 276 | StmtKind::Item(..) => StopEarly::KeepGoing, |
f20569fa XL |
277 | } |
278 | } | |
279 | ||
280 | /// A visitor that looks for reads from a variable. | |
281 | struct ReadVisitor<'a, 'tcx> { | |
282 | cx: &'a LateContext<'tcx>, | |
283 | /// The ID of the variable we're looking for. | |
284 | var: HirId, | |
285 | /// The expressions where the write to the variable occurred (for reporting | |
286 | /// in the lint). | |
287 | write_expr: &'tcx Expr<'tcx>, | |
288 | /// The last (highest in the AST) expression we've checked, so we know not | |
289 | /// to recheck it. | |
290 | last_expr: &'tcx Expr<'tcx>, | |
291 | } | |
292 | ||
293 | impl<'a, 'tcx> Visitor<'tcx> for ReadVisitor<'a, 'tcx> { | |
294 | type Map = Map<'tcx>; | |
295 | ||
296 | fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { | |
297 | if expr.hir_id == self.last_expr.hir_id { | |
298 | return; | |
299 | } | |
300 | ||
301 | if path_to_local_id(expr, self.var) { | |
302 | // Check that this is a read, not a write. | |
303 | if !is_in_assignment_position(self.cx, expr) { | |
304 | span_lint_and_note( | |
305 | self.cx, | |
306 | EVAL_ORDER_DEPENDENCE, | |
307 | expr.span, | |
17df50a5 | 308 | &format!("unsequenced read of `{}`", self.cx.tcx.hir().name(self.var)), |
f20569fa XL |
309 | Some(self.write_expr.span), |
310 | "whether read occurs before this write depends on evaluation order", | |
311 | ); | |
312 | } | |
313 | } | |
314 | match expr.kind { | |
315 | // We're about to descend a closure. Since we don't know when (or | |
316 | // if) the closure will be evaluated, any reads in it might not | |
317 | // occur here (or ever). Like above, bail to avoid false positives. | |
318 | ExprKind::Closure(_, _, _, _, _) | | |
319 | ||
320 | // We want to avoid a false positive when a variable name occurs | |
321 | // only to have its address taken, so we stop here. Technically, | |
322 | // this misses some weird cases, eg. | |
323 | // | |
324 | // ```rust | |
325 | // let mut x = 0; | |
326 | // let a = foo(&{x = 1; x}, x); | |
327 | // ``` | |
328 | // | |
329 | // TODO: fix this | |
330 | ExprKind::AddrOf(_, _, _) => { | |
331 | return; | |
332 | } | |
333 | _ => {} | |
334 | } | |
335 | ||
336 | walk_expr(self, expr); | |
337 | } | |
338 | fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { | |
339 | NestedVisitorMap::None | |
340 | } | |
341 | } | |
342 | ||
343 | /// Returns `true` if `expr` is the LHS of an assignment, like `expr = ...`. | |
344 | fn is_in_assignment_position(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { | |
345 | if let Some(parent) = get_parent_expr(cx, expr) { | |
cdc7bbd5 | 346 | if let ExprKind::Assign(lhs, ..) = parent.kind { |
f20569fa XL |
347 | return lhs.hir_id == expr.hir_id; |
348 | } | |
349 | } | |
350 | false | |
351 | } |