Function definitions #39

Open
joshtriplett opened this Issue Nov 15, 2016 · 61 comments

Projects

None yet
@joshtriplett
Member
joshtriplett commented Nov 15, 2016 edited

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
Collaborator
nrc commented Nov 15, 2016

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
Member
joshtriplett commented Nov 15, 2016 edited

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
Collaborator
nrc commented Nov 15, 2016

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
Member
solson commented Nov 15, 2016

+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
Member

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
Member

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
Collaborator
nrc commented Nov 15, 2016

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
regexident commented Nov 15, 2016 edited

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
Collaborator
nrc commented Nov 15, 2016

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
Member

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
Collaborator
nrc commented Nov 15, 2016

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
Member
solson commented Nov 15, 2016

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
Collaborator
nrc commented Nov 15, 2016

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
Member

@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
Member

@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
Collaborator
nrc commented Nov 15, 2016

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
Member
solson commented Nov 15, 2016 edited

@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
Member

@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
Member

@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
Member

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
Member
solson commented Nov 16, 2016

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
Collaborator
nrc commented Nov 16, 2016

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
Collaborator
nrc commented Nov 16, 2016

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.

@LukasKalbertodt

I agree a lot with @DanielKeep's comment, but I'd like to propose a few changes to their style (numbered, in case someone wants to argue about a specific one):

// (1) If it doesn't fit on one line, first try to break the return type only!
// (2) Indent the return type by one block!
fn function_name<T>(param: T, paramb: type)
    -> ReturnType
{
    body;
}

// (3) Never put a `where`-clause on the same line with `fn`. If it's such a short
// and easy where-clause, just specify it inline in the generic parameter 
// list. (4) If it's not, use a separate line for the where-clause.
fn function_name<T: Clone>(param: T) -> Return {
    body;
}


// Also: (5) the opening '{' of the body sits unindented on its own line whenever 
// the whole function signature doesn't fit on one line
@solson
Member
solson commented Nov 20, 2016

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?

There is a dedicated thread, but we already reached a basic decision there that "we like the use of block formatting as a principle, and that while there may be exceptions, these should be well-motivated and rare."

So now to avoid a lot of discussion diving into the details of various different features in one thread, we'd rather discuss the details in individual feature threads. These discussions should keep in mind the principle decided in that thread, though.

@regexident
regexident commented Nov 22, 2016 edited

(3) Never put a where-clause on the same line with fn. If it's such a short and easy where-clause, just specify it inline in the generic parameter list.

(4) If it's not, use a separate line for the where-clause.

