From 902c7d832b6f355414632ca55dc7c7017fde5250 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 14 Dec 2015 14:29:20 +0100 Subject: [PATCH 1/2] fix cc computation in the presence of diverging calls CFG treats diverging calls as its completely own path out of the function. While this makes sense, it should also mean that a panic should increase the cyclomatic complexity. Instead it decreases it. Minimal example: ```rust if a { b } else { panic!("cake"); } d ``` creates the following graph ```dot digraph G { "if a" -> "b" "if a" -> "panic!(\"cake\")" "b" -> c } ``` which has a CC of 1 (3 - 4 + 2). A CC of 1 means there is one path through the program. Obviously that is wrong. There are two paths. One returning normally, and one panicking. --- src/cyclomatic_complexity.rs | 52 ++++++--- tests/cc_seme.rs | 25 +++++ tests/compile-fail/cyclomatic_complexity.rs | 110 +++++++++++++++++++- 3 files changed, 171 insertions(+), 16 deletions(-) create mode 100644 tests/cc_seme.rs diff --git a/src/cyclomatic_complexity.rs b/src/cyclomatic_complexity.rs index 1379a8db15d..cb391769279 100644 --- a/src/cyclomatic_complexity.rs +++ b/src/cyclomatic_complexity.rs @@ -3,6 +3,7 @@ use rustc::lint::*; use rustc_front::hir::*; use rustc::middle::cfg::CFG; +use rustc::middle::ty; use syntax::codemap::Span; use syntax::attr::*; use syntax::ast::Attribute; @@ -32,7 +33,7 @@ impl LintPass for CyclomaticComplexity { } impl CyclomaticComplexity { - fn check(&mut self, cx: &LateContext, block: &Block, span: Span) { + fn check<'a, 'tcx>(&mut self, cx: &'a LateContext<'a, 'tcx>, block: &Block, span: Span) { if in_macro(cx, span) { return; } let cfg = CFG::new(cx.tcx, block); let n = cfg.graph.len_nodes() as u64; @@ -40,15 +41,16 @@ impl CyclomaticComplexity { let cc = e + 2 - n; let mut arm_counter = MatchArmCounter(0); arm_counter.visit_block(block); - let mut narms = arm_counter.0; - if narms > 0 { - narms = narms - 1; - } - - if cc < narms { - report_cc_bug(cx, cc, narms, span); + let narms = arm_counter.0; + + let mut diverge_counter = DivergenceCounter(0, &cx.tcx); + diverge_counter.visit_block(block); + let divergence = diverge_counter.0; + + if cc + divergence < narms { + report_cc_bug(cx, cc, narms, divergence, span); } else { - let rust_cc = cc - narms; + let rust_cc = cc + divergence - narms; if rust_cc > self.limit.limit() { cx.span_lint_help(CYCLOMATIC_COMPLEXITY, span, &format!("The function has a cyclomatic complexity of {}.", rust_cc), @@ -93,8 +95,28 @@ impl<'a> Visitor<'a> for MatchArmCounter { ExprMatch(_, ref arms, _) => { walk_expr(self, e); let arms_n: u64 = arms.iter().map(|arm| arm.pats.len() as u64).sum(); - if arms_n > 0 { - self.0 += arms_n - 1; + if arms_n > 1 { + self.0 += arms_n - 2; + } + }, + ExprClosure(..) => {}, + _ => walk_expr(self, e), + } + } +} + +struct DivergenceCounter<'a, 'tcx: 'a>(u64, &'a ty::ctxt<'tcx>); + +impl<'a, 'b, 'tcx> Visitor<'a> for DivergenceCounter<'b, 'tcx> { + fn visit_expr(&mut self, e: &'a Expr) { + match e.node { + ExprCall(ref callee, _) => { + walk_expr(self, e); + let ty = self.1.node_id_to_type(callee.id); + if let ty::TyBareFn(_, ty) = ty.sty { + if ty.sig.skip_binder().output.diverges() { + self.0 += 1; + } } }, ExprClosure(..) => {}, @@ -104,15 +126,15 @@ impl<'a> Visitor<'a> for MatchArmCounter { } #[cfg(feature="debugging")] -fn report_cc_bug(cx: &LateContext, cc: u64, narms: u64, span: Span) { +fn report_cc_bug(cx: &LateContext, cc: u64, narms: u64, div: u64, span: Span) { cx.sess().span_bug(span, &format!("Clippy encountered a bug calculating cyclomatic complexity: \ - cc = {}, arms = {}. Please file a bug report.", cc, narms));; + cc = {}, arms = {}, div = {}. Please file a bug report.", cc, narms, div));; } #[cfg(not(feature="debugging"))] -fn report_cc_bug(cx: &LateContext, cc: u64, narms: u64, span: Span) { +fn report_cc_bug(cx: &LateContext, cc: u64, narms: u64, div: u64, span: Span) { if cx.current_level(CYCLOMATIC_COMPLEXITY) != Level::Allow { cx.sess().span_note(span, &format!("Clippy encountered a bug calculating cyclomatic complexity \ (hide this message with `#[allow(cyclomatic_complexity)]`): \ - cc = {}, arms = {}. Please file a bug report.", cc, narms)); + cc = {}, arms = {}, div = {}. Please file a bug report.", cc, narms, div)); } } diff --git a/tests/cc_seme.rs b/tests/cc_seme.rs new file mode 100644 index 00000000000..a26731c396a --- /dev/null +++ b/tests/cc_seme.rs @@ -0,0 +1,25 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[allow(dead_code)] +enum Baz { + Baz1, + Baz2, +} + +struct Test { + t: Option, + b: Baz, +} + +fn main() { + use Baz::*; + let x = Test { t: Some(0), b: Baz1 }; + + match x { + Test { t: Some(_), b: Baz1 } => unreachable!(), + Test { t: Some(42), b: Baz2 } => unreachable!(), + Test { t: None, .. } => unreachable!(), + Test { .. } => unreachable!(), + } +} diff --git a/tests/compile-fail/cyclomatic_complexity.rs b/tests/compile-fail/cyclomatic_complexity.rs index 8e3bf123c26..1a6dfd28728 100644 --- a/tests/compile-fail/cyclomatic_complexity.rs +++ b/tests/compile-fail/cyclomatic_complexity.rs @@ -89,7 +89,7 @@ fn main() { //~ ERROR: The function has a cyclomatic complexity of 28. } #[cyclomatic_complexity = "0"] -fn kaboom() { //~ ERROR: The function has a cyclomatic complexity of 6 +fn kaboom() { //~ ERROR: The function has a cyclomatic complexity of 8 let n = 0; 'a: for i in 0..20 { 'b: for j in i..20 { @@ -170,6 +170,114 @@ fn barr() { //~ ERROR: The function has a cyclomatic complexity of 2 } } +#[cyclomatic_complexity = "0"] +fn barr2() { //~ ERROR: The function has a cyclomatic complexity of 3 + match 99 { + 0 => println!("hi"), + 1 => println!("bla"), + 2 | 3 => println!("blub"), + _ => println!("bye"), + } + match 99 { + 0 => println!("hi"), + 1 => println!("bla"), + 2 | 3 => println!("blub"), + _ => println!("bye"), + } +} + +#[cyclomatic_complexity = "0"] +fn barrr() { //~ ERROR: The function has a cyclomatic complexity of 2 + match 99 { + 0 => println!("hi"), + 1 => panic!("bla"), + 2 | 3 => println!("blub"), + _ => println!("bye"), + } +} + +#[cyclomatic_complexity = "0"] +fn barrr2() { //~ ERROR: The function has a cyclomatic complexity of 3 + match 99 { + 0 => println!("hi"), + 1 => panic!("bla"), + 2 | 3 => println!("blub"), + _ => println!("bye"), + } + match 99 { + 0 => println!("hi"), + 1 => panic!("bla"), + 2 | 3 => println!("blub"), + _ => println!("bye"), + } +} + +#[cyclomatic_complexity = "0"] +fn barrrr() { //~ ERROR: The function has a cyclomatic complexity of 2 + match 99 { + 0 => println!("hi"), + 1 => println!("bla"), + 2 | 3 => panic!("blub"), + _ => println!("bye"), + } +} + +#[cyclomatic_complexity = "0"] +fn barrrr2() { //~ ERROR: The function has a cyclomatic complexity of 3 + match 99 { + 0 => println!("hi"), + 1 => println!("bla"), + 2 | 3 => panic!("blub"), + _ => println!("bye"), + } + match 99 { + 0 => println!("hi"), + 1 => println!("bla"), + 2 | 3 => panic!("blub"), + _ => println!("bye"), + } +} + +#[cyclomatic_complexity = "0"] +fn cake() { //~ ERROR: The function has a cyclomatic complexity of 2 + if 4 == 5 { + println!("yea"); + } else { + panic!("meh"); + } + println!("whee"); +} + + +#[cyclomatic_complexity = "0"] +pub fn read_file(input_path: &str) -> String { //~ ERROR: The function has a cyclomatic complexity of 4 + use std::fs::File; + use std::io::{Read, Write}; + use std::path::Path; + let mut file = match File::open(&Path::new(input_path)) { + Ok(f) => f, + Err(err) => { + panic!("Can't open {}: {}", input_path, err); + } + }; + + let mut bytes = Vec::new(); + + match file.read_to_end(&mut bytes) { + Ok(..) => {}, + Err(_) => { + panic!("Can't read {}", input_path); + } + }; + + match String::from_utf8(bytes) { + Ok(contents) => contents, + Err(_) => { + panic!("{} is not UTF-8 encoded", input_path); + } + } +} + enum Void {} #[cyclomatic_complexity = "0"] From cc1d696cb9df64b1da9aeebcc47717d84ec649f8 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 14 Dec 2015 14:30:09 +0100 Subject: [PATCH 2/2] fix fallout from CC improvements --- src/loops.rs | 27 +++++++++++---------------- src/matches.rs | 12 +++++------- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/loops.rs b/src/loops.rs index 3c4e023b98e..8295e186172 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -147,14 +147,8 @@ impl LateLintPass for LoopsPass { // extract the expression from the first statement (if any) in a block let inner_stmt_expr = extract_expr_from_first_stmt(block); - // extract the first expression (if any) from the block - let inner_expr = extract_first_expr(block); - let (extracted, collect_expr) = match inner_stmt_expr { - Some(_) => (inner_stmt_expr, true), // check if an expression exists in the first statement - None => (inner_expr, false), // if not, let's go for the first expression in the block - }; - - if let Some(inner) = extracted { + // or extract the first expression (if any) from the block + if let Some(inner) = inner_stmt_expr.or_else(|| extract_first_expr(block)) { if let ExprMatch(ref matchexpr, ref arms, ref source) = inner.node { // collect the remaining statements below the match let mut other_stuff = block.stmts @@ -163,10 +157,11 @@ impl LateLintPass for LoopsPass { .map(|stmt| { format!("{}", snippet(cx, stmt.span, "..")) }).collect::>(); - if collect_expr { // if we have a statement which has a match, - match block.expr { // then collect the expression (without semicolon) below it - Some(ref expr) => other_stuff.push(format!("{}", snippet(cx, expr.span, ".."))), - None => (), + if inner_stmt_expr.is_some() { + // if we have a statement which has a match, + if let Some(ref expr) = block.expr { + // then collect the expression (without semicolon) below it + other_stuff.push(format!("{}", snippet(cx, expr.span, ".."))); } } @@ -180,12 +175,12 @@ impl LateLintPass for LoopsPass { is_break_expr(&arms[1].body) { if in_external_macro(cx, expr.span) { return; } - let loop_body = match inner_stmt_expr { + let loop_body = if inner_stmt_expr.is_some() { // FIXME: should probably be an ellipsis // tabbing and newline is probably a bad idea, especially for large blocks - Some(_) => Cow::Owned(format!("{{\n {}\n}}", other_stuff.join("\n "))), - None => expr_block(cx, &arms[0].body, - Some(other_stuff.join("\n ")), ".."), + Cow::Owned(format!("{{\n {}\n}}", other_stuff.join("\n "))) + } else { + expr_block(cx, &arms[0].body, Some(other_stuff.join("\n ")), "..") }; span_help_and_lint(cx, WHILE_LET_LOOP, expr.span, "this loop could be written as a `while let` loop", diff --git a/src/matches.rs b/src/matches.rs index 39459bafba7..460893d93ab 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -102,13 +102,11 @@ impl LateLintPass for MatchPass { if arms.len() == 2 && arms[0].pats.len() == 1 { // no guards let exprs = if let PatLit(ref arm_bool) = arms[0].pats[0].node { if let ExprLit(ref lit) = arm_bool.node { - if let LitBool(val) = lit.node { - if val { - Some((&*arms[0].body, &*arms[1].body)) - } else { - Some((&*arms[1].body, &*arms[0].body)) - } - } else { None } + match lit.node { + LitBool(true) => Some((&*arms[0].body, &*arms[1].body)), + LitBool(false) => Some((&*arms[1].body, &*arms[0].body)), + _ => None, + } } else { None } } else { None }; if let Some((ref true_expr, ref false_expr)) = exprs {