Rust-clippy: odd new_ret_no_self warning: return type that wraps Self

[matthiaskrgr]

clippy @ 601cc9d2c541f0a654b14a9c1fcc1d0207d8d33a

This code comes from rls
````rust
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]

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]

We also have this FP in Clippy: https://github.com/rust-lang-nursery/rust-clippy/pull/3253#issuecomment-427958037
https://github.com/rust-lang-nursery/rust-clippy/blob/d47dbf598adc4a6407ea34b44399907cb068d7a9/clippy_lints/src/types.rs#L1922

[JoshMcguigan]

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]

NonNull::new returns Option<Self>

[oli-obk]

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 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 https://github.com/rust-lang-nursery/futures-rs/pull/1291)

[JoshMcguigan]

I've started working on this.