I have come to prefer trait bounds to always be found in a separate where clause regardless of the number/length of bounds. (<T: U, …> doesn't scale, so where T: U, … it is.)

I don't want to have to look at multiple potential syntax positions for a single semantic thing/concept. Alternating declaration positions introduce ambiguities when visually scanning code.

Imagine having to search for a fn's parameters both within the parentheses …

fn function_name(foo: T, bar: U) -> Return {
    body;
}

… as well as at the end of its signature …

fn function_name() -> Return
    where (foo: T, bar: U, baz: V)
{
    body;
}

… depending on whether the signature fits into a single line.

We don't allow this for function parameters, and imho shouldn't allow (or at least not encourage) this for bounds either.

Even worse, having bounds at both positions:

fn function_name<T: Clone>(param: T) -> Return
    where T: LongTraitName + AnotherVerboseTraitName
{
    body;
}

The latter I'd actually consider a code smell that rustc/clippy should be warning about.

@LukasKalbertodt

There is a dedicated thread, but we already reached a basic decision there that "we like the use of block formatting as a principle, and that while there may be exceptions, these should be well-motivated and rare." [...]

Oh thanks, that makes sense!

I have come to prefer trait bounds to always be found in a separate where clause regardless of the number/length of bounds. [...]

I think I mostly agree with that. But this is a separate issue, right? I don't think function definitions should follow a different style regarding where than other situations in which a where can appear. But maybe I'm wrong again 😉

Anyway, in my opinion, where should still belong on a separate line. Parsing the return type and where-clause in one line with my eyes is not as easy as if I knew "the return type is the last thing in the line".

@aturon
aturon commented Nov 22, 2016

@alexcrichton

All of a sudden there's a massive cliff between "fits on on line" and "takes up half the screen".

I think this is a good concern to keep in mind, but I do want to point out one thing. There are many potential rules for how block indent might work here. We might say, for example, that if we block indent the args and then they all fit on one line, we keep them:

// fits entirely in one line
fn a_very_long_function_name(arg1: Type1, arg2: Type2) -> ReturnType

// goes to block indent, but keeps args together
fn a_very_long_function_name(
    arg1: Type, arg2: Type, arg3: Type, arg4: Type, arg5: Type
) -> ReturnType

// goes to block indent, one arg per line
fn a_very_long_function_name(
    arg1: Type, 
    arg2: Type, 
    arg3: Type, 
    arg4: Type, 
    arg5: Type,
    arg6: Type,
    arg7: Type,
) -> ReturnType
@joshtriplett
Member

@aturon I'd prefer that as well.

(Though I'd still put the trailing comma on the last item, even on one line, for consistency.)

@aturon
aturon commented Nov 22, 2016

@joshtriplett Ah yes, I would too, if I were paying more attention :)

@aturon
aturon commented Nov 22, 2016

To elaborate a bit, it's maybe good to contrast block and visual indent for the "in between" case:

// possible visual indent
fn a_very_long_function_name(arg1: Type, arg2: Type, arg3: Type, 
                             arg4: Type, arg5: Type) -> ReturnType


// possible block indent
fn a_very_long_function_name(
    arg1: Type, arg2: Type, arg3: Type, arg4: Type, arg5: Type,
) -> ReturnType

Some observations:

  • Block indent is one line longer.
  • Block indent emphasizes the structure a bit more, with clear separation between the function name, the arguments, and the return type.
  • Visual indent encourages left-to-right reading of the entire signature.
  • Visual indent emphasizes the function name in a long signature, by leaving whitespace around it in subsequent lines.

TLDR, both versions have some amount of "clutter"; with visual indent, that clutter is pushed to the right, making the function name stand out; with block indent, the clutter is spread evenly among different parts of the signature. At least, that's my take as someone very used to visual indent here. This is yet another reason I think it's vital to have automated support for the various proposals before reaching a final verdict, so that it's easy to spend time with real code under different stylings.

@nrc
Collaborator
nrc commented Nov 22, 2016 edited

re the 'intermediate' layouts, I dislike using these at all. My feeling is that we should not think about this in terms of using available space, but in terms of readability. In that sense, I think that when there are only a few arguments, then one line is easy to read, e.g.,

fn foo(x: u32, y: u32) -> u32

fn bar(name: String, job: Option<Profession>, age: u32)

But, once you get into many arguments, then putting one argument per line makes for much easier reading - you can find all the arguments at a glance, without searching for commas, you can distinguish arg names from types, and it is hard to miss an argument.

So, I think that in terms of readability, one line is optimal for 'small' functions, and one arg per line is optimal for 'large' functions. In my reading, then whether a function fits on one line or not is the heuristic for whether a function is 'small' or 'large', and once we don't fit on one line, then I prefer one arg per line, not because we have to in order to fit in the space, but because it is more readable. Thus I don't ever like to use, say 5 five args over two lines (as above) (obvs, there are some exceptions where there is some kind of semantics in the argument layout, but Rustfmt can never help there anyway).

All the above is independent of block or visual layout.

@aturon
aturon commented Nov 22, 2016

@nrc I'm certainly not making an argument on the basis of space usage alone. But I do think that "exploding" a parameter list onto separate lines can hurt readability, by making it more difficult to see the body of the function (or surrounding functions) at the same time as the signature. That is, space affects large-scale readability by determining how much you can see together.

you can find all the arguments at a glance, without searching for commas, you can distinguish arg names from types, and it is hard to miss an argument.

I'm not sure what you mean by "find all the arguments at a glance", but the rest of these concerns seem to apply equally whether the one line of arguments are together with the function name or on a line of their own. I'll also note that most of what you're mentioning here is already signaled in at least two other ways: capitalization/syntax and (in most editors) color.

It seems like you're arguing that there is a readability cliff happening in terms of how many arguments are on a single line -- since that's the primary thing that putting them on a separate line affects. What is the cutoff? And what about cases like

fn short(arg1: Type, arg2: Type, arg3: Type, arg4: Type, arg5: Type)

that allow you to pack a larger number of arguments by dint of a short function name?

@aturon
aturon commented Nov 22, 2016 edited

@nrc

But, once you get into many arguments, then putting one argument per line makes for much easier reading

Thinking a bit more, I do agree with this sentiment in at least one way: when compared to the "packed visual indent" style like:

fn a_very_long_function_name(arg1: Type, arg2: Type, arg3: Type, 
                             arg4: Type, arg5: Type) -> ReturnType

While this particular example isn't so bad, I've seen cases where the combination of rightward drift and long type names makes this work very poorly -- you end up getting 3-4 lines with 2-3 arguments each, at which point going ahead and putting things on separate lines does make it easier to parse quickly, even if it means you see less on the screen as a whole.

The key point, to me, was to mention the possibility of block indent with arguments on a single, standalone line. I think this is an interesting midpoint because it retains your ability to view the function body/surrounding context at the same time as the signature, avoids the "cliff" @alexcrichton mentioned, but (IMO) keeps the arguments themselves as readable as in the all-on-one-line version.

@chriskrycho

FWIW, I've been using the block-indent with arguments on a single, standalone line style in various C, C++, JavaScript, and Rust codebases over the last 7 or so years, and it really does make for a nice balance. It has solved most of the times I otherwise would have needed to break it up into multiple lines of arguments.

@alexcrichton
Member

@aturon yeah you've definitely got a good point that we have a possibility of gradients of formatting for various functions widths, avoiding an immediately cliff where when you go one character over the limit you're 10 lines longer.

I also definitely echo the sentiment that reading real formatted code is so very helpful here. My experience is slightly different than @nrc's experience where functions tend to fit into a large or small category at least, I find myself with lots of "medium" functions and hoping to retain the style of a "small" function signature at least (but maybe my code is weird!). I'm sure though that looking at a bunch of formatted code would be super helpful here.

One thing I'll also mention is that signatures like the visually indented one here I've come to avoid in time. I agree with @nrc that it's difficult at a glance to see how many arguments are passed (as the number varies per line) and often it's nice to have that info.

@nrc nrc added the I-nominated label Dec 18, 2016
@nrc
Collaborator
nrc commented Dec 18, 2016

Nominating for discussion at the style meeting where I hope we can go to FCP.

@nrc nrc removed the I-nominated label Dec 20, 2016
@nrc
Collaborator
nrc commented Dec 20, 2016

We discussed this at the style team meeting today and agreed it was ready for FCP.

Proposal

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, following option 1 from #39 (comment). 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. However, the style team felt that they could live with formatting of the following form, if there was overwhelming support:

fn foo(
    arg1: Type1, arg2:Type2, ...,
) {
    ...
}

I will create some larger examples of the formatting rules for easy comparison soon...

Summary of thread

Given the above summary, I won't endeavour to summarise the entire thread.

Of the proposals that have some support, there have been two large questions:

  • when a signature requires more than one line, should we use block or visual indent (the former has several variations)?
  • should we make use of intermediate layouts between fully horizontal and fully vertical?

In favour of block indent:

  • consistency with other formatting
  • less horizontal space
  • better diffs when the function names or generics change

In favour of visual indent

  • less vertical space
  • consistency with current Rust project formatting

People have argued that both styles have superior aesthetics and are easier to read.

In favour of intermediate layouts between horizontal and vertical:

  • absence of formatting cliffs
  • uses less vertical space

Against:

  • better diffs (due to quantum leaps)
  • simpler
  • arguably, easier to read larger function signatures when laid out vertically
@aturon
aturon commented Dec 21, 2016

I'd like to register my preference in favor of using the "intermediate" formatting mentioned. Like @alexcrichton, I frequently have function signatures that are just slightly too long, where the jump in vertical space would be a readability annoyance (since it means I can see less of the code at the same time). But maybe the examples @nrc is preparing will be illuminating.

@joshtriplett
Member

@aturon Note that we're still planning to advocate independently choosing to word-wrap function arguments, generic parameters, and where clauses. We're just (currently) leaning against breaking parameters onto their own line but still putting multiple parameters on one line.

I wouldn't object strongly to that particular intermediate step, though. I'd object a lot more strongly to having multiple lines with multiple parameters per line.

@nrc nrc added a commit that referenced this issue Dec 22, 2016
@nrc nrc Examples for #39
Each example is the same, except for how function signatures are formatted, the rest of the code is pretty similar to Rustfmt's current formatting, though I haven't actually run Rustfmt on it. I've removed some code so it will no longer compiler, though it should parse, so you should be able to run it through Rustfmt.

* fn-decl-visual.rs has visual indentation
* fn-decl-block.rs has 'all or nothing' block indentation
* fn-decl-block-progressive.rs has block indentation, and will put all args on one line if possible.
3c66946
@nrc
Collaborator
nrc commented Dec 22, 2016 edited

I made some examples in #48.

FWIW, none of the styles were trivial to apply manually, but visual indent was the easiest (since I use Sublime text and that aligns to an opening paren), I had to align the return type manually, however. The block indent examples I had to line up manually, but that was not really hard.

I still prefer the aesthetic of visual indent, but this is clearly subjective.

An interesting note - when using the progressive block style, we end up on one line in all but one case - that surprised me. I'm not sure whether that is good or bad overall. My feeling is that it is good for vertical space, but a little bad for readability and diffs.

One new observation for me is that block indent shifts the 'weight' of the signatures to the left more than I expected. For me, this made the signatures blend in more with the code. Whereas, with visual indent, the 'weight' of signatures is to the right, and of bodies to the left, that made scanning easier for me, especially using Sublime's fancy scroll bar thing (I'll get some screenshots actually, on reflection, it is more of a deal when just scrolling, so I suggest looking at the examples and scroll quickly as if searching for a function).

