# Review of rustfmt v2 items
###### tags: `deep-dive`

https://github.com/rust-lang/rustfmt/issues/5577

## 2023-02-01

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1305649570 - hard_tabs (Indentation for generics on impl blocks)
* Caleb: Thinks this is a bug fix
* Josh: Absence of hard tabs, these wouldn't be 8-space indented?
* Caleb: Correct
* Josh: Seems a bugfix

VERDICT: Bugfix

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1305717684 - Indentation for generics on impl blocks (No additional options)
* Josh: Same issue as the previous one, just with the default 4-space instead of tabs?
* Caleb: What I think is happening in this example is the alignment of `<` and indentation of the bounds. Matter of where (what starting point?)
* Caleb: Hope the style guide doesn't prescribe v2's behavior

* Caleb: Bugfix, need more style guide examples. Hard-tabs example just exacerbates the existing bug.

* Caleb: Tricky on the rustfmt side to add clarity to the existing style guidance, does rustfmt default then need fixing? Recommendation -- add clarity to the new edition, but keep existing behavior in old editions?
* Josh: Would be substantial churn to change old behavior, bugfixes would only go into 2024 style edition unless flagged otherwise.
Michael: Can't be fixed in a backwards compat manner, so part of future edition. Maybe notes that it's a bugfix in the new style guide, but doesn't need to be prescriptive part of the style.
Josh: Can link to bugfixes in 5577.

VERDICT: Bugfix

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1315302533 - Don't align unrelated trailing comments after items or at the end of blocks

* Michael: Didn't know we even did that.
~* Josh: This is definitely a bugfix.~
* Josh: Is there no way we could do this as a retroactive bugfix for <2024?
* Caleb: There's a mix, some people like this style and bring pitchforks! Don't think this is something we can change.

* Caleb: Might be some other case where we do this alignment in a list context.
* Josh: One case I don't see: trailing comments in fn parameters.
* Caleb: That's the one I'm thinking of.

* Josh: Is there anything in the style guide that talks about comments being lined up?
* Caleb: Someone asked for it, someone implemented it, then it became the default.
* Caleb: One more caveat -- we could stop doing this behavior, but it would be done in a way that takes the comments' position depending on how they're already aligned.
* Josh: Hesitant to suggest new mechanisms that depend on pre-existing formatting. Glad that rustfmt is independent of existing formatting.

VERDICT: Uncontroversial, but not a bugfix -- should be specified in the 2024 style guide
Josh: We don't need to specify the 2015 behavior

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1321035553 - Imports with raw identifiers reorder_imports=true (default)

* Josh: What does (default) mean?
* Caleb: This option is true by default.

VERDICT: Uncontroversial, but should be specified in the 2024 style guide.
~~Caleb to check if this could be done in 2015?~~

Josh: Suggestion withdrawn, we should stand on principle to not churn existing code.

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1321330401 - Force block closures for both for and while loops

* Josh: What's the behavior of `for` loops? It's ambiguous from the example.
* Josh: Isn't there a configurable option to change this behavior?
* Caleb: There's an overflow one, also have an option that allows people to always put blocks or try to preserve the block if you wrote it.
* Josh: Don't have an opinion on context-dependent formatting when it's opt-in.

* Josh: Consistency is a good thing, but we need to review further.

VERDICT: Review further, we could do either way.

* [2023-02-02 Update]: the text of this item has been corrected, as it had some confusing/mistaken info in the original posting when we reviewed. Caleb believes this constitutes a bugfix as the Style Guide is currently written because the Style Guide requires braces for closure bodies when the body contains a multi-line control flow expression

https://github.com/rust-lang/rust/blob/master/src/doc/style-guide/src/expressions.md#closures
https://github.com/rust-lang/rust/blob/master/src/doc/style-guide/src/expressions.md#control-flow-expressions

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1321344498 - Use correct indent when formatting complex fn type

* Michael: Looks like a bugfix.

* Josh: Clarifying something here - good that `Fn` is on a line of its own and args are aligned.
* Josh: `Fn` should be grouped with the rest of the auto traits.

* Josh: What does it do in other positions like WC?
* Caleb: Maybe this is one we need to discuss?

VERDICT: Review further. V2 is better, but needs further discussion of the indentation of auto trait bounds in `impl` in this example.

## 2023-02-08

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1321354613 - Consistency between function and macro when overflowing method calls

