New for_loop_index_linter (#1629)

* New for_loop_index_linter

* add examples

Author: MichaelChirico

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'

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)

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.

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"
+    )
+  })
+}

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

man/best_practices_linters.Rd

@@ -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)
+})