1454764 - (rustfmt) [meta] rustfmt mozilla-central

Status: REOPENED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Bobby Holley (:bholley)]

There are huge advantages to automatic source formatting. This is a tall mountain to climb for C++, but Rust is easier - there's less of it in the tree, and developers have had less time to become attached to their personal style fiefdoms.

rustfmt isn't entirely stable, but it's pretty close, and is mostly getting work around edge cases. We could wait for it to be entirely stable, or we could just format the codebase now and then pick up any tweaks as they come.

Comment 1

[Bobby Holley (:bholley)]

This is what the change looks like:

https://github.com/bholley/gecko/commits/rustfmt_mc

NIing a few stakeholders for comment. I believe the decision is ultimately Ehsan's.

Comment 2

[Nathan Froyd [:froydnj]]

I mean, I would complain about some of the style choices, but I don't think rustfmt has a large number of knobs to tweak ala clang-format.  So sure.

Do you plan to enforce formatting at all with a lint?  I'd be hesitant to format the whole tree and then not have a mechanism in place to ensure it stays formatted.

CC'ing :ato for the geckodriver heads-up.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> I mean, I would complain about some of the style choices, but I don't think
> rustfmt has a large number of knobs to tweak ala clang-format.  So sure.
> 
> Do you plan to enforce formatting at all with a lint?  I'd be hesitant to
> format the whole tree and then not have a mechanism in place to ensure it
> stays formatted.

+1. There's the thing about what happens when rustfmt changes. (either because of rustfmt fixes or default configuration changes or what not).

My proposal would be to enforce the usage of rustfmt-format-diff[1] per-commit / per push.

It worked fine when I wrote it, but last time I tried it[2] looks like it found a rustfmt bug where it formats the first line of the file without needing to. Other than fixing that, it'd provide progressive formatting without massive changesets.

What do people think of something like that?

[1]: https://github.com/rust-lang-nursery/rustfmt/blob/master/src/format-diff/main.rs
[2]: https://github.com/servo/servo/pull/20617

Comment 4

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> I mean, I would complain about some of the style choices, but I don't think
> rustfmt has a large number of knobs to tweak ala clang-format.  So sure.

It does have some knobs: https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md

Emilio is also followup up with the rustfmt folks about various nits that we encountered while formatting the servo tree, so we should expect to see some improvements there.

> 
> Do you plan to enforce formatting at all with a lint?  I'd be hesitant to
> format the whole tree and then not have a mechanism in place to ensure it
> stays formatted.

Yes, was just in the middle of filing a followup for that.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> My proposal would be to enforce the usage of rustfmt-format-diff[1]
> per-commit / per push.
> 
> It worked fine when I wrote it, but last time I tried it[2] looks like it
> found a rustfmt bug where it formats the first line of the file without
> needing to. Other than fixing that, it'd provide progressive formatting
> without massive changesets.
> 
> What do people think of something like that?

Per our discussion on the servo side, I'm opposed to incremental formatting in this case. I think consistency is important for people to infer the correct style, and I think it also sucks for things like rename patches to suddenly become responsible for fixing up surrounding style.

I think the biggest bulk of Rust code we have is the servo style system code, which has already been rustfmted.

Not my call though, so I'll defer to Ehsan/Nathan.

Comment 6

[Nick Cameron [:nrc]]

> It does have some knobs: https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md

It would be great to avoid using them though - there's a benefit to the code looking like the rest of the Rust community's code, and the options are likely to be the last things to be stable and thus code using them will suffer from more formatting changes over time.

We're planning to stabilise Rustfmt (release a 1.0) this quarter, which should include some mechanisms to work much better on CI, so it might be worth holding out on implementing the linting/CI stuff until we have better support in Rustfmt.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > I mean, I would complain about some of the style choices, but I don't think
> > rustfmt has a large number of knobs to tweak ala clang-format.  So sure.

Also, now is a great time to complain about any of the choices, since rustfmt isn't stable yet, and any obviously-silly things can be more easily fixed now than later. So please bring them up with nrc & company.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > I mean, I would complain about some of the style choices, but I don't think
> > rustfmt has a large number of knobs to tweak ala clang-format.  So sure.
> 
> It does have some knobs:
> https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md

Whoa, I guess I was mistaken about having a One True Way to format Rust code!

Having just skimmed through, I think the "visual" conventions are more consistent with our C++ coding style in many cases.  My impression was that the One True Style was more the "block" conventions?

Do we know offhand if editors just DTRT for certain rustfmt settings?  (Obviously, you can run rustfmt on your code, but it's nice if the editor just gets it right for the most part.)

> > Do you plan to enforce formatting at all with a lint?  I'd be hesitant to
> > format the whole tree and then not have a mechanism in place to ensure it
> > stays formatted.
> 
> Yes, was just in the middle of filing a followup for that.

\o/

As far as incremental vs. all-at-once, I think we should do all-at-once, so people don't have to remember "do I run rustfmt on this or not?" and so everything looks consistent, rather than some older, less-touched code only getting incrementally formatted as people touch it.  Maybe this consideration would change if we formatted the C++ code, but we probably have a (comparatively) small enough amount of Rust that doing it all now makes more sense.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > (In reply to Nathan Froyd [:froydnj] from comment #2)
> > > I mean, I would complain about some of the style choices, but I don't think
> > > rustfmt has a large number of knobs to tweak ala clang-format.  So sure.
> > 
> > It does have some knobs:
> > https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md
> 
> Whoa, I guess I was mistaken about having a One True Way to format Rust code!
> 
> Having just skimmed through, I think the "visual" conventions are more
> consistent with our C++ coding style in many cases.  My impression was that
> the One True Style was more the "block" conventions?

Having worked with both, I've become a convert to block indentation. It makes decisions so much more straightforward, and avoids the need to reindent a zillion arguments when you change the name of a method.

It's also very much the dominant Rust idiom, plus the style that we've already bulk-formatted the servo code to. I don't think we should go against the grain on this one.

> As far as incremental vs. all-at-once, I think we should do all-at-once, so
> people don't have to remember "do I run rustfmt on this or not?" and so
> everything looks consistent, rather than some older, less-touched code only
> getting incrementally formatted as people touch it.  Maybe this
> consideration would change if we formatted the C++ code, but we probably
> have a (comparatively) small enough amount of Rust that doing it all now
> makes more sense.

+1

Comment 10

[Mike Hommey [:glandium]]

The main drawback is that rustfmt likes to change how it formats stuff quite frequently still, and you can end up in formatting wars if people change the same files and apply cargo fmt with different versions. Another is that in a few occasions, it has actually busted some of my source code (and that's not old anecdotal reference, it actually happened to me last week or so, where it simply removed #[cfg]'d code, and interestingly, I wasn't able to reproduce afterwards).

Comment 11

[Henri Sivonen (:hsivonen)]

I think that in principle we should do this, but I share glandium's concern:

(In reply to Mike Hommey [:glandium] from comment #10)
> The main drawback is that rustfmt likes to change how it formats stuff quite
> frequently still, and you can end up in formatting wars if people change the
> same files and apply cargo fmt with different versions.

My main concern is that rustfmt is supposed to come from rustup, but that method of obtaining it has been consistenly broken for me. I try to follow the instructions rustup gives me. Then I get:
error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not have the binary `cargo-fmt`

Then I go back to installing it manually.

Everyone installing rustfmt manually gives everyone a different snapshot. Even if the rustup method was fixed, there'd still be the problem of rustfmt changing frequently and different people having a different version.

Maybe mach should manage its own copy of rustfmt like it does with clang-tidy to have a consistent rustfmt-for-m-c at a given point in time.

> Another is that in a
> few occasions, it has actually busted some of my source code (and that's not
> old anecdotal reference, it actually happened to me last week or so, where
> it simply removed #[cfg]'d code, and interestingly, I wasn't able to
> reproduce afterwards).

rustfmt breaking my code has happened to me, too, but, in fairness, it was 11 months ago:
https://github.com/hsivonen/encoding_rs/blob/master/fuzz/fuzzers/fuzz_encodings.rs#L537

On the topic of incremental vs. all at the same time, I think we should do all (non-vendored) crates at the same time.

Comment 12

[u480271]

As glandium said, I found rustfmt to change it's mind with each release. I did add custom settings to the crates I was working on, but in the end removed them at the insistence of nrc.  I happy to use what ever settings are decided by consensus. I'm not interested in bike-shedding code formatting. 

> My main concern is that rustfmt is supposed to come from rustup, but that method of obtaining it has been consistenly broken 
> for me. I try to follow the instructions rustup gives me. Then I get:
> error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not have the binary `cargo-fmt`

I had this error when I installed rustfmt via cargo and then it moved to being managed by rustup.  I can't remember exactly how I fixed it, but it might have involved `cargo uninstall rustfmt` and `rustup component add rustfmt-preview`. Maybe nrc can shed more light here?

Comment 13

[Marion Daly [:mdaly]]

This belongs under lint and formatting no?

Comment 14

[Andrew Halberstadt [:ahal]]

This looks like bug 1369792, but since there are more discussions and CC's here, I'll dupe that to this.

There is a WIP patch in the other bug that is probably horribly bit-rotted by now. But it could be a good starting point. However that patch tries to set rustfmt up more as a linter than a formatter, which may not be what we want (though |mach lint| has a --fix argument that could be used to do formatting too).

Comment 16

[Andrew Halberstadt [:ahal]]

Sorry, not sure how I managed to close this.

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to Marion Daly [:mdaly] from comment #13)
> This belongs under lint and formatting no?

I don't think so, it's not a build system issue, but we don't really have a good component for general Rust stuff in Firefox. Mostly I just want to be clear that nobody working on the build system is going to do this work.

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> (In reply to Marion Daly [:mdaly] from comment #13)
> > This belongs under lint and formatting no?
> 
> I don't think so, it's not a build system issue, but we don't really have a
> good component for general Rust stuff in Firefox. Mostly I just want to be
> clear that nobody working on the build system is going to do this work.

kmoir was going to create a Rust/C++ "stuff" component to provide a home for this sort of thing.

Comment 19

[Andrew Halberstadt [:ahal]]

The entire Lint and Formatting component was only stashed on Build System because there wasn't really any better place for it. As the triage-owner of this component, I think it belongs here (though if anyone feels strongly about moving it out, I don't really care that much).

Comment 20

[Bobby Holley (:bholley)]

(In reply to Nick Cameron [:nrc] from comment #6)
> We're planning to stabilise Rustfmt (release a 1.0) this quarter, which
> should include some mechanisms to work much better on CI, so it might be
> worth holding out on implementing the linting/CI stuff until we have better
> support in Rustfmt.

Seems like the consensus here is that we should do it, but wait for rustfmt to stabilize. Nick, how close are we?

Comment 21

[Bobby Holley (:bholley)]

(And I think Nathan's input is probably sufficient here unless Ehsan wants to jump in)

Comment 22

[Sylvestre Ledru [:Sylvestre]]

We should probably use the "./mach lint" infra, it is helping a lot with development and maintenance.
If we do that, we will also benefit almost at no cost from rustfmt at review phase (like the rest of the linters)

Comment 23

[Nick Cameron [:nrc]]

> how close are we?

Ah such hubris on my part to predict a release so soon. We've made progress but are sill waiting on an RFC about formatting to conclude and to implement a few formatting fixes. I expect a release candidate to be on nightly by August 2nd, for 1.0 to be on nightly by early September, and 1.0 to be on stable for the edition release on October 25th.

Comment 24

[Bobby Holley (:bholley)]

(In reply to Nick Cameron [:nrc] from comment #23)
> > how close are we?
> 
> Ah such hubris on my part to predict a release so soon. We've made progress
> but are sill waiting on an RFC about formatting to conclude and to implement
> a few formatting fixes. I expect a release candidate to be on nightly by
> August 2nd, for 1.0 to be on nightly by early September, and 1.0 to be on
> stable for the edition release on October 25th.

Thanks. We probably don't want to roll anything out here until rustfmt is stable, though I may attempt a dry run with the release candidate in August if I have time.

Comment 25

[Bobby Holley (:bholley)]

Attachment: Format mozilla-central with rustfmt 1.0 RC. v1

Here's the promised dry run with the release candidate for rustfmt. If anybody
has any gripes about the formatting, now is the time to raise them.

`cargo +nightly fmt --version` => rustfmt 0.99.2-nightly (5c9a2b6c 2018-08-07)

Command:

cargo +nightly fmt; pushd servo/components; for file in *; do pushd $file; cargo +nightly fmt; popd; done; popd;

This currently excludes geckolib because of [1]. It also excludes webrender because that's effectively vendored code (we should format upstream at some point down the line).

[1] https://github.com/rust-lang-nursery/rustfmt/issues/2936

MozReview-Commit-ID: CRdDMOGExMQ

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 9002232 [details] [diff] [review]
Format mozilla-central with rustfmt 1.0 RC. v1

Feedback from Nathan since I'll be requesting review of the final patch from him once rustfmt stabilizes (I don't plan to requesting review from individual module owners).

Feedback from Emilio since he's had opinions about what rustfmt does, and this is probably the last chance to file bugs and get behavior changed.

Comment 27

[Bobby Holley (:bholley)]

Also, per the command in comment 25, I had to manually traverse the servo directories to get cargo fmt to format them. It would be great if |cargo fmt| could provide an option to automatically traverse the dependency graph for all path dependencies. Is that feasible Nick?

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9002232 [details] [diff] [review]
Format mozilla-central with rustfmt 1.0 RC. v1

Review of attachment 9002232 [details] [diff] [review]:
-----------------------------------------------------------------

I'm generally ok with this, but we need to align with Servo, somehow, because I still sync code back and forth.

Servo has the following in its rustfmt.toml:

> match_block_trailing_comma = true
> binop_separator = "Back"
> # Turn off import reordering, since the new algorithm clashes with tidy.
> reorder_imports = false

I'm fine with import reordering, but we need to remove the equivalent tidy lint in Servo. Interestingly I posted https://groups.google.com/d/msg/mozilla.dev.servo/-06meTc7H1k/UqFr8rRNAAAJ this morning, so if people agree... :)

I think I prefer binop_separator = "Back" as well, since that's what most of us use and is closer to the Gecko style, but I don't mind much either way.

I couldn't care less about trailing comma vs. not. In any case, this diff is way more noisy than what it needs to because last time you reformatted Servo using a different style.

::: servo/components/style/values/generics/rect.rs
@@ +20,5 @@
> +    MallocSizeOf,
> +    PartialEq,
> +    SpecifiedValueInfo,
> +    ToComputedValue,
> +)]

This kind of thing is going to add so many lines to the style system... :)

Comment 29

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #28)
> Comment on attachment 9002232 [details] [diff] [review]
> Format mozilla-central with rustfmt 1.0 RC. v1
> 
> Review of attachment 9002232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm generally ok with this, but we need to align with Servo, somehow,
> because I still sync code back and forth.
> 
> Servo has the following in its rustfmt.toml:
> 
> > match_block_trailing_comma = true
> > binop_separator = "Back"
> > # Turn off import reordering, since the new algorithm clashes with tidy.
> > reorder_imports = false
> 
> I'm fine with import reordering, but we need to remove the equivalent tidy
> lint in Servo. Interestingly I posted
> https://groups.google.com/d/msg/mozilla.dev.servo/-06meTc7H1k/UqFr8rRNAAAJ
> this morning, so if people agree... :)

Yeah - commented on that thread.

> I think I prefer binop_separator = "Back" as well, since that's what most of
> us use and is closer to the Gecko style, but I don't mind much either way.

I'll defer to Nathan - see below.
 
> I couldn't care less about trailing comma vs. not. In any case, this diff is
> way more noisy than what it needs to because last time you reformatted Servo
> using a different style.

Yeah - at the time I wasn't thinking as hard about the advantages of using standard Rust style, so I just used Servo's config. Given that people are unlikely to have a strong opinion about trailing commas, I think we probably should accept the default on that one.

Nathan, thoughts on binop separator? Consistency with the rest of Rust is nice, though I think our existing code leans towards first line. The Mozilla style guide specifies first line for C++, and something more complicated for JS [1]. Google C++ leaves it unspecified [2].

Nick, is there a compelling reason for the default value of binop separator in rustfmt?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code
[2] https://google.github.io/styleguide/cppguide.html#Boolean_Expressions

Comment 30

[Bobby Holley (:bholley)]

Also NI jdm for thoughts, since we'd need to align servo and gecko's rustfmt configs if we want emilio's syncing job to stay easy.

My gut sense is that the |match_block_trailing_comma| doesn't matter much and we should just use the rustfmt default, absent any concrete reasons to do otherwise. Less sure about binop_separator.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) from comment #30)
> Also NI jdm for thoughts, since we'd need to align servo and gecko's rustfmt
> configs if we want emilio's syncing job to stay easy.
> 
> My gut sense is that the |match_block_trailing_comma| doesn't matter much
> and we should just use the rustfmt default, absent any concrete reasons to
> do otherwise. Less sure about binop_separator.

Forgot to ni? I assume :)

