Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-10 07:04:18 +00:00
Code Issues Projects Releases Packages Wiki Activity

compute cyclomatic complexity (adjusted to not punish Rust's match)

Browse source
This commit is contained in:
Oliver Schneider 2015-11-18 12:35:18 +01:00
parent b45745e905
commit 617c820e6b
11 changed files with 485 additions and 127 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
.gitignore vendored
View file

@ -12,3 +12,6 @@
# We don't pin yet
Cargo.lock
# Generated by dogfood
/target_recur/

3
README.md
View file

@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
[Jump to usage instructions](#usage)
##Lints
There are 79 lints included in this crate:
There are 80 lints included in this crate:
name | default | meaning
---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@ -22,6 +22,7 @@ name
[cmp_nan](https://github.com/Manishearth/rust-clippy/wiki#cmp_nan) | deny | comparisons to NAN (which will always return false, which is probably not intended)
[cmp_owned](https://github.com/Manishearth/rust-clippy/wiki#cmp_owned) | warn | creating owned instances for comparing with others, e.g. `x == "foo".to_string()`
[collapsible_if](https://github.com/Manishearth/rust-clippy/wiki#collapsible_if) | warn | two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }`
[cyclomatic_complexity](https://github.com/Manishearth/rust-clippy/wiki#cyclomatic_complexity) | warn | finds functions that should be split up into multiple functions
[empty_loop](https://github.com/Manishearth/rust-clippy/wiki#empty_loop) | warn | empty `loop {}` detected
[eq_op](https://github.com/Manishearth/rust-clippy/wiki#eq_op) | warn | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`)
[explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do

105
src/cyclomatic_complexity.rs Normal file
View file

@ -0,0 +1,105 @@
//! calculate cyclomatic complexity and warn about overly complex functions
use rustc::lint::*;
use rustc_front::hir::*;
use rustc::middle::cfg::CFG;
use syntax::codemap::Span;
use syntax::attr::*;
use syntax::ast::Attribute;
use rustc_front::intravisit::{Visitor, walk_expr};
use utils::{in_macro, LimitStack};
declare_lint! { pub CYCLOMATIC_COMPLEXITY, Warn,
"finds functions that should be split up into multiple functions" }
pub struct CyclomaticComplexity {
limit: LimitStack,
}
impl CyclomaticComplexity {
pub fn new(limit: u64) -> Self {
CyclomaticComplexity {
limit: LimitStack::new(limit),
}
}
}
impl LintPass for CyclomaticComplexity {
fn get_lints(&self) -> LintArray {
lint_array!(CYCLOMATIC_COMPLEXITY)
}
}
impl CyclomaticComplexity {
fn check(&mut self, cx: &LateContext, 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;
let e = cfg.graph.len_edges() as u64;
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 {
println!("cc = {}, arms = {}", cc, narms);
println!("{:?}", block);
println!("{:?}", span);
panic!("cc = {}, arms = {}", cc, narms);
}
let rust_cc = cc - narms;
if rust_cc > self.limit.limit() {
cx.span_lint_help(CYCLOMATIC_COMPLEXITY, span,
&format!("The function has a cyclomatic complexity of {}.", rust_cc),
"You could split it up into multiple smaller functions");
}
}
}
impl LateLintPass for CyclomaticComplexity {
fn check_item(&mut self, cx: &LateContext, item: &Item) {
if let ItemFn(_, _, _, _, _, ref block) = item.node {
self.check(cx, block, item.span);
}
}
fn check_impl_item(&mut self, cx: &LateContext, item: &ImplItem) {
if let ImplItemKind::Method(_, ref block) = item.node {
self.check(cx, block, item.span);
}
}
fn check_trait_item(&mut self, cx: &LateContext, item: &TraitItem) {
if let MethodTraitItem(_, Some(ref block)) = item.node {
self.check(cx, block, item.span);
}
}
fn enter_lint_attrs(&mut self, cx: &LateContext, attrs: &[Attribute]) {
self.limit.push_attrs(cx.sess(), attrs, "cyclomatic_complexity");
}
fn exit_lint_attrs(&mut self, cx: &LateContext, attrs: &[Attribute]) {
self.limit.pop_attrs(cx.sess(), attrs, "cyclomatic_complexity");
}
}
struct MatchArmCounter(u64);
impl<'a> Visitor<'a> for MatchArmCounter {
fn visit_expr(&mut self, e: &'a Expr) {
match e.node {
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;
}
},
ExprClosure(..) => {},
_ => walk_expr(self, e),
}
}
}

5
src/lib.rs
View file

@ -1,6 +1,6 @@
#![feature(plugin_registrar, box_syntax)]
#![feature(rustc_private, core, collections)]
#![feature(num_bits_bytes)]
#![feature(num_bits_bytes, iter_arith)]
#![allow(unknown_lints)]
#[macro_use]
@ -59,6 +59,7 @@ pub mod needless_update;
pub mod no_effect;
pub mod temporary_assignment;
pub mod transmute;
pub mod cyclomatic_complexity;
mod reexport {
pub use syntax::ast::{Name, Ident, NodeId};
@ -110,6 +111,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_late_lint_pass(box map_clone::MapClonePass);
reg.register_late_lint_pass(box temporary_assignment::TemporaryAssignmentPass);
reg.register_late_lint_pass(box transmute::UselessTransmute);
reg.register_late_lint_pass(box cyclomatic_complexity::CyclomaticComplexity::new(25));
reg.register_lint_group("clippy_pedantic", vec![
methods::OPTION_UNWRAP_USED,
@ -138,6 +140,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR,
block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
collapsible_if::COLLAPSIBLE_IF,
cyclomatic_complexity::CYCLOMATIC_COMPLEXITY,
eq_op::EQ_OP,
eta_reduction::REDUNDANT_CLOSURE,
identity_op::IDENTITY_OP,

19
src/lifetimes.rs
View file

@ -108,7 +108,7 @@ fn could_use_elision(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf>
// no input lifetimes? easy case!
if input_lts.is_empty() {
return false;
false
} else if output_lts.is_empty() {
// no output lifetimes, check distinctness of input lifetimes
@ -117,9 +117,7 @@ fn could_use_elision(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf>
return false;
}
// we have no output reference, so we only need all distinct lifetimes
if input_lts.len() == unique_lifetimes(&input_lts) {
return true;
}
input_lts.len() == unique_lifetimes(&input_lts)
} else {
// we have output references, so we need one input reference,
// and all output lifetimes must be the same
@ -128,16 +126,17 @@ fn could_use_elision(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf>
}
if input_lts.len() == 1 {
match (&input_lts[0], &output_lts[0]) {
(&Named(n1), &Named(n2)) if n1 == n2 => { return true; }
(&Named(_), &Unnamed) => { return true; }
(&Unnamed, &Named(_)) => { return true; }
_ => { } // already elided, different named lifetimes
(&Named(n1), &Named(n2)) if n1 == n2 => true,
(&Named(_), &Unnamed) => true,
(&Unnamed, &Named(_)) => true,
_ => false // already elided, different named lifetimes
// or something static going on
}
}
}
} else {
false
}
}
}
fn allowed_lts_from(named_lts: &[LifetimeDef]) -> HashSet<RefLt> {
let mut allowed_lts = HashSet::new();

210
src/loops.rs
View file

@ -56,6 +56,113 @@ impl LintPass for LoopsPass {
impl LateLintPass for LoopsPass {
fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
if let Some((pat, arg, body)) = recover_for_loop(expr) {
check_for_loop(cx, pat, arg, body, expr);
}
// check for `loop { if let {} else break }` that could be `while let`
// (also matches an explicit "match" instead of "if let")
// (even if the "match" or "if let" is used for declaration)
if let ExprLoop(ref block, _) = expr.node {
// also check for empty `loop {}` statements
if block.stmts.is_empty() && block.expr.is_none() {
span_lint(cx, EMPTY_LOOP, expr.span,
"empty `loop {}` detected. You may want to either \
use `panic!()` or add `std::thread::sleep(..);` to \
the loop body.");
}
// 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 {
if let ExprMatch(ref matchexpr, ref arms, ref source) = inner.node {
// collect the remaining statements below the match
let mut other_stuff = block.stmts
.iter()
.skip(1)
.map(|stmt| {
format!("{}", snippet(cx, stmt.span, ".."))
}).collect::<Vec<String>>();
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 => (),
}
}
// ensure "if let" compatible match structure
match *source {
MatchSource::Normal | MatchSource::IfLetDesugar{..} => if
arms.len() == 2 &&
arms[0].pats.len() == 1 && arms[0].guard.is_none() &&
arms[1].pats.len() == 1 && arms[1].guard.is_none() &&
// finally, check for "break" in the second clause
is_break_expr(&arms[1].body)
{
if in_external_macro(cx, expr.span) { return; }
let loop_body = match inner_stmt_expr {
// 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 ")), ".."),
};
span_help_and_lint(cx, WHILE_LET_LOOP, expr.span,
"this loop could be written as a `while let` loop",
&format!("try\nwhile let {} = {} {}",
snippet(cx, arms[0].pats[0].span, ".."),
snippet(cx, matchexpr.span, ".."),
loop_body));
},
_ => ()
}
}
}
}
if let ExprMatch(ref match_expr, ref arms, MatchSource::WhileLetDesugar) = expr.node {
let pat = &arms[0].pats[0].node;
if let (&PatEnum(ref path, Some(ref pat_args)),
&ExprMethodCall(method_name, _, ref method_args)) =
(pat, &match_expr.node) {
let iter_expr = &method_args[0];
if let Some(lhs_constructor) = path.segments.last() {
if method_name.node.as_str() == "next" &&
match_trait_method(cx, match_expr, &["core", "iter", "Iterator"]) &&
lhs_constructor.identifier.name.as_str() == "Some" &&
!is_iterator_used_after_while_let(cx, iter_expr) {
let iterator = snippet(cx, method_args[0].span, "_");
let loop_var = snippet(cx, pat_args[0].span, "_");
span_help_and_lint(cx, WHILE_LET_ON_ITERATOR, expr.span,
"this loop could be written as a `for` loop",
&format!("try\nfor {} in {} {{...}}",
loop_var,
iterator));
}
}
}
}
}
fn check_stmt(&mut self, cx: &LateContext, stmt: &Stmt) {
if let StmtSemi(ref expr, _) = stmt.node {
if let ExprMethodCall(ref method, _, ref args) = expr.node {
if args.len() == 1 && method.node.as_str() == "collect" &&
match_trait_method(cx, expr, &["core", "iter", "Iterator"]) {
span_lint(cx, UNUSED_COLLECT, expr.span, &format!(
"you are collect()ing an iterator and throwing away the result. \
Consider using an explicit for loop to exhaust the iterator"));
}
}
}
}
}
fn check_for_loop(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &Expr) {
// check for looping over a range and then indexing a sequence with it
// -> the iteratee must be a range literal
if let ExprRange(Some(ref l), _) = arg.node {
@ -167,109 +274,6 @@ impl LateLintPass for LoopsPass {
}
}
}
// check for `loop { if let {} else break }` that could be `while let`
// (also matches an explicit "match" instead of "if let")
// (even if the "match" or "if let" is used for declaration)
if let ExprLoop(ref block, _) = expr.node {
// also check for empty `loop {}` statements
if block.stmts.is_empty() && block.expr.is_none() {
span_lint(cx, EMPTY_LOOP, expr.span,
"empty `loop {}` detected. You may want to either \
use `panic!()` or add `std::thread::sleep(..);` to \
the loop body.");
}
// 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 {
if let ExprMatch(ref matchexpr, ref arms, ref source) = inner.node {
// collect the remaining statements below the match
let mut other_stuff = block.stmts
.iter()
.skip(1)
.map(|stmt| {
format!("{}", snippet(cx, stmt.span, ".."))
}).collect::<Vec<String>>();
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 => (),
}
}
// ensure "if let" compatible match structure
match *source {
MatchSource::Normal | MatchSource::IfLetDesugar{..} => if
arms.len() == 2 &&
arms[0].pats.len() == 1 && arms[0].guard.is_none() &&
arms[1].pats.len() == 1 && arms[1].guard.is_none() &&
// finally, check for "break" in the second clause
is_break_expr(&arms[1].body)
{
if in_external_macro(cx, expr.span) { return; }
let loop_body = match inner_stmt_expr {
// 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 ")), ".."),
};
span_help_and_lint(cx, WHILE_LET_LOOP, expr.span,
"this loop could be written as a `while let` loop",
&format!("try\nwhile let {} = {} {}",
snippet(cx, arms[0].pats[0].span, ".."),
snippet(cx, matchexpr.span, ".."),
loop_body));
},
_ => ()
}
}
}
}
if let ExprMatch(ref match_expr, ref arms, MatchSource::WhileLetDesugar) = expr.node {
let pat = &arms[0].pats[0].node;
if let (&PatEnum(ref path, Some(ref pat_args)),
&ExprMethodCall(method_name, _, ref method_args)) =
(pat, &match_expr.node) {
let iter_expr = &method_args[0];
if let Some(lhs_constructor) = path.segments.last() {
if method_name.node.as_str() == "next" &&
match_trait_method(cx, match_expr, &["core", "iter", "Iterator"]) &&
lhs_constructor.identifier.name.as_str() == "Some" &&
!is_iterator_used_after_while_let(cx, iter_expr) {
let iterator = snippet(cx, method_args[0].span, "_");
let loop_var = snippet(cx, pat_args[0].span, "_");
span_help_and_lint(cx, WHILE_LET_ON_ITERATOR, expr.span,
"this loop could be written as a `for` loop",
&format!("try\nfor {} in {} {{...}}",
loop_var,
iterator));
}
}
}
}
}
fn check_stmt(&mut self, cx: &LateContext, stmt: &Stmt) {
if let StmtSemi(ref expr, _) = stmt.node {
if let ExprMethodCall(ref method, _, ref args) = expr.node {
if args.len() == 1 && method.node.as_str() == "collect" &&
match_trait_method(cx, expr, &["core", "iter", "Iterator"]) {
span_lint(cx, UNUSED_COLLECT, expr.span, &format!(
"you are collect()ing an iterator and throwing away the result. \
Consider using an explicit for loop to exhaust the iterator"));
}
}
}
}
}
/// Recover the essential nodes of a desugared for loop:
/// `for pat in arg { body }` becomes `(pat, arg, body)`.

66
src/utils.rs
View file

@ -9,6 +9,9 @@ use std::borrow::Cow;
use syntax::ast::Lit_::*;
use syntax::ast;
use rustc::session::Session;
use std::str::FromStr;
// module DefPaths for certain structs/enums we check for
pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"];
pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"];
@ -65,8 +68,8 @@ macro_rules! if_let_chain {
};
}
/// returns true this expn_info was expanded by any macro
pub fn in_macro(cx: &LateContext, span: Span) -> bool {
/// returns true if this expn_info was expanded by any macro
pub fn in_macro<T: LintContext>(cx: &T, span: Span) -> bool {
cx.sess().codemap().with_expn_info(span.expn_id,
|info| info.is_some())
}
@ -397,3 +400,62 @@ macro_rules! if_let_chain {
}
};
}
pub struct LimitStack {
stack: Vec<u64>,
}
impl Drop for LimitStack {
fn drop(&mut self) {
assert_eq!(self.stack.len(), 1);
}
}
impl LimitStack {
pub fn new(limit: u64) -> LimitStack {
LimitStack {
stack: vec![limit],
}
}
pub fn limit(&self) -> u64 {
*self.stack.last().expect("there should always be a value in the stack")
}
pub fn push_attrs(&mut self, sess: &Session, attrs: &[ast::Attribute], name: &'static str) {
let stack = &mut self.stack;
parse_attrs(
sess,
attrs,
name,
|val| stack.push(val),
);
}
pub fn pop_attrs(&mut self, sess: &Session, attrs: &[ast::Attribute], name: &'static str) {
let stack = &mut self.stack;
parse_attrs(
sess,
attrs,
name,
|val| assert_eq!(stack.pop(), Some(val)),
);
}
}
fn parse_attrs<F: FnMut(u64)>(sess: &Session, attrs: &[ast::Attribute], name: &'static str, mut f: F) {
for attr in attrs {
let attr = &attr.node;
if attr.is_sugared_doc { continue; }
if let ast::MetaNameValue(ref key, ref value) = attr.value.node {
if *key == name {
if let LitStr(ref s, _) = value.node {
if let Ok(value) = FromStr::from_str(s) {
f(value)
} else {
sess.span_err(value.span, "not a number");
}
} else {
unreachable!()
}
}
}
}
}

