New which_grepl_linter (#2281)

* New which_grepl_linter * correct test to use '|' * examples * new "regex" linter tag * forgot to check this in * wording * fix doc --------- Co-authored-by: Indrajeet Patil <[email protected]>
r-lib · Nov 18, 2023 · 6634664 · 6634664
1 parent 1d84cae
commit 6634664
Show file tree

Hide file tree

Showing 16 changed files with 138 additions and 12 deletions.
diff --git a/DESCRIPTION b/DESCRIPTION
@@ -188,6 +188,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'

diff --git a/NAMESPACE b/NAMESPACE
@@ -152,6 +152,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)

diff --git a/NEWS.md b/NEWS.md
@@ -27,6 +27,7 @@
 * `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).
 * `nzchar_linter()` for encouraging `nzchar()` to test for empty strings, e.g. `nchar(x) > 0` can be `nzchar(x)` (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).
 * `list_comparison_linter()` for discouraging comparisons on the output of `lapply()`, e.g. `lapply(x, sum) > 10` (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).
 

diff --git a/R/linter_tag_docs.R b/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
diff --git a/R/which_grepl_linter.R b/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))."
+)
diff --git a/inst/lintr/linters.csv b/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
@@ -76,7 +76,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
@@ -90,7 +90,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
@@ -108,5 +108,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
diff --git a/man/consistency_linters.Rd b/man/consistency_linters.Rd
diff --git a/man/efficiency_linters.Rd b/man/efficiency_linters.Rd
diff --git a/man/fixed_regex_linter.Rd b/man/fixed_regex_linter.Rd
diff --git a/man/linters.Rd b/man/linters.Rd
diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd
diff --git a/man/regex_linters.Rd b/man/regex_linters.Rd
diff --git a/man/regex_subset_linter.Rd b/man/regex_subset_linter.Rd
diff --git a/man/string_boundary_linter.Rd b/man/string_boundary_linter.Rd
diff --git a/man/which_grepl_linter.Rd b/man/which_grepl_linter.Rd
diff --git a/tests/testthat/test-which_grepl_linter.R b/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
+  )
+})