Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Where clauses #38

Closed
nrc opened this issue Nov 6, 2016 · 60 comments
Closed

Where clauses #38

nrc opened this issue Nov 6, 2016 · 60 comments

Comments

@nrc
Copy link
Member

nrc commented Nov 6, 2016

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

@nrc
Copy link
Member Author

nrc commented Nov 6, 2016

Several comments on where clauses, starting as #30 (comment)

@joshtriplett
Copy link
Member

joshtriplett commented Nov 15, 2016

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
Copy link
Member

solson commented Nov 15, 2016

This is not indented as an argument

I see what you did there.

@withoutboats
Copy link

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
Copy link
Member Author

nrc commented Jan 10, 2017

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

  • a where clause should always start a new line and be block indented one level deeper than its 'parent'
  • if there is more than one sub-clause, then put each sub-clause on its own line. Sub-clauses should be visually indented.
  • there is no trailing comma (I'm not sure about this, I've just never done it myself, perhaps there should be one)
  • when a where clause is followed by a block, the opening brace is put on a new line and is block indented to the same depth as the where clause's parent
  • within sub-clauses, format as bounds elsewhere (is this all defined in the guide alread?), in particular, spaces around = and +, and only after :.

The example in #38 (comment) satisfies these guidelines.

@solson
Copy link
Member

solson commented Jan 10, 2017

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
Copy link
Member

joshtriplett commented Jan 10, 2017

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
Copy link
Member Author

nrc commented Jan 10, 2017

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
Copy link
Member

@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
Copy link

withoutboats commented Jan 15, 2017

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
Copy link
Member Author

nrc commented Jan 18, 2017

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
Copy link
Member Author

nrc commented Jan 18, 2017

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
Copy link
Member Author

nrc commented Jan 18, 2017

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
Copy link

jplatte commented Jan 18, 2017

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
Copy link
Member Author

nrc commented Jan 24, 2017

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
Copy link
Member Author

nrc commented Jan 24, 2017

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
Copy link
Member Author

nrc commented Jan 24, 2017

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

@joshtriplett
Copy link
Member

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
Copy link
Member Author

nrc commented Jan 24, 2017

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
Copy link
Member Author

nrc commented Jan 24, 2017

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

@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
Copy link
Member Author

nrc commented Feb 9, 2017

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

  • choose the overall formatting (visual vs block vs block indented, see Where clauses #38 (comment). My preference is visual)
  • whether where clauses should have a trailing comma (always, never, only if spanning multiple lines. My preference is never)
  • should we permit multiple clauses on one line (yes or no, I think no, I find this very hard to read)

Opinions?

@solson
Copy link
Member

solson commented Feb 9, 2017

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

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

  • block-indent
  • trailing comma only if spanning multiple lines
  • Allow multiple clauses on one line, only break across lines if needed.

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
Copy link
Member Author

nrc commented Feb 9, 2017

@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
Copy link
Member Author

nrc commented Feb 9, 2017

@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
Copy link
Member Author

nrc commented Feb 9, 2017

@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
Copy link
Member Author

nrc commented Feb 9, 2017

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

@rpjohnst
Copy link

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
Copy link

jplatte commented Mar 22, 2017

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
Copy link

Stebalien commented Mar 22, 2017

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
Copy link

rpjohnst commented Mar 22, 2017

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
Copy link

Should the where also go on its own line here?

According to the current proposal, yes.

@joshtriplett
Copy link
Member

joshtriplett commented Mar 23, 2017

@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
Copy link

emk commented Mar 23, 2017

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.

@joshtriplett
Copy link
Member

@emk I think the "short" case is the item we talked about longest. I do actually favor the formatting you suggest (putting all three of those on one line).

The conclusion we ended up coming to was that if the constraints are that short or simple, you could put them directly in the generics, which can go on one line if they fit. That works even better now that we use block indentation for arguments and don't try to align them after the open paren; a larger generics block won't push all the arguments off the right margin. Inline simple constraints in the generics; use where clauses for longer or more complex constraints.

@U007D
Copy link

U007D commented Mar 23, 2017

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?

IMHO, @jplatte When the where goes on its own line (as per the condition in your question), option #3.

@joshtriplett To keep things simple, I recommend avoiding ) where entirely; ie. when deciding to put it on a new line, actually put where on a new line (with a full indent).

@DorianListens
Copy link

I'd also be in favor of always putting where on its own line, with full indent. It's an important keyword, and it's important to separate where clauses from argument lists. I don't like the look of the where clause at the same indentation level as the function it acts upon, that seems like a very odd visual hierarchy mismatch.

@QuietMisdreavus
Copy link
Member

I'm currently updating rustdoc to conform to the newer style guidelines and I ran into a fun wrinkle: Type aliases can also have where clauses. Even better, they go between the alias name and the expanded form.

