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

blocks #21

Closed
nrc opened this issue Oct 3, 2016 · 20 comments
Closed

blocks #21

nrc opened this issue Oct 3, 2016 · 20 comments
Labels

Comments

@nrc
Copy link
Member

nrc commented Oct 3, 2016

Simple blocks, { ... }.

@nrc nrc mentioned this issue Oct 3, 2016
14 tasks
@nrc nrc added the P-high label Oct 3, 2016
@nrc
Copy link
Member Author

nrc commented Oct 12, 2016

Here are things I think need to be covered:

  • layout of blocks - newlines and indentation (including unsafe keyword)
  • attributes and comments
  • single line blocks
  • nesting blocks in expressions, and how to indent (though we might need to punt on this until we have the expressions themselves spec'ed - I imagine whether we block or visual indent an expression would affect how we layout a block nested inside it. We might at least spec blocks on the rhs of let statements.

Single line blocks are probably the most interesting aspect. Rustfmt currently only allows these for simple unsafe blocks, e.g.,

fn main() {
    let _ = unsafe { foo() };
}

IIRC, Rustfmt's definition of simple is:

  • a single expression (no statements)
  • no comments
  • short (width is under some threshold)
  • fits in the available space

@nrc
Copy link
Member Author

nrc commented Oct 12, 2016

and

  • empty blocks - {} vs { }

@strega-nil
Copy link

Empty blocks should be {}, imo, like empty parens:

fn foo() {}

@nrc
Copy link
Member Author

nrc commented Oct 12, 2016

Yeah, +1, Rustfmt currently does {}

@jared-mackey
Copy link

I am in favor of short and simple ones to be on one line no matter what it contains. Less than 20-25 chars.

Empty should definitely be {}.

@joshtriplett
Copy link
Member

+1 for {}.

Personally, I would recommend always using one-line blocks as long as 1) the contents are expressions, not statements, and 2) the entire result fits on a line. The moment the line goes over the threshold you feel comfortable with, the block is the very first thing to get line breaks. And any block containing a statement should get newlines and indentation.

So, in particular, I'd write:

let x = if cond { f() } else { g() };
let y = h(if cond { f() } else { g() });
let z = unsafe { u.field };

@nrc
Copy link
Member Author

nrc commented Oct 13, 2016

@joshtriplett I'm specifically talking about bare blocks here, not those as part of if or other expressions/items. So while I agree with all three of you examples, I'm only really discussing the last one. I find the following weird:

...
{ u.field }
...

basically and want to prevent it.

I'm not sure how I feel about

let z = { u.field };

I'm not actually sure that either of those should ever happen - I don't think there is a use case, but I might be wrong.

Oh also, I don't like

unsafe { u.field }

where the result is not used - although I'm not actually sure if Rustfmt can know about that.

@strega-nil
Copy link

strega-nil commented Oct 17, 2016

@nrc so a block in expression position may be one-line, but one in statement position must not be? I like that, personally.

@nrc
Copy link
Member Author

nrc commented Oct 17, 2016

@ubsan yes

@joshtriplett
Copy link
Member

@ubsan Exactly.

@joshtriplett joshtriplett mentioned this issue Oct 25, 2016
@nrc
Copy link
Member Author

nrc commented Oct 25, 2016

Some of the basics which haven't been covered:

Non simple blocks should look like:

// Comment
#[attribute]
{
    // Inner comment
    #![inner_attr]
    expr1;
    expr2;
    expr3
}

Avoid comments or attributes on the same lines as the braces.

With a let, we indent once:

let x = {
    expr1;
    expr2;
    expr3
};

With these definitions and the sum of discussion in the thread, I think this issue is ready for FCP.

@gsingh93
Copy link

Should we specify whether spaces are required surrounding the statement in a single line block? i.e. { foo() } instead of {foo()}? My vote is that they should be required.

@briansmith
Copy link

the entire result fits on a line

I agree that this should be the threshold and in particular there shouldn't be a separate threshold independent of the line length.

@nrc
Copy link
Member Author

nrc commented Nov 6, 2016

It seems to me we have rough consensus and thus we are ready for a PR here.

@jplatte
Copy link

jplatte commented Nov 7, 2016

Empty blocks should be {}, imo, like empty parens

Not a big thing, but I'd actually argue that to be consistent you'd have a space between the set braces if you also have spaces in single-line blocks. E.g. if you have unsafe { foo() } instead of unsafe {foo()} then you should also have { } instead of {}.

@DanielKeep
Copy link

I don't think there is a use case, but I might be wrong.

{ borrow } is useful for forcing Rust to move a mutable borrow, rather than re-borrow it. The alternative is to go off and define a fn move<T>(v: T) -> T { v } and use th... wait, no, move is a keyword. Uh... move_? id? dont_reborrow? Urgh. ... and that's why I like { borrow } :)

@strega-nil
Copy link

@jplatte I'm personally okay with either, and can see the arguments for both. I'm not sure we should decide this now, but in it's own issue.

@solson
Copy link
Member

solson commented Nov 9, 2016

@jplatte I'd rather be inconsistent and have {}. Although it's not really inconsistent if you think of the whitespace as a collapsing margin attached to the elements. :)

{} is the current majority community norm, as far as I know.

@solson
Copy link
Member

solson commented Nov 9, 2016

@ubsan The {}/{ } question seems in scope here to me (but I admit it's a minor point).

@joshtriplett joshtriplett mentioned this issue Jan 10, 2017
@nrc
Copy link
Member Author

nrc commented Mar 14, 2017

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

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

nrc added a commit that referenced this issue Aug 28, 2017
closes #21
cc #89
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants