From 5bb52c486967abed8f5f1de74f66d9c3a17a789f Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 17 Jul 2018 08:20:49 +0200 Subject: [PATCH] Fix use_self regressions --- clippy_lints/src/use_self.rs | 51 +++++++++------- tests/ui/use_self.rs | 25 ++++++-- tests/ui/use_self.stderr | 110 ++++++++++++++++++++--------------- 3 files changed, 113 insertions(+), 73 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 82dc0fd2b8a..a4450acea09 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -4,7 +4,6 @@ use rustc::hir::*; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc::ty; use syntax::ast::NodeId; -use syntax::symbol::keywords; use syntax_pos::symbol::keywords::SelfType; /// **What it does:** Checks for unnecessary repetition of structure name when a @@ -58,26 +57,29 @@ fn span_use_self_lint(cx: &LateContext, path: &Path) { } struct TraitImplTyVisitor<'a, 'tcx: 'a> { + item_path: &'a Path, cx: &'a LateContext<'a, 'tcx>, - type_walker: ty::walk::TypeWalker<'tcx>, + trait_type_walker: ty::walk::TypeWalker<'tcx>, + impl_type_walker: ty::walk::TypeWalker<'tcx>, } impl<'a, 'tcx> Visitor<'tcx> for TraitImplTyVisitor<'a, 'tcx> { fn visit_ty(&mut self, t: &'tcx Ty) { - let trait_ty = self.type_walker.next(); + let trait_ty = self.trait_type_walker.next(); + let impl_ty = self.impl_type_walker.next(); + if let TyKind::Path(QPath::Resolved(_, path)) = &t.node { - let impl_is_self_ty = if let def::Def::SelfTy(..) = path.def { - true - } else { - false - }; - if !impl_is_self_ty { - let trait_is_self_ty = if let Some(ty::TyParam(ty::ParamTy { name, .. })) = trait_ty.map(|ty| &ty.sty) { - *name == keywords::SelfType.name().as_str() + if self.item_path.def == path.def { + let is_self_ty = if let def::Def::SelfTy(..) = path.def { + true } else { false }; - if trait_is_self_ty { + + if !is_self_ty && impl_ty != trait_ty { + // The implementation and trait types don't match which means that + // the concrete type was specified by the implementation but + // it didn't use `Self` span_use_self_lint(self.cx, path); } } @@ -92,6 +94,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TraitImplTyVisitor<'a, 'tcx> { fn check_trait_method_impl_decl<'a, 'tcx: 'a>( cx: &'a LateContext<'a, 'tcx>, + item_path: &'a Path, impl_item: &ImplItem, impl_decl: &'tcx FnDecl, impl_trait_ref: &ty::TraitRef, @@ -110,24 +113,30 @@ fn check_trait_method_impl_decl<'a, 'tcx: 'a>( 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 impl_method_def_id = cx.tcx.hir.local_def_id(impl_item.id); + let impl_method_sig = cx.tcx.fn_sig(impl_method_def_id); + let impl_method_sig = cx.tcx.erase_late_bound_regions(&impl_method_sig); + let output_ty = if let FunctionRetTy::Return(ty) = &impl_decl.output { Some(&**ty) } else { None }; - for (impl_ty, trait_ty) in impl_decl - .inputs - .iter() - .chain(output_ty) - .zip(trait_method_sig.inputs_and_output) - { + for (impl_decl_ty, (impl_ty, trait_ty)) in impl_decl.inputs.iter().chain(output_ty).zip( + impl_method_sig + .inputs_and_output + .iter() + .zip(trait_method_sig.inputs_and_output), + ) { let mut visitor = TraitImplTyVisitor { cx, - type_walker: trait_ty.walk(), + item_path, + trait_type_walker: trait_ty.walk(), + impl_type_walker: impl_ty.walk(), }; - visitor.visit_ty(&impl_ty); + visitor.visit_ty(&impl_decl_ty); } } @@ -163,7 +172,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf { let impl_item = cx.tcx.hir.impl_item(impl_item_ref.id); if let ImplItemKind::Method(MethodSig{ decl: impl_decl, .. }, impl_body_id) = &impl_item.node { - check_trait_method_impl_decl(cx, impl_item, impl_decl, &impl_trait_ref); + check_trait_method_impl_decl(cx, item_path, impl_item, impl_decl, &impl_trait_ref); let body = cx.tcx.hir.body(*impl_body_id); visitor.visit_body(body); } else { diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index e3133b0a7a1..689c9d68d12 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -1,10 +1,6 @@ - - #![warn(use_self)] #![allow(dead_code)] #![allow(should_implement_trait)] -#![allow(boxed_local)] - fn main() {} @@ -68,9 +64,10 @@ mod lifetimes { } } +#[allow(boxed_local)] mod traits { - #![cfg_attr(feature = "cargo-clippy", allow(boxed_local))] + use std::ops::Mul; trait SelfTrait { fn refs(p1: &Self) -> &Self; @@ -104,6 +101,14 @@ mod traits { } } + impl Mul for Bad { + type Output = Bad; + + fn mul(self, rhs: Bad) -> Bad { + rhs + } + } + #[derive(Default)] struct Good; @@ -128,6 +133,14 @@ mod traits { } } + impl Mul for Good { + type Output = Self; + + fn mul(self, rhs: Self) -> Self { + rhs + } + } + trait NameTrait { fn refs(p1: &u8) -> &u8; fn ref_refs<'a>(p1: &'a &'a u8) -> &'a &'a u8; @@ -162,7 +175,7 @@ mod traits { impl Clone for Good { fn clone(&self) -> Self { // Note: Not linted and it wouldn't be valid - // because "can't use `Self` as a constructor` + // because "can't use `Self` as a constructor`" Good } } diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index ede95126f86..89936101252 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -1,106 +1,124 @@ error: unnecessary structure name repetition - --> $DIR/use_self.rs:15:21 + --> $DIR/use_self.rs:11:21 | -15 | fn new() -> Foo { +11 | fn new() -> Foo { | ^^^ help: use the applicable keyword: `Self` | = note: `-D use-self` implied by `-D warnings` error: unnecessary structure name repetition - --> $DIR/use_self.rs:16:13 + --> $DIR/use_self.rs:12:13 | -16 | Foo {} +12 | Foo {} | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:18:22 + --> $DIR/use_self.rs:14:22 | -18 | fn test() -> Foo { +14 | fn test() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:19:13 + --> $DIR/use_self.rs:15:13 | -19 | Foo::new() +15 | Foo::new() | ^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:24:25 + --> $DIR/use_self.rs:20:25 | -24 | fn default() -> Foo { +20 | fn default() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:25:13 + --> $DIR/use_self.rs:21:13 | -25 | Foo::new() +21 | Foo::new() | ^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:87:22 + --> $DIR/use_self.rs:84:22 | -87 | fn refs(p1: &Bad) -> &Bad { +84 | fn refs(p1: &Bad) -> &Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:87:31 + --> $DIR/use_self.rs:84:31 | -87 | fn refs(p1: &Bad) -> &Bad { +84 | fn refs(p1: &Bad) -> &Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:91:37 + --> $DIR/use_self.rs:88:37 | -91 | fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { +88 | 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.rs:91:53 + --> $DIR/use_self.rs:88:53 | -91 | fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { +88 | 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.rs:95:30 + --> $DIR/use_self.rs:92:30 | -95 | fn mut_refs(p1: &mut Bad) -> &mut Bad { +92 | fn mut_refs(p1: &mut Bad) -> &mut Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:95:43 + --> $DIR/use_self.rs:92:43 | -95 | fn mut_refs(p1: &mut Bad) -> &mut Bad { +92 | fn mut_refs(p1: &mut Bad) -> &mut Bad { | ^^^ help: use the applicable keyword: `Self` +error: unnecessary structure name repetition + --> $DIR/use_self.rs:96:28 + | +96 | fn nested(_p1: Box, _p2: (&u8, &Bad)) { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:96:46 + | +96 | fn nested(_p1: Box, _p2: (&u8, &Bad)) { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:99:20 + | +99 | fn vals(_: Bad) -> Bad { + | ^^^ help: use the applicable keyword: `Self` + error: unnecessary structure name repetition --> $DIR/use_self.rs:99:28 | -99 | fn nested(_p1: Box, _p2: (&u8, &Bad)) { +99 | fn vals(_: Bad) -> Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:99:46 - | -99 | fn nested(_p1: Box, _p2: (&u8, &Bad)) { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:102:20 + --> $DIR/use_self.rs:100:13 | -102 | fn vals(_: Bad) -> Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:102:28 - | -102 | fn vals(_: Bad) -> Bad { - | ^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> $DIR/use_self.rs:103:13 - | -103 | Bad::default() +100 | Bad::default() | ^^^^^^^^^^^^ help: use the applicable keyword: `Self` -error: aborting due to 17 previous errors +error: unnecessary structure name repetition + --> $DIR/use_self.rs:105:23 + | +105 | type Output = Bad; + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:107:27 + | +107 | fn mul(self, rhs: Bad) -> Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:107:35 + | +107 | fn mul(self, rhs: Bad) -> Bad { + | ^^^ help: use the applicable keyword: `Self` + +error: aborting due to 20 previous errors