de-duplicate condition scoping logic between AST→HIR lowering and `ScopeTree` construction #143213

dianne · 2025-06-29T20:39:12Z

There was some overlap between rustc_ast_lowering::LoweringContext::lower_cond and rustc_hir_analysis::check::region::resolve_expr, so I've removed the former and migrated its logic to the latter, with some simplifications.

Consequences:

For while and if expressions' let-chains, this changes the HirIds for the &&s to properly correspond to their AST nodes. This is how guards were handled already.
This makes match guards share previously-duplicated logic with if/while expressions. This will also be used by guard pattern¹ guards.
Aside from legacy syntax extensions (e.g. some builtin macros) that directly feed AST to the compiler, it's currently impossible to put attributes directly on && operators in let chains². Nonetheless, attributes on && operators in let chains in if/while expression conditions are no longer silently ignored and will be lowered.
This no longer wraps conditions in DropTemps, so the HIR and THIR will be slightly smaller.
DesugaringKind::CondTemporary is now gone. It's no longer applied to any spans, and all uses of it were dead since they were made to account for if and while being desugared to match on a boolean scrutinee.
Should be a marginal perf improvement beyond that due to leveraging ScopeTree construction's clever handling of && and ||:
- This removes some unnecessary terminating scopes that were placed around top-level && and || operators in conditions. When lowered to MIR, logical operator chains don't create intermediate boolean temporaries, so there's no temporary to drop. The linked snippet handles wrapping the operands in terminating scopes as necessary, in case they create temporaries.
- The linked snippet takes care of letting let temporaries live and terminating other operands, so we don't need separate traversals of && chains for that.

Tracking issue for RFC 3637: Guard patterns #129967 ↩
Case-by-case, here's my justification: #[attr] e1 && e2 applies the attribute to e1. In #[attr] (e1 && e2) , the attribute is on the parentheses in the AST, plus it'd fail to parse if e1 or e2 contains a let. In #[attr] expands_to_let_chain!(), the attribute would already be ignored (How to treat inert attributes on macro invocations? #63221) and it'd fail to parse anyway; even if the expansion site is a condition, the expansion wouldn't be parsed with Restrictions::ALLOW_LET. If it was allowed, the notion of a "reparse context" from https://github.com/rust-lang/rust/issues/61733#issuecomment-509626449 would be necessary in order to make let-chains left-associative; multiple places in the compiler assume they are. ↩

rustbot · 2025-06-29T20:39:17Z

r? @WaffleLapkin

rustbot has assigned @WaffleLapkin.
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-06-29T22:25:10Z

Some changes occurred in coverage tests.

cc @Zalathar

dianne · 2025-06-29T23:14:19Z

actually, I've convinced myself it's cleanest to get rid of lower_cond, so I've done that (diff of diffs). the coverage tests' maps are now back to normal. instead, I had to change a diagnostic and a handful of things in clippy that assumed if expressions have DropTemps in them. sorry for the noise! this ended up being more disruptive than intended. to keep keep the scope of this change as small as possible, I've tried to leave lints' behaviors untouched. without the extra meddling this does to avoid linting on let chains,

clippy::needless_if would lint on if let x = () {}
clippy::while_immutable_condition would lint on while let x = () {}
clippy::needless_bool would lint on if let x = () && true {}

edit: I just noticed that I've left DesugaringKind::CondTemporary unused. I'll have to clean that up. it looks like it was checked in some diagnostics, but when I tested locally at least I don't think I got any UI test failures, so I'll look closer into if anything changed.

@rustbot author

rustbot · 2025-06-30T03:35:07Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

rustbot · 2025-06-30T03:43:59Z

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

dianne · 2025-06-30T05:12:02Z

alright, I've removed DesugaringKind::CondTemporary. it looks like all uses of it were out-of-date.

