[R-package] [ci] static analysis CI job not compatible with {lintr} 3.0.0

[jameslamb]

Description

This project uses {lintr} to catch some types of efficiency and correctness issues and to enforce standard style across all its R code.

I noticed on @MichaelChirico 's Twitter tonight that a new major release of {lintr} will be published to CRAN soon.

https://twitter.com/michael_chirico/status/1528022047502217216

I tested {lightgbm} with that new release tonight, and found that this project's current linting setup raises some errors and warnings with that new version. If no changes are made, once {lintr} 3.0.0 is published to CRAN, LightGBM's linting CI job will start to break.

Reproducible example

Installed {lintr} from source on my Mac (R 4.1.0).

Rscript -e "remotes::install_github('https://github.com/r-lib/lintr.git')"

Then ran this project's linting.

Rscript .ci/lint_r_code.R $(pwd)/R-package

That produced the following errors and warnings.

Error: `fun` must be a function taking exactly one argument.
In addition: Warning messages:
1:  The use of linters of class 'function' was deprecated in lintr version 2.0.1.9001. Use linters classed as 'linter' (see ?Linter) instead.
2:  The use of linters of class 'function' was deprecated in lintr version 2.0.1.9001. Use linters classed as 'linter' (see ?Linter) instead.
Execution halted

After modifying LightGBM's CI code to get past that error, I saw the following errors.

Warning messages:
1: Linter closed_curly_linter was deprecated in lintr version 2.0.1.9001. Use brace_linter instead.
2: Linter open_curly_linter was deprecated in lintr version 2.0.1.9001. Use brace_linter instead.
3: Linter paren_brace_linter was deprecated in lintr version 2.0.1.9001. Use brace_linter instead.
4: Linter semicolon_terminator_linter was deprecated in lintr version 2.0.1.9001. Use semicolon_linter instead.

Also found that some of the R package's code will now fail the set of linters it's using. At first glance, these look to me like things I'd expected these linters to already catch, so they must be the benefit of bugfixes in {lintr}!

14 new linter failures (click me)

/Users/jlamb/repos/LightGBM/R-package/inst/make-r-def.R:74:20: warning: Use file.path() to construct portable file paths.
        pattern = "[Ordinal/Name Pointer] Table"
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
[[2]]
/Users/jlamb/repos/LightGBM/R-package/R/lgb.Booster.R:723:63: style: Trailing semicolons are not needed.
            list with attribute (name, value, higher_better)");
                                                              ^
[[3]]
/Users/jlamb/repos/LightGBM/R-package/R/lgb.convert_with_rules.R:7:33: style: Opening curly braces should never go on their own line and should always be followed by a new line.
            , FUN = function(x) {paste0(class(x), collapse = ",")}
                                ^
[[4]]
/Users/jlamb/repos/LightGBM/R-package/R/lgb.convert_with_rules.R:7:66: style: Closing curly-braces should always be on their own line, unless they are followed by an else.
            , FUN = function(x) {paste0(class(x), collapse = ",")}
                                                                 ^
[[5]]
/Users/jlamb/repos/LightGBM/R-package/R/lgb.convert_with_rules.R:43:51: style: Opening curly braces should never go on their own line and should always be followed by a new line.
.LGB_CONVERT_DEFAULT_FOR_LOGICAL_NA <- function() {return(-1L)}
                                                  ^
[[6]]
/Users/jlamb/repos/LightGBM/R-package/R/lgb.convert_with_rules.R:43:63: style: Closing curly-braces should always be on their own line, unless they are followed by an else.
.LGB_CONVERT_DEFAULT_FOR_LOGICAL_NA <- function() {return(-1L)}
                                                              ^
[[7]]
/Users/jlamb/repos/LightGBM/R-package/R/lgb.convert_with_rules.R:44:55: style: Opening curly braces should never go on their own line and should always be followed by a new line.
.LGB_CONVERT_DEFAULT_FOR_NON_LOGICAL_NA <- function() {return(0L)}
                                                      ^
[[8]]
/Users/jlamb/repos/LightGBM/R-package/R/lgb.convert_with_rules.R:44:66: style: Closing curly-braces should always be on their own line, unless they are followed by an else.
.LGB_CONVERT_DEFAULT_FOR_NON_LOGICAL_NA <- function() {return(0L)}
                                                                 ^
