False positive for needless_lifetimes

[andybarron]

Here's a trivial example that should trigger it. Removing the lifetime annotations causes the code not to compile (cannot infer an appropriate lifetime ...), but clippy gives a needless_lifetimes warning.

struct Test {
    vec: Vec<usize>,
}

impl Test {
    fn iter<'a>(&'a self) -> Box<Iterator<Item = usize> + 'a> {
        Box::new(self.vec.iter().cloned())
    }
}

[llogiq]

Yeah, I have a similar example (Adapter from rust-bio). I'm unsure how to fix it however.

[mcarton]

Rustful also has one:

src/router/tree_router.rs:80:5: 110:6 warning: explicit lifetimes given in parameter types where they could be elided, #[warn(needless_lifetimes)] on by default
src/router/tree_router.rs: 80     fn merge_router<'a, I: Iterator<Item = &'a [u8]> + Clone>(&mut self, state: InsertState<'a, I>, router: TreeRouter<T>) {
src/router/tree_router.rs: 81         self.item.insert_router(state.clone(), router.item);
src/router/tree_router.rs: 82 
src/router/tree_router.rs: 83         for (key, router) in router.static_routes {
src/router/tree_router.rs: 84             let next = match self.static_routes.entry(key.clone()) {
src/router/tree_router.rs: 85                 Occupied(entry) => entry.into_mut(),
                              ...
src/router/tree_router.rs:80:5: 110:6 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes

(source)

[llogiq]

Do I understand this right and the problem is that Box<T> defaults to Box<T + 'static>?

[birkenfeld]

Another case:

fn test<'a, T: Into<Cow<'a, str>>>(&'a self, x: T) { }

[dtolnay]

A tricky false positive from the standard library: HashMap::keys

pub fn keys<'a>(&'a self) -> Keys<'a, K, V> {
    fn first<A, B>((a, _): (A, B)) -> A { a }
    let first: fn((&'a K, &'a V)) -> &'a K = first; // coerce to fn ptr

    Keys { inner: self.iter().map(first) }
}

The lifetime could be elided in the function signature, but the implementation requires it to be named.

[mcarton]

That’s a tricky one, we don’t even look for the implementation right now AFAIK. @dtolnay thanks for the report!

[Manishearth]

That lifetime isn't necessary in the implementation AFAICT

[dtolnay]

How do I get rid of it? playpen link

It seems necessary because the inner function needs explicit lifetimes and cannot declare them itself.

[mcarton]

@Manishearth If you remove the lifetimes in that example:

fn foo<'a>(a: &'a u8) -> &'a u8 {
    fn bar<A, B>((a, _): (A, B)) -> A { a }
    let baz: fn((&'a u8, &'a u8)) -> &'a u8 = bar;

    baz((a, a))
}

you get:

<anon>:3:32: 3:35 error: missing lifetime specifier [E0106]
<anon>:3     let baz: fn((&u8, &u8)) -> &u8 = bar;
                                        ^~~
<anon>:3:32: 3:35 help: see the detailed explanation for E0106
<anon>:3:32: 3:35 help: this function's return type contains a borrowed value, but the signature does not say which one of argument 1's 2 elided lifetimes it is borrowed from

play.rust-lang.org

[Manishearth]

Huh, I see. Interesting. We could just walk the body for uses of the lifetime and bail

[nipunn1313]

Here is another example we're running into in our codebase (similar to OP).

pub fn components<'a>(&'a self) -> Box<Iterator<Item=&'a str> + 'a> {
    Box::new(self.path.components().map(|c| c.to_str().unwrap()))
}

The lifetime can't be elided because the box inner needs to have the same lifetime as self which is not inferable from the callsite.

[hanna-kruppe]

This also pops up with impl Trait return types:

pub fn pixels<'a>(&'a self) -> impl Iterator<Item=((u32, u32), &'a T)> { // ...

In that case it's not just a wrong default, AFAIK one can never omit the lifetime in &'a T. Not sure whether this should be a separate issue?

[oli-obk]

Another one:

fn f<'a, T: LintContext<'a>>(cx: &T) {}

[shssoichiro]

This is producing many false positives in Rocket's codebase. I want to suggest making this lint allow by default until this bug can be fixed.

[llogiq]

So to recap: We have three classes of false positives:

1. Lifetime signatures where elisions defaults to 'static (though we'll have to find out how to distinguish this)

2. impl trait in signatures. This one should be easy to detect

3. functions which are referenced as fn or Fn* value – we'll have to walk the crate before to collect all fn refs and compare with the current fn. Apart from needing another visitor, this should be doable.

[jturner314]

This is another type of false positive:

#![allow(dead_code)]

#[derive(Debug)]
struct Foo<'a> {
    x: &'a f64,
}

impl<'a> Foo<'a> {
    // Clippy treats this as fine.
    fn with_x(self, x: &'a f64) -> Foo<'a> {
        Foo {
            x: x
        }
    }
}

#[derive(Debug)]
struct Bar<'a> {
    x: &'a f64,
}

impl<'a> Bar<'a> {
    // Clippy gives a needless_lifetimes warning.
    fn with_x<'b>(self, x: &'b f64) -> Bar<'b> {
        Bar {
            x: x
        }
    }
}

fn main() {
    // This doesn't work for Foo:
    // let x1 = 1.;
    // let struct1 = {
    //     let x2 = 2.;
    //     let struct2 = Foo {
    //         x: &x2,
    //     };
    //     struct2.with_x(&x1)
    // };
    // println!("{:?}", struct1);

    // The exact same thing works for Bar:
    let x1 = 1.;
    let struct1 = {
        let x2 = 2.;
        let struct2 = Bar {
            x: &x2,
        };
        struct2.with_x(&x1)
    };
    println!("{:?}", struct1);
}

The with_x functions consume self with lifetime 'a and take a reference x. In the case of Foo, the return value has the same lifetime 'a, but Bar is more flexible because its return value has the lifetime of the x argument.

The flexible behavior of Bar is better, but clippy gives a needless_lifetimes warning even though the lifetime annotations are meaningful.