new_ret_no_self complains about new() -> impl Future<Self> if new() takes parameters

[Arnavion]

0.0.212 (2018-09-17 125907a)

Playground

#![feature(tool_lints)]

#![deny(clippy::new_ret_no_self)]

extern crate futures;
use futures::{Future, IntoFuture};

struct S;

impl S {
    pub fn new(_: String) -> impl Future<Item = Self, Error = ()>
    {
        Ok(S).into_future()
    }
}

fn main() {}

error: methods called `new` usually return `Self`
  --> src/main.rs:11:5
   |
11 | /     pub fn new(_: String) -> impl Future<Item = Self, Error = ()>
12 | |     {
13 | |         Ok(S).into_future()
14 | |     }
   | |_____^
   |
note: lint level defined here
  --> src/main.rs:3:9
   |
3  | #![deny(clippy::new_ret_no_self)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#new_ret_no_self

If S::new() does not have the String parameter, the lint does not complain.

[JoshMcguigan]

I've attempted to start solving this one and I ran into a couple issues. First, I could not find existing tests for new_ret_no_self, so I created a new test file. I was able to recreate this issue without using an external dependency at the playground below.

https://play.rust-lang.org/?gist=449e1c7a3455843bb7f032fa44a5c7a3&version=stable&mode=debug&edition=2015

I created a test based on this, and then modified this lint slightly, and the compiler crashes when running TESTNAME=ui/new_ret_no_self cargo test --test compile-test with the full test (I commented out the part that breaks it).

aea834f

The actual error message:

error: internal compiler error: librustc_typeck/collect.rs:1487: unexpected sort of node in fn_sig(): ImplItem(ImplItem { id: NodeId(15), ident: Item#0, hir_id: HirId { owner: DefIndex(0:7), local_id: ItemLocalId(0) }, vis: Spanned { node: Inherited, span: tests/ui/new_ret_no_self.rs:13:5: 13:5 }, defaultness: Final, attrs: [], generics: Generics { params: [], where_clause: WhereClause { id: NodeId(16), predicates: [] }, span: tests/ui/new_ret_no_self.rs:1:1: 1:1 }, node: Type(type(Self)), span: tests/ui/new_ret_no_self.rs:13:5: 13:22 })

thread 'main' panicked at 'Box<Any>', librustc_errors/lib.rs:599:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
error: aborting due to previous error


note: the compiler unexpectedly panicked. this is a bug.

Is this something I am doing, or is it possibly an actual compiler issue?

[Arnavion]

"The compiler unexpectedly panicked" happens if the compiler API panics. clippy lints plug into the compiler API. So yes, I would start by assuming it's from your change.

[mati865]

@JoshMcguigan run the test with RUST_BACKTRACE=1 and check the trace. If you need more help you can create WIP PR.

[JoshMcguigan]

Thanks for the help. The relevant part of the trace:

  22: clippy_lints::utils::return_ty
             at clippy_lints/src/utils/mod.rs:747

It was caused by calling cx.tcx.fn_sig(fn_def_id).output() where fn_def_id was the id of something other than a function.

At this point I've fixed the false negative when new doesn't take any arguments, but I'm still working on detecting if the return type is a trait with an associated type of Self. I'll keep going and report back if I have any issues.

[JoshMcguigan]

I opened a PR (#3253) which resolves this issue as well as another issue with this lint, but I'm not sure I did it in the best possible way. I'd be very open to any feedback.