New vector_logic_linter

DESCRIPTION

@@ -114,6 +114,7 @@ Collate:
     'undesirable_function_linter.R'
     'undesirable_operator_linter.R'
     'unneeded_concatenation_linter.R'
+    'vector_logic_linter.R'
     'with_id.R'
     'xp_utils.R'
     'zzz.R'

NAMESPACE

@@ -78,6 +78,7 @@ export(trailing_whitespace_linter)
 export(undesirable_function_linter)
 export(undesirable_operator_linter)
 export(unneeded_concatenation_linter)
+export(vector_logic_linter)
 export(with_defaults)
 export(with_id)
 import(rex)

NEWS.md

@@ -97,6 +97,7 @@ function calls. (#850, #851, @renkun-ken)
    * `expect_length_linter()` Require usage of `expect_length(x, n)` over `expect_equal(length(x), n)` 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
+   * `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))`
 * `assignment_linter()` now lints right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to lint `<<-` and `->>`; `allow_right_assign` toggles whether to lint `->` and `->>` (#915, @michaelchirico)
 * `infix_spaces_linter()` gains argument `exclude_operators` to disable lints on selected infix operators. By default, all "low-precedence" operators throw lints; see `?infix_spaces_linter` for an enumeration of these. (#914 @michaelchirico)

R/vector_logic_linter.R

@@ -0,0 +1,59 @@
+#' Enforce usage of scalar logical operators in conditional statements
+#'
+#' Usage of `&` in conditional statements is error-prone and inefficient.
+#'   `condition` in `if (condition) expr` must always be length-1, in which
+#'   case `&&` is to be preferred. Ditto for `|` vs. `||`.
+#'
+#' This linter covers inputs to `if()` and `while()` conditions and to
+#'   [testthat::expect_true()] and [testthat::expect_false()].
+#' @evalRd rd_tags("vector_logic_linter")
+#' @seealso
+#'   [linters] for a complete list of linters available in lintr.
+#'   <https://style.tidyverse.org/syntax.html#if-statements>
+#' @export
+vector_logic_linter <- function() {
+  Linter(function(source_file) {
+    if (length(source_file$xml_parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    # ensures the expr is in the cond part of `if/while (cond) expr` --
+    #   if on the XML parse tree is structured like
+    #   <expr>
+    #     <IF> | <expr><SYMBOL_FUNCTION_CALL>
+    #     <OP-LEFT-PAREN>
+    #     <expr> ... </expr> # <- loop condition
+    #     <OP-RIGHT-PAREN>
+    #     <expr> ... </expr> # <- evaluation; includes BRACEs if present
+    #     <ELSE>             # (here & below is optional)
+    #     <expr> ... </expr>
+    #  </expr>
+    #  we _don't_ want to match anything on the second expr, hence this
+    xpath <- "//*[
+      (self::AND or self::OR)
+      and ancestor::expr[
+        not(preceding-sibling::OP-RIGHT-PAREN)
+        and preceding-sibling::*[
+          self::IF
+          or self::WHILE
+          or self::expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'expect_false']]
+        ]
+      ]
+      and not(ancestor::expr[
+        preceding-sibling::expr[SYMBOL_FUNCTION_CALL[not(text() = 'expect_true' or text() = 'expect_false')]]
+        or preceding-sibling::OP-LEFT-BRACKET
+      ])
+    ]"
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    return(lapply(
+      bad_expr,
+      xml_nodes_to_lint,
+      source_file = source_file,
+      lint_message = "Conditional expressions require scalar logical operators (&& and ||)",
+      type = "warning"
+    ))
+  })
+}

R/zzz.R

@@ -107,7 +107,8 @@ default_linters <- with_defaults(
   spaces_left_parentheses_linter(),
   T_and_F_symbol_linter(),
   trailing_blank_lines_linter(),
-  trailing_whitespace_linter()
+  trailing_whitespace_linter(),
+  vector_logic_linter()
 )
 
 #' Default undesirable functions and operators

inst/lintr/linters.csv

@@ -52,3 +52,4 @@ trailing_whitespace_linter,style default
 undesirable_function_linter,style efficiency configurable robustness best_practices
 undesirable_operator_linter,style efficiency configurable robustness best_practices
 unneeded_concatenation_linter,style readability efficiency
+vector_logic_linter,default efficiency best_practices

tests/testthat/default_linter_testcode.R

@@ -25,6 +25,9 @@ someComplicatedFunctionWithALongCamelCaseName <- function(x)
   if (1 > 2 && 2 > 3 && 3 > 4 && 4 > 5 && 5*10 > 6 && 5 > 6 && 6 > 7 && x == NA) {T} else {F}
 }
 
+# vector_logic
+if (1 & 2) FALSE else TRUE
+
 # no_tab
 # pipe_continuation
 # seq_linter

tests/testthat/test-vector_logic_linter.R

@@ -0,0 +1,91 @@
+test_that("vector_logic_linter skips allowed usages", {
+  expect_lint("if (TRUE) 5 else if (TRUE) 2", NULL, vector_logic_linter())
+
+  expect_lint("if (TRUE || FALSE) 1; while (TRUE && FALSE) 2", NULL, vector_logic_linter())
+
+  # function calls and extractions may aggregate to scalars -- only catch
+  #   usages at the highest logical level
+  expect_lint("if (agg_function(x & y)) 1", NULL, vector_logic_linter())
+  expect_lint("if (DT[x | y, cond]) 1", NULL, vector_logic_linter())
+})
+
+test_that("vector_logic_linter blocks simple disallowed usages", {
+  expect_lint(
+    "if (TRUE & FALSE) 1",
+    rex::rex("Conditional expressions require scalar logical operators"),
+    vector_logic_linter()
+  )
+
+  expect_lint(
+    "while (TRUE | TRUE) 2",
+    rex::rex("Conditional expressions require scalar logical operators"),
+    vector_logic_linter()
+  )
+})
+
+test_that("vector_logic_linter detects nested conditions", {
+  expect_lint(
+    "if (TRUE & TRUE || FALSE) 4",
+    rex::rex("Conditional expressions require scalar logical operators"),
+    vector_logic_linter()
+  )
+
+  expect_lint(
+    "if (TRUE && (TRUE | FALSE)) 4",
+    rex::rex("Conditional expressions require scalar logical operators"),
+    vector_logic_linter()
+  )
+
+  # don't match potentially OK usages nested within calls
+  expect_lint("if (TRUE && any(TRUE | FALSE)) 4", NULL, vector_logic_linter())
+  # even if the usage is nested in those calls (b/181915948)
+  expect_lint("if (TRUE && any(TRUE | FALSE | TRUE)) 4", NULL, vector_logic_linter())
+
+  # don't match potentially OK usages in the branch itself
+  lines <- trim_some("
+    if (TRUE) {
+      x | y
+    }
+  ")
+  expect_lint(lines, NULL, vector_logic_linter())
+})
+
+test_that("vector_logic_linter catches usages in expect_true()/expect_false()", {
+  expect_lint(
+    "expect_true(TRUE & FALSE)",
+    rex::rex("Conditional expressions require scalar logical operators"),
+    vector_logic_linter()
+  )
+
+  expect_lint(
+    "expect_false(TRUE | TRUE)",
+    rex::rex("Conditional expressions require scalar logical operators"),
+    vector_logic_linter()
+  )
+
+  # ditto with namespace qualification
+  expect_lint(
+    "testthat::expect_true(TRUE & FALSE)",
+    rex::rex("Conditional expressions require scalar logical operators"),
+    vector_logic_linter()
+  )
+
+  expect_lint(
+    "testthat::expect_false(TRUE | TRUE)",
+    rex::rex("Conditional expressions require scalar logical operators"),
+    vector_logic_linter()
+  )
+
+  # valid nested usage within aggregator
+  expect_lint("testthat::expect_false(any(TRUE | TRUE))", NULL, vector_logic_linter())
+})
+
+test_that("vector_logic_linter doesn't get mixed up from complex usage", {
+  lines <- trim_some("
+  if (a) {
+    expect_true(ok)
+    x <- 2
+    a | b
+  }")
+  expect_lint(lines, NULL, vector_logic_linter())
+})