Rewrite redundant_else as a late pass. (#17329)

The main behaviour changes are:
* Lint on blocks returning `!`. This greatly increases the number of
cases this lints on.
* Don't lint inside a macro when the divergence comes from a different
macro.
* Suggest adding a semicolon when syntactically needed.

The old test file was a bit of a mess so it was rewritten. This was done
as the final commit and the new code passes both the old and the new
tests.

changelog: [`redundant_else`]: Take into account divergent function
calls.

Author: samueltardieu

clippy_lints/src/lib.rs

@@ -509,7 +509,6 @@ rustc_lint::early_lint_methods!(
         MiscEarlyLints: misc_early::MiscEarlyLints = misc_early::MiscEarlyLints,
         UnusedUnit: unused_unit::UnusedUnit = unused_unit::UnusedUnit,
         Precedence: precedence::Precedence = precedence::Precedence,
-        RedundantElse: redundant_else::RedundantElse = redundant_else::RedundantElse,
         NeedlessArbitrarySelfType: needless_arbitrary_self_type::NeedlessArbitrarySelfType = needless_arbitrary_self_type::NeedlessArbitrarySelfType,
         LiteralDigitGrouping: literal_representation::LiteralDigitGrouping = literal_representation::LiteralDigitGrouping::new(conf),
         DecimalLiteralRepresentation: literal_representation::DecimalLiteralRepresentation = literal_representation::DecimalLiteralRepresentation::new(conf),
@@ -862,6 +861,7 @@ rustc_lint::late_lint_methods!(
         ManualAssertEq: manual_assert_eq::ManualAssertEq = manual_assert_eq::ManualAssertEq,
         WithCapacityZero: with_capacity_zero::WithCapacityZero = with_capacity_zero::WithCapacityZero,
         RefPatterns: ref_patterns::RefPatterns = ref_patterns::RefPatterns,
+        RedundantElse: redundant_else::RedundantElse = redundant_else::RedundantElse,
         // add late passes here, used by `cargo dev new_lint`
     ]]
 );

clippy_lints/src/needless_borrows_for_generic_args.rs

@@ -356,9 +356,8 @@ fn is_mixed_projection_predicate<'tcx>(
                 },
             }
         }
-    } else {
-        false
     }
+    false
 }
 
 fn referent_used_exactly_once<'tcx>(

clippy_lints/src/redundant_else.rs

