rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.31k stars 1.62k forks source link

Unwrap block assist deletes entire match #18537

Open bjorn3 opened 2 days ago

bjorn3 commented 2 days ago

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 commented 1 day ago

What result would you have expected?

bjorn3 commented 1 day ago
fn foo() {
    match (name, mode) {
        (name, mode) => panic!()
    }
}
Giga-Bowser commented 1 day ago

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 commented 1 day ago

Maybe something like "Replace match with block"?

bjorn3 commented 1 day ago

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 commented 1 day ago

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

Giga-Bowser commented 1 day ago

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 commented 1 day ago

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 commented 19 hours ago

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.