From f469860dc23cd432b76a1d6779ce2f1206805c6f Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 6 Sep 2016 15:15:36 +0200 Subject: [PATCH] lint diverging expressions that are sub-expressions of others --- CHANGELOG.md | 1 + README.md | 3 +- clippy_lints/src/eval_order_dependence.rs | 94 ++++++++++++++++++- clippy_lints/src/lib.rs | 1 + .../compile-fail/diverging_sub_expression.rs | 14 +++ 5 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 tests/compile-fail/diverging_sub_expression.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ea0fb88b..84bbf87d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -211,6 +211,7 @@ All notable changes to this project will be documented in this file. [`cyclomatic_complexity`]: https://github.com/Manishearth/rust-clippy/wiki#cyclomatic_complexity [`deprecated_semver`]: https://github.com/Manishearth/rust-clippy/wiki#deprecated_semver [`derive_hash_xor_eq`]: https://github.com/Manishearth/rust-clippy/wiki#derive_hash_xor_eq +[`diverging_sub_expression`]: https://github.com/Manishearth/rust-clippy/wiki#diverging_sub_expression [`doc_markdown`]: https://github.com/Manishearth/rust-clippy/wiki#doc_markdown [`double_neg`]: https://github.com/Manishearth/rust-clippy/wiki#double_neg [`drop_ref`]: https://github.com/Manishearth/rust-clippy/wiki#drop_ref diff --git a/README.md b/README.md index 83b9788fb..ba0973b0e 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 170 lints included in this crate: +There are 171 lints included in this crate: name | default | triggers on ---------------------------------------------------------------------------------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------- @@ -50,6 +50,7 @@ name [cyclomatic_complexity](https://github.com/Manishearth/rust-clippy/wiki#cyclomatic_complexity) | warn | functions that should be split up into multiple functions [deprecated_semver](https://github.com/Manishearth/rust-clippy/wiki#deprecated_semver) | warn | use of `#[deprecated(since = "x")]` where x is not semver [derive_hash_xor_eq](https://github.com/Manishearth/rust-clippy/wiki#derive_hash_xor_eq) | warn | deriving `Hash` but implementing `PartialEq` explicitly +[diverging_sub_expression](https://github.com/Manishearth/rust-clippy/wiki#diverging_sub_expression) | warn | whether an expression contains a diverging sub expression [doc_markdown](https://github.com/Manishearth/rust-clippy/wiki#doc_markdown) | warn | presence of `_`, `::` or camel-case outside backticks in documentation [double_neg](https://github.com/Manishearth/rust-clippy/wiki#double_neg) | warn | `--x`, which is a double negation of `x` and not a pre-decrement as in C/C++ [drop_ref](https://github.com/Manishearth/rust-clippy/wiki#drop_ref) | warn | calls to `std::mem::drop` with a reference instead of an owned value diff --git a/clippy_lints/src/eval_order_dependence.rs b/clippy_lints/src/eval_order_dependence.rs index 4465d31bf..d6964c156 100644 --- a/clippy_lints/src/eval_order_dependence.rs +++ b/clippy_lints/src/eval_order_dependence.rs @@ -1,8 +1,9 @@ use rustc::hir::def_id::DefId; use rustc::hir::intravisit::{Visitor, walk_expr}; use rustc::hir::*; +use rustc::ty; use rustc::lint::*; -use utils::{get_parent_expr, span_note_and_lint}; +use utils::{get_parent_expr, span_note_and_lint, span_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 @@ -26,12 +27,32 @@ declare_lint! { "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_lint! { + pub DIVERGING_SUB_EXPRESSION, + Warn, + "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) + lint_array!(EVAL_ORDER_DEPENDENCE, DIVERGING_SUB_EXPRESSION) } } @@ -56,6 +77,75 @@ impl LateLintPass for EvalOrderDependence { _ => {} } } + fn check_stmt(&mut self, cx: &LateContext, stmt: &Stmt) { + match stmt.node { + StmtExpr(ref e, _) | StmtSemi(ref e, _) => DivergenceVisitor(cx).maybe_walk_expr(e), + StmtDecl(ref d, _) => { + if let DeclLocal(ref local) = d.node { + if let Local { init: Some(ref e), .. } = **local { + DivergenceVisitor(cx).visit_expr(e); + } + } + }, + } + } +} + +struct DivergenceVisitor<'a, 'tcx: 'a>(&'a LateContext<'a, 'tcx>); + +impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { + fn maybe_walk_expr(&mut self, e: &Expr) { + match e.node { + ExprClosure(..) => {}, + ExprMatch(ref e, ref arms, _) => { + self.visit_expr(e); + for arm in arms { + if let Some(ref guard) = arm.guard { + self.visit_expr(guard); + } + // make sure top level arm expressions aren't linted + walk_expr(self, &*arm.body); + } + } + _ => walk_expr(self, e), + } + } + fn report_diverging_sub_expr(&mut self, e: &Expr) { + span_lint( + self.0, + DIVERGING_SUB_EXPRESSION, + e.span, + "sub-expression diverges", + ); + } +} + +impl<'a, 'tcx, 'v> Visitor<'v> for DivergenceVisitor<'a, 'tcx> { + fn visit_expr(&mut self, e: &'v Expr) { + // this match can be replaced by just the default arm, once + // https://github.com/rust-lang/rust/issues/35121 makes sure that + // ! is propagated properly + match e.node { + ExprAgain(_) | + ExprBreak(_) | + ExprRet(_) => self.report_diverging_sub_expr(e), + ExprCall(ref func, _) => match self.0.tcx.expr_ty(func).sty { + ty::TyFnDef(_, _, fn_ty) | + ty::TyFnPtr(fn_ty) => if let ty::TyNever = self.0.tcx.erase_late_bound_regions(&fn_ty.sig).output.sty { + self.report_diverging_sub_expr(e); + }, + _ => {}, + }, + ExprMethodCall(..) => { /* TODO */ }, + _ => if let ty::TyNever = self.0.tcx.expr_ty(e).sty { + self.report_diverging_sub_expr(e); + }, + } + self.maybe_walk_expr(e); + } + fn visit_block(&mut self, _: &'v Block) { + // don't continue over blocks, LateLintPass already does that + } } /// Walks up the AST from the the given write expression (`vis.write_expr`) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 36d466ed9..27842d6ad 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -332,6 +332,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { eq_op::EQ_OP, escape::BOXED_LOCAL, eta_reduction::REDUNDANT_CLOSURE, + eval_order_dependence::DIVERGING_SUB_EXPRESSION, eval_order_dependence::EVAL_ORDER_DEPENDENCE, format::USELESS_FORMAT, formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING, diff --git a/tests/compile-fail/diverging_sub_expression.rs b/tests/compile-fail/diverging_sub_expression.rs new file mode 100644 index 000000000..929ef911f --- /dev/null +++ b/tests/compile-fail/diverging_sub_expression.rs @@ -0,0 +1,14 @@ +#![feature(plugin, never_type)] +#![plugin(clippy)] +#![deny(diverging_sub_expression)] + +#[allow(empty_loop)] +fn diverge() -> ! { loop {} } + +#[allow(unused_variables, unnecessary_operation)] +fn main() { + let b = true; + b || diverge(); //~ ERROR sub-expression diverges + let y = (5, diverge(), 6); //~ ERROR sub-expression diverges + println!("{}", y.1); //~ ERROR sub-expression diverges +}