r-lib / lintr

Static Code Analysis for R
https://lintr.r-lib.org
Other
1.2k stars 187 forks source link

Introduce `undesirable_loop_variable_linter()`? #1900

Open IndrajeetPatil opened 1 year ago

IndrajeetPatil commented 1 year ago

If you are coming from other programming languages (like C++ or JavaScript), you would expect that the loop variable name binding will be destroyed once the execution goes out of scope.

E.g., here is a JavaScript example. Note that elem doesn't change its value after the loop finishes executing.

Screenshot 2023-01-12 at 14 58 30

If you write R with the same expectation, you are in for a surprise.

elem <- -4
vec <- 1:3

for (elem in vec) {
  print(elem)
}
#> [1] 1
#> [1] 2
#> [1] 3

print(elem)
#> [1] 3

Created on 2023-01-12 with reprex v2.0.2

As mentioned in the docs for for loop:

When the loop terminates, var remains as a variable containing its latest value.

Although, in the minimal example shown here, it might be easier to see this problem, in a script of reasonable size, it won't be. And, given the difficulty of naming things, it is likely that users can re-use a name. So I am wondering if {lintr} should dissuade users from engaging in such practice.

MichaelChirico commented 1 year ago

What would be linted here, exactly? If the indexing variable is also defined in local scope?

MichaelChirico commented 1 year ago

Does for_loop_index_linter() cover this?

IndrajeetPatil commented 1 year ago

Not really.

For example, none of the linters discourages re-usage of elem as an index variable:

lintr::lint(
  text = "elem <- -4L
vec <- 1L:3L
for (elem in vec) {
  print(elem)
}",
  linters = lintr::all_linters()
)

Created on 2023-08-09 with reprex v2.0.2

MichaelChirico commented 1 year ago

Got it, makes sense. That looks like it would be appropriate to subsume under for_loop_index_linter(), so I'm marking it as a feature rather than a separate linter.