Remove false negatives with seq() in seq_linter()

DESCRIPTION

@@ -45,7 +45,7 @@ Suggests:
 License: MIT + file LICENSE
 Encoding: UTF-8
 VignetteBuilder: knitr
-RoxygenNote: 7.2.0
+RoxygenNote: 7.2.1
 Config/testthat/edition: 3
 Collate:
     'T_and_F_symbol_linter.R'

NEWS.md

@@ -6,6 +6,9 @@
 
 ## Changes to defaults
 
+* `seq_linter()` produces lint for `seq(...)`, since it also cannot properly 
+  handle empty edge cases (#1468, @IndrajeetPatil).
+
 * `seq_linter()` additionally lints on `1:n()` (from {dplyr}) 
   and `1:.N` (from {data.table}) (#1396, @IndrajeetPatil).
 

R/seq_linter.R

@@ -1,7 +1,11 @@
 #' Sequence linter
 #'
-#' Check for `1:length(...)`, `1:nrow(...)`, `1:ncol(...)`, `1:NROW(...)` and `1:NCOL(...)` expressions in base-R.
-#' Additionally check `1:n()` (from dplyr) and `1:.N` (from data.table).
+#' This linter checks for `1:length(...)`, `1:nrow(...)`, `1:ncol(...)`,
+#' `1:NROW(...)` and `1:NCOL(...)` expressions in base-R, or their usage in
+#' conjunction with `seq()` (e.g., `seq(length(...))`, `seq(nrow(...))`, etc.).
+#'
+#' Additionally, it checks for `1:n()` (from dplyr) and `1:.N` (from data.table).
+#'
 #' These often cause bugs when the right-hand side is zero.
 #' It is safer to use [base::seq_len()] or [base::seq_along()] instead.
 #'
@@ -11,15 +15,27 @@
 seq_linter <- function() {
   bad_funcs <- c("length", "n", "nrow", "ncol", "NROW", "NCOL", "dim")
 
+  # Exact `xpath` depends on whether bad function was used in conjunction with `seq()`
+  bad_func_xpath_with_seq <- glue::glue(
+    "expr[1][SYMBOL_FUNCTION_CALL[text() = 'seq']]
+    /following::expr[1]
+    /expr[SYMBOL_FUNCTION_CALL[ {xp_text_in_table(bad_funcs)} ]]"
+  )
+  bad_func_xpath_without_seq <- glue::glue(
+    "expr[expr[(expr|self::*)[SYMBOL_FUNCTION_CALL[ {xp_text_in_table(bad_funcs)} ]]]]"
+  )
+
   # `.N` from {data.table} is special since it's not a function but a symbol
   xpath <- glue::glue("//expr[
-    expr[NUM_CONST[text() =  '1' or text() =  '1L']]
-    and OP-COLON
-    and (
-          expr[expr[(expr|self::*)[SYMBOL_FUNCTION_CALL[ {xp_text_in_table(bad_funcs)} ]]]]
-          or
-          expr[SYMBOL = '.N']
-        )
+    (
+      {bad_func_xpath_with_seq}
+      and count(expr) = 2
+    ) or
+    (
+      expr[NUM_CONST[text() = '1' or text() = '1L']]
+      and OP-COLON
+      and ( {bad_func_xpath_without_seq} or expr[SYMBOL = '.N'] )
+    )
   ]")
 
   ## The actual order of the nodes is document order
@@ -57,9 +73,17 @@ seq_linter <- function() {
       "seq_along",
       "seq_len"
     )
-    lint_message <- sprintf(
-      "%s:%s is likely to be wrong in the empty edge case. Use %s() instead.",
-      dot_expr1, dot_expr2, replacement
+
+    lint_message <- ifelse(
+      grepl("seq", dot_expr1, fixed = TRUE),
+       sprintf(
+        "%s(%s) is likely to be wrong in the empty edge case. Use %s(...) instead.",
+        dot_expr1, dot_expr2, replacement
+      ),
+      sprintf(
+        "%s:%s is likely to be wrong in the empty edge case. Use %s() instead.",
+        dot_expr1, dot_expr2, replacement
+      )
     )
 
     xml_nodes_to_lints(badx, source_expression, lint_message, type = "warning")

tests/testthat/test-seq_linter.R

@@ -5,6 +5,32 @@ test_that("other : expressions are fine", {
   expect_lint("function(x) { 1:(length(x) || 1) }", NULL, linter)
 })
 
+test_that("seq_len(...) or seq_along(...) expressions are fine", {
+  linter <- seq_linter()
+
+  expect_lint("function(x) { seq_len(x) }", NULL, linter)
+  expect_lint("function(x) { seq_along(x) }", NULL, linter)
+
+  expect_lint("function(x) { seq(2, length(x)) }", NULL, linter)
+  expect_lint("function(x) { seq(length(x), 2) }", NULL, linter)
+})
+
+test_that("finds seq(...) expressions", {
+  linter <- seq_linter()
+
+  expect_lint(
+    "function(x) { seq(length(x)) }",
+    rex("seq(length(...))", anything, "Use seq_along(...)"),
+    linter
+  )
+
+  expect_lint(
+    "function(x) { seq(nrow(x)) }",
+    rex("seq(nrow(...))", anything, "Use seq_len(...)"),
+    linter
+  )
+})
+
 test_that("finds 1:length(...) expressions", {
   linter <- seq_linter()
 
@@ -89,4 +115,22 @@ test_that("Message vectorization works for multiple lints", {
     ),
     seq_linter()
   )
+
+  expect_lint(
+    "c(seq(length(x)), 1:nrow(y))",
+    list(
+      rex::rex("seq(length(...))", anything, "seq_along(...)"),
+      rex::rex("1:nrow(...)", anything, "seq_len()")
+    ),
+    seq_linter()
+  )
+
+  expect_lint(
+    "c(seq(length(x)), seq(nrow(y)))",
+    list(
+      rex::rex("seq(length(...))", anything, "seq_along(...)"),
+      rex::rex("seq(nrow(...))", anything, "seq_len(...)")
+    ),
+    seq_linter()
+  )
 })