From 50d17ae31340b8b8f818f2dbdbd4fb75ec0435b1 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 8 Dec 2023 13:45:17 +0800 Subject: [PATCH] Handle "weird" comments in duplicate_argument_linter() (#2404) --- NEWS.md | 1 + R/duplicate_argument_linter.R | 30 ++-- .../testthat/test-duplicate_argument_linter.R | 170 +++++++++++++++++- 3 files changed, 185 insertions(+), 16 deletions(-) diff --git a/NEWS.md b/NEWS.md index 731a8a9d9..d9de7e404 100644 --- a/NEWS.md +++ b/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 diff --git a/R/duplicate_argument_linter.R b/R/duplicate_argument_linter.R index ca579b82e..d152d9e3f 100644 --- a/R/duplicate_argument_linter.R +++ b/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" diff --git a/tests/testthat/test-duplicate_argument_linter.R b/tests/testthat/test-duplicate_argument_linter.R index 05a5e51cb..00fdd5e66 100644 --- a/tests/testthat/test-duplicate_argument_linter.R +++ b/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() + ) +})