Add needless_character_iteration lint
#12815
Conversation
rustbot
added
the
S-waiting-on-review
GuillaumeGomez
force-pushed
the
add-needless_character_iteration
branch
2 times, most recently
from
1fa6ea2
to
076e3bc
Compare
|
Fixed dogfood errors. |
| #![allow(clippy::unnecessary_operation)] | ||
|
|
||
| fn main() { | ||
| "foo".chars().all(|c| c.is_ascii()); |
There was a problem hiding this comment.
Can you add a test where there is a different method between chars() and all()? Also some test where the all call tests something else as well, like .all(|c| c.is_ascii() && c.is_alphabetic())
| "foo".chars().all(|c| { | ||
| //~^ ERROR: checking if a string is ascii using iterators | ||
| let x = c; | ||
| x.is_ascii() | ||
| }); |
There was a problem hiding this comment.
Could you also add a test where the closure contains some additional statements, like this:
"foo".chars().all(|c| {
let x = c;
magic(x);
x.is_ascii()
});
There was a problem hiding this comment.
That was a very good call, the lint was emitted in this case but shouldn't have! Fixed it. :)
| while let ExprKind::MethodCall(_, new_recv, _, _) = recv.kind { | ||
| recv = new_recv; | ||
| } |
There was a problem hiding this comment.
When is this loop actually used? It feels like this could iterate through more methods calls, then the ones that are being linted 🤔
An example I considered was
#[derive(Default)]
struct S {
field: &'static str,
}
impl S {
fn field(&self) -> &str {
&self.field
}
}
S::default().field().chars().all(|x| x.is_ascii());
There was a problem hiding this comment.
It's used to the get the whole chain of calls to suggest replacement for everything.
There was a problem hiding this comment.
Can you give an example for when this? I would thing that just taking recv.span() would already cover the whole thing? 🤔
There was a problem hiding this comment.
In the code example you gave, without this the span would cover chars().all(|x| x.is_ascii()). With this, it covers the entire expression: S::default().field().chars().all(|x| x.is_ascii()).
This is needed in case we need to have !.is_ascii(). If we don't have the full expression, then the suggestion will go wrong.
| ExprKind::Block(block, _) => { | ||
| if let Some(block_expr) = block.expr | ||
| // First we ensure that this is a "binding chain" (each statement is a binding | ||
| // of the previous one) and that it is a binding of the closure argument. | ||
| && let Some(last_chain_binding_id) = | ||
| get_last_chain_binding_hir_id(first_param, block.stmts) | ||
| { | ||
| handle_expr(cx, block_expr, last_chain_binding_id, span, before_chars, revert); | ||
| } | ||
| }, |
There was a problem hiding this comment.
I feel like this part complicates the lint unnecessary. If people write weird code where they chain these things, I'd say it's okay to not lint it, but we can keep it if you want.
There was a problem hiding this comment.
It's convenient and easy enough to check so if you don't mind, I'd prefer to keep it. ;)
GuillaumeGomez
force-pushed
the
add-needless_character_iteration
branch
from
076e3bc
to
566dfd9
Compare
|
Fixed the corner cases and added more tests. |
Fixes #4817.
r? @xFrednet
changelog: Add
needless_character_iterationlint