rust-lang / rust-analyzer

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

Unwrap block assist on match arm replacing entire expression #16629

Open ronnodas opened 2 months ago

ronnodas commented 2 months ago

rust-analyzer version: 0.3.1850-standalone

rustc version: rustc 1.76.0 (07dca489a 2024-02-04)

relevant settings: nothing obviously relevant

Using the "Unwrap block" assist at the marked cursor position in the code

fn foo() -> Option<()> {
    let foo = match "".strip_prefix("") {
        Some(_) => (),
        _ => |{
            return None
        }
    };
    Some(foo)
}

changes it to

fn foo() -> Option<()> {
    let foo = return None|;
    Some(foo)
}

which is unexpected and not equivalent.

l1nxy commented 2 months ago

@rustbot claim

l1nxy commented 2 months ago

This assist seems has many bugs ,according to the desc, it just remove the control statement and keep the body, and requires the user to make corrections. So, the code you provide has no problem with the assist. There a another example that i think it's a bug actually: Before:

fn foo2() {
    let x= if true {
        1
    } else {
          ^cursor here
        2
    };
}

After :

fn foo2() {
    let x= if true {
        1
    }
    2;
}

@Young-Flash What do you think? my thought is the code in this issue has no problem, and maybe should i open a new issue to fix thoes bugs?

Young-Flash commented 2 months ago

my thought is the code in this issue has no problem

IMO "Unwrap block" for the above code would better be:

fn foo() -> Option<()> {
    let foo = match "".strip_prefix("") {
        Some(_) => (),
        _ => return None

    };
    Some(foo)
}

which just remove the {}

cc https://github.com/rust-lang/rust-analyzer/issues/4156

l1nxy commented 2 months ago

@Young-Flash Hi, after reviewing #4156, I believe this assistance involves more than just removing {}; it also entails eliminating control statements or other elements while retaining the body. in fact, the rustfmt will remove redundant {}.

I will create a pull request to address this issue. It appears to be missing some expression tests like let match and let if, so the behavior is currently limited to standard match and if statements.

there is only one normal match test: https://github.com/rust-lang/rust-analyzer/blob/1f54f714caf7d38aaab9ac49ba25caa5abd52937/crates/ide-assists/src/handlers/unwrap_block.rs#L571-L593

l1nxy commented 1 month ago

@Young-Flash Hi, I reviewed the code and believe this issue is not a bug. The decision to unwrap the block should be made by the user. Please refer to https://github.com/rust-lang/rust-analyzer/issues/13679 and https://github.com/rust-lang/rust-analyzer/pull/13726 for more information.