only extract the first symbol for equal and left assignments

[AshesITR]

Fixes #666

Makes sure foo$bar$duh <- 42L returns "foo" as the only used (global) symbol.

• implement fix
• write regression tests
• update NEWS

[AshesITR]

@MichaelChirico Can you check the new XPath?
Tests are all passing and the new tests failed on master so I think it should work as intended, but I'm no XPath guru.

[dmurdoch]

I tried the branch code on the original example code from issue #666:

foo <- data.frame(0)
foo$bar <- 1
zero <- function() {
  file.info("/dev/null")$size
}
message(zero())

There was no error any more, which is good. However, I also don't get an error from this code:

zero <- function() {
  foo$bar <- 1
}
message(zero())

I still don't get any lint issues, and I think I should: this code is treating foo as a global variable without defining it. The PR acts as if the foo$bar assignment is creating foo, but it doesn't, it needs foo to already exist or it won't work. I do get no visible binding for global variable "foo" if I use foo on the RHS without defining it:

zero <- function() {
  x <- foo$bar
  x
}
message(zero())

[AshesITR]

Thanks for checking this out.
So I need to give this a little more thought.

$- Assignments should be considered a usage of the first object, not an assignment, i.e. assigned symbols should actually only be 1st level SYMBOLs of the LHS of an assignment expression.

[russHyde]

When run on gWidgets source code, I get a couple of object-usage lints using master that aren't lints using this branch and vice-versa (I checked many more packages).

These lines are flagged in master (but not 709):

                                               message
1         no visible binding for global variable ‘obj’
2 no visible binding for global variable ‘set_invalid’
                                                  line
1   h$obj$set_invalid(inherits(out, "try-error"), out)
2   h$obj$set_invalid(inherits(out, "try-error"), out)

These are flagged in #709 (6a04903) (but not master):

                                               message
1   no visible global function definition for ‘svalue’
2 no visible global function definition for ‘isExtant’
                            line
1           vals <- svalue(flyt)
2   if(!isExtant(obj))  return()

[russHyde]

Some other examples of object-usage lints that are called by 709 but not master (across 81 packages):

pr_lints[pr_unique_rows, ] %>% magrittr::extract(c("message", "line")) %>% as.data.frame()
                                                 message
1       no visible global function definition for ‘%p0%’
2  no visible global function definition for ‘%without%’
3  no visible global function definition for ‘%without%’
4       no visible global function definition for ‘%p0%’
5       no visible global function definition for ‘%p0%’
6  no visible global function definition for ‘%without%’
7       no visible global function definition for ‘%p0%’
8       no visible global function definition for ‘%p0%’
9      no visible binding for global variable ‘distance’
10     no visible binding for global variable ‘distance’
11    no visible global function definition for ‘svalue’
12  no visible global function definition for ‘isExtant’
                                                                                line
1                         .body[1] <- .body[1] %p0% "\\n.Object <- callNextMethod()"
2      args <- .exprTree$argDefaults[names(.exprTree$argDefaults) %without% ".Data"]
3                             args <- names(.exprTree$argDefaults) %without% ".Data"
4       c("'" %p0% .exprTree$names[1] %p0% "'", paste(args, args, sep = "="), "...")
5       c("'" %p0% .exprTree$names[1] %p0% "'", paste(args, args, sep = "="), "...")
6                              .genericArgNames <- names(formals(f)) %without% "..."
7                                          defCall <- "function" %p0% args %p0% body
8                                          defCall <- "function" %p0% args %p0% body
9                         with(sitepairs, distance <= maxdist & distance >= mindist)
10                        with(sitepairs, distance <= maxdist & distance >= mindist)
11                                                              vals <- svalue(flyt)
12                                                      if(!isExtant(obj))  return()

Interestingly none of these seem to relate to a$b$c subindexing

[russHyde]

Corresponding, a sample of the lints called by master but not 709:

head_lints[head_unique_rows, ] %>% magrittr::extract(c("message", "line")) %>% sample_n(12) %>% as.data.frame()
                                                     message
1           no visible binding for global variable ‘package’
2            no visible binding for global variable ‘python’
3        no visible binding for global variable ‘rook.input’
4              no visible binding for global variable ‘http’
5             no visible binding for global variable ‘token’
6           no visible binding for global variable ‘binding’
7  no visible binding for global variable ‘serverFuncSource’
8           no visible binding for global variable ‘OS.type’
9      no visible binding for global variable ‘defineOutput’
10              no visible binding for global variable ‘num’
11  no visible binding for global variable ‘createHttpuvApp’
12        no visible binding for global variable ‘enclosing’
                                                                              line
