fix [infinite_loop] suggestions on async funtion/closure
#12421
Conversation
rustbot
added
the
S-waiting-on-review
|
I have split the changes into 2 commits, the second one is not related to the issue, but it's something I realized when fixing it, it basically treats the explicite |
|
Hmmmm, a fairly familiar looking PR waiting to be triaged... Hey @Jarcho, do you have time to review this pile of spaghetti code 😆? You can assign someone else to do it if you don't wanna do it yourself, and I totally understand that~ |
| struct BindingVisitor<'tcx> { | ||
| res: Option<Ty<'tcx>>, | ||
| } | ||
| impl<'tcx> Visitor<'tcx> for BindingVisitor<'tcx> { | ||
| fn visit_assoc_type_binding(&mut self, type_binding: &'tcx hir::TypeBinding<'tcx>) { | ||
| if self.res.is_some() { | ||
| return; | ||
| } | ||
| if let hir::TypeBindingKind::Equality { | ||
| term: hir::Term::Ty(ty), | ||
| } = type_binding.kind | ||
| { | ||
| self.res = Some(*ty); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Visitors can return ControlFlow by setting type ResultTy = ControlFlow<T>
There was a problem hiding this comment.
I tried use this for the other visitor here as well, but sadly it doesn't work, I'm assuming it's somehow "conflicting" with those walk_expr calls?
There was a problem hiding this comment.
It would work there. walk_expr has the same return type as visit_expr. If you really want to clean that part up you can get rid the label and loop depth tracking. Destination::target_id will always hold the HirId of the loop expression or block it's targeting.
There was a problem hiding this comment.
after trying out your suggestion, I think the depth tracking is somehow needed in this case below right?
'outer: loop {
loop {
if cond {
break 'outer;
}
}
}'outer loop can be identified as finite when visitor sees the break with a matching target_id. However during the second iteration, the original logic assumes the break 'outer was breaking the current loop because the "depth" was 0.
if self.loop_depth == 0 || has_same_label() {
self.is_finite = true;
}Without the depth tracking I can't think of any better way to eliminate this FP, so I'm guessing it should be safe to assume that way?
There was a problem hiding this comment.
A small change to the example causes the current version to not work:
'outer: loop {
loop {
if cond {
loop {
break 'outer;
}
}
}
}The correct implementation would track the HirId of every loop and targeted block contained in the checked loop.
If you want to fix this feel free, otherwise leave the current code.
|
|
||
| // Since `item_id` is from a `TyKind::OpaqueDef`, | ||
| // the `Item` related to it should always be `OpaqueTy`. | ||
| assert!(matches!(opaque_ty_item.kind, ItemKind::OpaqueTy(_))); |
There was a problem hiding this comment.
Is there a reason this assert is here? An ICE isn't exactly a great outcome if it's wrong.
There was a problem hiding this comment.
hmmm, right, I guess I was being overly confident 😂. Should've be more careful with it, I'll change it~
| let Self::Return(ty) = self else { return false }; | ||
| matches!(ty.kind, TyKind::Never) |
There was a problem hiding this comment.
Could be matches!(self, Self::Return(ty) if ty.is_never()).
There was a problem hiding this comment.
ah, that's an hir::Ty, so there's no is_never calls, but I got what you mean, thanks
J-ZhengLi
force-pushed
the
issue12338
branch
2 times, most recently
from
88ea1b7 to
fc78fde
Compare
|
If you want to clean up the other visitor feel free. Otherwise I'll merge it as is. |
|
thanks for your time reviewing this~
|
J-ZhengLi
force-pushed
the
issue12338
branch
2 times, most recently
from
3867e00 to
fc78fde
Compare
add more test cases with async clousures;
|
☔ The latest upstream changes (presumably #13440) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Any changes on fixing this? |
apologies for the long wait, seems like I got stuck on trying to fix this comment #12421 (comment) which doesn't seems to be related to your issue, then I forgot about this pr's existence ... I'm splitting this PR now, sorry~ |
|
closing for now in favor of #13608. |
[`infinite_loops`]: fix incorrect suggestions on async functions/closures closes: #12338 I intend to fix this in #12421 but got distracted by some other problems in the same lint, delaying the process of closing the actual issue. So here's a separated PR that only focus on the issue and nothing else. --- changelog: [`infinite_loops`]: fix suggestion error on async functions/closures
fixes: #12338
changelog: fix [
infinite_loop] suggestions on async funtion/closure