Simplify `ObligationCauseCode::IfExpression` #139594

compiler-errors · 2025-04-09T18:05:53Z

This originally started out as an experiment to do less incremental invalidation by deferring the span operations that happen on the good path in check_expr_if, but it ended up not helping much (or at least not showing up in our incremental tests).

As a side-effect though, I think the code is a lot cleaner and there are modest diagnostics improvements with overlapping spans, so I think it's still worth landing.

compiler-errors · 2025-04-09T18:05:58Z

@bors try @rust-timer queue

[experiment] How expensive is `if_cause`? TODO r? `@ghost`

bors · 2025-04-09T18:07:10Z

⌛ Trying commit bce9c6d with merge aa2e738...

bors · 2025-04-09T20:05:02Z

☀️ Try build successful - checks-actions
Build commit: aa2e738 (aa2e73810fb1f33222bf3f89241008ff425e0900)

rust-timer · 2025-04-09T22:28:39Z

Finished benchmarking commit (aa2e738): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.1%]	18
Improvements ✅ (secondary)	-0.3%	[-0.6%, -0.2%]	30
All ❌✅ (primary)	-0.2%	[-0.3%, 0.2%]	19

Max RSS (memory usage)

Results (secondary 3.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.6%	[1.8%, 4.7%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 1.2%, secondary 2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	2.2%	[1.9%, 2.4%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.203s -> 781.608s (0.18%)
Artifact size: 366.14 MiB -> 366.17 MiB (0.01%)

compiler-errors · 2025-04-10T01:16:16Z

@bors try @rust-timer queue

[experiment] How expensive is `if_cause`? TODO r? `@ghost`

bors · 2025-04-10T01:17:27Z

⌛ Trying commit 4f520ff with merge a62fc50...

bors · 2025-04-10T03:28:34Z

☀️ Try build successful - checks-actions
Build commit: a62fc50 (a62fc50b127d7d867961605c5b0f92b81946c6b7)

rust-timer · 2025-04-10T07:16:01Z

Finished benchmarking commit (a62fc50): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.3%, 2.4%]	226
Regressions ❌ (secondary)	1.0%	[0.2%, 3.8%]	167
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[0.3%, 2.4%]	226

Max RSS (memory usage)

Results (primary 2.6%, secondary 0.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	2.6%	[1.3%, 5.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-4.1%, -2.0%]	3
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Cycles

Results (primary 1.3%, secondary 2.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.3%	[0.8%, 1.6%]	9
Regressions ❌ (secondary)	2.4%	[1.5%, 4.0%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[0.8%, 1.6%]	9

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.069s -> 780.476s (0.05%)
Artifact size: 366.22 MiB -> 366.14 MiB (-0.02%)

compiler-errors · 2025-04-10T17:16:21Z

@bors try @rust-timer queue

[experiment] How expensive is `if_cause`? TODO r? `@ghost`

bors · 2025-04-10T17:17:32Z

⌛ Trying commit 847c7b6 with merge 60f37a0...

compiler-errors · 2025-04-10T01:16:35Z

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

-   | ||_____|
-   |  |_____`if` and `else` have incompatible types
-   |        expected `i32`, found `()`
+LL |       let x = if let Ok(x) = res {


Good change.

compiler-errors · 2025-04-10T01:16:52Z

tests/ui/expr/if/if-else-type-mismatch.stderr

-   | |_____- expected because of this
-LL |           11u32
-   |           ^^^^^ expected `()`, found `u32`
+LL |        let _ = if true {


Neutral change. Spans weren't overlapping anyways.

compiler-errors · 2025-04-10T01:16:57Z

tests/ui/inference/deref-suggestion.stderr

-   | ||_____|
-   |  |_____`if` and `else` have incompatible types
-   |        expected `i32`, found `&{integer}`
+LL |       let val = if true {


Good change.

compiler-errors · 2025-04-10T01:17:04Z

tests/ui/suggestions/return-bindings.stderr

-   | |_____- expected because of this
-LL |           String::new()
-   |           ^^^^^^^^^^^^^ expected `()`, found `String`
+LL |        let s = if let Some(s) = opt_str {


Neutral. See above.

oli-obk · 2025-06-25T17:50:34Z

it ended up not helping much (or at least not showing up in our incremental tests).

yea I think we need to port a lot more things to get there. We're gonna get some ability to track whether we removed hir crate cache hits via the perf page, so at that point we can at least get something tangible

compiler-errors · 2025-06-25T17:52:57Z

Well they're not hir_crate hits, they're calls to track_span_parent which is an always-red query.

oli-obk · 2025-06-25T17:57:51Z

oof, right

@bors r+ rollup

bors · 2025-06-25T17:57:54Z

📌 Commit 76d4b27 has been approved by oli-obk

It is now in the queue for this repository.

Simplify `ObligationCauseCode::IfExpression` This originally started out as an experiment to do less incremental invalidation by deferring the span operations that happen on the good path in `check_expr_if`, but it ended up not helping much (or at least not showing up in our incremental tests). As a side-effect though, I think the code is a lot cleaner and there are modest diagnostics improvements with overlapping spans, so I think it's still worth landing.

bors · 2025-06-26T03:22:20Z

☔ The latest upstream changes (presumably #143026) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2025-06-26T03:43:25Z

@bors r=oli-obk

bors · 2025-06-26T03:43:28Z

📌 Commit 59e1a3c has been approved by oli-obk

It is now in the queue for this repository.

Simplify `ObligationCauseCode::IfExpression` This originally started out as an experiment to do less incremental invalidation by deferring the span operations that happen on the good path in `check_expr_if`, but it ended up not helping much (or at least not showing up in our incremental tests). As a side-effect though, I think the code is a lot cleaner and there are modest diagnostics improvements with overlapping spans, so I think it's still worth landing.

Rollup of 17 pull requests Successful merges: - #124595 (Suggest cloning `Arc` moved into closure) - #139594 (Simplify `ObligationCauseCode::IfExpression`) - #141311 (make `tidy-alphabetical` use a natural sort) - #141648 ([rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion) - #142255 (Add edition checks for some tests that had divergent output) - #142285 (tests: Do not run afoul of asm.validity.non-exhaustive in input-stats) - #142549 (small iter.intersperse.fold() optimization) - #142637 (Remove some glob imports from the type system) - #142647 ([perf] Compute hard errors without diagnostics in impl_intersection_has_impossible_obligation) - #142700 (Remove incorrect comments in `Weak`) - #142884 (StableMIR: Add method to retrieve body of coroutine) - #142925 (Rewrite `.gitattributes` CRLF ui tests into run-make tests) - #143001 (Rename run always ) - #143010 (Update `browser-ui-test` version to `0.20.7`) - #143015 (Add `sym::macro_pin` diagnostic item for `core::pin::pin!()`) - #143020 (codegen_fn_attrs: make comment more precise) - #143033 (Expand const-stabilized API links in relnotes) r? `@ghost` `@rustbot` modify labels: rollup

Simplify `ObligationCauseCode::IfExpression` This originally started out as an experiment to do less incremental invalidation by deferring the span operations that happen on the good path in `check_expr_if`, but it ended up not helping much (or at least not showing up in our incremental tests). As a side-effect though, I think the code is a lot cleaner and there are modest diagnostics improvements with overlapping spans, so I think it's still worth landing.

Rollup of 9 pull requests Successful merges: - #124595 (Suggest cloning `Arc` moved into closure) - #139594 (Simplify `ObligationCauseCode::IfExpression`) - #141311 (make `tidy-alphabetical` use a natural sort) - #141648 ([rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion) - #142285 (tests: Do not run afoul of asm.validity.non-exhaustive in input-stats) - #142393 (Don't give APITs names with macro expansion placeholder fragments in it) - #142884 (StableMIR: Add method to retrieve body of coroutine) - #142981 (Make missing lifetime suggestion verbose) - #143030 (Fix suggestion spans inside macros for the `unused_must_use` lint) r? `@ghost` `@rustbot` modify labels: rollup

Simplify `ObligationCauseCode::IfExpression` This originally started out as an experiment to do less incremental invalidation by deferring the span operations that happen on the good path in `check_expr_if`, but it ended up not helping much (or at least not showing up in our incremental tests). As a side-effect though, I think the code is a lot cleaner and there are modest diagnostics improvements with overlapping spans, so I think it's still worth landing.

Rollup of 8 pull requests Successful merges: - #124595 (Suggest cloning `Arc` moved into closure) - #139594 (Simplify `ObligationCauseCode::IfExpression`) - #141311 (make `tidy-alphabetical` use a natural sort) - #141648 ([rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion) - #142285 (tests: Do not run afoul of asm.validity.non-exhaustive in input-stats) - #142393 (Don't give APITs names with macro expansion placeholder fragments in it) - #142884 (StableMIR: Add method to retrieve body of coroutine) - #142981 (Make missing lifetime suggestion verbose) r? `@ghost` `@rustbot` modify labels: rollup

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 9, 2025