rust-lang / style-team

Home of the Rust style team
Apache License 2.0
453 stars 56 forks source link

Where clauses #38

Closed nrc closed 6 years ago

nrc commented 7 years ago

In functions, structs, enums, traits, impls, etc.

nrc commented 7 years ago

Several comments on where clauses, starting as https://github.com/rust-lang-nursery/fmt-rfcs/issues/30#issuecomment-258113982

joshtriplett commented 7 years ago

One note that came up about where regarding #8 in the rust-style meeting: some of the arguments for block indentation rather than visual indentation apply less to visual indentation by the width of a keyword. For instance, in the following:

fn f<T, U, V>{...)
    where T: ...,
          U: ...,
          V: ...
{
    ...
}

The visual alignment of T, U, and V causes less of a problem than other forms of visual indent, since the width of where won't change.

This is not intended as an argument for/against block/visual indentation, just an observation.

solson commented 7 years ago

This is not indented as an argument

I see what you did there.

withoutboats commented 7 years ago

Just wanted to inform y'all that I'm amending Rust RFC #1598 (associated type constructors) to include a syntax for higher rank types in which you attach a turbofish to the where keyword. If this is accepted I imagine it will impact the formatting choice you make here.

An example of the new syntax:

fn foo<F>() where::<'a> F: Fn(&'a T) { }
nrc commented 7 years ago

I'd like to propose (somewhat roughly) the following:

The example in https://github.com/rust-lang-nursery/fmt-rfcs/issues/38#issuecomment-260505823 satisfies these guidelines.

solson commented 7 years ago

I would include the trailing comma. You made a statement on another issue (I forget which) about including all trailing commas that are followed by newlines, and I would apply the same rule here.

joshtriplett commented 7 years ago

I feel conflicted about the trailing comma here. On the one hand, I initially wanted to suggest including the trailing comma as well for the multi-clause case. On the other hand, it feels really wrong for the single-clause case, even though all the same arguments apply to that case.

For other language constructs, switching from single-line to multi-line involves reformatting in other ways, due to surrounding punctuation. That doesn't apply here.

I think I may have put my finger on why it bothers me: I really disagree about putting each clause on its own line, in the case where all the clauses fit on one line. For instance, I prefer where I: IntoIterator<Item=T>, T: SomeTrait on one line, if it fits; I'd prefer only breaking it onto multiple lines if it doesn't fit.