Comment 32

[Dan Minor [:dminor]]

Please also exclude rsdparsa (media/webrtc/signaling/src/sdp/rsdparsa) for now. It is being developed on github [1] and we have upstream commits that have not been merged into mozilla-central yet. We'd be happy to run rustfmt on upstream before we merge again. Thanks!

[1] https://github.com/nils-ohlmeier/rsdparsa

Comment 33

[Josh Matthews [:jdm]]

We've started merging PRs to format Servo's components directory by directory as part of a plan to move to enforcing rustfmt style on all of our code. I'm fine with aligning with Gecko's style choices, but it would be nice if we could wait on reordering imports until Servo's ready to flip the switch (per https://groups.google.com/d/msg/mozilla.dev.servo/-06meTc7H1k/ia9CXcLSAAAJ).

Comment 34

[Nick Cameron [:nrc]]

> It would be great if |cargo fmt| could provide an option to automatically traverse the dependency graph for all path dependencies. Is that feasible Nick?

Hmm, not sure. For normal Rust usage, you wouldn't want to format deps since these would be read-only downloaded from crates.io. It does make sense to format a whole workspace, so if Servo is a single Cargo workspace, we should implement that. Otherwise, it probably isn't too hard to write a small program to discover deps and format them.

> is there a compelling reason for the default value of binop separator in rustfmt?

