diff --git a/NEWS.md b/NEWS.md index 477b8e61c..3385fa3b4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,6 +7,7 @@ ## New and improved features * More helpful errors for invalid configs (#2253, @MichaelChirico). +* `library_call_linter()` is extended to encourage all packages to be attached with `library(symbol)`, not `library("symbol", character.only = TRUE)` or "vectorized" approaches looping over package names (part of #884, @MichaelChirico). ### New linters diff --git a/R/library_call_linter.R b/R/library_call_linter.R index 80b8c3a47..6617e1bb8 100644 --- a/R/library_call_linter.R +++ b/R/library_call_linter.R @@ -1,6 +1,10 @@ #' Library call linter #' -#' Force library calls to all be at the top of the script. +#' This linter covers several rules related to [library()] calls: +#' +#' - Enforce such calls to all be at the top of the script. +#' - Block usage of argument `character.only`, in particular +#' for loading packages in a loop. #' #' @param allow_preamble Logical, default `TRUE`. If `FALSE`, #' no code is allowed to precede the first `library()` call, @@ -8,39 +12,53 @@ #' calls must follow consecutively after the first one. #' @examples #' # will produce lints +#' +#' code <- "library(dplyr)\nprint('test')\nlibrary(tidyr)" +#' writeLines(code) #' lint( -#' text = " -#' library(dplyr) -#' print('test') -#' library(tidyr) -#' ", +#' text = code, #' linters = library_call_linter() #' ) #' #' lint( -#' text = " -#' library(dplyr) -#' print('test') -#' library(tidyr) -#' library(purrr) -#' ", +#' text = "library('dplyr', character.only = TRUE)", +#' linters = library_call_linter() +#' ) +#' +#' code <- paste( +#' "pkg <- c('dplyr', 'tibble')", +#' "sapply(pkg, library, character.only = TRUE)", +#' sep = "\n" +#' ) +#' writeLines(code) +#' lint( +#' text = code, #' linters = library_call_linter() #' ) #' #' # okay +#' code <- "library(dplyr)\nprint('test')" +#' writeLines(code) #' lint( -#' text = " -#' library(dplyr) -#' print('test') -#' ", +#' text = code, #' linters = library_call_linter() #' ) #' +#' code <- "# comment\nlibrary(dplyr)" #' lint( -#' text = " -#' # comment -#' library(dplyr) -#' ", +#' text = code, +#' linters = library_call_linter() +#' ) +#' +#' code <- paste( +#' "foo <- function(pkg) {", +#' " sapply(pkg, library, character.only = TRUE)", +#' "}", +#' sep = "\n" +#' ) +#' writeLines(code) +#' lint( +#' text = code, #' linters = library_call_linter() #' ) #' @@ -56,7 +74,7 @@ library_call_linter <- function(allow_preamble = TRUE) { glue("@line1 > //SYMBOL_FUNCTION_CALL[{ attach_call }][1]/@line1") ) } - xpath <- glue(" + upfront_call_xpath <- glue(" //SYMBOL_FUNCTION_CALL[{ attach_call }][last()] /preceding::expr /SYMBOL_FUNCTION_CALL[{ unsuppressed_call }][last()] @@ -64,6 +82,35 @@ library_call_linter <- function(allow_preamble = TRUE) { /parent::expr ") + # STR_CONST: block library|require("..."), i.e., supplying a string literal + # ancestor::expr[FUNCTION]: Skip usages inside functions a la {knitr} + char_only_direct_xpath <- " + //SYMBOL_FUNCTION_CALL[text() = 'library' or text() = 'require'] + /parent::expr + /parent::expr[ + expr[2][STR_CONST] + or ( + SYMBOL_SUB[text() = 'character.only'] + and not(ancestor::expr[FUNCTION]) + ) + ] + " + + bad_indirect_funs <- c("do.call", "lapply", "sapply", "map", "walk") + call_symbol_cond <- " + SYMBOL[text() = 'library' or text() = 'require'] + or STR_CONST[text() = '\"library\"' or text() = '\"require\"'] + " + char_only_indirect_xpath <- glue(" + //SYMBOL_FUNCTION_CALL[{ xp_text_in_table(bad_indirect_funs) }] + /parent::expr + /parent::expr[ + not(ancestor::expr[FUNCTION]) + and expr[{ call_symbol_cond }] + ] + ") + call_symbol_path <- glue("./expr[{call_symbol_cond}]") + Linter(function(source_expression) { if (!is_lint_level(source_expression, "file")) { return(list()) @@ -71,15 +118,49 @@ library_call_linter <- function(allow_preamble = TRUE) { xml <- source_expression$full_xml_parsed_content - bad_expr <- xml_find_all(xml, xpath) + upfront_call_expr <- xml_find_all(xml, upfront_call_xpath) - call_name <- xp_call_name(bad_expr) + call_name <- xp_call_name(upfront_call_expr) - xml_nodes_to_lints( - bad_expr, + upfront_call_lints <- xml_nodes_to_lints( + upfront_call_expr, source_expression = source_expression, lint_message = sprintf("Move all %s calls to the top of the script.", call_name), type = "warning" ) + + char_only_direct_expr <- xml_find_all(xml, char_only_direct_xpath) + char_only_direct_calls <- xp_call_name(char_only_direct_expr) + character_only <- + xml_find_first(char_only_direct_expr, "./SYMBOL_SUB[text() = 'character.only']") + char_only_direct_msg_fmt <- ifelse( + is.na(character_only), + "Use symbols, not strings, in %s calls.", + "Use symbols in %s calls to avoid the need for 'character.only'." + ) + char_only_direct_msg <- + sprintf(as.character(char_only_direct_msg_fmt), char_only_direct_calls) + char_only_direct_lints <- xml_nodes_to_lints( + char_only_direct_expr, + source_expression = source_expression, + lint_message = char_only_direct_msg, + type = "warning" + ) + + char_only_indirect_expr <- xml_find_all(xml, char_only_indirect_xpath) + char_only_indirect_lib_calls <- get_r_string(char_only_indirect_expr, call_symbol_path) + char_only_indirect_loop_calls <- xp_call_name(char_only_indirect_expr) + char_only_indirect_msg <- sprintf( + "Call %s() directly, not vectorized with %s().", + char_only_indirect_lib_calls, char_only_indirect_loop_calls + ) + char_only_indirect_lints <- xml_nodes_to_lints( + char_only_indirect_expr, + source_expression = source_expression, + lint_message = char_only_indirect_msg, + type = "warning" + ) + + c(upfront_call_lints, char_only_direct_lints, char_only_indirect_lints) }) } diff --git a/man/library_call_linter.Rd b/man/library_call_linter.Rd index dcf6e40a0..02b65a5c9 100644 --- a/man/library_call_linter.Rd +++ b/man/library_call_linter.Rd @@ -13,43 +13,64 @@ otherwise some setup code is allowed, but all \code{library()} calls must follow consecutively after the first one.} } \description{ -Force library calls to all be at the top of the script. +This linter covers several rules related to \code{\link[=library]{library()}} calls: +} +\details{ +\itemize{ +\item Enforce such calls to all be at the top of the script. +\item Block usage of argument \code{character.only}, in particular +for loading packages in a loop. +} } \examples{ # will produce lints + +code <- "library(dplyr)\nprint('test')\nlibrary(tidyr)" +writeLines(code) lint( - text = " - library(dplyr) - print('test') - library(tidyr) - ", + text = code, linters = library_call_linter() ) lint( - text = " - library(dplyr) - print('test') - library(tidyr) - library(purrr) - ", + text = "library('dplyr', character.only = TRUE)", + linters = library_call_linter() +) + +code <- paste( + "pkg <- c('dplyr', 'tibble')", + "sapply(pkg, library, character.only = TRUE)", + sep = "\n" +) +writeLines(code) +lint( + text = code, linters = library_call_linter() ) # okay +code <- "library(dplyr)\nprint('test')" +writeLines(code) +lint( + text = code, + linters = library_call_linter() +) + +code <- "# comment\nlibrary(dplyr)" lint( - text = " - library(dplyr) - print('test') - ", + text = code, linters = library_call_linter() ) +code <- paste( + "foo <- function(pkg) {", + " sapply(pkg, library, character.only = TRUE)", + "}", + sep = "\n" +) +writeLines(code) lint( - text = " - # comment - library(dplyr) - ", + text = code, linters = library_call_linter() ) diff --git a/tests/testthat/test-library_call_linter.R b/tests/testthat/test-library_call_linter.R index 17bd30f42..429125e5a 100644 --- a/tests/testthat/test-library_call_linter.R +++ b/tests/testthat/test-library_call_linter.R @@ -128,31 +128,31 @@ test_that("require() treated the same as library()", { lint_message_require <- rex::rex("Move all require calls to the top of the script.") expect_lint( - trim_some(' + trim_some(" library(dplyr) - require("tidyr") - '), + require(tidyr) + "), NULL, linter ) expect_lint( - trim_some(' + trim_some(" library(dplyr) print(letters) - require("tidyr") - '), + require(tidyr) + "), list(lint_message_require, line_number = 3L), linter ) expect_lint( - trim_some(' + trim_some(" library(dplyr) print(letters) library(dbplyr) - require("tidyr") - '), + require(tidyr) + "), list( list(lint_message_library, line_number = 3L), list(lint_message_require, line_number = 4L) @@ -216,3 +216,98 @@ test_that("allow_preamble applies as intended", { expect_lint(lines, NULL, linter_preamble) expect_lint(lines, lint_msg, linter_no_preamble) }) + +test_that("skips allowed usages of library()/character.only=TRUE", { + linter <- library_call_linter() + + expect_lint("library(data.table)", NULL, linter) + expect_lint("function(pkg) library(pkg, character.only = TRUE)", NULL, linter) + expect_lint("function(pkgs) sapply(pkgs, require, character.only = TRUE)", NULL, linter) +}) + +test_that("blocks disallowed usages of strings in library()/require()", { + linter <- library_call_linter() + + expect_lint( + 'library("data.table")', + rex::rex("Use symbols, not strings, in library calls."), + linter + ) + + expect_lint( + 'library("data.table", character.only = TRUE)', + rex::rex("Use symbols in library calls", anything, "character.only"), + linter + ) + + expect_lint( + 'suppressWarnings(library("data.table", character.only = TRUE))', + rex::rex("Use symbols in library calls", anything, "character.only"), + linter + ) + + expect_lint( + "do.call(library, list(data.table))", + rex::rex("Call library() directly, not vectorized with do.call()"), + linter + ) + + expect_lint( + 'do.call("library", list(data.table))', + rex::rex("Call library() directly, not vectorized with do.call()"), + linter + ) + + expect_lint( + 'lapply("data.table", library, character.only = TRUE)', + rex::rex("Call library() directly, not vectorized with lapply()"), + linter + ) + + expect_lint( + 'purr::map("data.table", library, character.only = TRUE)', + rex::rex("Call library() directly, not vectorized with map()"), + linter + ) +}) + +test_that("character.only=TRUE is caught with multiple-line source", { + expect_lint( + trim_some(' + suppressWarnings(library( + "data.table", + character.only = TRUE + )) + '), + rex::rex("Use symbols in library calls", anything, "character.only"), + library_call_linter() + ) +}) + +test_that("character.only=TRUE is caught inside purrr::walk as well", { + expect_lint( + 'purr::walk("data.table", library, character.only = TRUE)', + rex::rex("Call library() directly, not vectorized with walk()"), + library_call_linter() + ) +}) + +test_that("multiple lints are generated correctly", { + expect_lint( + trim_some('{ + library("dplyr", character.only = TRUE) + print("not a library call") + require("gfile") + sapply(pkg_list, "library", character.only = TRUE) + purrr::walk(extra_list, require, character.only = TRUE) + }'), + list( + list(message = rex::rex("library calls", anything, "character.only")), + list(message = rex::rex("Move all require calls to the top of the script.")), + list(message = "symbols, not strings, in require calls"), + list(message = rex::rex("library() directly", anything, "vectorized with sapply()")), + list(message = rex::rex("require() directly", anything, "vectorized with walk()")) + ), + library_call_linter() + ) +})