Skip to content

lower bodies' params to thir before the body's value #141356

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
May 22, 2025

Conversation

dianne
Copy link
Contributor

@dianne dianne commented May 21, 2025

Two motivations for this:

@rustbot
Copy link
Collaborator

rustbot commented May 21, 2025

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 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 May 21, 2025
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice improvement!

@compiler-errors
Copy link
Member

r? compiler-errors @bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 21, 2025

📌 Commit 2a403dc has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 May 21, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 21, 2025
…ee-body-expr, r=compiler-errors

`unpretty=thir-tree`: don't require the final expr to be the body's value

Two motivations for this:
- I couldn't find a comment motivating this hard-coding. I can imagine it might be easier to read `unpretty=thir-flat` output if the final expression in the THIR is always the body's value, but if that's the reason, that should be the justification in the source. I can also imagine it's meant to check that all expressions will be visited by the pretty-printer, but the existing check doesn't quite do that either.
- Guard patterns (rust-lang#129967) contain expressions, so lowering params containing guard patterns may add more expressions to the THIR. Currently a body's params are lowered after its expression, so guard expressions in params would end up last, breaking this. As an alternative, the params could be lowered first (rust-lang#141356).
bors added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#140526 (docs: Specify that common sort functions sort in an ascending direction)
 - rust-lang#141230 (std: fix doctest and explain for `as_slices` and `as_mut_slices` in `VecDeque`)
 - rust-lang#141341 (limit impls of `VaArgSafe` to just types that are actually safe)
 - rust-lang#141347 (incorrectly prefer builtin `dyn` impls :3)
 - rust-lang#141351 (Move -Zcrate-attr injection to just after crate root parsing)
 - rust-lang#141356 (lower bodies' params to thir before the body's value)
 - rust-lang#141357 (`unpretty=thir-tree`: don't require the final expr to be the body's value)
 - rust-lang#141363 (Document why we allow escaping bound vars in LTA norm)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fea2e5c into rust-lang:master May 22, 2025
6 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 22, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2025
Rollup merge of rust-lang#141357 - dianne:unhardcode-unpretty-thir-tree-body-expr, r=compiler-errors

`unpretty=thir-tree`: don't require the final expr to be the body's value

Two motivations for this:
- I couldn't find a comment motivating this hard-coding. I can imagine it might be easier to read `unpretty=thir-flat` output if the final expression in the THIR is always the body's value, but if that's the reason, that should be the justification in the source. I can also imagine it's meant to check that all expressions will be visited by the pretty-printer, but the existing check doesn't quite do that either.
- Guard patterns (rust-lang#129967) contain expressions, so lowering params containing guard patterns may add more expressions to the THIR. Currently a body's params are lowered after its expression, so guard expressions in params would end up last, breaking this. As an alternative, the params could be lowered first (rust-lang#141356).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2025
Rollup merge of rust-lang#141356 - dianne:thir-lower-params-before-body-expr, r=compiler-errors

lower bodies' params to thir before the body's value

Two motivations for this:
- Lowering params first means errors from lowering the params are emitted before errors from lowering the body's expression. This comes up in [tests/ui/associated-consts/associated-const-type-parameter-pattern.stderr](https://github.com/rust-lang/rust/compare/master...dianne:rust:thir-lower-params-before-body-expr?expand=1#diff-acac6ea10e991af0da91633e08b2739f9f9ca0c8f826401b6ba829914d0806f2), where both the params and expression encounter errors in translating consts to patterns. This change puts the errors in the order they appear in the source file.
- Guard patterns (rust-lang#129967) contain expressions, so lowering params containing guard patterns may add more expressions to the THIR. However, there's a check for `-Zunpretty=thir-tree` that the final expression in the THIR corresponds to its value [(link)](https://github.com/rust-lang/rust/blob/c43786c9b7b8d8dcc3f9c604e0e3074c16ed69d3/compiler/rustc_mir_build/src/builder/mod.rs#L453-L455); lowering params last would break this. As an alternative way to get guard patterns to work, I think the pretty-printer could use the expression returned by `thir_body` and the check could be removed or changed (rust-lang#141357).
@dianne dianne deleted the thir-lower-params-before-body-expr branch May 22, 2025 11:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants