Ignore comments in empty call like foo() in missing_argument_linter (#…

…2754) * expect_no_lint * New tests * probable fix * NEWS --------- Co-authored-by: AshesITR <[email protected]>
r-lib · Feb 17, 2025 · a370a9b · a370a9b
1 parent 15b75d8
commit a370a9b
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 15 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -20,6 +20,7 @@
 * `unnecessary_nesting_linter()` treats function bodies under the shorthand lambda (`\()`) the same as normal function bodies (#2748, @MichaelChirico).
 * `string_boundary_linter()` omits lints of patterns like `\\^` which have an anchor but are not regular expressions (#2636, @MichaelChirico).
 * `implicit_integer_linter(allow_colon = TRUE)` is OK with negative literals, e.g. `-1:1` or `1:-1` (#2673, @MichaelChirico).
+* `missing_argument_linter()` allows empty calls like `foo()` even if there are comments between `(` and `)` (#2741, @MichaelChirico).
 
 ## Notes
 

diff --git a/R/missing_argument_linter.R b/R/missing_argument_linter.R
@@ -42,9 +42,10 @@ missing_argument_linter <- function(except = c("alist", "quote", "switch"), allo
     )
   }
 
-  # require >3 children to exclude foo(), which is <expr><OP-LEFT-PAREN><OP-RIGHT-PAREN>
+  # require >3 children to exclude foo(), which is <expr><OP-LEFT-PAREN><OP-RIGHT-PAREN>,
+  #   possibly with intervening comments too, #2741
   xpath <- glue("
-  parent::expr[count(*) > 3]
+  parent::expr[count(*) - count(COMMENT) > 3]
     /*[{xp_or(conds)}]
   ")
 

diff --git a/tests/testthat/test-missing_argument_linter.R b/tests/testthat/test-missing_argument_linter.R
@@ -1,19 +1,23 @@
 test_that("missing_argument_linter skips allowed usages", {
   linter <- missing_argument_linter()
 
-  expect_lint("fun(x, a = 1)", NULL, linter)
-  expect_lint("fun(x = 1, a = 1)", NULL, linter)
-  expect_lint("dt[, 1]", NULL, linter)
-  expect_lint("dt[, 'col']", NULL, linter)
-  expect_lint("array[, , 1]", NULL, linter)
-  expect_lint("switch(a =, b =, c = 1, 0)", NULL, linter)
-  expect_lint("alist(a =, b =, c = 1, 0)", NULL, linter)
-  expect_lint("pairlist(path = quote(expr = ))", NULL, linter) #1889
+  expect_no_lint("fun(x, a = 1)", linter)
+  expect_no_lint("fun(x = 1, a = 1)", linter)
+  expect_no_lint("dt[, 1]", linter)
+  expect_no_lint("dt[, 'col']", linter)
+  expect_no_lint("array[, , 1]", linter)
+  expect_no_lint("switch(a =, b =, c = 1, 0)", linter)
+  expect_no_lint("alist(a =, b =, c = 1, 0)", linter)
+  expect_no_lint("pairlist(path = quote(expr = ))", linter) #1889
 
   # always allow this missing usage
-  expect_lint("foo()", NULL, linter)
+  expect_no_lint("foo()", linter)
+  # even if there's intervening comment(s)
+  expect_no_lint("foo(#\n)", linter)
+  expect_no_lint("foo(\n#\n)", linter)
+  expect_no_lint("foo(#\n#\n)", linter)
 
-  expect_lint("test(a =, b =, c = 1, 0)", NULL, missing_argument_linter("test"))
+  expect_no_lint("test(a =, b =, c = 1, 0)", missing_argument_linter("test"))
 })
 
 test_that("missing_argument_linter blocks disallowed usages", {
@@ -99,15 +103,14 @@ test_that("except list can be empty", {
 test_that("allow_trailing can allow trailing empty args also for non-excepted functions", {
   linter <- missing_argument_linter(allow_trailing = TRUE)
 
-  expect_lint("fun(a = 1,)", NULL, linter)
-  expect_lint(
+  expect_no_lint("fun(a = 1,)", linter)
+  expect_no_lint(
     trim_some("
       fun(
         a = 1,
         # comment
       )
     "),
-    NULL,
     linter
   )
   # ... but not if the final argument is named