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

boolean and arithmetic expressions #18

Closed
nrc opened this Issue Oct 3, 2016 · 14 comments

Comments

Projects
None yet
4 participants
@nrc
Collaborator

nrc commented Oct 3, 2016

Including parenthesised expressions.

@nrc nrc referenced this issue Oct 3, 2016

Closed

Expressions #16

13 of 14 tasks complete

@nrc nrc added the P-high label Oct 3, 2016

@steveklabnik

This comment has been minimized.

steveklabnik commented Oct 7, 2016

I personally almost always use parens, even if not strictly needed:

(a || b) || c

It's easier than trying to remember specific precedence rules.

@solson

This comment has been minimized.

Contributor

solson commented Oct 7, 2016

I agree with @steveklabnik, but I think parenthesizing around uses of the exact same operator is over the top.

@steveklabnik

This comment has been minimized.

steveklabnik commented Oct 7, 2016

@solson once you use both right and left associative operators that aren't necessarily commutative....

I agree that it's a bit overly conservative though. I have scars.

@solson

This comment has been minimized.

Contributor

solson commented Oct 7, 2016

I was thinking of specifically making the point that Rust has few or possibly no right-associative operators at all, but I'm not sure if that's true. The reference doesn't seem to make any mention.

Here are a few of my other thoughts on the matter:

// I don't write (x * y) + z because I learned this precedence
// repeatedly in primary/secondary school
x * y + z

// I only learned this precedence in post-secondary, so I don't
// expect it to be widely known and I parenthesize
(x && y) || z
(x & y) | z

// When it comes to the full set of bitops, I don't trust myself or
// people reading my code to get it right, so I parenthesize
(x ^ y) | ((z & w) << 8)

// When it comes to mixing ops from different "families" I parenthesize
// for the same reason
x * (y | 8)

// Special case of the above: I don't write x == (y + z) because
// everyone knows this precedence
x == y + z

It may be a bit complex, but I think it's worth having fine-grained rules for operator parentheses in rustfmt. The last case makes me think that even a simple paren-heavy rule will need a number of special cases.

@steveklabnik

This comment has been minimized.

steveklabnik commented Oct 7, 2016

I was thinking of specifically making the point that Rust has few or possibly no right-associative operators at all, but I'm not sure if that's true.

I don't think it does, but I mean that this is something that's come out of using multiple languages: I can't remember if Rust does or not. So to be safe, I (possibly-over) use parens.

I do think that your rules seem reasonable.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Oct 8, 2016

I tend to parenthesize any subexpression that uses a different operator than the next expression up in the parse tree. I do rely on associativity, though, with chains of the same operator.

I don't think rustfmt should handle whether or not you use parentheses, though. (rustc potentially should have warnings about cases that should use parentheses, such as commonly confused operator precedences.)

This issue should, though, discuss how to format such expressions with spaces and newlines.

General principles I'd tend to follow (some of which the style guide already covers, but I've included them to paint a complete picture):

  • Put spaces around every binary operator.
  • In a range expression, don't put spaces around the .. or ... operator. Use parentheses around non-literal expressions used as operands, or around literal operands that contain a . (such as 2.0).
  • Don't use spaces to indicate precedence; if precedence feels ambiguous, use parentheses.
  • Don't parenthesize the argument to return, for, if, while, or match, unless you need to break a line in the middle of that expression.
  • Don't put spaces or line breaks between a unary operator and its operand, except for &mut. Put a space between the &mut operator and its operand.
  • When using the indexing operator [], don't put a space between the indexed object and the opening [, between the opening [ and the index expression, or between the index expression and the closing ]. Don't parenthesize the index expression.
  • If you need to break a line in the middle of an expression, break it right before a binary operator, placing the binary operator at the start of the next line. Indent that continuation line one step (four spaces) further than the line it continues (unless some other rule says to indent it further).
  • Don't use curly braces around a single expression; only use curly braces around a series of statements that includes at least one semicolon.

Since function calls can appear in expressions, and expressions can appear in the arguments of function calls, the rules for expressions and function calls will interact.

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 9, 2016

I mostly agree with @solson, except that I would probably parenthesise x * y + z, for uniformity if nothing else. I wouldn't parenthesise multiple uses of the same operator.

@joshtriplett

Use parentheses around non-literal expressions used as operands, or around literal operands that contain a . (such as 2.0).

Could you give some examples of this? Presumably you don't mean (1.0) + (2.0)?

Don't parenthesize the argument to return, for, if, while, or match, unless you need to break a line in the middle of that expression.

Why does the linebreak make a difference?

If you need to break a line in the middle of an expression, break it right before a binary operator, placing the binary operator at the start of the next line. Indent that continuation line one step (four spaces) further than the line it continues (unless some other rule says to indent it further).

