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

Enhance formatting for ConstBlock expressions #4483

Open
calebcartwright opened this issue Oct 20, 2020 · 7 comments
Open

Enhance formatting for ConstBlock expressions #4483

calebcartwright opened this issue Oct 20, 2020 · 7 comments
Labels
1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted

Comments

@calebcartwright
Copy link
Member

#4478 included some initial support for the new ConstBlock expression kind variant, but there's opportunities to enhance and improve the formatting.

The relevant section of the codebase can be found in formatting/expr.rs

ast::ExprKind::ConstBlock(ref anon_const) => {
Some(format!("const {}", anon_const.rewrite(context, shape)?))
}

There are 3 core activities to completing this issue:

  1. Honor the brace_style configuration option
  2. Account for any comments between the const keyword and the block
  3. Add test cases

1. brace_style

This should be relatively easy. When brace_style is set to AlwaysNextLine (can be checked with context.config.brace_style()) then we'll want to insert a newline and start the block on the next line (the correct newline and indentation can be achieved via shape.to_string_with_newline(context.config)), otherwise there should just be a space between the keyword and block

2. comments

First step will be to determine the span between the end of the const keyword and the start of the block. Consider using context.snippet.span_after to figure out the lo for this "between" span, and for the hi you will want to use the lo of the block (anon_const.value.span.lo())

Once you have that span there are a few different options and comment utility functions for formatting the expression and ensuring comments are properly formatted. You may want to take a look at one or more of them:

  • contains_comment
  • combine_strs_with_missing_comments
  • rewrite_missing_comment
  • recover_missing_comment_in_span

3. tests

Below are a few (nonexhaustive) set of tests to cover some top of mind scenarios.

You will probably want to add a const_block_always_next_line.rs file under the configs/brace_style directory (https://github.com/rust-lang/rustfmt/tree/master/tests/source/configs/brace_style) for both tests/source and tests/target

fn foo() -> i32 {
    const {
        let x = 5 + 10;
        x / 3
    }
}

fn bar() -> i32 {
    const { 4 }
}

fn foo() -> i32 {
    const
{
        let x = 5 + 10;
        x / 3
    }
}

fn foo() -> i32 {
    const // baz
{
        let x = 5 + 10;
        x / 3
    }
}

fn foo() -> i32 {
    const /*qux */ {
        let x = 5 + 10;
        x / 3
    }
}

fn foo() -> i32 {
    const
// baz
{
        let x = 5 + 10;
        x / 3
    }
}
@calebcartwright calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted hacktoberfest labels Oct 20, 2020
@frbimo
Copy link
Contributor

frbimo commented Oct 22, 2020

@calebcartwright any mentoring available? just want to confirm if i'm facing a wall later.

@calebcartwright
Copy link
Member Author

Sure! You can reach us here and/or in the wg-rustfmt channel on Discord to discuss things, ask questions, etc.

@frbimo
Copy link
Contributor

frbimo commented Oct 23, 2020

@calebcartwright ok. btw, may i know the test file you post is source's or target's?

@calebcartwright
Copy link
Member Author

@frbimo

@calebcartwright ok. btw, may i know the test file you post is source's or target's?

Not sure I understand your question. Are you referring to the snippets I included above in the issue description? If so, those are just some ideas for using with the various test source files and configurations. They were all intended to be used as pre-formatting, as several of those are definitely not formatted correctly.

Disclaimer that this is eyeball formatted with GitHub's inline editor, but the correct/formatted (target) output for those with default configuration would be

fn foo() -> i32 {
    const {
        let x = 5 + 10;
        x / 3
    }
}

fn bar() -> i32 {
    const { 4 }
}

fn foo() -> i32 {
    const {
        let x = 5 + 10;
        x / 3
    }
}

fn foo() -> i32 {
    const // baz
    {
        let x = 5 + 10;
        x / 3
    }
}

fn foo() -> i32 {
    const /*qux */ {
        let x = 5 + 10;
        x / 3
    }
}

fn foo() -> i32 {
    const
    // baz
    {
        let x = 5 + 10;
        x / 3
    }
}

@ytmimi ytmimi added the 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release label May 12, 2022
@ytmimi
Copy link
Contributor

ytmimi commented May 12, 2022

This can likely be closed when we backport #5289

@calebcartwright
Copy link
Member Author

This can likely be closed when we backport #5289

Perhaps, but likely depends on the outcome of outstanding discussion in #3376

We were originally carrying on with applying the existing options in additional contexts on the 2.x branch, but after some reflection I really don't think that's the right way to go. We will support a brace style option on the const block expressions, but how/under which config option(s) remains an open question

@ytmimi
Copy link
Contributor

ytmimi commented May 13, 2022

Got it. Thanks for giving me some more context on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted
Projects
None yet
Development

No branches or pull requests

3 participants