needless_collect: avoid warning if non-iterator methods are used #14147
Conversation
It can make sense to `collect()` an iterator and then immediately iterate over it if the iterator has special methods that you need. For example, the Map iterator doesn't implement Clone, but the collection iterator might. Or the collection iterator might support slicing.
rustbot
added
the
S-waiting-on-review
| impl<'tcx> Visitor<'tcx> for IteratorMethodCheckVisitor<'_, 'tcx> { | ||
| type Result = ControlFlow<()>; | ||
| fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) -> ControlFlow<()> { | ||
| if let ExprKind::MethodCall(_method_name, recv, _args, _) = &expr.kind | ||
| && (recv.hir_id == self.hir_id_of_expr | ||
| || self | ||
| .hir_id_of_let_binding | ||
| .is_some_and(|hid| path_to_local_id(recv, hid))) | ||
| && !is_trait_method(self.cx, expr, sym::Iterator) | ||
| { | ||
| return ControlFlow::Break(()); | ||
| } | ||
| walk_expr(self, expr) | ||
| } | ||
| fn visit_stmt(&mut self, stmt: &'tcx Stmt<'tcx>) -> ControlFlow<()> { | ||
| if let StmtKind::Let(LetStmt { | ||
| init: Some(expr), | ||
| pat: | ||
| Pat { | ||
| kind: PatKind::Binding(BindingMode::NONE | BindingMode::MUT, id, _, None), | ||
| .. | ||
| }, | ||
| .. | ||
| }) = &stmt.kind | ||
| && expr.hir_id == self.hir_id_of_expr | ||
| { | ||
| self.hir_id_of_let_binding = Some(*id); | ||
| } | ||
| walk_stmt(self, stmt) | ||
| } | ||
| } |
There was a problem hiding this comment.
I've been checking for already-computed alternatives to this visitor. Seeing it
gives you a "this has to be already computed somewhere", and there are liveness
checks throughout Rust that are near what we need, but it seems that there isn't
anywhere that we can check to traverse through all uses of a local.
Quite sad.
| } | ||
| } | ||
|
|
||
| fn check_iter_expr_used_only_as_iterator<'tcx>( |
There was a problem hiding this comment.
What would happen if we were to do something like this:
let x;
{
x = xxx.into_iter();
for i in x.clone();
}If I'm not wrong, the code that comes before this, only looks at the current
scope, first looking for the initializing local, and then assigning that to
some iterable.
Testing that, it seems that the x = part is a Path with a Def of Local, could
you also take that into acccount?
There was a problem hiding this comment.
You're correct. The code will also check assignment to a local now.
changelog: [
needless_collect]: avoid warning if non-Iteratormethods are called on the result ofinto_iterFixes #13430