181
tests/compile-fail/cyclomatic_complexity.rs Normal file
View file

@ -0,0 +1,181 @@
#![feature(plugin, custom_attribute)]
#![plugin(clippy)]
#![deny(clippy)]
#![deny(cyclomatic_complexity)]
#![allow(unused)]
fn main() { //~ ERROR: The function has a cyclomatic complexity of 28.
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
if true {
println!("a");
}
}
#[cyclomatic_complexity = "0"]
fn kaboom() { //~ ERROR: The function has a cyclomatic complexity of 6
let n = 0;
'a: for i in 0..20 {
'b: for j in i..20 {
for k in j..20 {
if k == 5 {
break 'b;
}
if j == 3 && k == 6 {
continue 'a;
}
if k == j {
continue;
}
println!("bake");
}
}
println!("cake");
}
}
fn bloo() {
match 42 {
0 => println!("hi"),
1 => println!("hai"),
2 => println!("hey"),
3 => println!("hallo"),
4 => println!("hello"),
5 => println!("salut"),
6 => println!("good morning"),
7 => println!("good evening"),
8 => println!("good afternoon"),
9 => println!("good night"),
10 => println!("bonjour"),
11 => println!("hej"),
12 => println!("hej hej"),
13 => println!("greetings earthling"),
14 => println!("take us to you leader"),
15 | 17 | 19 | 21 | 23 | 25 | 27 | 29 | 31 | 33 => println!("take us to you leader"),
35 | 37 | 39 | 41 | 43 | 45 | 47 | 49 | 51 | 53 => println!("there is no undefined behavior"),
55 | 57 | 59 | 61 | 63 | 65 | 67 | 69 | 71 | 73 => println!("I know borrow-fu"),
_ => println!("bye"),
}
}
#[cyclomatic_complexity = "0"]
fn baa() { //~ ERROR: The function has a cyclomatic complexity of 2
let x = || match 99 {
0 => true,
1 => false,
2 => true,
4 => true,
6 => true,
9 => true,
_ => false,
};
if x() {
println!("x");
} else {
println!("not x");
}
}
#[cyclomatic_complexity = "0"]
fn bar() { //~ ERROR: The function has a cyclomatic complexity of 2
match 99 {
0 => println!("hi"),
_ => println!("bye"),
}
}
#[cyclomatic_complexity = "0"]
fn barr() { //~ ERROR: The function has a cyclomatic complexity of 2
match 99 {
0 => println!("hi"),
1 => println!("bla"),
2 | 3 => println!("blub"),
_ => println!("bye"),
}
}
enum Void {}
#[cyclomatic_complexity = "0"]
fn void(void: Void) { //~ ERROR: The function has a cyclomatic complexity of 1
if true {
match void {
}
}
}

2
tests/compile-fail/for_loop.rs
View file

@ -16,7 +16,7 @@ impl Unrelated {
#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)]
#[deny(unused_collect)]
#[allow(linkedlist,shadow_unrelated,unnecessary_mut_passed)]
#[allow(linkedlist,shadow_unrelated,unnecessary_mut_passed, cyclomatic_complexity)]
fn main() {
let mut vec = vec![1, 2, 3, 4];
let vec2 = vec![1, 2, 3, 4];

2
tests/compile-fail/while_loop.rs
View file

@ -2,7 +2,7 @@
#![plugin(clippy)]
#![deny(while_let_loop, empty_loop, while_let_on_iterator)]
#![allow(dead_code, unused)]
#![allow(dead_code, unused, cyclomatic_complexity)]
fn main() {
let y = Some(true);