diff --git a/NEWS.md b/NEWS.md index b15057d4a..e12772ccd 100644 --- a/NEWS.md +++ b/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 diff --git a/R/fixed_regex_linter.R b/R/fixed_regex_linter.R index 40f43c1c9..3a9be9d60 100644 --- a/R/fixed_regex_linter.R +++ b/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) { diff --git a/tests/testthat/test-fixed_regex_linter.R b/tests/testthat/test-fixed_regex_linter.R index e00b32ff7..95e38e1f0 100644 --- a/tests/testthat/test-fixed_regex_linter.R +++ b/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) + ) +})