From fc334fb8f4cc7e6513578d88f52b2899f624a1de Mon Sep 17 00:00:00 2001 From: Tim Nielens Date: Sat, 10 Oct 2020 00:26:12 +0200 Subject: [PATCH] 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 | 96 ++++++++++++++++----------------- tests/ui/use_self_trait.fixed | 12 ++--- tests/ui/use_self_trait.stderr | 76 +------------------------- 6 files changed, 118 insertions(+), 173 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 1b5070d1cff..023fce947b3 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 916484eef93..d59750cbfd8 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 347f5e96555..85606049774 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 a88dd04f13d..4d213316cf5 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 - | -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:15:13 | 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:14:21 + | +LL | fn new() -> Foo { + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition --> $DIR/use_self.rs:17:22 @@ -19,36 +19,19 @@ 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 - | -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:118:17 + --> $DIR/use_self.rs:120:17 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` @@ -59,82 +42,93 @@ 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 + --> $DIR/use_self.rs:119:25 | -LL | fn bar() -> Bar { - | ^^^ 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 + | + = 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: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 - | -LL | fn foo(value: T) -> Foo { - | ^^^^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:283:13 + --> $DIR/use_self.rs:287:13 | LL | Foo { value } | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:320:21 + --> $DIR/use_self.rs:286:29 + | +LL | fn foo(value: T) -> Foo { + | ^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $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 d425f953a9c..680a0623839 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 fa528cc5b7d..5409ccedf85 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` + = note: `-D clippy::use-self` implied by `-D warnings` -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 13 previous errors +error: aborting due to previous error