Unwrap block assist deletes entire match

[bjorn3]

rust-analyzer version: rust-analyzer version: 0.3.2188-standalone [/home/bjorn/.vscode/extensions/rust-lang.rust-analyzer-0.3.2188-linux-x64/server/rust-analyzer]

rustc version: N/A

editor or extension: VSCode

relevant settings: None

repository link (if public, optional): None

code snippet to reproduce:

fn foo() {
    match (name, mode) {
        (name, mode) => {// <--- cursor is here when using the "Unwrap block" assist
            panic!()
        }
    }
}

turns into

fn foo() {
    panic!()
}

[flodiebold]

What result would you have expected?

[bjorn3]

fn foo() {
    match (name, mode) {
        (name, mode) => panic!()
    }
}

[Giga-Bowser]

You're looking for an assist to remove the braces from a match expression (which I am coincidentally working on right now and should have PR for soon). Unwrap block is intended to turn an expression wrapping a block (if, for, while, etc.) into just the block itself. The names definitely can be confusing but I'm not sure of a better name.

[Giga-Bowser]

Maybe something like "Replace match with block"?

[bjorn3]

In my head I parse the { panic!() } part as an expression that happens to be a block, not as a special syntax construct for match arms. As such "unwrap block" would change it to panic!(). It should not affect the entire match as a match is not a block. Also if there are multiple match arms, rust-analyzer currently removes all match arms except for the one the cursor is located at, while "unwrap block" should never delete any code, just simplify it by removing redundant blocks.

[ChayimFriedman2]

"Unwrap block" remove code in other cases too, e.g. if condition.

[Giga-Bowser]

The intention with "Unwrap block" is not to pull out the tail expression of blocks with no statements (this is what panic!() is in your case). It is to pull out the body of control statements entirely. Let's say you had the following code:

if should_foo {
    foo();
}

Next, you restructure your program such that should_foo is always true. You might now want to replace the if statement with the body, since checking the condition is no longer necessary. In this case, you could use "Unwrap block", which would turn your code into just:

foo();

This is the intended behavior of "Unwrap block". And what you see when this behavior is applied to match expressions is intentional as well. It is actually checked in one of the tests written for the assist.

I am in total agreement that this is not a particularly descriptive name, and as a whole this functionality is dubiously useful for match statements. Matching variants can bring values into scope that don't exist outside of the match arm. I would not be opposed to not offering this assist for match expressions.

As for the functionality you intended, I opened PR #18539 which introduces an assist for it.

[bjorn3]

Do I understand it correctly that if I inline a function outside of a match arm I did have to keep using unwrap block to remove the curly braces that the inlining assist introduces, but if I inline a function inside of a match arm I did have to use your new assist?

[Giga-Bowser]

Yes, that is correct. And, to be fair, these are two different behaviors, as the assist for match statements is only offered when the block contains only one statement, and handles adding a comma if required. I don't disagree that having separate assists here is weird UX, if that's what you're hinting at. I would not be opposed to consolidating much of this functionality. Plenty of assists as it stand have different behaviors depending on the context they're applied in.

To be honest, I am more and more skeptical of the use of this existing unwrap block assist for control statements as time goes on. Outside of simple if statements, every control statement in Rust can introduce bindings and/or allow keywords (continue, break), which means the odds the user gets a block that doesn't throw errors are pretty slim. Plenty of the tests for that assist produce code that would obviously not compile.

I have marked my PR #18539 as a draft and I will be updating it to be a full-fledged consolidation of block wrapping and unwrapping assists.

[Veykril]

I've always hated that unwrap block didn't do what one would expect, just like this issue is raising. We should absolutely change the name of this (as well as add a new assist that does what one would expect here). Unwrap block from if expression etc might be more telling of what this actually does/targets (if being whatever target expression)