Fix output lifetime collection in needless_lifetimes

[PartiallyUntyped]

This is currently draft as I am unsure about how to go with fixing this. I don't completely understand the code D:, but I've identified the issue more or less.

The problem

In the following snippet

mod issue11291 {
    use std::collections::HashMap;
    pub struct MyContainer(HashMap<u8, u32>);
    impl MyContainer {
        pub fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a u8, &'a u32)> + 'a {
            //~^ ERROR: the following explicit lifetimes could be elided: 'a
            self.0.iter()
        }
    }
}

Clippy will suggest:

 pub fn iter<'a>(&'a self) -> impl Iterator<Item = (&'_ u8, &'a u32)> + 'a

Which misses the 2 latter lifetimes.

I probed a bit into the implementation. The lifetimes of the output are collected via a walker. The implementation of the walker is identical for both input and output declaration.

The problem seems related to the truncation in TyKind::OpaqueDef

rust-clippy/clippy_lints/src/lifetimes.rs

Lines 532 to 542 in 86717f2

	TyKind::OpaqueDef(item, bounds, _) => {
	let map = self.cx.tcx.hir();
	let item = map.item(item);
	let len = self.lts.len();
	walk_item(self, item);
	self.lts.truncate(len);
	self.lts.extend(bounds.iter().filter_map(|bound| match bound {
	GenericArg::Lifetime(&l) => Some(l),
	_ => None,
	}));
	},

Without the truncation, all lifetimes are collected, but going by hir::DefId, we have 4 different lifetimes (LTs) instead of 3. Going by span, we have 3 LTs.

Removing the truncation and keeping only unique LT by their span causes something to break in the linting logic.

Update 1:

It seems that the issue is related to the defids of the LTs, as they can't be found in the allowed LTs.

Update 2:

Yup, using localdefid causes issues with opaque types. Using identifiers for any checks works better and the types are correlated selected. need to fix how the suggestions work to account for that, but otherwise it seems correct-ish.

changelog: [`needless_lifetimes`]: Suggested fix will now elide all output lifetimes.

fixes: #12085
fixes: #11291

r? @blyxyas

[PartiallyUntyped]

Current commit results in a regression where more LTs are collected than they should be.

[PartiallyUntyped]

This is wrong, it wrongly collects both 'a and 'b, should only elide 'a

[PartiallyUntyped]

'a shouldn't be collected.

[PartiallyUntyped]

Same issue as above.

[xFrednet]

Hey @m-rph, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

@rustbot author

[PartiallyUntyped]

@xFrednet yup. Need to find time. There's 5 issues that need to be addressed and will do all of them in 1 rust go.

[xFrednet]

Okay, just here to poke you a bit ^^ Let me know if you need any help.

error: found time
  --> https://github.com/rust-lang/rust-clippy/pull/12456/comment-5/1:29
   |
   | @xFrednet yup. Need to find time. 
   |                             ^^^^
   = note: This `time` might be a different type then the
           one you where refering to

Sorry, couldn't resist after your comment :P

[xFrednet]

The assignment was a test. I'm unassigning myself again. Thank you for the help!

[PartiallyUntyped]

With Alejandra we discussed whether I could rewrite the lint given how old it was, both options are on the table. Seeing that I have accumulated a large collection of issues around needless_lifetimes, I think it's best to just go ahead with the rewrite.

[xFrednet]

Hey, @m-rph this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

Also, is this PR maybe outdated, since you crated #13141?

@rustbot author

[PartiallyUntyped]

Forgot about this. I will close.