From 8cf01753c527a04fbdd1c2b2ec0d5288a2f5285d Mon Sep 17 00:00:00 2001 From: Mason Remaley Date: Mon, 28 Aug 2017 10:19:24 -0400 Subject: [PATCH 1/3] Updates `use_self` tests for generics/lifetimes These are gonna fail right now as I haven't actually implemented the fix yet. --- tests/ui/use_self.rs | 21 +++++++++++++++++++-- tests/ui/use_self.stderr | 26 +++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index 14e31aae8ee5..8a32af4010cf 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -45,8 +45,6 @@ mod better { } } -//todo the lint does not handle lifetimed struct -//the following module should trigger the lint on the third method only mod lifetimes { struct Foo<'a>{foo_str: &'a str} @@ -55,6 +53,7 @@ mod lifetimes { fn foo(s: &str) -> Foo { Foo { foo_str: s } } + // cannot replace with `Self`, because that's `Foo<'a>` fn bar() -> Foo<'static> { Foo { foo_str: "foo"} @@ -66,3 +65,21 @@ mod lifetimes { } } } + +mod generics { + struct Foo { + value: T, + } + + impl Foo { + // `Self` is applicable here + fn foo(value: T) -> Foo { + Foo { value } + } + + // `Cannot` use `Self` as a return type as the generic types are different + fn bar(value: i32) -> Foo { + Foo { value } + } + } +} diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index bfd334335d88..7f8c6317070c 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -36,5 +36,29 @@ error: unnecessary structure name repetition 24 | Foo::new() | ^^^^^^^^ help: use the applicable keyword: `Self` -error: aborting due to 6 previous errors +error: unnecessary structure name repetition + --> $DIR/use_self.rs:63:28 + | +63 | fn clone(&self) -> Foo<'a> { + | ^^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:64:13 + | +64 | Foo {foo_str: self.foo_str} + | ^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:76:29 + | +76 | fn foo(value: T) -> Foo { + | ^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:77:13 + | +77 | Foo { value } + | ^^^ help: use the applicable keyword: `Self` + +error: aborting due to 10 previous errors From 06281f2cc2d4b5473d75ab7955854a4f43b7c9e0 Mon Sep 17 00:00:00 2001 From: Mason Remaley Date: Mon, 28 Aug 2017 11:05:06 -0400 Subject: [PATCH 2/3] Only display `use_self` lint if lifetimes match --- clippy_lints/src/use_self.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index fffeb3bb6995..49bdc59ef836 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -58,7 +58,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf { let Ty_::TyPath(QPath::Resolved(_, ref item_path)) = item_type.node, ], { let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).parameters; - if !parameters.parenthesized && parameters.lifetimes.len() == 0 { + if !parameters.parenthesized { let visitor = &mut UseSelfVisitor { item_path: item_path, cx: cx, @@ -78,11 +78,18 @@ struct UseSelfVisitor<'a, 'tcx: 'a> { impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> { fn visit_path(&mut self, path: &'tcx Path, _id: NodeId) { + let lifetimes = &self.item_path.segments.last().expect(SEGMENTS_MSG).parameters.lifetimes; + let self_lifetimes = &path.segments.last().expect(SEGMENTS_MSG).parameters.lifetimes; + if self.item_path.def == path.def && path.segments .last() .expect(SEGMENTS_MSG) - .name != SelfType.name() { + .name != SelfType.name() && + lifetimes.iter() + .zip(self_lifetimes.iter()) + .all(|(a, b)| a.name == b.name) { + span_lint_and_then(self.cx, USE_SELF, path.span, "unnecessary structure name repetition", |db| { db.span_suggestion(path.span, "use the applicable keyword", "Self".to_owned()); }); From 88e4044d6df24e8a4f53412a9db091aae1d8bbd5 Mon Sep 17 00:00:00 2001 From: Mason Remaley Date: Wed, 13 Dec 2017 00:13:45 -0500 Subject: [PATCH 3/3] wip change, trying to get the type of Self --- clippy_lints/src/use_self.rs | 74 ++++++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 8203377e465f..842d1130f4c2 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -1,9 +1,10 @@ use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc::hir::*; -use rustc::hir::intravisit::{walk_path, NestedVisitorMap, Visitor}; +use rustc::hir::intravisit::{walk_path, walk_expr, walk_body, walk_impl_item, NestedVisitorMap, Visitor}; use utils::{in_macro, span_lint_and_then}; use syntax::ast::NodeId; use syntax_pos::symbol::keywords::SelfType; +use syntax::ptr::P; /// **What it does:** Checks for unnecessary repetition of structure name when a /// replacement with `Self` is applicable. @@ -56,45 +57,76 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf { } if_chain! { if let ItemImpl(.., ref item_type, ref refs) = item.node; - if let Ty_::TyPath(QPath::Resolved(_, ref item_path)) = item_type.node; + if let Ty_::TyPath(QPath::Resolved(_, ref self_path)) = item_type.node; then { - let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).parameters; - let should_check = if let Some(ref params) = *parameters { - !params.parenthesized && params.lifetimes.len() == 0 - } else { - true - }; - if should_check { + let parameters = &self_path.segments.last().expect(SEGMENTS_MSG).parameters; let visitor = &mut UseSelfVisitor { - item_path: item_path, - cx: cx, + self_path, + cx, + body_id: None, + self_type: item_type, }; for impl_item_ref in refs { visitor.visit_impl_item(cx.tcx.hir.impl_item(impl_item_ref.id)); } - } } } } } struct UseSelfVisitor<'a, 'tcx: 'a> { - item_path: &'a Path, + self_type: &'a P, + self_path: &'a Path, cx: &'a LateContext<'a, 'tcx>, + body_id: Option, } impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> { - fn visit_path(&mut self, path: &'tcx Path, _id: NodeId) { - if self.item_path.def == path.def && path.segments.last().expect(SEGMENTS_MSG).name != SelfType.name() { - span_lint_and_then(self.cx, USE_SELF, path.span, "unnecessary structure name repetition", |db| { - db.span_suggestion(path.span, "use the applicable keyword", "Self".to_owned()); - }); - } + // fn visit_path(&mut self, path: &'tcx Path, _id: NodeId) { + // if self.self_path.def == path.def && path.segments.last().expect(SEGMENTS_MSG).name != SelfType.name() { + // span_lint_and_then(self.cx, USE_SELF, path.span, "unnecessary structure name repetition", |db| { + // db.span_suggestion(path.span, "use the applicable keyword", "Self".to_owned()); + // }); + // } - walk_path(self, path); - } + // walk_path(self, path); + // } + + // fn visit_impl_item(&mut self, impl_item: &'tcx ImplItem) { + // println!("{:?}", impl_item); + // walk_impl_item(self, impl_item); + // } fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { NestedVisitorMap::OnlyBodies(&self.cx.tcx.hir) } + + //TODO: This is just a hack to see if having the body id is useful + fn visit_body(&mut self, body: &'tcx Body) { + self.body_id = Some(body.id()); + walk_body(self, body); + } + + fn visit_expr(&mut self, expr: &'tcx Expr) { + if let Expr_::ExprStruct(ref path, _, _) = expr.node { + let segment = match *path { + QPath::Resolved(_, ref path) => path.segments + .last() + .expect(SEGMENTS_MSG), + QPath::TypeRelative(_, ref segment) => segment, + }; + + if let Some(body_id) = self.body_id { + if segment.name != SelfType.name() { + let ty = self.cx.tcx.body_tables(body_id).expr_ty(expr); + println!("Self `P`: {:?}", self.self_type); + println!("Self `Path`: {:?}", self.self_path); + println!("Struct Literal `TyS`: {:?}", ty); + println!(); + } + } + } + + walk_expr(self, expr); + } }