Try Rustfmt on your code


#1

I would like to encourage you to try rustfmt on your projects. It is a tool for formatting your code according to some style guidelines. The guidelines it uses are pretty configurable if you don’t like the defaults. You can easily install it with

cargo install --git https://github.com/rust-lang-nursery/rustfmt

you should then be able to run rustfmt filename where filename is the root of a crate (it’ll actually work with any file in the crate too). This will reformat your whole crate (rustfmt walks sub-module files) and create backups. You can instead print the output to the terminal or some other choices, by using --write-mode. For more details, see the readme at https://github.com/rust-lang-nursery/rustfmt.

Rustfmt works pretty well on most projects, if you find problems, please file issues on the repo. If you need help running rustfmt or have any questions about, please ask here or you can ping me (nrc) on irc.


This Week in Rust editors' thread
#2

Rather than brain-dump into the thread, I brain-dumped my reaction in a Gist. I began by trying to track down issues for what I was seeing… but eventually gave up when I ran into more than I have time to track down.

TLDR: there’s still far too many things that rustfmt does that I either really don’t like, or are just objectively wrong, to consider using on my code. Nevertheless, it’s impressive how far it’s come to date.


#3

My main problem is aggressive rightward drift and then the subsequent line shortening:

+    let numbers_map: HashMap<String, u16> = try!((0..numbers_count)⏎
+                                                     .filter_map(|i| {⏎
+                                                         match read_le_u16(file) {⏎
+                                                             Ok(0xFFFF) => None,⏎
+                                                             Ok(n) =>⏎
+                                                                 Some(Ok((nnames[i].to_string(),⏎
+                                                                          n))),⏎
+                                                             Err(e) => Some(Err(e)),⏎
+                                                         }⏎
+                                                     })⏎
+                                                     .collect());⏎
-    let numbers_map: HashMap<String, u16> = try!(⏎
-        (0..numbers_count).filter_map(|i| match read_le_u16(file) {⏎
-            Ok(0xFFFF) => None,⏎
-            Ok(n) => Some(Ok((nnames[i].to_string(), n))),⏎
-            Err(e) => Some(Err(e))⏎
-        }).collect());⏎

This can be fixed by replacing try!() with try! {} which arguably makes more sense but still, it would be nice if rustfmt could avoid rightward drift when formatting.


#4

Also, this:

+        let string_offsets: Vec<u16> = try!((0..string_offsets_count)⏎
+                                                .map(|_| read_le_u16(file))⏎
+                                                .collect());⏎
-        let string_offsets: Vec<u16> = try!((0..string_offsets_count).map(|_| {⏎
-            read_le_u16(file)⏎
-        }).collect());⏎

I’d much rather start new lines inside a closure.


#5

I agree with @stebalien on this one:

Block comments are being turned into line comments.

Also, I do not like the “put space everywhere” strategy in small struct litterals:

+    fuzz(Foo { foo: 56 });⏎
-    fuzz(Foo{foo: 56});⏎

