diff --git a/DESCRIPTION b/DESCRIPTION index 31871bd90..cc801183a 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -73,6 +73,7 @@ Collate: 'boolean_arithmetic_linter.R' 'brace_linter.R' 'cache.R' + 'character_only_linter.R' 'class_equals_linter.R' 'commas_linter.R' 'comment_linters.R' diff --git a/NAMESPACE b/NAMESPACE index a6c17992e..fa799453c 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -30,6 +30,7 @@ export(available_tags) export(backport_linter) export(boolean_arithmetic_linter) export(brace_linter) +export(character_only_linter) export(checkstyle_output) export(class_equals_linter) export(clear_cache) diff --git a/NEWS.md b/NEWS.md index fb391d3fd..c80456f90 100644 --- a/NEWS.md +++ b/NEWS.md @@ -10,6 +10,7 @@ * `stopifnot_all_linter()` discourages tests with `all()` like `stopifnot(all(x > 0))`; `stopifnot()` runs `all()` itself, and uses a better error message (part of #884, @MichaelChirico). * `comparison_negation_linter()` for discouraging negated comparisons when a direct negation is preferable, e.g. `!(x == y)` could be `x != y` (part of #884, @MichaelChirico). * `terminal_close_linter()` for discouraging using `close()` to end functions (part of #884, @MichaelChirico). Such usages are not robust to errors, where `close()` will not be run as intended. Put `close()` in an `on.exit()` hook, or use {withr} to manage connections with proper cleanup. +* `character_only_linter()` for encouraging all attached packages to be included with `library(symbol)`, not `library("symbol", character.only = TRUE)` or "vectorized" approaches looping over package names (part of #884, @MichaelChirico). ### Lint accuracy fixes: removing false positives diff --git a/R/character_only_linter.R b/R/character_only_linter.R new file mode 100644 index 000000000..b848cf2ec --- /dev/null +++ b/R/character_only_linter.R @@ -0,0 +1,80 @@ +#' Enforce library calls using symbols +#' +#' R packages should be imported as symbols, like `library(lintr)`, +#' and *not* via strings like `library("lintr", character.only = TRUE)`, +#' with few exceptions. +#' +#' @evalRd rd_tags("character_only_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +character_only_linter <- function() { + # STR_CONST: block library|require("..."), i.e., supplying a string literal + # ancestor::expr[FUNCTION]: Skip usages inside functions a la {knitr} + 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 <- R"{ + SYMBOL[text() = 'library' or text() = 'require'] + or STR_CONST[text() = '"library"' or text() = '"require"'] + }" + 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, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + + direct_expr <- xml_find_all(xml, direct_xpath) + direct_calls <- xp_call_name(direct_expr) + character_only <- + xml_find_first(direct_expr, "./SYMBOL_SUB[text() = 'character.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'." + ) + direct_msg <- sprintf(as.character(direct_msg_fmt), direct_calls) + direct_lints <- xml_nodes_to_lints( + direct_expr, + source_expression = source_expression, + lint_message = direct_msg, + type = "warning" + ) + + indirect_expr <- xml_find_all(xml, indirect_xpath) + indirect_lib_calls <- get_r_string(indirect_expr, call_symbol_path) + indirect_loop_calls <- xp_call_name(indirect_expr) + indirect_msg <- sprintf( + "Call %s() directly, not vectorized with %s().", + indirect_lib_calls, indirect_loop_calls + ) + indirect_lints <- xml_nodes_to_lints( + indirect_expr, + source_expression = source_expression, + lint_message = indirect_msg, + type = "warning" + ) + + c(direct_lints, indirect_lints) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index c6ca4ca94..5aae3c0fa 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -6,6 +6,7 @@ assignment_linter,style consistency default configurable backport_linter,robustness configurable package_development boolean_arithmetic_linter,efficiency best_practices readability brace_linter,style readability default configurable +character_only_linter,consistency best_practices readability class_equals_linter,best_practices robustness consistency closed_curly_linter,style readability deprecated configurable commas_linter,style readability default configurable diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 9baf4f3bc..65f8491bc 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -16,6 +16,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{any_duplicated_linter}}} \item{\code{\link{any_is_na_linter}}} \item{\code{\link{boolean_arithmetic_linter}}} +\item{\code{\link{character_only_linter}}} \item{\code{\link{class_equals_linter}}} \item{\code{\link{commented_code_linter}}} \item{\code{\link{condition_message_linter}}} diff --git a/man/character_only_linter.Rd b/man/character_only_linter.Rd new file mode 100644 index 000000000..b2be7402f --- /dev/null +++ b/man/character_only_linter.Rd @@ -0,0 +1,19 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/character_only_linter.R +\name{character_only_linter} +\alias{character_only_linter} +\title{Enforce library calls using symbols} +\usage{ +character_only_linter() +} +\description{ +R packages should be imported as symbols, like \code{library(lintr)}, +and \emph{not} via strings like \code{library("lintr", character.only = TRUE)}, +with few exceptions. +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=consistency_linters]{consistency}, \link[=readability_linters]{readability} +} diff --git a/man/consistency_linters.Rd b/man/consistency_linters.Rd index 54e9064ad..1262f040b 100644 --- a/man/consistency_linters.Rd +++ b/man/consistency_linters.Rd @@ -14,6 +14,7 @@ something. The following linters are tagged with 'consistency': \itemize{ \item{\code{\link{assignment_linter}}} +\item{\code{\link{character_only_linter}}} \item{\code{\link{class_equals_linter}}} \item{\code{\link{comparison_negation_linter}}} \item{\code{\link{condition_message_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index ef71af792..eddd5d94d 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,10 +17,10 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (55 linters)} +\item{\link[=best_practices_linters]{best_practices} (56 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (8 linters)} \item{\link[=configurable_linters]{configurable} (34 linters)} -\item{\link[=consistency_linters]{consistency} (23 linters)} +\item{\link[=consistency_linters]{consistency} (24 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} \item{\link[=deprecated_linters]{deprecated} (8 linters)} @@ -28,7 +28,7 @@ The following tags exist: \item{\link[=executing_linters]{executing} (5 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} \item{\link[=pkg_testthat_linters]{pkg_testthat} (12 linters)} -\item{\link[=readability_linters]{readability} (57 linters)} +\item{\link[=readability_linters]{readability} (58 linters)} \item{\link[=robustness_linters]{robustness} (16 linters)} \item{\link[=style_linters]{style} (38 linters)} } @@ -43,6 +43,7 @@ The following linters exist: \item{\code{\link{backport_linter}} (tags: configurable, package_development, robustness)} \item{\code{\link{boolean_arithmetic_linter}} (tags: best_practices, efficiency, readability)} \item{\code{\link{brace_linter}} (tags: configurable, default, readability, style)} +\item{\code{\link{character_only_linter}} (tags: best_practices, consistency, readability)} \item{\code{\link{class_equals_linter}} (tags: best_practices, consistency, robustness)} \item{\code{\link{commas_linter}} (tags: configurable, default, readability, style)} \item{\code{\link{commented_code_linter}} (tags: best_practices, default, readability, style)} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index 06deb9233..307129c06 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -14,6 +14,7 @@ The following linters are tagged with 'readability': \itemize{ \item{\code{\link{boolean_arithmetic_linter}}} \item{\code{\link{brace_linter}}} +\item{\code{\link{character_only_linter}}} \item{\code{\link{commas_linter}}} \item{\code{\link{commented_code_linter}}} \item{\code{\link{comparison_negation_linter}}} diff --git a/tests/testthat/test-character_only_linter.R b/tests/testthat/test-character_only_linter.R new file mode 100644 index 000000000..723c4abd0 --- /dev/null +++ b/tests/testthat/test-character_only_linter.R @@ -0,0 +1,92 @@ +test_that("character_only_linter skips allowed usages", { + linter <- character_only_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("character_only_linter blocks disallowed usages", { + linter <- character_only_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("Printing character_only_linter works 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"), + character_only_linter() + ) +}) + +test_that("character_only_linter catches purrr::walk as well", { + expect_lint( + 'purr::walk("data.table", library, character.only = TRUE)', + rex::rex("Call library() directly, not vectorized with walk()"), + character_only_linter() + ) +}) + +test_that("multiple lints are generated correctly", { + expect_lint( + trim_some('{ + library("dplyr", character.only = TRUE) + 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 = "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()")) + ), + character_only_linter() + ) +})