normalize match reference style, #929

[flier]

match_reference_style

Normalize match reference style

• Default value: manual
• Possible values: manual, reference, dereference, auto
• Stable: No

Note: auto style need Rust v1.26.0 or later

manual

don't touch anything

reference

fn hello(name: &Option<&str>) {
    match name {
        &Some(name) => println!("Hello {}!", name),
        &None => println!("I don't know who you are."),
    }
}

dereference

fn hello(name: &Option<&str>) {
    match *name {
        Some(name) => println!("Hello {}!", name),
        None => println!("I don't know who you are."),
    }
}

auto

fn hello(name: &Option<&str>) {
    match name {
        Some(name) => println!("Hello {}!", name),
        None => println!("I don't know who you are."),
    }
}

[nrc]

Thanks for the PR! This is a really cool thing to add! I highlighted a couple of items inline, but I think it also needs a few more tests, e.g., using ref, mut, and ref mut, using some more complex patterns (e.g., deeper nesting, @ variables, and wildcard matching), perhaps some patterns with Copy types, etc.

[nrc]

I think this should &A

[flier]

Rust 1.26 has stabilized match_default_bindings

the compiler automatically references the Some, and since we’re borrowing, name is bound as ref name automatically as well. If we were mutating:

fn hello(arg: &mut Option<String>) {
    match arg {
        Some(name) => name.push_str(", world"),
        None => (),
    }
}

the compiler will automatically borrow by mutable reference, and name will be bound as ref mut too.

[nrc]

Hmm, then should it be *x? In any case the comments here don't seem to match the examples in the PR message. Perhaps it would be good to explain the reference/dereference modes here.

What is the motivation for having four modes? I would have thought that auto and manual would be the most widely used.

[flier]

Before Rust 1.26, you can choose reference/dereference modes as you wish, just different code style in same semantic in most cases.

the compiler automatically references the Some, and since we’re borrowing, name is bound as ref name automatically as well.

Since Rust 1.26, the compiler could do smarter, it can automatic handle the reference/dereference modes base on the type inferencing, that's why we need a new auto mode.

fn hello(name: &Option<&str>) {
    match name {
        Some(name) => println!("Hello {}!", name),
        None => println!("I don't know who you are."),
    }
}

fn main() {
    let name = Some("world");

    hello(&name);
}

So, if we use Rust 1.26 or later, just set the auto mode and migrate the old code, let's compiler do the dirty works.

[nrc]

Right, but since 1.26 is stable now, I think we could just offer auto and manual, and we don't need to offer ref and deref styles. Do you think there is an advantage to continue to offer ref and deref?

[flier]

The problem is some projects may still using the old Rust version, they may need to align the code style with rustfmt for ref/deref styles.

[nrc]

"need" should be "requires"

[clarfonthey]

This should have checks to make sure that Copy fields work as expected. For example:

let var;
match &Some(2) {
    None => return,
    &Some(x) => var = x,
}

This works and correctly sets var = 2. However:

let var;
match &Some(2) {
    None => return,
    Some(x) => var = x,
}

This is not equivalent, as it sets var = &2; you'd have to specifically say var = *x here. I think that moving between ref/deref styles is fine, but moving to default bindings does require checking types, and I'm not sure if it's the right thing for rustfmt and not for clippy.

Unless rustfmt's goal is to do transformations which involve type checking. In that case, I'd say this is okay.

[nrc]

Unless rustfmt's goal is to do transformations which involve type checking

Rustfmt does not have type info, so it can't check this sort of thing.

[nrc]

Sorry I've left this without a comment for so long. I'm not sure what to do here. I'm not keen to have rustfmt make changes unless we can guarantee they are correct and I don't think it is possible to identify a subset of changes such that they will always be correct (without type info). Do you think there is such a way to identify correct changes?

[flier]

In fact, no, because we haven't enough type info.

I think we can set it to manual by default, and let's user choose to use it or not, because they at least has an automatic tools to help the migration better than nothing, and we can fix the issue later base on the feedback.

[nrc]

Given the lack of a 'correct' way to do this in Rustfmt, I think it would be better for this to live in rustfix.

Thanks for the PR, and sorry it didn't land.