-
Notifications
You must be signed in to change notification settings - Fork 56
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
Comments
Several comments on where clauses, starting as #30 (comment) |
One note that came up about fn f<T, U, V>{...)
where T: ...,
U: ...,
V: ...
{
...
} The visual alignment of This is not intended as an argument for/against block/visual indentation, just an observation. |
I see what you did there. |
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 An example of the new syntax: fn foo<F>() where::<'a> F: Fn(&'a T) { } |
I'd like to propose (somewhat roughly) the following:
The example in #38 (comment) satisfies these guidelines. |
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. |
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 I do agree with putting a newline before the opening 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 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 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 |
yes
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.
agreed |
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>
{
...
} |
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<T: Bound>,
Type: Bound<T>,
Type: Bound<T>, It seems rather excessive to indent the where keyword and then indent again the clauses. |
Fleshing out @withoutboats 's suggestion:
|
And fleshing out @joshtriplett 's examples to explore the contrast with functions with a return type:
|
I think I don't like having I think I'm still neutral on same-line |
I really like having 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
} |
We started discussing at the meeting but did not reach a conclusion. The options we were discussing were:
|
The first two are block-indented My preference is for the third. |
We did at least agree that we should allow the |
My preference is for the second: no extra level of indent for |
Oh, look at this (also from the meeting):
I would prefer not to do this, keeping the where clause on its own line unless the args span multiple lines. |
@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:
I prefer having only the |
@nrc I agree that putting the Regarding the single-line case: hmmm, fair point. I find myself inclined towards indenting |
I'd like to move forward here. The open questions I believe are:
Opinions? |
@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. |
@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. |
@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
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:
|
@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. |
@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.,
|
c.f. with newlines:
|
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. |
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.
|
I consider the opening curly brace also as the closing delimiter for the where clause, since there's no
What makes this different from what the current proposal already does with short argument lists? Should the fn short<T, U>(x: T, y: U) -> i32 where
T: TraitWithLongName,
U: UraitWithLongName,
{
...
} |
According to the current proposal, yes. |
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 // inconsistency:
) -> ReturnType
where
T: SomeComplicatedTrait<With, Parameters>,
{
// versus
) where
T: SomeComplicatedTrait<With, Parameters>,
{
// or odd double-indentation
) where
T: SomeComplicatedTrait<With, Parameters>,
{ |
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
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. |
@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. |
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 |
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. |
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 |
@QuietMisdreavus Interesting. I think I'd ideally write it all on one line without a where clause ( |
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. |
Also should mention that the where clauses on type aliases don't do anything right now. |
The only alternative I see is indenting all the following lines:
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. |
…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
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
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 fn foo<T: Bound>(arg0: T) -> ReturnType {
body
} |
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. |
…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
In functions, structs, enums, traits, impls, etc.
The text was updated successfully, but these errors were encountered: