Rewrite redundant_else as a late pass.

[Jarcho]

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.

[rustbot]

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 8 candidates
• 8 candidates expanded to 8 candidates
• Random selection from dswij, llogiq, samueltardieu

[github-actions]

Lintcheck changes for 322f4dc

Lint	Added	Removed	Changed
clippy::redundant_else	8	1	7

This comment will be updated if you push new changes

[Jarcho]

Really thinking this should ignore loops as the final expression. They're the biggest source of something which doesn't obviously diverge. On the other hand large if/match expressions have the same issue and they already linted.

[dswij]

r? clippy

[samueltardieu]

Nit: I think that using check_block() would best reflect the intention (forget about GitHub borked way of showing a diff on a diff) and is one line shorter:

Suggested change

	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);
	}
	fn check_block(&mut self, cx: &LateContext<'tcx>, b: &'tcx Block<'_>) {
	for s in b.stmts {
	if let StmtKind::Expr(e) | StmtKind::Semi(e) = s.kind {
	check(cx, s.span.ctxt(), matches!(s.kind, StmtKind::Expr(_)), e);
	}
	}
	if let Some(e) = b.expr {
	check(cx, b.span.ctxt(), false, e);
	}
	}

Moreover, in any case, you should use span_lint_hir_and_then to post the lint on e.hir_id, otherwise this would fail as the lint would be posted on the block (because of check_block() or check_block_post()) instead of the last expression of the block:

            {
                #[expect(clippy::redundant_else)]
                if black_box(true) {
                    return
                } else {
                    $({ black_box(()) })
                }
            }

(nevermind the content, I just wanted a lintable expression as the last expr of a block)

Right now, it fails, but will succeed if #[expect] is applied to the block itself, which should not be necessary.

View changes since the review

[Jarcho]

check_block will be worse for perf due to the cache use. Or it would be if we fix other lints to not use check_block when check_stmt would be a better starting point; it might be neutral right now.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[samueltardieu]

Suggested change

	span_lint_and_then(cx, REDUNDANT_ELSE, sp, "redundant else block", |diag| {
	span_lint_hir_and_then(cx, REDUNDANT_ELSE, e.hir_id, sp, "redundant else block", |diag| {

View changes since the review