From 347b01eb1f41154e3e1d6f13c54afa0336d80516 Mon Sep 17 00:00:00 2001 From: Tim Nielens Date: Sun, 26 Apr 2020 02:48:02 +0200 Subject: [PATCH 1/8] rework use_self impl based on ty::Ty comparison --- clippy_lints/src/use_self.rs | 320 ++++++++++++++++++++------------- tests/ui/use_self.fixed | 230 ++++++++++++++++++++++-- tests/ui/use_self.rs | 216 +++++++++++++++++++++- tests/ui/use_self.stderr | 100 ++++------- tests/ui/use_self_trait.fixed | 4 +- tests/ui/use_self_trait.stderr | 14 +- 6 files changed, 668 insertions(+), 216 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 72d1ca7392913..1b5070d1cffbf 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -1,24 +1,24 @@ +use crate::utils; +use crate::utils::snippet_opt; +use crate::utils::span_lint_and_sugg; 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::def::DefKind; +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, ItemKind, Node, 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_semver::RustcVersion; -use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::symbol::kw; +use rustc_middle::ty::Ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{BytePos, Span}; use rustc_typeck::hir_ty_to_ty; -use crate::utils::{differing_macro_contexts, meets_msrv, span_lint_and_sugg}; - declare_clippy_lint! { /// **What it does:** Checks for unnecessary repetition of structure name when a /// replacement with `Self` is applicable. @@ -28,8 +28,7 @@ declare_clippy_lint! { /// feels inconsistent. /// /// **Known problems:** - /// - False positive when using associated types ([#2843](https://github.com/rust-lang/rust-clippy/issues/2843)) - /// - False positives in some situations when using generics ([#3410](https://github.com/rust-lang/rust-clippy/issues/3410)) + /// Unaddressed false negatives related to unresolved internal compiler errors. /// /// **Example:** /// ```rust @@ -54,23 +53,11 @@ declare_clippy_lint! { "unnecessary structure name repetition whereas `Self` is applicable" } -impl_lint_pass!(UseSelf => [USE_SELF]); +declare_lint_pass!(UseSelf => [USE_SELF]); const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element"; -fn span_use_self_lint(cx: &LateContext<'_>, path: &Path<'_>, last_segment: Option<&PathSegment<'_>>) { - let last_segment = last_segment.unwrap_or_else(|| path.segments.last().expect(SEGMENTS_MSG)); - - // Path segments only include actual path, no methods or fields. - let last_path_span = last_segment.ident.span; - - if differing_macro_contexts(path.span, last_path_span) { - return; - } - - // Only take path up to the end of last_path_span. - let span = path.span.with_hi(last_path_span.hi()); - +fn span_lint<'tcx>(cx: &LateContext<'tcx>, span: Span) { span_lint_and_sugg( cx, USE_SELF, @@ -82,107 +69,196 @@ fn span_use_self_lint(cx: &LateContext<'_>, path: &Path<'_>, last_segment: Optio ); } -// FIXME: always use this (more correct) visitor, not just in method signatures. -struct SemanticUseSelfVisitor<'a, 'tcx> { +#[allow(clippy::cast_possible_truncation)] +fn span_lint_until_last_segment<'tcx>(cx: &LateContext<'tcx>, span: Span, segment: &'tcx PathSegment<'tcx>) { + let sp = span.with_hi(segment.ident.span.lo()); + // remove the trailing :: + let span_without_last_segment = match snippet_opt(cx, sp) { + Some(snippet) => match snippet.rfind("::") { + Some(bidx) => sp.with_hi(sp.lo() + BytePos(bidx as u32)), + None => sp, + }, + None => sp, + }; + span_lint(cx, span_without_last_segment); +} + +fn span_lint_on_path_until_last_segment<'tcx>(cx: &LateContext<'tcx>, path: &'tcx Path<'tcx>) { + if path.segments.len() > 1 { + span_lint_until_last_segment(cx, path.span, path.segments.last().unwrap()); + } +} + +fn span_lint_on_qpath_resolved<'tcx>(cx: &LateContext<'tcx>, qpath: &'tcx QPath<'tcx>, until_last_segment: bool) { + if let QPath::Resolved(_, path) = qpath { + if until_last_segment { + span_lint_on_path_until_last_segment(cx, path); + } else { + span_lint(cx, path.span); + } + } +} + +struct ImplVisitor<'a, 'tcx> { cx: &'a LateContext<'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_ty(&mut self, hir_ty: &'tcx hir::Ty<'_>) { - if let TyKind::Path(QPath::Resolved(_, path)) = &hir_ty.kind { + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) + } + + fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'tcx>) { + if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind { match path.res { def::Res::SelfTy(..) => {}, _ => { - if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { - span_use_self_lint(self.cx, path, None); + match self.cx.tcx.hir().find(self.cx.tcx.hir().get_parent_node(hir_ty.hir_id)) { + Some(Node::Expr(Expr { + kind: ExprKind::Path(QPath::TypeRelative(_, _segment)), + .. + })) => { + // The following block correctly identifies applicable lint locations + // but `hir_ty_to_ty` calls cause odd ICEs. + // + // if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { + // // FIXME: this span manipulation should not be necessary + // // @flip1995 found an ast lowering issue in + // // https://github.com/rust-lang/rust/blob/master/src/librustc_ast_lowering/path.rs#L142-L162 + // span_lint_until_last_segment(self.cx, hir_ty.span, segment); + // } + }, + _ => { + if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { + span_lint(self.cx, hir_ty.span) + } + }, } }, } } - walk_ty(self, hir_ty) + walk_ty(self, hir_ty); } - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::None - } -} - -fn check_trait_method_impl_decl<'tcx>( - cx: &LateContext<'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_expr(&mut self, expr: &'tcx Expr<'tcx>) { + fn expr_ty_matches<'tcx>(expr: &'tcx Expr<'tcx>, self_ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool { + let def_id = expr.hir_id.owner; + if cx.tcx.has_typeck_results(def_id) { + cx.tcx.typeck(def_id).expr_ty_opt(expr) == Some(self_ty) + } else { + false + } } - } -} - -const USE_SELF_MSRV: RustcVersion = RustcVersion::new(1, 37, 0); - -pub struct UseSelf { - msrv: Option, -} - -impl UseSelf { - #[must_use] - pub fn new(msrv: Option) -> Self { - Self { msrv } + match expr.kind { + ExprKind::Struct(QPath::Resolved(_, path), ..) => { + if expr_ty_matches(expr, self.self_ty, self.cx) { + match path.res { + def::Res::SelfTy(..) => (), + def::Res::Def(DefKind::Variant, _) => span_lint_on_path_until_last_segment(self.cx, path), + _ => { + span_lint(self.cx, path.span); + }, + } + } + }, + // tuple struct instantiation (`Foo(arg)` or `Enum::Foo(arg)`) + ExprKind::Call(fun, _) => { + if let Expr { + kind: ExprKind::Path(ref qpath), + .. + } = fun + { + if expr_ty_matches(expr, self.self_ty, self.cx) { + let res = utils::qpath_res(self.cx, qpath, fun.hir_id); + + if let def::Res::Def(DefKind::Ctor(ctor_of, _), ..) = res { + match ctor_of { + def::CtorOf::Variant => { + span_lint_on_qpath_resolved(self.cx, qpath, true); + }, + def::CtorOf::Struct => { + span_lint_on_qpath_resolved(self.cx, qpath, false); + }, + } + } + } + } + }, + // unit enum variants (`Enum::A`) + ExprKind::Path(ref qpath) => { + if expr_ty_matches(expr, self.self_ty, self.cx) { + span_lint_on_qpath_resolved(self.cx, qpath, true); + } + }, + _ => (), + } + walk_expr(self, expr); } } impl<'tcx> LateLintPass<'tcx> for UseSelf { - fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { - if !meets_msrv(self.msrv.as_ref(), &USE_SELF_MSRV) { + fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) { + if in_external_macro(cx.sess(), impl_item.span) { return; } - if in_external_macro(cx.sess(), item.span) { - return; - } + let parent_id = cx.tcx.hir().get_parent_item(impl_item.hir_id); + let imp = cx.tcx.hir().expect_item(parent_id); + if_chain! { - if let ItemKind::Impl(impl_) = &item.kind; - if let TyKind::Path(QPath::Resolved(_, ref item_path)) = impl_.self_ty.kind; + if let ItemKind::Impl { self_ty: hir_self_ty, .. } = imp.kind; + if let TyKind::Path(QPath::Resolved(_, ref item_path)) = hir_self_ty.kind; then { let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args; let should_check = parameters.as_ref().map_or( @@ -191,31 +267,23 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { &&!params.args.iter().any(|arg| matches!(arg, GenericArg::Lifetime(_))) ); + // TODO: don't short-circuit upon lifetime parameters if should_check { - let visitor = &mut UseSelfVisitor { - item_path, - cx, - }; - 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 impl_.items { - 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 impl_.items { - let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id); - visitor.visit_impl_item(impl_item); + let self_ty = hir_ty_to_ty(cx.tcx, hir_self_ty); + let visitor = &mut ImplVisitor { cx, self_ty }; + + let tcx = cx.tcx; + let impl_def_id = tcx.hir().local_def_id(imp.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) = &impl_item.kind; + then { + visitor.check_trait_method_impl_decl(impl_item, impl_decl, impl_trait_ref); + let body = tcx.hir().body(*impl_body_id); + visitor.visit_body(body); + } else { + walk_impl_item(visitor, impl_item) } } } diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index bb2012441d90c..916484eef931f 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -15,13 +15,14 @@ mod use_self { Self {} } fn test() -> Self { - Self::new() + Foo::new() } } impl Default for Foo { fn default() -> Self { - Self::new() + // FIXME: applicable here + Foo::new() } } } @@ -87,7 +88,11 @@ mod existential { struct Foo; impl Foo { - fn bad(foos: &[Self]) -> impl Iterator { + // FIXME: + // TyKind::Def (used for `impl Trait` types) does not include type parameters yet. + // See documentation in rustc_hir::hir::TyKind. + // The hir tree walk stops at `impl Iterator` level and does not inspect &Foo. + fn bad(foos: &[Self]) -> impl Iterator { foos.iter() } @@ -177,11 +182,22 @@ 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 { + >::a(v).into_iter().map(Trait::a).collect() + } } } @@ -197,8 +213,8 @@ mod rustfix { fn fun_1() {} fn fun_2() { - Self::fun_1(); - Self::A; + nested::A::fun_1(); + nested::A::A; Self {}; } @@ -219,7 +235,8 @@ mod issue3567 { impl Test for TestStruct { fn test() -> TestStruct { - Self::from_something() + // FIXME: applicable here + TestStruct::from_something() } } } @@ -233,12 +250,14 @@ mod paths_created_by_lowering { const A: usize = 0; const B: usize = 1; - async fn g() -> Self { + // FIXME: applicable here + async fn g() -> S { Self {} } fn f<'a>(&self, p: &'a [u8]) -> &'a [u8] { - &p[Self::A..Self::B] + // FIXME: applicable here twice + &p[S::A..S::B] } } @@ -252,3 +271,194 @@ 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 } + } + } +} + +mod issue4140 { + pub struct Error { + _from: From, + _too: To, + } + + pub trait From { + type From; + type To; + + fn from(value: T) -> Self; + } + + pub trait TryFrom + where + Self: Sized, + { + type From; + type To; + + fn try_from(value: T) -> Result>; + } + + impl TryFrom for T + where + T: From, + { + type From = Self; + type To = Self; + + fn try_from(value: F) -> Result> { + Ok(From::from(value)) + } + } + + impl From for i64 { + type From = bool; + type To = Self; + + fn from(value: bool) -> Self { + if value { + 100 + } else { + 0 + } + } + } +} + +mod issue2843 { + trait Foo { + type Bar; + } + + impl Foo for usize { + type Bar = u8; + } + + impl Foo for Option { + type Bar = Option; + } +} + +mod issue3859 { + pub struct Foo; + pub struct Bar([usize; 3]); + + impl Foo { + pub const BAR: usize = 3; + + pub fn foo() { + const _X: usize = Foo::BAR; + // const _Y: usize = Self::BAR; + } + } +} + +mod issue4305 { + trait Foo: 'static {} + + struct Bar; + + impl Foo for Bar {} + + impl From for Box { + fn from(t: T) -> Self { + // FIXME: applicable here + Box::new(t) + } + } +} + +mod lint_at_item_level { + struct Foo {} + + #[allow(clippy::use_self)] + impl Foo { + fn new() -> Foo { + Foo {} + } + } + + #[allow(clippy::use_self)] + impl Default for Foo { + fn default() -> Foo { + Foo::new() + } + } +} + +mod lint_at_impl_item_level { + struct Foo {} + + impl Foo { + #[allow(clippy::use_self)] + fn new() -> Foo { + Foo {} + } + } + + impl Default for Foo { + #[allow(clippy::use_self)] + fn default() -> Foo { + Foo::new() + } + } +} + +mod issue4734 { + #[repr(C, packed)] + pub struct X { + pub x: u32, + } + + impl From for u32 { + fn from(c: X) -> Self { + unsafe { core::mem::transmute(c) } + } + } +} + +mod nested_paths { + use std::convert::Into; + mod submod { + pub struct B {} + pub struct C {} + + impl Into for B { + fn into(self) -> C { + C {} + } + } + } + + struct A { + t: T, + } + + impl A { + fn new>(v: V) -> Self { + Self { t: Into::into(v) } + } + } + + impl A { + fn test() -> Self { + // FIXME: applicable here + A::new::(submod::B {}) + } + } +} diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index ddfd2beba3107..347f5e9655562 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -21,6 +21,7 @@ mod use_self { impl Default for Foo { fn default() -> Foo { + // FIXME: applicable here Foo::new() } } @@ -87,7 +88,11 @@ mod existential { struct Foo; impl Foo { - fn bad(foos: &[Self]) -> impl Iterator { + // FIXME: + // TyKind::Def (used for `impl Trait` types) does not include type parameters yet. + // See documentation in rustc_hir::hir::TyKind. + // The hir tree walk stops at `impl Iterator` level and does not inspect &Foo. + fn bad(foos: &[Foo]) -> impl Iterator { foos.iter() } @@ -177,11 +182,22 @@ 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 { + >::a(v).into_iter().map(Trait::a).collect() + } } } @@ -219,6 +235,7 @@ mod issue3567 { impl Test for TestStruct { fn test() -> TestStruct { + // FIXME: applicable here TestStruct::from_something() } } @@ -233,11 +250,13 @@ mod paths_created_by_lowering { const A: usize = 0; const B: usize = 1; + // FIXME: applicable here async fn g() -> S { S {} } fn f<'a>(&self, p: &'a [u8]) -> &'a [u8] { + // FIXME: applicable here twice &p[S::A..S::B] } } @@ -252,3 +271,194 @@ 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 } + } + } +} + +mod issue4140 { + pub struct Error { + _from: From, + _too: To, + } + + pub trait From { + type From; + type To; + + fn from(value: T) -> Self; + } + + pub trait TryFrom + where + Self: Sized, + { + type From; + type To; + + fn try_from(value: T) -> Result>; + } + + impl TryFrom for T + where + T: From, + { + type From = T::From; + type To = T::To; + + fn try_from(value: F) -> Result> { + Ok(From::from(value)) + } + } + + impl From for i64 { + type From = bool; + type To = Self; + + fn from(value: bool) -> Self { + if value { + 100 + } else { + 0 + } + } + } +} + +mod issue2843 { + trait Foo { + type Bar; + } + + impl Foo for usize { + type Bar = u8; + } + + impl Foo for Option { + type Bar = Option; + } +} + +mod issue3859 { + pub struct Foo; + pub struct Bar([usize; 3]); + + impl Foo { + pub const BAR: usize = 3; + + pub fn foo() { + const _X: usize = Foo::BAR; + // const _Y: usize = Self::BAR; + } + } +} + +mod issue4305 { + trait Foo: 'static {} + + struct Bar; + + impl Foo for Bar {} + + impl From for Box { + fn from(t: T) -> Self { + // FIXME: applicable here + Box::new(t) + } + } +} + +mod lint_at_item_level { + struct Foo {} + + #[allow(clippy::use_self)] + impl Foo { + fn new() -> Foo { + Foo {} + } + } + + #[allow(clippy::use_self)] + impl Default for Foo { + fn default() -> Foo { + Foo::new() + } + } +} + +mod lint_at_impl_item_level { + struct Foo {} + + impl Foo { + #[allow(clippy::use_self)] + fn new() -> Foo { + Foo {} + } + } + + impl Default for Foo { + #[allow(clippy::use_self)] + fn default() -> Foo { + Foo::new() + } + } +} + +mod issue4734 { + #[repr(C, packed)] + pub struct X { + pub x: u32, + } + + impl From for u32 { + fn from(c: X) -> Self { + unsafe { core::mem::transmute(c) } + } + } +} + +mod nested_paths { + use std::convert::Into; + mod submod { + pub struct B {} + pub struct C {} + + impl Into for B { + fn into(self) -> C { + C {} + } + } + } + + struct A { + t: T, + } + + impl A { + fn new>(v: V) -> Self { + Self { t: Into::into(v) } + } + } + + impl A { + fn test() -> Self { + // FIXME: applicable here + A::new::(submod::B {}) + } + } +} diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index 80e1bfc75e80a..a88dd04f13d3f 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -18,12 +18,6 @@ error: unnecessary structure name repetition LL | fn test() -> Foo { | ^^^ help: use the applicable keyword: `Self` -error: unnecessary structure name repetition - --> $DIR/use_self.rs:18:13 - | -LL | Foo::new() - | ^^^ help: use the applicable keyword: `Self` - error: unnecessary structure name repetition --> $DIR/use_self.rs:23:25 | @@ -31,25 +25,19 @@ LL | fn default() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:24:13 - | -LL | Foo::new() - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:90:56 + --> $DIR/use_self.rs:94:24 | -LL | fn bad(foos: &[Self]) -> impl Iterator { - | ^^^ help: use the applicable keyword: `Self` +LL | fn bad(foos: &[Foo]) -> impl Iterator { + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:105:13 + --> $DIR/use_self.rs:109:13 | LL | TS(0) | ^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:113:25 + --> $DIR/use_self.rs:117:25 | LL | fn new() -> Foo { | ^^^ help: use the applicable keyword: `Self` @@ -60,7 +48,7 @@ LL | use_self_expand!(); // Should lint in local macros = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary structure name repetition - --> $DIR/use_self.rs:114:17 + --> $DIR/use_self.rs:118:17 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` @@ -71,94 +59,82 @@ LL | use_self_expand!(); // Should lint in local macros = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary structure name repetition - --> $DIR/use_self.rs:149:21 + --> $DIR/use_self.rs:141:29 | -LL | fn baz() -> Foo { - | ^^^ help: use the applicable keyword: `Self` +LL | fn bar() -> Bar { + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:150:13 + --> $DIR/use_self.rs:142:21 | -LL | Foo {} - | ^^^ help: use the applicable keyword: `Self` +LL | Bar { foo: Foo {} } + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:137:29 + --> $DIR/use_self.rs:153:21 | -LL | fn bar() -> Bar { - | ^^^ help: use the applicable keyword: `Self` +LL | fn baz() -> Foo { + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:138:21 + --> $DIR/use_self.rs:154:13 | -LL | Bar { foo: Foo {} } - | ^^^ help: use the applicable keyword: `Self` +LL | Foo {} + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:167:21 + --> $DIR/use_self.rs:171:21 | LL | let _ = Enum::B(42); | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:168:21 + --> $DIR/use_self.rs:172:21 | LL | let _ = Enum::C { field: true }; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:169:21 + --> $DIR/use_self.rs:173:21 | LL | let _ = Enum::A; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:200:13 - | -LL | nested::A::fun_1(); - | ^^^^^^^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:201:13 - | -LL | nested::A::A; - | ^^^^^^^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:203:13 + --> $DIR/use_self.rs:218:13 | LL | nested::A {}; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:222:13 + --> $DIR/use_self.rs:254:13 | -LL | TestStruct::from_something() - | ^^^^^^^^^^ help: use the applicable keyword: `Self` +LL | S {} + | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:236:25 + --> $DIR/use_self.rs:282:29 | -LL | async fn g() -> S { - | ^ help: use the applicable keyword: `Self` +LL | fn foo(value: T) -> Foo { + | ^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:237:13 + --> $DIR/use_self.rs:283:13 | -LL | S {} - | ^ help: use the applicable keyword: `Self` +LL | Foo { value } + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:241:16 + --> $DIR/use_self.rs:320:21 | -LL | &p[S::A..S::B] - | ^ help: use the applicable keyword: `Self` +LL | type From = T::From; + | ^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:241:22 + --> $DIR/use_self.rs:321:19 | -LL | &p[S::A..S::B] - | ^ help: use the applicable keyword: `Self` +LL | type To = T::To; + | ^^^^^ help: use the applicable keyword: `Self` -error: aborting due to 25 previous errors +error: aborting due to 21 previous errors diff --git a/tests/ui/use_self_trait.fixed b/tests/ui/use_self_trait.fixed index 1582ae114bf4c..d425f953a9c3c 100644 --- a/tests/ui/use_self_trait.fixed +++ b/tests/ui/use_self_trait.fixed @@ -33,7 +33,7 @@ impl SelfTrait for Bad { fn nested(_p1: Box, _p2: (&u8, &Self)) {} fn vals(_: Self) -> Self { - Self::default() + Bad::default() } } @@ -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 4f2506cc1192f..fa528cc5b7dd0 100644 --- a/tests/ui/use_self_trait.stderr +++ b/tests/ui/use_self_trait.stderr @@ -60,12 +60,6 @@ error: unnecessary structure name repetition LL | fn vals(_: Bad) -> Bad { | ^^^ help: use the applicable keyword: `Self` -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:36:9 - | -LL | Bad::default() - | ^^^ help: use the applicable keyword: `Self` - error: unnecessary structure name repetition --> $DIR/use_self_trait.rs:41:19 | @@ -84,11 +78,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 13 previous errors From fc334fb8f4cc7e6513578d88f52b2899f624a1de Mon Sep 17 00:00:00 2001 From: Tim Nielens Date: Sat, 10 Oct 2020 00:26:12 +0200 Subject: [PATCH 2/8] use_self - fix issue with `hir_ty_to_ty` --- clippy_lints/src/use_self.rs | 97 ++++++++++++++++++++-------------- tests/ui/use_self.fixed | 6 ++- tests/ui/use_self.rs | 4 ++ tests/ui/use_self.stderr | 84 ++++++++++++++--------------- tests/ui/use_self_trait.fixed | 12 ++--- tests/ui/use_self_trait.stderr | 76 +------------------------- 6 files changed, 112 insertions(+), 167 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 1b5070d1cffbf..023fce947b3aa 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -57,7 +57,7 @@ declare_lint_pass!(UseSelf => [USE_SELF]); const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element"; -fn span_lint<'tcx>(cx: &LateContext<'tcx>, span: Span) { +fn span_lint(cx: &LateContext<'_>, span: Span) { span_lint_and_sugg( cx, USE_SELF, @@ -99,12 +99,12 @@ fn span_lint_on_qpath_resolved<'tcx>(cx: &LateContext<'tcx>, qpath: &'tcx QPath< } } -struct ImplVisitor<'a, 'tcx> { +struct BodyVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, self_ty: Ty<'tcx>, } -impl<'a, 'tcx> ImplVisitor<'a, 'tcx> { +impl<'a, 'tcx> BodyVisitor<'a, 'tcx> { fn check_trait_method_impl_decl( &mut self, impl_item: &ImplItem<'tcx>, @@ -151,46 +151,13 @@ impl<'a, 'tcx> ImplVisitor<'a, 'tcx> { } } -impl<'a, 'tcx> Visitor<'tcx> for ImplVisitor<'a, 'tcx> { +impl<'a, 'tcx> Visitor<'tcx> for BodyVisitor<'a, 'tcx> { type Map = Map<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) } - fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'tcx>) { - if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind { - match path.res { - def::Res::SelfTy(..) => {}, - _ => { - match self.cx.tcx.hir().find(self.cx.tcx.hir().get_parent_node(hir_ty.hir_id)) { - Some(Node::Expr(Expr { - kind: ExprKind::Path(QPath::TypeRelative(_, _segment)), - .. - })) => { - // The following block correctly identifies applicable lint locations - // but `hir_ty_to_ty` calls cause odd ICEs. - // - // if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { - // // FIXME: this span manipulation should not be necessary - // // @flip1995 found an ast lowering issue in - // // https://github.com/rust-lang/rust/blob/master/src/librustc_ast_lowering/path.rs#L142-L162 - // span_lint_until_last_segment(self.cx, hir_ty.span, segment); - // } - }, - _ => { - if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { - span_lint(self.cx, hir_ty.span) - } - }, - } - }, - } - } - - walk_ty(self, hir_ty); - } - fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { fn expr_ty_matches<'tcx>(expr: &'tcx Expr<'tcx>, self_ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool { let def_id = expr.hir_id.owner; @@ -247,6 +214,52 @@ impl<'a, 'tcx> Visitor<'tcx> for ImplVisitor<'a, 'tcx> { } } +struct FnSigVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + self_ty: Ty<'tcx>, +} + +impl<'a, 'tcx> Visitor<'tcx> for FnSigVisitor<'a, 'tcx> { + type Map = Map<'tcx>; + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } + + fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'tcx>) { + if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind { + match path.res { + def::Res::SelfTy(..) => {}, + _ => { + match self.cx.tcx.hir().find(self.cx.tcx.hir().get_parent_node(hir_ty.hir_id)) { + Some(Node::Expr(Expr { + kind: ExprKind::Path(QPath::TypeRelative(_, segment)), + .. + })) => { + // The following block correctly identifies applicable lint locations + // but `hir_ty_to_ty` calls cause odd ICEs. + // + if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { + // fixme: this span manipulation should not be necessary + // @flip1995 found an ast lowering issue in + // https://github.com/rust-lang/rust/blob/master/src/librustc_ast_lowering/path.rs#l142-l162 + span_lint_until_last_segment(self.cx, hir_ty.span, segment); + } + }, + _ => { + if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { + span_lint(self.cx, hir_ty.span) + } + }, + } + }, + } + } + + walk_ty(self, hir_ty); + } +} + impl<'tcx> LateLintPass<'tcx> for UseSelf { fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) { if in_external_macro(cx.sess(), impl_item.span) { @@ -270,7 +283,8 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { // TODO: don't short-circuit upon lifetime parameters if should_check { let self_ty = hir_ty_to_ty(cx.tcx, hir_self_ty); - let visitor = &mut ImplVisitor { cx, self_ty }; + let body_visitor = &mut BodyVisitor { cx, self_ty }; + let fn_sig_visitor = &mut FnSigVisitor { cx, self_ty }; let tcx = cx.tcx; let impl_def_id = tcx.hir().local_def_id(imp.hir_id); @@ -279,11 +293,12 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { if let Some(impl_trait_ref) = impl_trait_ref; if let ImplItemKind::Fn(FnSig { decl: impl_decl, .. }, impl_body_id) = &impl_item.kind; then { - visitor.check_trait_method_impl_decl(impl_item, impl_decl, impl_trait_ref); + body_visitor.check_trait_method_impl_decl(impl_item, impl_decl, impl_trait_ref); let body = tcx.hir().body(*impl_body_id); - visitor.visit_body(body); + body_visitor.visit_body(body); } else { - walk_impl_item(visitor, impl_item) + walk_impl_item(body_visitor, impl_item); + walk_impl_item(fn_sig_visitor, impl_item); } } } diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index 916484eef931f..d59750cbfd8d9 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -15,12 +15,14 @@ mod use_self { Self {} } fn test() -> Self { + // FIXME: applicable here Foo::new() } } impl Default for Foo { - fn default() -> Self { + // FIXME: applicable here + fn default() -> Foo { // FIXME: applicable here Foo::new() } @@ -213,7 +215,9 @@ mod rustfix { fn fun_1() {} fn fun_2() { + // FIXME: applicable here nested::A::fun_1(); + // FIXME: applicable here nested::A::A; Self {}; diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index 347f5e9655562..85606049774e1 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -15,11 +15,13 @@ mod use_self { Foo {} } fn test() -> Foo { + // FIXME: applicable here Foo::new() } } impl Default for Foo { + // FIXME: applicable here fn default() -> Foo { // FIXME: applicable here Foo::new() @@ -213,7 +215,9 @@ mod rustfix { fn fun_1() {} fn fun_2() { + // FIXME: applicable here nested::A::fun_1(); + // FIXME: applicable here nested::A::A; nested::A {}; diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index a88dd04f13d3f..4d213316cf53c 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -1,16 +1,16 @@ error: unnecessary structure name repetition - --> $DIR/use_self.rs:14:21 + --> $DIR/use_self.rs:15:13 | -LL | fn new() -> Foo { - | ^^^ help: use the applicable keyword: `Self` +LL | Foo {} + | ^^^ help: use the applicable keyword: `Self` | = note: `-D clippy::use-self` implied by `-D warnings` error: unnecessary structure name repetition - --> $DIR/use_self.rs:15:13 + --> $DIR/use_self.rs:14:21 | -LL | Foo {} - | ^^^ help: use the applicable keyword: `Self` +LL | fn new() -> Foo { + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition --> $DIR/use_self.rs:17:22 @@ -19,28 +19,22 @@ LL | fn test() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:23:25 - | -LL | fn default() -> Foo { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:94:24 + --> $DIR/use_self.rs:96:24 | LL | fn bad(foos: &[Foo]) -> impl Iterator { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:109:13 + --> $DIR/use_self.rs:111:13 | LL | TS(0) | ^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:117:25 + --> $DIR/use_self.rs:120:17 | -LL | fn new() -> Foo { - | ^^^ help: use the applicable keyword: `Self` +LL | Foo {} + | ^^^ help: use the applicable keyword: `Self` ... LL | use_self_expand!(); // Should lint in local macros | ------------------- in this macro invocation @@ -48,10 +42,10 @@ LL | use_self_expand!(); // Should lint in local macros = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary structure name repetition - --> $DIR/use_self.rs:118:17 + --> $DIR/use_self.rs:119:25 | -LL | Foo {} - | ^^^ help: use the applicable keyword: `Self` +LL | fn new() -> Foo { + | ^^^ help: use the applicable keyword: `Self` ... LL | use_self_expand!(); // Should lint in local macros | ------------------- in this macro invocation @@ -59,82 +53,82 @@ LL | use_self_expand!(); // Should lint in local macros = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary structure name repetition - --> $DIR/use_self.rs:141:29 - | -LL | fn bar() -> Bar { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:142:21 + --> $DIR/use_self.rs:144:21 | LL | Bar { foo: Foo {} } | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:153:21 + --> $DIR/use_self.rs:143:29 | -LL | fn baz() -> Foo { - | ^^^ help: use the applicable keyword: `Self` +LL | fn bar() -> Bar { + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:154:13 + --> $DIR/use_self.rs:156:13 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:171:21 + --> $DIR/use_self.rs:155:21 + | +LL | fn baz() -> Foo { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:173:21 | LL | let _ = Enum::B(42); | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:172:21 + --> $DIR/use_self.rs:174:21 | LL | let _ = Enum::C { field: true }; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:173:21 + --> $DIR/use_self.rs:175:21 | LL | let _ = Enum::A; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:218:13 + --> $DIR/use_self.rs:222:13 | LL | nested::A {}; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:254:13 + --> $DIR/use_self.rs:258:13 | LL | S {} | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:282:29 + --> $DIR/use_self.rs:287:13 | -LL | fn foo(value: T) -> Foo { - | ^^^^^^ help: use the applicable keyword: `Self` +LL | Foo { value } + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:283:13 + --> $DIR/use_self.rs:286:29 | -LL | Foo { value } - | ^^^ help: use the applicable keyword: `Self` +LL | fn foo(value: T) -> Foo { + | ^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:320:21 + --> $DIR/use_self.rs:324:21 | LL | type From = T::From; | ^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:321:19 + --> $DIR/use_self.rs:325:19 | LL | type To = T::To; | ^^^^^ help: use the applicable keyword: `Self` -error: aborting due to 21 previous errors +error: aborting due to 20 previous errors diff --git a/tests/ui/use_self_trait.fixed b/tests/ui/use_self_trait.fixed index d425f953a9c3c..680a0623839e0 100644 --- a/tests/ui/use_self_trait.fixed +++ b/tests/ui/use_self_trait.fixed @@ -18,21 +18,21 @@ trait SelfTrait { struct Bad; impl SelfTrait for Bad { - fn refs(p1: &Self) -> &Self { + fn refs(p1: &Bad) -> &Bad { p1 } - fn ref_refs<'a>(p1: &'a &'a Self) -> &'a &'a Self { + fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { p1 } - fn mut_refs(p1: &mut Self) -> &mut Self { + fn mut_refs(p1: &mut Bad) -> &mut Bad { p1 } - fn nested(_p1: Box, _p2: (&u8, &Self)) {} + fn nested(_p1: Box, _p2: (&u8, &Bad)) {} - fn vals(_: Self) -> Self { + fn vals(_: Bad) -> Bad { Bad::default() } } @@ -40,7 +40,7 @@ impl SelfTrait for Bad { impl Mul for Bad { type Output = Self; - fn mul(self, rhs: Self) -> Self { + fn mul(self, rhs: Bad) -> Bad { rhs } } diff --git a/tests/ui/use_self_trait.stderr b/tests/ui/use_self_trait.stderr index fa528cc5b7dd0..5409ccedf8592 100644 --- a/tests/ui/use_self_trait.stderr +++ b/tests/ui/use_self_trait.stderr @@ -1,82 +1,10 @@ -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:21:18 - | -LL | fn refs(p1: &Bad) -> &Bad { - | ^^^ help: use the applicable keyword: `Self` - | - = note: `-D clippy::use-self` implied by `-D warnings` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:21:27 - | -LL | fn refs(p1: &Bad) -> &Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:25:33 - | -LL | fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:25:49 - | -LL | fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:29:26 - | -LL | fn mut_refs(p1: &mut Bad) -> &mut Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:29:39 - | -LL | fn mut_refs(p1: &mut Bad) -> &mut Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:33:24 - | -LL | fn nested(_p1: Box, _p2: (&u8, &Bad)) {} - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:33:42 - | -LL | fn nested(_p1: Box, _p2: (&u8, &Bad)) {} - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:35:16 - | -LL | fn vals(_: Bad) -> Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:35:24 - | -LL | fn vals(_: Bad) -> Bad { - | ^^^ help: use the applicable keyword: `Self` - error: unnecessary structure name repetition --> $DIR/use_self_trait.rs:41:19 | LL | type Output = Bad; | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:43:23 | -LL | fn mul(self, rhs: Bad) -> Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self_trait.rs:43:31 - | -LL | fn mul(self, rhs: Bad) -> Bad { - | ^^^ help: use the applicable keyword: `Self` + = note: `-D clippy::use-self` implied by `-D warnings` -error: aborting due to 13 previous errors +error: aborting due to previous error From ae2dd671f5003e6722ca4d18ef447141db237213 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Fri, 16 Oct 2020 13:25:38 +0200 Subject: [PATCH 3/8] Rewrite use_self lint one more time This rewrite gets rid of complicated visitors, by using the lint infrastructure as much as possible --- clippy_lints/src/use_self.rs | 604 ++++++++++++++++++++--------------- 1 file changed, 344 insertions(+), 260 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 023fce947b3aa..fbd996ad0e94d 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -1,21 +1,22 @@ -use crate::utils; -use crate::utils::snippet_opt; -use crate::utils::span_lint_and_sugg; +use crate::utils::{meets_msrv, qpath_res, snippet_opt, span_lint_and_sugg}; use if_chain::if_chain; + use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::def::DefKind; -use rustc_hir::intravisit::{walk_expr, walk_impl_item, walk_ty, NestedVisitorMap, Visitor}; use rustc_hir::{ - def, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, ImplItem, ImplItemKind, ItemKind, Node, Path, PathSegment, + def, + def_id::LocalDefId, + intravisit::{walk_ty, NestedVisitorMap, Visitor}, + Expr, ExprKind, FnRetTy, FnSig, GenericArg, HirId, Impl, ImplItemKind, Item, ItemKind, Node, 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::Ty; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_middle::ty::{AssocKind, Ty}; +use rustc_semver::RustcVersion; +use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{BytePos, Span}; use rustc_typeck::hir_ty_to_ty; @@ -28,9 +29,11 @@ declare_clippy_lint! { /// feels inconsistent. /// /// **Known problems:** - /// Unaddressed false negatives related to unresolved internal compiler errors. + /// - Unaddressed false negative in fn bodies of trait implementations + /// - False positive with assotiated types in traits (#4140) /// /// **Example:** + /// /// ```rust /// struct Foo {} /// impl Foo { @@ -53,113 +56,216 @@ declare_clippy_lint! { "unnecessary structure name repetition whereas `Self` is applicable" } -declare_lint_pass!(UseSelf => [USE_SELF]); +#[derive(Default)] +pub struct UseSelf { + msrv: Option, + stack: Vec, + types_to_skip: Vec, + types_to_lint: Vec, +} -const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element"; +const USE_SELF_MSRV: RustcVersion = RustcVersion::new(1, 37, 0); -fn span_lint(cx: &LateContext<'_>, span: Span) { - span_lint_and_sugg( - cx, - USE_SELF, - span, - "unnecessary structure name repetition", - "use the applicable keyword", - "Self".to_owned(), - Applicability::MachineApplicable, - ); +impl UseSelf { + #[must_use] + pub fn new(msrv: Option) -> Self { + Self { + msrv, + ..Self::default() + } + } } -#[allow(clippy::cast_possible_truncation)] -fn span_lint_until_last_segment<'tcx>(cx: &LateContext<'tcx>, span: Span, segment: &'tcx PathSegment<'tcx>) { - let sp = span.with_hi(segment.ident.span.lo()); - // remove the trailing :: - let span_without_last_segment = match snippet_opt(cx, sp) { - Some(snippet) => match snippet.rfind("::") { - Some(bidx) => sp.with_hi(sp.lo() + BytePos(bidx as u32)), - None => sp, - }, - None => sp, - }; - span_lint(cx, span_without_last_segment); +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum StackItem { + Check { + hir_id: HirId, + impl_trait_ref_def_id: Option, + }, + NoCheck, } -fn span_lint_on_path_until_last_segment<'tcx>(cx: &LateContext<'tcx>, path: &'tcx Path<'tcx>) { - if path.segments.len() > 1 { - span_lint_until_last_segment(cx, path.span, path.segments.last().unwrap()); +impl_lint_pass!(UseSelf => [USE_SELF]); + +const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element"; + +impl<'tcx> LateLintPass<'tcx> for UseSelf { + fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + // We push the self types of `impl`s on a stack here. Only the top type on the stack is + // relevant for linting, since this is the self type of the `impl` we're currently in. To + // avoid linting on nested items, we push `StackItem::NoCheck` on the stack to signal, that + // we're in an `impl` or nested item, that we don't want to lint + // + // NB: If you push something on the stack in this method, remember to also pop it in the + // `check_item_post` method. + match &item.kind { + ItemKind::Impl(Impl { + self_ty: hir_self_ty, + of_trait, + .. + }) => { + let should_check = if let TyKind::Path(QPath::Resolved(_, ref item_path)) = hir_self_ty.kind { + let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args; + parameters.as_ref().map_or(true, |params| { + !params.parenthesized && !params.args.iter().any(|arg| matches!(arg, GenericArg::Lifetime(_))) + }) + } else { + false + }; + let impl_trait_ref_def_id = of_trait.as_ref().map(|_| cx.tcx.hir().local_def_id(item.hir_id)); + if should_check { + self.stack.push(StackItem::Check { + hir_id: hir_self_ty.hir_id, + impl_trait_ref_def_id, + }); + } else { + self.stack.push(StackItem::NoCheck); + } + }, + ItemKind::Static(..) + | ItemKind::Const(..) + | ItemKind::Fn(..) + | ItemKind::Enum(..) + | ItemKind::Struct(..) + | ItemKind::Union(..) + | ItemKind::Trait(..) => { + self.stack.push(StackItem::NoCheck); + }, + _ => (), + } } -} -fn span_lint_on_qpath_resolved<'tcx>(cx: &LateContext<'tcx>, qpath: &'tcx QPath<'tcx>, until_last_segment: bool) { - if let QPath::Resolved(_, path) = qpath { - if until_last_segment { - span_lint_on_path_until_last_segment(cx, path); - } else { - span_lint(cx, path.span); + fn check_item_post(&mut self, _: &LateContext<'_>, item: &Item<'_>) { + use ItemKind::{Const, Enum, Fn, Impl, Static, Struct, Trait, Union}; + match item.kind { + Impl { .. } | Static(..) | Const(..) | Fn(..) | Enum(..) | Struct(..) | Union(..) | Trait(..) => { + self.stack.pop(); + }, + _ => (), } } -} -struct BodyVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - self_ty: Ty<'tcx>, -} + fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &hir::ImplItem<'_>) { + // We want to skip types in trait `impl`s that aren't declared as `Self` in the trait + // declaration. The collection of those types is all this method implementation does. + if_chain! { + if let ImplItemKind::Fn(FnSig { decl, .. }, ..) = impl_item.kind; + if let Some(StackItem::Check { impl_trait_ref_def_id: Some(def_id), .. }) = self.stack.last().copied(); + if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(def_id); + then { + // `self_ty` is the semantic self type of `impl for `. This cannot be + // `Self`. + let self_ty = impl_trait_ref.self_ty(); + + // `trait_method_sig` is the signature of the function, how it is declared in the + // trait, not in the impl of the trait. + let trait_method = cx + .tcx + .associated_items(impl_trait_ref.def_id) + .find_by_name_and_kind(cx.tcx, impl_item.ident, 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); + + // `impl_inputs_outputs` is an iterator over the types (`hir::Ty`) declared in the + // implementation of the trait. + let output_hir_ty = if let FnRetTy::Return(ty) = &decl.output { + Some(&**ty) + } else { + None + }; + let impl_inputs_outputs = decl.inputs.iter().chain(output_hir_ty); + + // `impl_hir_ty` (of type `hir::Ty`) represents the type written in the signature. + // + // `trait_sem_ty` (of type `ty::Ty`) is the semantic type for the signature in the + // trait declaration. This is used to check if `Self` was used in the trait + // declaration. + // + // If `any`where in the `trait_sem_ty` the `self_ty` was used verbatim (as opposed + // to `Self`), we want to skip linting that type and all subtypes of it. 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. + for (impl_hir_ty, trait_sem_ty) in impl_inputs_outputs.zip(trait_method_sig.inputs_and_output) { + if trait_sem_ty.walk().any(|inner| inner == self_ty.into()) { + let mut visitor = SkipTyCollector::default(); + visitor.visit_ty(&impl_hir_ty); + self.types_to_skip.extend(visitor.types_to_skip); + } + } + } + } + } -impl<'a, 'tcx> BodyVisitor<'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 - }; + fn check_impl_item_post(&mut self, _: &LateContext<'_>, _: &hir::ImplItem<'_>) { + self.types_to_skip.clear(); + } - // `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); - } + fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx hir::Body<'_>) { + // `hir_ty_to_ty` cannot be called in `Body`s or it will panic (sometimes). But in bodies + // we can use `cx.typeck_results.node_type(..)` to get the `ty::Ty` from a `hir::Ty`. + // However the `node_type()` method can *only* be called in bodies. + // + // This method implementation determines which types should get linted in a `Body` and + // which shouldn't, with a visitor. We could directly lint in the visitor, but then we + // could only allow this lint on item scope. And we would have to check if those types are + // already dealt with in `check_ty` anyway. + if let Some(StackItem::Check { hir_id, .. }) = self.stack.last() { + let self_ty = ty_from_hir_id(cx, *hir_id); + + let mut visitor = LintTyCollector { + cx, + self_ty, + types_to_lint: vec![], + types_to_skip: vec![], + }; + visitor.visit_expr(&body.value); + self.types_to_lint.extend(visitor.types_to_lint); + self.types_to_skip.extend(visitor.types_to_skip); } } -} -impl<'a, 'tcx> Visitor<'tcx> for BodyVisitor<'a, 'tcx> { - type Map = Map<'tcx>; + fn check_body_post(&mut self, _: &LateContext<'_>, _: &hir::Body<'_>) { + self.types_to_lint.clear(); + } - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) + fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>) { + if in_external_macro(cx.sess(), hir_ty.span) + | in_impl(cx, hir_ty) + | self.types_to_skip.contains(&hir_ty.hir_id) + | !meets_msrv(self.msrv.as_ref(), &USE_SELF_MSRV) + { + return; + } + + let lint_dependend_on_expr_kind = || { + // FIXME: this span manipulation should not be necessary + // @flip1995 found an ast lowering issue in + // https://github.com/rust-lang/rust/blob/master/src/librustc_ast_lowering/path.rs#l142-l162 + match cx.tcx.hir().find(cx.tcx.hir().get_parent_node(hir_ty.hir_id)) { + Some(Node::Expr(Expr { + kind: ExprKind::Path(QPath::TypeRelative(_, segment)), + .. + })) => span_lint_until_last_segment(cx, hir_ty.span, segment), + _ => span_lint(cx, hir_ty.span), + } + }; + + if self.types_to_lint.contains(&hir_ty.hir_id) { + lint_dependend_on_expr_kind(); + } else if let Some(StackItem::Check { hir_id, .. }) = self.stack.last() { + let self_ty = ty_from_hir_id(cx, *hir_id); + + if should_lint_ty(hir_ty, hir_ty_to_ty(cx.tcx, hir_ty), self_ty) { + lint_dependend_on_expr_kind(); + } + } } - fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - fn expr_ty_matches<'tcx>(expr: &'tcx Expr<'tcx>, self_ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + fn expr_ty_matches(cx: &LateContext<'_>, expr: &Expr<'_>, self_ty: Ty<'_>) -> bool { let def_id = expr.hir_id.owner; if cx.tcx.has_typeck_results(def_id) { cx.tcx.typeck(def_id).expr_ty_opt(expr) == Some(self_ty) @@ -167,205 +273,183 @@ impl<'a, 'tcx> Visitor<'tcx> for BodyVisitor<'a, 'tcx> { false } } - match expr.kind { - ExprKind::Struct(QPath::Resolved(_, path), ..) => { - if expr_ty_matches(expr, self.self_ty, self.cx) { - match path.res { - def::Res::SelfTy(..) => (), - def::Res::Def(DefKind::Variant, _) => span_lint_on_path_until_last_segment(self.cx, path), - _ => { - span_lint(self.cx, path.span); - }, + + if in_external_macro(cx.sess(), expr.span) | !meets_msrv(self.msrv.as_ref(), &USE_SELF_MSRV) { + return; + } + + if let Some(StackItem::Check { hir_id, .. }) = self.stack.last() { + let self_ty = ty_from_hir_id(cx, *hir_id); + + match &expr.kind { + ExprKind::Struct(QPath::Resolved(_, path), ..) => { + if expr_ty_matches(cx, expr, self_ty) { + match path.res { + def::Res::SelfTy(..) => (), + def::Res::Def(DefKind::Variant, _) => span_lint_on_path_until_last_segment(cx, path), + _ => { + span_lint(cx, path.span); + }, + } } - } - }, - // tuple struct instantiation (`Foo(arg)` or `Enum::Foo(arg)`) - ExprKind::Call(fun, _) => { - if let Expr { - kind: ExprKind::Path(ref qpath), - .. - } = fun - { - if expr_ty_matches(expr, self.self_ty, self.cx) { - let res = utils::qpath_res(self.cx, qpath, fun.hir_id); - - if let def::Res::Def(DefKind::Ctor(ctor_of, _), ..) = res { - match ctor_of { - def::CtorOf::Variant => { - span_lint_on_qpath_resolved(self.cx, qpath, true); - }, - def::CtorOf::Struct => { - span_lint_on_qpath_resolved(self.cx, qpath, false); - }, + }, + // tuple struct instantiation (`Foo(arg)` or `Enum::Foo(arg)`) + ExprKind::Call(fun, _) => { + if let Expr { + kind: ExprKind::Path(ref qpath), + .. + } = fun + { + if expr_ty_matches(cx, expr, self_ty) { + let res = qpath_res(cx, qpath, fun.hir_id); + + if let def::Res::Def(DefKind::Ctor(ctor_of, _), ..) = res { + match ctor_of { + def::CtorOf::Variant => { + span_lint_on_qpath_resolved(cx, qpath, true); + }, + def::CtorOf::Struct => { + span_lint_on_qpath_resolved(cx, qpath, false); + }, + } } } } - } - }, - // unit enum variants (`Enum::A`) - ExprKind::Path(ref qpath) => { - if expr_ty_matches(expr, self.self_ty, self.cx) { - span_lint_on_qpath_resolved(self.cx, qpath, true); - } - }, - _ => (), + }, + // unit enum variants (`Enum::A`) + ExprKind::Path(qpath) => { + if expr_ty_matches(cx, expr, self_ty) { + span_lint_on_qpath_resolved(cx, &qpath, true); + } + }, + _ => (), + } } - walk_expr(self, expr); } + + extract_msrv_attr!(LateContext); } -struct FnSigVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - self_ty: Ty<'tcx>, +#[derive(Default)] +struct SkipTyCollector { + types_to_skip: Vec, } -impl<'a, 'tcx> Visitor<'tcx> for FnSigVisitor<'a, 'tcx> { +impl<'tcx> Visitor<'tcx> for SkipTyCollector { type Map = Map<'tcx>; - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::None - } + fn visit_ty(&mut self, hir_ty: &hir::Ty<'_>) { + self.types_to_skip.push(hir_ty.hir_id); - fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'tcx>) { - if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind { - match path.res { - def::Res::SelfTy(..) => {}, - _ => { - match self.cx.tcx.hir().find(self.cx.tcx.hir().get_parent_node(hir_ty.hir_id)) { - Some(Node::Expr(Expr { - kind: ExprKind::Path(QPath::TypeRelative(_, segment)), - .. - })) => { - // The following block correctly identifies applicable lint locations - // but `hir_ty_to_ty` calls cause odd ICEs. - // - if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { - // fixme: this span manipulation should not be necessary - // @flip1995 found an ast lowering issue in - // https://github.com/rust-lang/rust/blob/master/src/librustc_ast_lowering/path.rs#l142-l162 - span_lint_until_last_segment(self.cx, hir_ty.span, segment); - } - }, - _ => { - if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty { - span_lint(self.cx, hir_ty.span) - } - }, - } - }, - } - } + walk_ty(self, hir_ty) + } - walk_ty(self, hir_ty); + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None } } -impl<'tcx> LateLintPass<'tcx> for UseSelf { - fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) { - if in_external_macro(cx.sess(), impl_item.span) { - return; - } +struct LintTyCollector<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + self_ty: Ty<'tcx>, + types_to_lint: Vec, + types_to_skip: Vec, +} - let parent_id = cx.tcx.hir().get_parent_item(impl_item.hir_id); - let imp = cx.tcx.hir().expect_item(parent_id); +impl<'a, 'tcx> Visitor<'tcx> for LintTyCollector<'a, 'tcx> { + type Map = Map<'tcx>; + fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'_>) { if_chain! { - if let ItemKind::Impl { self_ty: hir_self_ty, .. } = imp.kind; - if let TyKind::Path(QPath::Resolved(_, ref item_path)) = hir_self_ty.kind; + if let Some(ty) = self.cx.typeck_results().node_type_opt(hir_ty.hir_id); + if should_lint_ty(hir_ty, ty, self.self_ty); then { - let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args; - let should_check = parameters.as_ref().map_or( - true, - |params| !params.parenthesized - &&!params.args.iter().any(|arg| matches!(arg, GenericArg::Lifetime(_))) - ); - - // TODO: don't short-circuit upon lifetime parameters - if should_check { - let self_ty = hir_ty_to_ty(cx.tcx, hir_self_ty); - let body_visitor = &mut BodyVisitor { cx, self_ty }; - let fn_sig_visitor = &mut FnSigVisitor { cx, self_ty }; - - let tcx = cx.tcx; - let impl_def_id = tcx.hir().local_def_id(imp.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) = &impl_item.kind; - then { - body_visitor.check_trait_method_impl_decl(impl_item, impl_decl, impl_trait_ref); - let body = tcx.hir().body(*impl_body_id); - body_visitor.visit_body(body); - } else { - walk_impl_item(body_visitor, impl_item); - walk_impl_item(fn_sig_visitor, impl_item); - } - } - } + self.types_to_lint.push(hir_ty.hir_id); + } else { + self.types_to_skip.push(hir_ty.hir_id); } } + + walk_ty(self, hir_ty) + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None } - extract_msrv_attr!(LateContext); } -struct UseSelfVisitor<'a, 'tcx> { - item_path: &'a Path<'a>, - cx: &'a LateContext<'tcx>, +fn span_lint(cx: &LateContext<'_>, span: Span) { + span_lint_and_sugg( + cx, + USE_SELF, + span, + "unnecessary structure name repetition", + "use the applicable keyword", + "Self".to_owned(), + Applicability::MachineApplicable, + ); } -impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> { - type Map = Map<'tcx>; +#[allow(clippy::cast_possible_truncation)] +fn span_lint_until_last_segment(cx: &LateContext<'_>, span: Span, segment: &PathSegment<'_>) { + let sp = span.with_hi(segment.ident.span.lo()); + // remove the trailing :: + let span_without_last_segment = match snippet_opt(cx, sp) { + Some(snippet) => match snippet.rfind("::") { + Some(bidx) => sp.with_hi(sp.lo() + BytePos(bidx as u32)), + None => sp, + }, + None => sp, + }; + span_lint(cx, span_without_last_segment); +} - 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)); - } - } - } +fn span_lint_on_path_until_last_segment(cx: &LateContext<'_>, path: &Path<'_>) { + if path.segments.len() > 1 { + span_lint_until_last_segment(cx, path.span, path.segments.last().unwrap()); + } +} - 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); - } - } - } +fn span_lint_on_qpath_resolved(cx: &LateContext<'_>, qpath: &QPath<'_>, until_last_segment: bool) { + if let QPath::Resolved(_, path) = qpath { + if until_last_segment { + span_lint_on_path_until_last_segment(cx, path); + } else { + span_lint(cx, path.span); } + } +} - walk_path(self, path); +fn ty_from_hir_id<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Ty<'tcx> { + if let Some(Node::Ty(hir_ty)) = cx.tcx.hir().find(hir_id) { + hir_ty_to_ty(cx.tcx, hir_ty) + } else { + unreachable!("This function should only be called with `HirId`s that are for sure `Node::Ty`") } +} - 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 in_impl(cx: &LateContext<'tcx>, hir_ty: &hir::Ty<'_>) -> bool { + let map = cx.tcx.hir(); + let parent = map.get_parent_node(hir_ty.hir_id); + if_chain! { + if let Some(Node::Item(item)) = map.find(parent); + if let ItemKind::Impl { .. } = item.kind; + then { + true + } else { + false } } +} - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::All(self.cx.tcx.hir()) +fn should_lint_ty(hir_ty: &hir::Ty<'_>, ty: Ty<'_>, self_ty: Ty<'_>) -> bool { + if_chain! { + if ty == self_ty; + if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind; + then { + !matches!(path.res, def::Res::SelfTy(..)) + } else { + false + } } } From bb40db7adc6c42fb675559b7ac92468ed416747f Mon Sep 17 00:00:00 2001 From: flip1995 Date: Fri, 16 Oct 2020 13:26:02 +0200 Subject: [PATCH 4/8] Update test files --- tests/ui/use_self.fixed | 41 +++------- tests/ui/use_self.rs | 21 +---- tests/ui/use_self.stderr | 140 ++++++++++++++++++++++++--------- tests/ui/use_self_trait.fixed | 15 ++-- tests/ui/use_self_trait.rs | 1 + tests/ui/use_self_trait.stderr | 82 ++++++++++++++++++- 6 files changed, 208 insertions(+), 92 deletions(-) diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index d59750cbfd8d9..1632e6aca448d 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -3,7 +3,7 @@ #![warn(clippy::use_self)] #![allow(dead_code)] -#![allow(clippy::should_implement_trait, clippy::upper_case_acronyms)] +#![allow(clippy::should_implement_trait, clippy::upper_case_acronyms, clippy::from_over_into)] fn main() {} @@ -15,16 +15,13 @@ mod use_self { Self {} } fn test() -> Self { - // FIXME: applicable here - Foo::new() + Self::new() } } impl Default for Foo { - // FIXME: applicable here - fn default() -> Foo { - // FIXME: applicable here - Foo::new() + fn default() -> Self { + Self::new() } } } @@ -74,13 +71,12 @@ mod lifetimes { mod issue2894 { trait IntoBytes { - #[allow(clippy::wrong_self_convention)] - fn into_bytes(&self) -> Vec; + fn to_bytes(&self) -> Vec; } // This should not be linted impl IntoBytes for u8 { - fn into_bytes(&self) -> Vec { + fn to_bytes(&self) -> Vec { vec![*self] } } @@ -90,11 +86,7 @@ mod existential { struct Foo; impl Foo { - // FIXME: - // TyKind::Def (used for `impl Trait` types) does not include type parameters yet. - // See documentation in rustc_hir::hir::TyKind. - // The hir tree walk stops at `impl Iterator` level and does not inspect &Foo. - fn bad(foos: &[Self]) -> impl Iterator { + fn bad(foos: &[Self]) -> impl Iterator { foos.iter() } @@ -215,10 +207,8 @@ mod rustfix { fn fun_1() {} fn fun_2() { - // FIXME: applicable here - nested::A::fun_1(); - // FIXME: applicable here - nested::A::A; + Self::fun_1(); + Self::A; Self {}; } @@ -239,8 +229,7 @@ mod issue3567 { impl Test for TestStruct { fn test() -> TestStruct { - // FIXME: applicable here - TestStruct::from_something() + Self::from_something() } } } @@ -254,14 +243,12 @@ mod paths_created_by_lowering { const A: usize = 0; const B: usize = 1; - // FIXME: applicable here - async fn g() -> S { + async fn g() -> Self { Self {} } fn f<'a>(&self, p: &'a [u8]) -> &'a [u8] { - // FIXME: applicable here twice - &p[S::A..S::B] + &p[Self::A..Self::B] } } @@ -381,7 +368,6 @@ mod issue4305 { impl From for Box { fn from(t: T) -> Self { - // FIXME: applicable here Box::new(t) } } @@ -461,8 +447,7 @@ mod nested_paths { impl A { fn test() -> Self { - // FIXME: applicable here - A::new::(submod::B {}) + Self::new::(submod::B {}) } } } diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index 85606049774e1..bbe92c9e3386b 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -3,7 +3,7 @@ #![warn(clippy::use_self)] #![allow(dead_code)] -#![allow(clippy::should_implement_trait, clippy::upper_case_acronyms)] +#![allow(clippy::should_implement_trait, clippy::upper_case_acronyms, clippy::from_over_into)] fn main() {} @@ -15,15 +15,12 @@ mod use_self { Foo {} } fn test() -> Foo { - // FIXME: applicable here Foo::new() } } impl Default for Foo { - // FIXME: applicable here fn default() -> Foo { - // FIXME: applicable here Foo::new() } } @@ -74,13 +71,12 @@ mod lifetimes { mod issue2894 { trait IntoBytes { - #[allow(clippy::wrong_self_convention)] - fn into_bytes(&self) -> Vec; + fn to_bytes(&self) -> Vec; } // This should not be linted impl IntoBytes for u8 { - fn into_bytes(&self) -> Vec { + fn to_bytes(&self) -> Vec { vec![*self] } } @@ -90,10 +86,6 @@ mod existential { struct Foo; impl Foo { - // FIXME: - // TyKind::Def (used for `impl Trait` types) does not include type parameters yet. - // See documentation in rustc_hir::hir::TyKind. - // The hir tree walk stops at `impl Iterator` level and does not inspect &Foo. fn bad(foos: &[Foo]) -> impl Iterator { foos.iter() } @@ -215,9 +207,7 @@ mod rustfix { fn fun_1() {} fn fun_2() { - // FIXME: applicable here nested::A::fun_1(); - // FIXME: applicable here nested::A::A; nested::A {}; @@ -239,7 +229,6 @@ mod issue3567 { impl Test for TestStruct { fn test() -> TestStruct { - // FIXME: applicable here TestStruct::from_something() } } @@ -254,13 +243,11 @@ mod paths_created_by_lowering { const A: usize = 0; const B: usize = 1; - // FIXME: applicable here async fn g() -> S { S {} } fn f<'a>(&self, p: &'a [u8]) -> &'a [u8] { - // FIXME: applicable here twice &p[S::A..S::B] } } @@ -381,7 +368,6 @@ mod issue4305 { impl From for Box { fn from(t: T) -> Self { - // FIXME: applicable here Box::new(t) } } @@ -461,7 +447,6 @@ mod nested_paths { impl A { fn test() -> Self { - // FIXME: applicable here A::new::(submod::B {}) } } diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index 4d213316cf53c..d86453eb2f057 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -1,16 +1,16 @@ error: unnecessary structure name repetition - --> $DIR/use_self.rs:15:13 + --> $DIR/use_self.rs:14:21 | -LL | Foo {} - | ^^^ help: use the applicable keyword: `Self` +LL | fn new() -> Foo { + | ^^^ help: use the applicable keyword: `Self` | = note: `-D clippy::use-self` implied by `-D warnings` error: unnecessary structure name repetition - --> $DIR/use_self.rs:14:21 + --> $DIR/use_self.rs:15:13 | -LL | fn new() -> Foo { - | ^^^ help: use the applicable keyword: `Self` +LL | Foo {} + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition --> $DIR/use_self.rs:17:22 @@ -19,22 +19,46 @@ LL | fn test() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:96:24 + --> $DIR/use_self.rs:18:13 + | +LL | Foo::new() + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:23:25 + | +LL | fn default() -> Foo { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:24:13 + | +LL | Foo::new() + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:89:24 | LL | fn bad(foos: &[Foo]) -> impl Iterator { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:111:13 + --> $DIR/use_self.rs:89:55 + | +LL | fn bad(foos: &[Foo]) -> 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` error: unnecessary structure name repetition - --> $DIR/use_self.rs:120:17 + --> $DIR/use_self.rs:112:25 | -LL | Foo {} - | ^^^ help: use the applicable keyword: `Self` +LL | fn new() -> Foo { + | ^^^ help: use the applicable keyword: `Self` ... LL | use_self_expand!(); // Should lint in local macros | ------------------- in this macro invocation @@ -42,10 +66,10 @@ LL | use_self_expand!(); // Should lint in local macros = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary structure name repetition - --> $DIR/use_self.rs:119:25 + --> $DIR/use_self.rs:113:17 | -LL | fn new() -> Foo { - | ^^^ help: use the applicable keyword: `Self` +LL | Foo {} + | ^^^ help: use the applicable keyword: `Self` ... LL | use_self_expand!(); // Should lint in local macros | ------------------- in this macro invocation @@ -53,82 +77,124 @@ LL | use_self_expand!(); // Should lint in local macros = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary structure name repetition - --> $DIR/use_self.rs:144:21 - | -LL | Bar { foo: Foo {} } - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:143:29 + --> $DIR/use_self.rs:136:29 | LL | fn bar() -> Bar { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:156:13 + --> $DIR/use_self.rs:137:21 | -LL | Foo {} - | ^^^ help: use the applicable keyword: `Self` +LL | Bar { foo: Foo {} } + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:155:21 + --> $DIR/use_self.rs:148:21 | LL | fn baz() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:173:21 + --> $DIR/use_self.rs:149:13 + | +LL | 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:174:21 + --> $DIR/use_self.rs:167:21 | LL | let _ = Enum::C { field: true }; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:175:21 + --> $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:222:13 + --> $DIR/use_self.rs:210:13 + | +LL | nested::A::fun_1(); + | ^^^^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:211:13 + | +LL | nested::A::A; + | ^^^^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:213:13 | LL | nested::A {}; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:258:13 + --> $DIR/use_self.rs:232:13 + | +LL | TestStruct::from_something() + | ^^^^^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:246:25 + | +LL | async fn g() -> S { + | ^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:247:13 | LL | S {} | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:287:13 + --> $DIR/use_self.rs:251:16 | -LL | Foo { value } - | ^^^ help: use the applicable keyword: `Self` +LL | &p[S::A..S::B] + | ^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:251:22 + | +LL | &p[S::A..S::B] + | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:286:29 + --> $DIR/use_self.rs:274:29 | LL | fn foo(value: T) -> Foo { | ^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:324:21 + --> $DIR/use_self.rs:275:13 + | +LL | Foo { value } + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:312:21 | LL | type From = T::From; | ^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:325:19 + --> $DIR/use_self.rs:313:19 | LL | type To = T::To; | ^^^^^ help: use the applicable keyword: `Self` -error: aborting due to 20 previous errors +error: unnecessary structure name repetition + --> $DIR/use_self.rs:450:13 + | +LL | A::new::(submod::B {}) + | ^ help: use the applicable keyword: `Self` + +error: aborting due to 31 previous errors diff --git a/tests/ui/use_self_trait.fixed b/tests/ui/use_self_trait.fixed index 680a0623839e0..9bcd692fb3511 100644 --- a/tests/ui/use_self_trait.fixed +++ b/tests/ui/use_self_trait.fixed @@ -18,35 +18,36 @@ trait SelfTrait { struct Bad; impl SelfTrait for Bad { - fn refs(p1: &Bad) -> &Bad { + fn refs(p1: &Self) -> &Self { p1 } - fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { + fn ref_refs<'a>(p1: &'a &'a Self) -> &'a &'a Self { p1 } - fn mut_refs(p1: &mut Bad) -> &mut Bad { + fn mut_refs(p1: &mut Self) -> &mut Self { p1 } - fn nested(_p1: Box, _p2: (&u8, &Bad)) {} + fn nested(_p1: Box, _p2: (&u8, &Self)) {} - fn vals(_: Bad) -> Bad { - Bad::default() + fn vals(_: Self) -> Self { + Self::default() } } impl Mul for Bad { type Output = Self; - fn mul(self, rhs: Bad) -> Bad { + fn mul(self, rhs: Self) -> Self { rhs } } impl Clone for Bad { fn clone(&self) -> Self { + // FIXME: applicable here Bad } } diff --git a/tests/ui/use_self_trait.rs b/tests/ui/use_self_trait.rs index 70667b9797e76..de305d40f330b 100644 --- a/tests/ui/use_self_trait.rs +++ b/tests/ui/use_self_trait.rs @@ -47,6 +47,7 @@ impl Mul for Bad { impl Clone for Bad { fn clone(&self) -> Self { + // FIXME: applicable here Bad } } diff --git a/tests/ui/use_self_trait.stderr b/tests/ui/use_self_trait.stderr index 5409ccedf8592..55af3ff2a93d9 100644 --- a/tests/ui/use_self_trait.stderr +++ b/tests/ui/use_self_trait.stderr @@ -1,10 +1,88 @@ +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:21:18 + | +LL | fn refs(p1: &Bad) -> &Bad { + | ^^^ help: use the applicable keyword: `Self` + | + = note: `-D clippy::use-self` implied by `-D warnings` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:21:27 + | +LL | fn refs(p1: &Bad) -> &Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:25:33 + | +LL | fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:25:49 + | +LL | fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:29:26 + | +LL | fn mut_refs(p1: &mut Bad) -> &mut Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:29:39 + | +LL | fn mut_refs(p1: &mut Bad) -> &mut Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:33:24 + | +LL | fn nested(_p1: Box, _p2: (&u8, &Bad)) {} + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:33:42 + | +LL | fn nested(_p1: Box, _p2: (&u8, &Bad)) {} + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:35:16 + | +LL | fn vals(_: Bad) -> Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:35:24 + | +LL | fn vals(_: Bad) -> Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:36:9 + | +LL | Bad::default() + | ^^^ help: use the applicable keyword: `Self` + error: unnecessary structure name repetition --> $DIR/use_self_trait.rs:41:19 | LL | type Output = Bad; | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:43:23 | - = note: `-D clippy::use-self` implied by `-D warnings` +LL | fn mul(self, rhs: Bad) -> Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self_trait.rs:43:31 + | +LL | fn mul(self, rhs: Bad) -> Bad { + | ^^^ help: use the applicable keyword: `Self` -error: aborting due to previous error +error: aborting due to 14 previous errors From da65d8166fe50a817a49d542460986234e94dc6d Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sun, 17 Jan 2021 19:09:24 +0100 Subject: [PATCH 5/8] Don't trigger use_self in macros --- clippy_lints/src/use_self.rs | 7 +-- tests/ui/auxiliary/proc_macro_derive.rs | 12 ++++ tests/ui/use_self.fixed | 13 +++- tests/ui/use_self.rs | 9 ++- tests/ui/use_self.stderr | 82 +++++++++---------------- 5 files changed, 63 insertions(+), 60 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index fbd996ad0e94d..3dfec190541db 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -1,4 +1,4 @@ -use crate::utils::{meets_msrv, qpath_res, snippet_opt, span_lint_and_sugg}; +use crate::utils::{in_macro, meets_msrv, qpath_res, snippet_opt, span_lint_and_sugg}; use if_chain::if_chain; use rustc_errors::Applicability; @@ -13,7 +13,6 @@ use rustc_hir::{ }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; -use rustc_middle::lint::in_external_macro; use rustc_middle::ty::{AssocKind, Ty}; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; @@ -232,7 +231,7 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { } fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>) { - if in_external_macro(cx.sess(), hir_ty.span) + if in_macro(hir_ty.span) | in_impl(cx, hir_ty) | self.types_to_skip.contains(&hir_ty.hir_id) | !meets_msrv(self.msrv.as_ref(), &USE_SELF_MSRV) @@ -274,7 +273,7 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { } } - if in_external_macro(cx.sess(), expr.span) | !meets_msrv(self.msrv.as_ref(), &USE_SELF_MSRV) { + if in_macro(expr.span) | !meets_msrv(self.msrv.as_ref(), &USE_SELF_MSRV) { return; } diff --git a/tests/ui/auxiliary/proc_macro_derive.rs b/tests/ui/auxiliary/proc_macro_derive.rs index 24891682d368d..aebeaf346799d 100644 --- a/tests/ui/auxiliary/proc_macro_derive.rs +++ b/tests/ui/auxiliary/proc_macro_derive.rs @@ -41,3 +41,15 @@ pub fn derive_foo(_input: TokenStream) -> TokenStream { } } } + +#[proc_macro_derive(StructAUseSelf)] +pub fn derive_use_self(_input: TokenStream) -> proc_macro::TokenStream { + quote! { + struct A; + impl A { + fn new() -> A { + A + } + } + } +} diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index 1632e6aca448d..95e7bc754310f 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -1,10 +1,14 @@ // run-rustfix // edition:2018 +// aux-build:proc_macro_derive.rs #![warn(clippy::use_self)] #![allow(dead_code)] #![allow(clippy::should_implement_trait, clippy::upper_case_acronyms, clippy::from_over_into)] +#[macro_use] +extern crate proc_macro_derive; + fn main() {} mod use_self { @@ -109,8 +113,8 @@ mod tuple_structs { mod macros { macro_rules! use_self_expand { () => { - fn new() -> Self { - Self {} + fn new() -> Foo { + Foo {} } }; } @@ -118,8 +122,11 @@ mod macros { struct Foo {} impl Foo { - use_self_expand!(); // Should lint in local macros + use_self_expand!(); // Should not lint in local macros } + + #[derive(StructAUseSelf)] // Should not lint in derives + struct A; } mod nesting { diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index bbe92c9e3386b..75424f341597d 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -1,10 +1,14 @@ // run-rustfix // edition:2018 +// aux-build:proc_macro_derive.rs #![warn(clippy::use_self)] #![allow(dead_code)] #![allow(clippy::should_implement_trait, clippy::upper_case_acronyms, clippy::from_over_into)] +#[macro_use] +extern crate proc_macro_derive; + fn main() {} mod use_self { @@ -118,8 +122,11 @@ mod macros { struct Foo {} impl Foo { - use_self_expand!(); // Should lint in local macros + use_self_expand!(); // Should not lint in local macros } + + #[derive(StructAUseSelf)] // Should not lint in derives + struct A; } mod nesting { diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index d86453eb2f057..37dfef7cfe0e5 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -1,5 +1,5 @@ error: unnecessary structure name repetition - --> $DIR/use_self.rs:14:21 + --> $DIR/use_self.rs:18:21 | LL | fn new() -> Foo { | ^^^ help: use the applicable keyword: `Self` @@ -7,194 +7,172 @@ LL | fn new() -> Foo { = note: `-D clippy::use-self` implied by `-D warnings` error: unnecessary structure name repetition - --> $DIR/use_self.rs:15:13 + --> $DIR/use_self.rs:19:13 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:17:22 + --> $DIR/use_self.rs:21:22 | LL | fn test() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:18:13 + --> $DIR/use_self.rs:22:13 | LL | Foo::new() | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:23:25 + --> $DIR/use_self.rs:27:25 | LL | fn default() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:24:13 + --> $DIR/use_self.rs:28:13 | LL | Foo::new() | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:89:24 + --> $DIR/use_self.rs:93:24 | LL | fn bad(foos: &[Foo]) -> impl Iterator { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:89:55 + --> $DIR/use_self.rs:93:55 | LL | fn bad(foos: &[Foo]) -> impl Iterator { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:104:13 + --> $DIR/use_self.rs:108:13 | LL | TS(0) | ^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:112:25 - | -LL | fn new() -> Foo { - | ^^^ help: use the applicable keyword: `Self` -... -LL | use_self_expand!(); // Should lint in local macros - | ------------------- in this macro invocation - | - = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:113:17 - | -LL | Foo {} - | ^^^ help: use the applicable keyword: `Self` -... -LL | use_self_expand!(); // Should lint in local macros - | ------------------- in this macro invocation - | - = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:136:29 + --> $DIR/use_self.rs:143:29 | LL | fn bar() -> Bar { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:137:21 + --> $DIR/use_self.rs:144:21 | LL | Bar { foo: Foo {} } | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:148:21 + --> $DIR/use_self.rs:155:21 | LL | fn baz() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:149:13 + --> $DIR/use_self.rs:156:13 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:166:21 + --> $DIR/use_self.rs:173:21 | LL | let _ = Enum::B(42); | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:167:21 + --> $DIR/use_self.rs:174:21 | LL | let _ = Enum::C { field: true }; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:168:21 + --> $DIR/use_self.rs:175:21 | LL | let _ = Enum::A; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:210:13 + --> $DIR/use_self.rs:217:13 | LL | nested::A::fun_1(); | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:211:13 + --> $DIR/use_self.rs:218:13 | LL | nested::A::A; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:213:13 + --> $DIR/use_self.rs:220:13 | LL | nested::A {}; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:232:13 + --> $DIR/use_self.rs:239:13 | LL | TestStruct::from_something() | ^^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:246:25 + --> $DIR/use_self.rs:253:25 | LL | async fn g() -> S { | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:247:13 + --> $DIR/use_self.rs:254:13 | LL | S {} | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:251:16 + --> $DIR/use_self.rs:258:16 | LL | &p[S::A..S::B] | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:251:22 + --> $DIR/use_self.rs:258:22 | LL | &p[S::A..S::B] | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:274:29 + --> $DIR/use_self.rs:281:29 | LL | fn foo(value: T) -> Foo { | ^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:275:13 + --> $DIR/use_self.rs:282:13 | LL | Foo { value } | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:312:21 + --> $DIR/use_self.rs:319:21 | LL | type From = T::From; | ^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:313:19 + --> $DIR/use_self.rs:320:19 | LL | type To = T::To; | ^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:450:13 + --> $DIR/use_self.rs:457:13 | LL | A::new::(submod::B {}) | ^ help: use the applicable keyword: `Self` -error: aborting due to 31 previous errors +error: aborting due to 29 previous errors From 7e1c1c154166d9eaa4184eab166b540e17ff26f3 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Tue, 9 Feb 2021 19:38:40 -0600 Subject: [PATCH 6/8] Fix qpath_res call --- clippy_lints/src/use_self.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 3dfec190541db..cdaa08be86b14 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -1,4 +1,4 @@ -use crate::utils::{in_macro, meets_msrv, qpath_res, snippet_opt, span_lint_and_sugg}; +use crate::utils::{in_macro, meets_msrv, snippet_opt, span_lint_and_sugg}; use if_chain::if_chain; use rustc_errors::Applicability; @@ -300,7 +300,7 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { } = fun { if expr_ty_matches(cx, expr, self_ty) { - let res = qpath_res(cx, qpath, fun.hir_id); + let res = cx.qpath_res(qpath, fun.hir_id); if let def::Res::Def(DefKind::Ctor(ctor_of, _), ..) = res { match ctor_of { From 7f61ddd5b85f5e2340ef238476139df8a837afb1 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Tue, 9 Feb 2021 19:39:38 -0600 Subject: [PATCH 7/8] Move "types to lint" to the item stack --- clippy_lints/src/use_self.rs | 73 ++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index cdaa08be86b14..8c83ad5650d81 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -59,8 +59,6 @@ declare_clippy_lint! { pub struct UseSelf { msrv: Option, stack: Vec, - types_to_skip: Vec, - types_to_lint: Vec, } const USE_SELF_MSRV: RustcVersion = RustcVersion::new(1, 37, 0); @@ -75,11 +73,13 @@ impl UseSelf { } } -#[derive(Debug, PartialEq, Eq, Copy, Clone)] +#[derive(Debug)] enum StackItem { Check { hir_id: HirId, impl_trait_ref_def_id: Option, + types_to_skip: Vec, + types_to_lint: Vec, }, NoCheck, } @@ -116,6 +116,8 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { self.stack.push(StackItem::Check { hir_id: hir_self_ty.hir_id, impl_trait_ref_def_id, + types_to_lint: Vec::new(), + types_to_skip: Vec::new(), }); } else { self.stack.push(StackItem::NoCheck); @@ -149,7 +151,11 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { // declaration. The collection of those types is all this method implementation does. if_chain! { if let ImplItemKind::Fn(FnSig { decl, .. }, ..) = impl_item.kind; - if let Some(StackItem::Check { impl_trait_ref_def_id: Some(def_id), .. }) = self.stack.last().copied(); + if let Some(&mut StackItem::Check { + impl_trait_ref_def_id: Some(def_id), + ref mut types_to_skip, + .. + }) = self.stack.last_mut(); if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(def_id); then { // `self_ty` is the semantic self type of `impl for `. This cannot be @@ -191,17 +197,13 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { if trait_sem_ty.walk().any(|inner| inner == self_ty.into()) { let mut visitor = SkipTyCollector::default(); visitor.visit_ty(&impl_hir_ty); - self.types_to_skip.extend(visitor.types_to_skip); + types_to_skip.extend(visitor.types_to_skip); } } } } } - fn check_impl_item_post(&mut self, _: &LateContext<'_>, _: &hir::ImplItem<'_>) { - self.types_to_skip.clear(); - } - fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx hir::Body<'_>) { // `hir_ty_to_ty` cannot be called in `Body`s or it will panic (sometimes). But in bodies // we can use `cx.typeck_results.node_type(..)` to get the `ty::Ty` from a `hir::Ty`. @@ -211,7 +213,13 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { // which shouldn't, with a visitor. We could directly lint in the visitor, but then we // could only allow this lint on item scope. And we would have to check if those types are // already dealt with in `check_ty` anyway. - if let Some(StackItem::Check { hir_id, .. }) = self.stack.last() { + if let Some(StackItem::Check { + hir_id, + types_to_lint, + types_to_skip, + .. + }) = self.stack.last_mut() + { let self_ty = ty_from_hir_id(cx, *hir_id); let mut visitor = LintTyCollector { @@ -221,25 +229,36 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { types_to_skip: vec![], }; visitor.visit_expr(&body.value); - self.types_to_lint.extend(visitor.types_to_lint); - self.types_to_skip.extend(visitor.types_to_skip); + types_to_lint.extend(visitor.types_to_lint); + types_to_skip.extend(visitor.types_to_skip); } } - fn check_body_post(&mut self, _: &LateContext<'_>, _: &hir::Body<'_>) { - self.types_to_lint.clear(); - } - fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>) { - if in_macro(hir_ty.span) - | in_impl(cx, hir_ty) - | self.types_to_skip.contains(&hir_ty.hir_id) - | !meets_msrv(self.msrv.as_ref(), &USE_SELF_MSRV) - { + if in_macro(hir_ty.span) | in_impl(cx, hir_ty) | !meets_msrv(self.msrv.as_ref(), &USE_SELF_MSRV) { return; } - let lint_dependend_on_expr_kind = || { + let lint_dependend_on_expr_kind = if let Some(StackItem::Check { + hir_id, + types_to_lint, + types_to_skip, + .. + }) = self.stack.last() + { + if types_to_skip.contains(&hir_ty.hir_id) { + false + } else if types_to_lint.contains(&hir_ty.hir_id) { + true + } else { + let self_ty = ty_from_hir_id(cx, *hir_id); + should_lint_ty(hir_ty, hir_ty_to_ty(cx.tcx, hir_ty), self_ty) + } + } else { + false + }; + + if lint_dependend_on_expr_kind { // FIXME: this span manipulation should not be necessary // @flip1995 found an ast lowering issue in // https://github.com/rust-lang/rust/blob/master/src/librustc_ast_lowering/path.rs#l142-l162 @@ -250,16 +269,6 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { })) => span_lint_until_last_segment(cx, hir_ty.span, segment), _ => span_lint(cx, hir_ty.span), } - }; - - if self.types_to_lint.contains(&hir_ty.hir_id) { - lint_dependend_on_expr_kind(); - } else if let Some(StackItem::Check { hir_id, .. }) = self.stack.last() { - let self_ty = ty_from_hir_id(cx, *hir_id); - - if should_lint_ty(hir_ty, hir_ty_to_ty(cx.tcx, hir_ty), self_ty) { - lint_dependend_on_expr_kind(); - } } } From 52f98d832dc5ca982b9edad7807b2a9a9ddd13e0 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Tue, 9 Feb 2021 19:42:32 -0600 Subject: [PATCH 8/8] Use TyS::same_type --- clippy_lints/src/use_self.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 8c83ad5650d81..a0dd53af661b6 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -13,7 +13,7 @@ use rustc_hir::{ }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; -use rustc_middle::ty::{AssocKind, Ty}; +use rustc_middle::ty::{AssocKind, Ty, TyS}; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{BytePos, Span}; @@ -452,7 +452,7 @@ fn in_impl(cx: &LateContext<'tcx>, hir_ty: &hir::Ty<'_>) -> bool { fn should_lint_ty(hir_ty: &hir::Ty<'_>, ty: Ty<'_>, self_ty: Ty<'_>) -> bool { if_chain! { - if ty == self_ty; + if TyS::same_type(ty, self_ty); if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind; then { !matches!(path.res, def::Res::SelfTy(..))