]> git.lizzy.rs Git - rust.git/blobdiff - clippy_lints/src/functions.rs
Auto merge of #3946 - rchaser53:issue-3920, r=flip1995
[rust.git] / clippy_lints / src / functions.rs
index 19adf2d1dc4e54437554d80f4de3ce67888c6d74..307f03c692807cf3b7df4d2779c9a760cb65019a 100644 (file)
@@ -1,68 +1,81 @@
-// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
-// file at the top-level directory of this distribution.
-//
-// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
-// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
-// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
-// option. This file may not be copied, modified, or distributed
-// except according to those terms.
-
-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;
 use rustc_target::spec::abi::Abi;
-use syntax::ast;
 use syntax::source_map::Span;
 
-/// **What it does:** Checks for functions with too many parameters.
-///
-/// **Why is this bad?** Functions with lots of parameters are considered bad
-/// style and reduce readability (“what does the 5th parameter mean?”). Consider
-/// grouping some parameters into a new type.
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-/// ```rust
-/// fn foo(x: u32, y: u32, name: &str, c: Color, w: f32, h: f32, a: f32, b: f32) {
-///     ..
-/// }
-/// ```
 declare_clippy_lint! {
+    /// **What it does:** Checks for functions with too many parameters.
+    ///
+    /// **Why is this bad?** Functions with lots of parameters are considered bad
+    /// style and reduce readability (“what does the 5th parameter mean?”). Consider
+    /// grouping some parameters into a new type.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// fn foo(x: u32, y: u32, name: &str, c: Color, w: f32, h: f32, a: f32, b: f32) {
+    ///     ..
+    /// }
+    /// ```
     pub TOO_MANY_ARGUMENTS,
     complexity,
     "functions with too many arguments"
 }
 
-/// **What it does:** Checks for public functions that dereferences raw pointer
-/// arguments but are not marked unsafe.
-///
-/// **Why is this bad?** The function should probably be marked `unsafe`, since
-/// for an arbitrary raw pointer, there is no way of telling for sure if it is
-/// valid.
-///
-/// **Known problems:**
-///
-/// * It does not check functions recursively so if the pointer is passed to a
-/// private non-`unsafe` function which does the dereferencing, the lint won't
-/// trigger.
-/// * It only checks for arguments whose type are raw pointers, not raw pointers
-/// got from an argument in some other way (`fn foo(bar: &[*const u8])` or
-/// `some_argument.get_raw_ptr()`).
-///
-/// **Example:**
-/// ```rust
-/// pub fn foo(x: *const u8) {
-///     println!("{}", unsafe { *x });
-/// }
-/// ```
 declare_clippy_lint! {
+    /// **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!("");
+    /// }
+    /// ```
+    pub TOO_MANY_LINES,
+    pedantic,
+    "functions with too many lines"
+}
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for public functions that dereferences raw pointer
+    /// arguments but are not marked unsafe.
+    ///
+    /// **Why is this bad?** The function should probably be marked `unsafe`, since
+    /// for an arbitrary raw pointer, there is no way of telling for sure if it is
+    /// valid.
+    ///
+    /// **Known problems:**
+    ///
+    /// * It does not check functions recursively so if the pointer is passed to a
+    /// private non-`unsafe` function which does the dereferencing, the lint won't
+    /// trigger.
+    /// * It only checks for arguments whose type are raw pointers, not raw pointers
+    /// got from an argument in some other way (`fn foo(bar: &[*const u8])` or
+    /// `some_argument.get_raw_ptr()`).
+    ///
+    /// **Example:**
+    /// ```rust
+    /// pub fn foo(x: *const u8) {
+    ///     println!("{}", unsafe { *x });
+    /// }
+    /// ```
     pub NOT_UNSAFE_PTR_ARG_DEREF,
     correctness,
     "public functions dereferencing raw pointer arguments but not marked `unsafe`"
 #[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 {
+        "Functions"
     }
 }
 
