Skip to content

Commit f3879b3

Browse files
committed
[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
1 parent 93f0a9a commit f3879b3

File tree

4 files changed

+122
-57
lines changed

4 files changed

+122
-57
lines changed

clippy_lints/src/use_self.rs

+46-7
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,12 @@ use rustc_hir::def::{CtorOf, DefKind, Res};
88
use rustc_hir::def_id::LocalDefId;
99
use rustc_hir::intravisit::{walk_inf, walk_ty, Visitor};
1010
use rustc_hir::{
11-
self as hir, Expr, ExprKind, FnRetTy, FnSig, GenericArg, GenericArgsParentheses, GenericParam, GenericParamKind,
12-
HirId, Impl, ImplItemKind, Item, ItemKind, Pat, PatKind, Path, QPath, Ty, TyKind,
11+
self as hir, Expr, ExprKind, FnRetTy, FnSig, GenericArgsParentheses, GenericParam, GenericParamKind, HirId, Impl,
12+
ImplItemKind, Item, ItemKind, Pat, PatKind, Path, QPath, Ty, TyKind,
1313
};
1414
use rustc_hir_analysis::hir_ty_to_ty;
1515
use rustc_lint::{LateContext, LateLintPass};
16+
use rustc_middle::ty::Ty as MiddleTy;
1617
use rustc_session::impl_lint_pass;
1718
use rustc_span::Span;
1819

@@ -95,10 +96,9 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf {
9596
let stack_item = if let ItemKind::Impl(Impl { self_ty, generics, .. }) = item.kind
9697
&& let TyKind::Path(QPath::Resolved(_, item_path)) = self_ty.kind
9798
&& let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args
98-
&& parameters.as_ref().map_or(true, |params| {
99-
params.parenthesized == GenericArgsParentheses::No
100-
&& !params.args.iter().any(|arg| matches!(arg, GenericArg::Lifetime(_)))
101-
})
99+
&& parameters
100+
.as_ref()
101+
.map_or(true, |params| params.parenthesized == GenericArgsParentheses::No)
102102
&& !item.span.from_expansion()
103103
&& !is_from_proc_macro(cx, item)
104104
// expensive, should be last check
@@ -226,7 +226,12 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf {
226226
} else {
227227
hir_ty_to_ty(cx.tcx, hir_ty)
228228
}
229-
&& same_type_and_consts(ty, cx.tcx.type_of(impl_id).instantiate_identity())
229+
&& let impl_ty = cx.tcx.type_of(impl_id).instantiate_identity()
230+
&& same_type_and_consts(ty, impl_ty)
231+
// Ensure the type we encounter and the one from the impl have the same lifetime parameters. It may be that
232+
// the lifetime parameters of `ty` are ellided (`impl<'a> Foo<'a> { fn new() -> Self { Foo{..} } }`, in
233+
// which case we must still trigger the lint.
234+
&& (has_no_lifetime(ty) || same_lifetimes(ty, impl_ty))
230235
{
231236
span_lint(cx, hir_ty.span);
232237
}
@@ -318,3 +323,37 @@ fn lint_path_to_variant(cx: &LateContext<'_>, path: &Path<'_>) {
318323
span_lint(cx, span);
319324
}
320325
}
326+
327+
/// Returns `true` if types `a` and `b` have the same lifetime parameters, otherwise returns
328+
/// `false`.
329+
///
330+
/// This function does not check that types `a` and `b` are the same types.
331+
fn same_lifetimes<'tcx>(a: MiddleTy<'tcx>, b: MiddleTy<'tcx>) -> bool {
332+
use rustc_middle::ty::{Adt, GenericArgKind};
333+
match (&a.kind(), &b.kind()) {
334+
(&Adt(_, args_a), &Adt(_, args_b)) => {
335+
args_a
336+
.iter()
337+
.zip(args_b.iter())
338+
.all(|(arg_a, arg_b)| match (arg_a.unpack(), arg_b.unpack()) {
339+
// TODO: Handle inferred lifetimes
340+
(GenericArgKind::Lifetime(inner_a), GenericArgKind::Lifetime(inner_b)) => inner_a == inner_b,
341+
(GenericArgKind::Type(type_a), GenericArgKind::Type(type_b)) => same_lifetimes(type_a, type_b),
342+
_ => true,
343+
})
344+
},
345+
_ => a == b,
346+
}
347+
}
348+
349+
/// Returns `true` if `ty` has no lifetime parameter, otherwise returns `false`.
350+
fn has_no_lifetime(ty: MiddleTy<'_>) -> bool {
351+
use rustc_middle::ty::{Adt, GenericArgKind};
352+
match ty.kind() {
353+
&Adt(_, args) => !args
354+
.iter()
355+
// TODO: Handle inferred lifetimes
356+
.any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(..))),
357+
_ => true,
358+
}
359+
}

tests/ui/use_self.fixed

+14-4
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
clippy::should_implement_trait,
77
clippy::upper_case_acronyms,
88
clippy::from_over_into,
9-
clippy::self_named_constructors
9+
clippy::self_named_constructors,
10+
clippy::needless_lifetimes
1011
)]
1112

1213
#[macro_use]
@@ -53,6 +54,7 @@ mod better {
5354
}
5455

5556
mod lifetimes {
57+
#[derive(Clone, Copy)]
5658
struct Foo<'a> {
5759
foo_str: &'a str,
5860
}
@@ -68,11 +70,19 @@ mod lifetimes {
6870
Foo { foo_str: "foo" }
6971
}
7072

71-
// FIXME: the lint does not handle lifetimed struct
72-
// `Self` should be applicable here
73-
fn clone(&self) -> Foo<'a> {
73+
fn clone(&self) -> Self {
7474
Foo { foo_str: self.foo_str }
7575
}
76+
77+
// Cannot replace with `Self` because the lifetime is not `'a`.
78+
fn eq<'b>(&self, other: Foo<'b>) -> bool {
79+
let x: Foo<'_> = other;
80+
self.foo_str == other.foo_str
81+
}
82+
83+
fn f(&self) -> Foo<'_> {
84+
*self
85+
}
7686
}
7787
}
7888

tests/ui/use_self.rs

+13-3
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
clippy::should_implement_trait,
77
clippy::upper_case_acronyms,
88
clippy::from_over_into,
9-
clippy::self_named_constructors
9+
clippy::self_named_constructors,
10+
clippy::needless_lifetimes
1011
)]
1112

1213
#[macro_use]
@@ -53,6 +54,7 @@ mod better {
5354
}
5455

5556
mod lifetimes {
57+
#[derive(Clone, Copy)]
5658
struct Foo<'a> {
5759
foo_str: &'a str,
5860
}
@@ -68,11 +70,19 @@ mod lifetimes {
6870
Foo { foo_str: "foo" }
6971
}
7072

71-
// FIXME: the lint does not handle lifetimed struct
72-
// `Self` should be applicable here
7373
fn clone(&self) -> Foo<'a> {
7474
Foo { foo_str: self.foo_str }
7575
}
76+
77+
// Cannot replace with `Self` because the lifetime is not `'a`.
78+
fn eq<'b>(&self, other: Foo<'b>) -> bool {
79+
let x: Foo<'_> = other;
80+
self.foo_str == other.foo_str
81+
}
82+
83+
fn f(&self) -> Foo<'_> {
84+
*self
85+
}
7686
}
7787
}
7888

0 commit comments

Comments
 (0)