I prefer breaking after the binop, I'm having a hard time justifying why though - perhaps uniformity with commas and semicolons?

I find that indentation rule works well for 'top-level' expressions, but perhaps not for nested ones. Maybe we should punt on some of the details (e.g., to control flow and function call RFCs), but for example:

// This is odd-looking and hard to read
if foo(x || y ||
    z) {
    ...
}

// New-lining the { makes it easier to read
if foo(x || y ||
    z)
{
    ...
}

// Visual indent looks better, I think
if foo(x || y ||
       z) {
    ...
}

// But still needs the newline { in some cases
if (x || y ||
    z) && a
{
    ...
}

// Not sure how I feel about block indent for function calls, etc.
a_long_funcion(thing + thing2 +
    thing3);
// And how does it work with multiple args (assuming block indent)
a_long_funcion(thing + thing2 +
    thing3,
    thing4);
// or
a_long_funcion(thing + thing2 +
        thing3,
    thing4);

I assume that we should try and fit as many sub-expression on a line as posisble, e.g.,

thing1 + thing2 + thing3
    + thing4 + thing5

// not
thing1 + thing2 + thing3
    + thing4
    + thing5

I suppose also that we should prefer breaking at parens rather than inside parens.

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 9, 2016

I don't think rustfmt should handle whether or not you use parentheses, though. (rustc potentially should have warnings about cases that should use parentheses, such as commonly confused operator precedences.)

Rustfmt currently does change parenthesisation, but only in some cases, I think. In particular, it removes parens in control flow statements where unnecessary and I think that is the right thing to do. In general, I view parens as significant whitespace - they don't show up in the AST (although technically this is not true in Rust), and so Rustfmt should be free to mess with them. Are there concrete examples of where we wouldn't want that to happen? And are such cases common enough that that should be the default behaviour?

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Oct 9, 2016

@nrc

Use parentheses around non-literal expressions used as operands, or around literal operands that contain a . (such as 2.0).

Could you give some examples of this? Presumably you don't mean (1.0) + (2.0)?

That sentence appeared specifically in a point talking about the range operator (..). Consider (1.0)..(4.0) (assuming you had an implementation of std::iter::Step), or something..(something_else + 12).

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 9, 2016

That sentence appeared specifically in a point talking about the range operator (..). Consider (1.0)..(4.0) (assuming you had an implementation of std::iter::Step), or something..(something_else + 12).

Thanks for the explanation, that makes sense, I think. I probably wouldn't use parens for 1.0..2.0 or even e.f..e.g but I def would for a binop, etc.

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 25, 2016

See also #34 (comment) and discussion leading up to it.

@nrc

This comment has been minimized.

Collaborator

nrc commented Nov 29, 2016

Given we came to favour block indent for expressions elsewhere, I think this issue is ready for FCP. Most rules are given in #18 (comment), with a few others scattered around in other comments. The only remaining open questions are: whether Rustfmt should insert or remove parens (Josh thinks not, see previous link, I think we should, see #18 (comment), but it should be rare in any case (i.e., Rustfmt only makes such a change in extreme cases, not as a matter of course)). And, if we do split a line at a binop, should the operator go before or after the linebreak. (See also #34 (comment))

@nrc

This comment has been minimized.

Collaborator

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.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented May 15, 2017

This was discussed in issue #34 (#34 (comment)), but I'll copy it here because it's relevant to binops in general:

Commas and semicolons should both fade into the background as simple terminators.

Regarding other binops: I find that putting a binop at the end of the previous line makes it easier to get lost, and harder to construct the tree in your head, especially when you break an expression in multiple places. If I look at something like this:

if really_complicated_expression
   || another_complicated_expression
   || yet_another_complicated_expression

(where the complicated expressions themselves have piles of operators and calls in them), having the || at the start of the next line makes the tree of operations more obvious. This has nothing to do with diffs; I consider it a standalone readability issue. I don't want those operators to fade into the background the way commas do; I want them in the foreground.

Typically, a comma or semicolon has importance because of its presence or absence, but for most binary operators, I need to know which operator (such as && versus ||).

@nrc nrc referenced this issue Aug 4, 2017

Closed

Tracking issue: write the style guide #89

29 of 29 tasks complete

@topecongiro topecongiro referenced this issue Sep 15, 2017

Merged

RFC: binop #1964

@nrc nrc added has-PR and removed P-high ready-for-PR labels Dec 7, 2017

nrc added a commit that referenced this issue Dec 7, 2017

Binary ops
Closes #18

@nrc nrc closed this in 3806502 Feb 5, 2018

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