r-lib / lintr

Static Code Analysis for R
https://lintr.r-lib.org
Other
1.2k stars 187 forks source link

New linter to suggest parentheses to clarify boolean expressions? #1832

Open IndrajeetPatil opened 1 year ago

IndrajeetPatil commented 1 year ago

Parentheses are cheap, and they can radically improve readability of complex conditional expressions. It would be a great if we have a linter that suggests doing so in apt contexts.

# actual
if (a <= b && c == d || !okay) {
  ...
}

# suggested
if (((a <= b) && (c == d)) || (!okay)) { # `|| !okay) {` is also fine here
  ...
}
AshesITR commented 1 year ago

Maybe more generally mixing of different binary operators without parentheses? We should allow common cases like %>%, |>, %+% by default, but generally discourage relying on operator precedence. This would also lint e.g. 1:5 + 1

MichaelChirico commented 1 year ago

There's a lot of overlap with #1566 -- is it the same?

Personally, "parentheses are cheap" rings pretty false. Keeping track of matching parentheses is notoriously difficult -- we either have to keep a mental cache of the paren stack, or rely on clicking about / IDE nudges to clarify.

In your example, either is fine for me:

if ((a <= b && c == d) || !okay) { ... }
# maybe a bit excessive in this simple case, but better if the expressions are longer
if (
  (
    a <= b &&
      c == d
  ) ||
  !okay
) { ... }

PS, even %>% might generate some lints. I have seen a lot of code like:

DF %>%
  dplyr_verbs() %>%
  ggplot() +
  ggplot_grammar() %>%
  print()

That print() is not doing what you might think at first glance and it can be a pain to debug.

IndrajeetPatil commented 1 year ago

There's a lot of overlap with https://github.com/r-lib/lintr/issues/1566 -- is it the same?

I was just going to comment that. I think these two can be collapsed to one (e.g.) implicit_precedence_linter()?

"parentheses are cheap" rings pretty false

Sorry for the confusion. I meant performance-wise; i.e., one shouldn't have to worry that using them profusely is going to slow down the program.

Keeping track of matching parentheses is notoriously difficult

Fair enough, but it is nowhere near as difficult as creating your own parentheses mentally by taking precedence into account and then matching these invisible parentheses. Also, thankfully, many IDEs (VS Code, RStudio, etc.) support rainbow parentheses, which does make this task slightly easier.


Just to clarify: I think the lint message should only mention that the user can benefit from using more parentheses for the expression in question. It shouldn't suggest how many or where these should be placed, because this will be hard to decide statically.