new_ret_no_self

[JoshMcguigan]

Fixes #3220

This adds test coverage for the new_ret_no_self lint, corrects the false positive reported in #3220, and corrects a false negative when the new function takes no arguments and doesn't return self.

However, to correct the false positive from #3220, I am simply disabling the lint if the return type is impl trait. This causes a different false negative as shown in the following case.

// should trigger the lint, but currently does not
pub fn new(_: String) -> impl R<Item = u32> {
    S3
}

I was not able to figure out how to inspect the associated types of the returned impl trait, but if that is possible then we could check for the type of self (S3 in the example above) in the associated types and correct this false negative as well.

[oli-obk]

You can try debug printing ret_ty.sty, for this case it should be TyKind::Opaque. If it is not, then the return_ty function seems to alread reveal impl trait types. If that is the case, we'll need to figure out a way to get the unadjusted signature inside the return_ty function

[JoshMcguigan]

Adding println!("type: {:?}", ret_ty.sty); to the lint method results in the following being added to stdout when running TESTNAME=ui/new_ret_no_self cargo test --test compile-test.

type: Opaque(DefId(0/1:10 ~ new_ret_no_self[317d]::{{impl}}[1]::new[0]::{{impl-Trait}}[0]), [])
type: Opaque(DefId(0/1:12 ~ new_ret_no_self[317d]::{{impl}}[3]::new[0]::{{impl-Trait}}[0]), [])
type: Opaque(DefId(0/1:14 ~ new_ret_no_self[317d]::{{impl}}[5]::new[0]::{{impl-Trait}}[0]), [])
type: Adt(T, [])
type: Uint(u32)
type: Uint(u32)

Looking at the docs for TyKind it looks like Opaque means impl Trait as you said. Is there a way to get the associated types for a given impl Trait? I've tried the following, but the compiler crashes on associated_item_def_ids with the error below.

if let TyKind::Opaque(def_id, _) = ret_ty.sty {
    println!("type: {}", ret_ty.sty);
    println!("def_id: {:?}", def_id);
    println!("{:?}", cx.tcx.associated_item_def_ids(def_id));
}

error: internal compiler error: librustc/ty/mod.rs:2951: associated_item_def_ids: not impl or trait
  --> tests/ui/new_ret_no_self.rs:20:21
   |
20 |     pub fn new() -> impl R<Item = Self> {
   |                     ^^^^^^^^^^^^^^^^^^^

thread 'main' panicked at 'Box<Any>', librustc_errors/lib.rs:537:9

[oli-obk]

I presume you mean the concrete type that the impl trait hides. You get it by calling cx.tcx.type_of(def_id).

Using the concrete type will mess up your check in https://github.com/rust-lang-nursery/rust-clippy/pull/3253/files/3da7c1013cc9c62357261e786958c479bc5c3e09#diff-76448e132e544f8a1ff89ffa05ef9566R917 though and thus create new false positives I presume

[JoshMcguigan]

cx.tcx.type_of(def_id) returns S, S2, and S3 as you said. I am looking for something that would return impl R<Item = Self> or impl R<Item = u32>.

Does that exist?

edit - It looks like predicates_of does this.

[JoshMcguigan]

I think I got it this time. All previous tests pass, and it now correctly triggers the lint for pub fn new(_: String) -> impl R<Item = u32> and it does not trigger for pub fn new(_: String) -> impl Q<Item = u32, Item2 = Self>.

That said, I am way out of my depth here and basically got this working by poking around the rustc API until I found something that seemed to work. Please let me know if there is anything else that could be improved.

[phansch]

It looks like Clippy contains a case where new returns Option<Self> and our dogfood test fails on travis:

https://github.com/rust-lang-nursery/rust-clippy/blob/d47dbf598adc4a6407ea34b44399907cb068d7a9/clippy_lints/src/types.rs#L1922

I'm not really sure what this new method does, so I guess the easiest way would be to just add an allow for this one method.

[JoshMcguigan]

Hopefully that takes care of it. I did a rebase as well.

[JoshMcguigan]

Rebased again. Let me know if you see any other issues with this.

[oli-obk]

Thanks! Lgtm now