Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new_ret_no_self false positives #3338

Merged
merged 5 commits into from
Oct 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -936,10 +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);

// if return type is impl trait
// 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, 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), _) => {
Expand Down
1 change: 0 additions & 1 deletion clippy_lints/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Self> {
if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.node {
let params: Vec<_> = path.segments.last().as_ref()?.args.as_ref()?
Expand Down
84 changes: 84 additions & 0 deletions tests/ui/new_ret_no_self.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,87 @@ impl V {
unimplemented!();
}
}

struct TupleReturnerOk;

impl TupleReturnerOk {
// should not trigger lint
pub fn new() -> (Self, u32) { unimplemented!(); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

((Self, u32), u32) Will also trigger the lint. I'm not sure at all how strict we should be. I mean it seems perfectly reasonable to allow (Option<Self>, u32) or at least Option<(Self, u32)>. This might get out of hand quickly.

If you want to cover a lot of cases, you can call walk on the return type (giving you an iterator) and iterate over everything.

That would still cause false positives for struct Foo(pub Bar); impl Bar { fn new() -> Foo { ... } } but we need to restrict ourselves somewhere I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the tip, that sounds like a much more manageable approach. I pushed the commit below just to capture my progress, but I'll go back and try rewriting this to use walk.

}

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!(); }
}

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!(); }
}

struct GenericReturnerOk;

impl GenericReturnerOk {
// should not trigger lint
pub fn new() -> Option<Self> { unimplemented!(); }
}

struct GenericReturnerBad;

impl GenericReturnerBad {
// should trigger lint
pub fn new() -> Option<u32> { unimplemented!(); }
}

struct NestedReturnerOk;

impl NestedReturnerOk {
// should not trigger lint
pub fn new() -> (Option<Self>, 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!(); }
}
20 changes: 19 additions & 1 deletion tests/ui/new_ret_no_self.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,23 @@ 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: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:141:5
|
141 | pub fn new() -> *mut V { unimplemented!(); }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:155:5
|
155 | pub fn new() -> Option<u32> { unimplemented!(); }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 6 previous errors