Merge remote-tracking branch 'origin/unnecessary_nesting' into unnecessary_nesting

Author: MichaelChirico

R/unnecessary_nesting_linter.R

@@ -3,6 +3,13 @@
 #' Excessive nesting harms readability. Use helper functions or early returns
 #'   to reduce nesting wherever possible.
 #'
+#' @param allow_assignment Logical, default `TRUE`, in which case
+#'   braced expressions consisting only of a single assignment are skipped.
+#'   if `FALSE`, all braced expressions with only one child expression are linted.
+#'   The `TRUE` case facilitates interaction with [implicit_assignment_linter()]
+#'   for certain cases where an implicit assignment is necessary, so a braced
+#'   assignment is used to further distinguish the assignment. See examples.
+#'
 #' @examples
 #' # will produce lints
 #' code <- "if (A) {\n  stop('A is bad!')\n} else {\n  do_good()\n}"
@@ -19,6 +26,13 @@
 #'   linters = unnecessary_nesting_linter()
 #' )
 #'
+#' code <- "expect_warning(\n  {\n    x <- foo()\n  },\n  'warned'\n)"
+#' writeLines(code)
+#' lint(
+#'   text = code,
+#'   linters = unnecessary_nesting_linter(allow_assignment = FALSE)
+#' )
+#'
 #' # okay
 #' code <- "if (A) {\n  stop('A is bad because a.')\n} else {\n  stop('!A is bad too.')\n}"
 #' writeLines(code)
@@ -34,20 +48,27 @@
 #'   linters = unnecessary_nesting_linter()
 #' )
 #'
+#' code <- "expect_warning(\n  {\n    x <- foo()\n  },\n  'warned'\n)"
+#' writeLines(code)
+#' lint(
+#'   text = code,
+#'   linters = unnecessary_nesting_linter()
+#' )
+#'
 #' @evalRd rd_tags("unnecessary_nesting_linter")
 #' @seealso
 #'  - [cyclocomp_linter()] for another linter that penalizes overly complexcode.
 #'  - [linters] for a complete list of linters available in lintr.
 #' @export
