From af73210140d4960b2fd7b7c56af11cd7c2351f33 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 13:40:38 +0200 Subject: [PATCH 01/20] Update sprintf_linter() tests to lint constant strings --- tests/testthat/test-sprintf_linter.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-sprintf_linter.R b/tests/testthat/test-sprintf_linter.R index e0626a974d..748d1e6ea5 100644 --- a/tests/testthat/test-sprintf_linter.R +++ b/tests/testthat/test-sprintf_linter.R @@ -4,7 +4,6 @@ patrick::with_parameters_test_that( linter <- sprintf_linter() # NB: using paste0, not sprintf, to avoid escaping '%d' in sprint fmt= - expect_lint(paste0(call_name, "('hello')"), NULL, linter) expect_lint(paste0(call_name, "('hello %d', 1)"), NULL, linter) expect_lint(paste0(call_name, "('hello %d', x)"), NULL, linter) expect_lint(paste0(call_name, "('hello %d', x + 1)"), NULL, linter) @@ -23,7 +22,9 @@ patrick::with_parameters_test_that( linter <- sprintf_linter() unused_arg_msg <- if (getRversion() >= "4.1.0") "one argument not used by format" else NULL - expect_lint(paste0(call_name, "('hello', 1)"), unused_arg_msg, linter) + expect_lint(paste0(call_name, "('hello', 1)"), "constant", linter) + + expect_lint(paste0(call_name, "('hello')"), "constant", linter) expect_lint( paste0(call_name, "('hello %d', 'a')"), From 4efbd5a0d835ccadca6ad00750153bdee0baee68 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:20:19 +0200 Subject: [PATCH 02/20] Factor out fmt_by_name_xpath --- R/sprintf_linter.R | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index 1eb3b345d2..fe8763d01f 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -27,21 +27,22 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export sprintf_linter <- function() { - call_xpath <- " + fmt_by_name_xpath <- "SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/STR_CONST" + call_xpath <- glue::glue(" parent::expr[ ( - OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST or - SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/STR_CONST + OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST + or {fmt_by_name_xpath} ) and not(expr/SYMBOL[text() = '...']) - ]" + ]") pipes <- setdiff(magrittr_pipes, "%$%") in_pipe_xpath <- glue("self::expr[ preceding-sibling::*[1][self::PIPE or self::SPECIAL[{ xp_text_in_table(pipes) }]] and ( preceding-sibling::*[2]/STR_CONST - or SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/STR_CONST + or {fmt_by_name_xpath} ) ]") From e0c4903227e7672e991b6544d55053d593b643e1 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:21:16 +0200 Subject: [PATCH 03/20] Do not test for string fmt in in_pipe_xpath --- R/sprintf_linter.R | 4 ---- 1 file changed, 4 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index fe8763d01f..d70756b4ce 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -40,10 +40,6 @@ sprintf_linter <- function() { pipes <- setdiff(magrittr_pipes, "%$%") in_pipe_xpath <- glue("self::expr[ preceding-sibling::*[1][self::PIPE or self::SPECIAL[{ xp_text_in_table(pipes) }]] - and ( - preceding-sibling::*[2]/STR_CONST - or {fmt_by_name_xpath} - ) ]") is_missing <- function(x) is.symbol(x) && !nzchar(x) From 14236f065208ce93c73b4957f160e0b7e55e98e5 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:21:37 +0200 Subject: [PATCH 04/20] Add lint to detect constant strings in sprintf --- R/sprintf_linter.R | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index d70756b4ce..2b7a32beb0 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -101,15 +101,39 @@ sprintf_linter <- function() { Linter(linter_level = "file", function(source_expression) { xml_calls <- source_expression$xml_find_function_calls(c("sprintf", "gettextf")) sprintf_calls <- xml_find_all(xml_calls, call_xpath) + in_pipeline <- !is.na(xml_find_first(sprintf_calls, in_pipe_xpath)) - sprintf_warning <- vapply(sprintf_calls, capture_sprintf_warning, character(1L)) + fmt_by_name <- get_r_string(sprintf_calls, fmt_by_name_xpath) + fmt_by_pos <- get_r_string( + sprintf_calls, + "OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST" + ) + fmt_by_pos_piped <- get_r_string( + sprintf_calls, + "preceding-sibling::*[2]/STR_CONST" + ) + + fmt <- ifelse(!is.na(fmt_by_name), fmt_by_name, ifelse(in_pipeline, fmt_by_pos_piped, fmt_by_pos)) + constant_fmt <- !is.na(fmt) & !grepl("%[^%]", fmt) + + constant_fmt_lint <- xml_nodes_to_lints( + sprintf_calls[constant_fmt], + source_expression = source_expression, + lint_message = "A constant string is used and the sprintf call can be removed", + type = "warning" + ) + + templated_sprintf_calls <- sprintf_calls[!constant_fmt & !is.na(fmt)] + sprintf_warning <- vapply(templated_sprintf_calls, capture_sprintf_warning, character(1L)) has_warning <- !is.na(sprintf_warning) - xml_nodes_to_lints( - sprintf_calls[has_warning], + invalid_sprintf_lint <- xml_nodes_to_lints( + templated_sprintf_calls[has_warning], source_expression = source_expression, lint_message = sprintf_warning[has_warning], type = "warning" ) + + c(constant_fmt_lint, invalid_sprintf_lint) }) } From ea4aded72971a559e59c4e51b5f210c28d0e4707 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:30:11 +0200 Subject: [PATCH 05/20] Fix constant sprintf call() in lintr codebase --- R/expect_length_linter.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/expect_length_linter.R b/R/expect_length_linter.R index 74138feb34..d9e6236b7b 100644 --- a/R/expect_length_linter.R +++ b/R/expect_length_linter.R @@ -22,13 +22,13 @@ #' @export expect_length_linter <- function() { # TODO(#2465): also catch expect_true(length(x) == 1) - xpath <- sprintf(" + xpath <- " following-sibling::expr[ expr[1][SYMBOL_FUNCTION_CALL[text() = 'length']] and (position() = 1 or preceding-sibling::expr[NUM_CONST]) ] /parent::expr[not(SYMBOL_SUB[text() = 'info' or contains(text(), 'label')])] - ") + " Linter(linter_level = "expression", function(source_expression) { xml_calls <- source_expression$xml_find_function_calls(c("expect_equal", "expect_identical")) From cc37ebc25de321cc3f993ca0f67449c97bcbb8cb Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:33:30 +0200 Subject: [PATCH 06/20] Simplify call_xpath() Since it is now handled by !is.na(fmt) --- R/sprintf_linter.R | 4 ---- 1 file changed, 4 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index 2b7a32beb0..bc39faedfd 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -30,10 +30,6 @@ sprintf_linter <- function() { fmt_by_name_xpath <- "SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/STR_CONST" call_xpath <- glue::glue(" parent::expr[ - ( - OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST - or {fmt_by_name_xpath} - ) and not(expr/SYMBOL[text() = '...']) ]") From 8be9818a659fcc33e7abd2b25cf42091364bd192 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:34:48 +0200 Subject: [PATCH 07/20] Use fmt_by_name_xpath directly without assigning --- R/sprintf_linter.R | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index bc39faedfd..a744cf774e 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -27,7 +27,6 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export sprintf_linter <- function() { - fmt_by_name_xpath <- "SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/STR_CONST" call_xpath <- glue::glue(" parent::expr[ not(expr/SYMBOL[text() = '...']) @@ -99,7 +98,10 @@ sprintf_linter <- function() { sprintf_calls <- xml_find_all(xml_calls, call_xpath) in_pipeline <- !is.na(xml_find_first(sprintf_calls, in_pipe_xpath)) - fmt_by_name <- get_r_string(sprintf_calls, fmt_by_name_xpath) + fmt_by_name <- get_r_string( + sprintf_calls, + "SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/STR_CONST" + ) fmt_by_pos <- get_r_string( sprintf_calls, "OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST" From 83081e9ef3f16b59ba9b679e66cff01dd28ad538 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:40:37 +0200 Subject: [PATCH 08/20] Remove nested ifelse() --- R/sprintf_linter.R | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index a744cf774e..9f83050e59 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -102,16 +102,13 @@ sprintf_linter <- function() { sprintf_calls, "SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/STR_CONST" ) - fmt_by_pos <- get_r_string( - sprintf_calls, - "OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST" - ) - fmt_by_pos_piped <- get_r_string( - sprintf_calls, - "preceding-sibling::*[2]/STR_CONST" + fmt_by_pos <- ifelse( + in_pipeline, + get_r_string(sprintf_calls, "preceding-sibling::*[2]/STR_CONST"), + get_r_string(sprintf_calls, "OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST") ) - fmt <- ifelse(!is.na(fmt_by_name), fmt_by_name, ifelse(in_pipeline, fmt_by_pos_piped, fmt_by_pos)) + fmt <- ifelse(!is.na(fmt_by_name), fmt_by_name, fmt_by_pos) constant_fmt <- !is.na(fmt) & !grepl("%[^%]", fmt) constant_fmt_lint <- xml_nodes_to_lints( From c81fae3a495dca3dfb5faf15f225d76694ec3d05 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:45:00 +0200 Subject: [PATCH 09/20] Add example for new lint --- R/sprintf_linter.R | 5 +++++ man/sprintf_linter.Rd | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index 9f83050e59..23ed13d53a 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -12,6 +12,11 @@ #' linters = sprintf_linter() #' ) #' +#' lint( +#' text = 'sprintf("hello")', +#' linters = sprintf_linter() +#' ) +#' #' # okay #' lint( #' text = 'sprintf("hello %s %s %d", x, y, z)', diff --git a/man/sprintf_linter.Rd b/man/sprintf_linter.Rd index 97a3720737..b48e986e54 100644 --- a/man/sprintf_linter.Rd +++ b/man/sprintf_linter.Rd @@ -20,6 +20,11 @@ lint( linters = sprintf_linter() ) +lint( + text = 'sprintf("hello")', + linters = sprintf_linter() +) + # okay lint( text = 'sprintf("hello \%s \%s \%d", x, y, z)', From ffc47fdfbeb37c2a2386c23641bc6db2a8bf578b Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 16:48:04 +0200 Subject: [PATCH 10/20] Remove unnecessary glue() call --- R/sprintf_linter.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index 23ed13d53a..4905d990df 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -32,10 +32,10 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export sprintf_linter <- function() { - call_xpath <- glue::glue(" + call_xpath <- " parent::expr[ not(expr/SYMBOL[text() = '...']) - ]") + ]" pipes <- setdiff(magrittr_pipes, "%$%") in_pipe_xpath <- glue("self::expr[ From 85ce180e39795f05661df3ece10ae863d4c8c581 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 17:01:23 +0200 Subject: [PATCH 11/20] Use expect_no_lint() where appropriate --- tests/testthat/test-sprintf_linter.R | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/testthat/test-sprintf_linter.R b/tests/testthat/test-sprintf_linter.R index 748d1e6ea5..864c2ec653 100644 --- a/tests/testthat/test-sprintf_linter.R +++ b/tests/testthat/test-sprintf_linter.R @@ -4,13 +4,13 @@ patrick::with_parameters_test_that( linter <- sprintf_linter() # NB: using paste0, not sprintf, to avoid escaping '%d' in sprint fmt= - expect_lint(paste0(call_name, "('hello %d', 1)"), NULL, linter) - expect_lint(paste0(call_name, "('hello %d', x)"), NULL, linter) - expect_lint(paste0(call_name, "('hello %d', x + 1)"), NULL, linter) - expect_lint(paste0(call_name, "('hello %d', f(x))"), NULL, linter) - expect_lint(paste0(call_name, "('hello %1$s %1$s', x)"), NULL, linter) - expect_lint(paste0(call_name, "('hello %1$s %1$s %2$d', x, y)"), NULL, linter) - expect_lint(paste0(call_name, "('hello %1$s %1$s %2$d %3$s', x, y, 1.5)"), NULL, linter) + expect_no_lint(paste0(call_name, "('hello %d', 1)"), linter) + expect_no_lint(paste0(call_name, "('hello %d', x)"), linter) + expect_no_lint(paste0(call_name, "('hello %d', x + 1)"), linter) + expect_no_lint(paste0(call_name, "('hello %d', f(x))"), linter) + expect_no_lint(paste0(call_name, "('hello %1$s %1$s', x)"), linter) + expect_no_lint(paste0(call_name, "('hello %1$s %1$s %2$d', x, y)"), linter) + expect_no_lint(paste0(call_name, "('hello %1$s %1$s %2$d %3$s', x, y, 1.5)"), linter) }, .test_name = c("sprintf", "gettextf"), call_name = c("sprintf", "gettextf") @@ -79,12 +79,12 @@ test_that("edge cases are detected correctly", { ) # dots - expect_lint("sprintf('%d %d, %d', id, ...)", NULL, linter) + expect_no_lint("sprintf('%d %d, %d', id, ...)", linter) # TODO(#1265) extend ... detection to at least test for too many arguments. # named argument fmt - expect_lint("sprintf(x, fmt = 'hello %1$s %1$s')", NULL, linter) + expect_no_lint("sprintf(x, fmt = 'hello %1$s %1$s')", linter) expect_lint( "sprintf(x, fmt = 'hello %1$s %1$s %3$d', y)", @@ -93,7 +93,7 @@ test_that("edge cases are detected correctly", { ) # #2131: xml2lang stripped necessary whitespace - expect_lint("sprintf('%s', if (A) '' else y)", NULL, linter) + expect_no_lint("sprintf('%s', if (A) '' else y)", linter) }) local({ @@ -104,13 +104,13 @@ local({ patrick::with_parameters_test_that( "piping into sprintf works", { - expect_lint(paste("x", pipe, "sprintf(fmt = '%s')"), NULL, linter) + expect_no_lint(paste("x", pipe, "sprintf(fmt = '%s')"), linter) # no fmt= specified -> this is just 'sprintf("%s", "%s%s")', which won't lint - expect_lint(paste('"%s"', pipe, 'sprintf("%s%s")'), NULL, linter) + expect_no_lint(paste('"%s"', pipe, 'sprintf("%s%s")'), linter) expect_lint(paste("x", pipe, "sprintf(fmt = '%s%s')"), unused_fmt_msg, linter) # Cannot evaluate statically --> skip - expect_lint(paste("x", pipe, 'sprintf("a")'), NULL, linter) + expect_no_lint(paste("x", pipe, 'sprintf("a")'), linter) # Nested pipes expect_lint( paste("'%%sb'", pipe, "sprintf('%s')", pipe, "sprintf('a')"), From 8632a40e3ecc786647dae1bc872ba276190c76f2 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sat, 26 Jul 2025 20:19:05 +0200 Subject: [PATCH 12/20] Add test for constant '%' --- tests/testthat/test-sprintf_linter.R | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/test-sprintf_linter.R b/tests/testthat/test-sprintf_linter.R index 864c2ec653..7ee781a399 100644 --- a/tests/testthat/test-sprintf_linter.R +++ b/tests/testthat/test-sprintf_linter.R @@ -25,6 +25,7 @@ patrick::with_parameters_test_that( expect_lint(paste0(call_name, "('hello', 1)"), "constant", linter) expect_lint(paste0(call_name, "('hello')"), "constant", linter) + expect_lint(paste0(call_name, "('%%')"), "constant", linter) expect_lint( paste0(call_name, "('hello %d', 'a')"), From 906eb15ceca4f17fbc72fb236d1ed25284becde2 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sun, 27 Jul 2025 14:14:16 +0200 Subject: [PATCH 13/20] Test and handle %% case better --- R/sprintf_linter.R | 2 +- tests/testthat/test-sprintf_linter.R | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index 4905d990df..22e64b02a6 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -114,7 +114,7 @@ sprintf_linter <- function() { ) fmt <- ifelse(!is.na(fmt_by_name), fmt_by_name, fmt_by_pos) - constant_fmt <- !is.na(fmt) & !grepl("%[^%]", fmt) + constant_fmt <- !is.na(fmt) & !grepl("%", gsub("%%", "", fmt, fixed = TRUE), fixed = TRUE) constant_fmt_lint <- xml_nodes_to_lints( sprintf_calls[constant_fmt], diff --git a/tests/testthat/test-sprintf_linter.R b/tests/testthat/test-sprintf_linter.R index 7ee781a399..71b81600e1 100644 --- a/tests/testthat/test-sprintf_linter.R +++ b/tests/testthat/test-sprintf_linter.R @@ -25,7 +25,10 @@ patrick::with_parameters_test_that( expect_lint(paste0(call_name, "('hello', 1)"), "constant", linter) expect_lint(paste0(call_name, "('hello')"), "constant", linter) - expect_lint(paste0(call_name, "('%%')"), "constant", linter) + expect_lint(paste0(call_name, "('100%% automated')"), "constant", linter) + expect_lint(paste0(call_name, "('100%%%% automated')"), "constant", linter) + expect_lint(paste0(call_name, "('100%%%s')"), "too few", linter) + expect_lint(paste0(call_name, "('100%%%%s', x)"), "constant", linter) expect_lint( paste0(call_name, "('hello %d', 'a')"), @@ -73,10 +76,10 @@ test_that("edge cases are detected correctly", { sprintf( 'test fmt %s', # this is a comment 2 - ) - "), - NULL, - linter + ) + "), + NULL, + linter ) # dots @@ -95,6 +98,8 @@ test_that("edge cases are detected correctly", { # #2131: xml2lang stripped necessary whitespace expect_no_lint("sprintf('%s', if (A) '' else y)", linter) + + expect_no_lint("sprintf('100%%%s', x)", linter) }) local({ @@ -115,7 +120,7 @@ local({ # Nested pipes expect_lint( paste("'%%sb'", pipe, "sprintf('%s')", pipe, "sprintf('a')"), - if (getRversion() >= "4.1.0") list(column_number = nchar(paste("'%%sb'", pipe, "x")), message = unused_arg_msg), + if (getRversion() >= "4.1.0") list(column_number = nchar(paste("'%%sb'", pipe, "x")), message = "constant"), linter ) expect_lint( @@ -134,6 +139,7 @@ local({ ) }) + test_that("lints vectorize", { skip_if_not_r_version("4.1.0") From 5b15285f158afb3f5a562f9edc78b756c59d4778 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sun, 27 Jul 2025 15:35:04 +0200 Subject: [PATCH 14/20] Use actual function name in lint message --- R/sprintf_linter.R | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index 22e64b02a6..6728c6715b 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -116,10 +116,15 @@ sprintf_linter <- function() { fmt <- ifelse(!is.na(fmt_by_name), fmt_by_name, fmt_by_pos) constant_fmt <- !is.na(fmt) & !grepl("%", gsub("%%", "", fmt, fixed = TRUE), fixed = TRUE) + fct_name <- xml_text(xml_find_all( + sprintf_calls, + "expr/SYMBOL_FUNCTION_CALL[1]/text()" + )) + constant_fmt_lint <- xml_nodes_to_lints( sprintf_calls[constant_fmt], source_expression = source_expression, - lint_message = "A constant string is used and the sprintf call can be removed", + lint_message = sprintf("A constant string is used and the %s call can be removed", fct_name[constant_fmt]), type = "warning" ) From 39240dec211854130acc53471f522b9588c88e3f Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Sun, 27 Jul 2025 15:50:28 +0200 Subject: [PATCH 15/20] Convert one more expect_lint(, NULL, ) --- tests/testthat/test-sprintf_linter.R | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/testthat/test-sprintf_linter.R b/tests/testthat/test-sprintf_linter.R index 71b81600e1..35b5abb8e4 100644 --- a/tests/testthat/test-sprintf_linter.R +++ b/tests/testthat/test-sprintf_linter.R @@ -71,15 +71,14 @@ test_that("edge cases are detected correctly", { linter <- sprintf_linter() # works with multi-line sprintf and comments - expect_lint( + expect_no_lint( trim_some(" sprintf( 'test fmt %s', # this is a comment 2 - ) - "), - NULL, - linter + ) + "), + linter ) # dots @@ -139,7 +138,6 @@ local({ ) }) - test_that("lints vectorize", { skip_if_not_r_version("4.1.0") From ee22ed7150ff2a10fa0c0512196901c4a12c324b Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Mon, 28 Jul 2025 10:53:57 +0200 Subject: [PATCH 16/20] Support comments at various positions --- R/sprintf_linter.R | 12 +++++++----- tests/testthat/test-sprintf_linter.R | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index 6728c6715b..73eb683f30 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -38,9 +38,11 @@ sprintf_linter <- function() { ]" pipes <- setdiff(magrittr_pipes, "%$%") - in_pipe_xpath <- glue("self::expr[ - preceding-sibling::*[1][self::PIPE or self::SPECIAL[{ xp_text_in_table(pipes) }]] - ]") + in_pipe_xpath <- glue( + "self::expr[ + preceding-sibling::*[not(self::COMMENT)][1][self::PIPE or self::SPECIAL[{ xp_text_in_table(pipes) }]] + ]" + ) is_missing <- function(x) is.symbol(x) && !nzchar(x) @@ -86,7 +88,7 @@ sprintf_linter <- function() { arg_idx <- 2L:length(parsed_expr) parsed_expr[arg_idx + 1L] <- parsed_expr[arg_idx] names(parsed_expr)[arg_idx + 1L] <- arg_names[arg_idx] - parsed_expr[[2L]] <- xml2lang(xml_find_first(xml, "preceding-sibling::*[2]")) + parsed_expr[[2L]] <- xml2lang(xml_find_first(xml, "preceding-sibling::*[not(self::COMMENT)][2]")) names(parsed_expr)[2L] <- "" } parsed_expr <- zap_extra_args(parsed_expr) @@ -109,7 +111,7 @@ sprintf_linter <- function() { ) fmt_by_pos <- ifelse( in_pipeline, - get_r_string(sprintf_calls, "preceding-sibling::*[2]/STR_CONST"), + get_r_string(sprintf_calls, "preceding-sibling::*[not(self::COMMENT)][2]/STR_CONST"), get_r_string(sprintf_calls, "OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST") ) diff --git a/tests/testthat/test-sprintf_linter.R b/tests/testthat/test-sprintf_linter.R index 35b5abb8e4..2f0e958864 100644 --- a/tests/testthat/test-sprintf_linter.R +++ b/tests/testthat/test-sprintf_linter.R @@ -81,6 +81,25 @@ test_that("edge cases are detected correctly", { linter ) + expect_no_lint( + trim_some(" + 'test fmt %s' |> # this is a pipe comment + sprintf( # this is an opening comment + 2 # this is a mid-call comment + ) + "), + linter + ) + + expect_lint( + trim_some(" + 'test fmt' |> # this is a pipe comment + sprintf() + "), + "constant", + linter + ) + # dots expect_no_lint("sprintf('%d %d, %d', id, ...)", linter) From 72106247559f7e487a6662c87be6680cac4938ca Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Mon, 28 Jul 2025 11:36:36 +0200 Subject: [PATCH 17/20] Use action/reason format for message --- R/sprintf_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index 73eb683f30..d213760e3e 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -126,7 +126,7 @@ sprintf_linter <- function() { constant_fmt_lint <- xml_nodes_to_lints( sprintf_calls[constant_fmt], source_expression = source_expression, - lint_message = sprintf("A constant string is used and the %s call can be removed", fct_name[constant_fmt]), + lint_message = sprintf("%s call can be removed when a constant string is provided.", fct_name[constant_fmt]), type = "warning" ) From 6be92d3a4af211557a934b480c4037df98242bb5 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Mon, 28 Jul 2025 18:30:35 +0200 Subject: [PATCH 18/20] Use dedicated xp_call_name() function to get fct_name --- R/sprintf_linter.R | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/R/sprintf_linter.R b/R/sprintf_linter.R index d213760e3e..8cc33d2cb4 100644 --- a/R/sprintf_linter.R +++ b/R/sprintf_linter.R @@ -118,10 +118,7 @@ sprintf_linter <- function() { fmt <- ifelse(!is.na(fmt_by_name), fmt_by_name, fmt_by_pos) constant_fmt <- !is.na(fmt) & !grepl("%", gsub("%%", "", fmt, fixed = TRUE), fixed = TRUE) - fct_name <- xml_text(xml_find_all( - sprintf_calls, - "expr/SYMBOL_FUNCTION_CALL[1]/text()" - )) + fct_name <- xp_call_name(sprintf_calls) constant_fmt_lint <- xml_nodes_to_lints( sprintf_calls[constant_fmt], From 15526db124ba504ab0f75476be8f290b6a42af39 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Mon, 28 Jul 2025 18:34:15 +0200 Subject: [PATCH 19/20] Document new sprintf_linter() lints in NEWS --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index d6b1dae8d2..bf4bf78c2e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -47,6 +47,8 @@ * `fixed_regex_linter()` recognizes usage of the new (R 4.5.0) `grepv()` wrapper of `grep()`; `regex_subset_linter()` also recommends `grepv()` alternatives (#2855, @MichaelChirico). * `object_usage_linter()` lints missing packages that may cause false positives (#2872, @AshesITR) * New argument `include_s4_slots` for the `xml_find_function_calls()` entry in the `get_source_expressions()` to govern whether calls of the form `s4Obj@fun()` are included in the result (#2820, @MichaelChirico). +* `sprintf_linter()` lints `sprintf()` and `gettextf()` calls when a constant +strings is passed to `fmt` (#2894, @Bisaloo). ### New linters From 1447a292bc1348616d6608ff16187b5b1610fdcc Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Jul 2025 09:36:13 -0700 Subject: [PATCH 20/20] one line in NEWS --- NEWS.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index bf4bf78c2e..7bd7fb2340 100644 --- a/NEWS.md +++ b/NEWS.md @@ -47,8 +47,7 @@ * `fixed_regex_linter()` recognizes usage of the new (R 4.5.0) `grepv()` wrapper of `grep()`; `regex_subset_linter()` also recommends `grepv()` alternatives (#2855, @MichaelChirico). * `object_usage_linter()` lints missing packages that may cause false positives (#2872, @AshesITR) * New argument `include_s4_slots` for the `xml_find_function_calls()` entry in the `get_source_expressions()` to govern whether calls of the form `s4Obj@fun()` are included in the result (#2820, @MichaelChirico). -* `sprintf_linter()` lints `sprintf()` and `gettextf()` calls when a constant -strings is passed to `fmt` (#2894, @Bisaloo). +* `sprintf_linter()` lints `sprintf()` and `gettextf()` calls when a constant string is passed to `fmt` (#2894, @Bisaloo). ### New linters