I do agree with putting a newline before the opening { when you have a where clause.

I'll mildly argue against visual indent here, but only mildly, given the fixed-width observation I made in #38.

I'd assume, from your mention of visual indent, that when you say "each sub-clause on its own line", the first sub-clause shares the same line as the where?

One other observation: in the case where a function has no return value, I rather like this style:

fn function_name<T, U>(
    thing: T,
    uthing: U,
) where ...

However, that style does not naturally extend to the return-value case, where I'd prefer a newline before where. So, for consistency, I agree that we should always require a newline before where.

Finally, I'd suggest explicitly saying "a where clause should always start a new line, even when it would fit on the previous line". I don't care for fn shortfunc<T, U>(thing: T, uthing: U) where ... { even if it fits; if it fits that well, just put it directly in the generic parameters: <T: ..., U: ...>

nrc commented 7 years ago

I'd assume, from your mention of visual indent, that when you say "each sub-clause on its own line", the first sub-clause shares the same line as the where?

yes

One other observation: in the case where a function has no return value, I rather like this style:

I could live with this either way. I think it is fine to do this if there is no return type. Perhaps we should loo at some larger examples.

Finally, I'd suggest explicitly saying "a where clause should always start a new line, even when it would fit on the previous line". I don't care for fn shortfunc<T, U>(thing: T, uthing: U) where ... { even if it fits; if it fits that well, just put it directly in the generic parameters: <T: ..., U: ...>

agreed

joshtriplett commented 7 years ago

@nrc

I think it is fine to do this if there is no return type. Perhaps we should loo at some larger examples.

fn function_name<T>(
    param1: T,
    param2: Vec<T>,
) where T: Debug
{
    ...
}

fn function_name_2<I, T>(
    param1: I,
    param2: &mut Vec<T>,
) where I: IntoIterator<Item=T>
{
    ...
}
withoutboats commented 7 years ago

Over time I've come to prefer this style for where clauses:

item decl
where
    Type: Bound,
    Type: Bound,
{
    item body
}

I think this will also be better if we adopt the where-bound HRTB syntax, because the where statement could then be of variable length:

where<T: Bound>,
    Type: Bound<T>,
    Type: Bound<T>,

It seems rather excessive to indent the where keyword and then indent again the clauses.

nrc commented 7 years ago

Fleshing out @withoutboats 's suggestion:

fn function_name<T>(
    param1: T,
    param2: Vec<T>,
)
where
    T: Debug
{
    let x = foo();
    x
}

fn function_name<T>(param1: T, param2: Vec<T>)
where
    T: Debug
{
    let x = foo();
    x
}

fn function_name_2<I, T>(
    param1: I,
    param2: &mut Vec<T>,
)
where
    I: IntoIterator<Item=T>,
    T: Debug,
{
    let x = foo();
    x
}
nrc commented 7 years ago

And fleshing out @joshtriplett 's examples to explore the contrast with functions with a return type:

fn function_name<T>(param1: T, param2: Vec<T>)
    where T: Debug
{
    let x = foo();
    x
}

fn function_name<T>(
    param1: T,
    param2: Vec<T>,
) where T: Debug
{
    let x = foo();
    x
}

fn function_name<T>(
    param1: T,
    param2: Vec<T>,
) -> ReturnType<T>
    where T: Debug
{
    let x = foo();
    x
}

fn function_name_2<I, T>(
    param1: I,
    param2: &mut Vec<T>,
) where I: IntoIterator<Item=T>,
        T: Debug,
{
    let x = foo();
    x
}
nrc commented 7 years ago

I think I don't like having where clauses left indented - to me it makes it harder to scan the left margin for items. I'm also not a fan of the newline before the first clause, seems like a waste of vertical space for no gain.

I think I'm still neutral on same-line where clauses where there is no return type - I like the more condensed look and I don't think it harms readability. OTOH, it is another minor inconsistency and perhaps makes it harder to scan for where clauses (though I'm not sure I've every done that).

jplatte commented 7 years ago

I really like having where at the same indentation level as the function it belongs to. I'm also slightly in favor of always having a newline before the first clause, not just for consistency, but because I don't like how cramped together this looks:

fn function_name<T>(
    param1: T,
    param2: Vec<T>,
) -> ReturnType<T>
    where T: Debug
{
    let x = foo();
    x
}

// actually looks worse IMHO
fn function_name<T>(
    param1: T,
    param2: Vec<T>,
) -> ReturnType<T>
where T: Debug
{
    let x = foo();
    x
}
nrc commented 7 years ago

We started discussing at the meeting but did not reach a conclusion. The options we were discussing were:

fn foo(
    x,
    y,
    z,
) -> Bloop 
    where
        Foo,
        Bar,
        Baz,
{
}

fn foo(
    x,
    y,
    z,
) -> Bloop 
where
    Foo,
    Bar,
    Baz,
{
}

fn foo(
    x,
    y,
    z,
) -> Bloop 
    where Foo,
          Bar,
          Baz
{
}
nrc commented 7 years ago

The first two are block-indented where, the last is visual indented, the first is indented from the left, the second is not.

My preference is for the third.

nrc commented 7 years ago

We did at least agree that we should allow the where clause on the same line as ) if there is no return type.

joshtriplett commented 7 years ago

My preference is for the second: no extra level of indent for where, but an extra level of indent for its clauses (if they need breaking across multiple lines).

nrc commented 7 years ago

Oh, look at this (also from the meeting):

fn foo(x: i32, y: u32) where
    Foo,
    Bar,
{
}

I would prefer not to do this, keeping the where clause on its own line unless the args span multiple lines.

nrc commented 7 years ago

@joshtriplett the reason I dislike 2 is that for the single line where clause, you end up with a lot of left-indented code, which I find hard to scan:

fn foo(
    x,
    y,
    z,
) -> Bloop 
where Foo
{
    ...
}

I prefer having only the fn and { on the left - makes scanning the margin easer for me.

joshtriplett commented 7 years ago

@nrc I agree that putting the where on the same line as the ) should only happen in the multi-line no-return-value case, where the ) would otherwise appear on a line of its own.

Regarding the single-line case: hmmm, fair point. I find myself inclined towards indenting where for the single-line case, but I dislike doing so in the multi-line case. However, that seems wildly inconsistent.

nrc commented 7 years ago

I'd like to move forward here. The open questions I believe are:

Opinions?

solson commented 7 years ago

@nrc Why wouldn't we use trailing commas here? I would apply the same rule as elsewhere: include the trailing comma if it is followed by a newline.

joshtriplett commented 7 years ago

@solson ...unless the whole where clause fits on a single line. I don't think we should include the trailing comma when not splitting the where clause across multiple lines.

@nrc I would advocate for:

For that last one, consider simple cases like this:

fn f<T, U>(x: Vec<T>, y: U) -> usize
    where T: Ord, U: ?Sized

I don't think that where clause needs breaking across multiple lines.

nrc commented 7 years ago

@solson I'm having trouble explaining why we'd skip the commas here, the best I can do is similar to something Josh said higher up that it feels like the whole where clause is a kind of block, just without an actual brace, and with visual indent the invisible brace is on the same line:

where { foo }

where { foo,
        bar }

I think with block indent, the 'invisible brace' would be on the next line and we'd have a trailing comma.

More practically, if there is a single clause and it all fits on one line, then I think it looks a bit silly to have the trailing comma, and I want to be consistent about that:

where foo

where foo,
      bar

// c.f.,

where foo,

where foo,
      bar,
nrc commented 7 years ago

@solson and @joshtriplett

To clarify, I don't strongly oppose block indent (I feel a bit bad about the wasted vertical space, but it is more consistent with other stuff) and if we go for block indent then I would support a trailing comma.

nrc commented 7 years ago

@joshtriplett I pretty strongly oppose having single line where clauses with multiple sub-clauses, I think they can be very hard to read, e.g.,

fn function_name_2<I, T>(
    param1: I,
    param2: &mut Vec<T>,
) where I: IntoIterator<Item=T>, for<'a> T: FooBar<'a>, IntoIterator::Item: FooBar<'static>
{
    ...
}
nrc commented 7 years ago

c.f. with newlines:

fn function_name_2<I, T>(
    param1: I,
    param2: &mut Vec<T>,
) where
    I: IntoIterator<Item=T>,
    for<'a> T: FooBar<'a>,
    IntoIterator::Item: FooBar<'static>,
{
    ...
}
joshtriplett commented 7 years ago

@nrc Would this be a good case for "short"/"simple"? :)

