New literal_coercion_linter

DESCRIPTION

@@ -26,6 +26,7 @@ Imports:
     jsonlite,
     knitr,
     stats,
+    tibble,
     utils,
     xml2 (>= 1.0.0),
     xmlparsedata (>= 1.0.3)
@@ -86,6 +87,7 @@ Collate:
     'lint.R'
     'linter_tag_docs.R'
     'linter_tags.R'
+    'literal_coercion_linter.R'
     'make_linter_from_regex.R'
     'methods.R'
     'missing_argument_linter.R'

NAMESPACE

@@ -53,6 +53,7 @@ export(line_length_linter)
 export(lint)
 export(lint_dir)
 export(lint_package)
+export(literal_coercion_linter)
 export(missing_argument_linter)
 export(missing_package_linter)
 export(namespace_linter)

NEWS.md

@@ -102,6 +102,7 @@ function calls. (#850, #851, @renkun-ken)
    * `any_is_na_linter()` Require usage of `anyNA(x)` over `any(is.na(x))`
    * `outer_negation_linter()` Require usage of `!any(x)` over `all(!x)` and `!all(x)` over `any(!x)`
    * `numeric_leading_zero_linter()` Require a leading `0` in fractional numeric constants, e.g. `0.1` instead of `.1`
+   * `literal_coercion_linter()` Require using correctly-typed literals instead of direct coercion, e.g. `1L` instead of `as.numeric(1)`
 * `assignment_linter()` now lints right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to lint `<<-` and `->>`; `allow_right_assign` toggles whether to lint `->` and `->>` (#915, @michaelchirico)
 * `infix_spaces_linter()` gains argument `exclude_operators` to disable lints on selected infix operators. By default, all "low-precedence" operators throw lints; see `?infix_spaces_linter` for an enumeration of these. (#914 @michaelchirico)
 * `infix_spaces_linter()` now throws a lint on `a~b` and `function(a=1) {}` (#930, @michaelchirico)

R/literal_coercion_linter.R

@@ -0,0 +1,54 @@
+#' Require usage of correctly-typed literals over literal coercions
+#'
+#' `as.integer(1)` is the same as `1L` but the latter is more concise and
+#'   gets typed correctly at compilation.
+#'
+#' The same applies to missing sentinels like `NA` -- typically, it is not
+#'   necessary to specify the storage type of `NA`, but when it is, prefer
+#'   using the typed version (e.g. `NA_real_`) instead of a coercion
+#'   (like `as.numeric(NA)`).
+#'
+#' @evalRd rd_tags("literal_coercion_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+literal_coercion_linter <- function() {
+  Linter(function(source_file) {
+    if (length(source_file$parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    coercers <- xp_text_in_table(paste0(
+      "as.",
+      c("logical", "integer", "numeric", "double", "character")
+    ))
+    # notes for clarification:
+    #  - as.integer(1e6) is arguably easier to read than 1000000L
+    #  - in x$"abc", the "abc" STR_CONST is at the top level, so exclude OP-DOLLAR
+    #  - need condition against STR_CONST w/ EQ_SUB to skip quoted keyword arguments (see tests)
+    xpath <- glue::glue("//expr[
+      expr[SYMBOL_FUNCTION_CALL[ {coercers} ]]
+      and expr[2][
+        not(OP-DOLLAR)
+        and (
+          NUM_CONST[not(contains(translate(text(), 'E', 'e'), 'e'))]
+          or STR_CONST[not(following-sibling::*[1][self::EQ_SUB])]
+        )
+      ]
+    ]")
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    return(lapply(
+      bad_expr,
+      xml_nodes_to_lint,
+      source_file = source_file,
+      lint_message = paste(
+        "Use literals directly where possible, instead of coercion.",
+        "c.f. 1L instead of as.integer(1), or NA_real_ instead of as.numeric(NA)."
+      ),
+      type = "warning"
+    ))
+  })
+}

inst/lintr/linters.csv

@@ -26,6 +26,7 @@ if_else_match_braces_linter,default style readability
 implicit_integer_linter,style consistency best_practices
 infix_spaces_linter,style readability default
 line_length_linter,style readability default configurable
+literal_coercion_linter,best_practices efficiency
 missing_argument_linter,correctness common_mistakes configurable
 missing_package_linter,robustness common_mistakes
 namespace_linter,correctness robustness configurable

tests/testthat/test-literal_coercion_linter.R

@@ -0,0 +1,50 @@
+test_that("literal_coercion_linter skips allowed usages", {
+  # naive xpath includes the "_f0" here as a literal
+  expect_lint('as.numeric(x$"_f0")', NULL, literal_coercion_linter())
+  # only examine the first method for as.<type> methods
+  expect_lint("as.character(as.Date(x), '%Y%m%d')", NULL, literal_coercion_linter())
+
+  # we are as yet agnostic on whether to prefer literals over coerced vectors
+  expect_lint("as.integer(c(1, 2, 3))", NULL, literal_coercion_linter())
+  # even more ambiguous for character vectors like here, where quotes are much
+  #   more awkward to type than a sequence of numbers
+  expect_lint("as.character(c(1, 2, 3))", NULL, literal_coercion_linter())
+  # not possible to declare raw literals
+  expect_lint("as.raw(c(1, 2, 3))", NULL, literal_coercion_linter())
+  # also not taking a stand on as.complex(0) vs. 0 + 0i
+  expect_lint("as.complex(0)", NULL, literal_coercion_linter())
+  # ditto for as.integer(1e6) vs. 1000000L
+  expect_lint("as.integer(1e6)", NULL, literal_coercion_linter())
+  # ditto for as.numeric(1:3) vs. c(1, 2, 3)
+  expect_lint("as.numeric(1:3)", NULL, literal_coercion_linter())
+})
+
+skip_if_not_installed("tibble")
+skip_if_not_installed("patrick")
+patrick::with_parameters_test_that(
+  "literal_coercion_linter blocks simple disallowed usages",
+  expect_lint(
+    sprintf("as.%s(%s)", out_type, input),
+    rex::rex("Use literals directly where possible, instead of coercion."),
+    literal_coercion_linter()
+  ),
+  .cases = tibble::tribble(
+        ~.test_name,   ~out_type,   ~input,
+    "lgl, from int",   "logical",     "1L",
+    "lgl, from num",   "logical",      "1",
+    "lgl, from chr",   "logical", '"true"',
+    "int, from num",   "integer",      "1",
+    "num, from num",   "numeric",      "1",
+    "dbl, from num",    "double",      "1",
+    "chr, from num", "character",      "1",
+    # affirmatively lint as.<type>(NA) should be NA_<type>_
+    "int, from NA",    "integer",     "NA",
+    "num, from NA",    "numeric",     "NA",
+    "dbl, from NA",     "double",     "NA",
+    "chr, from NA",  "character",     "NA",
+  )
+)
+
+test_that("literal_coercion_linter skips quoted keyword arguments", {
+  expect_lint("as.numeric(foo('a' = 1))", NULL, literal_coercion_linter())
+})