diff --git a/.gitignore b/.gitignore index ac98a7d842f..acb3c020fe7 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,6 @@ # We don't pin yet Cargo.lock + +# Generated by dogfood +/target_recur/ diff --git a/README.md b/README.md index 0a10c849ca7..206d86ec1ee 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/src/cyclomatic_complexity.rs b/src/cyclomatic_complexity.rs new file mode 100644 index 00000000000..678ebe866cb --- /dev/null +++ b/src/cyclomatic_complexity.rs @@ -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), + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 7f3f9009411..57a92f99900 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,8 +1,12 @@ #![feature(plugin_registrar, box_syntax)] #![feature(rustc_private, core, collections)] -#![feature(num_bits_bytes)] +#![feature(num_bits_bytes, iter_arith)] #![allow(unknown_lints)] +// this only exists to allow the "dogfood" integration test to work +#[allow(dead_code)] +fn main() { println!("What are you doing? Don't run clippy as an executable"); } + #[macro_use] extern crate syntax; #[macro_use] @@ -59,6 +63,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 +115,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 +144,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, diff --git a/src/lifetimes.rs b/src/lifetimes.rs index acc7b014052..97eb7fa67a3 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -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,15 +126,16 @@ 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 - // or something static going on + (&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 } } - false } fn allowed_lts_from(named_lts: &[LifetimeDef]) -> HashSet { diff --git a/src/loops.rs b/src/loops.rs index cfde1741715..393c92b16ef 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -56,116 +56,7 @@ 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 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 { - // Range should start with `0` - if let ExprLit(ref lit) = l.node { - if let LitInt(0, _) = lit.node { - - // the var must be a single name - if let PatIdent(_, ref ident, _) = pat.node { - let mut visitor = VarVisitor { cx: cx, var: ident.node.name, - indexed: HashSet::new(), nonindex: false }; - walk_expr(&mut visitor, body); - // linting condition: we only indexed one variable - if visitor.indexed.len() == 1 { - let indexed = visitor.indexed.into_iter().next().expect( - "Len was nonzero, but no contents found"); - if visitor.nonindex { - span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!( - "the loop variable `{}` is used to index `{}`. Consider using \ - `for ({}, item) in {}.iter().enumerate()` or similar iterators", - ident.node.name, indexed, ident.node.name, indexed)); - } else { - span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!( - "the loop variable `{}` is only used to index `{}`. \ - Consider using `for item in &{}` or similar iterators", - ident.node.name, indexed, indexed)); - } - } - } - } - } - } - - // if this for loop is iterating over a two-sided range... - if let ExprRange(Some(ref start_expr), Some(ref stop_expr)) = arg.node { - // ...and both sides are compile-time constant integers... - if let Some(start_idx @ Constant::ConstantInt(..)) = constant_simple(start_expr) { - if let Some(stop_idx @ Constant::ConstantInt(..)) = constant_simple(stop_expr) { - // ...and the start index is greater than the stop index, - // this loop will never run. This is often confusing for developers - // who think that this will iterate from the larger value to the - // smaller value. - if start_idx > stop_idx { - span_help_and_lint(cx, REVERSE_RANGE_LOOP, expr.span, - "this range is empty so this for loop will never run", - &format!("Consider using `({}..{}).rev()` if you are attempting to \ - iterate over this range in reverse", stop_idx, start_idx)); - } else if start_idx == stop_idx { - // if they are equal, it's also problematic - this loop - // will never run. - span_lint(cx, REVERSE_RANGE_LOOP, expr.span, - "this range is empty so this for loop will never run"); - } - } - } - } - - if let ExprMethodCall(ref method, _, ref args) = arg.node { - // just the receiver, no arguments - if args.len() == 1 { - let method_name = method.node; - // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x - if method_name.as_str() == "iter" || method_name.as_str() == "iter_mut" { - if is_ref_iterable_type(cx, &args[0]) { - let object = snippet(cx, args[0].span, "_"); - span_lint(cx, EXPLICIT_ITER_LOOP, expr.span, &format!( - "it is more idiomatic to loop over `&{}{}` instead of `{}.{}()`", - if method_name.as_str() == "iter_mut" { "mut " } else { "" }, - object, object, method_name)); - } - } - // check for looping over Iterator::next() which is not what you want - else if method_name.as_str() == "next" && - match_trait_method(cx, arg, &["core", "iter", "Iterator"]) { - span_lint(cx, ITER_NEXT_LOOP, expr.span, - "you are iterating over `Iterator::next()` which is an Option; \ - this will compile but is probably not what you want"); - } - } - } - - // Look for variables that are incremented once per loop iteration. - let mut visitor = IncrementVisitor { cx: cx, states: HashMap::new(), depth: 0, done: false }; - walk_expr(&mut visitor, body); - - // For each candidate, check the parent block to see if - // it's initialized to zero at the start of the loop. - let map = &cx.tcx.map; - let parent_scope = map.get_enclosing_scope(expr.id).and_then(|id| map.get_enclosing_scope(id) ); - if let Some(parent_id) = parent_scope { - if let NodeBlock(block) = map.get(parent_id) { - for (id, _) in visitor.states.iter().filter( |&(_,v)| *v == VarState::IncrOnce) { - let mut visitor2 = InitializeVisitor { cx: cx, end_expr: expr, var_id: id.clone(), - state: VarState::IncrOnce, name: None, - depth: 0, - past_loop: false }; - walk_block(&mut visitor2, block); - - if visitor2.state == VarState::Warn { - if let Some(name) = visitor2.name { - span_lint(cx, EXPLICIT_COUNTER_LOOP, expr.span, - &format!("the variable `{0}` is used as a loop counter. Consider \ - using `for ({0}, item) in {1}.enumerate()` \ - or similar iterators", - name, snippet(cx, arg.span, "_"))); - } - } - } - } - } + 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") @@ -271,6 +162,119 @@ impl LateLintPass for LoopsPass { } } +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 { + // Range should start with `0` + if let ExprLit(ref lit) = l.node { + if let LitInt(0, _) = lit.node { + + // the var must be a single name + if let PatIdent(_, ref ident, _) = pat.node { + let mut visitor = VarVisitor { cx: cx, var: ident.node.name, + indexed: HashSet::new(), nonindex: false }; + walk_expr(&mut visitor, body); + // linting condition: we only indexed one variable + if visitor.indexed.len() == 1 { + let indexed = visitor.indexed.into_iter().next().expect( + "Len was nonzero, but no contents found"); + if visitor.nonindex { + span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!( + "the loop variable `{}` is used to index `{}`. Consider using \ + `for ({}, item) in {}.iter().enumerate()` or similar iterators", + ident.node.name, indexed, ident.node.name, indexed)); + } else { + span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!( + "the loop variable `{}` is only used to index `{}`. \ + Consider using `for item in &{}` or similar iterators", + ident.node.name, indexed, indexed)); + } + } + } + } + } + } + + // if this for loop is iterating over a two-sided range... + if let ExprRange(Some(ref start_expr), Some(ref stop_expr)) = arg.node { + // ...and both sides are compile-time constant integers... + if let Some(start_idx @ Constant::ConstantInt(..)) = constant_simple(start_expr) { + if let Some(stop_idx @ Constant::ConstantInt(..)) = constant_simple(stop_expr) { + // ...and the start index is greater than the stop index, + // this loop will never run. This is often confusing for developers + // who think that this will iterate from the larger value to the + // smaller value. + if start_idx > stop_idx { + span_help_and_lint(cx, REVERSE_RANGE_LOOP, expr.span, + "this range is empty so this for loop will never run", + &format!("Consider using `({}..{}).rev()` if you are attempting to \ + iterate over this range in reverse", stop_idx, start_idx)); + } else if start_idx == stop_idx { + // if they are equal, it's also problematic - this loop + // will never run. + span_lint(cx, REVERSE_RANGE_LOOP, expr.span, + "this range is empty so this for loop will never run"); + } + } + } + } + + if let ExprMethodCall(ref method, _, ref args) = arg.node { + // just the receiver, no arguments + if args.len() == 1 { + let method_name = method.node; + // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x + if method_name.as_str() == "iter" || method_name.as_str() == "iter_mut" { + if is_ref_iterable_type(cx, &args[0]) { + let object = snippet(cx, args[0].span, "_"); + span_lint(cx, EXPLICIT_ITER_LOOP, expr.span, &format!( + "it is more idiomatic to loop over `&{}{}` instead of `{}.{}()`", + if method_name.as_str() == "iter_mut" { "mut " } else { "" }, + object, object, method_name)); + } + } + // check for looping over Iterator::next() which is not what you want + else if method_name.as_str() == "next" && + match_trait_method(cx, arg, &["core", "iter", "Iterator"]) { + span_lint(cx, ITER_NEXT_LOOP, expr.span, + "you are iterating over `Iterator::next()` which is an Option; \ + this will compile but is probably not what you want"); + } + } + } + + // Look for variables that are incremented once per loop iteration. + let mut visitor = IncrementVisitor { cx: cx, states: HashMap::new(), depth: 0, done: false }; + walk_expr(&mut visitor, body); + + // For each candidate, check the parent block to see if + // it's initialized to zero at the start of the loop. + let map = &cx.tcx.map; + let parent_scope = map.get_enclosing_scope(expr.id).and_then(|id| map.get_enclosing_scope(id) ); + if let Some(parent_id) = parent_scope { + if let NodeBlock(block) = map.get(parent_id) { + for (id, _) in visitor.states.iter().filter( |&(_,v)| *v == VarState::IncrOnce) { + let mut visitor2 = InitializeVisitor { cx: cx, end_expr: expr, var_id: id.clone(), + state: VarState::IncrOnce, name: None, + depth: 0, + past_loop: false }; + walk_block(&mut visitor2, block); + + if visitor2.state == VarState::Warn { + if let Some(name) = visitor2.name { + span_lint(cx, EXPLICIT_COUNTER_LOOP, expr.span, + &format!("the variable `{0}` is used as a loop counter. Consider \ + using `for ({0}, item) in {1}.enumerate()` \ + or similar iterators", + name, snippet(cx, arg.span, "_"))); + } + } + } + } + } +} + /// Recover the essential nodes of a desugared for loop: /// `for pat in arg { body }` becomes `(pat, arg, body)`. fn recover_for_loop(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> { diff --git a/src/matches.rs b/src/matches.rs index b6a7fc7fa5b..2c7f0830a53 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -59,7 +59,7 @@ 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 let LitBool(val) = lit.node { if val { Some((&*arms[0].body, &*arms[1].body)) } else { diff --git a/src/methods.rs b/src/methods.rs index b0758bc6937..f1c868bec07 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -54,20 +54,19 @@ impl LateLintPass for MethodsPass { if let ExprMethodCall(ref name, _, ref args) = expr.node { let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); - if name.node.as_str() == "unwrap" { - if match_type(cx, obj_ty, &OPTION_PATH) { + match &*name.node.as_str() { + "unwrap" if match_type(cx, obj_ty, &OPTION_PATH) => { span_lint(cx, OPTION_UNWRAP_USED, expr.span, "used unwrap() on an Option value. If you don't want \ to handle the None case gracefully, consider using \ expect() to provide a better panic message"); - } else if match_type(cx, obj_ty, &RESULT_PATH) { + }, + "unwrap" if match_type(cx, obj_ty, &RESULT_PATH) => { span_lint(cx, RESULT_UNWRAP_USED, expr.span, "used unwrap() on a Result value. Graceful handling \ of Err values is preferred"); - } - } - else if name.node.as_str() == "to_string" { - if obj_ty.sty == ty::TyStr { + }, + "to_string" if obj_ty.sty == ty::TyStr => { let mut arg_str = snippet(cx, args[0].span, "_"); if ptr_depth > 1 { arg_str = Cow::Owned(format!( @@ -77,13 +76,12 @@ impl LateLintPass for MethodsPass { } span_lint(cx, STR_TO_STRING, expr.span, &format!( "`{}.to_owned()` is faster", arg_str)); - } else if match_type(cx, obj_ty, &STRING_PATH) { + }, + "to_string" if match_type(cx, obj_ty, &STRING_PATH) => { span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op; use \ `clone()` to make a copy"); - } - } - else if name.node.as_str() == "expect" { - if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { + }, + "expect" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { if inner_name.node.as_str() == "ok" && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &RESULT_PATH) { let result_type = cx.tcx.expr_ty(&inner_args[0]); @@ -96,11 +94,9 @@ impl LateLintPass for MethodsPass { } } } - } - } - // check Option.map(_).unwrap_or(_) - else if name.node.as_str() == "unwrap_or" { - if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { + }, + // check Option.map(_).unwrap_or(_) + "unwrap_or" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { if inner_name.node.as_str() == "map" && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) { // lint message @@ -126,11 +122,9 @@ impl LateLintPass for MethodsPass { span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg); }; } - } - } - // check Option.map(_).unwrap_or_else(_) - else if name.node.as_str() == "unwrap_or_else" { - if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { + }, + // check Option.map(_).unwrap_or_else(_) + "unwrap_or_else" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { if inner_name.node.as_str() == "map" && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) { // lint message @@ -156,7 +150,8 @@ impl LateLintPass for MethodsPass { span_lint(cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg); }; } - } + }, + _ => {}, } } } diff --git a/src/utils.rs b/src/utils.rs index 3fcfa66259c..365fc1bf99d 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -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(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, +} + +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(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!() + } + } + } + } +} diff --git a/tests/compile-fail/cyclomatic_complexity.rs b/tests/compile-fail/cyclomatic_complexity.rs new file mode 100644 index 00000000000..8e3bf123c26 --- /dev/null +++ b/tests/compile-fail/cyclomatic_complexity.rs @@ -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 { + } + } +} diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 3d19bd66094..7a18c210d0c 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -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]; diff --git a/tests/compile-fail/while_loop.rs b/tests/compile-fail/while_loop.rs index 7d1904ad446..ee8e4622e0e 100644 --- a/tests/compile-fail/while_loop.rs +++ b/tests/compile-fail/while_loop.rs @@ -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); diff --git a/tests/dogfood.rs b/tests/dogfood.rs new file mode 100644 index 00000000000..61e37c28c94 --- /dev/null +++ b/tests/dogfood.rs @@ -0,0 +1,24 @@ +extern crate compiletest_rs as compiletest; + +use std::path::Path; +use std::env::var; + +#[test] +fn dogfood() { + let mut config = compiletest::default_config(); + + let cfg_mode = "run-pass".parse().ok().expect("Invalid mode"); + let mut s = String::new(); + s.push_str(" -L target/debug/"); + s.push_str(" -L target/debug/deps"); + s.push_str(" -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy"); + config.target_rustcflags = Some(s); + if let Ok(name) = var::<&str>("TESTNAME") { + let s : String = name.to_owned(); + config.filter = Some(s) + } + + config.mode = cfg_mode; + + compiletest::runtest::run(config, &Path::new("src/lib.rs")); +}