Function definitions

[joshtriplett]

Part of #25: how should we format and indent function definitions? This includes arguments and their types, generics and their constraints, and the braces; the body of the function then falls under #11. This also interacts with where clauses (#38), which we may want to fold in.

[nrc]

Generics are also covered elsewhere: #29. I'd prefer to keep that and where clauses separate since they also affect many other items.

For reference the current style used in the compiler and std libs and enforced by Rustfmt is:

fn extract_one(&mut self,
               m: FxHashMap<PathBuf, PathKind>,
               flavor: CrateFlavor,
               slot: &mut Option<(Svh, MetadataBlob)>)
               -> Option<(PathBuf, PathKind)> {
    ...
}

To summarise:

• keywords/names are space-separated
• as much as possible on one line, break arguments before generics
• if args are broken:
  • one arg per line
  • pattern and type on one line, space after : only
  • comma terminated
  • visual indented
  • return type on a newline and visual indented with args
• opening brace (or semi-colon) on the same line as return type
• body is block indented.

I think the main alternative is to block indent the args, where there are a few options.

[joshtriplett]

One notable interaction with where clauses: if a function needs a where clause, do you then put the opening brace on the last line of the where clause, or at the start of the following line?

A few variations on block-indentation that I've seen in real code (again, assuming everything doesn't fit on one line):

/* 1: close paren, return type, and brace on one line */
fn extract_one(
    &mut self,
    m: FxHashMap<PathBuf, PathKind>,
    flavor: CrateFlavor,
    slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)> {
    ...
}

/* 2: close paren and return type on one line, brace on next line */
fn extract_one(
    &mut self,
    m: FxHashMap<PathBuf, PathKind>,
    flavor: CrateFlavor,
    slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)>
{
    ...
}

/* 3: close paren with last argument, return type and open brace on next line */
fn extract_one(
    &mut self,
    m: FxHashMap<PathBuf, PathKind>,
    flavor: CrateFlavor,
    slot: &mut Option<(Svh, MetadataBlob)>)
    -> Option<(PathBuf, PathKind)> {
    ...
}

/* 4: close paren with last argument, return type on next line, open brace on next line. */
fn extract_one(
    &mut self,
    m: FxHashMap<PathBuf, PathKind>,
    flavor: CrateFlavor,
    slot: &mut Option<(Svh, MetadataBlob)>)
    -> Option<(PathBuf, PathKind)>
{
    ...
}

Personally, I prefer the first or second option. The third and fourth break the "comma terminated" property.

Of the first and second, I mildly prefer the first (brace on the same line), for consistency with the one-line case. The second will look slightly more familiar to C/C++ users, and provides consistency with the where case.

[nrc]

One notable interaction with where clauses: if a function needs a where clause, do you then put the opening brace on the last line of the where clause, or at the start of the following line?

I think this affects traits, impls etc. in the same way as functions, so we can just discuss that with the rest of where clauses

Thanks for the block layouts! I think 3 is a non-starter because there is no separation between the args/return type and the body. The others look OK to me, I guess I prefer 1 over the others. Is there consensus that we should linebreak before the first argument?

Another option (either block or visual) is to put more than one arg/type pair on each line. I dislike this option, but just putting it out there.

[solson]

+1 for option 1. I find this style really easy to work with (even right now when I don't have rustfmt helping me).

I definitely prefer linebreaking before the first argument in this style.

[joshtriplett]

I prefer the first style with the linebreak before the first argument as well.

[bruno-medeiros]

Prefer option 2 or 1. Some preference towards 2 because it works better for functions with where clauses:

fn extract_one<FOO, BAR>(
    m: FxHashMap<PathBuf, PathKind>,
    slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)>
where 
    FOO : blah::FooTrait + Send,
    BAR : blah::BarTrait + 'static, 
{
    ...
}

I think it's better for the opening brace '{' to start on the same place regardless of the function having a where clause or not - it's a more uniform rule.

[joshtriplett]

I don't find it problematic to move the brace for a where clause, and it seems worth the compactness in the common case.

[nrc]

I don't find it problematic to move the brace for a where clause, and it seems worth the compactness in the common case.

Agreed, that is what we currently do in Rustfmt and std/compiler and it works pretty well.

[quodlibetor]

Something that hasn't been brought up here (but I've seen allusions to elsewhere) is the amount of block indent, assuming block indent is chosen. The thing that I like most about visual indent is the clear separation of blocks. To my mind, function parameters, where clauses, and the function block itself should not be indented the same.

Given this (from above):

(1)

fn extract_one<FOO, BAR>(
    m: FxHashMap<PathBuf, PathKind>,
    slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)>
where 
    FOO : blah::FooTrait + Send,
    BAR : blah::BarTrait + 'static, 
{
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

I would much prefer indentation that draws distinction to the various clauses. where is nice because it's an odd number of letters, so its parameters are automatically oddly indented. The code above manages to break that, even though where is never going to change length. I'd prefer something more like:

(2)

fn extract_one<FOO, BAR>(
        m: FxHashMap<PathBuf, PathKind>,
        slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)>
where FOO : blah::FooTrait + Send,
      BAR : blah::BarTrait + 'static, 
{
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

Maybe m and slot should be indented with 6 spaces to match the indent of where clauses?

(3)

fn extract_one<FOO, BAR>(
      m: FxHashMap<PathBuf, PathKind>,
      slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)>
where FOO : blah::FooTrait + Send,
      BAR : blah::BarTrait + 'static, 
{
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

I don't love what that does to the return value, I think of that as coming "after" the parameters:

(4)

fn extract_one<FOO, BAR>(
      m: FxHashMap<PathBuf, PathKind>,
      slot: &mut Option<(Svh, MetadataBlob)>,
        ) -> Option<(PathBuf, PathKind)>
where FOO : blah::FooTrait + Send,
      BAR : blah::BarTrait + 'static, 
{
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

That's... hideous.

Without where clauses:

(5)

fn extract_one<FOO, BAR>(
      m: FxHashMap<PathBuf, PathKind>,
      slot: &mut Option<(Svh, MetadataBlob)>,
        ) -> Option<(PathBuf, PathKind)> {
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

or maybe:

(6)

fn extract_one<FOO, BAR>(
      m: FxHashMap<PathBuf, PathKind>,
      slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)> {
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

Not great, but I still think a 4-space indent is worse:

(7)

fn extract_one<FOO, BAR>(
    m: FxHashMap<PathBuf, PathKind>,
    slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)> {
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

[regexident]

To my mind, function parameters, where clauses, and the function block itself should not be indented the same.

This would drive me nuts. 👎

[quodlibetor]

I understand your opinion (when I was first exposed to this idea I hated it) but it does a pretty good job with the pathological case, which I forgot to include, and which is the thing that I most want to avoid:

(8)

fn extract_one<FOO, BAR>(
    m: FxHashMap<PathBuf, PathKind>,
    slot: &mut Option<(Svh, MetadataBlob)>) {
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

compare to:

(9)

fn extract_one<FOO, BAR>(
        m: FxHashMap<PathBuf, PathKind>,
        slot: &mut Option<(Svh, MetadataBlob)>) {
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

and

(10)

fn extract_one<FOO, BAR>(
      m: FxHashMap<PathBuf, PathKind>,
      slot: &mut Option<(Svh, MetadataBlob)>) {
    let something = woah()
    for thing in my_real_code() {
        do_something_else()
    }
}

Again, I don't really care how block indenting happens, I just don't like it when it's hard to tell when function parameters end and function bodies begin. Similar to the reason that I prefer visual indent for method chains, it seems worth having "lines of thought" that distinguish different kinds of action.

[nrc]

Just to be clear, although I expressed a preference for the one of the block-formatted options. I massively prefer the visually indented option. I think that it looks better because we don't have a (\n. I feel that because of the lack of nesting, most of the arguments in favour of block formatting don't apply here, most editors make visual indentation easy to apply, and it is consistent with much code already in the wild (in particular, the std libs). The only reason I'd prefer block indentation is for consistency with expressions, where it seems very likely we'll prefer block indentation. However, I think the benefits of aesthetics and consistency with existing code outweigh that.

[joshtriplett]

The main downside of visual indentation here: if you change the function name, or if you change the generic parameters, you'll have to re-indent every line.

I expect the common case to remain a one-liner. This only matters for functions with enough parameters to need to wrap to multiple lines. And in those cases, I still prefer block indent for consistency.

[nrc]

The main downside of visual indentation here: if you change the function name, or if you change the generic parameters, you'll have to re-indent every line.

My experience is that this is trivially easy to do manually in any decent editor (again in contrast with expressions, where it seems to often require reformatting). And of course Rustfmt will do it for you even more easily.

I expect the common case to remain a one-liner.

Agreed, although 'common case' here means maybe 66% or 75%, rather than 90% or 99%

[solson]

Visual indentation often pushes the arguments much too far to the right, forcing the arguments themselves to be further broken into multiple lines to fit. In my experience it creates a mess far too often to be worthwhile.

[nrc]

Visual indentation often pushes the arguments much too far to the right, forcing the arguments themselves to be further broken into multiple lines to fit. In my experience it creates a mess far too often to be worthwhile.

This seems to be very rare in practice in the compiler/std libs. Do you know why you might see it more often? (Long names, nesting, smaller column limit, long type names?)

[joshtriplett]

@nrc Medium-length function names, plus generic parameters with medium-length trait names, plus a parameter with a medium-length name and a medium-length type name. Not hard to hit 100 characters that way.

[joshtriplett]

@nrc

My experience is that this is trivially easy to do manually in any decent editor

It's fairly doable (though not always trivial), but it produces a half-dozen lines of changes instead of a single line. Diff noise aside, that makes it harder to see what actually changed. For instance, consider changing a generic type parameter and one or two arguments using that parameter; with block indent, you can easily see from the diff that you changed the type parameter and those specific arguments.

[nrc]

plus generic parameters with medium-length trait names

Hmm, the compiler tends to always use where clauses, but otherwise we have plenty of those. Even with space for indent and punctuation, you'd need to average ~30 chars for each name to hit the 100 char limit, which seems more like long than medium to me.

that makes it harder to see what actually changed

I don't have this problem - GitHub at least is smarter at highlighting what changed within a diff, as are some diff tools. Even where such support doesn't exist, it's never been something that actually slowed me down reading a diff, but YMMV, I guess.

[solson]

@nrc On reflection, I'm remembering the visual indent expression mess rather than declarations. It was the visual indented function calls that made librustc_driver so difficult for me to follow when I was new to rustc.

That aside, I think visual indent sticks out like a sore thumb when combined with block indent, so I disagree with using it for something as pervasive as function definitions when most code will be block indented.

[joshtriplett]

@nrc You'd have more than just three names there: a function name, at least one trait name (if not a couple), a parameter name, and a type name (possibly with type parameters). Together with the various required punctuation, that makes it much easier to hit.

fn write_one_useful_thing<W: io::Write, T: AsRef<SomeType>>(out: W,
                                                            some_parameter: Option<UsefulType<String>>,
                                                            ...

That second line already hits 103 characters, and this doesn't seem contrived to me. Throw in a lifetime, or put this in an impl block, or use a pattern instead of a parameter name, and it gets even worse.

Yes, this could improve by using a where clause, but it could also improve via block indentation. :)

[DanielKeep]

More or less complete progression of the style I use; apologies for the length:

fn name<G>(param) -> Return where predicate { body }

fn name<G>(param) -> Return
where predicate: non_trivial {
    body
}

fn name<G>(param) -> Return
where
    predicate a,
    predicate b,
{
    body
}

fn name<G>(param)
-> ReallyQuiteAbsurdlyLongButItCanHappen
where
    predicate a,
    predicate b,
{
    body
}

fn name<G>(param a, param b, param c)
-> ReallyQuiteAbsurdlyLongButItCanHappen
where
    predicate a,
    predicate b,
{
    body
}

fn name<G>(
    param a,
    param b,
    param c,
) -> ReallyQuiteAbsurdlyLongButItCanHappen
where
    predicate a,
    predicate b,
{
    body
}

fn name<G, H, I, J, K>(
    param a,
    param b,
    param c,
) -> ReallyQuiteAbsurdlyLongButItCanHappen
where
    predicate a,
    predicate b,
{
    body
}

fn name<
    ABetterName,
    LotsOfRoomDontSkimp,
    Iterator,
    IteratorItem,
    IveWrittenWorseBefore,
>(
    param a,
    param b,
    param c,
) -> ReallyQuiteAbsurdlyLongButItCanHappen
where
    predicate a,
    predicate b,
{
    body
}

Generally, I'll prefer to keep things together until there's enough on a line that it can't be broken up quickly by just looking at it. where tends to get bumped to a second line pretty quickly, since predicates can very easily become visually noisy.

Also, the above is not intended to establish any sort of hard "up to three parameters" limit; it's always subject "what looks better".

The major reason I go with this style is because, assuming I have some monster of a function definition, the whole thing is easy to break into "blocks": generics block, param block, return line (or in pathologically awful calendar-related cases, block), where block, body. I can just scan down the left-hand side of the code looking for the relevant "sigil" to find the block I want.

Again, usual vote against visual indentation: it makes the code horrible to read because my eyes have to skip all over the damn place. It's like a tennis match during an earthquake: it constantly goes back and forth, and there's chasms of empty space opening up everywhere.

[joshtriplett]

@DanielKeep One case you didn't cover: How would you format a ReallyQuiteAbsurdlyLongButItCanHappen return type with no where clause?

[DanielKeep]

@joshtriplett bangs head on desk I went overboard to try and forestall "but what about..." comments.

fn name<G>(param)
-> ReallyQuiteAbsurdlyLongButItCanHappen {
    body
}

The { doesn't collapse into the last where clause, because that would cause diff churn (unless there's only one).

[bruno-medeiros]

This seems to be very rare in practice in the compiler/std libs. Do you know why you might see it more often? (Long names, nesting, smaller column limit, long type names?)

True, but the compiler/std libs world might be a bit idiosyncratic, and not representative of most Rust code? I've found that compiler/std-lib names tend to be shorter, in part because they tend to be more "primitive" in functionality, and more likely to be used often by everyone else.

Also compiler/std-lib nearly always uses one letter names for type parameters, something which also seems very common in other Rust code - however I'd argue this isn't necessarily ideal, or at very least shouldn't be a style forced onto every crate.

I tend to prefer longer function names, and actual words for type parameters, not just letters. Longer function names are also more common when you have overloading and need to disambiguate function name names. For example, here are several methods that would be visually indented around columns 50-60, either because of medium names and type parameters:
https://github.com/RustDT/RustLSP/blob/master/subcrates/melnorme_json_rpc/src/jsonrpc.rs#L220
https://github.com/RustDT/RustLSP/blob/master/subcrates/melnorme_json_rpc/src/jsonrpc.rs#L302
or just because of a long name:
https://github.com/RustDT/RustLSP/blob/master/src/lsp_server.rs#L54

[alexcrichton]

I personally agree with @nrc in that the visually indented function signatures aesthetically look better to me here at least. I think we should really not consider "one change changes a lot of lines" or "diffs are harder to read" as hard constraints but rather just points. The former is fixed as we have a tool to do all our formatting and the latter is optimizing for the wrong thing. I voiced concerns in the alignment issue that we should always be optimizing for readability first. Concerns like diffs and changes, while important, I think should always be secondary to actually reading function signatures.

For example styles I've seen like:

fn foo(
    a: A,
    b: B,
    c: C,
) -> D

to me seem like overkill for a function signature. All of a sudden there's a massive cliff between "fits on on line" and "takes up half the screen".

[solson]

we should always be optimizing for readability first

I agree, but optimizing for readability is why I've pushed for block indent, as I found visual indent disruptive and confusing when I was learning the rustc codebase (especially since rustfmt has a hard time making visual indentation work well).

For the case of basic function definitions visual indent has an easier time, but it looks increasingly weird to me when you add in generics lists, return types, and where clauses. @DanielKeep's suggestions are super easy for me to follow and understand by comparison.

the latter is optimizing for the wrong thing

I have to strongly disagree here. Optimizing for code review readability is a really import part of optimizing for readability.

[nrc]

especially since rustfmt has a hard time making visual indentation work well

I don't think this is true of signatures, right? Rustfmt is (IMO) pretty good there. It is nesting expressions and visual indent that seem to trip it up

[nrc]

especially since rustfmt has a hard time making visual indentation work well

I don't think Rustfmt has problems with function signatures, I think it is only nesting with visual indent that trips it up

I've found that compiler/std-lib names tend to be shorter

There are certainly lots of long names in the compiler (though not so much in the libs), especially the more modern parts of it.

For example, here are several methods that would be visually indented around columns 50-60, either because of medium names and type parameters...

And all of these would fit inside the 80 col limit if visually indented, let alone a 100 col limit, so this seems like an argument that the rightward drift here is acceptable.

[LukasKalbertodt]

About the visual vs block indentation: there is a dedicated discussion about this issue, right? Shouldn't that topic be discussed over there to avoid noise in this thread?

For consistency, I'd say that function signatures should follow the rule (visual OR block) that will be determined in the linked discussion. Having a different rule for one specific use case (like function declarations) seems wrong to me.

[aturon]

@nrc My preference here is not very strong; I think there's a good chance that it's just a matter of what I'm used to.

[joshtriplett]

On Tue, Jan 17, 2017 at 06:49:48PM -0800, Nick Cameron wrote: We currently (and afaik intend to continue) put the brace on a newline when there is a `where` clause. It is something we could consider doing in all cases if others agree with @aturon. My personal opinion is that there is enough separation without this and that the benefit in consistency is outweighed by the cost in vertical space, esp for short functions.

Where-clause placement aside, I've seen a strong preference in the Rust community for placing the opening brace of a function on the same line as the closing parenthesis. That style differs from C and C-like languages (where even K&R formatting still puts a newline before a function opening brace), but it has grown on me over time. And I don't think we should lightly attempt to change that entire style across the community.

[softprops]

I really think this process is awesome. I have a suggestion that may help solicit better feedback. At the point where there appears to be the beginnings of some consensus, publish a preview release with those preferences implemented behind a config option behind a feature flag. Have the interested parties apply the feature flagged option to their own code to get a better picture of what the future changeso would looks like in practice. Gather that feed back and decide.

[allengeorge]

Strong preference for block over visual indents. Weaker preference for all-or-nothing block indents.

[johnthagen]

Just wanted to add support for visual indents along the same lines as @alexcrichton and I think @nrc

I personally agree with @nrc in that the visually indented function signatures aesthetically look better to me here at least... that we should always be optimizing for readability first

If rustfmt were to one day be distributed with rustup, so it was more of a defaultly available tool (clippy too?) then hopefully those wanting to follow the Rust style guide would have access to the tool that does all of the work for them.

[devonhollowood]

I have a fairly strong preference for block indents. Sometimes I want a long function name in order to be descriptive about what a function does, and if this is combined with a long type name in one of the arguments visual indenting can get pretty messy.

[nrc]

We briefly discussed this at the style team meeting. We think we are ready to close out the FCP. We agreed that it is an important item, from a formatting perspective and thus Rustfmt should handle it sooner rather than later. I'll try to implement the details here asap. If anyone wants to help out, I'd be grateful and happy to mentor (rust-lang/rustfmt#1303).

Summary

Where the whole function signature fits on one line (including the opening brace), do that. If the body of the function is empty, then the closing brace can go on the same line as the signature.

If the signature would overflow a single line, prefer block formatting of the arguments, e.g.,

fn extract_one(
    &mut self,
    m: FxHashMap<PathBuf, PathKind>,
    flavor: CrateFlavor,
    slot: &mut Option<(Svh, MetadataBlob)>,
) -> Option<(PathBuf, PathKind)> {
    ...
}

Generics and the where clause are formatted independently of the arguments (and will be described elsewhere).

We will not use 'intermediate' formattings between the fully horizontal and fully vertical approaches.

[softprops]

Last update on this issue was about a month ago. And more recent updates? This is the rfc Im looking forward to the most

[nrc]

We've changed our process: rather than requiring a full RFC, a PR to close this issue only requires making the changes discussed here to the style guide. You can see more details about the process in the readme for this repo.

If you'd like to help with this work, let me know, I'd be very happy to help you help us.