Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustfmt adds useless brackets to unit value in match when pattern is too long #5134

Closed
zombiepigdragon opened this issue Dec 12, 2021 · 5 comments

Comments

@zombiepigdragon
Copy link

zombiepigdragon commented Dec 12, 2021

A simple reproducing case (assumed to be the correct formatting) is

fn demo() {
    if a {
        if b {
            match c {
                DemoItemA
                    if example_var == "val1" && example_var_2 == "get to the column limit" => (),
            }
        }
    }
}

which rustfmt will format to

fn demo() {
    if a {
        if b {
            match c {
                DemoItemA
                    if example_var == "val1" && example_var_2 == "get to the column limit" =>
                {
                    ()
                }
            }
        }
    }
}

This is particularly irritating as it triggers a clippy lint, leaving the code with a choice between being correct in clippy's or rustfmt's eyes. Without looking into the rustfmt source to troubleshoot, it seems as if rustfmt is generating the line with the brackets, seeing it passes the column limit, and laying out the code accordingly instead of removing the useless brackets.

Tested in versions rustfmt 1.4.37-stable (f1edd04 2021-11-29) and rustfmt 1.4.38-nightly (0b42dea 2021-12-09)

@bddap
Copy link

bddap commented Dec 15, 2021

@zombiepigdragon consider using this, which is equivalent:

fn demo() {
    if a {
        if b {
            match c {
                DemoItemA
                    if example_var == "val1" && example_var_2 == "get to the column limit" => {}
            }
        }
    }
}

That's what that clippy lint is trying to suggest.

assert_eq!((), {});

@ytmimi
Copy link
Contributor

ytmimi commented Dec 15, 2021

@bddap Thank you for clarifying what the clippy lint was pointing out! @zombiepigdragon I think that's the approach you should take. If you're using nightly rustfmt you might also consider using the match_arm_blocks = false configuration option. When you get a chance, can you confirm if either of these approaches helped?

@zombiepigdragon
Copy link
Author

@bddap Yes, that does help. Thank you! I've switched it over for the time being.
@ytmimi I tried using match_arm_blocks, and it doesn't introduce the {} block, but it does move the () to a new line, which isn't ideal, but does avoid clippy.
Is there an official preference between using {} and () as unit values in match arms? I've been in the habit of using () since I started using Rust and it looks more visually consistent (imo) than an empty block, because most of the arms call a function that returns (). If () in this context isn't abnormal, I'd still consider this incorrect formatting. Am I correct?

@calebcartwright
Copy link
Member

I'd still consider this incorrect formatting. Am I correct?

No. Correct/expected formatting isn't based on any subjective preference, but on the rules codified in the Rust Style Guide which are very explicit and prescriptive about all scenarios including this one

https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/expressions.md#match
https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/expressions.md#line-breaking

Particularly take note of the following lines from the guide:

Never break after => without using the block form of the body.

If the left-hand side must be split and there is an if clause, break before the if and block indent. In this case, always use a block body and start the body on a new line:

rustfmt remains highly configurable because everyone recognizes that few topics around styling will receive ubiquitous agreement, and that's why options like match_arm_blocks exist with non-default options that support formatting variations that differ from the style guide; rustfmt's defaults, however, must align.

Generally speaking, the style guide and rustfmt are focused on how the code written by the developer (or more specifically the various representations of code in the AST) should be formatted as opposed to what code the developer should write in cases where there's several syntactically viable alternatives.

Whether you want the empty/unit tuple for that arm to be explicit or implicit is really up to you. We may incorporate some more explicit guidance specifically for the case of such match arms (refs rust-lang/style-team#146) and that is leaning towards the implicit route, though in the same theme as described above we'd likely have a config option in rustfmt that allows flexibility for user preference.

@calebcartwright
Copy link
Member

Thanks again for reaching out, but going to close given the above context on why the behavior is correct along with options/workarounds for alternatives

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants