diff --git a/DESCRIPTION b/DESCRIPTION index 9e49cc3a6..afb59c379 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -79,6 +79,7 @@ Collate: 'declared_functions.R' 'deprecated.R' 'duplicate_argument_linter.R' + 'empty_assignment_linter.R' 'equals_na_linter.R' 'exclude.R' 'expect_comparison_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 7594686ee..2a00bc86d 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -42,6 +42,7 @@ export(default_settings) export(default_undesirable_functions) export(default_undesirable_operators) export(duplicate_argument_linter) +export(empty_assignment_linter) export(equals_na_linter) export(expect_comparison_linter) export(expect_identical_linter) diff --git a/NEWS.md b/NEWS.md index 2da107467..529b8d4e3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -51,6 +51,8 @@ * `for_loop_index_linter()` to prevent overwriting local variables in a `for` loop declared like `for (x in x) { ... }` (@MichaelChirico) +* `empty_assignment_linter()` for identifying empty assignments like `x = {}` that are more clearly written as `x = NULL` (@MichaelChirico) + ## Notes * `lint()` continues to support Rmarkdown documents. For users of custom .Rmd engines, e.g. diff --git a/R/empty_assignment_linter.R b/R/empty_assignment_linter.R new file mode 100644 index 000000000..f967e8be3 --- /dev/null +++ b/R/empty_assignment_linter.R @@ -0,0 +1,62 @@ +#' Block assignment of `{}` +#' +#' Assignment of `{}` is the same as assignment of `NULL`; use the latter +#' for clarity. Closely related: [unneeded_concatenation_linter()]. +#' +#' @examples +#' # will produce lints +#' lint( +#' text = "x <- {}", +#' linters = empty_assignment_linter() +#' ) +#' +#' cat("x = {\n}") +#' lint( +#' text = "x = {\n}", +#' linters = empty_assignment_linter() +#' ) +#' +#' # okay +#' lint( +#' text = "x <- { 3 + 4 }", +#' linters = empty_assignment_linter() +#' ) +#' +#' lint( +#' text = "x <- NULL", +#' linters = empty_assignment_linter() +#' ) +#' +#' @evalRd rd_tags("empty_assignment_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +empty_assignment_linter <- function() { + # for some reason, the parent in the `=` case is , not , hence parent::expr + xpath <- " + //OP-LEFT-BRACE[following-sibling::*[1][self::OP-RIGHT-BRACE]] + /parent::expr[ + preceding-sibling::LEFT_ASSIGN + or preceding-sibling::EQ_ASSIGN + or following-sibling::RIGHT_ASSIGN + ] + /parent::* + " + + Linter(function(source_expression) { + if (!is_lint_level(source_expression, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + + bad_expr <- xml2::xml_find_all(xml, xpath) + + xml_nodes_to_lints( + bad_expr, + source_expression = source_expression, + lint_message = + "Assign NULL explicitly or, whenever possible, allocate the empty object with the right type and size.", + type = "warning" + ) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 8a1b785ca..fbcb118c8 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -15,6 +15,7 @@ conjunct_test_linter,package_development best_practices readability consecutive_stopifnot_linter,style readability consistency cyclocomp_linter,style readability best_practices default configurable duplicate_argument_linter,correctness common_mistakes configurable +empty_assignment_linter,readability best_practices equals_na_linter,robustness correctness common_mistakes default expect_comparison_linter,package_development best_practices expect_identical_linter,package_development diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 0e50a0341..89c244df0 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -21,6 +21,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{condition_message_linter}}} \item{\code{\link{conjunct_test_linter}}} \item{\code{\link{cyclocomp_linter}}} +\item{\code{\link{empty_assignment_linter}}} \item{\code{\link{expect_comparison_linter}}} \item{\code{\link{expect_length_linter}}} \item{\code{\link{expect_named_linter}}} diff --git a/man/empty_assignment_linter.Rd b/man/empty_assignment_linter.Rd new file mode 100644 index 000000000..69d92d784 --- /dev/null +++ b/man/empty_assignment_linter.Rd @@ -0,0 +1,43 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/empty_assignment_linter.R +\name{empty_assignment_linter} +\alias{empty_assignment_linter} +\title{Block assignment of \code{{}}} +\usage{ +empty_assignment_linter() +} +\description{ +Assignment of \code{{}} is the same as assignment of \code{NULL}; use the latter +for clarity. Closely related: \code{\link[=unneeded_concatenation_linter]{unneeded_concatenation_linter()}}. +} +\examples{ +# will produce lints +lint( + text = "x <- {}", + linters = empty_assignment_linter() +) + +cat("x = {\n}") +lint( + text = "x = {\n}", + linters = empty_assignment_linter() +) + +# okay +lint( + text = "x <- { 3 + 4 }", + linters = empty_assignment_linter() +) + +lint( + text = "x <- NULL", + linters = empty_assignment_linter() +) + +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=readability_linters]{readability} +} diff --git a/man/linters.Rd b/man/linters.Rd index d27042536..8f45c9946 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,7 +17,7 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (43 linters)} +\item{\link[=best_practices_linters]{best_practices} (44 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (7 linters)} \item{\link[=configurable_linters]{configurable} (20 linters)} \item{\link[=consistency_linters]{consistency} (17 linters)} @@ -27,7 +27,7 @@ The following tags exist: \item{\link[=efficiency_linters]{efficiency} (20 linters)} \item{\link[=executing_linters]{executing} (5 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} -\item{\link[=readability_linters]{readability} (43 linters)} +\item{\link[=readability_linters]{readability} (44 linters)} \item{\link[=robustness_linters]{robustness} (13 linters)} \item{\link[=style_linters]{style} (36 linters)} } @@ -51,6 +51,7 @@ The following linters exist: \item{\code{\link{consecutive_stopifnot_linter}} (tags: consistency, readability, style)} \item{\code{\link{cyclocomp_linter}} (tags: best_practices, configurable, default, readability, style)} \item{\code{\link{duplicate_argument_linter}} (tags: common_mistakes, configurable, correctness)} +\item{\code{\link{empty_assignment_linter}} (tags: best_practices, readability)} \item{\code{\link{equals_na_linter}} (tags: common_mistakes, correctness, default, robustness)} \item{\code{\link{expect_comparison_linter}} (tags: best_practices, package_development)} \item{\code{\link{expect_identical_linter}} (tags: package_development)} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index 9e87c10e1..9454fcebb 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -20,6 +20,7 @@ The following linters are tagged with 'readability': \item{\code{\link{conjunct_test_linter}}} \item{\code{\link{consecutive_stopifnot_linter}}} \item{\code{\link{cyclocomp_linter}}} +\item{\code{\link{empty_assignment_linter}}} \item{\code{\link{expect_length_linter}}} \item{\code{\link{expect_named_linter}}} \item{\code{\link{expect_not_linter}}} diff --git a/tests/testthat/test-empty_assignment_linter.R b/tests/testthat/test-empty_assignment_linter.R new file mode 100644 index 000000000..01aa216bc --- /dev/null +++ b/tests/testthat/test-empty_assignment_linter.R @@ -0,0 +1,30 @@ +test_that("empty_assignment_linter skips valid usage", { + expect_lint("x <- { 3 + 4 }", NULL, empty_assignment_linter()) + expect_lint("x <- if (x > 1) { 3 + 4 }", NULL, empty_assignment_linter()) + + # also triggers assignment_linter + expect_lint("x = { 3 + 4 }", NULL, empty_assignment_linter()) +}) + +test_that("empty_assignment_linter blocks disallowed usages", { + linter <- empty_assignment_linter() + lint_msg <- rex::rex("Assign NULL explicitly or, whenever possible, allocate the empty object") + + expect_lint("xrep <- {}", lint_msg, linter) + + # assignment with equal works as well, and white space doesn't matter + expect_lint("x = { }", lint_msg, linter) + + # ditto right assignments + expect_lint("{} -> x", lint_msg, linter) + expect_lint("{} ->> x", lint_msg, linter) + + # ditto data.table-style walrus assignments + expect_lint("x[, col := {}]", lint_msg, linter) + + # newlines also don't matter + expect_lint("x <- {\n}", lint_msg, linter) + + # LHS of assignment doesn't matter + expect_lint("env$obj <- {}", lint_msg, linter) +})