Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add lint for reads and writes that depend on evaluation order
- Loading branch information
Showing
5 changed files
with
306 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 }); | ||
| } |