Unused import linter

[jimhester]

This still needs some more testing, and tests written, but works with limited inputs.

[codecov-io]

Codecov Report

Merging #239 into master will decrease coverage by 1.51%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #239      +/-   ##
=========================================
- Coverage   85.91%   84.4%   -1.52%     
=========================================
  Files          39      40       +1     
  Lines        2179    2218      +39     
=========================================
  Hits         1872    1872              
- Misses        307     346      +39

Impacted Files	Coverage Δ
R/unused_import_linter.R	0% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d64b9a...f52af6b. Read the comment docs.

[AshesITR]

This looks quite useful. Should we try to get it up to speed and add tests?

If so, for packages we should check for functions not used but imported in NAMESPACE instead of loaded via require / library.

[jimhester]

Yeah, I think that is right.

[AshesITR]

Rebased onto current HEAD as a first step.

[AshesITR]

@jimhester I think we have a problem for packages:
Without scanning all source files, we can never know if an import is truly never used.

WDYT about keeping it restricted to library() and require() calls for the moment?

[jimhester]

yeah that is true, we would have to scan for all source in the package. R CMD check already does this for packages, so I think it is fine to restrict this only to scripts.

[AshesITR]

NB: bit64 and data.table should also be exempt by default -- both cause other code to behave differently if loaded.

[AshesITR]

@MichaelChirico PTAL I've updated this PR and rebased onto current master.

[MichaelChirico]

What about packages that are used to load methods? I can't think of any example off-hand, but I'm imagining library(pkg) is used to make methods like print() or [ available. I guess it should be rare enough that users can just supply this to the exception argument?

[AshesITR]

That's exactly why the bit64 and data.table packages are excluded by default. LMK if there are more packages that need such an exclusion.

[AshesITR]

#1090 should fix the build issue.

[AshesITR]

Ready for another round of review.

[MichaelChirico]

Remind me why data.table is excluded? maybe a simple example of a script that needs library(data.table) but doesn't include any data.table exports. Can't think of one right now.

[MichaelChirico]

Looks like my added test isn't caught, please check the test is right & adjust the character.only xpath if so

[AshesITR]

Remind me why data.table is excluded? maybe a simple example of a script that needs library(data.table) but doesn't include any data.table exports. Can't think of one right now.

dt <- readRDS("a/data.table.RDS")
dt[col == 1]

It's about the [.data.table generic.

[MichaelChirico]

that doesn't trigger a lint though:

lintr::lint("dt <- readRDR('a/data.table.RDS')\ndt[col == 1]", lintr::unused_import_linter())

[AshesITR]

that doesn't trigger a lint though:

lintr::lint("dt <- readRDR('a/data.table.RDS')\ndt[col == 1]", lintr::unused_import_linter())

Because it's a default?
Does unused_import_linter(except_packages = character())?

[MichaelChirico]

OK, nvm. understood. Technically, that example is not the best because users should be running setDT() on the readRDS object:

Rscript -e "saveRDS(data.table::as.data.table(iris), 'tmp')"
Rscript -e "data.table::truelength(readRDS('tmp'))"
# [1] 0
rm tmp

Maybe data.table should be dropped as an exclusion? The example you gave is kind of a code smell, but then again that's not the purpose of this particular linter at all.

[AshesITR]

Well we actually have this in internal ETL code where we know for sure the RDS contains a data.table because it's written by an adjacent file.

The goal of except_packages is to avoid false positives.

[MichaelChirico]

Well we actually have this in internal ETL code where we know for sure the RDS contains a data.table because it's written by an adjacent file.

IINM without setDT(), we can load the table but it won't be a "properly" functioning data.table, e.g. we can't add columns:

https://stackoverflow.com/q/28078640/3576984

Anyway, besides the point of this PR, and I'm OK with the default exceptions list. Last thing to fix is the added test for character.only & we're good to go

[AshesITR]

Well we actually have this in internal ETL code where we know for sure the RDS contains a data.table because it's written by an adjacent file.

IINM without setDT(), we can load the table but it won't be a "properly" functioning data.table, e.g. we can't add columns:

https://stackoverflow.com/q/28078640/3576984

good to know that readRDS can mess things up - we mostly use feather locally, getting the data tables using fst::read_fst(..., as.data.table = TRUE) so that would be a more valid example.