@@ -1,11 +1,12 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::{indent_of, reindent_multiline, snippet};
-use rustc_ast::ast::{Block, Expr, ExprKind, Stmt, StmtKind};
-use rustc_ast::visit::{Visitor, walk_expr};
+use clippy_utils::diagnostics::span_lint_hir_and_then;
+use clippy_utils::is_from_proc_macro;
+use clippy_utils::source::{SpanExt, indent_of, reindent_multiline};
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass, LintContext};
+use rustc_hir::{Block, Expr, ExprKind, MatchSource, Stmt, StmtKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::TypeckResults;
 use rustc_session::declare_lint_pass;
-use rustc_span::Span;
+use rustc_span::{ExpnKind, SyntaxContext};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -46,125 +47,147 @@ declare_clippy_lint! {
 
 declare_lint_pass!(RedundantElse => [REDUNDANT_ELSE]);
 
-impl EarlyLintPass for RedundantElse {
-    fn check_stmt(&mut self, cx: &EarlyContext<'_>, stmt: &Stmt) {
-        if stmt.span.in_external_macro(cx.sess().source_map()) {
-            return;
-        }
-        // Only look at expressions that are a whole statement
-        let expr: &Expr = match &stmt.kind {
-            StmtKind::Expr(expr) | StmtKind::Semi(expr) => expr,
-            _ => return,
-        };
-        // if else
-        let (mut then, mut els): (&Block, &Expr) = match &expr.kind {
-            ExprKind::If(_, then, Some(els)) => (then, els),
-            _ => return,
-        };
-        loop {
-            if !BreakVisitor::default().check_block(then) {
-                // then block does not always break
-                return;
-            }
-            match &els.kind {
-                // else if else
-                ExprKind::If(_, next_then, Some(next_els)) => {
-                    then = next_then;
-                    els = next_els;
-                },
-                // else if without else
-                ExprKind::If(..) => return,
-                // done
-                _ => break,
-            }
+impl<'tcx> LateLintPass<'tcx> for RedundantElse {
+    fn check_block_post(&mut self, cx: &LateContext<'tcx>, b: &'tcx Block<'_>) {
+        if let Some(e) = b.expr {
+            check(cx, b.span.ctxt(), false, e);
         }
+    }
 
-        let mut app = Applicability::MachineApplicable;
-        if let ExprKind::Block(block, _) = &els.kind {
-            for stmt in &block.stmts {
-                // If the `else` block contains a local binding or a macro invocation, Clippy shouldn't auto-fix it
-                if matches!(&stmt.kind, StmtKind::Let(_) | StmtKind::MacCall(_)) {
-                    app = Applicability::Unspecified;
-                    break;
-                }
-            }
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, s: &'tcx Stmt<'_>) {
+        if let StmtKind::Expr(e) | StmtKind::Semi(e) = s.kind {
+            check(cx, s.span.ctxt(), matches!(s.kind, StmtKind::Expr(_)), e);
         }
-
-        // FIXME: The indentation of the suggestion would be the same as the one of the macro invocation in this implementation, see https://github.com/rust-lang/rust-clippy/pull/13936#issuecomment-2569548202
-        span_lint_and_sugg(
-            cx,
-            REDUNDANT_ELSE,
-            els.span.with_lo(then.span.hi()),
-            "redundant else block",
-            "remove the `else` block and move the contents out",
-            make_sugg(cx, els.span, "..", Some(expr.span)),
-            app,
-        );
     }
 }
 
-/// Call `check` functions to check if an expression always breaks control flow
-#[derive(Default)]
-struct BreakVisitor {
-    is_break: bool,
-}
-
-impl<'ast> Visitor<'ast> for BreakVisitor {
-    fn visit_block(&mut self, block: &'ast Block) {
-        self.is_break = match block.stmts.as_slice() {
-            [.., last] => self.check_stmt(last),
-            _ => false,
-        };
-    }
-
-    fn visit_expr(&mut self, expr: &'ast Expr) {
-        self.is_break = match expr.kind {
-            ExprKind::Break(..) | ExprKind::Continue(..) | ExprKind::Ret(..) => true,
-            ExprKind::Match(_, ref arms, _) => arms.iter().all(|arm|
-                arm.body.is_none() || arm.body.as_deref().is_some_and(|body| self.check_expr(body))
-            ),
-            ExprKind::If(_, ref then, Some(ref els)) => self.check_block(then) && self.check_expr(els),
-            ExprKind::If(_, _, None)
-            // ignore loops for simplicity
-            | ExprKind::While(..) | ExprKind::ForLoop { .. } | ExprKind::Loop(..) => false,
-            _ => {
-                walk_expr(self, expr);
-                return;
+fn check<'tcx>(cx: &LateContext<'tcx>, ctxt: SyntaxContext, needs_semi: bool, e: &'tcx Expr<'_>) {
+    // Find the final `else` block in an `if` chain.
+    let mut prev_then = None;
+    let mut next = e;
+    let (then, else_) = loop {
+        match next.kind {
+            ExprKind::If(_, then, Some(else_))
+                if is_never(cx.typeck_results(), ctxt, then)
+                    && then.span.ctxt() == ctxt
+                    && else_.span.ctxt() == ctxt =>
+            {
+                prev_then = Some(then);
+                next = else_;
             },
-        };
-    }
-}
-
-impl BreakVisitor {
-    fn check<T>(&mut self, item: T, visit: fn(&mut Self, T)) -> bool {
-        visit(self, item);
-        std::mem::replace(&mut self.is_break, false)
-    }
+            ExprKind::Block(b, _) if let Some(then) = prev_then => break (then, b),
+            _ => return,
+        }
+    };
 
-    fn check_block(&mut self, block: &Block) -> bool {
-        self.check(block, Self::visit_block)
+    if e.span.ctxt() == ctxt
+        && !ctxt.in_external_macro(cx.tcx.sess.source_map())
+        && !is_from_proc_macro(cx, e)
+        && let Some(src) = else_.span.get_text(cx)
+        && let Some(src) = src.strip_prefix('{')
+        && let Some(src) = src.strip_suffix('}')
+        // FIXME(@Jarcho): `indent_of` walks to the root context before getting the indent
+        // which gives the wrong result here.
+        && let Some(indent) = indent_of(cx, e.span)
+    {
+        let sp = else_.span.with_lo(then.span.hi());
+        span_lint_hir_and_then(cx, REDUNDANT_ELSE, e.hir_id, sp, "redundant else block", |diag| {
+            let mut sugg = reindent_multiline(src.trim_end(), false, Some(indent));
+            if needs_semi && else_.expr.is_some_and(|e| expr_needs_semi(ctxt, e)) {
+                sugg.push(';');
+            }
+            diag.span_suggestion(
+                sp,
+                "remove the `else` block and move the contents out",
+                sugg,
+                if ctxt.is_root() && else_.stmts.iter().all(|s| !matches!(s.kind, StmtKind::Let(_))) {
+                    Applicability::MachineApplicable
+                } else {
+                    Applicability::MaybeIncorrect
+                },
+            );
+        });
     }
+}
 
-    fn check_expr(&mut self, expr: &Expr) -> bool {
-        self.check(expr, Self::visit_expr)
+/// Checks if an expression, viewed from the specified context, needs a trailing semicolon
+/// to be parsed as a statement.
+fn expr_needs_semi(ctxt: SyntaxContext, e: &Expr<'_>) -> bool {
+    match e.kind {
+        ExprKind::Block(..) | ExprKind::Loop(..) | ExprKind::Match(..) | ExprKind::If(..) if ctxt == e.span.ctxt() => {
+            false
+        },
+        ExprKind::Loop(..) => {
+            let expn = e.span.ctxt().outer_expn_data();
+            ctxt != expn.call_site.ctxt() || !matches!(expn.kind, ExpnKind::Desugaring(_))
+        },
+        ExprKind::Match(_, _, MatchSource::ForLoopDesugar) => ctxt != e.span.ctxt().outer_expn_data().call_site.ctxt(),
+        _ => true,
     }
+}
 
-    fn check_stmt(&mut self, stmt: &Stmt) -> bool {
-        self.check(stmt, Self::visit_stmt)
+fn is_never(typeck: &TypeckResults<'_>, ctxt: SyntaxContext, e: &Expr<'_>) -> bool {
+    if ctxt.is_root() {
+        is_never_root(typeck, e)
+    } else {
+        is_never_mac(ctxt, e)
     }
 }
 
-// Extract the inner contents of an `else` block str
-// e.g. `{ foo(); bar(); }` -> `foo(); bar();`
-fn extract_else_block(mut block: &str) -> String {
-    block = block.strip_prefix("{").unwrap_or(block);
-    block = block.strip_suffix("}").unwrap_or(block);
-    block.trim_end().to_string()
+fn is_never_root(typeck: &TypeckResults<'_>, e: &Expr<'_>) -> bool {
+    match e.kind {
+        ExprKind::Break(..) | ExprKind::Continue(_) | ExprKind::Ret(_) | ExprKind::Become(..) => true,
+        ExprKind::DropTemps(e) => is_never_root(typeck, e),
+        ExprKind::Block(b, _)
+            if let Some(e) = b.expr
+                && !b.targeted_by_break =>
+        {
+            is_never_root(typeck, e)
+        },
+        ExprKind::Match(_, arms, _) => arms.iter().all(|a| is_never_root(typeck, a.body)),
+        ExprKind::If(_, then, Some(else_)) => is_never_root(typeck, then) && is_never_root(typeck, else_),
+        ExprKind::Call(..)
+        | ExprKind::MethodCall(..)
+        | ExprKind::Binary(..)
+        | ExprKind::Unary(..)
+        | ExprKind::Block(..)
+        | ExprKind::Loop(..)
+        | ExprKind::Path(_) => typeck.expr_ty(e).is_never(),
+        _ => false,
+    }
 }
 
-fn make_sugg(cx: &EarlyContext<'_>, els_span: Span, default: &str, indent_relative_to: Option<Span>) -> String {
-    let extracted = extract_else_block(&snippet(cx, els_span, default));
-    let indent = indent_relative_to.and_then(|s| indent_of(cx, s));
-
-    reindent_multiline(&extracted, false, indent)
+fn is_never_mac(ctxt: SyntaxContext, mut e: &Expr<'_>) -> bool {
+    loop {
+        let next = match e.kind {
+            ExprKind::Break(..) | ExprKind::Continue(_) | ExprKind::Ret(_) | ExprKind::Become(..) => return true,
+            ExprKind::DropTemps(e) => e,
+            ExprKind::Block(b, _)
+                if !b.targeted_by_break
+                    && let Some(e) = match (b.expr, b.stmts) {
+                        (Some(e), _) => Some(e),
+                        (None, [.., s]) if let StmtKind::Expr(e) | StmtKind::Semi(e) = s.kind => Some(e),
+                        _ => None,
+                    }
+                    && ctxt == b.span.ctxt() =>
+            {
+                e
+            },
+            ExprKind::Match(_, arms, _) => {
+                return arms
+                    .iter()
+                    .all(|a| ctxt == a.span.ctxt() && ctxt == a.body.span.ctxt() && is_never_mac(ctxt, a.body));
+            },
+            ExprKind::If(_, then, Some(else_))
+                if ctxt == then.span.ctxt() && ctxt == else_.span.ctxt() && is_never_mac(ctxt, then) =>
+            {
+                else_
+            },
+            _ => return false,
+        };
+        if ctxt != next.span.ctxt() {
+            return false;
+        }
+        e = next;
+    }
 }

src/main.rs

@@ -42,9 +42,8 @@ pub fn main() {
             process::exit(clippy_lints::explain(
                 &lint.strip_prefix("clippy::").unwrap_or(&lint).replace('-', "_"),
             ));
-        } else {
-            show_help();
         }
+        show_help();
         return;
     }
 

tests/integration.rs

@@ -102,13 +102,13 @@ fn integration_test() {
         panic!("incompatible crate versions");
     } else if stderr.contains("failed to run `rustc` to learn about target-specific information") {
         panic!("couldn't find librustc_driver, consider setting `LD_LIBRARY_PATH`");
-    } else {
-        assert!(
-            !stderr.contains("toolchain") || !stderr.contains("is not installed"),
-            "missing required toolchain"
-        );
     }
 
+    assert!(
+        !stderr.contains("toolchain") || !stderr.contains("is not installed"),
+        "missing required toolchain"
+    );
+
     match output.status.code() {
         Some(0) => println!("Compilation successful"),
         Some(code) => eprintln!("Compilation failed. Exit code: {code}"),