From 1169066a0b501ae508461181b76c37d7e3b8ae6a Mon Sep 17 00:00:00 2001 From: Araam Borhanian Date: Sun, 13 Jan 2019 10:19:02 -0500 Subject: [PATCH] Adding lint for too many lines. --- clippy_lints/src/assign_ops.rs | 1 + clippy_lints/src/bit_mask.rs | 1 + clippy_lints/src/eq_op.rs | 2 +- clippy_lints/src/functions.rs | 77 +++++++++++- clippy_lints/src/lib.rs | 4 +- clippy_lints/src/loops.rs | 2 + clippy_lints/src/methods/mod.rs | 2 + clippy_lints/src/needless_pass_by_value.rs | 1 + clippy_lints/src/non_expressive_names.rs | 1 + clippy_lints/src/ptr.rs | 1 + clippy_lints/src/redundant_clone.rs | 1 + clippy_lints/src/transmute.rs | 2 +- clippy_lints/src/types.rs | 7 +- clippy_lints/src/utils/author.rs | 2 + clippy_lints/src/utils/conf.rs | 2 + clippy_lints/src/utils/hir_utils.rs | 2 +- src/driver.rs | 1 + .../toml_unknown_key/conf_unknown_key.stderr | 2 +- tests/ui/functions_maxlines.rs | 112 ++++++++++++++++++ tests/ui/functions_maxlines.stderr | 16 +++ tests/ui/matches.rs | 2 +- 21 files changed, 228 insertions(+), 13 deletions(-) create mode 100644 tests/ui/functions_maxlines.rs create mode 100644 tests/ui/functions_maxlines.stderr diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index cc44b514ea7..2ef2a184746 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -64,6 +64,7 @@ impl LintPass for AssignOps { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps { + #[allow(clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) { match &expr.node { hir::ExprKind::AssignOp(op, lhs, rhs) => { diff --git a/clippy_lints/src/bit_mask.rs b/clippy_lints/src/bit_mask.rs index d4e30376199..f052ad6e5ac 100644 --- a/clippy_lints/src/bit_mask.rs +++ b/clippy_lints/src/bit_mask.rs @@ -177,6 +177,7 @@ fn check_compare(cx: &LateContext<'_, '_>, bit_op: &Expr, cmp_op: BinOpKind, cmp } } +#[allow(clippy::too_many_lines)] fn check_bit_mask( cx: &LateContext<'_, '_>, bit_op: BinOpKind, diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index 57291dd24ee..2602ad45986 100644 --- a/clippy_lints/src/eq_op.rs +++ b/clippy_lints/src/eq_op.rs @@ -59,7 +59,7 @@ impl LintPass for EqOp { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EqOp { - #[allow(clippy::similar_names)] + #[allow(clippy::similar_names, clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { if let ExprKind::Binary(op, ref left, ref right) = e.node { if in_macro(e.span) { diff --git a/clippy_lints/src/functions.rs b/clippy_lints/src/functions.rs index cb69e96c8e4..9710eba21ad 100644 --- a/clippy_lints/src/functions.rs +++ b/clippy_lints/src/functions.rs @@ -1,4 +1,4 @@ -use crate::utils::{iter_input_pats, span_lint, type_is_unsafe_function}; +use crate::utils::{iter_input_pats, snippet, span_lint, type_is_unsafe_function}; use matches::matches; use rustc::hir; use rustc::hir::def::Def; @@ -31,6 +31,22 @@ declare_clippy_lint! { "functions with too many arguments" } +/// **What it does:** Checks for functions with a large amount of lines. +/// +/// **Why is this bad?** Functions with a lot of lines are harder to understand +/// due to having to look at a larger amount of code to understand what the +/// function is doing. Consider splitting the body of the function into +/// multiple functions. +/// +/// **Known problems:** None. +/// +/// ``` +declare_clippy_lint! { + pub TOO_MANY_LINES, + pedantic, + "functions with too many lines" +} + /// **What it does:** Checks for public functions that dereferences raw pointer /// arguments but are not marked unsafe. /// @@ -62,17 +78,21 @@ declare_clippy_lint! { #[derive(Copy, Clone)] pub struct Functions { threshold: u64, + max_lines: u64 } impl Functions { - pub fn new(threshold: u64) -> Self { - Self { threshold } + pub fn new(threshold: u64, max_lines: u64) -> Self { + Self { + threshold, + max_lines + } } } impl LintPass for Functions { fn get_lints(&self) -> LintArray { - lint_array!(TOO_MANY_ARGUMENTS, NOT_UNSAFE_PTR_ARG_DEREF) + lint_array!(TOO_MANY_ARGUMENTS, TOO_MANY_LINES, NOT_UNSAFE_PTR_ARG_DEREF) } fn name(&self) -> &'static str { @@ -123,6 +143,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Functions { } self.check_raw_ptr(cx, unsafety, decl, body, nodeid); + self.check_line_number(cx, span); } fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::TraitItem) { @@ -153,6 +174,54 @@ impl<'a, 'tcx> Functions { } } + fn check_line_number(self, cx: &LateContext, span: Span) { + let code_snippet = snippet(cx, span, ".."); + let mut line_count = 0; + let mut in_comment = false; + for mut line in code_snippet.lines() { + if in_comment { + let end_comment_loc = match line.find("*/") { + Some(i) => i, + None => continue + }; + in_comment = false; + line = &line[end_comment_loc..]; + } + line = line.trim_left(); + if line.is_empty() || line.starts_with("//") { continue; } + if line.contains("/*") { + let mut count_line: bool = !line.starts_with("/*"); + let close_counts = line.match_indices("*/").count(); + let open_counts = line.match_indices("/*").count(); + + if close_counts > 1 || open_counts > 1 { + line_count += 1; + } else if close_counts == 1 { + match line.find("*/") { + Some(i) => { + line = line[i..].trim_left(); + if !line.is_empty() && !line.starts_with("//") { + count_line = true; + } + }, + None => continue + } + } else { + in_comment = true; + } + if count_line { line_count += 1; } + } else { + // No multipart comment, no single comment, non-empty string. + line_count += 1; + } + } + + if line_count > self.max_lines { + span_lint(cx, TOO_MANY_LINES, span, + "This function has a large number of lines.") + } + } + fn check_raw_ptr( self, cx: &LateContext<'a, 'tcx>, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 52cc2a88da4..8e8657c9d02 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -290,6 +290,7 @@ pub fn read_conf(reg: &rustc_plugin::Registry<'_>) -> Conf { } } +#[allow(clippy::too_many_lines)] #[rustfmt::skip] pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { let mut store = reg.sess.lint_store.borrow_mut(); @@ -427,7 +428,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box blacklisted_name::BlackListedName::new( conf.blacklisted_names.iter().cloned().collect() )); - reg.register_late_lint_pass(box functions::Functions::new(conf.too_many_arguments_threshold)); + reg.register_late_lint_pass(box functions::Functions::new(conf.too_many_arguments_threshold, conf.too_many_lines_threshold)); reg.register_early_lint_pass(box doc::Doc::new(conf.doc_valid_idents.iter().cloned().collect())); reg.register_late_lint_pass(box neg_multiply::NegMultiply); reg.register_early_lint_pass(box unsafe_removed_from_name::UnsafeNameRemoval); @@ -527,6 +528,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { enum_glob_use::ENUM_GLOB_USE, enum_variants::MODULE_NAME_REPETITIONS, enum_variants::PUB_ENUM_VARIANT_NAMES, + functions::TOO_MANY_LINES, if_not_else::IF_NOT_ELSE, infinite_iter::MAYBE_INFINITE_ITER, items_after_statements::ITEMS_AFTER_STATEMENTS, diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 4d6cc75135e..06266257d1e 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -472,6 +472,7 @@ impl LintPass for Pass { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + #[allow(clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { // we don't want to check expanded macros if in_macro(expr.span) { @@ -1066,6 +1067,7 @@ fn detect_manual_memcpy<'a, 'tcx>( /// Check for looping over a range and then indexing a sequence with it. /// The iteratee must be a range literal. +#[allow(clippy::too_many_lines)] fn check_for_loop_range<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, pat: &'tcx Pat, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 20ffc1fd406..4ce2ae03b9f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1005,6 +1005,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } /// Checks for the `OR_FUN_CALL` lint. +#[allow(clippy::too_many_lines)] fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Span, name: &str, args: &[hir::Expr]) { /// Check for `unwrap_or(T::new())` or `unwrap_or(T::default())`. fn check_unwrap_or_default( @@ -1151,6 +1152,7 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa } /// Checks for the `EXPECT_FUN_CALL` lint. +#[allow(clippy::too_many_lines)] fn lint_expect_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Span, name: &str, args: &[hir::Expr]) { // Strip `&`, `as_ref()` and `as_str()` off `arg` until we're left with either a `String` or // `&str` diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index 73c0ed72d3b..a8cc5eeec9f 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -73,6 +73,7 @@ macro_rules! need { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { + #[allow(clippy::too_many_lines)] fn check_fn( &mut self, cx: &LateContext<'a, 'tcx>, diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index 3bdf2c38d23..fbf60db28ee 100644 --- a/clippy_lints/src/non_expressive_names.rs +++ b/clippy_lints/src/non_expressive_names.rs @@ -158,6 +158,7 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> { ); } } + #[allow(clippy::too_many_lines)] fn check_name(&mut self, span: Span, name: Name) { let interned_name = name.as_str(); if interned_name.chars().any(char::is_uppercase) { diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index b990b7ab2bd..5a54971f49e 100644 --- a/clippy_lints/src/ptr.rs +++ b/clippy_lints/src/ptr.rs @@ -151,6 +151,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PointerPass { } } +#[allow(clippy::too_many_lines)] fn check_fn(cx: &LateContext<'_, '_>, decl: &FnDecl, fn_id: NodeId, opt_body_id: Option) { let fn_def_id = cx.tcx.hir().local_def_id(fn_id); let sig = cx.tcx.fn_sig(fn_def_id); diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 7ac147c8ac1..5c994d8a2bc 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -80,6 +80,7 @@ impl LintPass for RedundantClone { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { + #[allow(clippy::too_many_lines)] fn check_fn( &mut self, cx: &LateContext<'a, 'tcx>, diff --git a/clippy_lints/src/transmute.rs b/clippy_lints/src/transmute.rs index 90cfcd56c3b..56e1aa4a4e1 100644 --- a/clippy_lints/src/transmute.rs +++ b/clippy_lints/src/transmute.rs @@ -226,7 +226,7 @@ impl LintPass for Transmute { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute { - #[allow(clippy::similar_names)] + #[allow(clippy::similar_names, clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { if let ExprKind::Call(ref path_expr, ref args) = e.node { if let ExprKind::Path(ref qpath) = path_expr.node { diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 939dd27c441..e81df4af383 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -240,8 +240,9 @@ fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath, path: &[&str]) /// /// The parameter `is_local` distinguishes the context of the type; types from /// local bindings should only be checked for the `BORROWED_BOX` lint. -fn check_ty(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty, is_local: bool) { - if in_macro(hir_ty.span) { +#[allow(clippy::too_many_lines)] +fn check_ty(cx: &LateContext<'_, '_>, ast_ty: &hir::Ty, is_local: bool) { + if in_macro(ast_ty.span) { return; } match hir_ty.node { @@ -1968,7 +1969,7 @@ impl LintPass for ImplicitHasher { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitHasher { - #[allow(clippy::cast_possible_truncation)] + #[allow(clippy::cast_possible_truncation, clippy::too_many_lines)] fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) { use syntax_pos::BytePos; diff --git a/clippy_lints/src/utils/author.rs b/clippy_lints/src/utils/author.rs index 19c3f3ad230..5a76b965d26 100644 --- a/clippy_lints/src/utils/author.rs +++ b/clippy_lints/src/utils/author.rs @@ -194,6 +194,7 @@ struct PrintVisitor { } impl<'tcx> Visitor<'tcx> for PrintVisitor { + #[allow(clippy::too_many_lines)] fn visit_expr(&mut self, expr: &Expr) { print!(" if let ExprKind::"); let current = format!("{}.node", self.current); @@ -506,6 +507,7 @@ impl<'tcx> Visitor<'tcx> for PrintVisitor { } } + #[allow(clippy::too_many_lines)] fn visit_pat(&mut self, pat: &Pat) { print!(" if let PatKind::"); let current = format!("{}.node", self.current); diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 55256a25427..4ab274e598f 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -148,6 +148,8 @@ define_Conf! { (literal_representation_threshold, "literal_representation_threshold", 16384 => u64), /// Lint: TRIVIALLY_COPY_PASS_BY_REF. The maximum size (in bytes) to consider a `Copy` type for passing by value instead of by reference. (trivial_copy_size_limit, "trivial_copy_size_limit", None => Option), + /// Lint: TOO_MANY_LINES. The maximum number of lines a function or method can have + (too_many_lines_threshold, "too_many_lines_threshold", 101 => u64), } impl Default for Conf { diff --git a/clippy_lints/src/utils/hir_utils.rs b/clippy_lints/src/utils/hir_utils.rs index a176830be26..2b0b0e7121f 100644 --- a/clippy_lints/src/utils/hir_utils.rs +++ b/clippy_lints/src/utils/hir_utils.rs @@ -389,7 +389,7 @@ impl<'a, 'tcx: 'a> SpanlessHash<'a, 'tcx> { .hash(&mut self.s); } - #[allow(clippy::many_single_char_names)] + #[allow(clippy::many_single_char_names, clippy::too_many_lines)] pub fn hash_expr(&mut self, e: &Expr) { if let Some(e) = constant_simple(self.cx, self.tables, e) { return e.hash(&mut self.s); diff --git a/src/driver.rs b/src/driver.rs index 1ce6f6b7c49..b41895f3c2b 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -20,6 +20,7 @@ fn show_version() { println!(env!("CARGO_PKG_VERSION")); } +#[allow(clippy::too_many_lines)] pub fn main() { rustc_driver::init_rustc_env_logger(); exit( diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 05a04fb377a..ec2ac20684b 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -1,4 +1,4 @@ -error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `third-party` +error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `third-party` error: aborting due to previous error diff --git a/tests/ui/functions_maxlines.rs b/tests/ui/functions_maxlines.rs new file mode 100644 index 00000000000..5d8baf438df --- /dev/null +++ b/tests/ui/functions_maxlines.rs @@ -0,0 +1,112 @@ +#![warn(clippy::all, clippy::pedantic)] + +// TOO_MANY_LINES +fn good_lines() { + /* println!("This is good."); */ + // println!("This is good."); + /* */ // println!("This is good."); + /* */ // println!("This is good."); + /* */ // println!("This is good."); + /* */ // println!("This is good."); + /* println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); */ + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); + println!("This is good."); +} + +fn bad_lines() { + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); + println!("This is bad."); +} + +fn main() {} diff --git a/tests/ui/functions_maxlines.stderr b/tests/ui/functions_maxlines.stderr new file mode 100644 index 00000000000..9e1b2fe568a --- /dev/null +++ b/tests/ui/functions_maxlines.stderr @@ -0,0 +1,16 @@ +error: This function has a large number of lines. + --> $DIR/functions_maxlines.rs:59:1 + | +LL | / fn bad_lines() { +LL | | println!("This is bad."); +LL | | println!("This is bad."); +LL | | println!("This is bad."); +... | +LL | | println!("This is bad."); +LL | | } + | |_^ + | + = note: `-D clippy::too-many-lines` implied by `-D warnings` + +error: aborting due to previous error + diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index 013f12a1a0e..a40b80378ef 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -1,6 +1,6 @@ #![feature(exclusive_range_pattern)] #![warn(clippy::all)] -#![allow(unused, clippy::redundant_pattern_matching)] +#![allow(unused, clippy::redundant_pattern_matching, clippy::too_many_lines)] #![warn(clippy::match_same_arms)] fn dummy() {}