expending lint [`blocks_in_if_conditions`] to check match expr as well #11853

J-ZhengLi · 2023-11-22T01:15:06Z

changelog: rename lint blocks_in_if_conditions to [blocks_in_conditions] and expand it to check blocks in match scrutinees

rustbot · 2023-11-22T01:15:11Z

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

J-ZhengLi · 2023-11-22T01:36:14Z

clippy_lints/src/blocks_in_if_conditions.rs

+        let Some((cond, keyword)) = higher::If::hir(expr).map(|hif| (hif.cond, "if")).or(
+            if let ExprKind::Match(match_ex, _, MatchSource::Normal) = expr.kind {
+                Some((match_ex, "match"))
+            } else {
+                None
+            },
+        ) else {
+            return;
+        };


Hmmm, the rendering is messed up by auto formatting,
but here is what has changed, hoping this will make things a bit clearer

J-ZhengLi · 2023-11-22T01:36:31Z

clippy_lints/src/blocks_in_if_conditions.rs

@@ -52,88 +52,89 @@ impl<'tcx> LateLintPass<'tcx> for BlocksInIfConditions {
        if in_external_macro(cx.sess(), expr.span) {
            return;
        }
-        if let Some(higher::If { cond, .. }) = higher::If::hir(expr) {


replaced by the above statement

J-ZhengLi · 2023-11-22T01:37:10Z

clippy_lints/src/blocks_in_if_conditions.rs

+                        COMPLEX_BLOCK_MESSAGE,
+                        "try",
+                        format!(
+                            "let res = {}; {keyword} res",


if -> {keyword}

llogiq

This looks mostly good, but the error message for matches needs some work. Also we may want to rename the lint to block_in_condition or some such, because it now isn't restricted to if conditions.

llogiq · 2023-11-29T07:53:24Z

clippy_lints/src/blocks_in_if_conditions.rs

                            format!(
-                                "let res = {}; if res",
+                                "{}",
                                snippet_block_with_applicability(
                                    cx,
-                                    block.span,
+                                    ex.span,
                                    "..",
                                    Some(expr.span),
                                    &mut applicability
-                                ),
+                                )
                            ),


This format!("{}", _) can be reduced to _.to_string().

thanks, kinda weird this wasn't detected by useless_format and... me 😆

llogiq · 2023-11-29T07:55:17Z

tests/ui/blocks_in_if_conditions.stderr

@@ -32,5 +32,22 @@ LL |     if true && x == 3 { 6 } else { 10 }
   = note: `-D clippy::nonminimal-bool` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]`

-error: aborting due to 3 previous errors
+error: in an `if` condition, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`


We should also change the error message for match, as there is no if condition. Instead let's call it match scrutinee.

llogiq · 2023-11-29T07:56:06Z

tests/ui/blocks_in_if_conditions.rs

@@ -61,4 +61,16 @@ fn block_in_assert() {
    );
 }