@Mark-Simulacrum

In a recent PR to Rust, I renamed a lifetime parameter ('blk -> 'a) and because many functions were visually indented, most of them had to be readjusted, which was a rather annoying and painful process. Any functions that I had previously either converted to block or progressive block indentation had extremely minimal required changes, and most of it was automated (i.e., search and replace was sufficient).

It looks like one of @nrc's arguments for visual indentation is easier to find functions in when scrolling. I don't think we should focus on the aspect of searching for a function, since most editors provide either 'go to' functionality (Sublime's cmd/ctrl-R, IIRC) or easy to use find functionality. More so, when personally scrolling through, I was unable to distinguish functions in any of the examples, really, mostly because once we start using any of these methods of wrapping function arguments I find that we reduce the function signature's "width" to far less than the average line in the function. It's also likely true that the more arguments a function has, the longer and more complicated the function itself, which usually makes each line somewhat longer as well.

I personally prefer either the progressive or full on block indent, and I think full on block indent is perhaps better. I think the progressive option needlessly obfuscates the function arguments since the usually feel "sandwiched" between the name and return type.

@softprops

Just wanted to mention I'm so excited block intention for paraeter lists having a prospect for becoming a standard. The line indent never felt right to me. The longer the method name the worse it looked. Scanning a file, block intended params always felt easier to read because my eyes dont have to readjust for horizontal regions for each new method.

@samlh
samlh commented Jan 5, 2017

My employer's coding style causes most functions to have a doc comment - as a side effect I never have problems finding functions, despite using all or nothing block indent. I was thinking the intermediate block indent could be an improvement, but after viewing the examples by @nrc (thank you!) I prefer all or nothing block indent for similar reasons as @Mark-Simulacrum.

@nrc
Collaborator
nrc commented Jan 10, 2017

Consensus seems to be 'all or nothing' block indent. We could probably end FCP, and I intend to do so in a few days. However, I'd like to give people a chance to comment (there haven't been many comments either way since announcing FCP, and this is an important issue), especially on the examples mentioned in #39 (comment)

