|
| 1 | +use rustc::hir::def_id::DefId; |
| 2 | +use rustc::hir::intravisit::{Visitor, walk_expr}; |
| 3 | +use rustc::hir::*; |
| 4 | +use rustc::lint::*; |
| 5 | +use utils::{get_parent_expr, span_note_and_lint}; |
| 6 | + |
| 7 | +/// **What it does:** Checks for a read and a write to the same variable where |
| 8 | +/// whether the read occurs before or after the write depends on the evaluation |
| 9 | +/// order of sub-expressions. |
| 10 | +/// |
| 11 | +/// **Why is this bad?** It is often confusing to read. In addition, the |
| 12 | +/// sub-expression evaluation order for Rust is not well documented. |
| 13 | +/// |
| 14 | +/// **Known problems:** Code which intentionally depends on the evaluation |
| 15 | +/// order, or which is correct for any evaluation order. |
| 16 | +/// |
| 17 | +/// **Example:** |
| 18 | +/// ```rust |
| 19 | +/// let mut x = 0; |
| 20 | +/// let a = {x = 1; 1} + x; |
| 21 | +/// // Unclear whether a is 1 or 2. |
| 22 | +/// ``` |
| 23 | +declare_lint! { |
| 24 | + pub EVAL_ORDER_DEPENDENCE, |
| 25 | + Warn, |
| 26 | + "whether a variable read occurs before a write depends on sub-expression evaluation order" |
| 27 | +} |
| 28 | + |
| 29 | +#[derive(Copy,Clone)] |
| 30 | +pub struct EvalOrderDependence; |
| 31 | + |
| 32 | +impl LintPass for EvalOrderDependence { |
| 33 | + fn get_lints(&self) -> LintArray { |
| 34 | + lint_array!(EVAL_ORDER_DEPENDENCE) |
| 35 | + } |
| 36 | +} |
| 37 | + |
| 38 | +impl LateLintPass for EvalOrderDependence { |
| 39 | + fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { |
| 40 | + // Find a write to a local variable. |
| 41 | + match expr.node { |
| 42 | + ExprAssign(ref lhs, _) | ExprAssignOp(_, ref lhs, _) => { |
| 43 | + if let ExprPath(None, ref path) = lhs.node { |
| 44 | + if path.segments.len() == 1 { |
| 45 | + let var = cx.tcx.expect_def(lhs.id).def_id(); |
| 46 | + let mut visitor = ReadVisitor { |
| 47 | + cx: cx, |
| 48 | + var: var, |
| 49 | + write_expr: expr, |
| 50 | + last_expr: expr, |
| 51 | + }; |
| 52 | + check_for_unsequenced_reads(&mut visitor); |
| 53 | + } |
| 54 | + } |
| 55 | + } |
| 56 | + _ => {} |
| 57 | + } |
| 58 | + } |
| 59 | +} |
| 60 | + |
| 61 | +/// Walks up the AST from the the given write expression (`vis.write_expr`) |
| 62 | +/// looking for reads to the same variable that are unsequenced relative to the |
| 63 | +/// write. |
| 64 | +/// |
| 65 | +/// This means reads for which there is a common ancestor between the read and |
| 66 | +/// the write such that |
| 67 | +/// |
| 68 | +/// * evaluating the ancestor necessarily evaluates both the read and the write |
| 69 | +/// (for example, `&x` and `|| x = 1` don't necessarily evaluate `x`), and |
| 70 | +/// |
| 71 | +/// * which one is evaluated first depends on the order of sub-expression |
| 72 | +/// evaluation. Blocks, `if`s, loops, `match`es, and the short-circuiting |
| 73 | +/// logical operators are considered to have a defined evaluation order. |
| 74 | +/// |
| 75 | +/// When such a read is found, the lint is triggered. |
| 76 | +fn check_for_unsequenced_reads(vis: &mut ReadVisitor) { |
| 77 | + let map = &vis.cx.tcx.map; |
| 78 | + let mut cur_id = vis.write_expr.id; |
| 79 | + loop { |
| 80 | + let parent_id = map.get_parent_node(cur_id); |
| 81 | + if parent_id == cur_id { |
| 82 | + break; |
| 83 | + } |
| 84 | + let parent_node = match map.find(parent_id) { |
| 85 | + Some(parent) => parent, |
| 86 | + None => break, |
| 87 | + }; |
| 88 | + |
| 89 | + let stop_early = match parent_node { |
| 90 | + map::Node::NodeExpr(expr) => check_expr(vis, expr), |
| 91 | + map::Node::NodeStmt(stmt) => check_stmt(vis, stmt), |
| 92 | + map::Node::NodeItem(_) => { |
| 93 | + // We reached the top of the function, stop. |
| 94 | + break; |
| 95 | + }, |
| 96 | + _ => { StopEarly::KeepGoing } |
| 97 | + }; |
| 98 | + match stop_early { |
| 99 | + StopEarly::Stop => break, |
| 100 | + StopEarly::KeepGoing => {}, |
| 101 | + } |
| 102 | + |
| 103 | + cur_id = parent_id; |
| 104 | + } |
| 105 | +} |
| 106 | + |
| 107 | +/// Whether to stop early for the loop in `check_for_unsequenced_reads`. (If |
| 108 | +/// `check_expr` weren't an independent function, this would be unnecessary and |
| 109 | +/// we could just use `break`). |
| 110 | +enum StopEarly { |
| 111 | + KeepGoing, |
| 112 | + Stop, |
| 113 | +} |
| 114 | + |
| 115 | +fn check_expr<'v, 't>(vis: & mut ReadVisitor<'v, 't>, expr: &'v Expr) -> StopEarly { |
| 116 | + if expr.id == vis.last_expr.id { |
| 117 | + return StopEarly::KeepGoing; |
| 118 | + } |
| 119 | + |
| 120 | + match expr.node { |
| 121 | + ExprVec(_) | |
| 122 | + ExprTup(_) | |
| 123 | + ExprMethodCall(_, _, _) | |
| 124 | + ExprCall(_, _) | |
| 125 | + ExprAssign(_, _) | |
| 126 | + ExprIndex(_, _) | |
| 127 | + ExprRepeat(_, _) | |
| 128 | + ExprStruct(_, _, _) => { |
| 129 | + walk_expr(vis, expr); |
| 130 | + } |
| 131 | + ExprBinary(op, _, _) | |
| 132 | + ExprAssignOp(op, _, _) => { |
| 133 | + if op.node == BiAnd || op.node == BiOr { |
| 134 | + // x && y and x || y always evaluate x first, so these are |
| 135 | + // strictly sequenced. |
| 136 | + } else { |
| 137 | + walk_expr(vis, expr); |
| 138 | + } |
| 139 | + } |
| 140 | + ExprClosure(_, _, _, _) => { |
| 141 | + // Either |
| 142 | + // |
| 143 | + // * `var` is defined in the closure body, in which case we've |
| 144 | + // reached the top of the enclosing function and can stop, or |
| 145 | + // |
| 146 | + // * `var` is captured by the closure, in which case, because |
| 147 | + // evaluating a closure does not evaluate its body, we don't |
| 148 | + // necessarily have a write, so we need to stop to avoid |
| 149 | + // generating false positives. |
| 150 | + // |
| 151 | + // This is also the only place we need to stop early (grrr). |
| 152 | + return StopEarly::Stop; |
| 153 | + } |
| 154 | + // All other expressions either have only one child or strictly |
| 155 | + // sequence the evaluation order of their sub-expressions. |
| 156 | + _ => {} |
| 157 | + } |
| 158 | + |
| 159 | + vis.last_expr = expr; |
| 160 | + |
| 161 | + StopEarly::KeepGoing |
| 162 | +} |
| 163 | + |
| 164 | +fn check_stmt<'v, 't>(vis: &mut ReadVisitor<'v, 't>, stmt: &'v Stmt) -> StopEarly { |
| 165 | + match stmt.node { |
| 166 | + StmtExpr(ref expr, _) | |
| 167 | + StmtSemi(ref expr, _) => check_expr(vis, expr), |
| 168 | + StmtDecl(ref decl, _) => { |
| 169 | + // If the declaration is of a local variable, check its initializer |
| 170 | + // expression if it has one. Otherwise, keep going. |
| 171 | + let local = match decl.node { |
| 172 | + DeclLocal(ref local) => Some(local), |
| 173 | + _ => None, |
| 174 | + }; |
| 175 | + local.and_then(|local| local.init.as_ref()) |
| 176 | + .map_or(StopEarly::KeepGoing, |expr| check_expr(vis, expr)) |
| 177 | + } |
| 178 | + } |
| 179 | +} |
| 180 | + |
| 181 | +/// A visitor that looks for reads from a variable. |
| 182 | +struct ReadVisitor<'v, 't: 'v> { |
| 183 | + cx: &'v LateContext<'v, 't>, |
| 184 | + /// The id of the variable we're looking for. |
| 185 | + var: DefId, |
| 186 | + /// The expressions where the write to the variable occurred (for reporting |
| 187 | + /// in the lint). |
| 188 | + write_expr: &'v Expr, |
| 189 | + /// The last (highest in the AST) expression we've checked, so we know not |
| 190 | + /// to recheck it. |
| 191 | + last_expr: &'v Expr, |
| 192 | +} |
| 193 | + |
| 194 | +impl<'v, 't> Visitor<'v> for ReadVisitor<'v, 't> { |
| 195 | + fn visit_expr(&mut self, expr: &'v Expr) { |
| 196 | + if expr.id == self.last_expr.id { |
| 197 | + return; |
| 198 | + } |
| 199 | + |
| 200 | + match expr.node { |
| 201 | + ExprPath(None, ref path) => { |
| 202 | + if path.segments.len() == 1 && self.cx.tcx.expect_def(expr.id).def_id() == self.var { |
| 203 | + if is_in_assignment_position(self.cx, expr) { |
| 204 | + // This is a write, not a read. |
| 205 | + } else { |
| 206 | + span_note_and_lint( |
| 207 | + self.cx, |
| 208 | + EVAL_ORDER_DEPENDENCE, |
| 209 | + expr.span, |
| 210 | + "unsequenced read of a variable", |
| 211 | + self.write_expr.span, |
| 212 | + "whether read occurs before this write depends on evaluation order" |
| 213 | + ); |
| 214 | + } |
| 215 | + } |
| 216 | + } |
| 217 | + // We're about to descend a closure. Since we don't know when (or |
| 218 | + // if) the closure will be evaluated, any reads in it might not |
| 219 | + // occur here (or ever). Like above, bail to avoid false positives. |
| 220 | + ExprClosure(_, _, _, _) | |
| 221 | + |
| 222 | + // We want to avoid a false positive when a variable name occurs |
| 223 | + // only to have its address taken, so we stop here. Technically, |
| 224 | + // this misses some weird cases, eg. |
| 225 | + // |
| 226 | + // ```rust |
| 227 | + // let mut x = 0; |
| 228 | + // let a = foo(&{x = 1; x}, x); |
| 229 | + // ``` |
| 230 | + // |
| 231 | + // TODO: fix this |
| 232 | + ExprAddrOf(_, _) => { |
| 233 | + return; |
| 234 | + } |
| 235 | + _ => {} |
| 236 | + } |
| 237 | + |
| 238 | + walk_expr(self, expr); |
| 239 | + } |
| 240 | +} |
| 241 | + |
| 242 | +/// Returns true if `expr` is the LHS of an assignment, like `expr = ...`. |
| 243 | +fn is_in_assignment_position(cx: &LateContext, expr: &Expr) -> bool { |
| 244 | + if let Some(parent) = get_parent_expr(cx, expr) { |
| 245 | + if let ExprAssign(ref lhs, _) = parent.node { |
| 246 | + return lhs.id == expr.id; |
| 247 | + } |
| 248 | + } |
| 249 | + false |
| 250 | +} |
0 commit comments