False positive in map_identity with lifetimes

[human-0]

Summary

Mapping with an identity closure (|x| x) is sometimes required to fix lifetimes.

Lint Name

map_identity

Reproducer

I tried this code:

pub fn test<'a, I>(iter: I)
where
    I: IntoIterator<Item = &'a i32>
{
    let local = 0;
    let _ = std::iter::once(&local).chain(iter.into_iter().map(|x| x));
}

Removing the map results in a compile error.
I saw this happen:

warning: unnecessary map of the identity function
 --> src/lib.rs:6:59
  |
6 |     let _ = std::iter::once(&local).chain(iter.into_iter().map(|x| x));
  |                                                           ^^^^^^^^^^^ help: remove the call to `map`
  |
  = note: `#[warn(clippy::map_identity)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_identity

I expected to see this happen:
No warning, as the map cannot be removed without causing a compile error. This happens on both stable and nightly.

Version

rustc 1.64.0-nightly (0f4bcadb4 2022-07-30)
binary: rustc
commit-hash: 0f4bcadb46006bc484dad85616b484f93879ca4e
commit-date: 2022-07-30
host: x86_64-pc-windows-msvc
release: 1.64.0-nightly
LLVM version: 14.0.6

Additional Labels

@rustbot label +I-suggestion-causes-error

[squ1dd13]

Does clippy have enough information to be able to ignore identity map calls where the type input into the closure is different from the type that it outputs? Would that be a viable solution?
I suppose the important thing is that clippy's complaint here is actually wrong, on the grounds that the "identity function" isn't actually an identity function – even if it outputs the same value as is put in bit-for-bit, the types do not match. Should that still be considered an identity function? It clearly isn't for the compiler.

[xFrednet]

Does clippy have enough information to be able to ignore identity map calls where the type input into the closure is different from the type that it outputs?

Yes, Clippy has all the information that rustc has.

Would that be a viable solution?

Also yes, the current implementation is a bit naive IIRC. That causes a false positive. Even though I'm a bit surprised that we need the |x| x here. 🤔

[cyqsimon]

Just ran into this in sharkdp/bat#2755.

Asked on community Discord and got a proper explanation:

It's not that Iterator::map is "downcasting lifetimes", it's just that impl Iterator<Item = T> is invariant in T, so you need a closure to "do" the variance

See: https://discord.com/channels/273534239310479360/1120124565591425034/1170543402870382653

[cyqsimon]

I'll give this one a shot. Never messed with the compiler/linter before so wish me luck.

@rustbot claim

[cyqsimon]

Unsurprisingly, I am encountering significant difficulties. I've got a high-level sense of what I'm supposed to check for (don't raise a warning when the input lifetime is unequal to the output lifetime), but honestly not much else. Frankly I feel like I have no idea what I'm doing, which is probably true.

I'm not giving up just yet; you know, treating this as a learning opportunity etc. etc, but it will surely take me a lot of time. If anyone else wants to work on this (if you do, you are in all likelihood more competent than me), please go right ahead.

[xFrednet]

Hmm, I haven't looked much into this issue, but I would suggest you to look at the following two things:

1. You can look at the HIR structure of the expression with the #[clippy::dump] attribute: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=00f52daacb276412d25625dd4d6fa2d8
2. For the actual solution, you'll probably have a look at the expression type of the parameter and return type. This might give you some more hints how to get the semantic type: https://doc.rust-lang.org/clippy/development/common_tools_writing_lints.html Note that lifetimes are turned into regions on the semantic level.

[Alexendoo]

Some lifetimes are erased, it may not be possible to detect

[waynexia]

Run into this with another example:

pub fn foo(
    input: impl IntoIterator<Item = &'a (A, B)>,
) {
    let _ : HashMap<&'a A, &'a B> = input.into_iter().map(|(k, v)| (k, v)).collect();
}

Where the input type is &(A, B) and output type is (&A, &B)

[y21]

Where the input type is &(A, B) and output type is (&A, &B)

That bug was fixed in #11792 and I think match ergonomics is unrelated to the lifetime problem here?

If I run your code with a newer clippy build (tested nightly 2023-12-03), this indeed no longer emits a warning.

[stephaneyfx]

The bug regarding &(A, B) turned into (&A, &B) seems to be back.

pub struct Foo;

pub fn foo(x: Option<&(Foo, Foo)>) -> Option<(&Foo, &Foo)> {
    x.map(|(a, b)| (a, b))
}

pub fn bar(xs: Vec<&(Foo, Foo)>) -> Vec<(&Foo, &Foo)> {
    xs.into_iter().map(|(a, b)| (a, b)).collect()
}

Clippy output (clippy 0.1.75 (82e1608 2023-12-21)):

warning: unnecessary map of the identity function
 --> src/lib.rs:4:6
  |
4 |     x.map(|(a, b)| (a, b))
  |      ^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_identity
  = note: `#[warn(clippy::map_identity)]` on by default

warning: unnecessary map of the identity function
 --> src/lib.rs:8:19
  |
8 |     xs.into_iter().map(|(a, b)| (a, b)).collect()
  |                   ^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_identity

[y21]

@stephaneyfx Your Rust playground link uses the stable channel and stable does not have that fix yet (it takes quite a while for a change made today to actually land on stable, and the date that the playground shows for stable clippy is a bit misleading). Changing the channel to nightly makes the warnings go away, so I presume that it actually is fixed?
Then again, this is different from the original issue here, so please open a new issue if you think this case isn't properly fixed

[stephaneyfx]

My bad for incorrectly assuming the two-month-old fix was available on stable. I cannot reproduce using nightly with either the playground or my actual code, as you mentioned. Thank you and sorry for the noise.