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