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

odd new_ret_no_self warning: return type that wraps Self #3313

Closed
matthiaskrgr opened this Issue Oct 13, 2018 · 7 comments

Comments

Projects
None yet
6 participants
@matthiaskrgr
Collaborator

matthiaskrgr commented Oct 13, 2018

clippy @ 601cc9d

This code comes from rls

impl ConcurrentJob {
    pub fn new() -> (ConcurrentJob, JobToken) {
        let (tx, rx) = bounded(0);
        let job = ConcurrentJob { chan: rx };
        let token = JobToken { _chan: tx };
        (job, token)
    }

    fn is_completed(&self) -> bool {
        is_closed(&self.chan)
    }
}

It causes the following clippy warning:

warning: methods called `new` usually return `Self`
  --> src/concurrency.rs:62:5
   |
62 | /     pub fn new() -> (ConcurrentJob, JobToken) {
63 | |         let (tx, rx) = bounded(0);
64 | |         let job = ConcurrentJob { chan: rx };
65 | |         let token = JobToken { _chan: tx };
66 | |         (job, token)
67 | |     }
   | |_____^
   |
note: lint level defined here
  --> src/main.rs:21:9
   |
21 | #![warn(clippy::all, rust_2018_idioms)]
   |         ^^^^^^^^^^^
   = note: #[warn(clippy::new_ret_no_self)] implied by #[warn(clippy::all)]
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#new_ret_no_self

I tried fixing this by making both ConcurrentJobs be Self however, ciippy kept warning, probably because it does not understand that the constructor actually returns a tuple of Self and something else?

impl ConcurrentJob {
    pub fn new() -> (Self, JobToken) {
        let (tx, rx) = bounded(0);
        let job = Self { chan: rx };
        let token = JobToken { _chan: tx };
        (job, token)
    }

    fn is_completed(&self) -> bool {
        is_closed(&self.chan)
    }
}

=>

warning: methods called `new` usually return `Self`
  --> src/concurrency.rs:62:5
   |
62 | /     pub fn new() -> (Self, JobToken) {
63 | |         let (tx, rx) = bounded(0);
64 | |         let job = Self { chan: rx };
65 | |         let token = JobToken { _chan: tx };
66 | |         (job, token)
67 | |     }
   | |_____^
   |
note: lint level defined here
  --> src/main.rs:21:9
   |
21 | #![warn(clippy::all, rust_2018_idioms)]
   |         ^^^^^^^^^^^
   = note: #[warn(clippy::new_ret_no_self)] implied by #[warn(clippy::all)]
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#new_ret_no_self
@matthiaskrgr

This comment has been minimized.

Collaborator

matthiaskrgr commented Oct 15, 2018

Found another one in my own code

error: methods called `new` usually return `Self`
  --> src/library.rs:48:5
   |
48 | /     pub(crate) fn new() -> Result<Self, (ErrorKind, String)> {
49 | |         let cargo_cfg = match cargo::util::config::Config::default() {
50 | |             Ok(cargo_cfg) => cargo_cfg,
51 | |             _ => {
...  |
87 | |         })
88 | |     }
   | |_____^
   |
   = note: `-D clippy::new-ret-no-self` implied by `-D warnings`
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#new_ret_no_self

I consider this a FP.

@flip1995 flip1995 added the L-bug 🐞 label Oct 15, 2018

@flip1995

This comment has been minimized.

@JoshMcguigan

This comment has been minimized.

Contributor

JoshMcguigan commented Oct 16, 2018

Where does clippy look to determine what is considered standard behavior? Does the Rust standard library have any examples of a new method returning (self, other), Result<Self, Err>, or Option<Self>?

I'm not trying to express an opinion one way or another here, I'd just like to understand how it is determined whether something like this should be considered a false positive or not?

@programmerjake

This comment has been minimized.

programmerjake commented Oct 16, 2018

NonNull::new returns Option<Self>

@oli-obk

This comment has been minimized.

Collaborator

oli-obk commented Oct 16, 2018

Where does clippy look to determine what is considered standard behavior?

Mostly the libstd. There's no established process except for judgement calls though.

We could walk the return type and if it contains Self anywhere accept that.

Or if we want to be more strict, we only accept Result<Self, T>, Option<T> and tuples containing one element of type T

@cramertj

This comment has been minimized.

Member

cramertj commented Oct 16, 2018

This also resulted in many errors in futures on new functions that return *mut Self, Result<Self, E>, (Self, Someother), etc. +1 for "walk return type and see if it contains Self anywhere". (cc rust-lang-nursery/futures-rs#1291)

matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this issue Oct 18, 2018

matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this issue Oct 18, 2018

flip1995 added a commit that referenced this issue Oct 18, 2018

Merge pull request #3334 from matthiaskrgr/new-ret-no-self__doc
new_ret_no_self: add sample from #3313 to Known Problems section.

@matthiaskrgr matthiaskrgr changed the title from odd new_ret_no_self warning to odd new_ret_no_self warning: return type that wraps Self Oct 19, 2018

@JoshMcguigan

This comment has been minimized.

Contributor

JoshMcguigan commented Oct 19, 2018

I've started working on this.

bors bot added a commit that referenced this issue Oct 24, 2018

Merge #3338
3338: new_ret_no_self false positives r=flip1995 a=JoshMcguigan

~~WORK IN PROGRESS~~

I plan to fix all of the false positives in #3313 in this PR, but I wanted to open it now to start gathering feedback.

In this first commit, I've updated the lint to allow tuple return types as long as `Self` shows up at least once, in any position of the tuple. I believe this is the broadest possible interpretation of what should be allowed for tuple return types, but I would certainly be okay making the lint more strict. 

fixes #3313 

Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>

bors bot added a commit that referenced this issue Oct 24, 2018

Merge #3338
3338: new_ret_no_self false positives r=flip1995 a=JoshMcguigan

~~WORK IN PROGRESS~~

I plan to fix all of the false positives in #3313 in this PR, but I wanted to open it now to start gathering feedback.

In this first commit, I've updated the lint to allow tuple return types as long as `Self` shows up at least once, in any position of the tuple. I believe this is the broadest possible interpretation of what should be allowed for tuple return types, but I would certainly be okay making the lint more strict. 

fixes #3313 

Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>

@bors bors bot closed this in #3338 Oct 24, 2018

matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this issue Oct 25, 2018

Revert "new_ret_no_self: add sample from rust-lang#3313 to Known Prob…
…lems section."

This reverts commit fd2f6dd.

Issue rust-lang#3313 has been fixed.

bors bot added a commit that referenced this issue Oct 25, 2018

Merge #3358
3358: Revert "new_ret_no_self: add sample from #3313 to Known Problems section." r=oli-obk a=matthiaskrgr

This reverts commit fd2f6dd.

Issue #3313 has been fixed.

Co-authored-by: Matthias Krüger <matthias.krueger@famsik.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment