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

"possibly missing a comma here" conflicts with rustfmt #3396

Closed
nirvdrum opened this issue Nov 2, 2018 · 2 comments
Closed

"possibly missing a comma here" conflicts with rustfmt #3396

nirvdrum opened this issue Nov 2, 2018 · 2 comments

Comments

@nirvdrum
Copy link

nirvdrum commented Nov 2, 2018

I'm currently encountering a lint error for "possibly missing a comma" from code that rustfmt produced. This is similar to the issue described in #3244, but since that issue got into discussion about specifics about how the code sample in the issue was formatted, I thought I'd open a new issue with a very different code sample.

The code in question is a precedence climber for the pest parsing library:

fn create_precedence_climber() -> PrecClimber<Rule> {
    PrecClimber::new(vec![
        Operator::new(Rule::or, Assoc::Left),
        Operator::new(Rule::and, Assoc::Left),
        Operator::new(Rule::equal, Assoc::Right)
            | Operator::new(Rule::not_equal, Assoc::Right)
            | Operator::new(Rule::less_than_equal, Assoc::Right)
            | Operator::new(Rule::less_than, Assoc::Right)
            | Operator::new(Rule::greater_than_equal, Assoc::Right)
            | Operator::new(Rule::greater_than, Assoc::Right),
        Operator::new(Rule::add, Assoc::Left) | Operator::new(Rule::subtract, Assoc::Left),
        Operator::new(Rule::multiply, Assoc::Left) | Operator::new(Rule::divide, Assoc::Left),
    ])
}

The error is:

error: possibly missing a comma here                                                                                                                                                                                                                                                      
  --> src/parser.rs:51:68                                                                                                                                                                                                                                                                 
   |                                                                                                                                                                                                                                                                                      
51 |             | Operator::new(Rule::greater_than_equal, Assoc::Right)                                                                                                                                                                                                                  
   |                                                                    ^      

I can make the error go away by moving the pipe to the of the line, like so:

fn create_precedence_climber() -> PrecClimber<Rule> {
    PrecClimber::new(vec![
        Operator::new(Rule::or, Assoc::Left),
        Operator::new(Rule::and, Assoc::Left),
        Operator::new(Rule::equal, Assoc::Right) |
            Operator::new(Rule::not_equal, Assoc::Right) |
            Operator::new(Rule::less_than_equal, Assoc::Right) |
            Operator::new(Rule::less_than, Assoc::Right) |
            Operator::new(Rule::greater_than_equal, Assoc::Right) |
            Operator::new(Rule::greater_than, Assoc::Right),
        Operator::new(Rule::add, Assoc::Left) | Operator::new(Rule::subtract, Assoc::Left),
        Operator::new(Rule::multiply, Assoc::Left) | Operator::new(Rule::divide, Assoc::Left),
    ])
}

I personally think the code produced by rustfmt looks nice, but I can see the appeal of using a trailing operator to indicate a multiline expression. In either case, clippy and rustfmt need to come to an agreement over which is correct.

@ghost
Copy link

ghost commented Nov 4, 2018

Maybe we shouldn't lint if the second line has extra indentation.
So trigger on

vec![
1
- 2,
]

but not on

vec![
1
    - 2,
]

The spacing between the operator and the second operand might also be clue. Unary operators usually don't have spaces before the operand.

bors bot added a commit that referenced this issue Nov 7, 2018
3407: Fix `possible_missing_comma` false positives r=oli-obk a=mikerite

`possible_missing_comma` should only trigger when the binary operator has
unary equivalent. Otherwise, it's not possible to insert a comma without
breaking compilation. The operators identified were `+`, `&`, `*` and `-`.

This fixes the specific examples given in issues #3244 and #3396
but doesn't address the conflict this lint has with the style of starting
a line with a binary operator.

Co-authored-by: Michael Wright <[email protected]>
@basil-cow
Copy link
Contributor

Fixed by this pr, can be closed @flip1995

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

No branches or pull requests

3 participants