Improve selection handling for the `merge_match_arms` assist

[cmrschwarz]

This just fixes a minor annoyance where the merge_match_arms assist is not available if your selection has leading whitespace.

Example case that didn't work before but does now:

enum X { A, B, C }
fn main() {
    match X::A {
    $0  X::A => 0,
        X::B => 0,$0
        X::C => 1,
    }
}

Edit: It now also fixes that this assist would "overrun" a selection in a case like this, where the user intended to only merge the first two elements:

fn main() {
    match X::A {
    $0  X::A => todo!(),
        X::B => todo!(),$0
        X::C => todo!(),
    }
}

Edit2: It now no longer applies this selection restriction for small / potentially accidental selections like this:

fn main() {
    match X::A {
        X::A => $0todo$0!(),
        X::B => todo!(),
        X::C => todo!(),
    }
}

Should be pretty good now! Sorry for not getting it right the first time.

[cmrschwarz]

This never bothered me, but I suppose it's fine.

I suppose most people currently use this assist just with a cursor, not a selection. But using a selection has always felt more intuitive to me, so I thought why not show the usecase some love. I run into this about five times a day ^^.

Can you squash, please?

Squashed :saluting_face:

[cmrschwarz]

LGTM from implementation perspective, but we need to decide if we want to apply this globally, because there are some more assists that this principle could be applied to.

I think you are right, there are quite a few assists that currently do not think about selections and treat them like a single cursor.

crates/ide-assists/src/handlers/** currently has 143 mentions of find_node_at_offset.

I'll say it upfront, I don't have time right now to do all of them :sweat_smile: .

I quickly found one where it would probably make sense to use find_node_at_trimmed_offset instead: remove_parentheses. It took me like half an hour to verify that this did not break everything, and it turned out to be an easy one because it already guarded against parentheses outside of a selection at least. Many assists like this one unfortunately don't seem to have test cases for selection at all.

Here are the new regression cases for that one, although that should probably be a separate PR? @ChayimFriedman2 :

#[test]
fn remove_parens_in_selection_with_whitespace() {
    check_assist(
        remove_parentheses,
        r#"fn f() {$0 ($0 1 + 2 ) + 3; }"#,
        r#"fn f() { 1 + 2 + 3; }"#,
    );

    check_assist(
        remove_parentheses,
        r#"fn f() { (1+((2)$0 )$0 + 3; }"#,
        r#"fn f() { (1+ (2) + 3; }"#,
    );
}

[ChayimFriedman2]

If we decide this is worth pursuing, we can fix them one at a time, but we need to decide if it is worth it.

[cmrschwarz]

we need to decide if it is worth it

I think everybody agrees that this is an improvement, as you say it's really just a question of whether it's worth the effort.

So maybe it's not necessary to make any decision at all? What's worth the effort is subjective, and whoever deems it worth the effort for should just send the PR? Slowly improving in this area over time, one assist at a time seems totally fine to me.

But that's just my two cents, let me know if there's anything I can do for this PR specifically.

[ChayimFriedman2]

I'm not sure I agree this is an improvement. I mean, sure this is, but I am not sure this slight improvement is worth even a slight addition to code complexity.

[lnicola]

I'm not sure it's an improvement, but the added code certainly isn't so bad, at least in this PR :laughing:.

[cmrschwarz]

I'm not sure I agree this is an improvement

The ability to merge some match arms even though all match arms still say todo!() (e.g. after using Fill Match Arms) is a real usecase that I frequently have, I'm not sure why you would say that is not an improvement?

[ChayimFriedman2]

Yeah, but whitespace trimming is not.

[cmrschwarz]

Yeah, but whitespace trimming is not.

Maybe that's just my incompetence / bad eyes, but when I select text in an editor using a Mouse, the chance of me including a stray leading whitespace character are about 30%.

A tool that should clearly be able to know what I want to do but refuses to do so because of a single space is frustrating and wastes time. I'm sorry but I don't see how that is not a real usecase for a day to day productivity tool like RA.

[ChayimFriedman2]

Maybe this is because of different workflows. I rarely use the mouse, only the keyboard and mostly "Expand Selection".

However this doesn't really have maintenance costs, so if it helps you I'm not opposed to adding this.

[lnicola]

I'm not sure why you would say that is not an improvement?

Assist fatigue, mostly. We have a lot of them (and we'll likely add more), and sometimes the one I want is at the bottom of the list.

Anyway, I think we can merge this and see if it becomes too annoying.