-unnecessary_nesting_linter <- function() {
+unnecessary_nesting_linter <- function(allow_assignment = TRUE) {
   exit_calls <- c("stop", "return", "abort", "quit", "q")
   # These calls can be called in the sibling branch and not trigger a lint,
   #   allowing for cleanly parallel code, where breaking it would often harm readability:
-  #   if (A) {
-  #     stop()
-  #   } else {
-  #     warning()
-  #   }
+  #   > if (A) {
+  #   >   stop()
+  #   > } else {
+  #   >   warning()
+  #   > }
   # NB: print() is intentionally excluded since its usage is usually a mistake (?print_linter)
   signal_calls <- c(
     exit_calls,
@@ -93,6 +114,8 @@ unnecessary_nesting_linter <- function() {
   ]
   ")
 
+  assignment_cond <- if (allow_assignment) "expr[LEFT_ASSIGN or RIGHT_ASSIGN]" else "false"
+
   # several carve-outs of common cases where single-expression braces are OK
   #   - control flow statements: if, for, while, repeat, switch()
   #       + switch() is unique in being a function, not a language element
@@ -101,7 +124,7 @@ unnecessary_nesting_linter <- function() {
   #       + includes purrr-like anonymous functions as ~ {...}
   #   - rlang's double-brace expressions like {{ var }}
   #       + NB: both braces would trigger here, so we must exclude both of them
-  #   - any expression ending like `})` or `}]`
+  #   - any expression starting like `({` or `[{` or ending like `})` or `}]`
   #       + note that nesting is not improved by "fixing" such cases,
   #         and could also be worsened
   #       + motivated by the most common cases:
@@ -110,7 +133,7 @@ unnecessary_nesting_linter <- function() {
   #          * suppressWarnings({ expr })
   #          * DataTable[, { expr }]
   #          * DataTable[, col := { expr }] <- requires carve-out for `:=`
-  unnecessary_brace_xpath <- "
+  unnecessary_brace_xpath <- glue("
   //OP-LEFT-BRACE
     /parent::expr[
       count(expr) = 1
@@ -128,11 +151,12 @@ unnecessary_nesting_linter <- function() {
       and not(expr/OP-LEFT-BRACE)
       and not(preceding-sibling::OP-LEFT-BRACE)
       and not(
-        OP-RIGHT-BRACE/@end + 1 = following-sibling::OP-RIGHT-PAREN/@end
-        or OP-RIGHT-BRACE/@end + 1 = following-sibling::OP-RIGHT-BRACKET/@end
+        OP-LEFT-BRACE/@end - 1 = preceding-sibling::*[1][self::OP-LEFT-PAREN or self::OP-LEFT-BRACKET]/@end
+        or OP-RIGHT-BRACE/@end + 1 = following-sibling::*[1][self::OP-RIGHT-PAREN or self::OP-RIGHT-BRACKET]/@end
       )
+      and not({assignment_cond})
     ]
-  "
+  ")
 
   Linter(function(source_expression) {
     if (!is_lint_level(source_expression, "expression")) {

inst/lintr/linters.csv

@@ -109,7 +109,7 @@ undesirable_operator_linter,style efficiency configurable robustness best_practi
 unnecessary_concatenation_linter,style readability efficiency configurable
 unnecessary_lambda_linter,best_practices efficiency readability
 unnecessary_nested_if_linter,readability best_practices
-unnecessary_nesting_linter,readability consistency
+unnecessary_nesting_linter,readability consistency configurable
 unnecessary_placeholder_linter,readability best_practices
 unneeded_concatenation_linter,style readability efficiency configurable deprecated
 unreachable_code_linter,best_practices readability

man/configurable_linters.Rd

@@ -224,6 +224,18 @@ test_that("unnecessary_nesting_linter skips one-expression switch statements", {
   expect_lint(lines, NULL, unnecessary_nesting_linter())
 })
 
+test_that("unnecessary_nesting_linter skips one-expression assignments by default", {
+  expect_lint(
+    trim_some("
+      {
+        x <- foo()
+      }
+    "),
+    NULL,
+    unnecessary_nesting_linter()
+  )
+})
+
 test_that("unnecessary_nesting_linter passes for multi-line braced expressions", {
   lines <- c(
     "tryCatch(",
@@ -240,24 +252,55 @@ test_that("unnecessary_nesting_linter passes for multi-line braced expressions",
 test_that("unnecessary_nesting_linter skips if unbracing won't reduce nesting", {
   linter <- unnecessary_nesting_linter()
 
-  test_that_lines <- c(
-    "test_that('this works', {",
-    "  expect_true(TRUE)",
-    "})"
+  expect_lint(
+    trim_some("
+      test_that('this works', {
+        expect_true(TRUE)
+      })
+    "),
+    NULL,
+    linter
   )
-  expect_lint(test_that_lines, NULL, linter)
-  data_table_lines <- c(
-    "DT[, {",
-    "  plot(x, y)",
-    "}]"
+  expect_lint(
+    trim_some("
+      DT[, {
+        plot(x, y)
+      }]
+    "),
+    NULL,
+    linter
+  )
+  expect_lint(
+    trim_some("
+      DT[, x := {
+        foo(x, y)
+      }]
+    "),
+    NULL,
+    linter
+  )
+
+  # NB: styler would re-style these anyway
+  expect_lint(
+    trim_some("
+      tryCatch({
+        foo()
+      }, error = identity)
+    "),
+    NULL,
+    linter
   )
-  expect_lint(data_table_lines, NULL, linter)
-  data_table_assign_lines <- c(
-    "DT[, x := {",
-    "  foo(x, y)",
-    "}]"
+
+  expect_lint(
+    trim_some("
+      DT[{
+        n <- .N - 1
+        x[n] < y[n]
+      }, j = TRUE, by = x]
+    "),
+    NULL,
+    linter
   )
-  expect_lint(data_table_assign_lines, NULL, linter)
 })
 
 test_that("rlang's double-brace operator is skipped", {
@@ -269,17 +312,31 @@ test_that("rlang's double-brace operator is skipped", {
 })
 
 test_that("unnecessary_nesting_linter blocks one-expression braced expressions", {
-  lines <- c(
-    "tryCatch(",
-    "  {",
-    "    foo(x)",
-    "  },",
-    "  error = identity",
-    ")"
-  )
   expect_lint(
-    lines,
-    R"(Reduce the nesting of this statement by removing the braces \{\}\.)",
+    trim_some("
+      tryCatch(
+        {
+          foo(x)
+        },
+        error = identity
+      )
+    "),
+    rex::rex("Reduce the nesting of this statement by removing the braces {}."),
     unnecessary_nesting_linter()
   )
 })
+
+test_that("unnecessary_nesting_linter allow_assignment= argument works", {
+  expect_lint(
+    trim_some("
+      tryCatch(
+        {
+          idx <- foo(x)
+        },
+        error = identity
+      )
+    "),
+    rex::rex("Reduce the nesting of this statement by removing the braces {}."),
+    unnecessary_nesting_linter(allow_assignment = FALSE)
+  )
+})