Skip to content

Commit 124bb03

Browse files
buttercrabbuttercrab
committed
changed algorithm
1 parent 8faa938 commit 124bb03

File tree

1 file changed

+49
-38
lines changed

1 file changed

+49
-38
lines changed

‎clippy_lints/src/only_used_in_recursion.rs

Lines changed: 49 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -126,29 +126,36 @@ impl<'tcx> LateLintPass<'tcx> for OnlyUsedInRecursion {
126126
}
127127
}
128128

129-
let mut pre_order = FxHashMap::default();
130-
131-
visitor.graph.iter().for_each(|(_, next)| {
132-
next.iter().for_each(|i| {
133-
*pre_order.entry(*i).or_insert(0) += 1;
134-
});
135-
});
136-
137129
for (id, span, ident) in param_span {
138130
// if the variable is not used in recursion, it would be marked as unused
139-
if !visitor.has_side_effect.contains(&id)
140-
&& *pre_order.get(&id).unwrap_or(&0) > 0
141-
&& visitor.graph.contains_key(&id)
142-
{
143-
span_lint_and_sugg(
144-
cx,
145-
ONLY_USED_IN_RECURSION,
146-
span,
147-
"parameter is only used in recursion with no side-effects",
148-
"if this is intentional, prefix with an underscore",
149-
format!("_{}", ident.name.as_str()),
150-
Applicability::MaybeIncorrect,
151-
);
131+
if !visitor.has_side_effect.contains(&id) {
132+
let mut queue = VecDeque::new();
133+
let mut visited = FxHashSet::default();
134+
135+
queue.push_back(id);
136+
137+
while let Some(id) = queue.pop_front() {
138+
if let Some(next) = visitor.graph.get(&id) {
139+
for i in next {
140+
if !visited.contains(i) {
141+
visited.insert(id);
142+
queue.push_back(*i);
143+
}
144+
}
145+
}
146+
}
147+
148+
if visited.contains(&id) {
149+
span_lint_and_sugg(
150+
cx,
151+
ONLY_USED_IN_RECURSION,
152+
span,
153+
"parameter is only used in recursion with no side-effects",
154+
"if this is intentional, prefix with an underscore",
155+
format!("_{}", ident.name.as_str()),
156+
Applicability::MaybeIncorrect,
157+
);
158+
}
152159
}
153160
}
154161
}
@@ -200,7 +207,7 @@ impl<'tcx> Visitor<'tcx> for SideEffectVisit<'tcx> {
200207
StmtKind::Local(Local {
201208
pat, init: Some(init), ..
202209
}) => {
203-
self.visit_pat_expr(pat, init);
210+
self.visit_pat_expr(pat, init, false);
204211
},
205212
StmtKind::Item(i) => {
206213
let item = self.ty_ctx.hir().item(i);
@@ -229,7 +236,7 @@ impl<'tcx> Visitor<'tcx> for SideEffectVisit<'tcx> {
229236
self.visit_bin_op(lhs, rhs);
230237
},
231238
ExprKind::Unary(op, expr) => self.visit_un_op(op, expr),
232-
ExprKind::Let(Let { pat, init, .. }) => self.visit_pat_expr(pat, init),
239+
ExprKind::Let(Let { pat, init, .. }) => self.visit_pat_expr(pat, init, false),
233240
ExprKind::If(bind, then_expr, else_expr) => {
234241
self.visit_if(bind, then_expr, else_expr);
235242
},
@@ -333,7 +340,7 @@ impl<'tcx> SideEffectVisit<'tcx> {
333340
let lhs_vars = std::mem::take(&mut self.ret_vars);
334341
self.visit_expr(rhs);
335342
let rhs_vars = std::mem::take(&mut self.ret_vars);
336-
self.connect_assign(&lhs_vars, &rhs_vars);
343+
self.connect_assign(&lhs_vars, &rhs_vars, false);
337344
},
338345
}
339346
}
@@ -373,26 +380,26 @@ impl<'tcx> SideEffectVisit<'tcx> {
373380
}
374381
}
375382