@LukasKalbertodt

An interesting note - when using the progressive block style, we end up on one line in all but one case - that surprised me. I'm not sure whether that is good or bad overall. My feeling is that it is good for vertical space, but a little bad for readability and diffs.

It would hurt readability and diffs a lot, I think. Especially so, because there would be three ways to format function parameter (signatur in one line, progressive and one line per parameter) instead of only two (signature in one line and one line per parameter). I'd argue that it's a good thing to reduce the number of different formattings; for readability, refactoring and diffs.

@aturon
aturon commented Jan 11, 2017

@nrc Thanks for the examples, which are really helpful.

One question: the multi-line block indents keep the opening brace on the same line. I find this causes the function signature and body to run together (even with visual indent). Is that a settled question, or something that will be debated separately?

@nrc
Collaborator
nrc commented Jan 11, 2017

One question: the multi-line block indents keep the opening brace on the same line. I find this causes the function signature and body to run together (even with visual indent). Is that a settled question, or something that will be debated separately?

We should discuss this here.

Are you thinking about something like:

pub fn highlight<'a>(
    analysis: &'a AnalysisHost,
    project_path: &'a Path,
    file_name: String,
    file_text: String,
) -> String {
    debug!("highlight `{}` in `{}`", file_text, file_name);
    let sess = parse::ParseSess::new();

For me, the return type/opening brace line breaks this up kind of nicely (if anything the signature-on-one-line case has worse run on, but I feel that would be unpopular to change).

Do you prefer always using a newline? (We currently do this only if there is a where clause, and I suspect we will continue to do so there):

pub fn highlight<'a>(
    analysis: &'a AnalysisHost,
    project_path: &'a Path,
    file_name: String,
    file_text: String,
) -> String
{
    debug!("highlight `{}` in `{}`", file_text, file_name);
    let sess = parse::ParseSess::new();
@aturon
aturon commented Jan 11, 2017

@nrc Yes, that's the kind of example I had in mind.

Currently when I hand-format multi-line signatures, I always put the brace on its own line; I find it helpful to visually distinguish the signature from the body. OTOH, putting a brace on its own line is definitely uglier.

You may be right that the problem is particularly severe for where clauses. And it may well just be a matter of getting used to the style.

@joshtriplett
Member
joshtriplett commented Jan 11, 2017 edited

@aturon I find that the outdent for the ) { or ) -> ReturnType { provides a clear separation between the parameters and the body, preventing them from running together. (Similar to how the outdent for an } else { prevents the if body and else body from running together.)

@nrc
Collaborator
nrc commented Jan 18, 2017

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.

@aturon
aturon commented Jan 18, 2017

@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
Member
@ubsan ubsan closed this Jan 19, 2017
@ubsan ubsan reopened this Jan 19, 2017
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment