New style guidelines

The style team has been discussing new style/code formatting guidelines for Rust (On the fmt-rfc repo). I've been implementing many of these changes in Rustfmt (although there is a lot of work still to do there). I'd like to get feedback on the new style, especially where it differs from what we currently use in the Rust repo.

Please take a look at this example code, especially the more complex function signatures, for example.

If you want to try this yourself, you need to build your own Rustfmt from master to get the latest version. You can then use the rfc-rustfmt.toml config file (renamed to rustfmt.toml) to use the new guidelines on your project. Note that this is currently work in progress - formatting of items such as types and function signatures should be good, but expressions and statements are a bit hit and miss.

Let me know what you think here, or open issues/leave comments on the fmt-rfcs repo.

Thanks!

22 Likes

This all looks really good with one big exception: where clause formatting. I find the formatting in the examples extremely distracting. I've gravitated towards formatting like so:

    fn func<F, B>(
        foo: &F,
        bar: &B
    ) -> Result<()>
        where F: Foo,
              B: Bar
    {

The where itself has block indentation, and following lines have visual indentation. This keeps where out of appearing as a top level keyword here (it's part of pub fn, not a thing next to it), and it conserves vertical space without being cluttered. I believe the result of the "visual vs block" indentation was "block, with a few exceptions", so hopefully that's not an argument against this. Would love to hear other reasoning why the proposed guidelines would be preferable.

31 Likes

Discussion of where clauses is in https://github.com/rust-lang-nursery/fmt-rfcs/issues/38

To summarise, we preferred block formatting over visual for consistency with other places (arguments, generics, etc.). We felt that extra indent was not necessary since both the arguments and body are indented (iirc).

2 Likes

I scanned through that issue, and it seemed like everyone preferred the visual indentation, but then there was a style meeting and everyone changed their mind. I cannot find the rationale for the abrupt chance in consensus.

3 Likes

IIRC, the consensus was that we should prefer block indent in general, and there wasn't a good reason to avoid it here. Also, while experimenting with the various combinations of multiple lines, combining with previous lines. etc., we found it worked well. I don't recall if there were specific examples where visual indent did not work. Perhaps @josh or @scott remember?

In that case, I would like to propose some reasons against it here:

  • There is already momentum in many prominent projects for visual.
  • Without strong motivation for the change, fighting that momentum is a tough battle.
  • The proposed block indentation style is almost unseen in the wild -- the first time I've seen it (either in code or discussion) is this RFC

Also, while experimenting with the various combinations of multiple lines, combining with previous lines. etc., we found it worked well.

Seems like this is true of both solutions.

9 Likes

@jwilm

The proposed block indentation style is almost unseen in the wild -- the first time I've seen it (either in code or discussion) is this RFC

For what it's worth, my experience is that I never found a satisfactory where style in the wild, and almost every person did it a different way. This block indenting style was the first one that felt somewhat carefully designed to avoid problems, even if it feels a bit odd at first. That said, there might be some better ways to tweak it.

The where itself has block indentation, and following lines have visual indentation

The proposed where<T: Bound> feature that @withoutboats brought up is a fairly big motivation against visual indent. It leads to all the same problems that made us ultimately decide in favor of block indent in this issue.

If not for this problem I would be more open to the idea (since it would only align to a fixed-width keyword). @withoutboats: Can you weigh in on how likely it is that this feature will be added?

This all looks really good with one big exception

I'm glad we agree on the rest. I also think it looks really good. :slight_smile:

2 Likes

One thing I don't see here is how we will handle line wrapping when the line is too long. Can someone post the strategies used?

Personally I think rustfmt makes too much code happen to the right side of the screen like this

this_is_long(foo, 
             bar) 

when it should be

this_is_long(
    foo, bar)
5 Likes

Everything looks awesome, but have to say the where clause is a little distracting as well, this example:

pub fn itemize_list<'a, T, I, F1, F2, F3>(
    terminator: &'a str,
    get_lo: F1,
    get_hi: F2,
    get_item_string: F3,
    prev_span_end: BytePos,
    next_span_start: BytePos,
) -> ListItems<'a, I, F1, F2, F3>
where
    I: Iterator<Item = T>,
    F1: Fn(&T) -> BytePos,
    F2: Fn(&T) -> BytePos,
    F3: Fn(&T) -> Option<String>,
{
...
}

I think it's mostly the extra newlines are a visually striking for me.

pub fn itemize_list<'a, T, I, F1, F2, F3>(
    terminator: &'a str,
    get_lo: F1,
    get_hi: F2,
    get_item_string: F3,
    prev_span_end: BytePos,
    next_span_start: BytePos,
) -> ListItems<'a, I, F1, F2, F3>
where I: Iterator<Item = T>,
      F1: Fn(&T) -> BytePos,
      F2: Fn(&T) -> BytePos,
      F3: Fn(&T) -> Option<String>,
{
...
}