there was a use in checking the types of literal patterns to account for if and while being desugared as match expressions on booleans. this is not the case anymore, so the DesugaringKind check was dead.
there was a use in warn_if_unreachable, which also seems to have been accounting for desugaring if and while to match. now that if is its own thing in the HIR, the unreachability warning in check_expr_if uses the success block's span, so the DesugaringKind check was dead.
the doc comment on the DesugaringKind::CondTemporary variant was also out-of-date, again referring to desugaring if and while to match

all of this predates me contributing to the compiler, but I guess putting DropTemps in the conditions for if and while was a holdover from when it was necessary because they desugared to match?

@rustbot ready

cjgillot

This looks good. A few nits and questions.

cjgillot · 2025-07-05T14:59:31Z

compiler/rustc_hir_typeck/src/pat.rs

-                    })
-                    .is_some(),
-            );
+            err.emit();


Can this be moved into demand_suptype_with_origin?

There are other still other uses of demand_suptype_with_origin that augment the error in some way before emitting. There's a variant that emits the error, demand_suptype, but it uses a misc obligation cause rather than ObligationCauseCode::Pattern. One possible alternative would be to define a demand_suptype_pat like we do for demand_eqtype_pat, but that would be the only use of it.

cjgillot · 2025-07-05T14:59:48Z

compiler/rustc_ast_lowering/src/expr.rs

-                let cond = self.lower_expr(cond);
-                let reason = DesugaringKind::CondTemporary;
-                let span_block = self.mark_span_with_reason(reason, cond.span, None);
-                self.expr_drop_temps(span_block, cond)


Do you have ideas how to remove DropTemps completely? In a follow-up pr ofc.

In short, no. It could easily be removed from for loop desugaring, but afaict it can only easily be removed from async desugaring in Editions 2024 and newer.

For desugaring for pat in expr { .. }, it's to keep the expr's temps from living into the enclosing expression (which means an even longer lifetime for for in block tail expressions in old Editions). The difference can be seen here. If we wanted to remove DropTemps in a cross-Edition-compatible way without a breaking change, we could use a block with one statement and no tail expression, since for loops always return (). That said, as long as DropTemps still exists, it's probably the easiest way to get the current behavior.

For async desugaring, it's to make sure the body's temporaries are dropped before the parameters. Currently, it's lowered into the tail expression of a block, which ensures its temporaries are dropped in Edition 2024, but tail expressions aren't terminating scopes in old Editions. I don't have a good answer for this other than keeping DropTemps around for it, but I'm admittedly not too familiar with async desugaring.

cjgillot · 2025-07-05T15:27:40Z

compiler/rustc_hir_analysis/src/check/region.rs

+        // Temporaries for `let` expressions must live into the success branch.
+        hir::ExprKind::Let(_) => false,
+        // Logical operator chains are handled in `resolve_expr`. The operators themselves have no
+        // intermediate value to drop, and operands will be wrapped in terminating scopes.


This comment needs to be elaborated a bit. It took me some time to convince myself that resolve_expr does the right thing.

I've added some more details: diff. If that doesn't fully clarify it, are there any things in particular that you think need accounting for or mentioning?

dianne · 2025-07-06T00:18:24Z

I noticed a comment that I'd missed that references DropTemps being used for if, so I've updated it: diff.

WaffleLapkin · 2025-07-08T02:42:05Z

r? cjgillot
(since you started reviewing this)

cjgillot · 2025-07-11T14:23:04Z

Thanks!
@bors r+

bors · 2025-07-11T14:23:07Z

📌 Commit 68d860f has been approved by cjgillot

It is now in the queue for this repository.