* Caleb/t-rustfmt perspective: likely improvement, my recollection is that style guide is light on macro prescriptions
* meeting conclusion: likely improvement but needs further discussion, potential breakage concerns around how a whitespace sensitive macro might interact with tokens from different editions that are formatted in different style editions and making sure that macro authors can maintain compatibility with mutiple editions, concern about macros being able to depend on span information for logic, potential desire to disallow that going forward

Jane: Is there a need for a language feature to handle the case where macros are sensitive to formatting changes like this? How do we navigate edition changes for these cases?
Michael: Macros are subject to the formatting rules of their call-site. Needs further discussion about possible cases where whitespace-sensitive macros are changed due to style changes in 2024.
Josh: Are we okay with breaking whitespace-sensitive macros?
Michael: Yes, but we should acknowledge it explicitly.
Jane: No objection to breaking, open to other users' objections. Want to provide additional tools to navigate change.
Josh: Open to user objections, but currently only aware of stunt-macros that rely on this behavior. Other crates like indoc may get creative on the way they rely on whitespace. Want to hear from users who have real-world non-stunt macros this would harm.
Jane: displaydoc and expanddoc may rely on whitespace?

(2023-02-15 Update)
Subsequent chat in Zulip https://rust-lang.zulipchat.com/#narrow/stream/353175-t-style.2Fprivate/topic/meeting.20availability.2002-08

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1321519237 - Consistency between function and macro when deciding to overflow items or keep them on the same line

* Caleb/t-rustfmt perspective: likely improvement, my recollection is that style guide is light on macro prescriptions

Jane: Same exact concern as above.
Michael: Don't know what "macro context" means?

Defer, wait to discuss with Caleb.

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1322955255 - Wrap long arrays and slice patterns

