Stop using the `unpack!` macro in MIR building by Zalathar · Pull Request #127416 · rust-lang/rust

Zalathar · 2024-07-06T08:18:30Z

MIR building currently has an unpack! macro (documented in the dev guide) that is used to unpack values of BlockAnd<T>, by assigning the block to a variable and then returning the value. It is used in around 100 places in the rustc_mir_build crate.

The downside is that understanding this code requires understanding a custom macro, which to me seems like a poor tradeoff relative to the modest benefits of using the macro.

To explore this, I've prepared a PR that removes the unpack! macro completely, replacing it with a small number of ordinary methods on the BlockAnd type.

In the majority of cases, we replace this code:

// Note that the actual type of `self.foo(...)` does not match `value` or `block`.
let value = unpack!(block = self.foo(...));

With this code:

// This is just normal Rust code.
let value = self.foo(...).unpack(&mut block);

Zulip thread

rustbot · 2024-07-06T08:18:37Z

r? @fmease

rustbot has assigned @fmease.
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

Zalathar · 2024-07-06T08:29:53Z

One of the things I strongly dislike about unpack! is that it has two different forms that are inconsistent with each other. One assigns the block and returns the value, while the other returns the block and discards the value (which must be ()).

rustbot · 2024-07-06T08:30:11Z

Some changes occurred in match lowering

cc @Nadrieril

fmease

Thanks, that's a lot nicer!

r=me with feedback addressed in one way or another

fmease · 2024-07-07T15:42:46Z

compiler/rustc_mir_build/src/build/mod.rs

+    ///
+    /// This is similar to writing `let _ = ...`, except that it is also
+    /// (a) postfix, and (b) searchable.
+    fn unpack_discard(self) {}


It's not really unpacking anything. I'm not super convinced by this method, I'd just use let _ = … as the docs mention but er, it's fine.

fmease · 2024-07-07T15:52:48Z

compiler/rustc_mir_build/src/build/expr/into.rs

                // Unpack `BlockAnd<BasicBlock>` into `(then_blk, else_blk)`.
-                let (then_blk, mut else_blk);
-                else_blk = unpack!(then_blk = then_and_else_blocks);
+                let (then_blk, mut else_blk) = then_and_else_blocks.unpack_to_pair();


Why not just

Suggested change

let (then_blk, mut else_blk) = then_and_else_blocks.unpack_to_pair();

let BlockAnd(then_blk, mut else_blk) = then_and_else_blocks;

this would allow us to get rid of unpack_to_pair which essentially converts a tuple struct to a tuple.

fmease · 2024-07-07T15:54:16Z

compiler/rustc_mir_build/src/build/scope.rs

-        let mut block;
-        let rv = unpack!(block = f(self));
+
+        let (mut block, rv) = f(self).unpack_to_pair();


Suggested change

let (mut block, rv) = f(self).unpack_to_pair();

let BlockAnd(mut block, rv) = f(self);

ditto

Nadrieril · 2024-07-07T18:26:28Z