I use { as constructor parentheses here, and I do not like having too much whitespace.


#6

While I think running rustfmt on your code is more about testing it than on the exact choices it makes, line comments are considered idiomatic, broadly speaking.


#7

You mean @DanielKeep.


#8

Just to be clear, I’m operating under the assumption that this is about trying out rustfmt and seeing if there are things that will prevent it being used ubiquitously. Bothersome defaults can always be changed, and this should definitely not be about pushing for changes to the “standard” style. Now, that said…

Only by crazy people. Line comments are a huge pain for anything that isn’t a single line of comment. Which is why they exist. Using single line comments for more than one line is like building a house out of matchsticks when you have ready access to 2x4 and logs and steel and other, better-suited materials because you once saw a scale model of the Sistine Chapel made out of matchsticks and thought it was neat.

So it’s good thing this isn’t about the defaults or forcing everyone to use patently ridiculous style choices. Otherwise it’d degenerate into a huge fight really fast. :stuck_out_tongue:


#9

Some meta things:

  • If rustfmt should remain a standalone command, a --version switch would be nice.

  • I’d like some feedback from the program what it is doing, e.g.,

    $ rustfmt src/lib.rs
    Using configuration file "rustfmt.toml"
    formatting src/lib.rs (pre-formatted file remains src/lib.rs.bk)
    formatting src/foo.rs (pre-formatted file remains src/foo.rs.bk)

  • maybe also add cargo-awareness. I mean with this, if rustfmt is called in a cargo project without parameters, it should use the entry point (e.g. src/lib.rs or src/main.rs) for that project.


#10

I’ve decided to try it on one of my projects. Personally, I like how it worked very much but there are few things which I couldn’t find out how to tweak and which make it unsuitable for me yet (everything below except “minor points”).

First, as was already mentioned, is the rightward drift, for example:

-            try!(write!(
-                target, " {}=\"{}\"",
-                attr.name.repr_display(),
-                if self.config.perform_escaping { escape_str_attribute(attr.value) }
-                else { Cow::Borrowed(attr.value) }
-            ))
+            try!(write!(target,
+                        " {}=\"{}\"",
+                        attr.name.repr_display(),
+                        if self.config.perform_escaping {
+                            escape_str_attribute(attr.value)
+                        } else {
+                            Cow::Borrowed(attr.value)
+                        }))

and

-        let attr = Attribute::new(
-            Name::qualified("attribute", "urn:namespace", Some("n")),
-            "its value with > & \" ' < weird symbols"
-        );
-
-        assert_eq!(
-            &*attr.to_string(),
-            "{urn:namespace}n:attribute=\"its value with &gt; &amp; &quot; &apos; &lt; weird symbols\""
-        )
+        let attr = Attribute::new(Name::qualified("attribute", "urn:namespace", Some("n")),
+                                  "its value with > & \" ' < weird symbols");
+
+        assert_eq!(&*attr.to_string(),
+                   "{urn:namespace}n:attribute=\"its value with &gt; &amp; &quot; &apos; &lt; \
+                    weird symbols\"")

I prefer having long function calls to be indented like blocks, and indeed there is an option for this (fn_args_layout), however it also affects function declarations which begin to be always formatted as blocks, which I don’t want - I like Visual layout for function signatures.

Second point I don’t like is that rustfmt inserts spaces in associated types inside constraints:

-    fn extend<I: IntoIterator<Item=Value>>(&mut self, it: I) {
+    fn extend<I: IntoIterator<Item = Value>>(&mut self, it: I) {

I dislike this very much because without spaces it is visually clear that e.g. IntoIterator<Item=Value> is a single constraint; much less so with them. This behavior doesn’t seem to be tweakable at all.

There is also a bug, as far as I can see: doc comments on fields of enum struct variants is off:

     StartDocument {
-        /// XML version.
-        ///
-        /// If XML declaration is not present, defaults to `Version10`.
+    /// XML version.
+    ///
+    /// If XML declaration is not present, defaults to `Version10`.
         version: XmlVersion,

I probably should report it, if it isn’t already.

I was also under impression that chain_base_indent defines how chained method calls are indented - I prefer them to be indented like blocks, on the next line:

some_long.expression.method()
    .method2()
    .method3()

However, this option doesn’t seem to affect chained calls:

-                    let attributes: Vec<String> = attributes.iter().map(
-                        |a| format!("{} -> {}", a.name, a.value)
-                    ).collect();
+                    let attributes: Vec<String> = attributes.iter()
+                                                            .map(|a| format!("{} -> {}", a.name, a.value))
+                                                            .collect();

It would be nice if the following situations inside a match were also tweakable but this is a minor point:

-                        "xml"|"xmL"|"xMl"|"xML"|"Xml"|"XmL"|"XMl"|"XML"
-                            if self.encountered_element || self.parsed_declaration =>
+                        "xml" | "xmL" | "xMl" | "xML" | "Xml" | "XmL" | "XMl" | "XML" if self.encountered_element ||
+                                                                                         self.parsed_declaration =>

I mean the positioning of pattern guard clause - I prefer the original one.

This:

-            if self.encountered_element && self.st == State::OutsideTag {  // all is ok
+            if self.encountered_element && self.st == State::OutsideTag {
+                // all is ok

was really unexpected. I’d expect comments to be kept in place.

I think it would be great if there was the following logic for formatting chained calls: if the part before the last dot in the first line is short, like here:

something.emitter
         .namespace_stack_mut()  
         .push_empty()
         .checked_target()
         .extend(namespace.as_ref())

then chained calls should be aligned by this dot. Otherwise, they should be indented as blocks:

some_function(a.long_something.whatever)
    .chained_method()
    .another_one()

Another minor point is to allow keeping vertical structure in multiple sequential definitions or pattern match arms. For example:

             ClosingSubstate::First => match c {
                 '-' => self.move_to(State::CommentClosing(ClosingSubstate::Second)),
-                _   => self.move_to_with_unread(State::Normal, &[c], Token::Character('-'))
+                _ => self.move_to_with_unread(State::Normal, &[c], Token::Character('-')),
             },

here I’d prefer => to be aligned.

Anyway, most of this is probably only my preferences, so certainly I don’t expect them to be fixed soon :slight_smile: And regardless of that, I think that rustfmt is indeed a very powerful tool, and it works really great.


#11
  • I like having closing braces and else / else if statements on different lines. I had the impression that the single_line_if_else config option could solve that but it doesn’t. I’m not sure if I misunderstand what it does or if it doesn’t work. Setting it to false, I still get:
-            }
-            else if ... {
+            } else if .... 
  • rustfmt also deletes the comments I added between if branches. This is really annoying since information is actually lost:
                         // No color if nothing changed.
-                        if cell == cell_ref { buffer.push('1'); }
-                        // Color for 1 if we filled in a blank.
-                        else if *cell_ref == None { write!(&mut buffer, "\u{1b}[33m1\u{1b}[0m").unwrap(); }
-                        // Red for error if we overwrote a 0 (should never happen).
-                        else { write!(&mut buffer, "\u{1b}[31m0\u{1b}[0m").unwrap(); }
-                    },
+                        if cell == cell_ref {
+                            buffer.push('1');
+                        } else if *cell_ref == None {
+                            write!(&mut buffer, "\u{1b}[33m1\u{1b}[0m").unwrap();
+                        } else {
+                            write!(&mut buffer, "\u{1b}[31m0\u{1b}[0m").unwrap();
+                        }
+                    }

Suggestions

  • Sometimes there is visual layout that is easier to read before rustfmt compared to after. Could it be possible to retain those purposeful newlines?
-        self.0[row][col].is_some()
-            && self.check_cell_rule1(row, col)
-            && self.check_cell_rule2(row, col)
-            && self.check_cell_rule3(row, col)
+        self.0[row][col].is_some() && self.check_cell_rule1(row, col) && self.check_cell_rule2(row, col) &&
+        self.check_cell_rule3(row, col)
  • a similar case is for multiline strings where the visual layout is again meaningful:
-const USAGE_STRING: &'static str =
-r#"Usage: tackle [FILE]...
+const USAGE_STRING: &'static str = r#"Usage: tackle [FILE]...
...
...
"#;
  • IMO, rustfmt should keep blocks on one line when they contain only one short instruction and/or a return statement (or at least provide an option to do so), as in:
-                    Some(true) => { buffer.push('1'); }
-                    Some(false) => { buffer.push('0'); }
-                    None => { buffer.push('.'); }
+                    Some(true) => {
+                        buffer.push('1');
+                    }
+                    Some(false) => {
+                        buffer.push('0');
+                    }
+                    None => {
+                        buffer.push('.');
+                    }
  • imports reording could be case-insensitive (except when two imports have the same name i.e. Write vs write)
- use std::io::{stderr, stdin, Write};
+ use std::io::{Write, stderr, stdin};
  • Option to not add a semi-colon after a return, break or continue statement.

  • Option to add/keep the comma after the blocks in match statements. Arguably, it provides a better readability of what closing brace closes what when there are many levels of nested blocks.

  • Although line comments are idiomatic in Rust, line comments are good for code and short comments. In some cases it doesn’t make any sense to be able to decomment a single line from a block of comments. e.g. for a license:

-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.

#12

Thanks for all the comments! I’ll try to address some here, but there’s a lot, so sorry if I missed some. There are issues filed for a lot of the issues in this thread, in other cases we should probably add options. I filed a bunch of new issues too. Some of the bugs mentioned here have already been fixed. Where you like a formatting option which is not supported - adding an option to rustfmt to do things the way you like is usually fairly easy (easier than implementing formatting for an item from scratch), but is unlikely to be tacked soon unless it is a really popular alternative - so these are great ways to get stuck in and send a PR :slight_smile:

In general, there are always going to be some places where there is significant formatting that Rustfmt can’t be expected to know about and will mess it up. Obviously we want as few of these instances as possible, but there will always be some and there must be an easy way to opt out of reformatting there. Currently we support the #[rustfmt_skip] attribute to do this, but that only works in nightly Rust and can only be put on items, so it a bit limited. Short term, we should add better ways to opt out in Rustfmt. Long term, attributes in more places in Rust, and stable, scoped attributes will be solutions.

Comments are probably the biggest pain point in Rustfmt at the moment. In the short-term, I just added an option to skip wrapping comments, we should add another soon to skip converting one kind of comment to another. We’re looking at fixing some of the bugs, of course. Longer-term having more info from the compiler about comments is on the agenda and that will help Rustfmt a lot.

Rustfmt does tend to prefer a more spacious and visually indented style, I believe this is easier to scan code quickly, but I would love to see studies on this. Mostly I think we like what we are used to. The downside of this is that we often introduce more rightward drift (which Rust already suffers from, more than many languages). A good solution is extracting more variables for sub-expressions. Giving them good names leads to better self-documenting code, so this is often a good thing anyway.

Re inconsistent line length, Rustfmt has the max line width, but also has various heuristics for breaking lines before this is reached, in particular there is a max width for arguments in a function call and struct literals - these were added to match what was happening manually in the Rust repo - people don’t use the max line width for long function calls because they’re hard to read.

Chains of methods/fields often seem to be surprising. (E.g., foo.bar().baz().qux()). It’s hard to come up with a scheme which is both consistent and implementable here. We ended up going for one call per line with the .s aligned (where we can’t fit the whole chain on one line). This is not a style I used before, but I like it now - it ends up isolating operations to lines and is easy to scan. The precedence of the formatting reflects the precedence of the code. E.g., in

+        let string_offsets: Vec<u16> = try!((0..string_offsets_count)⏎
+                                                .map(|_| read_le_u16(file))⏎
+                                                .collect());⏎
-        let string_offsets: Vec<u16> = try!((0..string_offsets_count).map(|_| {⏎
-            read_le_u16(file)⏎
-        }).collect());⏎

The formatted code puts the closure on one line with the map since that is one operation - the original separates the map to the function being mapped over the iterator and since the collect does not start a line, it is easy to miss on scanning. Admittedly, this does lead to rightward drift, but this can be fixed with a variable, if it is really bad.

We’ve added --verbose to give feedback on what is being formatted (it could be improved with output too). We should add --version.


#13

For me it fails on most every file because the line exceeded maximum length or because it left behind trailing whitespace. I know I don’t have great formatting right now, but that’s why I’m interested in the tool.


#14

Rustfmt doesn’t currently seem to reformat the spacing of var : Type to whatever is the deemed norm, so sometes I see var: Type vs var : Type or any variation.

Would be an awesome addition


#15

@DrKwint - could you give some specific examples please? The only known areas where over-long lines are a problem are over-long comments, macro definitions, and some macro uses. If you have examples of other places where this is a problem, I’d like to investigate. Trailing whitespace is even rarer - examples here would be good because I’m not aware of any outstanding issues (maybe in comments?).

@bfrog - what is the context? I tried in both function arguments and let statements and couldn’t reproduce. We should be fixing this, so its a bug where we aren’t.