Make collapsible_if recognize the let_chains feature

samueltardieu · samueltardieu · commit cd701524701b · 2025-03-27T14:40:44.000+01:00
Until `if let` chains are stabilized, we do not collapse them together
or with other `if` expressions unless the `let_chains` feature is
enabled. This is the case for example in Clippy sources.
diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs
@@ -5,6 +5,7 @@ use rustc_ast::BinOpKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Block, Expr, ExprKind, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::TyCtxt;
 use rustc_session::impl_lint_pass;
 use rustc_span::Span;
 
@@ -77,12 +78,14 @@ declare_clippy_lint! {
 }
 
 pub struct CollapsibleIf {
+    let_chains_enabled: bool,
     lint_commented_code: bool,
 }
 
 impl CollapsibleIf {
-    pub fn new(conf: &'static Conf) -> Self {
+    pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self {
         Self {
+            let_chains_enabled: tcx.features().let_chains(),
             lint_commented_code: conf.lint_commented_code,
         }
     }
@@ -124,8 +127,7 @@ impl CollapsibleIf {
         if let Some(inner) = expr_block(then)
             && cx.tcx.hir_attrs(inner.hir_id).is_empty()
             && let ExprKind::If(check_inner, _, None) = &inner.kind
-            // Prevent triggering on `if c { if let a = b { .. } }`.
-            && !matches!(check_inner.kind, ExprKind::Let(..))
+            && self.eligible_condition(check_inner)
             && let ctxt = expr.span.ctxt()
             && inner.span.ctxt() == ctxt
             && (self.lint_commented_code || !block_starts_with_comment(cx, then))
@@ -160,6 +162,10 @@ impl CollapsibleIf {
             );
         }
     }
+
+    pub fn eligible_condition(&self, cond: &Expr<'_>) -> bool {
+        self.let_chains_enabled || !matches!(cond.kind, ExprKind::Let(..))
+    }
 }
 
 impl_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF, COLLAPSIBLE_ELSE_IF]);
@@ -174,11 +180,10 @@ impl LateLintPass<'_> for CollapsibleIf {
             {
                 Self::check_collapsible_else_if(cx, then.span, else_);
             } else if else_.is_none()
-                && !matches!(cond.kind, ExprKind::Let(..))
+                && self.eligible_condition(cond)
                 && let ExprKind::Block(then, None) = then.kind
             {
-                // Prevent triggering on `if c { if let a = b { .. } }`.
-                self.check_collapsible_if_if(cx, expr, cond, block!(then));
+                self.check_collapsible_if_if(cx, expr, cond, then);
             }
         }
     }
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -771,7 +771,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(redundant_closure_call::RedundantClosureCall));
     store.register_early_pass(|| Box::new(unused_unit::UnusedUnit));
     store.register_late_pass(|_| Box::new(returns::Return));
-    store.register_late_pass(move |_| Box::new(collapsible_if::CollapsibleIf::new(conf)));
+    store.register_late_pass(move |tcx| Box::new(collapsible_if::CollapsibleIf::new(tcx, conf)));
     store.register_late_pass(|_| Box::new(items_after_statements::ItemsAfterStatements));
     store.register_early_pass(|| Box::new(precedence::Precedence));
     store.register_late_pass(|_| Box::new(needless_parens_on_range_literals::NeedlessParensOnRangeLiterals));
