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)',
@@ -27,22 +32,14 @@
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
 sprintf_linter <- function() {
-  call_xpath <- "
+  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
-    ) 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))
 
-    sprintf_warning <- vapply(sprintf_calls, capture_sprintf_warning, character(1L))
+    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")
+    )
+
+    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,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')"),