Skip to content

Commit

Permalink
Fix FP in indirect needless_collect when used multiple times
Browse files Browse the repository at this point in the history
  • Loading branch information
@giraffate
giraffate committed Nov 12, 2020
1 parent bd13a35 commit 0a293bd
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 41 deletions.
114 changes: 73 additions & 41 deletions clippy_lints/src/loops.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ impl<'tcx> LateLintPass<'tcx> for Loops {
applicability,
);
}
},
}
_ => (),
}
}
Expand Down Expand Up @@ -685,7 +685,7 @@ fn combine_both(left: NeverLoopResult, right: NeverLoopResult) -> NeverLoopResul
match (left, right) {
(NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) => {
NeverLoopResult::MayContinueMainLoop
},
}
(NeverLoopResult::AlwaysBreak, _) | (_, NeverLoopResult::AlwaysBreak) => NeverLoopResult::AlwaysBreak,
(NeverLoopResult::Otherwise, NeverLoopResult::Otherwise) => NeverLoopResult::Otherwise,
}
Expand All @@ -698,7 +698,7 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
(NeverLoopResult::AlwaysBreak, NeverLoopResult::AlwaysBreak) => NeverLoopResult::AlwaysBreak,
(NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) => {
NeverLoopResult::MayContinueMainLoop
},
}
(NeverLoopResult::Otherwise, _) | (_, NeverLoopResult::Otherwise) => NeverLoopResult::Otherwise,
}
}
Expand Down Expand Up @@ -731,7 +731,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
| ExprKind::DropTemps(ref e) => never_loop_expr(e, main_loop_id),
ExprKind::Array(ref es) | ExprKind::MethodCall(_, _, ref es, _) | ExprKind::Tup(ref es) => {
never_loop_expr_all(&mut es.iter(), main_loop_id)
},
}
ExprKind::Call(ref e, ref es) => never_loop_expr_all(&mut once(&**e).chain(es.iter()), main_loop_id),
ExprKind::Binary(_, ref e1, ref e2)
| ExprKind::Assign(ref e1, ref e2, _)
Expand All @@ -740,7 +740,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
ExprKind::Loop(ref b, _, _) => {
// Break can come from the inner loop so remove them.
absorb_break(&never_loop_block(b, main_loop_id))
},
}
ExprKind::Match(ref e, ref arms, _) => {
let e = never_loop_expr(e, main_loop_id);
if arms.is_empty() {
Expand All @@ -749,7 +749,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
let arms = never_loop_expr_branch(&mut arms.iter().map(|a| &*a.body), main_loop_id);
combine_seq(e, arms)
}
},
}
ExprKind::Block(ref b, _) => never_loop_block(b, main_loop_id),
ExprKind::Continue(d) => {
let id = d
Expand All @@ -760,7 +760,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
} else {
NeverLoopResult::AlwaysBreak
}
},
}
ExprKind::Break(_, ref e) | ExprKind::Ret(ref e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| {
combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)
}),
Expand All @@ -775,7 +775,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
InlineAsmOperand::Out { expr, .. } => never_loop_expr_all(&mut expr.iter(), main_loop_id),
InlineAsmOperand::SplitInOut { in_expr, out_expr, .. } => {
never_loop_expr_all(&mut once(in_expr).chain(out_expr.iter()), main_loop_id)
},
}
})
.fold(NeverLoopResult::Otherwise, combine_both),
ExprKind::Struct(_, _, None)
Expand Down Expand Up @@ -1030,10 +1030,10 @@ fn get_details_from_idx<'tcx>(
.or_else(|| get_start(cx, rhs, starts).and_then(|s| get_offset(cx, lhs, starts).map(|o| (s, o))));

offset_opt.map(|(s, o)| (s, Offset::positive(o)))
},
}
BinOpKind::Sub => {
get_start(cx, lhs, starts).and_then(|s| get_offset(cx, rhs, starts).map(|o| (s, Offset::negative(o))))
},
}
_ => None,
},
ExprKind::Path(..) => get_start(cx, idx, starts).map(|s| (s, Offset::empty())),
Expand Down Expand Up @@ -1176,7 +1176,7 @@ fn build_manual_memcpy_suggestion<'tcx>(
)
.into_sugg(),
)
},
}
};

