delete if_else_match_braces_linter and merge it into brace_linter (#1095)

* delete if_else_match_braces_linter and merge it into brace_linter

* deprecate open_curly_linter and merge it into brace_linter (#1096)

* deprecate open_curly_linter

 - remove open_curly_linter from defaults
 - refactor to XPath based approach
 - no longer lint trailing whitespace (there's a separate linter for that)

* merge paren_brace_linter into brace_linter (#1097)

* deprecate paren_brace_linter

 - remove paren_brace_linter from defaults
 - extend to else{ and repeat{

* `code`

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* add explicit test for different behaviour compared to closed_curly_linter

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

Author: AshesITR

DESCRIPTION

@@ -85,7 +85,6 @@ Collate:
     'function_left_parentheses.R'
     'get_source_expressions.R'
     'ids_with_token.R'
-    'if_else_match_braces_linter.R'
     'ifelse_censor_linter.R'
     'implicit_integer_linter.R'
     'infix_spaces_linter.R'

NAMESPACE

@@ -52,7 +52,6 @@ export(extraction_operator_linter)
 export(function_left_parentheses_linter)
 export(get_source_expressions)
 export(ids_with_token)
-export(if_else_match_braces_linter)
 export(ifelse_censor_linter)
 export(implicit_integer_linter)
 export(infix_spaces_linter)

NEWS.md

@@ -11,8 +11,11 @@
 * Rename `semicolon_terminator_linter` to `semicolon_linter` for better consistency. `semicolon_terminator_linter` survives but is marked for deprecation. The new linter also has a new signature, taking arguments `allow_compound` and `allow_trailing` to replace the old single argument `semicolon=`, again for signature consistency with other linters.
 * Combined several curly brace related linters into a new `brace_linter` (#1041, @AshesITR):
   + `closed_curly_linter()`
+  + `open_curly_linter()`, no longer linting unnecessary trailing whitespace
+  + `paren_brace_linter()`, also linting `if`/`else` and `repeat` with missing whitespace
   + Require `else` to come on the same line as the preceding `}`, if present (#884, @michaelchirico)
   + Require functions spanning multiple lines to use curly braces (@michaelchirico)
+  + Require balanced usage of `{}` in `if`/`else` conditions (@michaelchirico)
 * The `...` arguments for `lint()`, `lint_dir()`, and `lint_package()` have promoted to an earlier position to better match the [Tidyverse design principal](https://design.tidyverse.org/args-data-details.html) of data->descriptor->details. This change enables passing objects to `...` without needing to specify non-required arguments, e.g. `lint_dir("/path/to/dir", linter())` now works without the need to specify `relative_path`. This affects some code that uses positional arguments. (#935, @michaelchirico)
   + For `lint()`, `...` is now the 3rd argument, where earlier this was `cache=`
   + For `lint_dir()` and `lint_package()`, `...` is now the 2nd argument, where earlier this was `relative_path=`
@@ -118,7 +121,6 @@ function calls. (#850, #851, @renkun-ken)
    * `yoda_test_linter()` Require usage of `expect_identical(x, 1L)` over `expect_equal(1L, x)` and similar
    * `expect_identical_linter()` Require usage of `expect_identical()` by default, and `expect_equal()` only by exception
    * `expect_comparison_linter()` Require usage of `expect_gt(x, y)` over `expect_true(x > y)` and similar
-   * `if_else_match_braces_linter()` Require balanced usage of `{}` in `if`/`else` conditions
    * `vector_logic_linter()` Require use of scalar logical operators (`&&` and `||`) inside `if()` conditions and similar
    * `any_is_na_linter()` Require usage of `anyNA(x)` over `any(is.na(x))`
    * `class_equals_linter()` Prevent comparing `class(x)` with `==`, `!=`, or `%in%`, where `inherits()` is typically preferred
@@ -132,7 +134,10 @@ function calls. (#850, #851, @renkun-ken)
    * `nested_ifelse_linter()` Prevent nested calls to `ifelse()` like `ifelse(A, x, ifelse(B, y, z))`, and similar
    * `condition_message_linter` Prevent condition messages from being constructed like `stop(paste(...))` (where just `stop(...)` is preferable)
    * `redundant_ifelse_linter()` Prevent usage like `ifelse(A & B, TRUE, FALSE)` or `ifelse(C, 0, 1)` (the latter is `as.numeric(!C)`)
-   * `brace_linter()` Require `else` to come on the same line as the preceding `}`, if present
+   * Extensions to `brace_linter()` 
+     + Require `else` to come on the same line as the preceding `}`, if present
+     + Require balanced usage of `{}` in `if`/`else` conditions
+     + Require functions spanning multiple lines to use curly braces
    * `unreachable_code_linter()` Prevent code after `return()` and `stop()` statements that will never be reached (extended for #1051 thanks to early user testing, thanks @bersbersbers!)
    * `regex_subset_linter()` Require usage of `grep(ptn, x, value = TRUE)` over `x[grep(ptn, x)]` and similar
    * `consecutive_stopifnot_linter()` Require consecutive calls to `stopifnot()` to be unified into one

R/brace_linter.R

@@ -2,15 +2,20 @@
 #'
 #' Perform various style checks related to placement and spacing of curly braces:
 #'
-#'  - Curly braces are on their own line unless they are followed by an `else`.
+#'  - Opening curly braces are never on their own line and are always followed by a newline.
+#'  - Opening curly braces have a space before them.
+#'  - Closing curly braces are on their own line unless they are followed by an `else`.
 #'  - Closing curly braces in `if` conditions are on the same line as the corresponding `else`.
+#'  - Either both or neither branch in `if`/`else` use curly braces, i.e., either both branches use `{...}` or neither
+#'    does.
 #'  - Functions spanning multiple lines use curly braces.
 #'
 #' @param allow_single_line if `TRUE`, allow an open and closed curly pair on the same line.
 #'
 #' @evalRd rd_tags("brace_linter")
 #' @seealso [linters] for a complete list of linters available in lintr. \cr
-#'   <https://style.tidyverse.org/syntax.html#indenting>
+#'   <https://style.tidyverse.org/syntax.html#indenting> \cr
+#'   <https://style.tidyverse.org/syntax.html#if-statements>
 #' @export
 brace_linter <- function(allow_single_line = FALSE) {
   Linter(function(source_expression) {
@@ -20,6 +25,51 @@ brace_linter <- function(allow_single_line = FALSE) {
 
     lints <- list()
 
+    xp_cond_open <- xp_and(c(
+      # matching } is on same line
+      if (isTRUE(allow_single_line)) {
+        "(@line1 != following-sibling::OP-LEFT-BRACE/@line1)"
+      },
+      # double curly
+      "not(
+        (@line1 = parent::expr/preceding-sibling::OP-LEFT-BRACE/@line1) or
+        (@line1 = following-sibling::expr/OP-LEFT-BRACE/@line1)
+      )"
+    ))
+
+    # TODO (AshesITR): if c_style_braces is TRUE, invert the preceding-sibling condition
+    xp_open_curly <- glue::glue("//OP-LEFT-BRACE[
+      { xp_cond_open } and (
+        not(@line1 = parent::expr/preceding-sibling::*/@line2) or
+        @line1 = following-sibling::*[1][not(self::COMMENT)]/@line1
+      )
+    ]")
+
+    lints <- c(lints, lapply(
+      xml2::xml_find_all(source_expression$xml_parsed_content, xp_open_curly),
+      xml_nodes_to_lint,
+      source_file = source_expression,
+      lint_message = paste(
+        "Opening curly braces should never go on their own line and",
+        "should always be followed by a new line."
+      )
+    ))
+
+    xp_open_preceding <- "parent::expr/preceding-sibling::*[1][self::OP-RIGHT-PAREN or self::ELSE or self::REPEAT]"
+
+    xp_paren_brace <- glue::glue("//OP-LEFT-BRACE[
+      @line1 = { xp_open_preceding }/@line1
+      and
+      @col1 = { xp_open_preceding }/@col2 + 1
+    ]")
+
+    lints <- c(lints, lapply(
+      xml2::xml_find_all(source_expression$xml_parsed_content, xp_paren_brace),
+      xml_nodes_to_lint,
+      source_file = source_expression,
+      lint_message = "There should be a space before an opening curly brace."
+    ))
+
     xp_cond_closed <- xp_and(c(
       # matching { is on same line
       if (isTRUE(allow_single_line)) {
@@ -74,6 +124,32 @@ brace_linter <- function(allow_single_line = FALSE) {
       lint_message = "Any function spanning multiple lines should use curly braces."
     ))
 
+    # if (x) { ... } else if (y) { ... } else { ... } is OK; fully exact pairing
+    #   of if/else would require this to be
+    #   if (x) { ... } else { if (y) { ... } else { ... } } since there's no
+    #   elif operator/token in R, which is pretty unseemly
+    xp_if_else_match_brace <- "
+    //IF[
+      following-sibling::expr[2][OP-LEFT-BRACE]
+      and following-sibling::ELSE
+          /following-sibling::expr[1][not(OP-LEFT-BRACE or IF/following-sibling::expr[2][OP-LEFT-BRACE])]
+    ]
+
+    |
+
+    //ELSE[
+      following-sibling::expr[1][OP-LEFT-BRACE]
+      and preceding-sibling::IF/following-sibling::expr[2][not(OP-LEFT-BRACE)]
+    ]
+    "
+
+    lints <- c(lints, lapply(
+      xml2::xml_find_all(source_expression$xml_parsed_content, xp_if_else_match_brace),
+      xml_nodes_to_lint,
+      source_file = source_expression,
+      lint_message = "Either both or neither branch in `if`/`else` should use curly braces."
+    ))
+
     lints
   })
 }

R/if_else_match_braces_linter.R

@@ -9,6 +9,7 @@
 #'   <https://style.tidyverse.org/syntax.html#indenting>
 #' @export
 open_curly_linter <- function(allow_single_line = FALSE) {
+  lintr_deprecated("open_curly_linter", new = "brace_linter", version = "2.0.1.9001", type = "Linter")
   Linter(function(source_file) {
     lapply(
       ids_with_token(source_file, "'{'"),

R/open_curly_linter.R

@@ -6,6 +6,7 @@
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
 paren_brace_linter <- function() {
+  lintr_deprecated("paren_brace_linter", new = "brace_linter", version = "2.0.1.9001", type = "Linter")
   Linter(function(source_file) {
     if (is.null(source_file$xml_parsed_content)) {
       return(NULL)

R/paren_brace_linter.R

@@ -19,16 +19,13 @@ default_linters <- with_defaults(
   cyclocomp_linter(),
   equals_na_linter(),
   function_left_parentheses_linter(),
-  if_else_match_braces_linter(),
   infix_spaces_linter(),
   line_length_linter(),
   no_tab_linter(),
   object_length_linter(),
   object_name_linter(),
   object_usage_linter(),
-  open_curly_linter(),
   paren_body_linter(),
-  paren_brace_linter(),
   pipe_continuation_linter(),
   semicolon_linter(),
   seq_linter(),

R/zzz.R

@@ -27,7 +27,6 @@ expect_true_false_linter,package_development best_practices readability
 expect_type_linter,package_development best_practices
 extraction_operator_linter,style best_practices
 function_left_parentheses_linter,style readability default
-if_else_match_braces_linter,default style readability
 ifelse_censor_linter,best_practices efficiency
 implicit_integer_linter,style consistency best_practices
 infix_spaces_linter,style readability default
@@ -44,11 +43,11 @@ numeric_leading_zero_linter,style consistency readability
 object_length_linter,style readability default configurable
 object_name_linter,style consistency default configurable
 object_usage_linter,style readability correctness default
-open_curly_linter,style readability default configurable
+open_curly_linter,style readability configurable
 outer_negation_linter,readability efficiency best_practices
 package_hooks_linter,style correctness package_development
 paren_body_linter,style readability default
-paren_brace_linter,style readability default
+paren_brace_linter,style readability
 paste_linter,best_practices consistency
 pipe_call_linter,style readability
 pipe_continuation_linter,style readability default