Rollup merge of #5869 - wiomoc:feature/implicit-self, r=ebroto,flip1995

New lint against `Self` as an arbitrary self type

Fixes #5861

changelog: * [`needless_arbitrary_self_type`] [#5869](#5869)

Author: flip1995

CHANGELOG.md

@@ -1616,6 +1616,7 @@ Released 2018-09-13
 [`mutex_atomic`]: https://rust-lang.github.io/rust-clippy/master/index.html#mutex_atomic
 [`mutex_integer`]: https://rust-lang.github.io/rust-clippy/master/index.html#mutex_integer
 [`naive_bytecount`]: https://rust-lang.github.io/rust-clippy/master/index.html#naive_bytecount
+[`needless_arbitrary_self_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_arbitrary_self_type
 [`needless_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_bool
 [`needless_borrow`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
 [`needless_borrowed_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrowed_reference

clippy_lints/src/lib.rs

@@ -250,6 +250,7 @@ mod mut_mut;
 mod mut_reference;
 mod mutable_debug_assertion;
 mod mutex_atomic;
+mod needless_arbitrary_self_type;
 mod needless_bool;
 mod needless_borrow;
 mod needless_borrowed_ref;
@@ -718,6 +719,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL,
         &mutex_atomic::MUTEX_ATOMIC,
         &mutex_atomic::MUTEX_INTEGER,
+        &needless_arbitrary_self_type::NEEDLESS_ARBITRARY_SELF_TYPE,
         &needless_bool::BOOL_COMPARISON,
         &needless_bool::NEEDLESS_BOOL,
         &needless_borrow::NEEDLESS_BORROW,
@@ -1028,6 +1030,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| box items_after_statements::ItemsAfterStatements);
     store.register_early_pass(|| box precedence::Precedence);
     store.register_early_pass(|| box needless_continue::NeedlessContinue);
+    store.register_early_pass(|| box needless_arbitrary_self_type::NeedlessArbitrarySelfType);
     store.register_early_pass(|| box redundant_static_lifetimes::RedundantStaticLifetimes);
     store.register_late_pass(|| box cargo_common_metadata::CargoCommonMetadata);
     store.register_late_pass(|| box multiple_crate_versions::MultipleCrateVersions);
@@ -1373,6 +1376,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&mut_key::MUTABLE_KEY_TYPE),
         LintId::of(&mut_reference::UNNECESSARY_MUT_PASSED),
         LintId::of(&mutex_atomic::MUTEX_ATOMIC),
+        LintId::of(&needless_arbitrary_self_type::NEEDLESS_ARBITRARY_SELF_TYPE),
         LintId::of(&needless_bool::BOOL_COMPARISON),
         LintId::of(&needless_bool::NEEDLESS_BOOL),
         LintId::of(&needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE),
@@ -1605,6 +1609,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&misc::SHORT_CIRCUIT_STATEMENT),
         LintId::of(&misc_early::UNNEEDED_WILDCARD_PATTERN),
         LintId::of(&misc_early::ZERO_PREFIXED_LITERAL),
+        LintId::of(&needless_arbitrary_self_type::NEEDLESS_ARBITRARY_SELF_TYPE),
         LintId::of(&needless_bool::BOOL_COMPARISON),
         LintId::of(&needless_bool::NEEDLESS_BOOL),
         LintId::of(&needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE),

clippy_lints/src/needless_arbitrary_self_type.rs

@@ -0,0 +1,118 @@
+use crate::utils::span_lint_and_sugg;
+use if_chain::if_chain;
+use rustc_ast::ast::{BindingMode, Lifetime, Mutability, Param, PatKind, Path, TyKind};
+use rustc_errors::Applicability;
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::symbol::kw;
+use rustc_span::Span;
+
+declare_clippy_lint! {
+    /// **What it does:** The lint checks for `self` in fn parameters that
+    /// specify the `Self`-type explicitly
+    /// **Why is this bad?** Increases the amount and decreases the readability of code
+    ///
+    /// **Known problems:** None
+    ///
+    /// **Example:**
+    /// ```rust
+    /// enum ValType {
+    ///     I32,
+    ///     I64,
+    ///     F32,
+    ///     F64,
+    /// }
+    ///
+    /// impl ValType {
+    ///     pub fn bytes(self: Self) -> usize {
+    ///         match self {
+    ///             Self::I32 | Self::F32 => 4,
+    ///             Self::I64 | Self::F64 => 8,
+    ///         }
+    ///     }
+    /// }
+    /// ```
+    ///
+    /// Could be rewritten as
+    ///
+    /// ```rust
+    /// enum ValType {
+    ///     I32,
+    ///     I64,
+    ///     F32,
+    ///     F64,
+    /// }
+    ///
+    /// impl ValType {
+    ///     pub fn bytes(self) -> usize {
+    ///         match self {
+    ///             Self::I32 | Self::F32 => 4,
+    ///             Self::I64 | Self::F64 => 8,
+    ///         }
+    ///     }
+    /// }
+    /// ```
+    pub NEEDLESS_ARBITRARY_SELF_TYPE,
+    complexity,
+    "type of `self` parameter is already by default `Self`"
+}
+
+declare_lint_pass!(NeedlessArbitrarySelfType => [NEEDLESS_ARBITRARY_SELF_TYPE]);
+
+enum Mode {
+    Ref(Option<Lifetime>),
+    Value,
+}
+
+fn check_param_inner(cx: &EarlyContext<'_>, path: &Path, span: Span, binding_mode: &Mode, mutbl: Mutability) {
+    if_chain! {
+        if let [segment] = &path.segments[..];
+        if segment.ident.name == kw::SelfUpper;
+        then {
+            let self_param = match (binding_mode, mutbl) {
+                (Mode::Ref(None), Mutability::Mut) => "&mut self".to_string(),
+                (Mode::Ref(Some(lifetime)), Mutability::Mut) => format!("&{} mut self", &lifetime.ident.name),
+                (Mode::Ref(None), Mutability::Not) => "&self".to_string(),
+                (Mode::Ref(Some(lifetime)), Mutability::Not) => format!("&{} self", &lifetime.ident.name),
+                (Mode::Value, Mutability::Mut) => "mut self".to_string(),
+                (Mode::Value, Mutability::Not) => "self".to_string(),
+            };
+
+            span_lint_and_sugg(
+                cx,
+                NEEDLESS_ARBITRARY_SELF_TYPE,
+                span,
+                "the type of the `self` parameter does not need to be arbitrary",
+                "consider to change this parameter to",
+                self_param,
+                Applicability::MachineApplicable,
+            )
+        }
+    }
+}
+
+impl EarlyLintPass for NeedlessArbitrarySelfType {
+    fn check_param(&mut self, cx: &EarlyContext<'_>, p: &Param) {
+        if !p.is_self() {
+            return;
+        }
+
+        match &p.ty.kind {
+            TyKind::Path(None, path) => {
+                if let PatKind::Ident(BindingMode::ByValue(mutbl), _, _) = p.pat.kind {
+                    check_param_inner(cx, path, p.span.to(p.ty.span), &Mode::Value, mutbl)
+                }
+            },
+            TyKind::Rptr(lifetime, mut_ty) => {
+                if_chain! {
+                if let TyKind::Path(None, path) = &mut_ty.ty.kind;
+                if let PatKind::Ident(BindingMode::ByValue(Mutability::Not), _, _) = p.pat.kind;
+                    then {
+                        check_param_inner(cx, path, p.span.to(p.ty.span), &Mode::Ref(*lifetime), mut_ty.mutbl)
+                    }
+                }
+            },
+            _ => {},
+        }
+    }
+}

clippy_lints/src/trait_bounds.rs

@@ -50,7 +50,7 @@ declare_clippy_lint! {
     /// fn func<T: Clone + Default>(arg: T) {}
     /// ```
     /// or
-    ///  ///
+    ///
     /// ```rust
     /// fn func<T>(arg: T) where T: Clone + Default {}
     /// ```

src/lintlist/mod.rs

@@ -1459,6 +1459,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "bytecount",
     },
+    Lint {
+        name: "needless_arbitrary_self_type",
+        group: "complexity",
+        desc: "type of `self` parameter is already by default `Self`",
+        deprecation: None,
+        module: "needless_arbitrary_self_type",
+    },
     Lint {
         name: "needless_bool",
         group: "complexity",

tests/ui/extra_unused_lifetimes.rs

@@ -1,4 +1,10 @@
-#![allow(unused, dead_code, clippy::needless_lifetimes, clippy::needless_pass_by_value)]
+#![allow(
+    unused,
+    dead_code,
+    clippy::needless_lifetimes,
+    clippy::needless_pass_by_value,
+    clippy::needless_arbitrary_self_type
+)]
 #![warn(clippy::extra_unused_lifetimes)]
 
 fn empty() {}

tests/ui/extra_unused_lifetimes.stderr

@@ -1,25 +1,25 @@
 error: this lifetime isn't used in the function definition
-  --> $DIR/extra_unused_lifetimes.rs:8:14
+  --> $DIR/extra_unused_lifetimes.rs:14:14
    |
 LL | fn unused_lt<'a>(x: u8) {}
    |              ^^
    |
    = note: `-D clippy::extra-unused-lifetimes` implied by `-D warnings`
 
 error: this lifetime isn't used in the function definition
-  --> $DIR/extra_unused_lifetimes.rs:10:25
+  --> $DIR/extra_unused_lifetimes.rs:16:25
    |
 LL | fn unused_lt_transitive<'a, 'b: 'a>(x: &'b u8) {
    |                         ^^
 
 error: this lifetime isn't used in the function definition
-  --> $DIR/extra_unused_lifetimes.rs:35:10
+  --> $DIR/extra_unused_lifetimes.rs:41:10
    |
 LL |     fn x<'a>(&self) {}
    |          ^^
 
 error: this lifetime isn't used in the function definition
-  --> $DIR/extra_unused_lifetimes.rs:61:22
+  --> $DIR/extra_unused_lifetimes.rs:67:22
    |
 LL |         fn unused_lt<'a>(x: u8) {}
    |                      ^^

tests/ui/len_without_is_empty.rs

@@ -4,14 +4,14 @@
 pub struct PubOne;
 
 impl PubOne {
-    pub fn len(self: &Self) -> isize {
+    pub fn len(&self) -> isize {
         1
     }
 }
 
 impl PubOne {
     // A second impl for this struct -- the error span shouldn't mention this.
-    pub fn irrelevant(self: &Self) -> bool {
+    pub fn irrelevant(&self) -> bool {
         false
     }
 }
@@ -21,57 +21,57 @@ pub struct PubAllowed;
 
 #[allow(clippy::len_without_is_empty)]
 impl PubAllowed {
-    pub fn len(self: &Self) -> isize {
+    pub fn len(&self) -> isize {
         1
     }
 }
 
 // No `allow` attribute on this impl block, but that doesn't matter -- we only require one on the
 // impl containing `len`.
 impl PubAllowed {
-    pub fn irrelevant(self: &Self) -> bool {
+    pub fn irrelevant(&self) -> bool {
         false
     }
 }
 
 pub trait PubTraitsToo {
-    fn len(self: &Self) -> isize;
+    fn len(&self) -> isize;
 }
 
 impl PubTraitsToo for One {
-    fn len(self: &Self) -> isize {
+    fn len(&self) -> isize {
         0
     }
 }
 
 pub struct HasIsEmpty;
 
 impl HasIsEmpty {
-    pub fn len(self: &Self) -> isize {
+    pub fn len(&self) -> isize {
         1
     }
 
-    fn is_empty(self: &Self) -> bool {
+    fn is_empty(&self) -> bool {
         false
     }
 }
 
 pub struct HasWrongIsEmpty;
 
 impl HasWrongIsEmpty {
-    pub fn len(self: &Self) -> isize {
+    pub fn len(&self) -> isize {
         1
     }
 
-    pub fn is_empty(self: &Self, x: u32) -> bool {
+    pub fn is_empty(&self, x: u32) -> bool {
         false
     }
 }
 
 struct NotPubOne;
 
 impl NotPubOne {
-    pub fn len(self: &Self) -> isize {
+    pub fn len(&self) -> isize {
         // No error; `len` is pub but `NotPubOne` is not exported anyway.
         1
     }
@@ -80,48 +80,48 @@ impl NotPubOne {
 struct One;
 
 impl One {
-    fn len(self: &Self) -> isize {
+    fn len(&self) -> isize {
         // No error; `len` is private; see issue #1085.
         1
     }
 }
 
 trait TraitsToo {
-    fn len(self: &Self) -> isize;
+    fn len(&self) -> isize;
     // No error; `len` is private; see issue #1085.
 }
 
 impl TraitsToo for One {
-    fn len(self: &Self) -> isize {
+    fn len(&self) -> isize {
         0
     }
 }
 
 struct HasPrivateIsEmpty;
 
 impl HasPrivateIsEmpty {
-    pub fn len(self: &Self) -> isize {
+    pub fn len(&self) -> isize {
         1
     }
 
-    fn is_empty(self: &Self) -> bool {
+    fn is_empty(&self) -> bool {
         false
     }
 }
 
 struct Wither;
 
 pub trait WithIsEmpty {
-    fn len(self: &Self) -> isize;
-    fn is_empty(self: &Self) -> bool;
+    fn len(&self) -> isize;
+    fn is_empty(&self) -> bool;
 }
 
 impl WithIsEmpty for Wither {
-    fn len(self: &Self) -> isize {
+    fn len(&self) -> isize {
         1
     }
 
-    fn is_empty(self: &Self) -> bool {
+    fn is_empty(&self) -> bool {
         false
     }
 }