The { on the new line does create more visual distinction since the next line will be indented, so I can really go 50/50 on that. Edit: removed the newline diff suggestion.

Edit2: I really like this proposal from @stebalien https://github.com/rust-lang-nursery/fmt-rfcs/issues/38#issuecomment-288441318

1 Like

You are touching on the difference between visual and block indent, and indeed the new guidelines have a preference for block rather than visual. You can see this in some of the longer function declarations.

Yeah, this is basically a show-stopper for me - we need the newline brace otherwise the where clause and body blend together.

3 Likes

We put the { for a function on a new line when we have a where clause, because otherwise the where clause and the function body tend to run together.

Agreed, I updated to remove, that's more obvious to me now. I realize in my own code I end up with that newline anyway in many cases...

On the where clause though, that extra newline after the where feels unnecessary. I don't know that I personally care about the indentation of the where itself as in @jwilm 's suggestion... I'd be happy either way.

During the style team meeting, we experimented with several different styles, both block and indent. We seriously considered visual style here, despite using block indent almost everywhere else, specifically because the keyword where has a fixed width so it wouldn't result in as many ripple effects across lines when editing.

One of the bigger motivating factors to stick with block indent: without it, the where clause has a tendency to blend into the function signature and return type. When you have a complex type in the last argument, then a complex type in the return type, and then a complex type in the where clause, the three run together and make it hard to scan through as quickly. Each time we considered a rule with a visual indent, and applied it to a sample like that, we ended up finding the result harder to read. With the where keyword on a line by itself (or on the same line as the close paren when you have no return type), the where clause ends up more visually offset and easier to read at a glance.

4 Likes

Pretty happy with how function signatures have turned out.

I'll need to run my own code through this for evaluating expressions; the sample code seems relatively boring in those terms (although I still see a few places where you're using "full" blocks more aggressively than I would).

Attributes

Why attributes/derives above docs? I can't find an issue for this. For items with long doc comments, it will be really hard to figure out what struct/function the attributes apply to.

Where

I agree that the new syntax is objectively better (after getting over my initial knee-jerk reaction). However...

Compatibility

Changing this at this point will cause a lot of code churn and/or inconsistent code. I'm not convinced it's worth it.

Aesthetic/Readability

Whenever I see two identifiers at the same indent level, I immediately assume that they have a sibling/sequential relationship, not a parent-child relationship. This is why I've always hated the following C declaration syntax:

void
my_function()
{
    return;
}

If you're going to go with this new where syntax, how about indenting where by two spaces (half indent). I believe Haskell does something similar.

pub fn itemize_list<'a, T, I, F1, F2, F3>(
    codemap: &'a CodeMap,
    inner: I,
    terminator: &'a str,
    get_lo: F1,
    get_hi: F2,
    get_item_string: F3,
    prev_span_end: BytePos,
    next_span_start: BytePos,
) -> ListItems<'a, I, F1, F2, F3>
  where
    I: Iterator<Item = T>,
    F1: Fn(&T) -> BytePos,
    F2: Fn(&T) -> BytePos,
    F3: Fn(&T) -> Option<String>,
{
    ListItems {
        codemap: codemap,
        inner: inner.peekable(),
        get_lo: get_lo,
        get_hi: get_hi,
        get_item_string: get_item_string,
        prev_span_end: prev_span_end,
        next_span_start: next_span_start,
        terminator: terminator,
    }
}

This way, it's clear that the where clause is part of the item and doesn't start a new one.

12 Likes

I still support it, but that's as far as I can say. This where syntax is what I've taken to using as my projects have grown more and more complex bounds.

All in all this proposal is consistent with how I already write code, except for aligning matchrockets, which I expected. So I'm happy :slight_smile:

Oh, thanks for pointing this out! I didn't notice this, and I 100% agree with you. I don't think it has been discussed previously, so I opened a fmt-rfcs issue.

I'm not sure if I get what you mean. There is no existing standard Rust style, and whatever style we choose is going to be different from most existing code in a bunch of little ways, but updating code style will be automated with rustfmt and can be done all at once when we're done the style guide RFC process.

Please bring this up on the where clauses issue. :slight_smile:

FWIW, I'm happy with the proposed use of block indent everywhere.

5 Likes

Most code I've read (and written) uses the visual indent style (the default style enforced by rustfmt).