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