Fix check_expr_if to point to a more accurate location of the compilation error in some cases #147484

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

…ation error in some cases

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

tests/ui/expr/if/if-else-chain-missing-else.rs

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

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

⚠️ Warning ⚠️

This PR is based on an upstream commit that is older than 28 days.

It's recommended to update your branch according to the rustc-dev-guide.

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

rust-timer · 2025-10-29T03:41:26Z

Cannot parse build command: Missing SHA in build command

Fix check_expr_if to point to a more accurate location of the compilation error in some cases

rust-bors · 2025-10-29T05:55:30Z

☀️ Try build successful (CI)
Build commit: 9877dbd (9877dbd994fb24ec6e7e51ae64b82777ed0daea4, parent: 044d68c3cb6a0b893b18293fa7f5719119403215)

jackh726 · 2025-10-31T13:55:39Z

@rust-timer build 9877dbd

rust-timer · 2025-10-31T15:05:10Z

Finished benchmarking commit (9877dbd): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	2
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	1

Max RSS (memory usage)

Results (primary 0.0%, secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	5.9%	[5.9%, 5.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-3.6%, -2.3%]	2
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	0.0%	[-3.6%, 5.9%]	3

Cycles

Results (secondary 1.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	6.5%	[6.5%, 6.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.3%]	4
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.1%, 0.3%]	7

Bootstrap: 474.307s -> 475.954s (0.35%)
Artifact size: 390.07 MiB -> 390.09 MiB (0.00%)

jackh726

Perf looks good, so that's good to see.

Haven't yet reviewed the meat of this, but some comments. If you want to try to address them, that's great. If you want to wait until I review the meat of this, that's okay too.

I'll try to get back to this later today or this weekend, but want to share what I have.

View changes since this review

jackh726 · 2025-10-31T16:20:06Z

compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs

                    .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. This went from non-opt to opt. Is this actually ever going to be none? I guess, I would expect so, but surprised the previous never bug!ed.

Is this actually ever going to be none?

Yes, the new flattening approach collects all branches first, then processes them sequentially. During collection, we only record types for branch bodies, not the if-expression nodes themselves.

previous never bug!ed.

That is because the old implementation processed if-else chains recursively (depth-first).

when else_expr is an else-if node:

Recursively calls check_expr_if(else_expr)

Returns the computed type

write_ty records it immediately

Error reporting finds the type

The new implementation skips check_expr_with_expectation on intermediate if-nodes during collection, so their types aren't recorded yet when early errors occur.

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

jackh726 · 2025-10-31T16:41:37Z

tests/ui/expr/if/if-else-chain-missing-else.rs

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.

jackh726 · 2025-10-31T17:10:23Z

tests/ui/typeck/consider-borrowing-141810-2.stderr

So, this isn't right :)

removed this borrowing suggestions in this commit!

fix: early returns suggest_deref_or_ref when the expr is an empty block

jackh726 · 2025-10-31T17:10:32Z

tests/ui/typeck/consider-borrowing-141810-3.stderr

removed this borrowing suggestions in this commit!

fix: early returns suggest_deref_or_ref when the expr is an empty block

jackh726

Sorry, another round of review. These changes I am going to pause for you to address before I get back to this.

View changes since this review

jackh726 · 2025-11-03T15:46:32Z

compiler/rustc_hir_typeck/src/_if.rs

+    fn set_diverges(
+        &self,
+        initial_diverges: Diverges,
+        guarded: &IfGuardedBranches<'tcx>,
+        tail: &IfChainTail<'tcx>,
+    ) {
+        let mut tail_diverges = tail.diverges();
+        for branch in guarded.branches.iter().rev() {
+            tail_diverges = branch.cond_diverges | (branch.body.diverges & tail_diverges);
+        }
+        self.diverges.set(initial_diverges | tail_diverges);
+    }
+}


I'm actually not a fan of splitting this (along with reset_diverges_to_maybe and take_diverges) into separate functions.

I guess, the more I look over this, I also am not a huge fan of explain_if_branch_mismatch either.

removed those methods in this commit!

refactor: remove set_diverges, reset_diverges_to_maybe, take_diverges…

I will check other comments tomorrow 🙇

compiler/rustc_hir_typeck/src/_if.rs

jackh726 · 2025-11-03T16:23:12Z

compiler/rustc_hir_typeck/src/_if.rs

+            let Some(else_expr) = self.collect_if_branch(current_if, expected, &mut chain) else {
+                return (chain, IfChainTail::Missing(current_if));
+            };
+
+            if let hir::ExprKind::If(..) = else_expr.kind {
+                current_if = else_expr;
+                continue;
+            }


Something seems weird to me...what is there both an optional return in collect_if_branch and a check for ExprKind::If for the kind?

I guess, perhaps, I could see some duplicate logic here - you should be able to maintain all the state in chain, I think.

removed duplicate logics and added some extra refactorings(sorry it if is hard to check diffs....)

refactor: remove duplicate logic for checking Expr content. and some …

…, explain_if_branch_mismatch

…other refactors

…-2.rs

sgasho · 2025-11-15T12:29:06Z

@jackh726
Sorry to be late. I made changes base on your feedback!

jackh726

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

View changes since this review

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

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

+        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

compiler/rustc_hir_typeck/src/_if.rs

+        &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

compiler/rustc_hir_typeck/src/_if.rs

+        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

compiler/rustc_hir_typeck/src/_if.rs

+                .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

compiler/rustc_hir_typeck/src/_if.rs

+                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

compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs

                    .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.

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 3 times, most recently from 5212a66 to ebfaecc Compare October 14, 2025 15:41

fix: check_expr_if to point to a more accurate location of the compil…

5c38b51

…ation error in some cases

sgasho force-pushed the fix/141690_check_expr_if branch 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

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

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Oct 29, 2025

Auto merge of #147484 - sgasho:fix/141690_check_expr_if, r=<try>

9877dbd

Fix check_expr_if to point to a more accurate location of the compilation error in some cases

This comment has been minimized.

Sign in to view

jackh726 reviewed Oct 31, 2025

View reviewed changes

jackh726 reviewed Nov 3, 2025

View reviewed changes

jackh726 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 3, 2025

sgasho added 5 commits November 7, 2025 01:23

refactor: remove set_diverges, reset_diverges_to_maybe, take_diverges…

19e05e0

…, explain_if_branch_mismatch

refactor: pass cond, then, else to reduce use of

278fd28

refactor: remove duplicate logic for checking Expr content. and some …

c52e5f9

…other refactors

fix: early returns suggest_deref_or_ref when the expr is an empty block

0fa9220

rollback unrelated diffs at tests/ui/inference/deref-suggestion.rs

682541b

sgasho force-pushed the fix/141690_check_expr_if branch from 2c15958 to 682541b Compare November 15, 2025 08:48

rollback unrelated diffs at tests/ui/typeck/consider-borrowing-141810…

fdd59f6

…-2.rs

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

Fix check_expr_if to point to a more accurate location of the compilation error in some cases #147484

Are you sure you want to change the base?

Fix check_expr_if to point to a more accurate location of the compilation error in some cases #147484

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!

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!

rustbot commented Oct 15, 2025

Uh oh!

fmease commented Oct 19, 2025

Uh oh!

jackh726 commented Oct 29, 2025

Uh oh!

rust-timer commented Oct 29, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Oct 29, 2025

Uh oh!

jackh726 commented Oct 31, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Oct 31, 2025

Overall result: ✅ improvements - no action needed

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgasho commented Nov 15, 2025

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

jackh726 left a comment •

edited by rustbot

Loading

jackh726 left a comment •

edited by rustbot

Loading