X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Feval_order_dependence.rs;h=7967d99d104aac2f98269e435f3406d082b13ef9;hb=6d1225981177587fbb68d9c4902c770c3dbaafb0;hp=31fae2d1967e6bce30d20e0e9b446c8560018c48;hpb=1e0729d48a2c062042f9587bca8e9078bb8ef619;p=rust.git diff --git a/clippy_lints/src/eval_order_dependence.rs b/clippy_lints/src/eval_order_dependence.rs index 31fae2d1967..7967d99d104 100644 --- a/clippy_lints/src/eval_order_dependence.rs +++ b/clippy_lints/src/eval_order_dependence.rs @@ -1,90 +1,80 @@ -// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - - -use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; -use crate::rustc::hir::*; -use crate::rustc::ty; -use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; -use crate::rustc::{declare_tool_lint, lint_array}; -use if_chain::if_chain; -use crate::syntax::ast; use crate::utils::{get_parent_expr, span_lint, span_note_and_lint}; +use if_chain::if_chain; +use rustc::declare_lint_pass; +use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use rustc::hir::*; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use rustc::ty; +use rustc_session::declare_tool_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_clippy_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. + /// ``` pub EVAL_ORDER_DEPENDENCE, complexity, "whether a variable read occurs before a write depends on sub-expression evaluation order" } -/// **What it does:** Checks for diverging calls that are not match arms or -/// statements. -/// -/// **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:** Someone might want to use `some_bool || panic!()` as a -/// shorthand. -/// -/// **Example:** -/// ```rust -/// let a = b() || panic!() || c(); -/// // `c()` is dead, `panic!()` is only called if `b()` returns `false` -/// let x = (a, b, c, panic!()); -/// // can simply be replaced by `panic!()` -/// ``` declare_clippy_lint! { + /// **What it does:** Checks for diverging calls that are not match arms or + /// statements. + /// + /// **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:** Someone might want to use `some_bool || panic!()` as a + /// shorthand. + /// + /// **Example:** + /// ```rust,no_run + /// # fn b() -> bool { true } + /// # fn c() -> bool { true } + /// let a = b() || panic!() || c(); + /// // `c()` is dead, `panic!()` is only called if `b()` returns `false` + /// let x = (a, b, c, panic!()); + /// // can simply be replaced by `panic!()` + /// ``` pub DIVERGING_SUB_EXPRESSION, complexity, "whether an expression contains a diverging sub expression" } -#[derive(Copy, Clone)] -pub struct EvalOrderDependence; - -impl LintPass for EvalOrderDependence { - fn get_lints(&self) -> LintArray { - lint_array!(EVAL_ORDER_DEPENDENCE, DIVERGING_SUB_EXPRESSION) - } -} +declare_lint_pass!(EvalOrderDependence => [EVAL_ORDER_DEPENDENCE, DIVERGING_SUB_EXPRESSION]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EvalOrderDependence { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { // Find a write to a local variable. - match expr.node { - ExprKind::Assign(ref lhs, _) | ExprKind::AssignOp(_, ref lhs, _) => if let ExprKind::Path(ref qpath) = lhs.node { - if let QPath::Resolved(_, ref path) = *qpath { - if path.segments.len() == 1 { - if let def::Def::Local(var) = cx.tables.qpath_def(qpath, lhs.hir_id) { - let mut visitor = ReadVisitor { - cx, - var, - write_expr: expr, - last_expr: expr, - }; - check_for_unsequenced_reads(&mut visitor); + match expr.kind { + ExprKind::Assign(ref lhs, _) | ExprKind::AssignOp(_, ref lhs, _) => { + if let ExprKind::Path(ref qpath) = lhs.kind { + if let QPath::Resolved(_, ref path) = *qpath { + if path.segments.len() == 1 { + if let def::Res::Local(var) = cx.tables.qpath_res(qpath, lhs.hir_id) { + let mut visitor = ReadVisitor { + cx, + var, + write_expr: expr, + last_expr: expr, + }; + check_for_unsequenced_reads(&mut visitor); + } } } } @@ -93,28 +83,26 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { } } fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) { - match stmt.node { - StmtKind::Expr(ref e, _) | StmtKind::Semi(ref e, _) => DivergenceVisitor { cx }.maybe_walk_expr(e), - StmtKind::Decl(ref d, _) => if let DeclKind::Local(ref local) = d.node { - if let Local { - init: Some(ref e), .. - } = **local - { + match stmt.kind { + StmtKind::Local(ref local) => { + if let Local { init: Some(ref e), .. } = **local { DivergenceVisitor { cx }.visit_expr(e); } }, + StmtKind::Expr(ref e) | StmtKind::Semi(ref e) => DivergenceVisitor { cx }.maybe_walk_expr(e), + StmtKind::Item(..) => {}, } } } -struct DivergenceVisitor<'a, 'tcx: 'a> { +struct DivergenceVisitor<'a, 'tcx> { cx: &'a LateContext<'a, 'tcx>, } impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { fn maybe_walk_expr(&mut self, e: &'tcx Expr) { - match e.node { - ExprKind::Closure(.., _) => {}, + match e.kind { + ExprKind::Closure(..) => {}, ExprKind::Match(ref e, ref arms, _) => { self.visit_expr(e); for arm in arms { @@ -137,14 +125,14 @@ fn report_diverging_sub_expr(&mut self, e: &Expr) { impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { fn visit_expr(&mut self, e: &'tcx Expr) { - match e.node { + match e.kind { ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => self.report_diverging_sub_expr(e), ExprKind::Call(ref func, _) => { let typ = self.cx.tables.expr_ty(func); - match typ.sty { + match typ.kind { ty::FnDef(..) | ty::FnPtr(_) => { let sig = typ.fn_sig(self.cx.tcx); - if let ty::Never = self.cx.tcx.erase_late_bound_regions(&sig).output().sty { + if let ty::Never = self.cx.tcx.erase_late_bound_regions(&sig).output().kind { self.report_diverging_sub_expr(e); } }, @@ -179,17 +167,17 @@ fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { /// 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 +/// * 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. +/// * 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.hir; - let mut cur_id = vis.write_expr.id; + let map = &vis.cx.tcx.hir(); + let mut cur_id = vis.write_expr.hir_id; loop { let parent_id = map.get_parent_node(cur_id); if parent_id == cur_id { @@ -227,19 +215,19 @@ enum StopEarly { } fn check_expr<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, expr: &'tcx Expr) -> StopEarly { - if expr.id == vis.last_expr.id { + if expr.hir_id == vis.last_expr.hir_id { return StopEarly::KeepGoing; } - match expr.node { - ExprKind::Array(_) | - ExprKind::Tup(_) | - ExprKind::MethodCall(..) | - ExprKind::Call(_, _) | - ExprKind::Assign(_, _) | - ExprKind::Index(_, _) | - ExprKind::Repeat(_, _) | - ExprKind::Struct(_, _, _) => { + match expr.kind { + ExprKind::Array(_) + | ExprKind::Tup(_) + | ExprKind::MethodCall(..) + | ExprKind::Call(_, _) + | ExprKind::Assign(_, _) + | ExprKind::Index(_, _) + | ExprKind::Repeat(_, _) + | ExprKind::Struct(_, _, _) => { walk_expr(vis, expr); }, ExprKind::Binary(op, _, _) | ExprKind::AssignOp(op, _, _) => { @@ -253,13 +241,12 @@ fn check_expr<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, expr: &'tcx Expr) -> St ExprKind::Closure(_, _, _, _, _) => { // 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 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. + // * `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; @@ -275,27 +262,23 @@ fn check_expr<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, expr: &'tcx Expr) -> St } fn check_stmt<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, stmt: &'tcx Stmt) -> StopEarly { - match stmt.node { - StmtKind::Expr(ref expr, _) | StmtKind::Semi(ref expr, _) => check_expr(vis, expr), - StmtKind::Decl(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 { - DeclKind::Local(ref local) => Some(local), - _ => None, - }; - local - .and_then(|local| local.init.as_ref()) - .map_or(StopEarly::KeepGoing, |expr| check_expr(vis, expr)) - }, + match stmt.kind { + StmtKind::Expr(ref expr) | StmtKind::Semi(ref expr) => check_expr(vis, expr), + // If the declaration is of a local variable, check its initializer + // expression if it has one. Otherwise, keep going. + StmtKind::Local(ref local) => local + .init + .as_ref() + .map_or(StopEarly::KeepGoing, |expr| check_expr(vis, expr)), + _ => StopEarly::KeepGoing, } } /// A visitor that looks for reads from a variable. -struct ReadVisitor<'a, 'tcx: 'a> { +struct ReadVisitor<'a, 'tcx> { cx: &'a LateContext<'a, 'tcx>, - /// The id of the variable we're looking for. - var: ast::NodeId, + /// The ID of the variable we're looking for. + var: HirId, /// The expressions where the write to the variable occurred (for reporting /// in the lint). write_expr: &'tcx Expr, @@ -306,16 +289,16 @@ struct ReadVisitor<'a, 'tcx: 'a> { impl<'a, 'tcx> Visitor<'tcx> for ReadVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr) { - if expr.id == self.last_expr.id { + if expr.hir_id == self.last_expr.hir_id { return; } - match expr.node { + match expr.kind { ExprKind::Path(ref qpath) => { if_chain! { if let QPath::Resolved(None, ref path) = *qpath; if path.segments.len() == 1; - if let def::Def::Local(local_id) = self.cx.tables.qpath_def(qpath, expr.hir_id); + if let def::Res::Local(local_id) = self.cx.tables.qpath_res(qpath, expr.hir_id); if local_id == self.var; // Check that this is a read, not a write. if !is_in_assignment_position(self.cx, expr); @@ -346,7 +329,7 @@ fn visit_expr(&mut self, expr: &'tcx Expr) { // ``` // // TODO: fix this - ExprKind::AddrOf(_, _) => { + ExprKind::AddrOf(_, _, _) => { return; } _ => {} @@ -359,11 +342,11 @@ fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { } } -/// Returns true if `expr` is the LHS of an assignment, like `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 ExprKind::Assign(ref lhs, _) = parent.node { - return lhs.id == expr.id; + if let ExprKind::Assign(ref lhs, _) = parent.kind { + return lhs.hir_id == expr.hir_id; } } false