From 6e75050be07d5f457c8854e9392fd756ef211a06 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 19 Oct 2018 04:55:06 -0700 Subject: [PATCH 1/5] new_ret_no_self correct linting of tuple return types --- clippy_lints/src/methods/mod.rs | 11 +++++++++++ tests/ui/new_ret_no_self.rs | 28 ++++++++++++++++++++++++++++ tests/ui/new_ret_no_self.stderr | 8 +++++++- 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6d22abf2d339..6e0154875619 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -936,6 +936,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let hir::ImplItemKind::Method(_, _) = implitem.node { let ret_ty = return_ty(cx, implitem.id); +// println!("ret_ty: {:?}", ret_ty); +// println!("ret_ty.sty {:?}", ret_ty.sty); + // if return type is impl trait if let TyKind::Opaque(def_id, _) = ret_ty.sty { @@ -955,6 +958,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } + // if return type is tuple + if let TyKind::Tuple(list) = ret_ty.sty { + // then at least one of the types in the tuple must be Self + for ret_type in list { + if same_tys(cx, ty, ret_type) { return; } + } + } + if name == "new" && !same_tys(cx, ret_ty, ty) { span_lint(cx, NEW_RET_NO_SELF, diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 1a4b91cc9da9..777311496780 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -91,3 +91,31 @@ impl V { unimplemented!(); } } + +struct TupleReturnerOk; + +impl TupleReturnerOk { + // should not trigger lint + pub fn new() -> (Self, u32) { unimplemented!(); } +} + +struct TupleReturnerOk2; + +impl TupleReturnerOk2 { + // should not trigger lint (it doesn't matter which element in the tuple is Self) + pub fn new() -> (u32, Self) { unimplemented!(); } +} + +struct TupleReturnerOk3; + +impl TupleReturnerOk3 { + // should not trigger lint (tuple can contain multiple Self) + pub fn new() -> (Self, Self) { unimplemented!(); } +} + +struct TupleReturnerBad; + +impl TupleReturnerBad { + // should trigger lint + pub fn new() -> (u32, u32) { unimplemented!(); } +} diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index ad26438d4efe..6f8e2d136a78 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -24,5 +24,11 @@ error: methods called `new` usually return `Self` 92 | | } | |_____^ -error: aborting due to 3 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:120:5 + | +120 | pub fn new() -> (u32, u32) { unimplemented!(); } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 4 previous errors From 097df8f223e8a3d10ea8bd66ecd94ead376c4416 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 19 Oct 2018 05:20:33 -0700 Subject: [PATCH 2/5] new_ret_no_self correct false positive on raw pointer return types --- clippy_lints/src/methods/mod.rs | 6 ++++++ tests/ui/new_ret_no_self.rs | 21 +++++++++++++++++++++ tests/ui/new_ret_no_self.stderr | 8 +++++++- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6e0154875619..0992067636ed 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -966,6 +966,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } + // if return type is mutable pointer + if let TyKind::RawPtr(ty::TypeAndMut{ty: ret_type, ..}) = ret_ty.sty { + // then the pointer must point to Self + if same_tys(cx, ty, ret_type) { return; } + } + if name == "new" && !same_tys(cx, ret_ty, ty) { span_lint(cx, NEW_RET_NO_SELF, diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 777311496780..b267a3aecdf0 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -119,3 +119,24 @@ impl TupleReturnerBad { // should trigger lint pub fn new() -> (u32, u32) { unimplemented!(); } } + +struct MutPointerReturnerOk; + +impl MutPointerReturnerOk { + // should not trigger lint + pub fn new() -> *mut Self { unimplemented!(); } +} + +struct MutPointerReturnerOk2; + +impl MutPointerReturnerOk2 { + // should not trigger lint + pub fn new() -> *const Self { unimplemented!(); } +} + +struct MutPointerReturnerBad; + +impl MutPointerReturnerBad { + // should trigger lint + pub fn new() -> *mut V { unimplemented!(); } +} diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index 6f8e2d136a78..20f0dbbe8a39 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -30,5 +30,11 @@ error: methods called `new` usually return `Self` 120 | pub fn new() -> (u32, u32) { unimplemented!(); } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 4 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:141:5 + | +141 | pub fn new() -> *mut V { unimplemented!(); } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors From 079f9f45b5a44a0feaf60d56576758dd7b0c9fdd Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 19 Oct 2018 17:54:25 -0700 Subject: [PATCH 3/5] new_ret_no_self walk return type to check for self --- clippy_lints/src/methods/mod.rs | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 0992067636ed..f0810c906eff 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -936,13 +936,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let hir::ImplItemKind::Method(_, _) = implitem.node { let ret_ty = return_ty(cx, implitem.id); -// println!("ret_ty: {:?}", ret_ty); -// println!("ret_ty.sty {:?}", ret_ty.sty); + // walk the return type and check for Self (this does not check associated types) + for inner_type in ret_ty.walk() { + if same_tys(cx, ty, inner_type) { return; } + } - // if return type is impl trait + // if return type is impl trait, check the associated types if let TyKind::Opaque(def_id, _) = ret_ty.sty { - // then one of the associated types must be Self + // 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), _) => { @@ -958,20 +960,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } - // if return type is tuple - if let TyKind::Tuple(list) = ret_ty.sty { - // then at least one of the types in the tuple must be Self - for ret_type in list { - if same_tys(cx, ty, ret_type) { return; } - } - } - - // if return type is mutable pointer - if let TyKind::RawPtr(ty::TypeAndMut{ty: ret_type, ..}) = ret_ty.sty { - // then the pointer must point to Self - if same_tys(cx, ty, ret_type) { return; } - } - if name == "new" && !same_tys(cx, ret_ty, ty) { span_lint(cx, NEW_RET_NO_SELF, From a6245835573760d7e2dfd6a608af40fab7ba5223 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 20 Oct 2018 06:29:17 -0700 Subject: [PATCH 4/5] new_ret_no_self added test cases --- clippy_lints/src/types.rs | 1 - tests/ui/new_ret_no_self.rs | 21 +++++++++++++++++++++ tests/ui/new_ret_no_self.stderr | 8 +++++++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 59c551682329..035ca2b04964 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1920,7 +1920,6 @@ 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()? diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index b267a3aecdf0..b7daf3d49bcb 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -140,3 +140,24 @@ impl MutPointerReturnerBad { // should trigger lint pub fn new() -> *mut V { unimplemented!(); } } + +struct GenericReturnerOk; + +impl GenericReturnerOk { + // should not trigger lint + pub fn new() -> Option { unimplemented!(); } +} + +struct GenericReturnerBad; + +impl GenericReturnerBad { + // should trigger lint + pub fn new() -> Option { unimplemented!(); } +} + +struct NestedReturnerOk; + +impl NestedReturnerOk { + // should trigger lint + pub fn new() -> (Option, u32) { unimplemented!(); } +} diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index 20f0dbbe8a39..bab9627ca225 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -36,5 +36,11 @@ error: methods called `new` usually return `Self` 141 | pub fn new() -> *mut V { unimplemented!(); } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:155:5 + | +155 | pub fn new() -> Option { unimplemented!(); } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 6 previous errors From 30ffc17ef754f6b6c7bd47809afd51b1c5632f22 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 24 Oct 2018 06:43:21 -0700 Subject: [PATCH 5/5] new_ret_no_self added test cases --- tests/ui/new_ret_no_self.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index b7daf3d49bcb..bed43f550f21 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -158,6 +158,20 @@ impl GenericReturnerBad { struct NestedReturnerOk; impl NestedReturnerOk { - // should trigger lint + // should not trigger lint pub fn new() -> (Option, u32) { unimplemented!(); } } + +struct NestedReturnerOk2; + +impl NestedReturnerOk2 { + // should not trigger lint + pub fn new() -> ((Self, u32), u32) { unimplemented!(); } +} + +struct NestedReturnerOk3; + +impl NestedReturnerOk3 { + // should not trigger lint + pub fn new() -> Option<(Self, u32)> { unimplemented!(); } +}