New which_grepl_linter

DESCRIPTION

@@ -186,6 +186,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

@@ -150,6 +150,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

@@ -15,6 +15,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).
 * `print_linter()` for discouraging usage of `print()` on string literals like `print("Reached here")` or `print(paste("Found", nrow(DF), "rows."))` (#1894, @MichaelChirico).
 
 ### Lint accuracy fixes: removing false positives

R/linter_tag_docs.R

@@ -111,3 +111,11 @@ NULL
 #'  - <https://testthat.r-lib.org>
 #'  - <https://r-pkgs.org/testing-basics.html>
 NULL
+
+#' Regular expression linters
+#' @name regex_linters
+#' @description
+#' Linters that examine the usage of regular expressions and functions executing them in user code.
+#' @evalRd rd_linters("regex")
+#' @seealso [linters] for a complete list of linters available in lintr.
+NULL

R/which_grepl_linter.R

@@ -0,0 +1,30 @@
+#' 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.
+#'
+#' @examples
+#' # will produce lints
+#' lint(
+#'   text = "which(grepl('^a', x))",
+#'   linters = which_grepl_linter()
+#' )
+#'
+#' # okay
+#' lint(
+#'   text = "which(grepl('^a', x) | grepl('^b', x))",
+#'   linters = which_grepl_linter()
+#' )
+#'
+#' @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

@@ -30,7 +30,7 @@ expect_s4_class_linter,package_development best_practices pkg_testthat
 expect_true_false_linter,package_development best_practices readability pkg_testthat
 expect_type_linter,package_development best_practices pkg_testthat
 extraction_operator_linter,style best_practices
-fixed_regex_linter,best_practices readability efficiency configurable
+fixed_regex_linter,best_practices readability efficiency configurable regex
 for_loop_index_linter,best_practices readability robustness
 function_argument_linter,style consistency best_practices
 function_left_parentheses_linter,style readability default
@@ -74,7 +74,7 @@ print_linter,best_practices consistency
 quotes_linter,style consistency readability default configurable
 redundant_equals_linter,best_practices readability efficiency common_mistakes
 redundant_ifelse_linter,best_practices efficiency consistency configurable
-regex_subset_linter,best_practices efficiency
+regex_subset_linter,best_practices efficiency regex
 repeat_linter,style readability
 routine_registration_linter,best_practices efficiency robustness
 sample_int_linter,efficiency readability robustness
@@ -88,7 +88,7 @@ spaces_inside_linter,style readability default
 spaces_left_parentheses_linter,style readability default
 sprintf_linter,correctness common_mistakes
 stopifnot_all_linter,readability best_practices
-string_boundary_linter,readability efficiency configurable
+string_boundary_linter,readability efficiency configurable regex
 strings_as_factors_linter,robustness
 system_file_linter,consistency readability best_practices
 T_and_F_symbol_linter,style readability robustness consistency best_practices default
@@ -106,5 +106,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 regex
 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
+  )
+})