nrc commented 7 years ago

Do you have examples of short/simple where clauses where you'd keep them on one line? For me, they always seem a bit punctuation heavy even in the simplest cases.

joshtriplett commented 7 years ago

@nrc I included an example in https://github.com/rust-lang-nursery/fmt-rfcs/issues/38#issuecomment-278741804

They seem no more punctuation-heavy than function declaration parameter lists.

nrc commented 7 years ago

Hmm, true, I would write

fn f<T: Ord, U: ?Sized>(x: Vec<T>, y: U) -> usize

but also

fn f<T, U>(x: Vec<T>, y: U) -> usize
    where T: Ord,
          U: ?Sized

Which I guess is inconsistent.

On the other hand, I'd argue that if a where clause is simple enough to be on one line then it should be bounds on the generics and not a where clause at all. IOW, the fact that a where clause exists implies that it passes (or fails, depending on your point of view) the short/simple test.

joshtriplett commented 7 years ago

Summarizing the consensus from the style meeting:

// With arguments fitting on one line:
fn function<T, U>(args)
where
    T: Bound,
    U: AnotherBound,
{
    body
}

// With arguments split onto their own lines, and no return type:
fn foo<T, U>(
    args,
) where
    T: Bound,
    U: AnotherBound,
{
    body
}

// With arguments split onto their own lines, and a return type:
fn foo<T, U>(
    args
) -> ReturnType
where
    T: Bound,
    U: AnotherBound,
{
    body
}

