Fix some false positive in needless_lifetimes lint #452
Conversation
|
\o/ thanks! 🍰 reviewing now |
| TyPath(_, ref path) => { | ||
| self.collect_anonymous_lifetimes(path, ty); | ||
| }, | ||
| _ => {} |
There was a problem hiding this comment.
We should bail if there is a TyObjectSum or TyPolyTraitRef, or a TyRptr of any kind around a trait (&Trait). Can be done in followup if you want.
There was a problem hiding this comment.
What is the problem with those? I've tested this on functions with trait object arguments, and I haven't noticed any problems.
There was a problem hiding this comment.
It's an extra lifetime position with some strange rules around it IIRC.
There was a problem hiding this comment.
Ok. Since I don't think this PR makes the situation regarding those any worse than it is right now (or does it?), I'd rather do this separately.
|
This looks great to me, minor changes. |
|
I think we may want more test cases to see at a glance what works and what doesn't. |
|
@llogiq I'm happy to add more test cases. Any case in particular that you think is missing? |
|
I think we could do with more tests where lifetimes are only partially specified. But we can do this in a followup. r+ |
Fix some false positive in needless_lifetimes lint
Fixes #417
The current lifetime-collecting visitor in
needless_lifetimesdoesn't take into account elided lifetimes on structs, enums or trait objects when collecting input and output lifetimes, which can lead to false positives. These changes fix it to consider these elided lifetimes as well.One problem that I couldn't figure out is how to look up the definitions for type (and trait) aliases, which I would need to do to make this work with type aliases in function argument and return types as well. Any hints would be appreciated (although this PR should already strictly, and significantly, decrease the number of false positives).