de-duplicate condition scoping logic between AST→HIR lowering and `ScopeTree` construction There was some overlap between `rustc_ast_lowering::LoweringContext::lower_cond` and `rustc_hir_analysis::check::region::resolve_expr`, so I've removed the former and migrated its logic to the latter, with some simplifications. Consequences: - For `while` and `if` expressions' `let`-chains, this changes the `HirId`s for the `&&`s to properly correspond to their AST nodes. This is how guards were handled already. - This makes match guards share previously-duplicated logic with `if`/`while` expressions. This will also be used by guard pattern[^1] guards. - Aside from legacy syntax extensions (e.g. some builtin macros) that directly feed AST to the compiler, it's currently impossible to put attributes directly on `&&` operators in `let` chains[^2]. Nonetheless, attributes on `&&` operators in `let` chains in `if`/`while` expression conditions are no longer silently ignored and will be lowered. - This no longer wraps conditions in `DropTemps`, so the HIR and THIR will be slightly smaller. - `DesugaringKind::CondTemporary` is now gone. It's no longer applied to any spans, and all uses of it were dead since they were made to account for `if` and `while` being desugared to `match` on a boolean scrutinee. - Should be a marginal perf improvement beyond that due to leveraging [`ScopeTree` construction](https://github.com/rust-lang/rust/blob/5e749eb66f93ee998145399fbdde337e57cd72ef/compiler/rustc_hir_analysis/src/check/region.rs#L312-L355)'s clever handling of `&&` and `||`: - This removes some unnecessary terminating scopes that were placed around top-level `&&` and `||` operators in conditions. When lowered to MIR, logical operator chains don't create intermediate boolean temporaries, so there's no temporary to drop. The linked snippet handles wrapping the operands in terminating scopes as necessary, in case they create temporaries. - The linked snippet takes care of letting `let` temporaries live and terminating other operands, so we don't need separate traversals of `&&` chains for that. [^1]: rust-lang#129967 [^2]: Case-by-case, here's my justification: `#[attr] e1 && e2` applies the attribute to `e1`. In `#[attr] (e1 && e2)` , the attribute is on the parentheses in the AST, plus it'd fail to parse if `e1` or `e2` contains a `let`. In `#[attr] expands_to_let_chain!()`, the attribute would already be ignored (rust-lang#63221) and it'd fail to parse anyway; even if the expansion site is a condition, the expansion wouldn't be parsed with `Restrictions::ALLOW_LET`. If it *was* allowed, the notion of a "reparse context" from rust-lang#61733 (comment) would be necessary in order to make `let`-chains left-associative; multiple places in the compiler assume they are.

Rollup of 9 pull requests Successful merges: - #143213 (de-duplicate condition scoping logic between AST→HIR lowering and `ScopeTree` construction) - #143461 (make `cfg_select` a builtin macro) - #143519 (Check assoc consts and tys later like assoc fns) - #143554 (slice: Mark `rotate_left`, `rotate_right` unstably const) - #143704 (Be a bit more careful around exotic cycles in in the inliner) - #143774 (constify `From` and `Into`) - #143786 (Fix fallback for CI_JOB_NAME) - #143796 (Fix ICE for parsed attributes with longer path not handled by CheckAttribute) - #143798 (Remove format short command trait) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2025-07-13T04:20:11Z

⌛ Testing commit 68d860f with merge d2baa49...

bors · 2025-07-13T07:23:52Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing d2baa49 to master...

github-actions · 2025-07-13T07:27:10Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing b1d2f2c (parent) -> d2baa49 (this PR)

Test differences

Show 12 test diffs

12 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard d2baa49a106fad06fbf6202fb6ea8a0b3d2767cc --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

x86_64-apple-2: 5567.4s -> 3998.9s (-28.2%)
dist-apple-various: 7285.2s -> 6393.5s (-12.2%)
dist-aarch64-apple: 6454.7s -> 5665.9s (-12.2%)
aarch64-msvc-2: 5459.9s -> 4951.0s (-9.3%)
tidy: 73.9s -> 68.5s (-7.3%)
x86_64-mingw-2: 7153.2s -> 7657.9s (7.1%)
aarch64-msvc-1: 7045.8s -> 6667.9s (-5.4%)
aarch64-gnu-debug: 4168.8s -> 3952.8s (-5.2%)
dist-x86_64-netbsd: 4533.2s -> 4766.7s (5.2%)
dist-x86_64-apple: 10386.6s -> 9862.4s (-5.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-07-13T08:33:16Z

Finished benchmarking commit (d2baa49): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2%	[0.1%, 0.3%]	5
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.2%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.2%]	3

Max RSS (memory usage)

Results (primary 0.7%)

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

	mean	range	count
Regressions ❌ (primary)	2.1%	[1.7%, 2.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.9%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[-0.9%, 2.5%]	4

Cycles

Results (primary -2.8%, secondary 1.5%)

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)	3.3%	[2.6%, 4.0%]	2
Improvements ✅ (primary)	-2.8%	[-2.8%, -2.8%]	1
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-2.8%	[-2.8%, -2.8%]	1

Binary size

Results (primary -0.1%, 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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.3%, -0.0%]	65
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	23
All ❌✅ (primary)	-0.1%	[-0.3%, -0.0%]	65

Bootstrap: 466.487s -> 464.852s (-0.35%)
Artifact size: 374.73 MiB -> 374.71 MiB (-0.01%)

rustbot assigned WaffleLapkin Jun 29, 2025

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 Jun 29, 2025

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

dianne force-pushed the lower-cond-tweaks branch from 686da48 to 13c796b Compare June 29, 2025 22:25

dianne force-pushed the lower-cond-tweaks branch from 13c796b to be191a0 Compare June 29, 2025 23:13

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 Jun 30, 2025

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 Jun 30, 2025

cjgillot approved these changes Jul 5, 2025

View reviewed changes

dianne added 3 commits July 5, 2025 17:14

de-duplicate condition scoping logic

75cea03

clippy: conditions are no longer wrapped in DropTemps

fc557bc

remove DesugaringKind::CondTemporary

68d860f

dianne force-pushed the lower-cond-tweaks branch from f3f5287 to 68d860f Compare July 6, 2025 00:16

rustbot assigned cjgillot and unassigned WaffleLapkin Jul 8, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2025

matthiaskrgr mentioned this pull request Jul 12, 2025

Rollup of 9 pull requests #143840

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 13, 2025

bors merged commit d2baa49 into rust-lang:master Jul 13, 2025
12 checks passed

rustbot added this to the 1.90.0 milestone Jul 13, 2025

rustbot added the perf-regression Performance regression. label Jul 13, 2025

de-duplicate condition scoping logic between AST→HIR lowering and ScopeTree construction #143213

de-duplicate condition scoping logic between AST→HIR lowering and ScopeTree construction #143213

Conversation

dianne commented Jun 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

rustbot commented Jun 29, 2025

Uh oh!

This comment has been minimized.

This comment was marked as resolved.

rustbot commented Jun 29, 2025

Uh oh!

dianne commented Jun 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 30, 2025

Uh oh!

rustbot commented Jun 30, 2025

Uh oh!

dianne commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjgillot left a comment

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!

dianne Jul 6, 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!

dianne Jul 6, 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!

dianne commented Jul 6, 2025

Uh oh!

WaffleLapkin commented Jul 8, 2025

Uh oh!

cjgillot commented Jul 11, 2025

Uh oh!

bors commented Jul 11, 2025

Uh oh!

bors commented Jul 13, 2025

Uh oh!

bors commented Jul 13, 2025

Uh oh!

Uh oh!

github-actions bot commented Jul 13, 2025

Test differences

Job duration changes

Uh oh!

rust-timer commented Jul 13, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

Uh oh!

de-duplicate condition scoping logic between AST→HIR lowering and `ScopeTree` construction #143213

de-duplicate condition scoping logic between AST→HIR lowering and `ScopeTree` construction #143213

dianne commented Jun 29, 2025 •

edited

Loading

dianne commented Jun 29, 2025 •

edited

Loading

dianne commented Jun 30, 2025 •

edited

Loading

dianne Jul 6, 2025 •

edited

Loading

dianne Jul 6, 2025 •

edited

Loading