From b0a96def09237932d02f6ffa5013ea6f5673e314 Mon Sep 17 00:00:00 2001 From: scurest Date: Wed, 10 Aug 2016 22:16:28 -0500 Subject: [PATCH] Add lint for reads and writes that depend on evaluation order --- CHANGELOG.md | 1 + README.md | 3 +- clippy_lints/src/eval_order_dependence.rs | 250 ++++++++++++++++++++ clippy_lints/src/lib.rs | 3 + tests/compile-fail/eval_order_dependence.rs | 50 ++++ 5 files changed, 306 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/eval_order_dependence.rs create mode 100644 tests/compile-fail/eval_order_dependence.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d83611f1fd3..40cf282a069 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index b5e69b918b3..4e6f864fec3 100644 --- a/README.md +++ b/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 diff --git a/clippy_lints/src/eval_order_dependence.rs b/clippy_lints/src/eval_order_dependence.rs new file mode 100644 index 00000000000..4465d31bff5 --- /dev/null +++ b/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 +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0ab736d2920..260b5c1ba57 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -78,6 +78,7 @@ macro_rules! declare_restriction_lint { 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, diff --git a/tests/compile-fail/eval_order_dependence.rs b/tests/compile-fail/eval_order_dependence.rs new file mode 100644 index 00000000000..0b2605d01bd --- /dev/null +++ b/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 }); +} -- 2.44.0