fixed_regex_linter is pipe-aware (#2094)

* add tests to get working

* fixed_regex_linter pipe-aware

* merge NEWS items

* xpath style

* test all pipes

---------

Co-authored-by: AshesITR <[email protected]>

Author: MichaelChirico

NEWS.md

@@ -8,6 +8,9 @@
 ## New and improved features
 
 * New exclusion sentinel `# nolint next` to signify the next line should skip linting (#1791, @MichaelChirico). The usual rules apply for excluding specific linters, e.g. `# nolint next: assignment_linter.`. The exact string used to match a subsequent-line exclusion is controlled by the `exclude_next` config entry or R option `"lintr.exclude_next"`.
+* `fixed_regex_linter()`
+  + Is pipe-aware, in particular removing false positives arong piping into {stringr} functions like `x |> str_replace(fixed("a"), "b")` (#1811, @MichaelChirico).
+  + Gains an option `allow_unescaped` (default `FALSE`) to toggle linting regexes not requiring any escapes or character classes (#1689, @MichaelChirico). Thus `fixed_regex_linter(allow_unescaped = TRUE)` would lint on `grepl("[$]", x)` but not on `grepl("a", x)` since the latter does not use any regex special characters.
 * Linters with logic around the magrittr pipe `%>%` consistently apply it to the other pipes `%!>%`, `%T>%`, `%<>%` (and possibly `%$%`) where appropriate (#2008, @MichaelChirico).
   + `brace_linter()`
   + `pipe_call_linter()`
@@ -27,7 +30,6 @@
 * `conjunct_test_linter()` also lints usage like `dplyr::filter(x, A & B)` in favor of using `dplyr::filter(x, A, B)` (part of #884, @MichaelChirico).
 * `sort_linter()` checks for code like `x == sort(x)` which is better served by using the function `is.unsorted()` (part of #884, @MichaelChirico).
 * `paste_linter()` gains detection for file paths that are better constructed with `file.path()`, e.g. `paste0(dir, "/", file)` would be better as `file.path(dir, file)` (part of #884, @MichaelChirico).
-* `fixed_regex_linter()` gains an option `allow_unescaped` (default `FALSE`) to toggle linting regexes not requiring any escapes or character classes (#1689, @MichaelChirico). Thus `fixed_regex_linter(allow_unescaped = TRUE)` would lint on `grepl("[$]", x)` but not on `grepl("a", x)` since the latter does not use any regex special characters.
 
 ### New linters
 

R/fixed_regex_linter.R

@@ -97,6 +97,12 @@ fixed_regex_linter <- function(allow_unescaped = FALSE) {
     "str_view", "str_view_all", "str_which"
   ))
 
+  pipes <- setdiff(magrittr_pipes, c("%$%", "%T>%"))
+  in_pipe_cond <- glue("
+    parent::expr/preceding-sibling::SPECIAL[{ xp_text_in_table(pipes) }]
+    | parent::expr/preceding-sibling::PIPE
+  ")
+
   # NB: strsplit doesn't have an ignore.case argument
   # NB: we intentionally exclude cases like gsub(x, c("a" = "b")), where "b" is fixed
   xpath <- glue("
@@ -107,7 +113,16 @@ fixed_regex_linter <- function(allow_unescaped = FALSE) {
         and following-sibling::expr[1][NUM_CONST[text() = 'TRUE'] or SYMBOL[text() = 'T']]
       ])
     ]
-    /following-sibling::expr[1][STR_CONST and not(EQ_SUB)]
+    /following-sibling::expr[
+      (
+        position() = 1
+        and STR_CONST
+        and not(EQ_SUB)
+        and not({ in_pipe_cond })
+      ) or (
+        preceding-sibling::*[2][self::SYMBOL_SUB/text() = 'pattern']
+      )
+    ]
   |
   //SYMBOL_FUNCTION_CALL[ {pos_2_regex_funs} ]
     /parent::expr[
@@ -116,7 +131,11 @@ fixed_regex_linter <- function(allow_unescaped = FALSE) {
         and following-sibling::expr[1][NUM_CONST[text() = 'TRUE'] or SYMBOL[text() = 'T']]
       ])
     ]
-    /following-sibling::expr[2][STR_CONST and not(EQ_SUB)]
+    /following-sibling::expr[
+      position() = 2 - count({ in_pipe_cond })
+      and STR_CONST
+      and not(EQ_SUB)
+    ]
   ")
 
   Linter(function(source_expression) {

tests/testthat/test-fixed_regex_linter.R

@@ -353,3 +353,30 @@ test_that("'unescaped' regex can optionally be skipped", {
   expect_lint("str_detect(x, 'a')", NULL, linter)
   expect_lint("grepl('[$]', x)", rex::rex('Here, you can use "$"'), linter)
 })
+
+local({
+  pipes <- pipes(exclude = c("%$%", "%T>%"))
+  patrick::with_parameters_test_that(
+    "linter is pipe-aware",
+    {
+      linter <- fixed_regex_linter()
+      lint_msg <- "This regular expression is static"
+
+      expect_lint(paste("x", pipe, "grepl(pattern = 'a')"), lint_msg, linter)
+      expect_lint(paste("x", pipe, "grepl(pattern = '^a')"), NULL, linter)
+      expect_lint(paste("x", pipe, "grepl(pattern = 'a', fixed = TRUE)"), NULL, linter)
+      expect_lint(paste("x", pipe, "str_detect('a')"), lint_msg, linter)
+      expect_lint(paste("x", pipe, "str_detect('^a')"), NULL, linter)
+      expect_lint(paste("x", pipe, "str_detect(fixed('a'))"), NULL, linter)
+
+      expect_lint(paste("x", pipe, "gsub(pattern = 'a', replacement = '')"), lint_msg, linter)
+      expect_lint(paste("x", pipe, "gsub(pattern = '^a', replacement = '')"), NULL, linter)
+      expect_lint(paste("x", pipe, "gsub(pattern = 'a', replacement = '', fixed = TRUE)"), NULL, linter)
+      expect_lint(paste("x", pipe, "str_replace('a', '')"), lint_msg, linter)
+      expect_lint(paste("x", pipe, "str_replace('^a', '')"), NULL, linter)
+      expect_lint(paste("x", pipe, "str_replace(fixed('a'), '')"), NULL, linter)
+    },
+    pipe = pipes,
+    .test_name = names(pipes)
+  )
+})