1                 outfile <- file.path(lib, paste0(pkg$package, "-commands.Rout"))
2                                               msg <- sprintf(fmt, config$python)
3                            params <- URLdecode(rawToChar(req$rook.input$read()))
4                    handlerManager$addHandler(appHandlers$http, "/", tail = TRUE)
5                                      appsNeedingFlush$remove(shinysession$token)
6                                   new[[i]]$enclosing$super <- new[[i+1]]$binding
7    appHandlers <- createAppHandlers(appObj$httpHandler, appObj$serverFuncSource)
8                                          identical(.Platform$OS.type, "windows")
9                             .subset2(x, 'impl')$defineOutput(name, value, label)
10                                          width <- floor(log10(max(st$num))) + 1
11                                   httpuvApp <- handlerManager$createHttpuvApp()
12                                   new_slice$enclosing$self <- new_slice$binding

All of these sampled lint-diffs are from a$b subindexing.

And for all the 117 (master-not-709) lints, they all had message of the form "no visible binding for global variable ..."

[AshesITR]

@dmurdoch I directly checked

zero <- function() {foo$bar <- 1L}

with codetools::checkUsage() - it yields no messages so we can't throw a lint.
The same (no message) unfortunately happens with

zero <- function() {dim(foo) <- 1L}

and even

zero <- function() {
  foo$bar <- 1L
  foo
}

I've submitted an issue at codetools for this.

[AshesITR]

@russHyde Can you re-run the current HEAD of #709?
I'm expecting new lints to appear in the packages.

I will manually check gWidgets if the new lints are correct or not:

> lints_new %>% anti_join(lints_old, by = c("filename", "linter", "line_number"))
                 filename line_number column_number    type                                         message                                         line              linter
1 inst\\doc\\gWidgets.Rnw        1281            23 warning no visible global function definition for ‘tag’       dropHandlers <- tag(gd,"dropHandlers") object_usage_linter
2 inst\\doc\\gWidgets.Rnw        1292            21 warning no visible global function definition for ‘tag’       dropHandlers <- tag(gd,"dropHandlers") object_usage_linter
3 inst\\doc\\gWidgets.Rnw        1309            11 warning no visible global function definition for ‘tag’                  data <- tag(widget, "data") object_usage_linter
> lints_old %>% anti_join(lints_new, by = c("filename", "linter", "line_number"))
                 filename line_number column_number    type                                           message                                                line              linter
1 inst\\doc\\gWidgets.Rnw         980            35 warning       no visible binding for global variable ‘to’   if(exists("buddyList")) widgets$to[] <- buddyList object_usage_linter
2 inst\\doc\\gWidgets.Rnw        1274            21 warning no visible binding for global variable ‘dropdata’                              view.col <- h$dropdata object_usage_linter
3 inst\\doc\\gWidgets.Rnw        1453            19 warning    no visible binding for global variable ‘isdir’                              return(children$isdir) object_usage_linter
4 inst\\doc\\gWidgets.Rnw        1463            14 warning    no visible binding for global variable ‘isdir’                    x[children$isdir] <- "directory" object_usage_linter

All new lints relate to the tag S4 generic which is defined in gWidgets, but I didn't install or load the package for linting, so tag was correctly detected as not visible during the linting.
All missing lints were false positives.

[russHyde]

So the only lints that are 709-specific come from 'aoos', 'shiny' and 'gWidgets2':

The shiny ones all relate to the .globals object

  package                  filename line_number column_number    type
1   shiny             R/app-state.R          21             3 warning
2   shiny            R/conditions.R         145            29 warning
3   shiny            R/conditions.R         168            29 warning
4   shiny            R/conditions.R         206            38 warning
5   shiny      R/reactive-domains.R          84             3 warning
6   shiny R/server-resource-paths.R         103            17 warning
7   shiny R/server-resource-paths.R         154            14 warning
8   shiny         R/shiny-options.R          33            12 warning
9   shiny                 R/utils.R        1475             3 warning
                                            message
1 no visible binding for global variable ‘.globals’
2 no visible binding for global variable ‘.globals’
3 no visible binding for global variable ‘.globals’
4 no visible binding for global variable ‘.globals’
5 no visible binding for global variable ‘.globals’
6 no visible binding for global variable ‘.globals’
7 no visible binding for global variable ‘.globals’
8 no visible binding for global variable ‘.globals’
9 no visible binding for global variable ‘.globals’
                                                     line              linter
1                                       .globals$appState object_usage_linter
2                  currentDeepStack <- .globals$deepStack object_usage_linter
3                  currentDeepStack <- .globals$deepStack object_usage_linter
4       attr(e, "deep.stack.trace") <- .globals$deepStack object_usage_linter
5                                         .globals$domain object_usage_linter
6                   urls <- names(.globals$resourcePaths) object_usage_linter
7                 resInfo <- .globals$resources[[prefix]] object_usage_linter
8                        return(.globals$options[[name]]) object_usage_linter
9                                     .globals$serverInfo object_usage_linter

