Add lint for reads and writes that depend on evaluation order

CHANGELOG.md

@@ -170,6 +170,7 @@ All notable changes to this project will be documented in this file.
 [`enum_glob_use`]: https://github.com/Manishearth/rust-clippy/wiki#enum_glob_use
 [`enum_variant_names`]: https://github.com/Manishearth/rust-clippy/wiki#enum_variant_names
 [`eq_op`]: https://github.com/Manishearth/rust-clippy/wiki#eq_op
+[`eval_order_dependence`]: https://github.com/Manishearth/rust-clippy/wiki#eval_order_dependence
 [`expl_impl_clone_on_copy`]: https://github.com/Manishearth/rust-clippy/wiki#expl_impl_clone_on_copy
 [`explicit_counter_loop`]: https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop
 [`explicit_iter_loop`]: https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop

README.md

@@ -17,7 +17,7 @@ Table of contents:
 
 ## Lints
 
-There are 162 lints included in this crate:
+There are 163 lints included in this crate:
 
 name                                                                                                                 | default | triggers on
 ---------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -57,6 +57,7 @@ name
 [enum_glob_use](https://github.com/Manishearth/rust-clippy/wiki#enum_glob_use)                                       | allow   | use items that import all variants of an enum
 [enum_variant_names](https://github.com/Manishearth/rust-clippy/wiki#enum_variant_names)                             | warn    | enums where all variants share a prefix/postfix
 [eq_op](https://github.com/Manishearth/rust-clippy/wiki#eq_op)                                                       | warn    | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`)
+[eval_order_dependence](https://github.com/Manishearth/rust-clippy/wiki#eval_order_dependence)                       | warn    | whether a variable read occurs before a write depends on sub-expression evaluation order
 [expl_impl_clone_on_copy](https://github.com/Manishearth/rust-clippy/wiki#expl_impl_clone_on_copy)                   | warn    | implementing `Clone` explicitly on `Copy` types
 [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop)                       | warn    | for-looping with an explicit counter when `_.enumerate()` would do
 [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop)                             | warn    | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do

clippy_lints/src/eval_order_dependence.rs

@@ -0,0 +1,250 @@
+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
+}

clippy_lints/src/lib.rs

@@ -78,6 +78,7 @@ pub mod enum_variants;
 pub mod eq_op;
 pub mod escape;
 pub mod eta_reduction;
+pub mod eval_order_dependence;
 pub mod format;
 pub mod formatting;
 pub mod functions;
@@ -256,6 +257,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box arithmetic::Arithmetic::default());
     reg.register_late_lint_pass(box assign_ops::AssignOps);
     reg.register_late_lint_pass(box let_if_seq::LetIfSeq);
+    reg.register_late_lint_pass(box eval_order_dependence::EvalOrderDependence);
 
     reg.register_lint_group("clippy_restrictions", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -325,6 +327,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         eq_op::EQ_OP,
         escape::BOXED_LOCAL,
         eta_reduction::REDUNDANT_CLOSURE,
+        eval_order_dependence::EVAL_ORDER_DEPENDENCE,
         format::USELESS_FORMAT,
         formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
         formatting::SUSPICIOUS_ELSE_FORMATTING,

tests/compile-fail/eval_order_dependence.rs

@@ -0,0 +1,50 @@
+#![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 });
+}