Dealing with `object_usage_linter` during local development

[AlexAxthelm]

Hi! I love object_usage_linter; it makes identifying issues during refactoring so much easier than the "run things until they break" strategy 😆

One issue that I've been finding in my package development workflow is that newly-created package objects aren't picked up by lintr if they're spread across files.

For example, if I have R/foo.R:

foo <- function(x) {
  return(x + 1L)
}

and R/bar.r:

bar <- function(y) {
  z <- foo(y)
  return(z)
}

Then I get an object_usage_linter flag for the call to foo() from bar.

If the functions are in the same file, then everything is fine, and lintr runs cleanly. Or if I install the work-in-progress version of the package (using pak::local_install()) then the function gets recognized in the namespace, but that's not ideal.

Is there something I'm missing that would allow lintr to effectively run something like pkgload::load_all()? Ideally it would be something I could set in my local environment, but leave unset for my CI runners.

[F-Noelle]

Perhaps lintr::lint_package() and other functions of that type can check for a pkgload installation and run load_all() when it is found?

[IndrajeetPatil]

Yes, pkgload::load_all() will indeed solve this issue, and perhaps we should be doing this by default in the context of lintr::lint_package():

Lint

dir <- withr::local_tempdir()
withr::local_options(list(usethis.quiet = TRUE))
library(usethis)
pkg_dir <- create_package(dir, open = FALSE, rstudio = FALSE)

withr::with_dir(pkg_dir, {
  usethis::use_mit_license()
  fs::dir_create("R")
  writeLines(
    c("foo <- function(x) return(x + 1L)", "NULL"), 
    "R/foo.R"
  )
  writeLines(
    c("bar <- function(y) foo(y)", "NULL"), 
    "R/bar.R"
  )
  
  lintr::lint_package(linters = lintr::object_usage_linter())
})
#> R/bar.R:1:20: warning: [object_usage_linter] no visible global function definition for 'foo'
#> bar <- function(y) foo(y)
#>                    ^~~

^{Created on 2024-05-30 with reprex v2.1.0}

No lint

dir <- withr::local_tempdir()
withr::local_options(list(usethis.quiet = TRUE))
library(usethis)
pkg_dir <- create_package(dir, open = FALSE, rstudio = FALSE)

withr::with_dir(pkg_dir, {
  usethis::use_mit_license()
  fs::dir_create("R")
  writeLines(
    c("foo <- function(x) return(x + 1L)", "NULL"), 
    "R/foo.R"
  )
  writeLines(
    c("bar <- function(y) foo(y)", "NULL"), 
    "R/bar.R"
  )
  
  pkgload::load_all(quiet = TRUE)
  lintr::lint_package(linters = lintr::object_usage_linter())
})

^{Created on 2024-05-30 with reprex v2.1.0}

[AshesITR]

Regarding defaults we should bear in mind that if pkgload failed in your reprex, linting would fail without a helpful message and we explicitly document which linters run linted code, to provide a way to opt out of running (potentially untrusted) code.

Also, not a big fan of divergent behavior for lint_package() vs. lint() on a set of files. So if we add this, it should be supported by lint() as well IMO.

[IndrajeetPatil]

But what happens when lint() is run outside of package context? Will pkgload::load_all() just fail or won't run at all?

[AshesITR]

I haven't tried this yet, but we should be resilient to failure of pkgload anyway.

[AlexAxthelm]

Maybe something along the lines of

lint_package(<other_args>, load = TRUE) (with similar arg in lint()), which could have something that lives just after here:

https://github.com/r-lib/lintr/blob/1c7f4a0c0023305be4fbebebbb6c6d8cd1d4340e/R/lint.R#L242

if (load && requireNamespace("pkgload", quietly = TRUE)) {
  pkgload::load_all(pkg_path)
  if (!pkgload::is_dev_package(pkg_name(pkg_path))) {
    warning("package didn't load correctly")
  }
}
# Continue on with normal linting process

[F-Noelle]

Either that or the other way around, that lint gets a pkgload argument, which can be enabled but lint_package always loads the package, because there is frankly not much reason not to.

[AlexAxthelm]

If it's not a big lift, I think having an option to have lint_package() not load would be useful for CI systems (where the package gets installed anyway)

[MichaelChirico]

I might set load = requireNamespace("pkgload"), then if you want a lean CI that doesn't try to load, just make sure {pkgload} is unavailable, does that make sense?

[AshesITR]

Is the loading mechanic generalizable for non-package projects? Maybe we could support hooking this load.

e.g. if there are some common R script files that are sourced from several locations in the codebase to provide helper functions.

[MichaelChirico]

@klmr what might this look like for a box project?

[IndrajeetPatil]

FWIW, this seems to be a nifty package for users to apply lintr in box projects:

https://cran.rstudio.com/web/packages/box.linters/

[klmr]

@MichaelChirico I am on holidays without computer access this week. However, I would be very interested in a way to support this, and this has been a repeated question from ‘box’ users, so I will get back to you as soon as I am back home.

(Long story short, ‘lintr’ would need to execute the use() declaration in a way similar to what pkgload does.)

[radbasa]

@klmr , to make {box.linters} work we wrote a "hacky" get_box_module_exports() to get results similar to base::getNamespaceExports(). As far as we can tell, this does not attach the modules or functions to the environment. For now, this only works with modules, not packages. And it returns all functions and objects exported by a module.

We intend to contribute something similar to {box} itself that does both modules and packages. We are also considering handling the differences between mod/pkg, mod/pkg[...], and mod/pkg[attach_list]. And the aliases. At the moment, {box.linters} handles all of these in the linter functions themselves.

[IndrajeetPatil]

@radbasa Does {box.linters} also support modules from https://github.com/wahani/modules?

[radbasa]

@radbasa Does {box.linters} also support modules from https://github.com/wahani/modules?

{box.linters} is specific to how {box} works.