Forced rustfmt is a roadblock to contributing

tools and infrastructure
84

The diff looks the same it git diff locally, which I use a lot during development. Having a better diff view somewhere is great, but doesn't solve this problem. The problem is the diff, not how it is rendered.

85

It's not like rustfmt will totally ignore any whitespace you have used. For example, if you use two lines in-between a series of statements, which I feel is one of the more common ways of using whitespace, then it's respected:

fn foo() {
    let foo = 0;

    let bar = 1;
}

One case where rustfmt does annoy me is when I'm writing something like:

let x = match foo { // Some comment
    ...
};

and rustfmt decides to move the comment down to the next line.

Is there some other particular case of rustfmt removing whitespace where it signalled semantic information that you feel is noteworthy?

Personally I think GitHub didn't do too bad a job there when you check "hide whitespace changes". The diff seems overall fine actually.

86

Fair.

Yeah, I have hit that, too. This gets particularly nasty around match arms (again matches...); I have to basically manually turn

  pat => // commant

into

  // comment
  pat =>

or else rustfmt will move the comment down and (IIRC) even add curly braces.

Anything column-aligned. Arrays are one case here, another is code like this:

short_stmt1;              // comment
longer_but_symmetric_smt; // comment

As part of rustc adopting rustfmt I saw a few cases of this, but forgot where unfortunately.

87

That's pretty much by design. rustfmt, in its default configuration, avoids ever aligning things at any point of the line other than the initial indentation, for a variety of reasons.

1 Like
88

We need to be careful to distinguish rustfmt actively choosing a non-trivial alignment (I agree with all the linked reasoning for why it doesn't do that), and rustfmt discarding / ignoring any non-trivial alignment made by the programmer. AFAIK the complaint has always been that it shouldn't discard / ignore as much as it does today.

89

Indeed. Gerrit should be mentioned too, which shares non-trivial ancestry with Critique. As a user of all three, I despise GH's review because I know it could be so, so much better. Over time, I've slowly come to believe that code formatting is at odds with the traditional way of presenting a diff to reviewers, especially since the former post-dates the latter. Critique in particular handles clang-format really well, and I consider clang-format to be much, much more aggressive than rustfmt.

I'd very much like a smarter tool than git diff for viewing local changes... its output feels more like the input for git apply than for human consumption. But I think this point is off-topic.

2 Likes
90

It will also remove braces, and sometimes commas, example of both

92

My big example is removing key-value pairs in assorted contexts, e.g.

let args = vec![
  "/usr/bin/curl",
  "-O",
  "--header", "X-Some-Header: Foo",
  "--cookie", "SomeCookie=Value",
  "https://some.url",
];

I find that this pairing of key-value pairs is much more clear and useful because of its semantic meaning than rustfmt deciding to put these all on one line (if there are few enough and they're short enough) or one-per-line (if they are long enough).

13 Likes
93

Holy crap, I didn't know about that setting! It's great.

1 Like
94

So, in the interest of avoiding dilution of responsibility, can we make a list of actionable tasks that could be undertaken to alleviate bookkeeping problems, and keep it somewhere visible (eg a RFC or whatever)?

Improvements proposed for rustfmt so far:

  • Change formatting rules to be more dependent on the input text.
    • In particular, have more generous threshold line lengths, so that changing a single character won't lead to an expression being immediately spread out / set on a single line.
    • Maybe try to respect expressions being grouped by line in argument lists (not sure how desirable this is).
  • Agree on a better for standard for when to use #[rustfmt::skip]. Right now using skip is considered taboo; maybe it would help to have a known page in the contributor's documentation saying "We know that in situations X, Y and Z, rustfmt's heuristics produce sub-optimal results; you're allowed to use skip provided you respect conditions J, K and L".
  • Improve rustfmt's internals to better recognize cases where dependent constructs need to be similarly laid out for visibility (maybe with user-provided hints?). For instance, rustfmt should recognize that a match block's arms should have the same indentation if possible.

Improvements proposed for the rust repository:

  • Change the CI to not block on formatting errors. (though it looks like doing so would cost a lot of server time? I'm really not familiar with the specifics here)
  • Change cargo test to be closer to x.py, so that users aren't taken by surprise when cargo test passes but their build fails on CI anyway.

It would also help if people willing to undertake these tasks stepped up. Pinging people who've been heavily involved so far: @kornel @burntsushi @CAD97 @RalfJung @tkaitchuck

7 Likes
95

Do you know the --ignore-all-space option? Personally I also sometimes use --word-diff=color.

96

Thank you for handling this!

Another good suggestion I've seen in this thread was to apply rustfmt by bors on merge. This way master would always be rustfmt-formatted, but individual PRs wouldn't have to worry about it.

2 Likes
97

I'd put it at low priority because of how controversial it, but yes, that too.

98

I don't know exactly how the rust-lang/rust repository CI is set up, but here's roughly how you'd do it in a GH workflow out of tree:

jobs:
  main:
    - uses: actions/checkout@v2
    - uses: actions-rs/toolchain@v1
    - run: cargo fmt -- --check
    - run: cargo clippy
      if: "!cancelled()"
    - run: cargo test
      if: "!cancelled()"
99

It seems like the two things are intimately connected and to meaningfully discuss one you have to discuss the other.

1 Like
100

Also, I'd be interested in working on rustfmt, provided I could find someone to mentor me through the codebase.

3 Likes
101

Turns out it doesn't do that everywhere though.

This also explains why the list of attributes for e.g. libstd, which had some semantic grouping to it, is now just one big thing. In particular, some of the comments only apply to the immediately following line, which used to be clear:

// std may use features in a platform-specific way
#![allow(unused_features)]

but now it became

// std may use features in a platform-specific way
#![allow(unused_features)]
#![cfg_attr(test, feature(print_internals, set_stdio, update_panic_count))]
#![cfg_attr(all(target_vendor = "fortanix", target_env = "sgx"),
            feature(slice_index_methods, coerce_unsized,
                    sgx_platform, ptr_wrapping_offset_from))]
#![cfg_attr(all(test, target_vendor = "fortanix", target_env = "sgx"),
            feature(fixed_size_array, maybe_uninit_extra))]
11 Likes
102
103

One thing I have noticed recently, is that having a form of persistent undo really changes my willingness to experiment with large refactors because of the ease at which it makes undoing those changes.

While it's theoretically possible to make rustfmt play nice with the editors concept of the undo stack, by injecting the difference into the editors undo stack. This sort of thing requires a lot of coordination between applications & as it is running rustfmt currently just truncates the undo stack, since my editor isnt able to apply deltas atop the externally modified files.

Anyhow, sort of a bummer at times.

2 Likes
104

The truly persistent form is called version control.

But yes, it'd be amazing if editors could integrate external formatters with the undo stack. Though said undo stack is still often finite; I lost some work a couple days ago to a cat walking over my keyboard while I was asleep and overwriting some code that hadn't been vcs committed yet with enough edits to fill up the undo stack.

2 Likes