Auto merge of rust-lang#12386 - Ethiraric:fix-12381, r=blyxyas

[`use_self`]: Make it aware of lifetimes

Have the lint trigger even if `Self` has generic lifetime parameters.

```rs
impl<'a> Foo<'a> {
    type Item = Foo<'a>; // Can be replaced with Self

    fn new() -> Self {
        Foo { // No lifetime, but they are inferred to be that of Self
              // Can be replaced as well
            ...
        }
    }

    // Don't replace `Foo<'b>`, the lifetime is different!
    fn eq<'b>(self, other: Foo<'b>) -> bool {
        ..
    }
```

Fixes rust-lang#12381

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`use_self`]: Have the lint trigger even if `Self` has generic lifetime parameters

Author: bors

clippy_lints/src/use_self.rs

@@ -8,11 +8,12 @@ use rustc_hir::def::{CtorOf, DefKind, Res};
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::intravisit::{walk_inf, walk_ty, Visitor};
 use rustc_hir::{
-    self as hir, Expr, ExprKind, FnRetTy, FnSig, GenericArg, GenericArgsParentheses, GenericParam, GenericParamKind,
-    HirId, Impl, ImplItemKind, Item, ItemKind, Pat, PatKind, Path, QPath, Ty, TyKind,
+    self as hir, Expr, ExprKind, FnRetTy, FnSig, GenericArgsParentheses, GenericParam, GenericParamKind, HirId, Impl,
+    ImplItemKind, Item, ItemKind, Pat, PatKind, Path, QPath, Ty, TyKind,
 };
 use rustc_hir_analysis::hir_ty_to_ty;
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::Ty as MiddleTy;
 use rustc_session::impl_lint_pass;
 use rustc_span::Span;
 
@@ -95,10 +96,9 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf {
         let stack_item = if let ItemKind::Impl(Impl { self_ty, generics, .. }) = item.kind
             && let TyKind::Path(QPath::Resolved(_, item_path)) = self_ty.kind
             && let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args
-            && parameters.as_ref().map_or(true, |params| {
-                params.parenthesized == GenericArgsParentheses::No
-                    && !params.args.iter().any(|arg| matches!(arg, GenericArg::Lifetime(_)))
-            })
+            && parameters
+                .as_ref()
+                .map_or(true, |params| params.parenthesized == GenericArgsParentheses::No)
             && !item.span.from_expansion()
             && !is_from_proc_macro(cx, item)
         // expensive, should be last check
@@ -226,7 +226,12 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf {
             } else {
                 hir_ty_to_ty(cx.tcx, hir_ty)
             }
-            && same_type_and_consts(ty, cx.tcx.type_of(impl_id).instantiate_identity())
+            && let impl_ty = cx.tcx.type_of(impl_id).instantiate_identity()
+            && same_type_and_consts(ty, impl_ty)
+            // Ensure the type we encounter and the one from the impl have the same lifetime parameters. It may be that
+            // the lifetime parameters of `ty` are ellided (`impl<'a> Foo<'a> { fn new() -> Self { Foo{..} } }`, in
+            // which case we must still trigger the lint.
+            && (has_no_lifetime(ty) || same_lifetimes(ty, impl_ty))
         {
             span_lint(cx, hir_ty.span);
         }
@@ -318,3 +323,37 @@ fn lint_path_to_variant(cx: &LateContext<'_>, path: &Path<'_>) {
         span_lint(cx, span);
     }
 }
+
+/// Returns `true` if types `a` and `b` have the same lifetime parameters, otherwise returns
+/// `false`.
+///
+/// This function does not check that types `a` and `b` are the same types.
+fn same_lifetimes<'tcx>(a: MiddleTy<'tcx>, b: MiddleTy<'tcx>) -> bool {
+    use rustc_middle::ty::{Adt, GenericArgKind};
+    match (&a.kind(), &b.kind()) {
+        (&Adt(_, args_a), &Adt(_, args_b)) => {
+            args_a
+                .iter()
+                .zip(args_b.iter())
+                .all(|(arg_a, arg_b)| match (arg_a.unpack(), arg_b.unpack()) {
+                    // TODO: Handle inferred lifetimes
+                    (GenericArgKind::Lifetime(inner_a), GenericArgKind::Lifetime(inner_b)) => inner_a == inner_b,
+                    (GenericArgKind::Type(type_a), GenericArgKind::Type(type_b)) => same_lifetimes(type_a, type_b),
+                    _ => true,
+                })
+        },
+        _ => a == b,
+    }
+}
+
+/// Returns `true` if `ty` has no lifetime parameter, otherwise returns `false`.
+fn has_no_lifetime(ty: MiddleTy<'_>) -> bool {
+    use rustc_middle::ty::{Adt, GenericArgKind};
+    match ty.kind() {
+        &Adt(_, args) => !args
+            .iter()
+            // TODO: Handle inferred lifetimes
+            .any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(..))),
+        _ => true,
+    }
+}

tests/ui/use_self.fixed

@@ -6,7 +6,8 @@
     clippy::should_implement_trait,
     clippy::upper_case_acronyms,
     clippy::from_over_into,
-    clippy::self_named_constructors
+    clippy::self_named_constructors,
+    clippy::needless_lifetimes
 )]
 
 #[macro_use]
@@ -53,6 +54,7 @@ mod better {
 }
 
 mod lifetimes {
+    #[derive(Clone, Copy)]
     struct Foo<'a> {
         foo_str: &'a str,
     }
@@ -68,11 +70,19 @@ mod lifetimes {
             Foo { foo_str: "foo" }
         }
 
-        // FIXME: the lint does not handle lifetimed struct
-        // `Self` should be applicable here
-        fn clone(&self) -> Foo<'a> {
+        fn clone(&self) -> Self {
             Foo { foo_str: self.foo_str }
         }
+
+        // Cannot replace with `Self` because the lifetime is not `'a`.
+        fn eq<'b>(&self, other: Foo<'b>) -> bool {
+            let x: Foo<'_> = other;
+            self.foo_str == other.foo_str
+        }
+
+        fn f(&self) -> Foo<'_> {
+            *self
+        }
     }
 }
 

tests/ui/use_self.rs

@@ -6,7 +6,8 @@
     clippy::should_implement_trait,
     clippy::upper_case_acronyms,
     clippy::from_over_into,
-    clippy::self_named_constructors
+    clippy::self_named_constructors,
+    clippy::needless_lifetimes
 )]
 
 #[macro_use]
@@ -53,6 +54,7 @@ mod better {
 }
 
 mod lifetimes {
+    #[derive(Clone, Copy)]
     struct Foo<'a> {
         foo_str: &'a str,
     }
@@ -68,11 +70,19 @@ mod lifetimes {
             Foo { foo_str: "foo" }
         }
 
-        // FIXME: the lint does not handle lifetimed struct
-        // `Self` should be applicable here
         fn clone(&self) -> Foo<'a> {
             Foo { foo_str: self.foo_str }
         }
+
+        // Cannot replace with `Self` because the lifetime is not `'a`.
+        fn eq<'b>(&self, other: Foo<'b>) -> bool {
+            let x: Foo<'_> = other;
+            self.foo_str == other.foo_str
+        }
+
+        fn f(&self) -> Foo<'_> {
+            *self
+        }
     }
 }