False positive for needless_lifetimes

[fhartwig]

While working on https://github.com/Manishearth/rust-clippy/pull/396 I ran into a false positive for the needless_lifetimes lint. A small example:

pub struct Foo<'a> {
    r: &'a str
}

fn do_foo<'a>(foo: &'a Foo) -> &'a str { // compiles, but clippy warns
//fn do_foo(foo: &Foo) -> &str { // doesn't compile (missing lifetime specifier)
    foo.r
}

fn main() { do_foo(&Foo {r: "hello" }); }

When running clippy on this code, it gives

src/main.rs:5:1: 8:2 warning: explicit lifetimes given in parameter types where they could be elided, #[warn(needless_lifetimes)] on by default

but after removing the explicit lifetimes, rustc fails with the following error:

src/main.rs:6:25: 6:29 error: missing lifetime specifier [E0106]
src/main.rs:6 fn do_foo(foo: &Foo) -> &str {

[Manishearth]

https://github.com/Manishearth/rust-clippy/issues/292

[Manishearth]

Been on my radar for a while, just haven't had the time to fix.

We need to bail whenever there are lifetime positions (trait pointer bounds, struct/trait params) which are not specified.

[fhartwig]

I'm having a look at this.

I don't understand why we should necessarily bail on unspecified lifetimes though. E.g.

pub struct Foo<'a> {
    r: &'a str
}
fn do_foo<'a>(foo: Foo, other_string: &'a str) {

should give a warning, even though we didn't specify the lifetime of the foo argument. As far as I can tell, we should treat unspecified lifetimes on structs etc. just like unspecified lifetimes on references. Or maybe I'm just misunderstanding your comment.

[Manishearth]

we should treat unspecified lifetimes on structs etc. just like unspecified lifetimes on references

That's fine with me too. Bailing is just the easier option since there are a lot of cases here.

So there are four types of lifetimes:

• Specified
• Unspecified
• 'static
• Parent, i.e. defined in the parent impl

We should make note of the number of each type of lifetime in the input and output, and handle all the cases there. There are a lot of cases, but if you think you can work them all out, go ahead 😄

[fhartwig]

But don't we have to consider exactly the same cases for lifetimes on references? I'm probably missing something here. I don't think the logic to work out whether lifetimes can be elided would have to change at all. We just have to keep track of unspecified lifetimes on structs/enums as well as references, which the visitor currently doesn't do.

[Manishearth]

Looking at the implementation it's possible to do it without needing much change, I think. You seem to be on the right track.

[Manishearth]

👯

Thanks!