It's discussed on this GH thread. tl;dr it makes code easier to scan because running the eye down the lhs is easier than running it down the rhs.

Note that the Rustfmt 1.0 stability guarantees only apply to the default options. However, in practice I expect that small tweaks like the binop position are likely to be stable. If you end up with any major departures we should work to try and ensure stability of those options in rustfmt.

Comment 35

[Bobby Holley (:bholley)]

(In reply to Josh Matthews [:jdm] from comment #33)
> We've started merging PRs to format Servo's components directory by
> directory as part of a plan to move to enforcing rustfmt style on all of our
> code. I'm fine with aligning with Gecko's style choices,

Ok great, sounds like it's up to Nathan then. Looks like I forgot to NI him in comment 29.

> but it would be
> nice if we could wait on reordering imports until Servo's ready to flip the
> switch (per
> https://groups.google.com/d/msg/mozilla.dev.servo/-06meTc7H1k/ia9CXcLSAAAJ).

That seems fine. I think we wouldn't enable rustfmt until 1.0 hit stable Rust, which will be some months. So Servo will probably beat Gecko to that milestone, assuming efforts there don't stall.

Comment 36

[Nathan Froyd [:froydnj]]

Comment on attachment 9002232 [details] [diff] [review]
Format mozilla-central with rustfmt 1.0 RC. v1

Review of attachment 9002232 [details] [diff] [review]:
-----------------------------------------------------------------

This was with the default rustfmt settings, correct?

Maybe it's because I haven't used it enough, but just reading through the diff, I'm finding it kind of hard to formulate a mental heuristic of what rustfmt will do.  I've noted a couple of examples below.  I have not looked at the entire diff.

I realize that the whole point of automatic formatting is that you don't really have to think about it, but it seems valuable to have people have a pretty good idea of what might get changed about their code.  Maybe there are rules that people internalize and I'm just not there yet.  The fairly-consistent expansion of things into multiple lines seems unfortunate.

I have a slight preference for match_block_trailing_comma = true for consistency with...well, everything else that's a list of stuff, really.

binop_separator should /probably/ be "Back" for consistency with Gecko?  I don't think that's great for consistency with the wider ecosystem, but I think it eases some of the mental burden of switching between C++ and Rust.  (Though JS uses "Front", apparently, so maybe consistency can just get thrown out the window.)

::: intl/encoding_glue/src/lib.rs
@@ -529,5 @@
>                  return (NS_OK, output_encoding);
>              }
>              CoderResult::OutputFull => {
> -                if let Some(needed) =
> -                    checked_add(total_written,

This version seems slightly easier to scan than the rustfmt version.

::: js/rust/build.rs
@@ +41,5 @@
>  
>      for entry in entries {
>          if let Ok(path) = entry {
> +            return path
> +                .canonicalize()

It seems bizarre to me that rustfmt expands this line, but contracts the bits above.  What is going on here?  This one is expanded because of the following .expect(), and chained function calls in a single expression call for expanding them?

::: servo/components/selectors/parser.rs
@@ +2373,5 @@
> +            Ok(SelectorList::from_vec(vec![Selector::from_vec(
> +                vec![Component::LocalName(LocalName {
> +                    name: DummyAtom::from("EeÉ"),
> +                    lower_name: DummyAtom::from("eeÉ"),
> +                }),],

This trailing comma after the `vec!` element smooshed together with everything else seems suboptimal.  This is much more compressed than I would expect from rustfmt.

@@ +2388,5 @@
> +                        lower_name: DummyAtom::from("e"),
> +                    }),
> +                ],
> +                specificity(0, 0, 1),
> +            ),]))

This whole block right here is virtually identical to the above (one extra element in the innermost `vec!`, I guess), but the formatting is much more pleasing.  Still with the smooshed comma at the end, though.

It seems not good that relatively minor changes lead to large differences in formatting.

@@ +2425,5 @@
>          );
>          assert_eq!(
>              parse("*"),
> +            Ok(SelectorList::from_vec(vec![Selector::from_vec(
> +                vec![Component::ExplicitUniversalType],

Why doesn't this single-element vector have a trailing comma, as the example just a couple lines above, or the `Selector::from_vec()` element in the outermost `vec!` here?

::: servo/components/style/animation.rs
@@ +507,5 @@
>                  Some(previous_style),
>                  Some(previous_style),
>                  font_metrics_provider,
> +                CascadeMode::Unvisited {
> +                    visited_rules: None,

I don't get why this needs to be expanded.

::: servo/components/style/counter_style/mod.rs
@@ +496,5 @@
>                          (opt_start, opt_end)
>                      {
>                          if start > end {
> +                            return Err(
> +                                input.new_custom_error(StyleParseErrorKind::UnspecifiedError)

What rule splits this up into multiple lines?

::: servo/components/style/gecko/media_features.rs
@@ +41,5 @@
>  fn device_size(device: &Device) -> Size2D<Au> {
>      let mut width = 0;
>      let mut height = 0;
>      unsafe {
> +        bindings::Gecko_MediaFeatures_GetDeviceSize(device.document(), &mut width, &mut height);

I feel like there are multiple examples in this diff where rustfmt makes the exact opposite transformation of that being done here: it takes a call that fits just fine on a single line--shorter even than this one!--and expanding it to multiple lines.

::: servo/components/style/gecko/wrapper.rs
@@ +1244,5 @@
>      }
>  
>      fn owner_doc_matches_for_testing(&self, device: &Device) -> bool {
> +        self.as_node().owner_doc().0 as *const structs::nsIDocument == device
> +            .pres_context()

This formatting of `device` on the previous line really hides what's going on here, IMHO.

@@ +1869,5 @@
>                  ));
>              }
>  
> +            let active = self
> +                .state()

I don't get the rules for chained function call expressions.  Sometimes they start on different lines (here), sometimes the second call goes on a different line (below, with the debug_assert!), sometimes they all stay on the same line (above, owner_doc_matches_for_testing).

@@ +2093,5 @@
>  
> +        debug_assert!(
> +            self.as_node()
> +                .parent_node()
> +                .map_or(false, |p| p.is_document())

This seems like reasonable chaining formatting...although the line was already short enough as-is.

@@ +2100,5 @@
>      }
>  
>      fn is_empty(&self) -> bool {
> +        !self
> +            .as_node()

This change seems weird, given the debug_assert! formatting above.  Why the difference?

::: servo/components/style/media_queries/media_list.rs
@@ +29,5 @@
>      /// found, the media query list is only filled with the equivalent of
>      /// "not all", see:
>      ///
>      /// <https://drafts.csswg.org/mediaqueries/#error-handling>
> +    pub fn parse(context: &ParserContext, input: &mut Parser) -> Self {

See the below comment in media_query.rs.  Why does rustfmt compress this onto a single line, but expand the below into multiple lines?

::: servo/components/style/media_queries/media_query.rs
@@ +143,5 @@
>          let media_type = explicit_media_type.unwrap_or(MediaQueryType::All);
> +        Ok(Self {
> +            qualifier,
> +            media_type,
> +            condition,

What is the bit that controls formatting this?  `use_small_heuristics`?  This change seems like unnecessary expansion of code.

::: testing/mozbase/rust/mozprofile/src/prefreader.rs
@@ +28,5 @@
>      fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +        write!(
> +            f,
> +            "{} at line {}, column {}",
> +            self.message, self.position.line, self.position.column

So this call gets split into multiple lines, but some arguments are on the same line, which seems kind of abnormal after looking at other similar calls.  Is this because rustfmt knows something about the semantics of `write!`?

@@ +180,5 @@
>          let data = match str::from_utf8(&buf[self.start_pos..end_pos]) {
>              Ok(x) => x,
>              Err(_) => {
> +                return Err(PrefReaderError::new(
> +                    "Could not convert string to utf8",

This sort of splitting over multiple lines seems unnecessary.  Is it because of the nested function calls, multiple arguments, or some combination of the two?

::: testing/webdriver/src/common.rs
@@ +46,5 @@
>      }
>  
>      pub fn map<F, U: ToJson>(self, f: F) -> Nullable<U>
> +    where
> +        F: FnOnce(T) -> U,

It seems a little straight that having function declarations with multiple arguments can fit on a single line, but `where` predicates are forced to be multi-line like this.

::: testing/webdriver/src/httpapi.rs
@@ +23,5 @@
> +        (Get, "/session/{sessionId}/window", Route::GetWindowHandle),
> +        (
> +            Get,
> +            "/session/{sessionId}/window/handles",
> +            Route::GetWindowHandles,

Why do this?  This line is already well within `max_width`.

@@ +238,5 @@
>      CloseWindow,
> +    GetWindowSize,     // deprecated
> +    SetWindowSize,     // deprecated
> +    GetWindowPosition, // deprecated
> +    SetWindowPosition, // deprecated

This feature is nice, but it seems contrary to our ignorning alignment in other contexts.  I know Emacs has bindings for automatically aligning trailing comments like this (though I think its defaults are much further right), but I don't know about our other editors.

::: testing/webdriver/src/response.rs
@@ +56,5 @@
>  impl CloseWindowResponse {
>      pub fn new(handles: Vec<String>) -> CloseWindowResponse {
> +        CloseWindowResponse {
> +            window_handles: handles,
> +        }

OK, why does this get expanded...

@@ +110,5 @@
>  }
>  
>  impl ValueResponse {
>      pub fn new(value: json::Json) -> ValueResponse {
> +        ValueResponse { value: value }

But this gets contracted?  Aren't they basically examples of the exact same thing, number of tokens and everything?

::: xpcom/rust/xpcom/src/base.rs
@@ +33,5 @@
>          unsafe {
> +            if (*(self as *const Self as *const nsISupports))
> +                .QueryInterface(&T::IID, ga.void_ptr())
> +                .succeeded()
> +            {

I don't get where this rule that the brace gets put on its own line comes from.  AFAICT from `control_brace_style`, this shouldn't happen?

Comment 37

[Nathan Froyd [:froydnj]]

Commented in comment 36.

Comment 38

[Bobby Holley (:bholley)]

This is great feedback. Passing it on to Nick.

Comment 39

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #36)
> This was with the default rustfmt settings, correct?

Oh, and yes.

Comment 40

[Nick Cameron [:nrc]]

A couple of general points: commas can have semantic significance in macros, so we never add or remove them. If you want a trailing comma you need to add it manually and then Rustfmt will preserve it.

A lot of the questions are about why an expression gets put on multiple lines when it doesn't need to - we found that some expression were *much* more readable when put on separate lines. We also found that people were doing this in practice without Rustfmt (usually based on complexity rather than absolute width, but that is hard to model). We use the number of characters as the heuristic for this - it's not perfect, but it comes out pretty well in practice.

The same applies to chains of method calls/field accesses. One oddity with these is that if we go multi-line, then we usually put a newline before the first `.`. However, if that leaves the first line shorter than the indent of the second line it looks weird, so we put it all on one line.

Hopefully I'll answer the rest below and not miss any

> It seems bizarre to me that rustfmt expands this line, but contracts the bits above.  What is going on here?

There are two different rules acting here: if we can't put a chain on one line, then we split it one item per line (we nearly always do something like this, we avoid multiple lines and multiple items per line). When we get a `let ... = ...` statement (or any other assignment), we try putting the rhs on its own line, if that doesn't work then we keep the start of the rhs on the same line as the lhs (and go multiline from there). In this case the first chain fits just below the width heuristic, so we can split the assignment and keep the chain on one line. For the second chain, we're just above the width heuristic or maybe exceeding the max width.

> This whole block right here is virtually identical to the above (one extra element in the innermost `vec!`, I guess)

We have a 'combining rule' where there is only a single argument in a function call/macro (and some other expressions) we'll combine the first lines. This prevents things like:

```
foo(
    foo_bar(
        baz_baz(
            Foo {
                x: String,
            }
        )
    )
)

// Rustfmt gives:
foo(foo_bar(baz_baz(Foo {
    x: String,
})))
```

This can go a bit overboard sometimes, but in most cases its an improvement.

> This formatting of `device` on the previous line really hides what's going on here, IMHO.

Agreed: https://github.com/rust-lang-nursery/rustfmt/issues/2967 Although I'm not sure if we'll be able to come up with a fix (binops are suprisingly complicated because the AST does not match the intuition when you have say `a + b + c + d`).

> Why does rustfmt compress this onto a single line, but expand the below into multiple lines?

Function declarations are formatted differently to expressions, the latter usually has a width heuristic as well as the max width.

> What is the bit that controls formatting this?  `use_small_heuristics`?  This change seems like unnecessary expansion of code.

Yes. It's not perfect, but things look a lot worse without it - edge cases still go wrong, but it means the edge cases aren't as extreme and thus things look better.

> Is this because rustfmt knows something about the semantics of `write!`?

Sort of - it knows that a format string followed by some expressions is a common pattern and formats that specially.

> but `where` predicates are forced to be multi-line like this.

`where` clauses were a pain to get right. In the end we went for always putting each component on it's own line because `where` clauses should only be used with complicated constraints (if they are simple you can put them on the declaration of generic args), and when they are complex, then it really aids reading to have one per line. We even do this with just one constraint, because otherwise the indentation is bad or things look inconsistent.

> this feature is nice, but it seems contrary to our ignorning alignment in other contexts.

it's kind of an odd rule, it doesn't fit very well with everything else, but always using n spaces looks horrible. In general I would recommend putting comments above each variant, rather than to the right.

> OK, why does this get expanded...

The heuristic width for struct literals is very low. This kind of case is sub-optimal, but in general spread out struct literals are a big win.

> I don't get where this rule that the brace gets put on its own line comes from

So, for some item `blah blah { ... }` (control flow, declarations, whatever) we nearly always leave the `{` trailing. However, if `blah blah` goes multiline, we put `{` unindented on it's own line. The reason is that the second and subsequent lines of `blah blah` will be indented and so is the body (`...`). If we trail `{` then there is no indentation difference so it is hard to scan since the header expression blends in with the body.

> This trailing comma after the `vec!` element smooshed together with everything else seems suboptimal.

This is fixed on master, I get:

```
    Ok(SelectorList::from_vec(vec![Selector::from_vec(
        vec![Component::LocalName(LocalName {
            name: DummyAtom::from("e"),
            lower_name: DummyAtom::from("e"),
        })],
        specificity(0, 0, 1),
    )]))
```


I hope that covers everything. Most of the discussion on the formatting rules is here: https://github.com/rust-lang-nursery/fmt-rfcs/issues?utf8=%E2%9C%93&q=is%3Aissue

Comment 41

[Nathan Froyd [:froydnj]]

Thanks for the explanations!

(In reply to Nick Cameron [:nrc] from comment #40)
> A lot of the questions are about why an expression gets put on multiple
> lines when it doesn't need to - we found that some expression were *much*
> more readable when put on separate lines. We also found that people were
> doing this in practice without Rustfmt (usually based on complexity rather
> than absolute width, but that is hard to model). We use the number of
> characters as the heuristic for this - it's not perfect, but it comes out
> pretty well in practice.

Do you have a pointer to discussion for this?  Number of characters seems right, but I'd expect some sort of "complexity" metric to kick in first.  I expect there are good reasons for this.

> > This whole block right here is virtually identical to the above (one extra element in the innermost `vec!`, I guess)
> 
> We have a 'combining rule' where there is only a single argument in a
> function call/macro (and some other expressions) we'll combine the first
> lines. This prevents things like:
> 
> ```
> foo(
>     foo_bar(
>         baz_baz(
>             Foo {
>                 x: String,
>             }
>         )
>     )
> )
> 
> // Rustfmt gives:
> foo(foo_bar(baz_baz(Foo {
>     x: String,
> })))
> ```
> 
> This can go a bit overboard sometimes, but in most cases its an improvement.

OK, thanks for the explanation.  This one seems a bit unfortunate that `x:` lines up with what could be `foo`'s arguments, but oh well.

> > OK, why does this get expanded...
> 
> The heuristic width for struct literals is very low. This kind of case is
> sub-optimal, but in general spread out struct literals are a big win.

This seems like an unfortunate heuristic for this case.  Do you have a pointer to some discussion for this?  I found:

https://github.com/rust-lang-nursery/fmt-rfcs/issues/64 (struct literals, which seems to suggest that things should be placed on one line if possible)
https://github.com/rust-lang-nursery/fmt-rfcs/issues/47 (which I think agrees with not expanding things so eagerly, but it's hard to tell)

but either I skimmed the discussions too fast, or there is some other discussion I couldn't find, since rustfmt doesn't agree with my understanding of those issues.  (Or I have completely misunderstood the discussions!)

> > This trailing comma after the `vec!` element smooshed together with everything else seems suboptimal.
> 
> This is fixed on master, I get:
> 
> ```
>     Ok(SelectorList::from_vec(vec![Selector::from_vec(
>         vec![Component::LocalName(LocalName {
>             name: DummyAtom::from("e"),
>             lower_name: DummyAtom::from("e"),
>         })],
>         specificity(0, 0, 1),
>     )]))
> ```

Cool, that looks about right.  Did the single-element case for the inner vec! get fixed as well as a byproduct?

Did you have a comment on this particular one?  I find it a little hard to understand why rustfmt put all this on one line, while other relatively similar examples got expanded:

::: servo/components/style/gecko/media_features.rs
@@ +41,5 @@
>  fn device_size(device: &Device) -> Size2D<Au> {
>      let mut width = 0;
>      let mut height = 0;
>      unsafe {
> +        bindings::Gecko_MediaFeatures_GetDeviceSize(device.document(), &mut width, &mut height);

Comment 42

[Nick Cameron [:nrc]]

> Do you have a pointer to discussion for this?  Number of characters seems right, but I'd expect some sort of "complexity" metric to kick in first.  I expect there are good reasons for this.

Discussion was here - https://github.com/rust-lang-nursery/fmt-rfcs/issues/47 - and a lot more happened in meetings. We did prefer a complexity metric rather than a character count. However, our attempts to specify an algorithm got complex really fast. In the end we left it as an implementation detail for the tool to decide. For Rustfmt, we've stuck with char count because it is simple. We would like to implement a complexity metric, but not for 1.0 (hopefully for 2.0) - implementation is probably not too hard, but from experience it takes a lot of tweaking to get satisfactory parameters.

> Do you have a pointer to some discussion for this?

I think #64 would be where it was discussed, but it may have happened in video meetings. We found that in general people used multiline structs, so that should be the default, and that putting it on line should be exceptional, so the heuristic for that is pretty strict. This has been a bit controversial, people who think of struct literals like function calls (i.e., single-line by default) tend to find it odd, whereas people who think of struct literals like struct declarations with values find it more natural.

> Did the single-element case for the inner vec! get fixed as well as a byproduct?

I think this is the single-element case? I might be reading it wrong.

> Did you have a comment on this particular one?  I find it a little hard to understand why rustfmt put all this on one line, while other relatively similar examples got expanded:

The whole line is under the max width and the heuristic we check applies only to he arguments, `device.document(), &mut width, &mut height` is fairly short.

Comment 43

[Emilio Cobos Álvarez (:emilio)]

FWIW the Servo rustfmt happened and is in mozilla-central now. In bug 1369792 I want to figure out a way to use use the same rustfmt version between Servo and Gecko so that I can keep keeping them in sync, and add a lint to the servo/ directory.

Once that's done we can probably proceed here without much problem.

Comment 44

[Sylvestre Ledru [:Sylvestre]]

Started some of the work in bug 1617369

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Let's morph this bug into a meta bug