Handle "weird" comments in duplicate_argument_linter() (#2404)

NEWS.md

@@ -72,6 +72,7 @@
 ### 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).
+* `duplicate_argument_linter()` no longer misses cases with duplicate arguments where a comment comes between the argument name and `=` (#2402, @MichaelChirico).
 
 # lintr 3.1.1
 

R/duplicate_argument_linter.R

@@ -37,26 +37,32 @@
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
 duplicate_argument_linter <- function(except = c("mutate", "transmute")) {
-  xpath_call_with_args <- "//EQ_SUB/parent::expr"
-  xpath_arg_name <- "./EQ_SUB/preceding-sibling::*[1]"
+  # NB: approach checking for duplicates in XPath is hard because of
+  #   quoted names, e.g. foo(a = 1, `a` = 2), so compute duplicates in R
+  xpath_call_with_args <- glue("
+    //EQ_SUB[not(
+      preceding-sibling::expr/SYMBOL_FUNCTION_CALL[{ xp_text_in_table(except) }]
+    )]
+      /parent::expr[count(EQ_SUB) > 1]
+  ")
+  xpath_arg_name <- "./EQ_SUB/preceding-sibling::*[not(self::COMMENT)][1]"
 
   Linter(linter_level = "file", function(source_expression) {
     xml <- source_expression$full_xml_parsed_content
     if (is.null(xml)) return(list())
 
-    calls <- xml_find_all(xml, xpath_call_with_args)
+    call_expr <- xml_find_all(xml, xpath_call_with_args)
 
-    if (length(except) > 0L) {
-      calls_text <- get_r_string(xp_call_name(calls))
-      calls <- calls[!(calls_text %in% except)]
-    }
-
-    all_arg_nodes <- lapply(calls, xml_find_all, xpath_arg_name)
-    arg_names <- lapply(all_arg_nodes, get_r_string)
-    is_duplicated <- lapply(arg_names, duplicated)
+    bad_expr <- lapply(
+      call_expr,
+      function(expr) {
+        arg_expr <- xml_find_all(expr, xpath_arg_name)
+        arg_expr[duplicated(get_r_string(arg_expr))]
+      }
+    )
 
     xml_nodes_to_lints(
-      unlist(all_arg_nodes, recursive = FALSE)[unlist(is_duplicated)],
+      unlist(bad_expr, recursive = FALSE),
       source_expression = source_expression,
       lint_message = "Avoid duplicate arguments in function calls.",
       type = "warning"

tests/testthat/test-duplicate_argument_linter.R

@@ -21,10 +21,12 @@ test_that("duplicate_argument_linter blocks disallowed usages", {
   expect_lint("dt[i = 1, i = 2]", lint_msg, linter)
 
   expect_lint(
-    "list(
-      var = 1,
-      var = 2
-    )",
+    trim_some("
+      list(
+        var = 1,
+        var = 2
+      )
+    "),
     lint_msg,
     linter
   )
@@ -95,3 +97,163 @@ test_that("doesn't lint duplicated arguments in allowed functions", {
     linter
   )
 })
+
+test_that("interceding comments don't trip up logic", {
+  linter <- duplicate_argument_linter()
+  lint_msg <- rex::rex("Avoid duplicate arguments")
+
+  # comment before the EQ_SUB
+  # actually this case "just works" even before #2402 since
+  #   get_r_string() returns NA for both argument names
+  expect_lint(
+    trim_some("
+      fun(
+        arg # xxx
+        = 1,
+        arg # yyy
+        = 2
+      )
+    "),
+    list(lint_msg, line_number = 4L),
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      fun(
+        arg # xxx
+        = 1,
+        arg = 2
+      )
+    "),
+    list(lint_msg, line_number = 4L),
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      fun(
+        arg = 1,
+        arg # yyy
+        = 2
+      )
+    "),
+    list(lint_msg, line_number = 3L),
+    linter
+  )
+
+  # comment after the EQ_SUB
+  expect_lint(
+    trim_some("
+      fun(
+        arg = # xxx
+        1,
+        arg = # yyy
+        2
+      )
+    "),
+    list(lint_msg, line_number = 4L),
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      fun(
+        arg = # xxx
+        1,
+        arg = 2
+      )
+    "),
+    list(lint_msg, line_number = 4L),
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      fun(
+        arg = 1,
+        arg = # yyy
+        2
+      )
+    "),
+    list(lint_msg, line_number = 3L),
+    linter
+  )
+
+  # comment after the arg value
+  expect_lint(
+    trim_some("
+      fun(
+        arg = 1 # xxx
+        ,
+        arg = 2 # yyy
+      )
+    "),
+    list(lint_msg, line_number = 4L),
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      fun(
+        arg = 1 # xxx
+        ,
+        arg = 2
+      )
+    "),
+    list(lint_msg, line_number = 4L),
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      fun(
+        arg = 1,
+        arg = 2 # yyy
+      )
+    "),
+    list(lint_msg, line_number = 3L),
+    linter
+  )
+
+  # comment after the OP-COMMA
+  expect_lint(
+    trim_some("
+      fun(
+        arg = 1, # xxx
+        arg = 2 # yyy
+      )
+    "),
+    list(lint_msg, line_number = 3L),
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      fun(
+        arg = 1, # xxx
+        arg = 2
+      )
+    "),
+    list(lint_msg, line_number = 3L),
+    linter
+  )
+})
+
+test_that("lints vectorize", {
+  lint_msg <- rex::rex("Avoid duplicate arguments")
+
+  expect_lint(
+    trim_some("{
+      c(a = 1, a = 2, a = 3)
+      list(b = 1, b = 2, b = 3)
+    }"),
+    list(
+      list(lint_msg, line_number = 2L, column_number = 12L),
+      list(lint_msg, line_number = 2L, column_number = 19L),
+      list(lint_msg, line_number = 3L, column_number = 15L),
+      list(lint_msg, line_number = 3L, column_number = 22L)
+    ),
+    duplicate_argument_linter()
+  )
+})