let (dst_offset, dst_limit) = print_offset_and_limit(&dst);
Expand Down Expand Up @@ -1307,7 +1307,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SameItemPushVisitor<'a, 'tcx> {
// Non-determinism may occur ... don't give a lint
ExprKind::Loop(_, _, _) | ExprKind::Match(_, _, _) => self.should_lint = false,
ExprKind::Block(block, _) => self.visit_block(block),
_ => {},
_ => {}
}
}

Expand All @@ -1323,7 +1323,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SameItemPushVisitor<'a, 'tcx> {
// Current statement is not a push so visit inside
match &s.kind {
StmtKind::Expr(expr) | StmtKind::Semi(expr) => self.visit_expr(&expr),
_ => {},
_ => {}
}
} else {
// Current statement is a push ...check whether another
Expand Down Expand Up @@ -1436,14 +1436,14 @@ fn detect_same_item_push<'tcx>(
}
}
}
},
}
// constant
Res::Def(DefKind::Const, ..) => emit_lint(cx, vec, pushed_item),
_ => {},
_ => {}
}
},
}
ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item),
_ => {},
_ => {}
}
}
}
Expand Down Expand Up @@ -1550,7 +1550,7 @@ fn check_for_loop_range<'tcx>(
ast::RangeLimits::Closed => {
let take_expr = sugg::Sugg::hir(cx, take_expr, "<count>");
format!(".take({})", take_expr + sugg::ONE)
},
}
ast::RangeLimits::HalfOpen => format!(".take({})", snippet(cx, take_expr.span, "..")),
}
}
Expand Down Expand Up @@ -2221,13 +2221,13 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
self.visit_expr(lhs);
self.prefer_mutable = false;
self.visit_expr(rhs);
},
}
ExprKind::AddrOf(BorrowKind::Ref, mutbl, ref expr) => {
if mutbl == Mutability::Mut {
self.prefer_mutable = true;
}
self.visit_expr(expr);
},
}
ExprKind::Call(ref f, args) => {
self.visit_expr(f);
for expr in args {
Expand All @@ -2240,7 +2240,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
}
self.visit_expr(expr);
}
},
}
ExprKind::MethodCall(_, _, args, _) => {
let def_id = self.cx.typeck_results().type_dependent_def_id(expr.hir_id).unwrap();
for (ty, expr) in self.cx.tcx.fn_sig(def_id).inputs().skip_binder().iter().zip(args) {
Expand All @@ -2252,11 +2252,11 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
}
self.visit_expr(expr);
}
},
}
ExprKind::Closure(_, _, body_id, ..) => {
let body = self.cx.tcx.hir().body(body_id);
self.visit_expr(&body.value);
},
}
_ => walk_expr(self, expr),
}
self.prefer_mutable = old;
Expand Down Expand Up @@ -2454,13 +2454,13 @@ impl<'a, 'tcx> Visitor<'tcx> for IncrementVisitor<'a, 'tcx> {
IncrementVisitorVarState::DontWarn
};
}
},
}
ExprKind::Assign(ref lhs, _, _) if lhs.hir_id == expr.hir_id => {
*state = IncrementVisitorVarState::DontWarn
},
}
ExprKind::AddrOf(BorrowKind::Ref, mutability, _) if mutability == Mutability::Mut => {
*state = IncrementVisitorVarState::DontWarn
},
}
_ => (),
}
}
Expand Down Expand Up @@ -2569,7 +2569,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> {
match parent.kind {
ExprKind::AssignOp(_, ref lhs, _) if lhs.hir_id == expr.hir_id => {
self.state = InitializeVisitorState::DontWarn;
},
}
ExprKind::Assign(ref lhs, ref rhs, _) if lhs.hir_id == expr.hir_id => {
self.state = if_chain! {
if self.depth == 0;
Expand All @@ -2581,10 +2581,10 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> {
InitializeVisitorState::DontWarn
}
}
},
}
ExprKind::AddrOf(BorrowKind::Ref, mutability, _) if mutability == Mutability::Mut => {
self.state = InitializeVisitorState::DontWarn
},
}
_ => (),
}
}
Expand Down Expand Up @@ -2653,7 +2653,7 @@ fn is_loop_nested(cx: &LateContext<'_>, loop_expr: &Expr<'_>, iter_expr: &Expr<'
if let ExprKind::Loop(..) = expr.kind {
return true;
};
},
}
Some(Node::Block(block)) => {
let mut block_visitor = LoopNestVisitor {
hir_id: id,
Expand All @@ -2664,11 +2664,11 @@ fn is_loop_nested(cx: &LateContext<'_>, loop_expr: &Expr<'_>, iter_expr: &Expr<'
if block_visitor.nesting == RuledOut {
return false;
}
},
}
Some(Node::Stmt(_)) => (),
_ => {
return false;
},
}
}
id = parent;
}
Expand Down Expand Up @@ -2713,7 +2713,7 @@ impl<'tcx> Visitor<'tcx> for LoopNestVisitor {
if match_var(path, self.iterator) {
self.nesting = RuledOut;
}
},
}
_ => walk_expr(self, expr),
}
}
Expand Down Expand Up @@ -2810,8 +2810,8 @@ impl<'tcx> Visitor<'tcx> for HasBreakOrReturnVisitor {
ExprKind::Ret(_) | ExprKind::Break(_, _) => {
self.has_break_or_return = true;
return;
},
_ => {},
}
_ => {}
}

