diff --git a/NEWS.md b/NEWS.md index 4a6599e81..e85efaeb1 100644 --- a/NEWS.md +++ b/NEWS.md @@ -22,6 +22,7 @@ * `.lintr` config validation correctly accepts regular expressions which only compile under `perl = TRUE` (#2375, @MichaelChirico). These have always been valid (since `rex::re_matches()`, which powers the lint exclusion logic, also uses this setting), but the new up-front validation in v3.1.1 incorrectly used `perl = FALSE`. * `.lintr` configs set by option `lintr.linter_file` or environment variable `R_LINTR_LINTER_FILE` can point to subdirectories (#2512, @MichaelChirico). * `indentation_linter()` returns `ranges[1L]==1L` when the offending line has 0 spaces (#2550, @MichaelChirico). +* `literal_coercion_linter()` doesn't surface a warning about NAs during coercion for code like `as.integer("a")` (#2566, @MichaelChirico). ## Changes to default linters diff --git a/R/literal_coercion_linter.R b/R/literal_coercion_linter.R index 63eb245c3..bfc93fefe 100644 --- a/R/literal_coercion_linter.R +++ b/R/literal_coercion_linter.R @@ -97,9 +97,14 @@ literal_coercion_linter <- function() { needs_prefix <- is_rlang_coercer & !startsWith(coercion_str, "rlang::") coercion_str[needs_prefix] <- paste0("rlang::", coercion_str[needs_prefix]) } - # the linter logic & rlang requirement should ensure that it's safe to run eval() here + # the linter logic & rlang requirement should ensure that it's safe to run eval() here; + # suppressWarnings() is for cases like 'as.integer("a")' which have an NA result, #2566. # TODO(#2473): Avoid a recommendation like '1' that clashes with implicit_integer_linter(). - literal_equivalent_str <- vapply(str2expression(coercion_str), function(expr) deparse1(eval(expr)), character(1L)) + literal_equivalent_str <- vapply( + str2expression(coercion_str), + function(expr) deparse1(suppressWarnings(eval(expr))), + character(1L) + ) lint_message <- sprintf( "Use %s instead of %s, i.e., use literals directly where possible, instead of coercion.", literal_equivalent_str, report_str diff --git a/tests/testthat/test-literal_coercion_linter.R b/tests/testthat/test-literal_coercion_linter.R index 247416a2c..aa5f752fb 100644 --- a/tests/testthat/test-literal_coercion_linter.R +++ b/tests/testthat/test-literal_coercion_linter.R @@ -1,5 +1,5 @@ test_that("literal_coercion_linter skips allowed usages", { - linter <- line_length_linter() + linter <- literal_coercion_linter() # naive xpath includes the "_f0" here as a literal expect_lint('as.numeric(x$"_f0")', NULL, linter) @@ -23,7 +23,7 @@ test_that("literal_coercion_linter skips allowed usages", { }) test_that("literal_coercion_linter skips allowed rlang usages", { - linter <- line_length_linter() + linter <- literal_coercion_linter() expect_lint("int(1, 2.0, 3)", NULL, linter) expect_lint("chr('e', 'ab', 'xyz')", NULL, linter) @@ -40,6 +40,18 @@ test_that("literal_coercion_linter skips quoted keyword arguments", { expect_lint("as.numeric(foo('a' = 1))", NULL, literal_coercion_linter()) }) +test_that("no warnings surfaced by running coercion", { + linter <- literal_coercion_linter() + + expect_no_warning( + expect_lint("as.integer('a')", "Use NA_integer_", linter) + ) + + expect_no_warning( + expect_lint("as.integer(2147483648)", "Use NA_integer_", linter) + ) +}) + skip_if_not_installed("tibble") patrick::with_parameters_test_that( "literal_coercion_linter blocks simple disallowed usages",