Fix check_expr_if to point to a more accurate location of the compilation error in some cases by sgasho · Pull Request #147484 · rust-lang/rust

sgasho · 2025-10-08T15:36:19Z

The problem linked on the issue above lays on how Expr::If is processed.

Currently, Expr::If is processed by depth-first like approach, which is natural considering that Expr::If is a nested structure.

e.g.

fn main() {
    let oa = Some(1);
    let oa2 = Some(1);
    let v = if let Some(a) = oa {
        Some(&a)
    } else if let Some(a) = oa2 {
        &Some(a)
    } else {
        None
    };
    println!("{v:?}");
}

Simplified Expr::If structure for this code is like below.

Expr::If {
  cond: "if let Some(a) = oa",
  then: "Some(&a)",
  else: {
    Expr::If {
      cond: "else if let Some(a) = oa2",
      then: "&Some(a)",
      else: {
        cond: "else",
        then: "None",
        else: None,
      }
    }
  }
}

Current check_expr_if processes this Expr::If by depth-first-search like approach, that is why "if and else have incompatible types" occurs between else if let Some(a) = oa2 and else, not between if let Some(a) = oa and if let Some(a) = oa2.

New approach on this PR is first flatten the Expr::If nested nodes, then process forward.

I created _if.rs and moved the check_expr_if becase this new approach requires a bit more lines of codes.

I added a test case that guarantees the problem on the linked issue has been resolved.
tests/ui/expr/if/if-multi-else-if-incompatible-types-issue-146190.rs

Discussion on Zulip: https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Struggling.20to.20fix.20issue.20.23146190.20check_expr_if/with/540960074

sgasho · 2025-10-15T15:05:56Z

Was "if and else (have incompatible types) " appropriate for this case?
I think this else if branch has expression of type ! and it is compatible for any type. (source: "Expressions of type ! can be coerced into any other type." at https://doc.rust-lang.org/reference/types/never.html ).

I could be wrong but my understanding is showing "if may be missing an else clause" for this case (all if-else-if have compatible types and the only problem is missing else branch, which would be evaluated to ())

So,

Hmm, well, I wouldn't expect the incompatible types error. But I also wouldn't expect to see an expected .. found ... here.

I guess, the found () is possibly the type of the entire if/else if expression.

The expected … found … label in this case isn’t new with this PR. I saw that current nightly already emits expected integer, found () for error[E0317]: if may be missing an else clause. e.g. tests/ui/expr/if/issue-4201.rs

Should this be discussed or fixed in another issue?

rustbot · 2025-10-15T15:06:19Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
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

fmease · 2025-10-19T10:49:32Z

r? types

jackh726 · 2025-10-29T03:41:23Z

Sorry for the delay here. Before I do an actual review here, I want to check perf.

@bors2 try @rust-timer build

jackh726

Some more review comments. Looking better, but still have some questions/thoughts.

View changes since this review

jackh726 · 2025-11-24T22:48:13Z

+        if let hir::ExprKind::Block(block, _) = &expr.kind
+            && block.expr.is_none()
+        {
+            return None;
+        }
+


Why did you add this? Is there a test difference that you can comment and link to?

jackh726 · 2025-11-25T05:55:41Z

+        &self,
+        expr_id: HirId,
+        sp: Span,
+        parts: &IfExprParts<'tcx>,


here, and elsewhere in the file, these are Copy and so shouldn't be passed by ref

jackh726 · 2025-11-25T06:00:26Z

+        parts: &IfExprParts<'tcx>,
+        orig_expected: Expectation<'tcx>,
+    ) -> Ty<'tcx> {
+        let root_if_expr = self.tcx.hir_expect_expr(expr_id);


Could pass this in instead of expr_id and sp

jackh726 · 2025-11-25T06:08:56Z

+                .get(idx + 1)
+                .map(|next| next.expr_with_parts.expr)
+                .or(chain.last_expr().into());


Hmm, this doesn't seem right (or at least, is too subtle for my liking): when idx is len-1, chain.last_expr() is called and there is no final else, uses the current expr (since it is the last one).

jackh726 · 2025-11-25T06:22:02Z

+                self.misc(branch_body.span)
+            };
+            let cause_span =
+                if idx == 0 { Some(root_if_expr.span) } else { Some(branch_body.span) };


If idx == 0, isn't branch_body.span == root_if_expr.span?

jackh726 · 2025-11-25T06:23:24Z

                    .typeck_results
                    .as_ref()
                    .expect("if expression only expected inside FnCtxt")
-                    .expr_ty(else_expr);
-                if let hir::ExprKind::If(_cond, _then, None) = else_expr.kind
+                    .expr_ty_opt(else_expr);


Hmm, so...is this ever going to return Some?

jackh726 · 2025-11-25T06:24:33Z

@rustbot author

rustbot · 2025-11-25T06:24:38Z

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

jackh726 · 2026-03-31T02:31:22Z

Ping @sgasho. Are you still working on this?

sgasho · 2026-03-31T02:40:09Z

@jackh726