@@ -93,9 +111,13 @@ fn check_fn(
         decl: &'tcx hir::FnDecl,
         body: &'tcx hir::Body,
         span: Span,
-        nodeid: ast::NodeId,
+        hir_id: hir::HirId,
     ) {
-        let is_impl = if let Some(hir::Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(nodeid)) {
+        let is_impl = if let Some(hir::Node::Item(item)) = cx
+            .tcx
+            .hir()
+            .find_by_hir_id(cx.tcx.hir().get_parent_node_by_hir_id(hir_id))
+        {
             matches!(item.node, hir::ItemKind::Impl(_, _, _, _, Some(_), _, _))
         } else {
             false
@@ -127,7 +149,8 @@ fn check_fn(
             }
         }
 
-        self.check_raw_ptr(cx, unsafety, decl, body, nodeid);
+        self.check_raw_ptr(cx, unsafety, decl, body, hir_id);
+        self.check_line_number(cx, span, body);
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::TraitItem) {
@@ -139,7 +162,7 @@ fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Trai
 
             if let hir::TraitMethod::Provided(eid) = *eid {
                 let body = cx.tcx.hir().body(eid);
-                self.check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.id);
+                self.check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.hir_id);
             }
         }
     }
@@ -158,16 +181,82 @@ fn check_arg_number(self, cx: &LateContext<'_, '_>, decl: &hir::FnDecl, span: Sp
         }
     }
 
+    fn check_line_number(self, cx: &LateContext<'_, '_>, span: Span, body: &'tcx hir::Body) {
+        if in_external_macro(cx.sess(), span) {
+            return;
+        }
+
+        let code_snippet = snippet(cx, body.value.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>,
         unsafety: hir::Unsafety,
         decl: &'tcx hir::FnDecl,
         body: &'tcx hir::Body,
-        nodeid: ast::NodeId,
+        hir_id: hir::HirId,
     ) {
         let expr = &body.value;
-        if unsafety == hir::Unsafety::Normal && cx.access_levels.is_exported(nodeid) {
+        if unsafety == hir::Unsafety::Normal && cx.access_levels.is_exported(hir_id) {
             let raw_ptrs = iter_input_pats(decl, body)
                 .zip(decl.inputs.iter())
                 .filter_map(|(arg, ty)| raw_ptr_arg(arg, ty))
@@ -187,7 +276,7 @@ fn check_raw_ptr(
     }
 }
 
-fn raw_ptr_arg(arg: &hir::Arg, ty: &hir::Ty) -> Option<ast::NodeId> {
+fn raw_ptr_arg(arg: &hir::Arg, ty: &hir::Ty) -> Option<hir::HirId> {
     if let (&hir::PatKind::Binding(_, id, _, _), &hir::TyKind::Ptr(_)) = (&arg.pat.node, &ty.node) {
         Some(id)
     } else {
@@ -197,7 +286,7 @@ fn raw_ptr_arg(arg: &hir::Arg, ty: &hir::Ty) -> Option<ast::NodeId> {
 
 struct DerefVisitor<'a, 'tcx: 'a> {
     cx: &'a LateContext<'a, 'tcx>,
-    ptrs: FxHashSet<ast::NodeId>,
+    ptrs: FxHashSet<hir::HirId>,
     tables: &'a ty::TypeckTables<'tcx>,
 }
 
@@ -214,7 +303,7 @@ fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
                 }
             },
             hir::ExprKind::MethodCall(_, _, ref args) => {
-                let def_id = self.tables.type_dependent_defs()[expr.hir_id].def_id();
+                let def_id = self.tables.type_dependent_def_id(expr.hir_id).unwrap();
                 let base_type = self.cx.tcx.type_of(def_id);
 
                 if type_is_unsafe_function(self.cx, base_type) {
@@ -238,7 +327,7 @@ impl<'a, 'tcx: 'a> DerefVisitor<'a, 'tcx> {
     fn check_arg(&self, ptr: &hir::Expr) {
         if let hir::ExprKind::Path(ref qpath) = ptr.node {
             if let Def::Local(id) = self.cx.tables.qpath_def(qpath, ptr.hir_id) {
-                if self.ptrs.contains(&id) {
+                if self.ptrs.contains(&self.cx.tcx.hir().node_to_hir_id(id)) {
                     span_lint(
                         self.cx,
                         NOT_UNSAFE_PTR_ARG_DEREF,