Use ControlFlow in more places #12830
Conversation
rustbot
added
the
S-waiting-on-review
|
Looks like this broke some tests 😅 |
|
I've done some changes, it doesn't break any test files but it seems to not run EDIT: On CI it seems like there aren't any issues, could you test the patch on your PC to check? Maybe it's just my workflow |
|
I've had this commit already finished for the last... 3 days? I'm not sure why I didn't commit it 😅 |
Oh sorry, didn't see this edit. Huh... yeah, I'm seeing something similar. I don't think this is something caused by this PR though. It also seems to happen on master for me. Anyway, I'll try taking a closer look at the changes in a bit |
|
@oli-obk do you have some commentary on this weird behaviour |
clippy_lints/src/if_let_mutex.rs
Outdated
| visit::walk_expr(self, expr); | ||
| ControlFlow::Continue(()) |
There was a problem hiding this comment.
| visit::walk_expr(self, expr); | |
| ControlFlow::Continue(()) | |
| visit::walk_expr(self, expr) |
This is currently ignoring Breaks that happen in nested visits. We should propagate them here.
This is in a bunch of places in this PR, can you fix the other instances of walk_*(); Continue(()) also?
| } | ||
|
|
||
| type Result = ControlFlow<()>; | ||
| fn visit_expr(&mut self, expr: &'tcx Expr<'_>) -> ControlFlow<()> { | ||
| match expr.kind { | ||
| ExprKind::Ret(_) | ExprKind::Break(_, _) => { | ||
| self.has_break_or_return = true; |
There was a problem hiding this comment.
I think we can even remove this bool from the struct, by using .is_break() on the visit result
There was a problem hiding this comment.
same in option_map_unwrap_or.rs and lifetimes.rs
clippy_lints/src/if_let_mutex.rs
Outdated
| @@ -90,12 +91,14 @@ pub struct OppVisitor<'a, 'tcx> { | |||
| } | |||
|
|
|||
| impl<'tcx> Visitor<'tcx> for OppVisitor<'_, 'tcx> { | |||
| fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { | |||
| type Result = ControlFlow<()>; | |||
There was a problem hiding this comment.
This could be ControlFlow<&'tcx Expr<'tcx>> with Break(mutex). (Also gets rid of the found_mutex field)
| if self.found_default_call { | ||
| return; | ||
| } | ||
| fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) -> ControlFlow<()> { | ||
| walk_expr(self, expr); |
There was a problem hiding this comment.
| walk_expr(self, expr); | |
| walk_expr(self, expr)?; |
propagating any Breaks
There was a problem hiding this comment.
pls also double check that the Breaks from the other walk_* calls are propagated too in case I missed one :3
There was a problem hiding this comment.
I'm having some problems with this suggestion. I'm having this problem with other walk_exprs:
error[E0308]: `?` operator has incompatible types
--> clippy_lints/src/if_let_mutex.rs:100:9
|
96 | ...n visit_expr(&mut self, expr: &'tcx Expr<'_>) -> ControlFlow<&'tcx Expr<'tcx>...
| ----------------------------- expected `ControlFlow<&'tcx rustc_hir::Expr<'tcx>>` because of return type
...
100 | ... visit::walk_expr(self, expr)?
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
| | |
| | help: try removing this `?`
| expected `ControlFlow<&Expr<'_>>`, found `()`
|
= note: `?` operator cannot convert from `()` to `ControlFlow<&'tcx rustc_hir::Expr<'tcx>>`
= note: expected enum `ControlFlow<&'tcx rustc_hir::Expr<'tcx>>`
found unit type `()`
There was a problem hiding this comment.
Ah, my second comment might've been confusing. It was unrelated to the ? (just meant that we should make sure all ControlFlow::Breaks are properly handled, didn't mean to imply that we should use ? everywhere).
Looks like the ? is only needed in unconditional_recursion specifically, since it would otherwise be ignored.
In the if_let_mutex case (and all other cases where walk_expr is the last expression in the function) just walk_expr(self, expr) without the ? should be enough since its immediately returned.
|
btw didn't this also change |
Now, instead of manually using variables in visitors to signify that a visit is "done" and that the visitor should stop traversing. We use the trait type "Result" to signify this (in relevant places).
I'll schedule a perf run, I don't think it will be much of a difference, but every bit of performance is welcomed :)
Fixes #12829
r? @y21