Auto merge of #2857 - avborhanian:master, r=phansch

Adding lint test for excessive LOC.

This is a WIP for #2377. Just wanted to pull in because I had a few questions:

1. Is it okay that I'm approaching this via counting by looking at each line in the snippet instead of looking at the AST tree? If there's another way to do it, I want to make sure I'm doing the correct way, but I wasn't sure since the output AST JSON doesn't seem to contain whitespace.

2. My function is definitely going to trigger the lint, so also wanted to see if there was something obvious I could do to reduce it.

3. Are the two tests fine, or is there something obvious I'm missing?

4. Obviously bigger question - am I approaching the line count correctly. Current strategy is count a line if it contains some code, so skip if it's just comments or empty.

CHANGELOG.md

@@ -981,6 +981,7 @@ All notable changes to this project will be documented in this file.
 [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
 [`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr
 [`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
+[`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines
 [`toplevel_ref_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg
 [`transmute_bytes_to_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_bytes_to_str
 [`transmute_int_to_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_int_to_bool

README.md

@@ -7,7 +7,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 294 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 295 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

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) => {

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,

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) {

clippy_lints/src/functions.rs

@@ -1,9 +1,9 @@
-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;
 use rustc::hir::intravisit;
-use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintContext, LintPass};
 use rustc::ty;
 use rustc::{declare_tool_lint, lint_array};
 use rustc_data_structures::fx::FxHashSet;
@@ -31,6 +31,29 @@ 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.
+///
+/// **Example:**
+/// ``` rust
+/// fn im_too_long() {
+/// println!("");
+/// // ... 100 more LoC
+/// println!("");
+/// }
+/// ```
+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 +85,18 @@ 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 +147,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 +178,72 @@ impl<'a, 'tcx> Functions {
         }
     }
 
+    fn check_line_number(self, cx: &LateContext<'_, '_>, span: Span) {
+        if in_external_macro(cx.sess(), span) {
+            return;
+        }
+
+        let code_snippet = snippet(cx, span, "..");
+        let mut line_count: u64 = 0;
+        let mut in_comment = false;
+        let mut code_in_line;
+
+        // Skip the surrounding function decl.
+        let start_brace_idx = match code_snippet.find('{') {
+            Some(i) => i + 1,
+            None => 0,
+        };
+        let end_brace_idx = match code_snippet.find('}') {
+            Some(i) => i,
+            None => code_snippet.len(),
+        };
+        let function_lines = code_snippet[start_brace_idx..end_brace_idx].lines();
+
+        for mut line in function_lines {
+            code_in_line = false;
+            loop {
+                line = line.trim_start();
+                if line.is_empty() {
+                    break;
+                }
+                if in_comment {
+                    match line.find("*/") {
+                        Some(i) => {
+                            line = &line[i + 2..];
+                            in_comment = false;
+                            continue;
+                        },
+                        None => break,
+                    }
+                } else {
+                    let multi_idx = match line.find("/*") {
+                        Some(i) => i,
+                        None => line.len(),
+                    };
+                    let single_idx = match line.find("//") {
+                        Some(i) => i,
+                        None => line.len(),
+                    };
+                    code_in_line |= multi_idx > 0 && single_idx > 0;
+                    // Implies multi_idx is below line.len()
+                    if multi_idx < single_idx {
+                        line = &line[multi_idx + 2..];
+                        in_comment = true;
+                        continue;
+                    }
+                    break;
+                }
+            }
+            if code_in_line {
+                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>,

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,

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,

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`

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>,

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) {

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<BodyId>) {
     let fn_def_id = cx.tcx.hir().local_def_id(fn_id);
     let sig = cx.tcx.fn_sig(fn_def_id);

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>,

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 {

clippy_lints/src/types.rs

@@ -240,6 +240,7 @@ 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.
+#[allow(clippy::too_many_lines)]
 fn check_ty(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty, is_local: bool) {
     if in_macro(hir_ty.span) {
         return;
@@ -1966,7 +1967,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;
 

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);

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<u64>),
+    /// Lint: TOO_MANY_LINES. The maximum number of lines a function or method can have
+    (too_many_lines_threshold, "too_many_lines_threshold", 100 => u64),
 }
 
 impl Default for Conf {

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);

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(

tests/ui-toml/functions_maxlines/clippy.toml

@@ -0,0 +1 @@
+too-many-lines-threshold = 1

tests/ui-toml/functions_maxlines/test.rs

@@ -0,0 +1,45 @@
+#![warn(clippy::too_many_lines)]
+
+// This function should be considered one line.
+fn many_comments_but_one_line_of_code() {
+    /* 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.");
+}
+
+// This should be considered two and a fail.
+fn too_many_lines() {
+    println!("This is bad.");
+    println!("This is bad.");
+}
+
+// This should be considered one line.
+#[rustfmt::skip]
+fn comment_starts_after_code() {
+    let _ = 5; /* closing comment. */ /*
+    this line shouldn't be counted theoretically.
+    */
+}
+
+// This should be considered one line.
+fn comment_after_code() {
+    let _ = 5; /* this line should get counted once. */
+}
+
+// This should fail since it is technically two lines.
+#[rustfmt::skip]
+fn comment_before_code() {
+    let _ = "test";
+    /* This comment extends to the front of
+    teh code but this line should still count. */ let _ = 5;
+}
+
+// This should be considered one line.
+fn main() {}