Use ControlFlow in more places #12830

blyxyas · 2024-05-21T23:51:52Z

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 :)

changelog: Improve performance, less memory use in visitors

Fixes #12829
r? @y21

y21 · 2024-05-22T13:03:20Z

Looks like this broke some tests 😅

blyxyas · 2024-05-26T17:04:18Z

I've done some changes, it doesn't break any test files but it seems to not run .fixed rustfix tests.

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

blyxyas · 2024-05-29T18:08:10Z

I've had this commit already finished for the last... 3 days? I'm not sure why I didn't commit it 😅

y21 · 2024-05-29T20:54:56Z

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

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

blyxyas · 2024-05-29T21:07:24Z

@oli-obk do you have some commentary on this weird behaviour

y21

some possible improvements :)

clippy_lints/src/if_let_mutex.rs

clippy_lints/src/loops/while_immutable_condition.rs

clippy_lints/src/if_let_mutex.rs

clippy_lints/src/unconditional_recursion.rs

y21 · 2024-05-29T21:17:26Z

btw didn't this also change for_each_expr? I could swear I looked at this PR for a very short time before and saw a change there

clippy_utils/src/visitors.rs

y21 · 2024-06-05T14:18:57Z

clippy_utils/src/visitors.rs

@@ -109,30 +109,43 @@ pub fn for_each_expr<'tcx, B, C: Continue>(
        res: Option<B>,
    }
    impl<'tcx, B, C: Continue, F: FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B, C>> Visitor<'tcx> for V<B, F> {
-        fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) {
+        type Result = ControlFlow<()>;


wondering if this can also just be ControlFlow<B> like in some of the other visitors to get rid of the res field? tho this might require some more involved changes... 🤔

I've tried this, and the generic rules in the language doesn't permit us to do it because B in the internal impl and B in the external impl could be different (even if it's never actually different) :/

I feel like this should be possible, but fine, I can also look into- and try this in a followup, separately, and we can just keep this one as is

y21 · 2024-06-05T14:49:11Z

clippy_lints/src/if_let_mutex.rs

    cx: &'a LateContext<'tcx>,
 }

 impl<'tcx> Visitor<'tcx> for OppVisitor<'_, 'tcx> {
-    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+    type Result = ControlFlow<&'tcx Expr<'tcx>>;


side note, not caused by this pr: isn't this visitor doing exactly the same thing as the other ArmVisitor in this file? 😅

I think this comment could be worth to look at as a small cleanup. But it's optional.

y21 · 2024-06-05T14:56:41Z

clippy_lints/src/if_let_mutex.rs

+                if found_mutex.is_none() {
+                    found_mutex = arm_visit.visit_expr(if_else).break_value();


this is technically a semantic change, we're no longer overriding the mutex if we found one in the then branch, that is, this no longer emits a warning:

let x = Mutex::new(42); let unrelated = Mutex::new(()); if let 42..=50 = *x.lock().unwrap() { drop(unrelated.lock()); // ... `arm_visit.found_mutex_if_same_as()` is false here } else { drop(x.lock()); // never visited };

on the other hand, the same argument can be made for the already existing logic, the current code already also has this bug if you swap the x.lock() and unrelated.lock() in the two branches, so.. 🤷 maybe we should just leave it and open an issue

I'll just keep the same behaviour, we can then talk about if the current behaviour should be like that :)

clippy_lints/src/redundant_closure_call.rs

y21 · 2024-06-05T15:09:49Z

clippy_lints/src/unconditional_recursion.rs

@@ -305,7 +304,9 @@ where
        {
            self.found_default_call = true;


same here, still has a found_* field

y21 · 2024-06-05T15:10:25Z

clippy_lints/src/unused_peekable.rs

@@ -137,14 +135,15 @@ impl<'tcx> Visitor<'tcx> for PeekableVisitor<'_, 'tcx> {
                                    && func_did == into_iter_did
                                {
                                    // Probably a for loop desugar, stop searching
-                                    return;
+                                    return ControlFlow::Continue(());
                                }

                                if args.iter().any(|arg| arg_is_mut_peekable(self.cx, arg)) {
                                    self.found_peek_call = true;


bors · 2024-06-13T10:47:59Z

☔ The latest upstream changes (presumably #12921) made this pull request unmergeable. Please resolve the merge conflicts.

y21

Two more small things (also CI is red, need to fix that too)

y21 · 2024-06-22T02:13:20Z

clippy_lints/src/loops/mut_range_bound.rs

        }
+        intravisit::walk_expr(self, expr)


This was previously inside of an else {} but isn't anymore with this change, so we descend into things when we don't need to right now

y21 · 2024-06-22T02:37:15Z

clippy_lints/src/unused_peekable.rs

@@ -70,15 +71,22 @@ impl<'tcx> LateLintPass<'tcx> for UnusedPeekable {
                    return;
                }

+                let mut found_peek_call = false;
+
                for stmt in &block.stmts[idx..] {


Could simplify this for loop into let found_peek_call = block.stmts[idx..].iter().any(|stmt| vis.visit_stmt(stmt).is_break());

bors · 2024-06-27T17:04:21Z

☔ The latest upstream changes (presumably #12999) made this pull request unmergeable. Please resolve the merge conflicts.

y21

Alright, the changes look fine to me now. But I would like another person to quickly skim over the changes here and double check since some of these visitors are used by a lot of lints and a bug here can easily cause subtle bugs.

Maybe @xFrednet? Quick tldr/context: like the description says, this adds a type Result = ControlFlow and uses Break in some visitors to short circuit them (also makes it nicer since the final value that the lints are interested in are directly in the ControlFlow::Break and don't need their own field

xFrednet · 2024-07-19T21:19:55Z

Sure, I'll have a look at it tomorrow.

xFrednet · 2024-07-19T21:20:16Z

If we rebase, we can also use our CI to check if there are any changed :3

…lFlow rust-lang#12830

xFrednet

Four smaller formatting nits. (As usual, probably a bit nitpicky ^^') The semantic changes all look good to me and I like the refactoring!

The CI is also happy and doesn't report any lint changes 👍

You can r=y21,xFrednet once you've addressed the last comments.

clippy_lints/src/loops/mut_range_bound.rs

xFrednet · 2024-07-20T12:27:36Z

clippy_lints/src/if_let_mutex.rs

    cx: &'a LateContext<'tcx>,
 }

 impl<'tcx> Visitor<'tcx> for OppVisitor<'_, 'tcx> {
-    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+    type Result = ControlFlow<&'tcx Expr<'tcx>>;


I think this comment could be worth to look at as a small cleanup. But it's optional.

clippy_lints/src/methods/option_map_unwrap_or.rs

clippy_utils/src/visitors.rs

bors · 2024-07-22T19:51:31Z

☔ The latest upstream changes (presumably #13065) made this pull request unmergeable. Please resolve the merge conflicts.

…lFlow rust-lang#12830

xFrednet · 2024-07-24T08:03:16Z

LGTM, thanks!

Roses are red,
ControlFlow is used,
Timo approves,
And I have checked :D

bors · 2024-07-24T08:03:18Z

📌 Commit b21a91a has been approved by y21,xFrednet

It is now in the queue for this repository.

bors · 2024-07-24T08:04:25Z

⌛ Testing commit b21a91a with merge fcecf3f...

Use ControlFlow in more places 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`

bors · 2024-07-24T08:04:58Z

💔 Test failed - checks-action_test

blyxyas · 2024-07-24T09:47:14Z

@bors retry

blyxyas · 2024-07-24T09:47:49Z

@bors r=y21,xFredNet

bors · 2024-07-24T09:47:51Z

💡 This pull request was already approved, no need to approve it again.

bors · 2024-07-24T09:47:52Z

📌 Commit b21a91a has been approved by y21,xFredNet

It is now in the queue for this repository.

bors · 2024-07-24T09:48:22Z

⌛ Testing commit b21a91a with merge 16953ce...

bors · 2024-07-24T09:56:30Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21,xFredNet
Pushing 16953ce to master...

Remove unnecessary `res` field in `for_each_expr` visitors Small refactor in the `for_each_expr*` visitors. This should not change anything functionally. Instead of storing the final value `Option<B>` in the visitor and setting it to `Some` when we get a `ControlFlow::Break(B)` from the closure, we can just directly return it from the visitor itself now that visitors support that. cc #12829 and #12830 (comment) changelog: none

rustbot assigned y21 May 21, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 21, 2024

blyxyas force-pushed the more-controlflow branch from b36cebd to 73db344 Compare May 21, 2024 23:52

blyxyas force-pushed the more-controlflow branch from 73db344 to 7a50e0f Compare May 26, 2024 16:52

blyxyas force-pushed the more-controlflow branch from 7a50e0f to b3f22fd Compare May 29, 2024 17:59

y21 reviewed May 29, 2024

View reviewed changes

blyxyas force-pushed the more-controlflow branch from e969126 to a8797c9 Compare June 2, 2024 21:27

blyxyas requested a review from y21 June 4, 2024 15:41

y21 reviewed Jun 5, 2024

View reviewed changes

blyxyas force-pushed the more-controlflow branch from 3d549fb to d241556 Compare June 15, 2024 15:52

blyxyas added the performance-project For issues and PRs related to the Clippy Performance Project label Jun 16, 2024

blyxyas mentioned this pull request Jun 18, 2024

Revert "Bump ui_test to 0.23" #12949

Closed

y21 reviewed Jun 22, 2024

View reviewed changes

y21 approved these changes Jul 19, 2024

View reviewed changes

xFrednet self-assigned this Jul 19, 2024

blyxyas force-pushed the more-controlflow branch from a52281b to aca0401 Compare July 20, 2024 11:17

blyxyas added a commit to blyxyas/rust-clippy that referenced this pull request Jul 20, 2024

Replace local variables signifying "done" or "loop break", use Contro…

4e222c6

…lFlow rust-lang#12830

blyxyas force-pushed the more-controlflow branch from aca0401 to 4e222c6 Compare July 20, 2024 11:20

xFrednet approved these changes Jul 20, 2024

View reviewed changes

blyxyas added a commit to blyxyas/rust-clippy that referenced this pull request Jul 23, 2024

Replace local variables signifying "done" or "loop break", use Contro…

ffa9be7

…lFlow rust-lang#12830

blyxyas force-pushed the more-controlflow branch from cfe4c15 to 5fedd60 Compare July 23, 2024 23:39

Replace local variables signifying "done" or "loop break", use Contro…

b21a91a

…lFlow rust-lang#12830

blyxyas force-pushed the more-controlflow branch from 5fedd60 to b21a91a Compare July 23, 2024 23:41

xFrednet approved these changes Jul 24, 2024

View reviewed changes

bors merged commit 16953ce into rust-lang:master Jul 24, 2024
8 checks passed

blyxyas mentioned this pull request Jul 24, 2024

Optimizing Clippy & linting rust-lang/rust-project-goals#114

Open

2 tasks

y21 mentioned this pull request Jul 25, 2024

Remove unnecessary res field in for_each_expr visitors #13156

Merged

		if found_mutex.is_none() {
		found_mutex = arm_visit.visit_expr(if_else).break_value();

Use ControlFlow in more places #12830

Use ControlFlow in more places #12830

Conversation

blyxyas commented May 21, 2024 • edited Loading

y21 commented May 22, 2024

blyxyas commented May 26, 2024 • edited Loading

blyxyas commented May 29, 2024

y21 commented May 29, 2024 • edited Loading

blyxyas commented May 29, 2024 • edited Loading

y21 left a comment

Choose a reason for hiding this comment

y21 commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jun 13, 2024

y21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jun 27, 2024

y21 left a comment • edited Loading

Choose a reason for hiding this comment

xFrednet commented Jul 19, 2024

xFrednet commented Jul 19, 2024

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 22, 2024

xFrednet commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

blyxyas commented Jul 24, 2024

blyxyas commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

blyxyas commented May 21, 2024 •

edited

Loading

blyxyas commented May 26, 2024 •

edited

Loading

y21 commented May 29, 2024 •

edited

Loading

blyxyas commented May 29, 2024 •

edited

Loading

y21 left a comment •

edited

Loading

xFrednet left a comment •

edited

Loading