walk_expr(self, expr);
Expand Down Expand Up @@ -2950,7 +2950,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo
for ref stmt in block.stmts {
if_chain! {
if let StmtKind::Local(
Local { pat: Pat { kind: PatKind::Binding(_, _, ident, .. ), .. },
Local { pat: Pat { hir_id: pat_id, kind: PatKind::Binding(_, _, ident, .. ), .. },
init: Some(ref init_expr), .. }
) = stmt.kind;
if let ExprKind::MethodCall(ref method_name, _, &[ref iter_source], ..) = init_expr.kind;
Expand All @@ -2964,6 +2964,16 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo
if let Some(iter_calls) = detect_iter_and_into_iters(block, *ident);
if iter_calls.len() == 1;
then {
let mut used_count_visitor = UsedCountVisitor {
cx,
id: *pat_id,
count: 0,
};
walk_block(&mut used_count_visitor, block);
if used_count_visitor.count > 1 {
return;
}

// Suggest replacing iter_call with iter_replacement, and removing stmt
let iter_call = &iter_calls[0];
span_lint_and_then(
Expand Down Expand Up @@ -3006,23 +3016,23 @@ impl IterFunction {
} else {
format!(".any(|x| x == *{})", s)
}
},
}
}
}
fn get_suggestion_text(&self) -> &'static str {
match &self.func {
IterFunctionKind::IntoIter => {
"Use the original Iterator instead of collecting it and then producing a new one"
},
}
IterFunctionKind::Len => {
"Take the original Iterator's count instead of collecting it and finding the length"
},
}
IterFunctionKind::IsEmpty => {
"Check if the original Iterator has anything instead of collecting it and seeing if it's empty"
},
}
IterFunctionKind::Contains(_) => {
"Check if the original Iterator contains an element instead of collecting then checking"
},
}
}
}
}
Expand Down Expand Up @@ -3087,6 +3097,28 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor {
}
}

struct UsedCountVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
id: HirId,
count: usize,
}

impl<'a, 'tcx> Visitor<'tcx> for UsedCountVisitor<'a, 'tcx> {
type Map = Map<'tcx>;

fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
if same_var(self.cx, expr, self.id) {
self.count += 1;
} else {
walk_expr(self, expr);
}
}

fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
}
}

/// Detect the occurrences of calls to `iter` or `into_iter` for the
/// given identifier
fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) -> Option<Vec<IterFunction>> {
Expand Down
20 changes: 20 additions & 0 deletions tests/ui/needless_collect_indirect.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,24 @@ fn main() {
let sample = vec![a.clone(), "b".to_string(), "c".to_string()];
let non_copy_contains = sample.into_iter().collect::<Vec<_>>();
non_copy_contains.contains(&a);

// Fix #5991
let vec_a = vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
let vec_b = vec_a.iter().collect::<Vec<_>>();
if vec_b.len() > 3 {}
let other_vec = vec![1, 3, 12, 4, 16, 2];
let we_got_the_same_numbers = other_vec.iter().filter(|item| vec_b.contains(item)).collect::<Vec<_>>();

// Fix #6297
let sample = [1; 5];
let multiple_indirect = sample.iter().collect::<Vec<_>>();
let sample2 = vec![2, 3];
if multiple_indirect.is_empty() {
// do something
} else {
let found = sample2
.iter()
.filter(|i| multiple_indirect.iter().any(|s| **s % **i == 0))
.collect::<Vec<_>>();
}
}

0 comments on commit 0a293bd

Please sign in to comment.