* Caleb/t-rustfmt perspective: bugfix, [style guide prescriptions for patterns](https://github.com/rust-lang/rust/blob/master/src/doc/style-guide/src/expressions.md#patterns) states that patterns should be formatted like their corresponding expression, so slice patterns should, ostensibly, be formatted like arrays

Josh: Obvious win

Concur that this is a bugfix

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1322963218 - Nested tuples access without spaces

* Caleb/t-rustfmt perspective: not sure how to classify this one, but I suppose it's a formatting improvement since there's no explicit style guide rules on this front that are being violated AFAIK. This was something we begrudgingly had to do as I summarized in https://github.com/rust-lang/rustfmt/issues/4355#issuecomment-723764276

Jane: _exclaims_ Oh I'm familiar with this one
Josh: First rustc had to be fixed, then rustfmt can be updated to support this.
[some aside about lexer being cursed]

Unambiguous improvement, probably not a bugfix because it's a rustc problem that was fixed

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1331585855 - Don't Change multi-line string literal indentation inside macro

* Caleb/t-rustfmt perspective: most likely a bugfix, as rustfmt shouldn't have been messing with those indents to begin with

Josh: We're not supposed to be changing these in the first place...
Josh: Why precisely are we not supposed to change this?
Michael: This is an inconsistency that we need to discuss. Same as above macro discussions, but in the other direction.

Needs further discussion, seems inconsistent with proposed changes above

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1331592380 - Don't indent strings within comments

Josh: Wishes there was an original, then the output of both formattings.
Josh: Seems like a bugfix,
michael agrees.

Bugfix

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1331628360 - Don't wrap string literals in Option with wrap_str when each line ends with line continuation (\\) and format_string=false and Version=Two.

Josh: format_string=true will break strings, =false is default and won't touch them.
[team tries to find out what's going on here]
Josh: Possibly a case of rustfmt doing its cascading giving-up-on-formatting behavior. V2 is not wrapping the long string literal, but will account for it being long by wrapping the rest of the expression.
Josh: Cases 1 and 2 are examples where the behavior is working.
Josh: Thinks it's a bugfix, but example 2 is not doing something we think it should be doing.

This input:

fn main() {
    // long string literal in chains prevent formatting and are formatted the same for `Version::One` and `Version::Two`.
    something.something(|| {
// ...
}).unwrap_or_else(|| "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. \
Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. \
Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.");
}

Should look like:

fn main() {
    // long string literal in chains prevent formatting and are formatted the same for `Version::One` and `Version::Two`.
    something.something(|| {
        // ...
    }).unwrap_or_else(|| {
        "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. \
Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. \
Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."
    });
}

Bugfix, but further discussion

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1345037570 - Break function return types before -> instead of breaking between () for zero argument functions

Jane: Has an example
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=06e2830d4e4f872f05767870bce6d7ac

Michael and Josh both think the v1 example looks better here, because it's easier to add a function parameter.
Jane: Wants us to make sure we're being consistent with other cases that may wrap similarly.
Josh: Believe that v2 formatting may be inconsistency, requiring special case in the style guide "except if it's a zero-arg function". :+1: for verifying that though.

Needs discussion

## 2023-02-15

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1345068602 - Trailing semicolon for return statements inside a match arm

Semicolon being added

Josh: expected because return statement inside a block
Jane: initially thought it was being removed
Josh: end of a function, `return` without semi will result in rustfmt adding a semi today; this seems consistent

Consensus: bugfix, favor proceeding with v2

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1345097702 - Format the last expression-statement as expression

Josh: surprised closure body braces not removed
Caleb: style guide rule relative to control flow/if else as the sole closure body item
Caleb: Doublechecked style guide, it says "if it's multiple lines *and* it's a control-flow expression"
Josh: This is another bug
Jane: Is rustfmt idempotent across runs, or does using v2 potentially allow for rustfmt to change the output of prior rustfmt output
Caleb: Idempotent
Josh: Style Guide needs updates because it contains a snippet that aligns with current v2 behavior

Consensus: bugfix, plus another bug found both in rustfmt and style guide snippet for closures
https://github.com/rust-lang/rust/blob/master/src/doc/style-guide/src/expressions.md#closures

This should seemingly be single lined and the closure body doesn't need braces

|| {
    if true {
        blah
    } else {
        boo
    }
}

Similarly, the following should use one line:

|| Foo {
    field1,
    field2: 0,
}

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1407489330 - Proper indention of single generic bound with multiline formatting

Consensus: Obvious bugfix

### Revisiting of prior items

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1321354613 - Consistency between function and macro when overflowing method calls

Subsequent chat in Zulip https://rust-lang.zulipchat.com/#narrow/stream/353175-t-style.2Fprivate/topic/meeting.20availability.2002-08

Jane: Offline conensus reached, no real use cases known but we'll consider individual cases in the future if whitespace-significant macros come to the surface

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1321519237 - Consistency between function and macro when deciding to overflow items or keep them on the same line

Jane: Same conensus as previous item
Caleb: Agreed
Josh: Also discussed on Zulip
Caleb: correct

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1322963218 - Nested tuples access without spaces

Some discussion about how this means rustfmt modifies code to make it dependent on a version of rustc that supports this bugfix.

## 2023-03-01

All items have been reviewed at least once, all the below are follow up reviews to items that were visited during a prior meeting, but either needed additional discussion and/or input from a team member that wasn't present during the prior review

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1321344498 - Use correct indent when formatting complex fn type

Josh: Would be nice if the indentations of Fn and Send/Sync/etc made it obvious that they went together.

Seem to have consensus that the V2 version is better than V1, just seeking possibilities for doing even better.

Conclusion: Let's ship it. If we come up with something better, we can change 2024 if it hasn't stabilized yet, or 2027 if it has.

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1331585855 - Don't Change multi-line string literal indentation inside macro

(desire for additional team discussion + input from caleb/t-rustfmt)

Caleb: this is another case of confusing examples. The specifics of this one are related to a combination of fixing an idempotency bug without impacting a fix for strings in comments - https://github.com/rust-lang/rustfmt/pull/3326#issuecomment-462044852

Caleb: the snippets are essentially highlighting the _lack_ of the idempotency bug

Verdict: Bugfix

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1331628360 - Don't wrap string literals in Option with wrap_str when each line ends with line continuation (\\) and format_string=false and Version=Two.

ACTION ITEM: @wXuA_el9SUuHDZHu5dU4JQ (caleb's other nickname) to clarify

(desire for additional team discussion + input from caleb/t-rustfmt)

https://github.com/rust-lang/rustfmt/issues/5577#issuecomment-1345037570 - Break function return types before -> instead of breaking between () for zero argument functions

(desire for additional team discussion)

Josh: Mild preference for v1, suggest we go back to v1. Makes it easier to add additional arguments in the future
Josh: Was there a rationale here or something we're missing?
Caleb: Not from what I remember. This was a case of a rustfmt contributor thinking this formatting was better, putting up a change, and having it merge for v2.

Jane: Proposes a verdict -- we should revert back to the v1 formatting here.

VERDICT: we should revert back to the v1 formatting here