Catch all missing arguments when several are in the same call (#2400)

* also test allow_missing=TRUE case * narrow line * restore ddeleted test --------- Co-authored-by: AshesITR <[email protected]>
r-lib · Dec 6, 2023 · e1862b2 · e1862b2
1 parent e84ab51
commit e1862b2
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 2 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -69,6 +69,10 @@
   + ignores calls on the RHS of operators like `lapply(l, function(x) "a" %in% names(x))` (#2310, @MichaelChirico).
 * `vector_logic_linter()` recognizes some cases where bitwise `&`/`|` are used correctly (#1453, @MichaelChirico).
 
+### Lint accuracy fixes: removing false negatives
+
+* `missing_argument_linter()` catches all missing arguments in calls with several, e.g. `foo(,,)` gives 3 lints instead of 2 (#2399, @MichaelChirico).
+
 # lintr 3.1.1
 
 ## Breaking changes

diff --git a/R/missing_argument_linter.R b/R/missing_argument_linter.R
@@ -37,10 +37,13 @@ missing_argument_linter <- function(except = c("alist", "quote", "switch"), allo
     "self::EQ_SUB[following-sibling::*[not(self::COMMENT)][1][self::OP-RIGHT-PAREN or self::OP-COMMA]]"
   )
   if (!allow_trailing) {
-    conds <- c(conds, "self::OP-COMMA[following-sibling::*[not(self::COMMENT)][1][self::OP-RIGHT-PAREN]]")
+    conds <- c(conds,
+      "self::OP-RIGHT-PAREN[preceding-sibling::*[not(self::COMMENT)][1][self::OP-LEFT-PAREN or self::OP-COMMA]]"
+    )
   }
 
-  xpath <- glue("//SYMBOL_FUNCTION_CALL/parent::expr/parent::expr/*[{xp_or(conds)}]")
+  # require >3 children to exclude foo(), which is <expr><OP-LEFT-PAREN><OP-RIGHT-PAREN>
+  xpath <- glue("//SYMBOL_FUNCTION_CALL/parent::expr/parent::expr[count(*) > 3]/*[{xp_or(conds)}]")
   to_function_xpath <- "string(./preceding-sibling::expr[last()]/SYMBOL_FUNCTION_CALL)"
 
   Linter(linter_level = "file", function(source_expression) {

diff --git a/tests/testthat/test-missing_argument_linter.R b/tests/testthat/test-missing_argument_linter.R
@@ -10,6 +10,9 @@ test_that("missing_argument_linter skips allowed usages", {
   expect_lint("alist(a =, b =, c = 1, 0)", NULL, linter)
   expect_lint("pairlist(path = quote(expr = ))", NULL, linter) #1889
 
+  # always allow this missing usage
+  expect_lint("foo()", NULL, linter)
+
   expect_lint("test(a =, b =, c = 1, 0)", NULL, missing_argument_linter("test"))
 })
 
@@ -100,3 +103,47 @@ test_that("missing_argument_linter blocks disallowed usages", {
     linter
   )
 })
+
+test_that("lints vectorize", {
+  linter <- missing_argument_linter()
+  linter_trailing <- missing_argument_linter(allow_trailing = TRUE)
+  lint_msg <- rex::rex("Missing argument in function call.")
+
+  expect_lint(
+    "foo(,,)",
+    list(
+      list(lint_msg, column_number = 5L),
+      list(lint_msg, column_number = 6L),
+      list(lint_msg, column_number = 7L)
+    ),
+    linter
+  )
+  expect_lint(
+    "foo(,,)",
+    list(
+      list(lint_msg, column_number = 5L),
+      list(lint_msg, column_number = 6L)
+    ),
+    linter_trailing
+  )
+
+  expect_lint(
+    trim_some("{
+      foo(1,)
+      bar(,2)
+    }"),
+    list(
+      list(lint_msg, line_number = 2L),
+      list(lint_msg, line_number = 3L)
+    ),
+    linter
+  )
+  expect_lint(
+    trim_some("{
+      foo(1,)
+      bar(,2)
+    }"),
+    list(lint_msg, line_number = 3L),
+    linter_trailing
+  )
+})