The aoos ones relate to the operators "%p0%" and "%without%" (I couldn't find their definition)

  package           filename line_number column_number    type
1    aoos R/S4-expressions.R         129            24 warning
2    aoos R/S4-expressions.R         143            64 warning
3    aoos R/S4-expressions.R         153            42 warning
4    aoos R/S4-expressions.R         154            11 warning
5    aoos R/S4-expressions.R         154            11 warning
6    aoos R/S4-expressions.R         181            41 warning
7    aoos R/S4-expressions.R         208            25 warning
8    aoos R/S4-expressions.R         208            25 warning
                                                message
1      no visible global function definition for ‘%p0%’
2 no visible global function definition for ‘%without%’
3 no visible global function definition for ‘%without%’
4      no visible global function definition for ‘%p0%’
5      no visible global function definition for ‘%p0%’
6 no visible global function definition for ‘%without%’
7      no visible global function definition for ‘%p0%’
8      no visible global function definition for ‘%p0%’
                                                                               line
1                        .body[1] <- .body[1] %p0% "\\n.Object <- callNextMethod()"
2     args <- .exprTree$argDefaults[names(.exprTree$argDefaults) %without% ".Data"]
3                            args <- names(.exprTree$argDefaults) %without% ".Data"
4      c("'" %p0% .exprTree$names[1] %p0% "'", paste(args, args, sep = "="), "...")
5      c("'" %p0% .exprTree$names[1] %p0% "'", paste(args, args, sep = "="), "...")
6                             .genericArgNames <- names(formals(f)) %without% "..."
7                                         defCall <- "function" %p0% args %p0% body
8                                         defCall <- "function" %p0% args %p0% body
               linter
1 object_usage_linter
2 object_usage_linter
3 object_usage_linter
4 object_usage_linter
5 object_usage_linter
6 object_usage_linter
7 object_usage_linter
8 object_usage_linter

The gWidgets2 ones are similar:

 pr_lints %>% filter(!line %in% head_lints$line & package == "gWidgets2") %>% as.data.frame()
    package                      filename line_number column_number    type
1 gWidgets2 inst/examples/ex-read-table.R          76            11 warning
2 gWidgets2                   R/methods.R         576             7 warning
                                               message
1   no visible global function definition for ‘svalue’
2 no visible global function definition for ‘isExtant’
                            line              linter
1           vals <- svalue(flyt) object_usage_linter
2   if(!isExtant(obj))  return() object_usage_linter

[russHyde]

The lints that were thrown using the master branch, but not in 709, had the following frequencies across packages:

head_lints %>% filter(!line %in% pr_lints$line) %>% count(package)
# A tibble: 9 x 2
  package         n
* <chr>       <int>
1 covr            6
2 gWidgets2       2
3 htmlwidgets     1
4 R6             41
5 reticulate      3
6 rhub            1
7 rJava           2
8 rlang           5
9 shiny          51

These all contain $-subsetting

[russHyde]

The studied packages:

[1] "aoos"           "aprof"          "argparse"       "assertr"
 [5] "available"      "backports"      "badgecreatr"    "checkmate"
 [9] "checkpoint"     "CodeDepends"    "covr"           "cranly"
[13] "devtools"       "docopt"         "drat"           "ensurer"
[17] "formatR"        "functools"      "GetoptLong"     "getPass"
[21] "git2r"          "gitlabr"        "GRANBase"       "gWidgets2"
[25] "htmlwidgets"    "hunspell"       "import"         "inline"
[29] "inlinedocs"     "js"             "knitr"          "later"
[33] "lintr"          "log4r"          "matlabr"        "microbenchmark"
[37] "miniCRAN"       "mockr"          "optigrab"       "packrat"
[41] "pacman"         "pipeR"          "pkgconfig"      "pkgdown"
[45] "pkggraph"       "pkgmaker"       "pkgnet"         "prof.tree"
[49] "profmem"        "profr"          "progress"       "proto"
[53] "purrr"          "R.methodsS3"    "R6"             "rcmdcheck"
[57] "Rcpp"           "Rd2roxygen"     "RDocumentation" "Rdpack"
[61] "remotes"        "reticulate"     "rhub"           "RInside"
[65] "rJava"          "rlang"          "roxygen2"       "RStata"
[69] "rstudioapi"     "rtype"          "semver"         "shiny"
[73] "skeletor"       "svUnit"         "sys"            "testit"
[77] "testthat"       "unitizer"       "V8"             "vdiffr"
[81] "withr"

[AshesITR]

@russHyde looks good; thanks again! Did you run devtools::load_all() before linting? My guess would be most of the new lints would go away if you do that.

[russHyde]

I loaded lintr using devtools::load_all, but didn't load any of the analysed packages.

[AshesITR]

Can you try that for shiny?
I'm hoping this will make the .globals lints disappear.

[AshesITR]

@russHyde If you share your script, I can run it myself, too.

[russHyde]

Please see here: https://gist.github.com/russHyde/568fd5af558c860d97e1b932ca773ff7

[AshesITR]

I've confirmed that there are no new lints in shiny (and other packages) when ensuring the linted package is loaded via load_all().

All removed lints appear to be false positives in master.

[iqis]

I came from https://gitlab.com/luke-tierney/codetools/-/issues/3, have you guys found a way to deal with the following not being picked up by codetools?

f2 <- function() {
  dim(foo) <- 2L
}