From a2477f775973e6625a1dd322e8934199beec02e0 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 8 Sep 2019 11:39:42 +0100 Subject: [PATCH] Remove use of the HIR CFG --- clippy_lints/src/cognitive_complexity.rs | 156 ++++------------------- tests/ui/cognitive_complexity.rs | 42 +++--- tests/ui/cognitive_complexity.stderr | 117 ++++------------- 3 files changed, 75 insertions(+), 240 deletions(-) diff --git a/clippy_lints/src/cognitive_complexity.rs b/clippy_lints/src/cognitive_complexity.rs index fccec347ef0..3e7f3c980fc 100644 --- a/clippy_lints/src/cognitive_complexity.rs +++ b/clippy_lints/src/cognitive_complexity.rs @@ -1,15 +1,13 @@ //! calculate cognitive complexity and warn about overly complex functions -use rustc::cfg::CFG; use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use rustc::hir::*; use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass}; -use rustc::ty; use rustc::{declare_tool_lint, impl_lint_pass}; use syntax::ast::Attribute; use syntax::source_map::Span; -use crate::utils::{is_allowed, match_type, paths, span_help_and_lint, LimitStack}; +use crate::utils::{match_type, paths, span_help_and_lint, LimitStack}; declare_clippy_lint! { /// **What it does:** Checks for methods with high cognitive complexity. @@ -46,30 +44,11 @@ impl CognitiveComplexity { return; } - let cfg = CFG::new(cx.tcx, body); let expr = &body.value; - let n = cfg.graph.len_nodes() as u64; - let e = cfg.graph.len_edges() as u64; - if e + 2 < n { - // the function has unreachable code, other lints should catch this - return; - } - let cc = e + 2 - n; - let mut helper = CCHelper { - match_arms: 0, - divergence: 0, - short_circuits: 0, - returns: 0, - cx, - }; + + let mut helper = CCHelper { cc: 1, returns: 0 }; helper.visit_expr(expr); - let CCHelper { - match_arms, - divergence, - short_circuits, - returns, - .. - } = helper; + let CCHelper { cc, returns } = helper; let ret_ty = cx.tables.node_type(expr.hir_id); let ret_adjust = if match_type(cx, ret_ty, &paths::RESULT) { returns @@ -78,36 +57,23 @@ impl CognitiveComplexity { (returns / 2) }; - if cc + divergence < match_arms + short_circuits { - report_cc_bug( + let mut rust_cc = cc; + // prevent degenerate cases where unreachable code contains `return` statements + if rust_cc >= ret_adjust { + rust_cc -= ret_adjust; + } + if rust_cc > self.limit.limit() { + span_help_and_lint( cx, - cc, - match_arms, - divergence, - short_circuits, - ret_adjust, + COGNITIVE_COMPLEXITY, span, - body.id().hir_id, + &format!( + "the function has a cognitive complexity of ({}/{})", + rust_cc, + self.limit.limit() + ), + "you could split it up into multiple smaller functions", ); - } else { - let mut rust_cc = cc + divergence - match_arms - short_circuits; - // prevent degenerate cases where unreachable code contains `return` statements - if rust_cc >= ret_adjust { - rust_cc -= ret_adjust; - } - if rust_cc > self.limit.limit() { - span_help_and_lint( - cx, - COGNITIVE_COMPLEXITY, - span, - &format!( - "the function has a cognitive complexity of ({}/{})", - rust_cc, - self.limit.limit() - ), - "you could split it up into multiple smaller functions", - ); - } } } } @@ -136,99 +102,27 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CognitiveComplexity { } } -struct CCHelper<'a, 'tcx> { - match_arms: u64, - divergence: u64, +struct CCHelper { + cc: u64, returns: u64, - short_circuits: u64, // && and || - cx: &'a LateContext<'a, 'tcx>, } -impl<'a, 'tcx> Visitor<'tcx> for CCHelper<'a, 'tcx> { +impl<'tcx> Visitor<'tcx> for CCHelper { fn visit_expr(&mut self, e: &'tcx Expr) { + walk_expr(self, e); match e.node { ExprKind::Match(_, ref arms, _) => { - walk_expr(self, e); let arms_n: u64 = arms.iter().map(|arm| arm.pats.len() as u64).sum(); if arms_n > 1 { - self.match_arms += arms_n - 2; - } - }, - ExprKind::Call(ref callee, _) => { - walk_expr(self, e); - let ty = self.cx.tables.node_type(callee.hir_id); - match ty.sty { - ty::FnDef(..) | ty::FnPtr(_) => { - let sig = ty.fn_sig(self.cx.tcx); - if sig.skip_binder().output().sty == ty::Never { - self.divergence += 1; - } - }, - _ => (), - } - }, - ExprKind::Closure(.., _) => (), - ExprKind::Binary(op, _, _) => { - walk_expr(self, e); - match op.node { - BinOpKind::And | BinOpKind::Or => self.short_circuits += 1, - _ => (), + self.cc += 1; } + self.cc += arms.iter().filter(|arm| arm.guard.is_some()).count() as u64; }, ExprKind::Ret(_) => self.returns += 1, - _ => walk_expr(self, e), + _ => {}, } } fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { NestedVisitorMap::None } } - -#[cfg(feature = "debugging")] -#[allow(clippy::too_many_arguments)] -fn report_cc_bug( - _: &LateContext<'_, '_>, - cc: u64, - narms: u64, - div: u64, - shorts: u64, - returns: u64, - span: Span, - _: HirId, -) { - span_bug!( - span, - "Clippy encountered a bug calculating cognitive complexity: cc = {}, arms = {}, \ - div = {}, shorts = {}, returns = {}. Please file a bug report.", - cc, - narms, - div, - shorts, - returns - ); -} -#[cfg(not(feature = "debugging"))] -#[allow(clippy::too_many_arguments)] -fn report_cc_bug( - cx: &LateContext<'_, '_>, - cc: u64, - narms: u64, - div: u64, - shorts: u64, - returns: u64, - span: Span, - id: HirId, -) { - if !is_allowed(cx, COGNITIVE_COMPLEXITY, id) { - cx.sess().span_note_without_error( - span, - &format!( - "Clippy encountered a bug calculating cognitive complexity \ - (hide this message with `#[allow(cognitive_complexity)]`): \ - cc = {}, arms = {}, div = {}, shorts = {}, returns = {}. \ - Please file a bug report.", - cc, narms, div, shorts, returns - ), - ); - } -} diff --git a/tests/ui/cognitive_complexity.rs b/tests/ui/cognitive_complexity.rs index 7c81cc73d3c..0f7857a45d1 100644 --- a/tests/ui/cognitive_complexity.rs +++ b/tests/ui/cognitive_complexity.rs @@ -87,7 +87,7 @@ fn main() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn kaboom() { let n = 0; 'a: for i in 0..20 { @@ -133,17 +133,19 @@ fn bloo() { } } -#[clippy::cognitive_complexity = "0"] +// Short circuiting operations don't increase the complexity of a function. +// Note that the minimum complexity of a function is 1. +#[clippy::cognitive_complexity = "1"] fn lots_of_short_circuits() -> bool { true && false && true && false && true && false && true } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn lots_of_short_circuits2() -> bool { true || false || true || false || true || false || true } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn baa() { let x = || match 99 { 0 => 0, @@ -161,7 +163,7 @@ fn baa() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn bar() { match 99 { 0 => println!("hi"), @@ -170,7 +172,7 @@ fn bar() { } #[test] -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] /// Tests are usually complex but simple at the same time. `clippy::cognitive_complexity` used to /// give lots of false-positives in tests. fn dont_warn_on_tests() { @@ -180,7 +182,7 @@ fn dont_warn_on_tests() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn barr() { match 99 { 0 => println!("hi"), @@ -190,7 +192,7 @@ fn barr() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn barr2() { match 99 { 0 => println!("hi"), @@ -206,7 +208,7 @@ fn barr2() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn barrr() { match 99 { 0 => println!("hi"), @@ -216,7 +218,7 @@ fn barrr() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn barrr2() { match 99 { 0 => println!("hi"), @@ -232,7 +234,7 @@ fn barrr2() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn barrrr() { match 99 { 0 => println!("hi"), @@ -242,7 +244,7 @@ fn barrrr() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn barrrr2() { match 99 { 0 => println!("hi"), @@ -258,7 +260,7 @@ fn barrrr2() { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn cake() { if 4 == 5 { println!("yea"); @@ -268,7 +270,7 @@ fn cake() { println!("whee"); } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] pub fn read_file(input_path: &str) -> String { use std::fs::File; use std::io::{Read, Write}; @@ -299,20 +301,20 @@ pub fn read_file(input_path: &str) -> String { enum Void {} -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn void(void: Void) { if true { match void {} } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn mcarton_sees_all() { panic!("meh"); panic!("möh"); } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn try_() -> Result { match 5 { 5 => Ok(5), @@ -320,7 +322,7 @@ fn try_() -> Result { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn try_again() -> Result { let _ = Ok(42)?; let _ = Ok(43)?; @@ -336,7 +338,7 @@ fn try_again() -> Result { } } -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn early() -> Result { return Ok(5); return Ok(5); @@ -350,7 +352,7 @@ fn early() -> Result { } #[rustfmt::skip] -#[clippy::cognitive_complexity = "0"] +#[clippy::cognitive_complexity = "1"] fn early_ret() -> i32 { let a = if true { 42 } else { return 0; }; let a = if a < 99 { 42 } else { return 0; }; diff --git a/tests/ui/cognitive_complexity.stderr b/tests/ui/cognitive_complexity.stderr index 32a56f00c5d..1fca4f5bd82 100644 --- a/tests/ui/cognitive_complexity.stderr +++ b/tests/ui/cognitive_complexity.stderr @@ -13,7 +13,7 @@ LL | | } = note: `-D clippy::cognitive-complexity` implied by `-D warnings` = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (7/0) +error: the function has a cognitive complexity of (7/1) --> $DIR/cognitive_complexity.rs:91:1 | LL | / fn kaboom() { @@ -27,28 +27,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (1/0) - --> $DIR/cognitive_complexity.rs:137:1 - | -LL | / fn lots_of_short_circuits() -> bool { -LL | | true && false && true && false && true && false && true -LL | | } - | |_^ - | - = help: you could split it up into multiple smaller functions - -error: the function has a cognitive complexity of (1/0) - --> $DIR/cognitive_complexity.rs:142:1 - | -LL | / fn lots_of_short_circuits2() -> bool { -LL | | true || false || true || false || true || false || true -LL | | } - | |_^ - | - = help: you could split it up into multiple smaller functions - -error: the function has a cognitive complexity of (2/0) - --> $DIR/cognitive_complexity.rs:147:1 +error: the function has a cognitive complexity of (2/1) + --> $DIR/cognitive_complexity.rs:149:1 | LL | / fn baa() { LL | | let x = || match 99 { @@ -61,8 +41,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (2/0) - --> $DIR/cognitive_complexity.rs:148:13 +error: the function has a cognitive complexity of (2/1) + --> $DIR/cognitive_complexity.rs:150:13 | LL | let x = || match 99 { | _____________^ @@ -76,8 +56,8 @@ LL | | }; | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (2/0) - --> $DIR/cognitive_complexity.rs:165:1 +error: the function has a cognitive complexity of (2/1) + --> $DIR/cognitive_complexity.rs:167:1 | LL | / fn bar() { LL | | match 99 { @@ -89,8 +69,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (2/0) - --> $DIR/cognitive_complexity.rs:184:1 +error: the function has a cognitive complexity of (2/1) + --> $DIR/cognitive_complexity.rs:186:1 | LL | / fn barr() { LL | | match 99 { @@ -103,8 +83,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (3/0) - --> $DIR/cognitive_complexity.rs:194:1 +error: the function has a cognitive complexity of (3/1) + --> $DIR/cognitive_complexity.rs:196:1 | LL | / fn barr2() { LL | | match 99 { @@ -117,8 +97,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (2/0) - --> $DIR/cognitive_complexity.rs:210:1 +error: the function has a cognitive complexity of (2/1) + --> $DIR/cognitive_complexity.rs:212:1 | LL | / fn barrr() { LL | | match 99 { @@ -131,8 +111,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (3/0) - --> $DIR/cognitive_complexity.rs:220:1 +error: the function has a cognitive complexity of (3/1) + --> $DIR/cognitive_complexity.rs:222:1 | LL | / fn barrr2() { LL | | match 99 { @@ -145,8 +125,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (2/0) - --> $DIR/cognitive_complexity.rs:236:1 +error: the function has a cognitive complexity of (2/1) + --> $DIR/cognitive_complexity.rs:238:1 | LL | / fn barrrr() { LL | | match 99 { @@ -159,8 +139,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (3/0) - --> $DIR/cognitive_complexity.rs:246:1 +error: the function has a cognitive complexity of (3/1) + --> $DIR/cognitive_complexity.rs:248:1 | LL | / fn barrrr2() { LL | | match 99 { @@ -173,8 +153,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (2/0) - --> $DIR/cognitive_complexity.rs:262:1 +error: the function has a cognitive complexity of (2/1) + --> $DIR/cognitive_complexity.rs:264:1 | LL | / fn cake() { LL | | if 4 == 5 { @@ -187,8 +167,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (4/0) - --> $DIR/cognitive_complexity.rs:272:1 +error: the function has a cognitive complexity of (4/1) + --> $DIR/cognitive_complexity.rs:274:1 | LL | / pub fn read_file(input_path: &str) -> String { LL | | use std::fs::File; @@ -201,8 +181,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (1/0) - --> $DIR/cognitive_complexity.rs:303:1 +error: the function has a cognitive complexity of (2/1) + --> $DIR/cognitive_complexity.rs:305:1 | LL | / fn void(void: Void) { LL | | if true { @@ -213,49 +193,8 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: the function has a cognitive complexity of (1/0) - --> $DIR/cognitive_complexity.rs:316:1 - | -LL | / fn try_() -> Result { -LL | | match 5 { -LL | | 5 => Ok(5), -LL | | _ => return Err("bla"), -LL | | } -LL | | } - | |_^ - | - = help: you could split it up into multiple smaller functions - -error: the function has a cognitive complexity of (1/0) - --> $DIR/cognitive_complexity.rs:324:1 - | -LL | / fn try_again() -> Result { -LL | | let _ = Ok(42)?; -LL | | let _ = Ok(43)?; -LL | | let _ = Ok(44)?; -... | -LL | | } -LL | | } - | |_^ - | - = help: you could split it up into multiple smaller functions - -error: the function has a cognitive complexity of (1/0) - --> $DIR/cognitive_complexity.rs:340:1 - | -LL | / fn early() -> Result { -LL | | return Ok(5); -LL | | return Ok(5); -LL | | return Ok(5); -... | -LL | | return Ok(5); -LL | | } - | |_^ - | - = help: you could split it up into multiple smaller functions - -error: the function has a cognitive complexity of (8/0) - --> $DIR/cognitive_complexity.rs:354:1 +error: the function has a cognitive complexity of (8/1) + --> $DIR/cognitive_complexity.rs:356:1 | LL | / fn early_ret() -> i32 { LL | | let a = if true { 42 } else { return 0; }; @@ -268,5 +207,5 @@ LL | | } | = help: you could split it up into multiple smaller functions -error: aborting due to 20 previous errors +error: aborting due to 15 previous errors