Merge pull request #2816 from mockersf/multiple-impl

adding to restriction a lint that check for multiple inherent implementations

Author: oli-obk

.travis.yml

@@ -24,10 +24,13 @@ before_install:
     fi
 
 install:
-  - . $HOME/.nvm/nvm.sh
-  - nvm install stable
-  - nvm use stable
-  - npm install remark-cli remark-lint
+  - |
+    if [ -z ${INTEGRATION} ]; then
+      . $HOME/.nvm/nvm.sh
+      nvm install stable
+      nvm use stable
+      npm install remark-cli remark-lint
+    fi
 
 matrix:
   include:

clippy_lints/src/inherent_impl.rs

@@ -0,0 +1,95 @@
+//! lint on inherent implementations
+
+use rustc::hir::*;
+use rustc::lint::*;
+use std::collections::HashMap;
+use std::default::Default;
+use syntax_pos::Span;
+
+/// **What it does:** Checks for multiple inherent implementations of a struct
+///
+/// **Why is this bad?** Splitting the implementation of a type makes the code harder to navigate.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// struct X;
+/// impl X {
+///     fn one() {}
+/// }
+/// impl X {
+///     fn other() {}
+/// }
+/// ```
+///
+/// Could be written:
+///
+/// ```rust
+/// struct X;
+/// impl X {
+///     fn one() {}
+///     fn other() {}
+/// }
+/// ```
+declare_clippy_lint! {
+    pub MULTIPLE_INHERENT_IMPL,
+    restriction,
+    "Multiple inherent impl that could be grouped"
+}
+
+pub struct Pass {
+    impls: HashMap<def_id::DefId, (Span, Generics)>,
+}
+
+impl Default for Pass {
+    fn default() -> Self {
+        Pass { impls: HashMap::new() }
+    }
+}
+
+impl LintPass for Pass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(MULTIPLE_INHERENT_IMPL)
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
+    fn check_item(&mut self, _: &LateContext<'a, 'tcx>, item: &'tcx Item) {
+        if let Item_::ItemImpl(_, _, _, ref generics, None, _, _) = item.node {
+            // Remember for each inherent implementation encoutered its span and generics
+            self.impls
+                .insert(item.hir_id.owner_def_id(), (item.span, generics.clone()));
+        }
+    }
+
+    fn check_crate_post(&mut self, cx: &LateContext<'a, 'tcx>, krate: &'tcx Crate) {
+        if let Some(item) = krate.items.values().nth(0) {
+            // Retrieve all inherent implementations from the crate, grouped by type
+            for impls in cx
+                .tcx
+                .crate_inherent_impls(item.hir_id.owner_def_id().krate)
+                .inherent_impls
+                .values()
+            {
+                // Filter out implementations that have generic params (type or lifetime)
+                let mut impl_spans = impls
+                    .iter()
+                    .filter_map(|impl_def| self.impls.get(impl_def))
+                    .filter(|(_, generics)| generics.params.len() == 0)
+                    .map(|(span, _)| span);
+                if let Some(initial_span) = impl_spans.nth(0) {
+                    impl_spans.for_each(|additional_span| {
+                        cx.span_lint_note(
+                            MULTIPLE_INHERENT_IMPL,
+                            *additional_span,
+                            "Multiple implementations of this structure",
+                            *initial_span,
+                            "First implementation here",
+                        )
+                    })
+                }
+            }
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -139,6 +139,7 @@ pub mod if_let_redundant_pattern_matching;
 pub mod if_not_else;
 pub mod infallible_destructuring_match;
 pub mod infinite_iter;
+pub mod inherent_impl;
 pub mod inline_fn_without_body;
 pub mod int_plus_one;
 pub mod invalid_ref;
@@ -417,6 +418,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_early_lint_pass(box multiple_crate_versions::Pass);
     reg.register_late_lint_pass(box map_unit_fn::Pass);
     reg.register_late_lint_pass(box infallible_destructuring_match::Pass);
+    reg.register_late_lint_pass(box inherent_impl::Pass::default());
 
 
     reg.register_lint_group("clippy_restriction", vec![
@@ -425,6 +427,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         array_indexing::INDEXING_SLICING,
         assign_ops::ASSIGN_OPS,
         else_if_without_else::ELSE_IF_WITHOUT_ELSE,
+        inherent_impl::MULTIPLE_INHERENT_IMPL,
         literal_representation::DECIMAL_LITERAL_REPRESENTATION,
         mem_forget::MEM_FORGET,
         methods::CLONE_ON_REF_PTR,

tests/ui/impl.rs

@@ -0,0 +1,36 @@
+#![allow(dead_code)]
+#![warn(multiple_inherent_impl)]
+
+struct MyStruct;
+
+impl MyStruct {
+    fn first() {}
+}
+
+impl MyStruct {
+    fn second() {}
+}
+
+impl<'a> MyStruct {
+    fn lifetimed() {}
+}
+
+mod submod {
+    struct MyStruct;
+    impl MyStruct {
+        fn other() {}
+    }
+
+    impl super::MyStruct {
+        fn third() {}
+    }
+}
+
+use std::fmt;
+impl fmt::Debug for MyStruct {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "MyStruct {{ }}")
+    }
+}
+
+fn main() {}

tests/ui/impl.stderr

@@ -0,0 +1,35 @@
+error: Multiple implementations of this structure
+  --> $DIR/impl.rs:10:1
+   |
+10 | / impl MyStruct {
+11 | |     fn second() {}
+12 | | }
+   | |_^
+   |
+   = note: `-D multiple-inherent-impl` implied by `-D warnings`
+note: First implementation here
+  --> $DIR/impl.rs:6:1
+   |
+6  | / impl MyStruct {
+7  | |     fn first() {}
+8  | | }
+   | |_^
+
+error: Multiple implementations of this structure
+  --> $DIR/impl.rs:24:5
+   |
+24 | /     impl super::MyStruct {
+25 | |         fn third() {}
+26 | |     }
+   | |_____^
+   |
+note: First implementation here
+  --> $DIR/impl.rs:6:1
+   |
+6  | / impl MyStruct {
+7  | |     fn first() {}
+8  | | }
+   | |_^
+
+error: aborting due to 2 previous errors
+