Convert the collapsible_if early lint to a late one

Author: samueltardieu

clippy_lints/src/collapsible_if.rs

@@ -1,12 +1,10 @@
 use clippy_config::Conf;
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
-use clippy_utils::source::{
-    HasSession, IntoSpan as _, SpanRangeExt, snippet, snippet_block, snippet_block_with_applicability,
-};
-use clippy_utils::span_contains_comment;
-use rustc_ast::ast;
+use clippy_utils::source::{IntoSpan as _, SpanRangeExt, snippet, snippet_block, snippet_block_with_applicability};
+use rustc_ast::BinOpKind;
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_hir::{Block, Expr, ExprKind, StmtKind};
+use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
 use rustc_span::Span;
 
@@ -89,17 +87,16 @@ impl CollapsibleIf {
         }
     }
 
-    fn check_collapsible_else_if(cx: &EarlyContext<'_>, then_span: Span, else_: &ast::Expr) {
-        if let ast::ExprKind::Block(ref block, _) = else_.kind
-            && !block_starts_with_comment(cx, block)
-            && let Some(else_) = expr_block(block)
-            && else_.attrs.is_empty()
+    fn check_collapsible_else_if(cx: &LateContext<'_>, then_span: Span, else_block: &Block<'_>) {
+        if !block_starts_with_comment(cx, else_block)
+            && let Some(else_) = expr_block(else_block)
+            && cx.tcx.hir_attrs(else_.hir_id).is_empty()
             && !else_.span.from_expansion()
-            && let ast::ExprKind::If(..) = else_.kind
+            && let ExprKind::If(..) = else_.kind
         {
             // Prevent "elseif"
             // Check that the "else" is followed by whitespace
-            let up_to_else = then_span.between(block.span);
+            let up_to_else = then_span.between(else_block.span);
             let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() {
                 !c.is_whitespace()
             } else {
@@ -110,29 +107,28 @@ impl CollapsibleIf {
             span_lint_and_sugg(
                 cx,
                 COLLAPSIBLE_ELSE_IF,
-                block.span,
+                else_block.span,
                 "this `else { if .. }` block can be collapsed",
                 "collapse nested if block",
                 format!(
                     "{}{}",
                     if requires_space { " " } else { "" },
-                    snippet_block_with_applicability(cx, else_.span, "..", Some(block.span), &mut applicability)
+                    snippet_block_with_applicability(cx, else_.span, "..", Some(else_block.span), &mut applicability)
                 ),
                 applicability,
             );
         }
     }
 
-    fn check_collapsible_if_if(&self, cx: &EarlyContext<'_>, expr: &ast::Expr, check: &ast::Expr, then: &ast::Block) {
+    fn check_collapsible_if_if(&self, cx: &LateContext<'_>, expr: &Expr<'_>, check: &Expr<'_>, then: &Block<'_>) {
         if let Some(inner) = expr_block(then)
-            && inner.attrs.is_empty()
-            && let ast::ExprKind::If(check_inner, _, None) = &inner.kind
+            && 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, ast::ExprKind::Let(..))
+            && !matches!(check_inner.kind, ExprKind::Let(..))
             && let ctxt = expr.span.ctxt()
             && inner.span.ctxt() == ctxt
-            && let contains_comment = span_contains_comment(cx.sess().source_map(), check.span.to(check_inner.span))
-            && (!contains_comment || self.lint_commented_code)
+            && (self.lint_commented_code || !block_starts_with_comment(cx, then))
         {
             span_lint_and_then(
                 cx,
@@ -168,44 +164,54 @@ impl CollapsibleIf {
 
 impl_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF, COLLAPSIBLE_ELSE_IF]);
 
-impl EarlyLintPass for CollapsibleIf {
-    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) {
-        if let ast::ExprKind::If(cond, then, else_) = &expr.kind
+impl LateLintPass<'_> for CollapsibleIf {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        if let ExprKind::If(cond, then, else_) = &expr.kind
             && !expr.span.from_expansion()
         {
-            if let Some(else_) = else_ {
+            if let Some(else_) = else_
+                && let ExprKind::Block(else_, None) = else_.kind
+            {
                 Self::check_collapsible_else_if(cx, then.span, else_);
-            } else if !matches!(cond.kind, ast::ExprKind::Let(..)) {
+            } else if else_.is_none()
+                && !matches!(cond.kind, ExprKind::Let(..))
+                && let ExprKind::Block(then, None) = then.kind
+            {
                 // Prevent triggering on `if c { if let a = b { .. } }`.
-                self.check_collapsible_if_if(cx, expr, cond, then);
+                self.check_collapsible_if_if(cx, expr, cond, block!(then));
             }
         }
     }
 }
 
-fn block_starts_with_comment(cx: &EarlyContext<'_>, expr: &ast::Block) -> bool {
+fn block_starts_with_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
     // We trim all opening braces and whitespaces and then check if the next string is a comment.
-    let trimmed_block_text = snippet_block(cx, expr.span, "..", None)
+    let trimmed_block_text = snippet_block(cx, block.span, "..", None)
         .trim_start_matches(|c: char| c.is_whitespace() || c == '{')
         .to_owned();
     trimmed_block_text.starts_with("//") || trimmed_block_text.starts_with("/*")
 }
 
-/// If the block contains only one expression, return it.
-fn expr_block(block: &ast::Block) -> Option<&ast::Expr> {
-    if let [stmt] = &*block.stmts
-        && let ast::StmtKind::Expr(expr) | ast::StmtKind::Semi(expr) = &stmt.kind
-    {
-        Some(expr)
-    } else {
-        None
+/// If `block` is a block with either one expression or a statement containing an expression,
+/// return the expression. We don't peel blocks recursively, as extra blocks might be intentional.
+fn expr_block<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr<'tcx>> {
+    match block.stmts {
+        [] => block.expr,
+        [stmt] => {
+            if let StmtKind::Semi(expr) = stmt.kind {
+                Some(expr)
+            } else {
+                None
+            }
+        },
+        _ => None,
     }
 }
 
 /// If the expression is a `||`, suggest parentheses around it.
-fn parens_around(expr: &ast::Expr) -> Vec<(Span, String)> {
-    if let ast::ExprKind::Binary(op, _, _) = expr.kind
-        && op.node == ast::BinOpKind::Or
+fn parens_around(expr: &Expr<'_>) -> Vec<(Span, String)> {
+    if let ExprKind::Binary(op, _, _) = expr.peel_drop_temps().kind
+        && op.node == BinOpKind::Or
     {
         vec![
             (expr.span.shrink_to_lo(), String::from("(")),

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_early_pass(move || Box::new(collapsible_if::CollapsibleIf::new(conf)));
+    store.register_late_pass(move |_| Box::new(collapsible_if::CollapsibleIf::new(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));