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

[dianne]

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.

Footnotes

1. Tracking issue for RFC 3637: Guard patterns #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 (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]

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]

Some changes occurred in coverage tests.

cc @Zalathar

[dianne]

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]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[rustbot]

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

[dianne]

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]

Can this be moved into demand_suptype_with_origin?

[dianne]

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]

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

[dianne]

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]

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

[dianne]

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]

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