Note that we never put a bound (even a single bound) on the same line as where, or multiple bounds on the same line as each other. We always block-indent each bound. We never indent where on a line by itself. And each bound gets a trailing comma.

If bounds seem short and simple enough that this seems to add too much vertical space, consider putting them directly in the generics <> rather than in a separate where clause.

solson commented 7 years ago

In addition, the style @joshtriplett summarized avoids the potential future problem @withoutboats brought up regarding where<T: Bound> and visual alignment.

nrc commented 7 years ago

A note, I think if the whole item ends with a ; we should skip the trailing comma. E.g.,

fn foo<T, U>(
    args
) -> ReturnType
where
    T: Bound,
    U: AnotherBound;

not

where
    T: Bound,
    U: AnotherBound,;

An alternative would be to put the ; on its own line, but I think we never otherwise do this.

joshtriplett commented 7 years ago

@nrc Agreed, that looks like the right way to format standalone declarations without a function body.

nrc commented 7 years ago

This is now implemented in Rustfmt, you'll need where_style = "Rfc" in your rustfmt.toml (or see the rfc-rustfmt.toml file in the root of the rustfmt repo).

You can see what this looks like in practice at https://github.com/rust-lang-nursery/rustfmt/blob/master/tests/target/where-clause-rfc.rs

dotdash commented 7 years ago

Like @stebalien says here: https://users.rust-lang.org/t/new-style-guidelines/10037/16 I quite dislike the fact that where while being a child of the fn/impl still has the same indentation as the fn/impl itself instead of being indented further.

Stebalien commented 7 years ago

Specifically, I'm suggesting indenting where by two spaces:

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,
    }
}

One clear downside is that it introduces a "half" indent. One could indent where by four spaces (and then the constraints by another four) but that starts to get a bit deep:

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,
    }
}
nrc commented 7 years ago

Copying @joshtriplett's summary of some of the decisions from meetings:

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.

nrc commented 7 years ago

I would be amenable to using a full (but not half) indent for where. I see nothing bad about it. IIRC we thought it was not important to indent and left alignment would be fine. But, from the users thread there seems to be some objection to this, so using an indent seems fine to me.

I prefer to avoid half indents. I think they are kinda ugly, it makes working with custom indent widths more difficult, and it can confuse text editors (which try to infer the indent width).

U007D commented 7 years ago

I too am in support of the (full) indent for where. Since the where is a specification of types begun by the fn keyword, as a continuation, I would expect to see it indented, consistent with any other continuation--these are usually (always?) indented.

This greatly aids visual scanning of real code (as opposed to the trivial examples with one function consisting of single-character parameters names that are sometimes being shown here--those are too 'clean'--almost any reasonable format would look OK in that scenario).

Just my 2 cents, -Brad

jplatte commented 7 years ago

So if we had a full indent for where for the cases where it goes onto its own line, what happens in the case of a multiline parameter list and no return type? Does where still go on the same line as the ), and if so, how far indented are the clauses?

// 1. where on the same line, clauses indented once
fn complicated_function<'a, T, U>(
    first_long_argument_name: SomeType<T>,
    second_long_argument_name: AnotherType<U>,
) where
    T: SomeTrait<AssocType = u32>,
    U: Iterator<Item = &'a T>,
{
    …
}

// 2. where on the same line, clauses indented twice
fn complicated_function<'a, T, U>(
    first_long_argument_name: SomeType<T>,
    second_long_argument_name: AnotherType<U>,
) where
        T: SomeTrait<AssocType = u32>,
        U: Iterator<Item = &'a T>,
{
    …
}

// 3. where on new line, same as when there is a return type
fn complicated_function<'a, T, U>(
    first_long_argument_name: SomeType<T>,
    second_long_argument_name: AnotherType<U>,
)
    where
        T: SomeTrait<AssocType = u32>,
        U: Iterator<Item = &'a T>,
{
    …
}
rpjohnst commented 7 years ago

