new lint for needless lifetimes (fixes #115) #140
Conversation
|
Please check my conditions for elision, I'm not 100% firm on these things yet. |
|
Note, this will not catch things like where apparently you can also elide the lifetimes. Is this a good thing? Otherwise I'll have to write a function that walks the types in depth. |
| // * output reference, exactly one input reference with same LT | ||
|
|
||
| // extract lifetime of input arguments | ||
| let mut input_lts = func.inputs.iter().filter_map(|arg| extract_lifetime(&*arg.ty)) |
There was a problem hiding this comment.
We can just flat_map this with http://manishearth.github.io/rust-internals-docs/rustc/middle/ty/struct.TyS.html#method.walk first to find lifetime params deep in the type sig
There was a problem hiding this comment.
Wait, scratch that, wrong ty.
There was a problem hiding this comment.
We need to write a small Visitor that collects all type params from TyPath and TyRptr, and use http://manishearth.github.io/rust-internals-docs/syntax/visit/fn.walk_ty.html. Not too hard.
|
The problem is that we might get false positives too if we don't walk the types. For example, the following: will lint as elidable, but it isn't. But it's not too hard to write a visitor that collects lifetimes, so no biggie 😄 Clippy is okay with false positives (they're lints, not compiler errors), but we should minimize them as much as possible. |
| .collect::<Vec<_>>(); | ||
| // extract lifetime of "self" for methods | ||
| if let FnKind::FkMethod(_, sig, _) = kind { | ||
| if let SelfRegion(ref lt_opt, _, _) = sig.explicit_self.node { |
There was a problem hiding this comment.
It can also be SelfExplicit, e.g. fn foo(self: &mut Foo) {}. We should match on SelfExplicit and walk that type too.
|
Looks like a good start! We just need to extend |
|
Thanks for the review! Looking at the comments now... |
birkenfeld
force-pushed
the
lifetimes
branch
2 times, most recently
from
6c2c7ca to
3820a05
Compare
|
Ok, now we shouldn't have false positives anymore. (Running clippy on a large codebase like rustc might be useful to check?) I'm mostly concerned about false negatives with the "no output references" case. |
| // in func.inputs, but its type is TyInfer) | ||
| if let FnKind::FkMethod(_, sig, _) = kind { | ||
| match sig.explicit_self.node { | ||
| SelfRegion(ref opt_lt, _, _) => input_visitor.record(opt_lt), |
There was a problem hiding this comment.
We should additionally walk the inner type, e.g. &'a Foo<'b> and &'a Foo<'a> have different effects, and we should only be suggesting elision for the former
There was a problem hiding this comment.
Oh nvm I'm an idiot this is a self type. Ignore the above
|
Yeah, As far as testing goes, feel free to use it against rustc, but Rust contains a lot of old code so we'll get a lot of legitimate warnings -- we want to look for spurious ones, so perhaps pick something like Hyper and try. I'll be hooking clippy up to servo at some point, we'll get more feedback then. |
|
LGTM, minor addition to the tests needed. I'm fine with testing this for false positives after merging -- I'll wait on publishing a new version for a bit then. |
|
|
||
| #![deny(needless_lifetimes)] | ||
|
|
||
| fn distinct_lifetimes<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: u8) { } //~ERROR |
There was a problem hiding this comment.
It is good custom to copy the start of the error message after the //~ERROR (if you dislike the width, you can also write //~^ERROR in the next line. This also allows to declare multiple errors in one line, just increment the number of ^). I recently failed to follow this good custom and got a successful compile-fail test when the lint in fact didn't work.
There was a problem hiding this comment.
Totally agreed, will use that here, and also in my other recently written tests.
No description provided.