New redundant_equals_linter

DESCRIPTION

@@ -127,6 +127,7 @@ Collate:
     'path_linters.R'
     'pipe_call_linter.R'
     'pipe_continuation_linter.R'
+    'redundant_equals_linter.R'
     'redundant_ifelse_linter.R'
     'regex_subset_linter.R'
     'semicolon_linter.R'

NAMESPACE

@@ -92,6 +92,7 @@ export(paren_brace_linter)
 export(paste_linter)
 export(pipe_call_linter)
 export(pipe_continuation_linter)
+export(redundant_equals_linter)
 export(redundant_ifelse_linter)
 export(regex_subset_linter)
 export(sarif_output)

NEWS.md

@@ -16,6 +16,10 @@
 
 * `object_usage_linter()` improves lint metadata when detecting undefined infix operators, e.g. `%>%` or `:=` (#1497, @MichaelChirico)
 
+### New linters
+
+* `redundant_equals_linter()` for redundant comparisons to `TRUE` or `FALSE` like `is_treatment == TRUE` (#1500, @MichaelChirico)
+
 # lintr 3.0.1
 
 * Skip multi-byte tests in non UTF-8 locales (#1504)

R/redundant_equals_linter.R

@@ -0,0 +1,36 @@
+#' Block usage of ==, != on logical vectors
+#'
+#' Testing `x == TRUE` is redundant if `x` is a logical vector. Wherever this
+#'   is used to improve readability, the solution should instead be to improve
+#'   the naming of the object to better indicate that its contents are logical.
+#'
+#' @export
+redundant_equals_linter <- function() {
+  xpath <- paste0(
+    c("//EQ", "//NE"),
+    "/parent::expr/expr[NUM_CONST[text() = 'TRUE' or text() = 'FALSE']]/parent::expr",
+    collapse = " | "
+  )
+
+  Linter(function(source_expression) {
+    if (!is_lint_level(source_expression, "expression")) {
+      return(list())
+    }
+
+    xml <- source_expression$xml_parsed_content
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+    op <- xml2::xml_text(xml2::xml_find_first(bad_expr, "*[2]"))
+
+    xml_nodes_to_lints(
+      bad_expr,
+      source_expression = source_expression,
+      lint_message = paste(
+        "Using", op, "on a logical vector is redundant.",
+        "Well-named logical vectors can be used directly in filtering.",
+        "For data.table's `i` argument, wrap the column name in (), like DT[(is_treatment)]."
+      ),
+      type = "warning"
+    )
+  })
+}

inst/lintr/linters.csv

@@ -53,6 +53,7 @@ paren_brace_linter,style readability deprecated
 paste_linter,best_practices consistency
 pipe_call_linter,style readability
 pipe_continuation_linter,style readability default
+redundant_equals_linter,best_practices readability efficiency common_mistakes
 redundant_ifelse_linter,best_practices efficiency consistency
 regex_subset_linter,best_practices efficiency
 semicolon_linter,style readability default configurable

tests/testthat/test-redundant_equals_linter.R

@@ -0,0 +1,30 @@
+test_that("redundant_equals_linter skips allowed usages", {
+  # comparisons to non-logical constants
+  expect_lint("x == 1", NULL, redundant_equals_linter())
+  # comparison to TRUE as a string
+  expect_lint("x != 'TRUE'", NULL, redundant_equals_linter())
+})
+
+test_that("redundant_equals_linter blocks simple disallowed usages", {
+  linter <- redundant_equals_linter()
+  eq_msg <- rex::rex("Using == on a logical vector is redundant.")
+  ne_msg <- rex::rex("Using != on a logical vector is redundant.")
+  expect_lint("x == TRUE", eq_msg, linter)
+  expect_lint("x != TRUE", ne_msg, linter)
+  expect_lint("x == FALSE", eq_msg, linter)
+  expect_lint("x != FALSE", ne_msg, linter)
+
+  # order doesn't matter
+  expect_lint("TRUE == x", eq_msg, linter)
+})
+
+test_that("mutliple lints return correct custom messages", {
+  expect_lint(
+    "list(x == TRUE, y != TRUE)",
+    list(
+      "Using == on a logical vector",
+      "Using != on a logical vector"
+    ),
+    redundant_equals_linter()
+  )
+})