diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index f8e1aff33e77..4d518ba98e6b 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -1,19 +1,18 @@ use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::def::{DefKind, Res}; -use rustc_hir::intravisit::{walk_item, walk_path, walk_ty, NestedVisitorMap, Visitor}; +use rustc_hir::intravisit::{walk_expr, walk_impl_item, walk_ty, NestedVisitorMap, Visitor}; use rustc_hir::{ - def, FnDecl, FnRetTy, FnSig, GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Path, PathSegment, QPath, - TyKind, + def, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, ImplItem, ImplItemKind, Item, ItemKind, Path, PathSegment, + QPath, TyKind, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; use rustc_middle::lint::in_external_macro; use rustc_middle::ty; -use rustc_middle::ty::{DefIdTree, Ty}; +use rustc_middle::ty::Ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::symbol::kw; +use rustc_span::Span; use rustc_typeck::hir_ty_to_ty; use crate::utils::{differing_macro_contexts, span_lint_and_sugg}; @@ -81,15 +80,87 @@ fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path<'_>, last_segment: O ); } -// FIXME: always use this (more correct) visitor, not just in method signatures. -struct SemanticUseSelfVisitor<'a, 'tcx> { +struct ImplVisitor<'a, 'tcx> { cx: &'a LateContext<'a, 'tcx>, + item: &'tcx Item<'tcx>, self_ty: Ty<'tcx>, } -impl<'a, 'tcx> Visitor<'tcx> for SemanticUseSelfVisitor<'a, 'tcx> { +impl<'a, 'tcx> ImplVisitor<'a, 'tcx> { + fn check_trait_method_impl_decl( + &mut self, + impl_item: &ImplItem<'tcx>, + impl_decl: &'tcx FnDecl<'tcx>, + impl_trait_ref: ty::TraitRef<'tcx>, + ) { + let tcx = self.cx.tcx; + let trait_method = tcx + .associated_items(impl_trait_ref.def_id) + .find_by_name_and_kind(tcx, impl_item.ident, ty::AssocKind::Fn, impl_trait_ref.def_id) + .expect("impl method matches a trait method"); + + let trait_method_sig = tcx.fn_sig(trait_method.def_id); + let trait_method_sig = tcx.erase_late_bound_regions(&trait_method_sig); + + let output_hir_ty = if let FnRetTy::Return(ty) = &impl_decl.output { + Some(&**ty) + } else { + None + }; + + // `impl_hir_ty` (of type `hir::Ty`) represents the type written in the signature. + // `trait_ty` (of type `ty::Ty`) is the semantic type for the signature in the trait. + // We use `impl_hir_ty` to see if the type was written as `Self`, + // `hir_ty_to_ty(...)` to check semantic types of paths, and + // `trait_ty` to determine which parts of the signature in the trait, mention + // the type being implemented verbatim (as opposed to `Self`). + for (impl_hir_ty, trait_ty) in impl_decl + .inputs + .iter() + .chain(output_hir_ty) + .zip(trait_method_sig.inputs_and_output) + { + // Check if the input/output type in the trait method specifies the implemented + // type verbatim, and only suggest `Self` if that isn't the case. + // This avoids suggestions to e.g. replace `Vec` with `Vec`, + // in an `impl Trait for u8`, when the trait always uses `Vec`. + // See also https://github.com/rust-lang/rust-clippy/issues/2894. + let self_ty = impl_trait_ref.self_ty(); + if !trait_ty.walk().any(|inner| inner == self_ty.into()) { + self.visit_ty(&impl_hir_ty); + } + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for ImplVisitor<'a, 'tcx> { type Map = Map<'tcx>; + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + if let ExprKind::Struct(QPath::Resolved(_, path), ..) = expr.kind { + if self + .cx + .tcx + .typeck_tables_of(expr.hir_id.owner.to_def_id()) + .node_type(expr.hir_id) + == self.self_ty + { + match path.res { + def::Res::SelfTy(..) => {}, + _ => { + span_use_self_lint(self.cx, path, None); + }, + } + span_use_self_lint(self.cx, path, None); + } + } + walk_expr(self, expr) + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) + } + fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'_>) { if let TyKind::Path(QPath::Resolved(_, path)) = &hir_ty.kind { match path.res { @@ -105,54 +176,20 @@ impl<'a, 'tcx> Visitor<'tcx> for SemanticUseSelfVisitor<'a, 'tcx> { walk_ty(self, hir_ty) } - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::None - } -} - -fn check_trait_method_impl_decl<'a, 'tcx>( - cx: &'a LateContext<'a, 'tcx>, - impl_item: &ImplItem<'_>, - impl_decl: &'tcx FnDecl<'_>, - impl_trait_ref: ty::TraitRef<'tcx>, -) { - let trait_method = cx - .tcx - .associated_items(impl_trait_ref.def_id) - .find_by_name_and_kind(cx.tcx, impl_item.ident, ty::AssocKind::Fn, impl_trait_ref.def_id) - .expect("impl method matches a trait method"); - - let trait_method_sig = cx.tcx.fn_sig(trait_method.def_id); - let trait_method_sig = cx.tcx.erase_late_bound_regions(&trait_method_sig); - - let output_hir_ty = if let FnRetTy::Return(ty) = &impl_decl.output { - Some(&**ty) - } else { - None - }; - - // `impl_hir_ty` (of type `hir::Ty`) represents the type written in the signature. - // `trait_ty` (of type `ty::Ty`) is the semantic type for the signature in the trait. - // We use `impl_hir_ty` to see if the type was written as `Self`, - // `hir_ty_to_ty(...)` to check semantic types of paths, and - // `trait_ty` to determine which parts of the signature in the trait, mention - // the type being implemented verbatim (as opposed to `Self`). - for (impl_hir_ty, trait_ty) in impl_decl - .inputs - .iter() - .chain(output_hir_ty) - .zip(trait_method_sig.inputs_and_output) - { - // Check if the input/output type in the trait method specifies the implemented - // type verbatim, and only suggest `Self` if that isn't the case. - // This avoids suggestions to e.g. replace `Vec` with `Vec`, - // in an `impl Trait for u8`, when the trait always uses `Vec`. - // See also https://github.com/rust-lang/rust-clippy/issues/2894. - let self_ty = impl_trait_ref.self_ty(); - if !trait_ty.walk().any(|inner| inner == self_ty.into()) { - let mut visitor = SemanticUseSelfVisitor { cx, self_ty }; - - visitor.visit_ty(&impl_hir_ty); + fn visit_impl_item(&mut self, ii: &'tcx ImplItem<'tcx>) { + let tcx = self.cx.tcx; + let impl_def_id = tcx.hir().local_def_id(self.item.hir_id); + let impl_trait_ref = tcx.impl_trait_ref(impl_def_id); + if_chain! { + if let Some(impl_trait_ref) = impl_trait_ref; + if let ImplItemKind::Fn(FnSig { decl: impl_decl, .. }, impl_body_id) = &ii.kind; + then { + self.check_trait_method_impl_decl(ii, impl_decl, impl_trait_ref); + let body = tcx.hir().body(*impl_body_id); + self.visit_body(body); + } else { + walk_impl_item(self, ii) + } } } } @@ -177,96 +214,19 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf { }; if should_check { - let visitor = &mut UseSelfVisitor { - item_path, + let self_ty= hir_ty_to_ty(cx.tcx, item_type); + let visitor = &mut ImplVisitor { cx, + item, + self_ty, }; - let impl_def_id = cx.tcx.hir().local_def_id(item.hir_id); - let impl_trait_ref = cx.tcx.impl_trait_ref(impl_def_id); - - if let Some(impl_trait_ref) = impl_trait_ref { - for impl_item_ref in refs { - let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id); - if let ImplItemKind::Fn(FnSig{ decl: impl_decl, .. }, impl_body_id) - = &impl_item.kind { - check_trait_method_impl_decl(cx, impl_item, impl_decl, impl_trait_ref); - let body = cx.tcx.hir().body(*impl_body_id); - visitor.visit_body(body); - } else { - visitor.visit_impl_item(impl_item); - } - } - } else { - for impl_item_ref in refs { - let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id); - visitor.visit_impl_item(impl_item); - } + for impl_item_ref in refs { + let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id); + visitor.visit_impl_item(impl_item); } } } } } } - -struct UseSelfVisitor<'a, 'tcx> { - item_path: &'a Path<'a>, - cx: &'a LateContext<'a, 'tcx>, -} - -impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_path(&mut self, path: &'tcx Path<'_>, _id: HirId) { - if !path.segments.iter().any(|p| p.ident.span.is_dummy()) { - if path.segments.len() >= 2 { - let last_but_one = &path.segments[path.segments.len() - 2]; - if last_but_one.ident.name != kw::SelfUpper { - let enum_def_id = match path.res { - Res::Def(DefKind::Variant, variant_def_id) => self.cx.tcx.parent(variant_def_id), - Res::Def(DefKind::Ctor(def::CtorOf::Variant, _), ctor_def_id) => { - let variant_def_id = self.cx.tcx.parent(ctor_def_id); - variant_def_id.and_then(|def_id| self.cx.tcx.parent(def_id)) - }, - _ => None, - }; - - if self.item_path.res.opt_def_id() == enum_def_id { - span_use_self_lint(self.cx, path, Some(last_but_one)); - } - } - } - - if path.segments.last().expect(SEGMENTS_MSG).ident.name != kw::SelfUpper { - if self.item_path.res == path.res { - span_use_self_lint(self.cx, path, None); - } else if let Res::Def(DefKind::Ctor(def::CtorOf::Struct, _), ctor_def_id) = path.res { - if self.item_path.res.opt_def_id() == self.cx.tcx.parent(ctor_def_id) { - span_use_self_lint(self.cx, path, None); - } - } - } - } - - walk_path(self, path); - } - - fn visit_item(&mut self, item: &'tcx Item<'_>) { - match item.kind { - ItemKind::Use(..) - | ItemKind::Static(..) - | ItemKind::Enum(..) - | ItemKind::Struct(..) - | ItemKind::Union(..) - | ItemKind::Impl { .. } - | ItemKind::Fn(..) => { - // Don't check statements that shadow `Self` or where `Self` can't be used - }, - _ => walk_item(self, item), - } - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::All(self.cx.tcx.hir()) - } -} diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index ebb3aa28daf3..77b18f805752 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -86,7 +86,7 @@ mod existential { struct Foo; impl Foo { - fn bad(foos: &[Self]) -> impl Iterator { + fn bad(foos: &[Self]) -> impl Iterator { foos.iter() } @@ -101,7 +101,7 @@ mod tuple_structs { impl TS { pub fn ts() -> Self { - Self(0) + TS(0) } } } @@ -163,9 +163,9 @@ mod nesting { } fn method2() { - let _ = Self::B(42); - let _ = Self::C { field: true }; - let _ = Self::A; + let _ = Enum::B(42); + let _ = Self { field: true }; + let _ = Enum::A; } } } @@ -176,11 +176,26 @@ mod issue3410 { struct B; trait Trait { - fn a(v: T); + fn a(v: T) -> Self; } impl Trait> for Vec { - fn a(_: Vec) {} + fn a(_: Vec) -> Self { + unimplemented!() + } + } + + impl Trait> for Vec + where + T: Trait, + { + fn a(v: Vec) -> Self { + // fix false positive, should not trigger here + >::a(v) + .into_iter() + .map(Trait::a) + .collect() + } } } @@ -232,7 +247,7 @@ mod paths_created_by_lowering { const A: usize = 0; const B: usize = 1; - async fn g() -> Self { + async fn g() -> S { Self {} } @@ -251,3 +266,22 @@ mod paths_created_by_lowering { } } } + +// reused from #1997 +mod generics { + struct Foo { + value: T, + } + + impl Foo { + // `Self` is applicable here + fn foo(value: T) -> Self { + Self { value } + } + + // `Cannot` use `Self` as a return type as the generic types are different + fn bar(value: i32) -> Foo { + Foo { value } + } + } +} diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index 8a182192ab34..5f519dfd8e03 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -176,11 +176,26 @@ mod issue3410 { struct B; trait Trait { - fn a(v: T); + fn a(v: T) -> Self; } impl Trait> for Vec { - fn a(_: Vec) {} + fn a(_: Vec) -> Self { + unimplemented!() + } + } + + impl Trait> for Vec + where + T: Trait, + { + fn a(v: Vec) -> Self { + // fix false positive, should not trigger here + >::a(v) + .into_iter() + .map(Trait::a) + .collect() + } } } @@ -251,3 +266,22 @@ mod paths_created_by_lowering { } } } + +// reused from #1997 +mod generics { + struct Foo { + value: T, + } + + impl Foo { + // `Self` is applicable here + fn foo(value: T) -> Foo { + Foo { value } + } + + // `Cannot` use `Self` as a return type as the generic types are different + fn bar(value: i32) -> Foo { + Foo { value } + } + } +} diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index b33928597c14..650f7d9fef7f 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -37,16 +37,10 @@ LL | Foo::new() | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:89:56 + --> $DIR/use_self.rs:34:13 | -LL | fn bad(foos: &[Self]) -> impl Iterator { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:104:13 - | -LL | TS(0) - | ^^ help: use the applicable keyword: `Self` +LL | Self {} + | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition --> $DIR/use_self.rs:112:25 @@ -94,71 +88,71 @@ error: unnecessary structure name repetition LL | Bar { foo: Foo {} } | ^^^ help: use the applicable keyword: `Self` -error: unnecessary structure name repetition - --> $DIR/use_self.rs:166:21 - | -LL | let _ = Enum::B(42); - | ^^^^ help: use the applicable keyword: `Self` - error: unnecessary structure name repetition --> $DIR/use_self.rs:167:21 | LL | let _ = Enum::C { field: true }; - | ^^^^ help: use the applicable keyword: `Self` + | ^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:168:21 - | -LL | let _ = Enum::A; - | ^^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:199:13 + --> $DIR/use_self.rs:214:13 | LL | nested::A::fun_1(); | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:200:13 + --> $DIR/use_self.rs:215:13 | LL | nested::A::A; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:202:13 + --> $DIR/use_self.rs:217:13 | LL | nested::A {}; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:221:13 + --> $DIR/use_self.rs:226:13 | -LL | TestStruct::from_something() - | ^^^^^^^^^^ help: use the applicable keyword: `Self` +LL | Self {} + | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:235:25 + --> $DIR/use_self.rs:236:13 | -LL | async fn g() -> S { - | ^ help: use the applicable keyword: `Self` +LL | TestStruct::from_something() + | ^^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:236:13 + --> $DIR/use_self.rs:251:13 | LL | S {} | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:240:16 + --> $DIR/use_self.rs:255:16 | LL | &p[S::A..S::B] | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:240:22 + --> $DIR/use_self.rs:255:22 | LL | &p[S::A..S::B] | ^ help: use the applicable keyword: `Self` -error: aborting due to 25 previous errors +error: unnecessary structure name repetition + --> $DIR/use_self.rs:278:29 + | +LL | fn foo(value: T) -> Foo { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:279:13 + | +LL | Foo { value } + | ^^^ help: use the applicable keyword: `Self` + +error: aborting due to 24 previous errors diff --git a/tests/ui/use_self_trait.fixed b/tests/ui/use_self_trait.fixed index 1582ae114bf4..53f1ff2f8c47 100644 --- a/tests/ui/use_self_trait.fixed +++ b/tests/ui/use_self_trait.fixed @@ -47,7 +47,7 @@ impl Mul for Bad { impl Clone for Bad { fn clone(&self) -> Self { - Self + Bad } } diff --git a/tests/ui/use_self_trait.stderr b/tests/ui/use_self_trait.stderr index 4f2506cc1192..55af3ff2a93d 100644 --- a/tests/ui/use_self_trait.stderr +++ b/tests/ui/use_self_trait.stderr @@ -84,11 +84,5 @@ error: unnecessary structure name repetition LL | fn mul(self, rhs: Bad) -> Bad { | ^^^ help: use the applicable keyword: `Self` -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:50:9 - | -LL | Bad - | ^^^ help: use the applicable keyword: `Self` - -error: aborting due to 15 previous errors +error: aborting due to 14 previous errors