diff --git a/DESCRIPTION b/DESCRIPTION index 0296fca65..9e49cc3a6 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -94,6 +94,7 @@ Collate: 'extract.R' 'extraction_operator_linter.R' 'fixed_regex_linter.R' + 'for_loop_index_linter.R' 'function_argument_linter.R' 'function_left_parentheses_linter.R' 'function_return_linter.R' diff --git a/NAMESPACE b/NAMESPACE index b39d61b16..7594686ee 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -57,6 +57,7 @@ export(expect_true_false_linter) export(expect_type_linter) export(extraction_operator_linter) export(fixed_regex_linter) +export(for_loop_index_linter) export(function_argument_linter) export(function_left_parentheses_linter) export(function_return_linter) diff --git a/NEWS.md b/NEWS.md index 95f56f6be..8d953b789 100644 --- a/NEWS.md +++ b/NEWS.md @@ -44,6 +44,8 @@ * `boolean_arithmetic_linter()` for identifying places where logical aggregations are more appropriate, e.g. `length(which(x == y)) == 0` is the same as `!any(x == y)` or even `all(x != y)` (@MichaelChirico) +* `for_loop_index_linter()` to prevent overwriting local variables in a `for` loop declared like `for (x in x) { ... }` (@MichaelChirico) + ## Notes * `lint()` continues to support Rmarkdown documents. For users of custom .Rmd engines, e.g. diff --git a/R/for_loop_index_linter.R b/R/for_loop_index_linter.R new file mode 100644 index 000000000..08c070531 --- /dev/null +++ b/R/for_loop_index_linter.R @@ -0,0 +1,61 @@ +#' Block usage of for loops directly overwriting the indexing variable +#' +#' `for (x in x)` is a poor choice of indexing variable. This overwrites +#' `x` in the calling scope and is confusing to read. +#' +#' @examples +#' # will produce lints +#' lint( +#' text = "for (x in x) { TRUE }", +#' linters = for_loop_index_linter() +#' ) +#' +#' lint( +#' text = "for (x in foo(x, y)) { TRUE }", +#' linters = for_loop_index_linter() +#' ) +#' +#' # okay +#' lint( +#' text = "for (xi in x) { TRUE }", +#' linters = for_loop_index_linter() +#' ) +#' +#' lint( +#' text = "for (col in DF$col) { TRUE }", +#' linters = for_loop_index_linter() +#' ) +#' +#' @evalRd rd_tags("for_loop_index_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +for_loop_index_linter <- function() { + xpath <- " + //forcond + /SYMBOL[text() = + following-sibling::expr + //SYMBOL[not( + preceding-sibling::OP-DOLLAR + or parent::expr[preceding-sibling::OP-LEFT-BRACKET] + )] + /text() + ] + " + + 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 = "Don't re-use any sequence symbols as the index symbol in a for loop.", + type = "warning" + ) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 68a699ed4..8a1b785ca 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -28,6 +28,7 @@ expect_true_false_linter,package_development best_practices readability expect_type_linter,package_development best_practices extraction_operator_linter,style best_practices fixed_regex_linter,best_practices readability efficiency +for_loop_index_linter,best_practices readability robustness function_argument_linter,style consistency best_practices function_left_parentheses_linter,style readability default function_return_linter,readability best_practices diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index b1c0f7033..0e50a0341 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -32,6 +32,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{expect_type_linter}}} \item{\code{\link{extraction_operator_linter}}} \item{\code{\link{fixed_regex_linter}}} +\item{\code{\link{for_loop_index_linter}}} \item{\code{\link{function_argument_linter}}} \item{\code{\link{function_return_linter}}} \item{\code{\link{ifelse_censor_linter}}} diff --git a/man/for_loop_index_linter.Rd b/man/for_loop_index_linter.Rd new file mode 100644 index 000000000..ccc4dab97 --- /dev/null +++ b/man/for_loop_index_linter.Rd @@ -0,0 +1,42 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/for_loop_index_linter.R +\name{for_loop_index_linter} +\alias{for_loop_index_linter} +\title{Block usage of for loops directly overwriting the indexing variable} +\usage{ +for_loop_index_linter() +} +\description{ +\verb{for (x in x)} is a poor choice of indexing variable. This overwrites +\code{x} in the calling scope and is confusing to read. +} +\examples{ +# will produce lints +lint( + text = "for (x in x) { TRUE }", + linters = for_loop_index_linter() +) + +lint( + text = "for (x in foo(x, y)) { TRUE }", + linters = for_loop_index_linter() +) + +# okay +lint( + text = "for (xi in x) { TRUE }", + linters = for_loop_index_linter() +) + +lint( + text = "for (col in DF$col) { TRUE }", + linters = for_loop_index_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}, \link[=robustness_linters]{robustness} +} diff --git a/man/linters.Rd b/man/linters.Rd index f92caf059..d27042536 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} (42 linters)} +\item{\link[=best_practices_linters]{best_practices} (43 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,8 +27,8 @@ 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} (42 linters)} -\item{\link[=robustness_linters]{robustness} (12 linters)} +\item{\link[=readability_linters]{readability} (43 linters)} +\item{\link[=robustness_linters]{robustness} (13 linters)} \item{\link[=style_linters]{style} (36 linters)} } } @@ -64,6 +64,7 @@ The following linters exist: \item{\code{\link{expect_type_linter}} (tags: best_practices, package_development)} \item{\code{\link{extraction_operator_linter}} (tags: best_practices, style)} \item{\code{\link{fixed_regex_linter}} (tags: best_practices, efficiency, readability)} +\item{\code{\link{for_loop_index_linter}} (tags: best_practices, readability, robustness)} \item{\code{\link{function_argument_linter}} (tags: best_practices, consistency, style)} \item{\code{\link{function_left_parentheses_linter}} (tags: default, readability, style)} \item{\code{\link{function_return_linter}} (tags: best_practices, readability)} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index 325709985..9e87c10e1 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -25,6 +25,7 @@ The following linters are tagged with 'readability': \item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_true_false_linter}}} \item{\code{\link{fixed_regex_linter}}} +\item{\code{\link{for_loop_index_linter}}} \item{\code{\link{function_left_parentheses_linter}}} \item{\code{\link{function_return_linter}}} \item{\code{\link{infix_spaces_linter}}} diff --git a/man/robustness_linters.Rd b/man/robustness_linters.Rd index 7cf6cc82d..c73e0bd06 100644 --- a/man/robustness_linters.Rd +++ b/man/robustness_linters.Rd @@ -16,6 +16,7 @@ The following linters are tagged with 'robustness': \item{\code{\link{backport_linter}}} \item{\code{\link{class_equals_linter}}} \item{\code{\link{equals_na_linter}}} +\item{\code{\link{for_loop_index_linter}}} \item{\code{\link{missing_package_linter}}} \item{\code{\link{namespace_linter}}} \item{\code{\link{nonportable_path_linter}}} diff --git a/tests/testthat/test-for_loop_index_linter.R b/tests/testthat/test-for_loop_index_linter.R new file mode 100644 index 000000000..47e0382ca --- /dev/null +++ b/tests/testthat/test-for_loop_index_linter.R @@ -0,0 +1,18 @@ +test_that("for_loop_index_linter skips allowed usages", { + expect_lint("for (xi in x) {}", NULL, for_loop_index_linter()) + + # this is OK, so not every symbol is problematic + expect_lint("for (col in DF$col) {}", NULL, for_loop_index_linter()) + expect_lint("for (col in DT[, col]) {}", NULL, for_loop_index_linter()) +}) + +test_that("for_loop_index_linter blocks simple disallowed usages", { + linter <- for_loop_index_linter() + lint_msg <- "Don't re-use any sequence symbols as the index symbol in a for loop" + + expect_lint("for (x in x) {}", lint_msg, linter) + # these also overwrite a variable in calling scope + expect_lint("for (x in foo(x)) {}", lint_msg, linter) + # arbitrary nesting + expect_lint("for (x in foo(bar(y, baz(2, x)))) {}", lint_msg, linter) +})