object_usage_linter marks wrong line with wrong message

[thothal]

Disclaimer
I have posted this question first on SO but I guess it is a bug and hence it should be better posted here.

Problem
Consider the following code snippet (comments at the end indicate line numbers and are not part of the problem):

set.seed(1)                                   # 1
my_dat1 <- data.frame(x = runif(100, 1, 10),  # 2
                      w = runif(100, 1, 10))  # 3
                                              # 4
my_dat1$y_orig <- my_dat1$x * my_dat1$w       # 5
my_dat1$y_obs  <- my_dat1$y_orig + rnorm(100) # 6
                                              # 7
my_stat <- function(data, orig_mod) {         # 8
  new_mod <- update(orig_mod, data = data)    # 9
  r <- residuals(new_mod)                     #10
  f <- fitted(new_mod)                        #11
  l <- lowess(f, r)                           #12
  res <- c(l$x, l$y)                          #13
  res                                         #14
}                                             #15

When I lint this file via lintr:::addin_lint() I get the following strange error:

$ test.R:List of 8
 ..$ filename     : chr "test.R
 ..$ line_number  : int 6
 ..$ column_number: int 14
 ..$ type         : chr "warning"
 ..$ message      : chr "no visible binding for global variable ‘x’"
 ..$ line         : chr "  res <- c(l$x, l$y)                          #13"
 ..$ ranges       :List of 1
 .. ..$ : int [1:2] 14 14
 ..$ linter       : chr "object_usage_linter"
 ..- attr(*, "class")= chr "lint"
$ test.R:List of 8
 ..$ filename     : chr "test.R"
 ..$ line_number  : int 6
 ..$ column_number: int 19
 ..$ type         : chr "warning"
 ..$ message      : chr "no visible binding for global variable ‘y’"
 ..$ line         : chr "  res <- c(l$x, l$y)                          #13"
 ..$ ranges       :List of 1
 .. ..$ : int [1:2] 19 19
 ..$ linter       : chr "object_usage_linter"
 ..- attr(*, "class")= chr "lint"
- attr(*, "class")= chr "lints"

However, if I remove lines #5 and #6 I do not have the error any more:

set.seed(1)                                   # 1
my_dat1 <- data.frame(x = runif(100, 1, 10),  # 2
                      w = runif(100, 1, 10))  # 3
                                              # 4
                                              # 7
my_stat <- function(data, orig_mod) {         # 8
  new_mod <- update(orig_mod, data = data)    # 9
  r <- residuals(new_mod)                     #10
  f <- fitted(new_mod)                        #11
  l <- lowess(f, r)                           #12
  res <- c(l$x, l$y)                          #13
  res                                         #14
}                                             #15

There is no error when linting this file.

The wrong line number maybe solved in #440 but I am still at loss why the error appears in the first place and why it goes away if the lines are removed.

Session Info

R version 3.5.0 (2018-04-23)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 18363)

Matrix products: default

locale:
[1] LC_COLLATE=German_Germany.1252  LC_CTYPE=German_Germany.1252   
[3] LC_MONETARY=German_Germany.1252 LC_NUMERIC=C                   
[5] LC_TIME=German_Germany.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] lintr_2.0.0

loaded via a namespace (and not attached):
 [1] Rcpp_1.0.3       ps_1.3.0         crayon_1.3.4     withr_2.1.2     
 [5] rprojroot_1.3-2  assertthat_0.2.1 R6_2.4.1         backports_1.1.5 
 [9] magrittr_1.5     lazyeval_0.2.2   remotes_2.1.0    callr_3.4.0     
[13] rex_1.1.2        xml2_1.2.2       cyclocomp_1.1.0  desc_1.2.0      
[17] tools_3.5.0      compiler_3.5.0   processx_3.4.1  

[russHyde]

I've just ran this in my branch for #440 (using your first file; #440 has now been merged into master)

> as.data.frame(lint("temp.R"))
                                       filename line_number column_number
1 /.../lintr/temp.R          13            14
2 /.../lintr/temp.R          13            19
     type                                    message                      line
1 warning no visible binding for global variable ‘x’   res <- c(l$x, l$y) # 13
2 warning no visible binding for global variable ‘y’   res <- c(l$x, l$y) # 13
               linter
1 object_usage_linter
2 object_usage_linter

So

• the line-numbers are now consistent with the position of the res <- c(l$x, l$y) line
• but the object_usage_linter is incorrectly throwing the lint still

So I agree that there is a bug that is unrelated to that in #432

[russHyde]

However, I don't get the object-usage lint on a file that just contains your my_stat function

[russHyde]

[WIP] debugging the above (apologies if this makes no sense)

If we

• put a break point at the line res <- parse_check_usage(fun) in object_usage_linter
• call lint("temp.R", linters = object_usage_linter)

Then

• the assignment_symbols that are found in the file are (my_dat1, $, y_orig, y_obs, my_stat)
• object_usage_linter evaluates the function my_stat in an env containing these assignment symbols
• parse_check_usage finds no visible binding for x or y (in "res")

But, if we evaluate my_stat outside of that environment, the "no visible binding" rows do not arise in res (compare eval(parse(text = code), envir = env) with eval(parse(text = code)))

---

Should all of those assignment symbols have been added to the environment in which my_stat was evaluated?

Adding $ to the environment seemed a bit odd to me, so I modified how the env was constructed:

# in object_usage_linter
  for(symbol in symbols) {
    if (symbol != "$"){
      assign(symbol, function(...) invisible(), envir = env)
    }
  }

Now, on calling lint("temp.R", linters = object_usage_linter) with the same debug breakpoint:

• the env in which my_stat is evaluated includes (my_dat1, y_orig, y_obs, my_stat), but not $
• parse_check_usage no longer finds the (mistaken) assignment issue
• (all existing tests pass, although this doesn't mean much as I don't understand the origin of the issue yet)

---

I suspect that get_assignment_symbols is too lenient: in the above, the only top-level assignments should be my_dat1 and my_stat, and so checkUsage should be called in an env that is supplemented with my_dat1 and my_stat only.

[russHyde]

No visible binding for global variable '[x|y]' is still an issue as of 3dbf24d

[russHyde]

The issue that remains seems to be a duplicate of #666. Closing.