New which_grepl_linter

DESCRIPTION

@@ -185,6 +185,7 @@ Collate:
     'unused_import_linter.R'
     'use_lintr.R'
     'vector_logic_linter.R'
+    'which_grepl_linter.R'
     'whitespace_linter.R'
     'with.R'
     'with_id.R'

NAMESPACE

@@ -149,6 +149,7 @@ export(unreachable_code_linter)
 export(unused_import_linter)
 export(use_lintr)
 export(vector_logic_linter)
+export(which_grepl_linter)
 export(whitespace_linter)
 export(with_defaults)
 export(with_id)

NEWS.md

@@ -10,6 +10,7 @@
 * `stopifnot_all_linter()` discourages tests with `all()` like `stopifnot(all(x > 0))`; `stopifnot()` runs `all()` itself, and uses a better error message (part of #884, @MichaelChirico).
 * `comparison_negation_linter()` for discouraging negated comparisons when a direct negation is preferable, e.g. `!(x == y)` could be `x != y` (part of #884, @MichaelChirico).
 * `terminal_close_linter()` for discouraging using `close()` to end functions (part of #884, @MichaelChirico). Such usages are not robust to errors, where `close()` will not be run as intended. Put `close()` in an `on.exit()` hook, or use {withr} to manage connections with proper cleanup.
+* `which_grepl_linter()` for discouraging `which(grepl(ptn, x))` in favor of directly using `grep(ptn, x)` (part of #884, @MichaelChirico).
 
 ### Lint accuracy fixes: removing false positives
 

R/which_grepl_linter.R

@@ -0,0 +1,17 @@
+#' Require usage of grep over which(grepl(.))
+#'
+#' `which(grepl(pattern, x))` is the same as `grep(pattern, x)`, but harder
+#'   to read and requires two passes over the vector.
+#'
+#' @evalRd rd_tags("which_grepl_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+which_grepl_linter <- make_linter_from_xpath(
+  xpath = "
+  //SYMBOL_FUNCTION_CALL[text() = 'grepl']
+    /parent::expr
+    /parent::expr
+    /parent::expr[expr/SYMBOL_FUNCTION_CALL[text() = 'which']]
+  ",
+  lint_message = "grep(pattern, x) is better than which(grepl(pattern, x))."
+)

inst/lintr/linters.csv

@@ -105,5 +105,6 @@ unneeded_concatenation_linter,style readability efficiency configurable deprecat
 unreachable_code_linter,best_practices readability
 unused_import_linter,best_practices common_mistakes configurable executing
 vector_logic_linter,default efficiency best_practices
+which_grepl_linter,readability efficiency consistency
 whitespace_linter,style consistency default
 yoda_test_linter,package_development best_practices readability pkg_testthat

tests/testthat/test-which_grepl_linter.R

@@ -0,0 +1,27 @@
+test_that("which_grepl_linter skips allowed usages", {
+  # this _could_ be combined as p1|p2, but often it's cleaner to read this way
+  expect_lint("which(grepl(p1, x) & grepl(p2, x))", NULL, which_grepl_linter())
+})
+
+test_that("which_grepl_linter blocks simple disallowed usages", {
+  linter <- which_grepl_linter()
+  lint_msg <- rex::rex("grep(pattern, x) is better than which(grepl(pattern, x)).")
+
+  expect_lint("which(grepl('^a', x))", lint_msg, linter)
+  # options also don't matter (grep has more arguments: value, invert)
+  expect_lint("which(grepl('^a', x, perl = TRUE, fixed = TRUE))", lint_msg, linter)
+
+  expect_lint(
+    trim_some('{
+      which(x)
+      grepl(y)
+      which(grepl("pt1", x))
+      which(grepl("pt2", y))
+    }'),
+    list(
+      list(lint_msg, line_number = 4L, column_number = 3L),
+      list(lint_msg, line_number = 5L, column_number = 3L)
+    ),
+    linter
+  )
+})