about summary refs log tree commit diff
diff options
context:
space:
mode:
authorEthiraric <ethiraric@gmail.com>2024-02-29 14:13:54 +0100
committerEthiraric <ethiraric@gmail.com>2024-03-07 20:45:59 +0100
commitf3879b3630f80f18fe2fbdbf048e3b693727c05b (patch)
tree78c66eef3c9e57257bebafadd4e17528564cef17
parent93f0a9a91f58c9b2153868f458402155fb6265bb (diff)
downloadrust-f3879b3630f80f18fe2fbdbf048e3b693727c05b.tar.gz
rust-f3879b3630f80f18fe2fbdbf048e3b693727c05b.zip
[`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 #12381
-rw-r--r--clippy_lints/src/use_self.rs53
-rw-r--r--tests/ui/use_self.fixed18
-rw-r--r--tests/ui/use_self.rs16
-rw-r--r--tests/ui/use_self.stderr92
4 files changed, 122 insertions, 57 deletions
diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs
index a1b08d105b9..b28037db112 100644
--- a/clippy_lints/src/use_self.rs
+++ b/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,
+    }
+}
diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed
index 787dd3ec7e6..6ea7857a238 100644
--- a/tests/ui/use_self.fixed
+++ b/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
+        }
     }
 }
 
diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs
index 39e182faea6..338cc00e45a 100644
--- a/tests/ui/use_self.rs
+++ b/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
+        }
     }
 }
 
diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr
index 8d045f05ed2..d7aa8410a47 100644
--- a/tests/ui/use_self.stderr
+++ b/tests/ui/use_self.stderr
@@ -1,5 +1,5 @@
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:21:21
+  --> tests/ui/use_self.rs:22:21
    |
 LL |         fn new() -> Foo {
    |                     ^^^ help: use the applicable keyword: `Self`
@@ -8,250 +8,256 @@ LL |         fn new() -> Foo {
    = help: to override `-D warnings` add `#[allow(clippy::use_self)]`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:22:13
+  --> tests/ui/use_self.rs:23:13
    |
 LL |             Foo {}
    |             ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:24:22
+  --> tests/ui/use_self.rs:25:22
    |
 LL |         fn test() -> Foo {
    |                      ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:25:13
+  --> tests/ui/use_self.rs:26:13
    |
 LL |             Foo::new()
    |             ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:30:25
+  --> tests/ui/use_self.rs:31:25
    |
 LL |         fn default() -> Foo {
    |                         ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:31:13
+  --> tests/ui/use_self.rs:32:13
    |
 LL |             Foo::new()
    |             ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:96:24
+  --> tests/ui/use_self.rs:73:28
+   |
+LL |         fn clone(&self) -> Foo<'a> {
+   |                            ^^^^^^^ help: use the applicable keyword: `Self`
+
+error: unnecessary structure name repetition
+  --> tests/ui/use_self.rs:106:24
    |
 LL |         fn bad(foos: &[Foo]) -> impl Iterator<Item = &Foo> {
    |                        ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:96:55
+  --> tests/ui/use_self.rs:106:55
    |
 LL |         fn bad(foos: &[Foo]) -> impl Iterator<Item = &Foo> {
    |                                                       ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:111:13
+  --> tests/ui/use_self.rs:121:13
    |
 LL |             TS(0)
    |             ^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:146:29
+  --> tests/ui/use_self.rs:156:29
    |
 LL |                 fn bar() -> Bar {
    |                             ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:147:21
+  --> tests/ui/use_self.rs:157:21
    |
 LL |                     Bar { foo: Foo {} }
    |                     ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:158:21
+  --> tests/ui/use_self.rs:168:21
    |
 LL |         fn baz() -> Foo {
    |                     ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:159:13
+  --> tests/ui/use_self.rs:169:13
    |
 LL |             Foo {}
    |             ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:176:21
+  --> tests/ui/use_self.rs:186:21
    |
 LL |             let _ = Enum::B(42);
    |                     ^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:177:21
+  --> tests/ui/use_self.rs:187:21
    |
 LL |             let _ = Enum::C { field: true };
    |                     ^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:178:21
+  --> tests/ui/use_self.rs:188:21
    |
 LL |             let _ = Enum::A;
    |                     ^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:220:13
+  --> tests/ui/use_self.rs:230:13
    |
 LL |             nested::A::fun_1();
    |             ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:221:13
+  --> tests/ui/use_self.rs:231:13
    |
 LL |             nested::A::A;
    |             ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:223:13
+  --> tests/ui/use_self.rs:233:13
    |
 LL |             nested::A {};
    |             ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:242:13
+  --> tests/ui/use_self.rs:252:13
    |
 LL |             TestStruct::from_something()
    |             ^^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:256:25
+  --> tests/ui/use_self.rs:266:25
    |
 LL |         async fn g() -> S {
    |                         ^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:257:13
+  --> tests/ui/use_self.rs:267:13
    |
 LL |             S {}
    |             ^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:261:16
+  --> tests/ui/use_self.rs:271:16
    |
 LL |             &p[S::A..S::B]
    |                ^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:261:22
+  --> tests/ui/use_self.rs:271:22
    |
 LL |             &p[S::A..S::B]
    |                      ^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:284:29
+  --> tests/ui/use_self.rs:294:29
    |
 LL |         fn foo(value: T) -> Foo<T> {
    |                             ^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:285:13
+  --> tests/ui/use_self.rs:295:13
    |
 LL |             Foo::<T> { value }
    |             ^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:457:13
+  --> tests/ui/use_self.rs:467:13
    |
 LL |             A::new::<submod::B>(submod::B {})
    |             ^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:494:13
+  --> tests/ui/use_self.rs:504:13
    |
 LL |             S2::new()
    |             ^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:531:17
+  --> tests/ui/use_self.rs:541:17
    |
 LL |                 Foo::Bar => unimplemented!(),
    |                 ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:532:17
+  --> tests/ui/use_self.rs:542:17
    |
 LL |                 Foo::Baz => unimplemented!(),
    |                 ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:538:20
+  --> tests/ui/use_self.rs:548:20
    |
 LL |             if let Foo::Bar = self {
    |                    ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:562:17
+  --> tests/ui/use_self.rs:572:17
    |
 LL |                 Something::Num(n) => *n,
    |                 ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:563:17
+  --> tests/ui/use_self.rs:573:17
    |
 LL |                 Something::TupleNums(n, _m) => *n,
    |                 ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:564:17
+  --> tests/ui/use_self.rs:574:17
    |
 LL |                 Something::StructNums { one, two: _ } => *one,
    |                 ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:570:17
+  --> tests/ui/use_self.rs:580:17
    |
 LL |                 crate::issue8845::Something::Num(n) => *n,
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:571:17
+  --> tests/ui/use_self.rs:581:17
    |
 LL |                 crate::issue8845::Something::TupleNums(n, _m) => *n,
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:572:17
+  --> tests/ui/use_self.rs:582:17
    |
 LL |                 crate::issue8845::Something::StructNums { one, two: _ } => *one,
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:588:17
+  --> tests/ui/use_self.rs:598:17
    |
 LL |             let Foo(x) = self;
    |                 ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:593:17
+  --> tests/ui/use_self.rs:603:17
    |
 LL |             let crate::issue8845::Foo(x) = self;
    |                 ^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:600:17
+  --> tests/ui/use_self.rs:610:17
    |
 LL |             let Bar { x, .. } = self;
    |                 ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:605:17
+  --> tests/ui/use_self.rs:615:17
    |
 LL |             let crate::issue8845::Bar { x, .. } = self;
    |                 ^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> tests/ui/use_self.rs:644:17
+  --> tests/ui/use_self.rs:654:17
    |
 LL |                 E::A => {},
    |                 ^ help: use the applicable keyword: `Self`
 
-error: aborting due to 42 previous errors
+error: aborting due to 43 previous errors