Ping @sgasho. Are you still working on this?

Sorry, Currently No. I've been working on around autodiff and infra issues for several months and had no time to working on this.
But I can resume it around April. If this is urgent, another person can continue it.

jackh726 · 2026-03-31T14:59:09Z

No worries - this can wait. (If anyone else wants to pick this up, let me know.)

This reverts commit ac49139.

rustbot · 2026-05-04T10:25:28Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

sgasho · 2026-05-04T11:15:08Z

@jackh726

Changed direction. The 7 old commits are squashed + reverted in history so both designs are reviewable.

Keeping the code architecture clean with the flattening approach turned out to be harder than I
expected, so I rethought it.

The new approach keeps the nested Expr::If and passes parent ty to children to coerce them.

Also, I added a test case for an if-chain nested inside another if-chain at tests/ui/typeck/if-else-type-mismatch-chain-nested.rs.

FWIW, I made some simple illustrations of the before/after approaches.

Before

After

…t from depth first approach

sgasho · 2026-05-04T14:59:44Z

@rustbot ready

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 8, 2025

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

sgasho changed the title ~~needs_help: wip fix 146190~~ Fix check_expr_if to point to a more accurate location of the compilation error in some cases Oct 10, 2025

This comment has been minimized.

Sign in to view

sgasho force-pushed the fix/141690_check_expr_if branch from f309ba8 to 98bc078 Compare October 13, 2025 01:55

This comment has been minimized.

Sign in to view

sgasho force-pushed the fix/141690_check_expr_if branch from 98bc078 to c50157b Compare October 13, 2025 02:24

This comment has been minimized.

Sign in to view

sgasho force-pushed the fix/141690_check_expr_if branch from c50157b to b522404 Compare October 13, 2025 06:52

rustbot added the T-clippy Relevant to the Clippy team. label Oct 13, 2025

sgasho force-pushed the fix/141690_check_expr_if branch 4 times, most recently from ebfaecc to 5c38b51 Compare October 15, 2025 13:39

sgasho commented Oct 15, 2025

View reviewed changes

sgasho marked this pull request as ready for review October 15, 2025 15:06

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2025

rustbot assigned SparrowLii Oct 15, 2025

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 15, 2025

This comment has been minimized.

Sign in to view

rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Oct 19, 2025

rustbot assigned jackh726 and unassigned SparrowLii Oct 19, 2025

sgasho requested a review from jackh726 November 15, 2025 12:29

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 15, 2025

jackh726 reviewed Nov 25, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2025

sgasho force-pushed the fix/141690_check_expr_if branch from fdd59f6 to 71919dc Compare May 4, 2026 10:18

This comment has been minimized.

Sign in to view

sgasho force-pushed the fix/141690_check_expr_if branch from 71919dc to da8de8f Compare May 4, 2026 10:23

sgasho added 2 commits May 4, 2026 19:24

old approach (squashed for revert)

62755d7

Revert "old approach (squashed for revert)"

b7077d4

This reverts commit ac49139.

sgasho force-pushed the fix/141690_check_expr_if branch from da8de8f to dddffd2 Compare May 4, 2026 10:25

This comment has been minimized.

Sign in to view

sgasho force-pushed the fix/141690_check_expr_if branch from dddffd2 to 1f3fe1e Compare May 4, 2026 10:33

This comment has been minimized.

Sign in to view

sgasho force-pushed the fix/141690_check_expr_if branch from 1f3fe1e to 57eeb19 Compare May 4, 2026 10:45

This comment has been minimized.

Sign in to view

fix: diagnostics - check if - else chain types from top to bottom, no…

7d6060c

…t from depth first approach

sgasho force-pushed the fix/141690_check_expr_if branch from 57eeb19 to 7d6060c Compare May 4, 2026 13:29

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 4, 2026

sgasho requested a review from jackh726 May 4, 2026 15:00

Uh oh!

Conversation

sgasho commented Oct 8, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment has been minimized.

This comment was marked as outdated.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sgasho Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackh726 Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

sgasho Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Oct 15, 2025

Uh oh!

This comment has been minimized.

fmease commented Oct 19, 2025

Uh oh!

jackh726 commented Oct 29, 2025

Uh oh!

jackh726 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackh726 Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

jackh726 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

jackh726 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

jackh726 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

jackh726 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

jackh726 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

jackh726 commented Nov 25, 2025

Uh oh!

rustbot commented Nov 25, 2025

Uh oh!

jackh726 commented Mar 31, 2026

Uh oh!

sgasho commented Mar 31, 2026

Uh oh!

jackh726 commented Mar 31, 2026

Uh oh!

This comment has been minimized.

rustbot commented May 4, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

sgasho commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

This comment has been minimized.

sgasho commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

sgasho commented Oct 8, 2025 •

edited by rustbot

Loading

sgasho Oct 15, 2025 •

edited

Loading

sgasho Nov 15, 2025 •

edited

Loading

jackh726 left a comment •

edited by rustbot

Loading

sgasho commented May 4, 2026 •

edited

Loading