[[9]]
/Users/jlamb/repos/LightGBM/R-package/R/utils.R:72:58: style: Opening curly braces should never go on their own line and should always be followed by a new line.
    if (!all(sapply(interaction_constraints, function(x) {is.character(x) || is.numeric(x)}))) {
                                                         ^
[[10]]
/Users/jlamb/repos/LightGBM/R-package/src/install.libs.R:99:4: warning: Use file.path() to construct portable file paths.
  "../inst/bin/CMakeLists.txt"
   ^~~~~~~~~~~~~~~~~~~~~~~~~~
[[11]]
/Users/jlamb/repos/LightGBM/R-package/src/install.libs.R:125:6: warning: Use file.path() to construct portable file paths.
    "../../inst/make-r-def.R"
     ^~~~~~~~~~~~~~~~~~~~~~~
[[12]]
/Users/jlamb/repos/LightGBM/R-package/tests/testthat/test_basic.R:2424:12: warning: Use file.path() to construct portable file paths.
test_that("lgb.train() w/ linear learner fails already-constructed dataset with linear=false", {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[[13]]
/Users/jlamb/repos/LightGBM/R-package/tests/testthat/test_learning_to_rank.R:52:55: style: Opening curly braces should never go on their own line and should always be followed by a new line.
    expect_identical(sapply(eval_results, function(x) {x$name}), eval_names)
                                                      ^
[[14]]
/Users/jlamb/repos/LightGBM/R-package/tests/testthat/test_lgb.Booster.R:1206:20: style: Opening curly braces should never go on their own line and should always be followed by a new line.
    expect_warning({bst2 <- readRDS.lgb.Booster(file = model_file)})

Environment info

LightGBM version or commit hash: c000b8c

Command(s) you used to install LightGBM

sh build-cran-package.sh --no-build-vignettes
R CMD INSTALL --with-keep.source ./lightgbm_3.3.2.99.tar.gz

Additional Comments

Full list of changes in {lintr} 3.0.0: https://github.com/r-lib/lintr/blob/master/NEWS.md

[MichaelChirico]

Error: fun must be a function taking exactly one argument.

could you elaborate on this error / what you needed to do to fix it? the deprecation warnings are expected, errors, less so -- maybe there's a back-compatibility bug we need to address.

thanks for early testing!!

please also file bugs if you need some finer control of the linters or if you think anything there's a new false positive.

[jameslamb]

@MichaelChirico no problem! I'm only just starting to investigate this, and for now I just put up this issue so other maintainers here know that I'm working on it.

I can give you a single early finding, but I've only been looking into this for a few minutes so I'm not saying it's an issue with {lintr}.

The following code:

library(lintr)
f <- tempfile(fileext = ".R")
writeLines("print(1)", f)
lintr::lint(
    filename = f
    , linters = list(
        "absolute_path" = lintr::assignment_linter
    )
    , cache = FALSE
)

Produces this error:

Error: `fun` must be a function taking exactly one argument.

Changing from lintr::asignment_linter to lintr::assignment_linter() makes the error go away.

If that's not how you expect v3.0.0 to work, let me know and I'd be happy to open an issue over at https://github.com/r-lib/lintr/issues.

[MichaelChirico]

that's definitely the right usage going forward, but I'm a bit surprised we broke you. I thought we would just warn about the non-() usage in this release. filed #1174 to try and ease the transition.

thanks for the reprex!

[jameslamb]

As of #5240 and #5232, there are now just 4 warnings remaining that would be raised by {lintr} 3.0.0 for this project.

[1] Total linting issues found: 4
[[1]]
/Users/jlamb/repos/LightGBM/R-package/inst/make-r-def.R:74:20: warning: Use file.path() to construct portable file paths.
        pattern = "[Ordinal/Name Pointer] Table"
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

[[2]]
/Users/jlamb/repos/LightGBM/R-package/src/install.libs.R:99:4: warning: Use file.path() to construct portable file paths.
  "../inst/bin/CMakeLists.txt"
   ^~~~~~~~~~~~~~~~~~~~~~~~~~

[[3]]
/Users/jlamb/repos/LightGBM/R-package/src/install.libs.R:125:6: warning: Use file.path() to construct portable file paths.
    "../../inst/make-r-def.R"
     ^~~~~~~~~~~~~~~~~~~~~~~

[[4]]
/Users/jlamb/repos/LightGBM/R-package/tests/testthat/test_basic.R:2428:12: warning: Use file.path() to construct portable file paths.
test_that("lgb.train() w/ linear learner fails already-constructed dataset with linear=false", {
           ^~

[jameslamb]

As noticed in #5290, {lintr} 3.0.0 was released to CRAN today: https://cran.r-project.org/web/packages/lintr/index.html

I'm planning to put up a PR in the next day or two to get LightGBM's CI compliant with that new version.

[jmoralez]

@jameslamb are you working on this already? I think I can put up an initial PR. The required changes are removing the deprecated linter functions and adding the parenthesis?

[jameslamb]

I am working on it and was planning to put up a PR tonight. Now that #5290 is done there isn't a rush.

[github-actions]

This issue has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.