Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect needless_lifetimes warning for impl Trait based code #2944

Closed
vitalyd opened this issue Jul 20, 2018 · 8 comments · Fixed by #5978
Closed

Incorrect needless_lifetimes warning for impl Trait based code #2944

vitalyd opened this issue Jul 20, 2018 · 8 comments · Fixed by #5978
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@vitalyd
Copy link

vitalyd commented Jul 20, 2018

Given the following code:

#![crate_type = "lib"]
#![allow(unused)]
use std::collections::{HashMap, HashSet};

struct MyMap {
    inner: HashMap<u32, HashSet<String>>,
}

impl MyMap {
    fn iter<'a>(&'a self, key: u32) -> Option<impl Iterator<Item = impl AsRef<str> + 'a>> {
        self.inner.get(&key).map(|set| set.iter())
    }
}

fn main() {}

Playground

clippy reports the following:

 Checking playground v0.0.1 (file:///playground)
warning: explicit lifetimes given in parameter types where they could be elided
  --> src/main.rs:10:5
   |
10 | /     fn iter<'a>(&'a self, key: u32) -> Option<impl Iterator<Item = impl AsRef<str> + 'a>> {
11 | |         self.inner.get(&key).map(|set| set.iter())
12 | |     }
   | |_____^
   |
   = note: #[warn(needless_lifetimes)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#needless_lifetimes

    Finished dev [unoptimized + debuginfo] target(s) in 1.40s

But if you take its suggestion and remove 'a from iter(), you get the following compiler error:

 Compiling playground v0.0.1 (file:///playground)
error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
  --> src/lib.rs:11:20
   |
11 |         self.inner.get(&key).map(|set| set.iter())
   |                    ^^^
   |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the method body at 10:5...
  --> src/lib.rs:10:5
   |
10 | /     fn iter(&self, key: u32) -> Option<impl Iterator<Item = impl AsRef<str>>> {
11 | |         self.inner.get(&key).map(|set| set.iter())
12 | |     }
   | |_____^
note: ...so that reference does not outlive borrowed content
  --> src/lib.rs:11:9
   |
11 |         self.inner.get(&key).map(|set| set.iter())
   |         ^^^^^^^^^^
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that return value is valid for the call
  --> src/lib.rs:10:40
   |
10 |     fn iter(&self, key: u32) -> Option<impl Iterator<Item = impl AsRef<str>>> {
   |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0495`.
error: Could not compile `playground`.

To learn more, run the command again with --verbose.

It's possible this is a dupe of an existing issue but a cursory search revealed a few old (and closed) impl Trait related bugs, and I didn't find anything current.

@phansch phansch added the C-bug Category: Clippy is not doing the correct thing label Dec 11, 2018
@jbaum98
Copy link

jbaum98 commented Aug 21, 2019

I think I have also encountered this issue, with a more minimal example:

struct Context {
    x: i32,
}

impl Context {
    fn get_x<'a>(&'a self) -> impl Fn() -> &'a i32 {
        move || &self.x
    }
}

Playground

With a little guidance, I'm happy to make a PR to fix the issue.

@flip1995
Copy link
Member

The fix needs to be made in the

fn could_use_elision<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
func: &'tcx FnDecl,
body: Option<BodyId>,
named_generics: &'tcx [GenericParam],
bounds_lts: Vec<&'tcx Lifetime>,
) -> bool {

function.

This function assumes, that elision is possible, if

// * output references, exactly one input reference with same LT

This is wrong in this case.

This lint is one of the oldest lints Clippy has (#140, way before my time), and was only touched once (#1672) since then. So I'm not really sure how exactly it works and can only point you to the right place 😉

@hadronized
Copy link

Same issue here while hacking on glsl. Minimal reproducible code I was able to get:

use nom::IResult;
use nom::error::VerboseError;
use std::ops::Fn;

pub type ParserResult<'a, O> = IResult<&'a str, O, VerboseError<&'a str>>;

pub fn keyword<'a>(_: &'a str) -> impl Fn(&'a str) -> ParserResult<'a, &'a str> {
  |_| unimplemented!()
}

With nom-5.

@tnielens
Copy link
Contributor

I'm taking a stab at this one.

@tnielens
Copy link
Contributor

tnielens commented Aug 25, 2020

There are two distinct cases in this issue.

case 1, the original one in the description:

impl MyMap {
    fn iter<'a>(&'a self, key: u32) -> Option<impl Iterator<Item = impl AsRef<str> + 'a>> {
        self.inner.get(&key).map(|set| set.iter())
    }
}

In this case, the lint is actually correct. Removing the lifetime notations produces an error, but the compiler error has improved:

...
help: to declare that the `impl Trait` captures data from argument `self`, you can add an explicit `'_` lifetime bound
   |
10 |     fn iter(& self, key: u32) -> Option<impl Iterator<Item = impl AsRef<str>> + '_> {
...

So the following version with anonymous lifetime compiles fine.

impl MyMap {
    fn iter(&self, key: u32) -> Option<impl Iterator<Item = impl AsRef<str> + '_>> {
        self.inner.get(&key).map(|set| set.iter())
    }
}

Since the lint is not making any wrong suggestion here, I would leave it as is. The compiler error will help the user to find the correct elided syntax.

case 2: the next examples commented in the issue boil down to this:

fn get_x<'a>(i32_pointer: &'a i32) -> impl Fn() -> &'a i32 {
    move || i32_pointer
}

Lifetime elision applies to three constructs: function items, function pointers, and closure trait bounds. This case has a nested elision candidate in impl Fn() -> &'a i32 (a closure trait bound). If 'a explicit lifetimes are removed, the compiler infers lifetimes for impl Fn() -> &i32 separately and will assign the 'static lifetime to the output reference &'static i32.

One solution to avoid FPs here is to bail out if a function pointer or a closure trait bound is found in the function signature. That might introduce a few FNs though.

@andrewbanchich
Copy link
Contributor

I believe I'm getting this as well for:

pub async fn profile<'a>(&self, doc: &'a Value) -> Result<ProfileResult<'a>, ProfilerError>

Using '_ for any of these lifetimes just makes rustc yell at me.

@ebroto
Copy link
Member

ebroto commented Sep 3, 2020

@andrewbanchich I think your case looks more like #4746 as you can't elide the lifetime here because it's not related to self.

@andrewbanchich
Copy link
Contributor

@ebroto That makes sense. Thanks!

@bors bors closed this as completed in d431373 Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants