Update sprintf_linter() to lint constant strings

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"))

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)',
@@ -29,20 +34,12 @@
 sprintf_linter <- function() {
   call_xpath <- "
   parent::expr[
-    (
-      OP-LEFT-PAREN/following-sibling::expr[1]/STR_CONST or
-      SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/STR_CONST
-    ) 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
-    )
   ]")
 
   is_missing <- function(x) is.symbol(x) && !nzchar(x)
@@ -104,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))
+
+    fmt_by_name <- get_r_string(
+      sprintf_calls,
+      "SYMBOL_SUB[text() = 'fmt']/following-sibling::expr[1]/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")
+    )
 
-    sprintf_warning <- vapply(sprintf_calls, capture_sprintf_warning, character(1L))
+    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(
+      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)
   })
 }

tests/testthat/test-sprintf_linter.R

@@ -4,14 +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')"), 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)
-    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")
@@ -23,7 +22,10 @@ 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, "('%%')"), "constant", linter)
 
     expect_lint(
       paste0(call_name, "('hello %d', 'a')"),
@@ -78,12 +80,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)",
@@ -92,7 +94,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({
@@ -103,13 +105,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')"),