+// issue #11814
+fn block_in_match_expr(num: i32) -> i32 {
+    match {


I'd like to see a test with an unsafe block, too (just so be sure it's handled correctly).

J-ZhengLi · 2023-11-30T03:35:32Z

... we may want to rename the lint to block_in_condition or some such

I agree, I'll use that name as I couldn't think of a better name~

just find it interesting that this lint has been renamed from block_in_if_condition_expr to block_in_if_condition, and now it's becoming block_in_condition 🤣 , maybe one day it'll becomes just "block"

~~Oof, changing the name causes a lot damage~~

add more test cases with `match`; minor fixes in message output regarding review feedback

llogiq · 2023-12-02T14:02:39Z

Thank you, this looks good to merge now.

@bors r+

bors · 2023-12-02T14:02:41Z

📌 Commit 40b558a has been approved by llogiq

It is now in the queue for this repository.

bors · 2023-12-02T14:03:48Z

⌛ Testing commit 40b558a with merge 75bdbfc...

bors · 2023-12-02T14:10:37Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 75bdbfc to master...

``` warning: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let` --> init/src/main.rs:127:23 | 127 | match (|| { | _______________________^ 128 | | let entry = entry.context("read_dir(/bin) failed")?; 129 | | let path = entry.path(); 130 | | let status = std::process::Command::new(&path) ... | 139 | | } 140 | | })() { | |_____________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_conditions = note: `#[warn(clippy::blocks_in_conditions)]` on by default ``` rust-lang/rust-clippy#11853 landed in nightly.

rustbot assigned llogiq Nov 22, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 22, 2023

J-ZhengLi force-pushed the issue11814 branch from 53fecbd to b747ee2 Compare November 22, 2023 01:29

J-ZhengLi commented Nov 22, 2023

View reviewed changes

llogiq reviewed Nov 29, 2023

View reviewed changes

J-ZhengLi force-pushed the issue11814 branch from 2f8846c to 6084294 Compare November 30, 2023 06:43

expending lint [blocks_in_if_conditions] to check match expr as well

fff7aa0

J-ZhengLi force-pushed the issue11814 branch 4 times, most recently from 8a1bcaf to 40b558a Compare November 30, 2023 07:41

rename [blocks_in_if_conditions] to [blocks_in_conditions];

40b558a

add more test cases with `match`; minor fixes in message output regarding review feedback

bors merged commit 75bdbfc into rust-lang:master Dec 2, 2023
8 checks passed

tamird mentioned this pull request Dec 18, 2023

init: appease clippy aya-rs/aya#855

Merged

xxchan mentioned this pull request Dec 26, 2023

await triggers blocks_in_conditions #12016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expending lint [`blocks_in_if_conditions`] to check match expr as well #11853

expending lint [`blocks_in_if_conditions`] to check match expr as well #11853

J-ZhengLi commented Nov 22, 2023 •

edited by llogiq

rustbot commented Nov 22, 2023

J-ZhengLi Nov 22, 2023 •

edited

J-ZhengLi Nov 22, 2023 •

edited

J-ZhengLi Nov 22, 2023

llogiq left a comment

llogiq Nov 29, 2023

J-ZhengLi Nov 30, 2023 •

edited

llogiq Nov 29, 2023

llogiq Nov 29, 2023

J-ZhengLi commented Nov 30, 2023 •

edited

llogiq commented Dec 2, 2023

bors commented Dec 2, 2023

bors commented Dec 2, 2023

bors commented Dec 2, 2023

expending lint [blocks_in_if_conditions] to check match expr as well #11853

expending lint [blocks_in_if_conditions] to check match expr as well #11853

Conversation

J-ZhengLi commented Nov 22, 2023 • edited by llogiq

rustbot commented Nov 22, 2023

J-ZhengLi Nov 22, 2023 • edited

Choose a reason for hiding this comment

J-ZhengLi Nov 22, 2023 • edited

Choose a reason for hiding this comment

J-ZhengLi Nov 22, 2023

Choose a reason for hiding this comment

llogiq left a comment

Choose a reason for hiding this comment

llogiq Nov 29, 2023

Choose a reason for hiding this comment

J-ZhengLi Nov 30, 2023 • edited

Choose a reason for hiding this comment

llogiq Nov 29, 2023

Choose a reason for hiding this comment

llogiq Nov 29, 2023

Choose a reason for hiding this comment

J-ZhengLi commented Nov 30, 2023 • edited

llogiq commented Dec 2, 2023

bors commented Dec 2, 2023

bors commented Dec 2, 2023

bors commented Dec 2, 2023

expending lint [`blocks_in_if_conditions`] to check match expr as well #11853

expending lint [`blocks_in_if_conditions`] to check match expr as well #11853

J-ZhengLi commented Nov 22, 2023 •

edited by llogiq

J-ZhengLi Nov 22, 2023 •

edited

J-ZhengLi Nov 22, 2023 •

edited

J-ZhengLi Nov 30, 2023 •

edited

J-ZhengLi commented Nov 30, 2023 •

edited