boolean and arithmetic expressions #18

Open
nrc opened this Issue Oct 3, 2016 · 12 comments

Projects

None yet

4 participants

@nrc
Collaborator
nrc commented Oct 3, 2016

Including parenthesised expressions.

@nrc nrc referenced this issue Oct 3, 2016
Open

Expressions #16

0 of 3 tasks complete
@nrc nrc added the P-high label Oct 3, 2016
@steveklabnik
steveklabnik commented Oct 7, 2016 edited

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

@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
Member
solson commented Oct 7, 2016 edited

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

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
Member
joshtriplett commented Oct 8, 2016 edited

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

@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
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
Collaborator
nrc commented Oct 25, 2016

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

@nrc
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))

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