diff --git a/tests/ui-toml/collapsible_if/collapsible_if_let_chains.fixed b/tests/ui-toml/collapsible_if/collapsible_if_let_chains.fixed
@@ -0,0 +1,25 @@
+#![feature(let_chains)]
+#![warn(clippy::collapsible_if)]
+
+fn main() {
+    if let Some(a) = Some(3)
+        // with comment
+        && let Some(b) = Some(4) {
+            let _ = a + b;
+        }
+    //~^^^^^^ collapsible_if
+
+    if let Some(a) = Some(3)
+        // with comment
+        && a + 1 == 4 {
+            let _ = a;
+        }
+    //~^^^^^^ collapsible_if
+
+    if Some(3) == Some(4).map(|x| x - 1)
+        // with comment
+        && let Some(b) = Some(4) {
+            let _ = b;
+        }
+    //~^^^^^^ collapsible_if
+}
diff --git a/tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs b/tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs
@@ -0,0 +1,28 @@
+#![feature(let_chains)]
+#![warn(clippy::collapsible_if)]
+
+fn main() {
+    if let Some(a) = Some(3) {
+        // with comment
+        if let Some(b) = Some(4) {
+            let _ = a + b;
+        }
+    }
+    //~^^^^^^ collapsible_if
+
+    if let Some(a) = Some(3) {
+        // with comment
+        if a + 1 == 4 {
+            let _ = a;
+        }
+    }
+    //~^^^^^^ collapsible_if
+
+    if Some(3) == Some(4).map(|x| x - 1) {
+        // with comment
+        if let Some(b) = Some(4) {
+            let _ = b;
+        }
+    }
+    //~^^^^^^ collapsible_if
+}
diff --git a/tests/ui-toml/collapsible_if/collapsible_if_let_chains.stderr b/tests/ui-toml/collapsible_if/collapsible_if_let_chains.stderr
@@ -0,0 +1,64 @@
+error: this `if` statement can be collapsed
+  --> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:5:5
+   |
+LL | /     if let Some(a) = Some(3) {
+LL | |         // with comment
+LL | |         if let Some(b) = Some(4) {
+LL | |             let _ = a + b;
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+   = note: `-D clippy::collapsible-if` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::collapsible_if)]`
+help: collapse nested if block
+   |
+LL ~     if let Some(a) = Some(3)
+LL |         // with comment
+LL ~         && let Some(b) = Some(4) {
+LL |             let _ = a + b;
+LL ~         }
+   |
+
+error: this `if` statement can be collapsed
+  --> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:13:5
+   |
+LL | /     if let Some(a) = Some(3) {
+LL | |         // with comment
+LL | |         if a + 1 == 4 {
+LL | |             let _ = a;
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     if let Some(a) = Some(3)
+LL |         // with comment
+LL ~         && a + 1 == 4 {
+LL |             let _ = a;
+LL ~         }
+   |
+
+error: this `if` statement can be collapsed
+  --> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:21:5
+   |
+LL | /     if Some(3) == Some(4).map(|x| x - 1) {
+LL | |         // with comment
+LL | |         if let Some(b) = Some(4) {
+LL | |             let _ = b;
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     if Some(3) == Some(4).map(|x| x - 1)
+LL |         // with comment
+LL ~         && let Some(b) = Some(4) {
+LL |             let _ = b;
+LL ~         }
+   |
+
+error: aborting due to 3 previous errors
+
diff --git a/tests/ui/auxiliary/proc_macros.rs b/tests/ui/auxiliary/proc_macros.rs
@@ -131,12 +131,12 @@ fn write_with_span(s: Span, mut input: IntoIter, out: &mut TokenStream) -> Resul
 pub fn make_it_big(input: TokenStream) -> TokenStream {
     let mut expr_repeat = syn::parse_macro_input!(input as syn::ExprRepeat);
     let len_span = expr_repeat.len.span();
-    if let syn::Expr::Lit(expr_lit) = &mut *expr_repeat.len {
-        if let syn::Lit::Int(lit_int) = &expr_lit.lit {
-            let orig_val = lit_int.base10_parse::<usize>().expect("not a valid length parameter");
-            let new_val = orig_val.saturating_mul(10);
-            expr_lit.lit = syn::parse_quote_spanned!( len_span => #new_val);
-        }
+    if let syn::Expr::Lit(expr_lit) = &mut *expr_repeat.len
+        && let syn::Lit::Int(lit_int) = &expr_lit.lit
+    {
+        let orig_val = lit_int.base10_parse::<usize>().expect("not a valid length parameter");
+        let new_val = orig_val.saturating_mul(10);
+        expr_lit.lit = syn::parse_quote_spanned!( len_span => #new_val);
     }
     quote::quote!(#expr_repeat).into()
 }
diff --git a/tests/ui/collapsible_if_let_chains.fixed b/tests/ui/collapsible_if_let_chains.fixed
@@ -0,0 +1,29 @@
+#![feature(let_chains)]
+#![warn(clippy::collapsible_if)]
+
+fn main() {
+    if let Some(a) = Some(3) {
+        // with comment, so do not lint
+        if let Some(b) = Some(4) {
+            let _ = a + b;
+        }
+    }
+
+    if let Some(a) = Some(3)
+        && let Some(b) = Some(4) {
+            let _ = a + b;
+        }
+    //~^^^^^ collapsible_if
+
+    if let Some(a) = Some(3)
+        && a + 1 == 4 {
+            let _ = a;
+        }
+    //~^^^^^ collapsible_if
+
+    if Some(3) == Some(4).map(|x| x - 1)
+        && let Some(b) = Some(4) {
+            let _ = b;
+        }
+    //~^^^^^ collapsible_if
+}
diff --git a/tests/ui/collapsible_if_let_chains.rs b/tests/ui/collapsible_if_let_chains.rs
@@ -0,0 +1,32 @@
+#![feature(let_chains)]
+#![warn(clippy::collapsible_if)]
+
+fn main() {
+    if let Some(a) = Some(3) {
+        // with comment, so do not lint
+        if let Some(b) = Some(4) {
+            let _ = a + b;
+        }
+    }
+
+    if let Some(a) = Some(3) {
+        if let Some(b) = Some(4) {
+            let _ = a + b;
+        }
+    }
+    //~^^^^^ collapsible_if
+
+    if let Some(a) = Some(3) {
+        if a + 1 == 4 {
+            let _ = a;
+        }
+    }
+    //~^^^^^ collapsible_if
+
+    if Some(3) == Some(4).map(|x| x - 1) {
+        if let Some(b) = Some(4) {
+            let _ = b;
+        }
+    }
+    //~^^^^^ collapsible_if
+}
diff --git a/tests/ui/collapsible_if_let_chains.stderr b/tests/ui/collapsible_if_let_chains.stderr
@@ -0,0 +1,58 @@
+error: this `if` statement can be collapsed
+  --> tests/ui/collapsible_if_let_chains.rs:12:5
+   |
+LL | /     if let Some(a) = Some(3) {
+LL | |         if let Some(b) = Some(4) {
+LL | |             let _ = a + b;
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+   = note: `-D clippy::collapsible-if` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::collapsible_if)]`
+help: collapse nested if block
+   |
+LL ~     if let Some(a) = Some(3)
+LL ~         && let Some(b) = Some(4) {
+LL |             let _ = a + b;
+LL ~         }
+   |
+
+error: this `if` statement can be collapsed
+  --> tests/ui/collapsible_if_let_chains.rs:19:5
+   |
+LL | /     if let Some(a) = Some(3) {
+LL | |         if a + 1 == 4 {
+LL | |             let _ = a;
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     if let Some(a) = Some(3)
+LL ~         && a + 1 == 4 {
+LL |             let _ = a;
+LL ~         }
+   |
+
+error: this `if` statement can be collapsed
+  --> tests/ui/collapsible_if_let_chains.rs:26:5
+   |
+LL | /     if Some(3) == Some(4).map(|x| x - 1) {
+LL | |         if let Some(b) = Some(4) {
+LL | |             let _ = b;
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     if Some(3) == Some(4).map(|x| x - 1)
+LL ~         && let Some(b) = Some(4) {
+LL |             let _ = b;
+LL ~         }
+   |
+
+error: aborting due to 3 previous errors
+

Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,7 @@ use rustc_ast::BinOpKind;`
`5`	`5`	`use rustc_errors::Applicability;`
`6`	`6`	`use rustc_hir::{Block, Expr, ExprKind, StmtKind};`
`7`	`7`	`use rustc_lint::{LateContext, LateLintPass};`
	`8`	`+use rustc_middle::ty::TyCtxt;`
`8`	`9`	`use rustc_session::impl_lint_pass;`
`9`	`10`	`use rustc_span::Span;`
`10`	`11`
`@@ -77,12 +78,14 @@ declare_clippy_lint! {`
`77`	`78`	`}`
`78`	`79`
`79`	`80`	`pub struct CollapsibleIf {`
	`81`	`+ let_chains_enabled: bool,`
`80`	`82`	`lint_commented_code: bool,`
`81`	`83`	`}`
`82`	`84`
`83`	`85`	`impl CollapsibleIf {`
`84`		`- pub fn new(conf: &'static Conf) -> Self {`
	`86`	`+ pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self {`
`85`	`87`	`Self {`
	`88`	`+ let_chains_enabled: tcx.features().let_chains(),`
`86`	`89`	`lint_commented_code: conf.lint_commented_code,`
`87`	`90`	`}`
`88`	`91`	`}`
`@@ -124,8 +127,7 @@ impl CollapsibleIf {`
`124`	`127`	`if let Some(inner) = expr_block(then)`
`125`	`128`	`&& cx.tcx.hir_attrs(inner.hir_id).is_empty()`
`126`	`129`	`&& let ExprKind::If(check_inner, _, None) = &inner.kind`
`127`		- // Prevent triggering on `if c { if let a = b { .. } }`.
`128`		`- && !matches!(check_inner.kind, ExprKind::Let(..))`
	`130`	`+ && self.eligible_condition(check_inner)`
`129`	`131`	`&& let ctxt = expr.span.ctxt()`
`130`	`132`	`&& inner.span.ctxt() == ctxt`
`131`	`133`	`&& (self.lint_commented_code \|\| !block_starts_with_comment(cx, then))`
`@@ -160,6 +162,10 @@ impl CollapsibleIf {`
`160`	`162`	`);`
`161`	`163`	`}`
`162`	`164`	`}`
	`165`	`+`
	`166`	`+ pub fn eligible_condition(&self, cond: &Expr<'_>) -> bool {`
	`167`	`+ self.let_chains_enabled \|\| !matches!(cond.kind, ExprKind::Let(..))`
	`168`	`+ }`
`163`	`169`	`}`
`164`	`170`
`165`	`171`	`impl_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF, COLLAPSIBLE_ELSE_IF]);`
`@@ -174,11 +180,10 @@ impl LateLintPass<'_> for CollapsibleIf {`
`174`	`180`	`{`
`175`	`181`	`Self::check_collapsible_else_if(cx, then.span, else_);`
`176`	`182`	`} else if else_.is_none()`
`177`		`- && !matches!(cond.kind, ExprKind::Let(..))`
	`183`	`+ && self.eligible_condition(cond)`
`178`	`184`	`&& let ExprKind::Block(then, None) = then.kind`
`179`	`185`	`{`
`180`		- // Prevent triggering on `if c { if let a = b { .. } }`.
`181`		`- self.check_collapsible_if_if(cx, expr, cond, block!(then));`
	`186`	`+ self.check_collapsible_if_if(cx, expr, cond, then);`
`182`	`187`	`}`
`183`	`188`	`}`
`184`	`189`	`}`