]> 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 8903766c330b4636dfc488d446ce4856bcd3bb4f..307f03c692807cf3b7df4d2779c9a760cb65019a 100644 (file)
@@ -1,56 +1,81 @@
+use crate::utils::{iter_input_pats, snippet, span_lint, type_is_unsafe_function};
 use matches::matches;
-use rustc::hir::intravisit;
 use rustc::hir;
-use rustc::lint::*;
-use rustc::{declare_lint, lint_array};
-use rustc::ty;
 use rustc::hir::def::Def;
-use std::collections::HashSet;
-use syntax::ast;
+use rustc::hir::intravisit;
+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::codemap::Span;
-use crate::utils::{iter_input_pats, span_lint, type_is_unsafe_function};
-
-/// **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) { .. }
-/// ```
+use syntax::source_map::Span;
+
 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"
     }
 }
 
@@ -83,11 +111,13 @@ fn check_fn(
         decl: &'tcx hir::FnDecl,
         body: &'tcx hir::Body,
         span: Span,
-        nodeid: ast::NodeId,
+        hir_id: hir::HirId,
     ) {
-        use rustc::hir::map::Node::*;
-
-        let is_impl = if let Some(NodeItem(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
@@ -103,13 +133,24 @@ fn check_fn(
         if !is_impl {
             // don't lint extern functions decls, it's not their fault either
             match kind {
-                hir::intravisit::FnKind::Method(_, &hir::MethodSig { header: hir::FnHeader { abi: Abi::Rust, .. }, .. }, _, _) |
-                hir::intravisit::FnKind::ItemFn(_, _, hir::FnHeader { abi: Abi::Rust, .. }, _, _) => self.check_arg_number(cx, decl, span),
+                hir::intravisit::FnKind::Method(
+                    _,
+                    &hir::MethodSig {
+                        header: hir::FnHeader { abi: Abi::Rust, .. },
+                        ..
+                    },
+                    _,
+                    _,
+                )
+                | hir::intravisit::FnKind::ItemFn(_, _, hir::FnHeader { abi: Abi::Rust, .. }, _, _) => {
+                    self.check_arg_number(cx, decl, span)
+                },
                 _ => {},
             }
         }
 
-        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) {
@@ -120,8 +161,8 @@ 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);
+                let body = cx.tcx.hir().body(eid);
+                self.check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.hir_id);
             }
         }
     }
@@ -140,20 +181,86 @@ 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))
-                .collect::<HashSet<_>>();
+                .collect::<FxHashSet<_>>();
 
             if !raw_ptrs.is_empty() {
                 let tables = cx.tcx.body_tables(body.id());
@@ -169,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 {
@@ -179,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: HashSet<ast::NodeId>,
+    ptrs: FxHashSet<hir::HirId>,
     tables: &'a ty::TypeckTables<'tcx>,
 }
 
@@ -196,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) {
@@ -220,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,