Was there a reason to put where on its own line only when the function has a return type? I like the single-intended clauses with a newline before the opening brace, but it seems inconsistent to have the opening delimiter (the where keyword) on its own line when all other opening delimiters are on the previous line.

For example, I would prefer this:

fn complicated_function<'a, T, U>(
    first_long_argument_name: SomeType<T>,
    second_long_argument_name: AnotherType<U>,
) -> ReturnType<T> where
    T: SomeTrait<AssocType = u32>,
    U: Iterator<Item = &'a T>,
{
    …
}
jplatte commented 7 years ago

when all other opening delimiters are on the previous line

That's not actually the case though. If you were to enforce that, the curly brace that starts the function body would have to be on the line of the last where clause in your example. And then it would be very hard to see where the clauses end and the function body begins.

Stebalien commented 7 years ago

Was there a reason to put where on its own line only when the function has a return type?

Yes, it makes it harder to distinguish between the return type and the where clause. All variants of the current proposal have short lines without any types separating out the where constraints block.


...Type // arg
...Type // arg
...Type // arg
(...Type) // return?
      -- no types --
...Type // constraint
...Type // constraint
      -- no types --
...code...
rpjohnst commented 7 years ago

That's not actually the case though.

I consider the opening curly brace also as the closing delimiter for the where clause, since there's no endwhere. If there were, I imagine it would go on the next line along with the opening curly brace.

All variants of the current proposal have short lines without any types separating out the where constraints block.

What makes this different from what the current proposal already does with short argument lists? Should the where also go on its own line here?

fn short<T, U>(x: T, y: U) -> i32 where
    T: TraitWithLongName,
    U: UraitWithLongName,
{
    ...
}
Stebalien commented 7 years ago

Should the where also go on its own line here?

According to the current proposal, yes.

joshtriplett commented 7 years ago

@nrc

I would be amenable to using a full (but not half) indent for where. I see nothing bad about it. IIRC we thought it was not important to indent and left alignment would be fine. But, from the users thread there seems to be some objection to this, so using an indent seems fine to me.

We did actually have a couple of reasons for not doing so. The biggest one: it would mean that either functions with return types and functions without return types would indent the where clauses differently (meaning that adding or removing a return type would require reindenting the entire where clause, an odd inconsistency), or that where clauses for functions with no return type would have a double-indent and an odd lack of overlap (because ) where is only 7 characters long and a double indent has 8 spaces):

// inconsistency:
) -> ReturnType
    where
        T: SomeComplicatedTrait<With, Parameters>,
{

// versus
) where
    T: SomeComplicatedTrait<With, Parameters>,
{

// or odd double-indentation
) where
        T: SomeComplicatedTrait<With, Parameters>,
{
emk commented 7 years ago

I'm really happy with the fmt RFCs overall! The decisions are not all always what I would have chosen, but they seem reasonable and I'm sure I'll get used to them.

My biggest concern is definitely the where formatting, and the amount of vertical space it uses. For example:

where
      S1: AsRef<str>,
      S2: Into<String>,
      P: AsRef<Path>,

Especially when the constraints are short, I would personally write this as:

where S1: AsRef<str>, S2: Into<String>, P: AsRef<Path>

This is by analogy to short parameter lists:

// We write:
fn foo(s1: AsRef<str>, s2: Into<String>, p: AsRef<Path>)

// And not:
fn foo(
    s1: AsRef<str>,
    s2: Into<String>,
    p: AsRef<Path>
)

As a compromise, this is reasonable:

where S1: AsRef<str>,
        S2: Into<String>,
        P: AsRef<Path>,

...but using block formatting will use an extra line of vertical whitespace, and it only gains two horizontal spaces.

I'm relatively concerned about vertical space, because my eyes aren't getting any younger, and I can only comfortably fit 52 vertical lines of code in my editor on a good laptop. And that's already a slightly unpleasantly small font if I'm going to stare at it for hours.

And there's a real benefit to being able to see lots of code at once, and maintaining a "global overview" of what a program is doing.