--- /dev/null
+use rustc::hir::def_id::DefId;
+use rustc::hir::intravisit::{Visitor, walk_expr};
+use rustc::hir::*;
+use rustc::lint::*;
+use utils::{get_parent_expr, span_note_and_lint};
+
+/// **What it does:** Checks for a read and a write to the same variable where
+/// whether the read occurs before or after the write depends on the evaluation
+/// order of sub-expressions.
+///
+/// **Why is this bad?** It is often confusing to read. In addition, the
+/// sub-expression evaluation order for Rust is not well documented.
+///
+/// **Known problems:** Code which intentionally depends on the evaluation
+/// order, or which is correct for any evaluation order.
+///
+/// **Example:**
+/// ```rust
+/// let mut x = 0;
+/// let a = {x = 1; 1} + x;
+/// // Unclear whether a is 1 or 2.
+/// ```
+declare_lint! {
+ pub EVAL_ORDER_DEPENDENCE,
+ Warn,
+ "whether a variable read occurs before a write depends on sub-expression evaluation order"
+}
+
+#[derive(Copy,Clone)]
+pub struct EvalOrderDependence;
+
+impl LintPass for EvalOrderDependence {
+ fn get_lints(&self) -> LintArray {
+ lint_array!(EVAL_ORDER_DEPENDENCE)
+ }
+}
+
+impl LateLintPass for EvalOrderDependence {
+ fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+ // Find a write to a local variable.
+ match expr.node {
+ ExprAssign(ref lhs, _) | ExprAssignOp(_, ref lhs, _) => {
+ if let ExprPath(None, ref path) = lhs.node {
+ if path.segments.len() == 1 {
+ let var = cx.tcx.expect_def(lhs.id).def_id();
+ let mut visitor = ReadVisitor {
+ cx: cx,
+ var: var,
+ write_expr: expr,
+ last_expr: expr,
+ };
+ check_for_unsequenced_reads(&mut visitor);
+ }
+ }
+ }
+ _ => {}
+ }
+ }
+}
+
+/// Walks up the AST from the the given write expression (`vis.write_expr`)
+/// looking for reads to the same variable that are unsequenced relative to the
+/// write.
+///
+/// This means reads for which there is a common ancestor between the read and
+/// the write such that
+///
+/// * evaluating the ancestor necessarily evaluates both the read and the write
+/// (for example, `&x` and `|| x = 1` don't necessarily evaluate `x`), and
+///
+/// * which one is evaluated first depends on the order of sub-expression
+/// evaluation. Blocks, `if`s, loops, `match`es, and the short-circuiting
+/// logical operators are considered to have a defined evaluation order.
+///
+/// When such a read is found, the lint is triggered.
+fn check_for_unsequenced_reads(vis: &mut ReadVisitor) {
+ let map = &vis.cx.tcx.map;
+ let mut cur_id = vis.write_expr.id;
+ loop {
+ let parent_id = map.get_parent_node(cur_id);
+ if parent_id == cur_id {
+ break;
+ }
+ let parent_node = match map.find(parent_id) {
+ Some(parent) => parent,
+ None => break,
+ };
+
+ let stop_early = match parent_node {
+ map::Node::NodeExpr(expr) => check_expr(vis, expr),
+ map::Node::NodeStmt(stmt) => check_stmt(vis, stmt),
+ map::Node::NodeItem(_) => {
+ // We reached the top of the function, stop.
+ break;
+ },
+ _ => { StopEarly::KeepGoing }
+ };
+ match stop_early {
+ StopEarly::Stop => break,
+ StopEarly::KeepGoing => {},
+ }
+
+ cur_id = parent_id;
+ }
+}
+
+/// Whether to stop early for the loop in `check_for_unsequenced_reads`. (If
+/// `check_expr` weren't an independent function, this would be unnecessary and
+/// we could just use `break`).
+enum StopEarly {
+ KeepGoing,
+ Stop,
+}
+
+fn check_expr<'v, 't>(vis: & mut ReadVisitor<'v, 't>, expr: &'v Expr) -> StopEarly {
+ if expr.id == vis.last_expr.id {
+ return StopEarly::KeepGoing;
+ }
+
+ match expr.node {
+ ExprVec(_) |
+ ExprTup(_) |
+ ExprMethodCall(_, _, _) |
+ ExprCall(_, _) |
+ ExprAssign(_, _) |
+ ExprIndex(_, _) |
+ ExprRepeat(_, _) |
+ ExprStruct(_, _, _) => {
+ walk_expr(vis, expr);
+ }
+ ExprBinary(op, _, _) |
+ ExprAssignOp(op, _, _) => {
+ if op.node == BiAnd || op.node == BiOr {
+ // x && y and x || y always evaluate x first, so these are
+ // strictly sequenced.
+ } else {
+ walk_expr(vis, expr);
+ }
+ }
+ ExprClosure(_, _, _, _) => {
+ // Either
+ //
+ // * `var` is defined in the closure body, in which case we've
+ // reached the top of the enclosing function and can stop, or
+ //
+ // * `var` is captured by the closure, in which case, because
+ // evaluating a closure does not evaluate its body, we don't
+ // necessarily have a write, so we need to stop to avoid
+ // generating false positives.
+ //
+ // This is also the only place we need to stop early (grrr).
+ return StopEarly::Stop;
+ }
+ // All other expressions either have only one child or strictly
+ // sequence the evaluation order of their sub-expressions.
+ _ => {}
+ }
+
+ vis.last_expr = expr;
+
+ StopEarly::KeepGoing
+}
+
+fn check_stmt<'v, 't>(vis: &mut ReadVisitor<'v, 't>, stmt: &'v Stmt) -> StopEarly {
+ match stmt.node {
+ StmtExpr(ref expr, _) |
+ StmtSemi(ref expr, _) => check_expr(vis, expr),
+ StmtDecl(ref decl, _) => {
+ // If the declaration is of a local variable, check its initializer
+ // expression if it has one. Otherwise, keep going.
+ let local = match decl.node {
+ DeclLocal(ref local) => Some(local),
+ _ => None,
+ };
+ local.and_then(|local| local.init.as_ref())
+ .map_or(StopEarly::KeepGoing, |expr| check_expr(vis, expr))
+ }
+ }
+}
+
+/// A visitor that looks for reads from a variable.
+struct ReadVisitor<'v, 't: 'v> {
+ cx: &'v LateContext<'v, 't>,
+ /// The id of the variable we're looking for.
+ var: DefId,
+ /// The expressions where the write to the variable occurred (for reporting
+ /// in the lint).
+ write_expr: &'v Expr,
+ /// The last (highest in the AST) expression we've checked, so we know not
+ /// to recheck it.
+ last_expr: &'v Expr,
+}
+
+impl<'v, 't> Visitor<'v> for ReadVisitor<'v, 't> {
+ fn visit_expr(&mut self, expr: &'v Expr) {
+ if expr.id == self.last_expr.id {
+ return;
+ }
+
+ match expr.node {
+ ExprPath(None, ref path) => {
+ if path.segments.len() == 1 && self.cx.tcx.expect_def(expr.id).def_id() == self.var {
+ if is_in_assignment_position(self.cx, expr) {
+ // This is a write, not a read.
+ } else {
+ span_note_and_lint(
+ self.cx,
+ EVAL_ORDER_DEPENDENCE,
+ expr.span,
+ "unsequenced read of a variable",
+ self.write_expr.span,
+ "whether read occurs before this write depends on evaluation order"
+ );
+ }
+ }
+ }
+ // We're about to descend a closure. Since we don't know when (or
+ // if) the closure will be evaluated, any reads in it might not
+ // occur here (or ever). Like above, bail to avoid false positives.
+ ExprClosure(_, _, _, _) |
+
+ // We want to avoid a false positive when a variable name occurs
+ // only to have its address taken, so we stop here. Technically,
+ // this misses some weird cases, eg.
+ //
+ // ```rust
+ // let mut x = 0;
+ // let a = foo(&{x = 1; x}, x);
+ // ```
+ //
+ // TODO: fix this
+ ExprAddrOf(_, _) => {
+ return;
+ }
+ _ => {}
+ }
+
+ walk_expr(self, expr);
+ }
+}
+
+/// Returns true if `expr` is the LHS of an assignment, like `expr = ...`.
+fn is_in_assignment_position(cx: &LateContext, expr: &Expr) -> bool {
+ if let Some(parent) = get_parent_expr(cx, expr) {
+ if let ExprAssign(ref lhs, _) = parent.node {
+ return lhs.id == expr.id;
+ }
+ }
+ false
+}
--- /dev/null
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#[deny(eval_order_dependence)]
+#[allow(unused_assignments, unused_variables, many_single_char_names, no_effect, dead_code, blacklisted_name)]
+fn main() {
+ let mut x = 0;
+ let a = { x = 1; 1 } + x;
+ //~^ ERROR unsequenced read
+
+ // Example from iss#277
+ x += { x = 20; 2 }; //~ERROR unsequenced read
+
+ // Does it work in weird places?
+ // ...in the base for a struct expression?
+ struct Foo { a: i32, b: i32 };
+ let base = Foo { a: 4, b: 5 };
+ let foo = Foo { a: x, .. { x = 6; base } };
+ //~^ ERROR unsequenced read
+ // ...inside a closure?
+ let closure = || {
+ let mut x = 0;
+ x += { x = 20; 2 }; //~ERROR unsequenced read
+ };
+ // ...not across a closure?
+ let mut y = 0;
+ let b = (y, || { y = 1 });
+
+ // && and || evaluate left-to-right.
+ let a = { x = 1; true } && (x == 3);
+ let a = { x = 1; true } || (x == 3);
+
+ // Make sure we don't get confused by alpha conversion.
+ let a = { let mut x = 1; x = 2; 1 } + x;
+
+ // No warning if we don't read the variable...
+ x = { x = 20; 2 };
+ // ...if the assignment is in a closure...
+ let b = { || { x = 1; }; 1 } + x;
+ // ... or the access is under an address.
+ let b = ({ let p = &x; 1 }, { x = 1; x });
+
+ // Limitation: l-values other than simple variables don't trigger
+ // the warning.
+ let mut tup = (0, 0);
+ let c = { tup.0 = 1; 1 } + tup.0;
+ // Limitation: you can get away with a read under address-of.
+ let mut z = 0;
+ let b = (&{ z = x; x }, { x = 3; x });
+}
projects / rust.git / commitdiff