376-
fn visit_pat_expr(&mut self, pat: &'tcx Pat<'tcx>, expr: &'tcx Expr<'tcx>) {
383+
fn visit_pat_expr(&mut self, pat: &'tcx Pat<'tcx>, expr: &'tcx Expr<'tcx>, connect_self: bool) {
377384
match (&pat.kind, &expr.kind) {
378385
(PatKind::Tuple(pats, _), ExprKind::Tup(exprs)) => {
379386
self.ret_vars = izip!(*pats, *exprs)
380387
.flat_map(|(pat, expr)| {
381-
self.visit_pat_expr(pat, expr);
388+
self.visit_pat_expr(pat, expr, connect_self);
382389
std::mem::take(&mut self.ret_vars)
383390
})
384391
.collect();
385392
},
386393
(PatKind::Slice(front_exprs, _, back_exprs), ExprKind::Array(exprs)) => {
387394
let mut vars = izip!(*front_exprs, *exprs)
388395
.flat_map(|(pat, expr)| {
389-
self.visit_pat_expr(pat, expr);
396+
self.visit_pat_expr(pat, expr, connect_self);
390397
std::mem::take(&mut self.ret_vars)
391398
})
392399
.collect();
393400
self.ret_vars = izip!(back_exprs.iter().rev(), exprs.iter().rev())
394401
.flat_map(|(pat, expr)| {
395-
self.visit_pat_expr(pat, expr);
402+
self.visit_pat_expr(pat, expr, connect_self);
396403
std::mem::take(&mut self.ret_vars)
397404
})
398405
.collect();
@@ -403,7 +410,7 @@ impl<'tcx> SideEffectVisit<'tcx> {
403410
pat.each_binding(|_, id, _, _| lhs_vars.push((id, false)));
404411
self.visit_expr(expr);
405412
let rhs_vars = std::mem::take(&mut self.ret_vars);
406-
self.connect_assign(&lhs_vars, &rhs_vars);
413+
self.connect_assign(&lhs_vars, &rhs_vars, connect_self);
407414
self.ret_vars = rhs_vars;
408415
},
409416
}
@@ -424,7 +431,7 @@ impl<'tcx> SideEffectVisit<'tcx> {
424431
then {
425432
izip!(self.params.clone(), args)
426433
.for_each(|(pat, expr)| {
427-
self.visit_pat_expr(pat, expr);
434+
self.visit_pat_expr(pat, expr, true);
428435
self.ret_vars.clear();
429436
});
430437
} else {
@@ -463,7 +470,7 @@ impl<'tcx> SideEffectVisit<'tcx> {
463470
then {
464471
izip!(self.params.clone(), args.iter())
465472
.for_each(|(pat, expr)| {
466-
self.visit_pat_expr(pat, expr);
473+
self.visit_pat_expr(pat, expr, true);
467474
self.ret_vars.clear();
468475
});
469476
} else {
@@ -511,7 +518,7 @@ impl<'tcx> SideEffectVisit<'tcx> {
511518
self.contains_side_effect = false;
512519
// this would visit `expr` multiple times
513520
// but couldn't think of a better way
514-
self.visit_pat_expr(arm.pat, expr);
521+
self.visit_pat_expr(arm.pat, expr, false);
515522
let mut vars = std::mem::take(&mut self.ret_vars);
516523
let _ = arm.guard.as_ref().map(|guard| {
517524
self.visit_expr(match guard {
@@ -532,7 +539,7 @@ impl<'tcx> SideEffectVisit<'tcx> {
532539
self.ret_vars.append(&mut expr_vars);
533540
}
534541

535-
fn connect_assign(&mut self, lhs: &[(HirId, bool)], rhs: &[(HirId, bool)]) {
542+
fn connect_assign(&mut self, lhs: &[(HirId, bool)], rhs: &[(HirId, bool)], connect_self: bool) {
536543
// if mutable dereference is on assignment it can have side-effect
537544
// (this can lead to parameter mutable dereference and change the original value)
538545
// too hard to detect whether this value is from parameter, so this would all
@@ -554,15 +561,19 @@ impl<'tcx> SideEffectVisit<'tcx> {
554561
// unwrap is possible since rhs is not empty
555562
let rhs_first = rhs.first().unwrap();
556563
for (id, _) in lhs.iter() {
557-
self.graph
558-
.entry(*id)
559-
.or_insert_with(FxHashSet::default)
560-
.insert(rhs_first.0);
564+
if connect_self || *id != rhs_first.0 {
565+
self.graph
566+
.entry(*id)
567+
.or_insert_with(FxHashSet::default)
568+
.insert(rhs_first.0);
569+
}
561570
}
562571

563572
let rhs = rhs.iter();
564573
izip!(rhs.clone().cycle().skip(1), rhs).for_each(|(from, to)| {
565-
self.graph.entry(from.0).or_insert_with(FxHashSet::default).insert(to.0);
574+
if connect_self || from.0 != to.0 {
575+
self.graph.entry(from.0).or_insert_with(FxHashSet::default).insert(to.0);
576+
}
566577
});
567578
}
568579

0 commit comments

Comments
 (0)