From eb854b233c353441f86c4a346a941c5965a2333a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 2 Oct 2018 20:11:56 -0700 Subject: [PATCH 1/8] new_ret_no_self added positive test cases --- clippy_lints/src/methods/mod.rs | 22 ++++++------ tests/ui/new_ret_no_self.rs | 63 +++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 11 deletions(-) create mode 100644 tests/ui/new_ret_no_self.rs diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7c15eb677cc..d11dbf0e773 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -878,6 +878,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { let name = implitem.ident.name; let parent = cx.tcx.hir.get_parent(implitem.id); let item = cx.tcx.hir.expect_item(parent); + let def_id = cx.tcx.hir.local_def_id(item.id); + let ty = cx.tcx.type_of(def_id); if_chain! { if let hir::ImplItemKind::Method(ref sig, id) = implitem.node; if let Some(first_arg_ty) = sig.decl.inputs.get(0); @@ -899,8 +901,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } // check conventions w.r.t. conversion method names and predicates - let def_id = cx.tcx.hir.local_def_id(item.id); - let ty = cx.tcx.type_of(def_id); let is_copy = is_copy(cx, ty); for &(ref conv, self_kinds) in &CONVENTIONS { if conv.check(&name.as_str()) { @@ -928,17 +928,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { break; } } - - let ret_ty = return_ty(cx, implitem.id); - if name == "new" && - !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { - span_lint(cx, - NEW_RET_NO_SELF, - implitem.span, - "methods called `new` usually return `Self`"); - } } } + + let ret_ty = return_ty(cx, implitem.id); + if name == "new" && + !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { + span_lint(cx, + NEW_RET_NO_SELF, + implitem.span, + "methods called `new` usually return `Self`"); + } } } diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs new file mode 100644 index 00000000000..67933f00262 --- /dev/null +++ b/tests/ui/new_ret_no_self.rs @@ -0,0 +1,63 @@ +#![feature(tool_lints)] + +#![warn(clippy::new_ret_no_self)] +#![allow(dead_code, clippy::trivially_copy_pass_by_ref)] + +fn main(){} + +//trait R { +// type Item; +//} +// +//struct S; +// +//impl R for S { +// type Item = Self; +//} +// +//impl S { +// // should not trigger the lint +// pub fn new() -> impl R { +// S +// } +//} +// +//struct S2; +// +//impl R for S2 { +// type Item = Self; +//} +// +//impl S2 { +// // should not trigger the lint +// pub fn new(_: String) -> impl R { +// S2 +// } +//} +// +//struct T; +// +//impl T { +// // should not trigger lint +// pub fn new() -> Self { +// unimplemented!(); +// } +//} + +struct U; + +impl U { + // should trigger lint + pub fn new() -> u32 { + unimplemented!(); + } +} + +struct V; + +impl V { + // should trigger lint + pub fn new(_: String) -> u32 { + unimplemented!(); + } +} From 13ce96c4bfd236ec49bcd7b63f42f9a51b0ee599 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 3 Oct 2018 03:55:31 -0700 Subject: [PATCH 2/8] new_ret_no_self corrected panic and added test stderr --- clippy_lints/src/methods/mod.rs | 16 ++++--- tests/ui/new_ret_no_self.rs | 76 ++++++++++++++++----------------- tests/ui/new_ret_no_self.stderr | 18 ++++++++ 3 files changed, 65 insertions(+), 45 deletions(-) create mode 100644 tests/ui/new_ret_no_self.stderr diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index d11dbf0e773..81cb1cd1182 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -931,13 +931,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } - let ret_ty = return_ty(cx, implitem.id); - if name == "new" && - !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { - span_lint(cx, - NEW_RET_NO_SELF, - implitem.span, - "methods called `new` usually return `Self`"); + if let hir::ImplItemKind::Method(ref sig, id) = implitem.node { + let ret_ty = return_ty(cx, implitem.id); + if name == "new" && + !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { + span_lint(cx, + NEW_RET_NO_SELF, + implitem.span, + "methods called `new` usually return `Self`"); + } } } } diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 67933f00262..762dd582168 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -5,44 +5,44 @@ fn main(){} -//trait R { -// type Item; -//} -// -//struct S; -// -//impl R for S { -// type Item = Self; -//} -// -//impl S { -// // should not trigger the lint -// pub fn new() -> impl R { -// S -// } -//} -// -//struct S2; -// -//impl R for S2 { -// type Item = Self; -//} -// -//impl S2 { -// // should not trigger the lint -// pub fn new(_: String) -> impl R { -// S2 -// } -//} -// -//struct T; -// -//impl T { -// // should not trigger lint -// pub fn new() -> Self { -// unimplemented!(); -// } -//} +trait R { + type Item; +} + +struct S; + +impl R for S { + type Item = Self; +} + +impl S { + // should not trigger the lint + pub fn new() -> impl R { + S + } +} + +struct S2; + +impl R for S2 { + type Item = Self; +} + +impl S2 { + // should not trigger the lint + pub fn new(_: String) -> impl R { + S2 + } +} + +struct T; + +impl T { + // should not trigger lint + pub fn new() -> Self { + unimplemented!(); + } +} struct U; diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr new file mode 100644 index 00000000000..1d698892449 --- /dev/null +++ b/tests/ui/new_ret_no_self.stderr @@ -0,0 +1,18 @@ +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:51:5 + | +51 | / pub fn new() -> u32 { +52 | | unimplemented!(); +53 | | } + | |_____^ + +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:60:5 + | +60 | / pub fn new(_: String) -> u32 { +61 | | unimplemented!(); +62 | | } + | |_____^ + +error: aborting due to 2 previous errors + From 1c4fa419f33211db3fa60f3bc8d59a8f42992558 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 3 Oct 2018 04:59:14 -0700 Subject: [PATCH 3/8] new_ret_no_self fix false positive for impl trait return with associated type self --- clippy_lints/src/methods/mod.rs | 3 ++- tests/ui/new_ret_no_self.rs | 13 +++++++++++++ tests/ui/new_ret_no_self.stderr | 18 ++++++++++-------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 81cb1cd1182..7426ece5ba9 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -934,7 +934,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let hir::ImplItemKind::Method(ref sig, id) = implitem.node { let ret_ty = return_ty(cx, implitem.id); if name == "new" && - !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { + !same_tys(cx, ret_ty, ty) && + !ret_ty.is_impl_trait() { span_lint(cx, NEW_RET_NO_SELF, implitem.span, diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 762dd582168..3b7ff7780ef 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -35,6 +35,19 @@ impl S2 { } } +struct S3; + +impl R for S3 { + type Item = u32; +} + +impl S3 { + // should trigger the lint, but currently does not + pub fn new(_: String) -> impl R { + S3 + } +} + struct T; impl T { diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index 1d698892449..cab5fa55cb6 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -1,17 +1,19 @@ error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:51:5 + --> $DIR/new_ret_no_self.rs:64:5 | -51 | / pub fn new() -> u32 { -52 | | unimplemented!(); -53 | | } +64 | / pub fn new() -> u32 { +65 | | unimplemented!(); +66 | | } | |_____^ + | + = note: `-D clippy::new-ret-no-self` implied by `-D warnings` error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:60:5 + --> $DIR/new_ret_no_self.rs:73:5 | -60 | / pub fn new(_: String) -> u32 { -61 | | unimplemented!(); -62 | | } +73 | / pub fn new(_: String) -> u32 { +74 | | unimplemented!(); +75 | | } | |_____^ error: aborting due to 2 previous errors From 2ef4af7db23c5522db2d71b60908b93127df5036 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 3 Oct 2018 05:00:43 -0700 Subject: [PATCH 4/8] Removed unused variables --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7426ece5ba9..c78bb48db2a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -931,7 +931,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } - if let hir::ImplItemKind::Method(ref sig, id) = implitem.node { + if let hir::ImplItemKind::Method(_, _) = implitem.node { let ret_ty = return_ty(cx, implitem.id); if name == "new" && !same_tys(cx, ret_ty, ty) && From a5e4805ecf1ff5b38f0d467ba90530e43bfd0d9c Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 4 Oct 2018 19:01:04 -0700 Subject: [PATCH 5/8] new_ret_no_self correctly lint impl return --- clippy_lints/src/methods/mod.rs | 26 ++++++++++++++++++++++---- tests/ui/new_ret_no_self.rs | 21 ++++++++++++++++++++- tests/ui/new_ret_no_self.stderr | 26 +++++++++++++++++--------- 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c78bb48db2a..f9c010beea7 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -11,7 +11,7 @@ use crate::rustc::hir; use crate::rustc::hir::def::Def; use crate::rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass}; -use crate::rustc::ty::{self, Ty}; +use crate::rustc::ty::{self, Ty, TyKind, Predicate}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc_errors::Applicability; use crate::syntax::ast; @@ -933,9 +933,27 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let hir::ImplItemKind::Method(_, _) = implitem.node { let ret_ty = return_ty(cx, implitem.id); - if name == "new" && - !same_tys(cx, ret_ty, ty) && - !ret_ty.is_impl_trait() { + + // if return type is impl trait + if let TyKind::Opaque(def_id, _) = ret_ty.sty { + + // then one of the associated types must be Self + for predicate in cx.tcx.predicates_of(def_id).predicates.iter() { + match predicate { + (Predicate::Projection(poly_projection_predicate), _) => { + let binder = poly_projection_predicate.ty(); + let associated_type = binder.skip_binder(); + let associated_type_is_self_type = same_tys(cx, ty, associated_type); + + // if the associated type is self, early return and do not trigger lint + if associated_type_is_self_type { return; } + }, + (_, _) => {}, + } + } + } + + if name == "new" && !same_tys(cx, ret_ty, ty) { span_lint(cx, NEW_RET_NO_SELF, implitem.span, diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 3b7ff7780ef..e9f41d34133 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -9,6 +9,11 @@ trait R { type Item; } +trait Q { + type Item; + type Item2; +} + struct S; impl R for S { @@ -42,12 +47,26 @@ impl R for S3 { } impl S3 { - // should trigger the lint, but currently does not + // should trigger the lint pub fn new(_: String) -> impl R { S3 } } +struct S4; + +impl Q for S4 { + type Item = u32; + type Item2 = Self; +} + +impl S4 { + // should not trigger the lint + pub fn new(_: String) -> impl Q { + S4 + } +} + struct T; impl T { diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index cab5fa55cb6..aa3a633c418 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -1,20 +1,28 @@ error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:64:5 + --> $DIR/new_ret_no_self.rs:51:5 | -64 | / pub fn new() -> u32 { -65 | | unimplemented!(); -66 | | } +51 | / pub fn new(_: String) -> impl R { +52 | | S3 +53 | | } | |_____^ | = note: `-D clippy::new-ret-no-self` implied by `-D warnings` error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:73:5 + --> $DIR/new_ret_no_self.rs:83:5 | -73 | / pub fn new(_: String) -> u32 { -74 | | unimplemented!(); -75 | | } +83 | / pub fn new() -> u32 { +84 | | unimplemented!(); +85 | | } | |_____^ -error: aborting due to 2 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:92:5 + | +92 | / pub fn new(_: String) -> u32 { +93 | | unimplemented!(); +94 | | } + | |_____^ + +error: aborting due to 3 previous errors From 348d18ebd8ee5182d4705aba8341fb469f936ff5 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 4 Oct 2018 21:37:28 -0700 Subject: [PATCH 6/8] Removed new_ret_no_self tests from method.rs --- tests/ui/methods.rs | 4 ++-- tests/ui/methods.stderr | 12 ++---------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 883dbf589d7..ae1b1642be7 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -14,7 +14,7 @@ #![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)] #![allow(clippy::blacklisted_name, unused, clippy::print_stdout, clippy::non_ascii_literal, clippy::new_without_default, clippy::new_without_default_derive, clippy::missing_docs_in_private_items, clippy::needless_pass_by_value, - clippy::default_trait_access, clippy::use_self, clippy::useless_format)] + clippy::default_trait_access, clippy::use_self, clippy::new_ret_no_self, clippy::useless_format)] use std::collections::BTreeMap; use std::collections::HashMap; @@ -43,7 +43,7 @@ impl T { fn to_something(self) -> u32 { 0 } - fn new(self) {} + fn new(self) -> Self { unimplemented!(); } } struct Lt<'a> { diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 124edee6a52..307814824ea 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -23,17 +23,9 @@ error: methods called `to_*` usually take self by reference; consider choosing a error: methods called `new` usually take no self; consider choosing a less ambiguous name --> $DIR/methods.rs:46:12 | -46 | fn new(self) {} +46 | fn new(self) -> Self { unimplemented!(); } | ^^^^ -error: methods called `new` usually return `Self` - --> $DIR/methods.rs:46:5 - | -46 | fn new(self) {} - | ^^^^^^^^^^^^^^^ - | - = note: `-D clippy::new-ret-no-self` implied by `-D warnings` - error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead --> $DIR/methods.rs:114:13 | @@ -465,5 +457,5 @@ error: used unwrap() on an Option value. If you don't want to handle the None ca | = note: `-D clippy::option-unwrap-used` implied by `-D warnings` -error: aborting due to 58 previous errors +error: aborting due to 57 previous errors From 54506705cec65652c0607cfe8af7284546e9b576 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Mon, 8 Oct 2018 19:35:37 -0700 Subject: [PATCH 7/8] Added new_ret_no_self exception to clippy to pass dogfood tests --- clippy_lints/src/types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 035ca2b0496..59c55168232 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1920,6 +1920,7 @@ enum ImplicitHasherType<'tcx> { impl<'tcx> ImplicitHasherType<'tcx> { /// Checks that `ty` is a target type without a BuildHasher. + #[allow(clippy::new_ret_no_self)] fn new<'a>(cx: &LateContext<'a, 'tcx>, hir_ty: &hir::Ty) -> Option { if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.node { let params: Vec<_> = path.segments.last().as_ref()?.args.as_ref()? From 3f386d33f92c4bf439043cf2866f44fc0ee5b27c Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 13 Oct 2018 06:33:46 -0700 Subject: [PATCH 8/8] new_ret_no_self test remove tool lints cfg flag --- tests/ui/new_ret_no_self.rs | 2 -- tests/ui/new_ret_no_self.stderr | 24 ++++++++++++------------ 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index e9f41d34133..1a4b91cc9da 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -1,5 +1,3 @@ -#![feature(tool_lints)] - #![warn(clippy::new_ret_no_self)] #![allow(dead_code, clippy::trivially_copy_pass_by_ref)] diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index aa3a633c418..ad26438d4ef 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -1,27 +1,27 @@ error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:51:5 + --> $DIR/new_ret_no_self.rs:49:5 | -51 | / pub fn new(_: String) -> impl R { -52 | | S3 -53 | | } +49 | / pub fn new(_: String) -> impl R { +50 | | S3 +51 | | } | |_____^ | = note: `-D clippy::new-ret-no-self` implied by `-D warnings` error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:83:5 + --> $DIR/new_ret_no_self.rs:81:5 | -83 | / pub fn new() -> u32 { -84 | | unimplemented!(); -85 | | } +81 | / pub fn new() -> u32 { +82 | | unimplemented!(); +83 | | } | |_____^ error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:92:5 + --> $DIR/new_ret_no_self.rs:90:5 | -92 | / pub fn new(_: String) -> u32 { -93 | | unimplemented!(); -94 | | } +90 | / pub fn new(_: String) -> u32 { +91 | | unimplemented!(); +92 | | } | |_____^ error: aborting due to 3 previous errors