compiler/rustc_mir_build/src/build/expr/as_operand.rs

            Category::Constant | Category::Place | Category::Rvalue(..) => {
-                let operand = unpack!(block = this.as_temp(block, scope, expr_id, Mutability::Mut));
+                let operand =
+                    this.as_temp(block, scope, expr_id, Mutability::Mut).unpack(&mut block);


I have mixed feelings about this, the flow of block is easier to follow with the macro imo. Have we considered:

Suggested change

this.as_temp(block, scope, expr_id, Mutability::Mut).unpack(&mut block);

this.as_temp(&mut block, scope, expr_id, Mutability::Mut);

I quite like using &mut block as an in+out parameter, and have used it in some of my previous coverage work.

The main reason I didn't try to use it here was to keep this PR as modest as possible. But it's certainly something I'd like to explore.

Nadrieril · 2024-07-07T18:29:56Z

compiler/rustc_mir_build/src/build/mod.rs

+impl BlockAnd<()> {
+    /// Unpacks `BlockAnd<()>` into a [`BasicBlock`].
+    #[must_use]
+    fn unpack_block(self) -> BasicBlock {


The name wasn't clear to me. How about unpack_unit?

I might just try unpack_block_and_unit instead.

Zalathar · 2024-07-07T23:59:43Z

After considering feedback, I think my current plan is to split this up into 3 stages:

Extract the BlockAnd<()> changes into a separate smaller PR to land first (with feedback addressed).
Replace unpack! with destructuring assignment on BlockAnd in most places, but leave unpack! in the minority of cases where shadowing makes this awkward.
Leave the remaining awkward cases (and the complete removal of unpack!) for a later PR where the stakes are lower and it's easier to agree on a good replacement.

This kind of unpacking can be expressed as an ordinary method on `BlockAnd<()>`.

MIR building: Stop using `unpack!` for `BlockAnd<()>` This is a subset of rust-lang#127416, containing only the parts related to `BlockAnd<()>`. The first patch removes the non-assigning form of the `unpack!` macro, because it is frustratingly inconsistent with the main form. We can replace it with an ordinary method that discards the `()` and returns the block. The second patch then finds all of the remaining code that was using `unpack!` with `BlockAnd<()>`, and updates it to use that new method instead. --- Changes since original review of rust-lang#127416: - Renamed `fn unpack_block` → `fn into_block` - Removed `fn unpack_discard`, replacing it with `let _: BlockAnd<()> = ...` (2 occurrences) - Tweaked `arm_end_blocks` to unpack earlier and build `Vec<BasicBlock>` instead of `Vec<BlockAnd<()>>`

Rollup merge of rust-lang#127472 - Zalathar:block-and-unit, r=fmease MIR building: Stop using `unpack!` for `BlockAnd<()>` This is a subset of rust-lang#127416, containing only the parts related to `BlockAnd<()>`. The first patch removes the non-assigning form of the `unpack!` macro, because it is frustratingly inconsistent with the main form. We can replace it with an ordinary method that discards the `()` and returns the block. The second patch then finds all of the remaining code that was using `unpack!` with `BlockAnd<()>`, and updates it to use that new method instead. --- Changes since original review of rust-lang#127416: - Renamed `fn unpack_block` → `fn into_block` - Removed `fn unpack_discard`, replacing it with `let _: BlockAnd<()> = ...` (2 occurrences) - Tweaked `arm_end_blocks` to unpack earlier and build `Vec<BasicBlock>` instead of `Vec<BlockAnd<()>>`

bors · 2024-07-18T00:52:05Z

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

Zalathar · 2024-07-26T05:57:10Z

Having dealt with the BlockAnd<()> cases, the rest of this feels a lot less compelling to me, especially since the alternatives aren't necessarily clear improvements.

So I think I'm going to close this for now.

rustbot assigned fmease Jul 6, 2024

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 Jul 6, 2024

Zalathar marked this pull request as ready for review July 6, 2024 08:30

fmease approved these changes Jul 7, 2024

View reviewed changes

fmease 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 Jul 7, 2024

Nadrieril reviewed Jul 7, 2024

View reviewed changes

Zalathar added 2 commits July 8, 2024 10:22

Remove the non-assigning form of unpack!

d9b0e97

This kind of unpacking can be expressed as an ordinary method on `BlockAnd<()>`.

Stop using unpack! for BlockAnd<()>

5066e6c

Zalathar force-pushed the unpack branch from 7299d1e to 03d264e Compare July 8, 2024 00:25

Stop using the unpack! macro

8357e4e

Zalathar force-pushed the unpack branch from 03d264e to 8357e4e Compare July 8, 2024 00:28

Zalathar mentioned this pull request Jul 8, 2024

MIR building: Stop using unpack! for BlockAnd<()> #127472

Merged

Zalathar closed this Jul 26, 2024

Zalathar deleted the unpack branch July 26, 2024 05:57

	let (then_blk, mut else_blk) = then_and_else_blocks.unpack_to_pair();
	let BlockAnd(then_blk, mut else_blk) = then_and_else_blocks;

	let (mut block, rv) = f(self).unpack_to_pair();
	let BlockAnd(mut block, rv) = f(self);

	this.as_temp(block, scope, expr_id, Mutability::Mut).unpack(&mut block);
	this.as_temp(&mut block, scope, expr_id, Mutability::Mut);

Uh oh!

Conversation

Zalathar commented Jul 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 6, 2024

Uh oh!

Zalathar commented Jul 6, 2024

Uh oh!

rustbot commented Jul 6, 2024

Uh oh!

fmease left a comment

Choose a reason for hiding this comment

Uh oh!

fmease Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

fmease Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

fmease Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

Nadrieril Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Nadrieril Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Jul 7, 2024

Uh oh!

bors commented Jul 18, 2024

Uh oh!

Zalathar commented Jul 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Zalathar commented Jul 6, 2024 •

edited

Loading