When written inline, a type alias with a where clause looks like this:

type TestAlias<T> where T: Copy = Vec<T>;

With the proposed formatting, it seems like it should be somewhat like this:

type FormattedAlias<T>
where
    T: Copy
= Vec<T>;

Is this considered proper? That = Vec<T> on its own line looks really awkward, but without changing the grammar to move the where clause after the expanded form (is that the right term?), that seems to be the way to go. It doesn't help that type aliases don't check their type constraints, either.

@joshtriplett
Copy link
Member

@QuietMisdreavus Interesting. I think I'd ideally write it all on one line without a where clause (type Something<T: Copy> = Vec<T>;). But in the case where you do want a where clause, that doesn't look like reasonable formatting to me. I don't see a better place to put the =, though; having it start a line seems better than hanging it off the same line as the last where clause. (Also, the where clauses should still have trailing commas when they appear on their own lines.)

@QuietMisdreavus
Copy link
Member

Right, I didn't want it to hang off of the last condition, for the same reason as people talking about the opening brace earlier in the thread. I'll let it work like that for now. (And my mistake on the commas; I haven't absorbed that one into my personal style so I forgot them when sketching out the example. >_>)

@withoutboats
Copy link

Also should mention that the where clauses on type aliases don't do anything right now.

@nrc
Copy link
Member Author

nrc commented Mar 29, 2017

The only alternative I see is indenting all the following lines:

type FormattedAlias<T>
    where
        T: Copy,
    = Vec<T>;

I think @QuietMisdreavus's suggestion fits our rules better, but having everything on the left margin is annoying.

I agree that getting rid of the where clause is better than both.

QuietMisdreavus added a commit to QuietMisdreavus/rust that referenced this issue Apr 1, 2017
bors added a commit to rust-lang/rust that referenced this issue Apr 9, 2017
…cxv,GuillaumeGomez

rustdoc: update formatting of fn signatures and where clauses to match style rfcs

Recent updates to style RFCs ([where clauses](rust-lang/style-team#38), [function definitions](rust-lang/style-team#39)) changed the "canonical" style for these items, so this is a rustdoc update to make it emit that style where necessary. This is mainly a conversion from visual indent to block indent, which helps out in situations where there was excessive indent causing lines to wrap regardless.

Samples:

![std::iter::IntoIterator](https://cloud.githubusercontent.com/assets/5217170/24712947/e586604c-19e9-11e7-87ae-4fe64d689dc3.png)

![excerpt from std::iter::Iterator](https://cloud.githubusercontent.com/assets/5217170/24713209/91e65112-19ea-11e7-9ff8-d4cf6b31aae1.png)

![std::iter::FromIterator](https://cloud.githubusercontent.com/assets/5217170/24713138/59f36114-19ea-11e7-9dbb-5f5ba7126e2e.png)

![std::cmp::min](https://cloud.githubusercontent.com/assets/5217170/24713038/1bab88b4-19ea-11e7-935d-defed5648de4.png)

![some trait impls on std::collections::HashMap](https://cloud.githubusercontent.com/assets/5217170/24713251/b7ef69e8-19ea-11e7-94a7-e01fbf89fa31.png)

![`fn extract_code_blocks`, an example given in #40687](https://cloud.githubusercontent.com/assets/5217170/24713159/672717cc-19ea-11e7-9acb-6ac278b90339.png)

![excerpt from itertools::Itertools](https://cloud.githubusercontent.com/assets/5217170/24713323/f06716ea-19ea-11e7-94cc-6ef68d9980ec.png)

fixes #41025 and #40687

r? @rust-lang/docs
@alteous
Copy link

alteous commented May 3, 2017

I think the following style (adapted from @joshtriplett 's post / @withoutboats ' suggestion) is best:

fn foo<'a, T, U>(
    arg0: &'a T,
    arg1: U,
) -> ReturnType
where
    T: Bound,
    U: AnotherBound,
{
    body
}

Rationale

  • Quick to parse mentally since the where keyword is a visual barrier between the parameter declarations and the type bounds.
  • Indentation is consistent every time.
  • Suitable for long return types.

There maybe a few extra newlines than absolutely necessary, but I think this is a very reasonable trade-off for the gains in readability.

Note that if there's only one type parameter then there's no need for the where clause in the first place:

fn foo<T: Bound>(arg0: T) -> ReturnType {
    body
}

@joshtriplett
Copy link
Member

We discussed this further in the style meeting, and #38 (comment) is still an accurate summary of the style we've got consensus around.

Regarding type aliases (as raised by @QuietMisdreavus), I don't think we have a complete consensus on those. We'd like to discuss type aliases and the possibility of single-line where clauses in #74. I think we have consensus that at a minimum, there should be an option available for that; we don't, however, have consensus on that being the default.

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

No branches or pull requests