From f469860dc23cd432b76a1d6779ce2f1206805c6f Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 6 Sep 2016 15:15:36 +0200 Subject: [PATCH 1/4] 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 7ea0fb88b09..84bbf87d29c 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 83b9788fba5..ba0973b0e67 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 4465d31bff5..d6964c156eb 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 36d466ed9c9..27842d6addd 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 00000000000..929ef911f43 --- /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 +} From a2257280ecab8692795098ac02198e98e7d00efc Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 13 Sep 2016 12:41:20 +0200 Subject: [PATCH 2/4] don't lint expressions referencing `!` objects, just expressions creating them --- clippy_lints/src/eval_order_dependence.rs | 7 ++----- tests/compile-fail/diverging_sub_expression.rs | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/eval_order_dependence.rs b/clippy_lints/src/eval_order_dependence.rs index d6964c156eb..f357cd8f6f9 100644 --- a/clippy_lints/src/eval_order_dependence.rs +++ b/clippy_lints/src/eval_order_dependence.rs @@ -122,9 +122,6 @@ impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { 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(_) | @@ -137,8 +134,8 @@ impl<'a, 'tcx, 'v> Visitor<'v> for DivergenceVisitor<'a, 'tcx> { _ => {}, }, ExprMethodCall(..) => { /* TODO */ }, - _ => if let ty::TyNever = self.0.tcx.expr_ty(e).sty { - self.report_diverging_sub_expr(e); + _ => { + // do not lint expressions referencing objects of type `!`, as that required a diverging expression to begin with }, } self.maybe_walk_expr(e); diff --git a/tests/compile-fail/diverging_sub_expression.rs b/tests/compile-fail/diverging_sub_expression.rs index 929ef911f43..20b284732b0 100644 --- a/tests/compile-fail/diverging_sub_expression.rs +++ b/tests/compile-fail/diverging_sub_expression.rs @@ -10,5 +10,5 @@ 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 + println!("{}", y.1); } From e6bfe4b514c9b7201c47726f5dde4c5aee53d054 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 13 Sep 2016 12:41:37 +0200 Subject: [PATCH 3/4] also lint diverging methods --- clippy_lints/src/eval_order_dependence.rs | 10 +++++++++- tests/compile-fail/diverging_sub_expression.rs | 7 +++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/eval_order_dependence.rs b/clippy_lints/src/eval_order_dependence.rs index f357cd8f6f9..e53c830e11d 100644 --- a/clippy_lints/src/eval_order_dependence.rs +++ b/clippy_lints/src/eval_order_dependence.rs @@ -133,7 +133,15 @@ impl<'a, 'tcx, 'v> Visitor<'v> for DivergenceVisitor<'a, 'tcx> { }, _ => {}, }, - ExprMethodCall(..) => { /* TODO */ }, + ExprMethodCall(..) => { + let method_call = ty::MethodCall::expr(e.id); + let borrowed_table = self.0.tcx.tables.borrow(); + let method_type = borrowed_table.method_map.get(&method_call).expect("This should never happen."); + let result_ty = method_type.ty.fn_ret(); + if let ty::TyNever = self.0.tcx.erase_late_bound_regions(&result_ty).sty { + self.report_diverging_sub_expr(e); + } + }, _ => { // do not lint expressions referencing objects of type `!`, as that required a diverging expression to begin with }, diff --git a/tests/compile-fail/diverging_sub_expression.rs b/tests/compile-fail/diverging_sub_expression.rs index 20b284732b0..d82832ef11f 100644 --- a/tests/compile-fail/diverging_sub_expression.rs +++ b/tests/compile-fail/diverging_sub_expression.rs @@ -5,10 +5,17 @@ #[allow(empty_loop)] fn diverge() -> ! { loop {} } +struct A; + +impl A { + fn foo(&self) -> ! { diverge() } +} + #[allow(unused_variables, unnecessary_operation)] fn main() { let b = true; b || diverge(); //~ ERROR sub-expression diverges + b || A.foo(); //~ ERROR sub-expression diverges let y = (5, diverge(), 6); //~ ERROR sub-expression diverges println!("{}", y.1); } From 9427a4ae8055d3ecf3ecfb2296d6a8e60868cbc0 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 13 Sep 2016 14:52:21 +0200 Subject: [PATCH 4/4] also test match statements, return, continue and break --- tests/compile-fail/diverging_sub_expression.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/compile-fail/diverging_sub_expression.rs b/tests/compile-fail/diverging_sub_expression.rs index d82832ef11f..782c406d74c 100644 --- a/tests/compile-fail/diverging_sub_expression.rs +++ b/tests/compile-fail/diverging_sub_expression.rs @@ -19,3 +19,19 @@ fn main() { let y = (5, diverge(), 6); //~ ERROR sub-expression diverges println!("{}", y.1); } + +#[allow(dead_code, unused_variables)] +fn foobar() { + loop { + let x = match 5 { + 4 => return, + 5 => continue, + 6 => (println!("foo"), return), //~ ERROR sub-expression diverges + 7 => (println!("bar"), continue), //~ ERROR sub-expression diverges + 8 => break, + 9 => diverge(), + 3 => (println!("moo"), diverge()), //~ ERROR sub-expression diverges + _ => (println!("boo"), break), //~ ERROR sub-expression diverges + }; + } +}