[R-package] Add explicit return statement to R functions.

[zenggyu]

Closes #3352.

Hi, @jameslamb and @Juniper-23 , and happy new year!

I noticed that the issue has been opened for quite some time and is still not fixed, so I thought I'd give it a try. You may want to merge this PR in case @Juniper-23 does not have time to create one.

There may be some code formatting issues in this PR though. I found two formatting styles in the original files when the code in return() is long (e.g., exceeding 80 characters), for example:

# style1
return(lgb.call(
  fun_name = "LGBM_BoosterGetCurrentIteration_R"
  , ret = cur_iter
  , private$handle
))

# style2
return(
  lgb.call(
    fun_name = "LGBM_BoosterGetCurrentIteration_R"
    , ret = cur_iter
    , private$handle
  )
)

Tell me which one you prefer and I'll be happy to make a few more edits!

[zenggyu]

The commits have passed all the tests on my laptop but failed many GitHub Actions checks. Do you know why?

[jameslamb]

Thanks very much! I think it's ok to proceed with this, since #3352 has been open for a few months. @Juniper-23 I hope you'll come back to LightGBM and contribute when you have time!

---

There may be some code formatting issues in this PR though. I found two formatting styles in the original files when the code in return() is long

Thanks for identifying this inconsistency. I prefer this style:

return(
  lgb.call(
    fun_name = "LGBM_BoosterGetCurrentIteration_R"
    , ret = cur_iter
    , private$handle
  )
)

Could you please do that for any of the multi-line return statements you see? Both those this PR touches and any that currently exist in the package with the other style like return(lgb.call(....

---

Could you please also add return() statements to these calls?

• build_r.R:
  • .handle_result() should have return(invisible(NULL))
• lgb.plot.interpretation.R
  • lgb.plot.interpretation() should have return(invisible(NULL))

---

The commits have passed all the tests on my laptop but failed many GitHub Actions checks. Do you know why?

This is coming from R CMD check. It looks like somewhere in this PR, you have a typo and accidentally typed inivisible instead of invisible.

* checking R code for possible problems ... � � NOTE
cb.reset.parameters : init: no visible global function definition for
  ‘inivisble’
Undefined global functions or variables:
  inivisble

You can find this by clicking "details" on any of the failed builds below, which will take you to the logs: https://github.com/microsoft/LightGBM/pull/3703/checks?check_run_id=1633145099.

You can reproduce this locally too:

sh build-cran-package.sh
R CMD check lightgbm_*.tar.gz --as-cran

The unit tests passing but these checks failing proobably means that this typo occurred in a place that isn't covered by {lightgbm}'s unit tests yet.

[jameslamb]

instead of doing this, can you please just add a single return(invisible(NULL)) outside of the if-else block?

[zenggyu]

Adding these modifications results in many GitHub Action failures. Judging from the log messages, it seems that the failures are caused by unit tests:

* checking tests ...
  Running ‘testthat.R’
 � � ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  Length mismatch: comparison on first 0 components
  ── Failure (test_lgb.Booster.R:454:5): Booster$params should include dataset params, before and after Booster$reset_parameter() ──
  ret_bst$params not identical to `expected_params`.
  Length mismatch: comparison on first 3 components
  ── Failure (test_lgb.Booster.R:455:5): Booster$params should include dataset params, before and after Booster$reset_parameter() ──
  bst$params not identical to `expected_params`.
  Length mismatch: comparison on first 3 components
  ── Failure (test_lgb.Booster.R:723:5): params (including dataset params) should be stored in .rds file for Booster ──
  bst_from_file$params not identical to list(...).
  names for current but not for target
  Length mismatch: comparison on first 0 components
  
  [ FAIL 5 | WARN 0 | SKIP 3 | PASS 605 ]
  Error: Test failures
  Execution halted

However, when I run R CMD check --as-cran on my laptop (Ubuntu 20.04, R 4.0.2), it seemed to be OK:

* checking tests ...
  Running ‘testthat.R’
 OK

This is confusing, and I can't find the failed tests in test_lgb.Booster.R. Can you help?

[zenggyu]

I just realized that you have merged #3662, which contains the failed tests, into master. I guess that's the reason, right? However, this is confusing: why does this PR run tests from branch master instead of its own branch (fix-3352)? Can/Should we set up GitHub Action in a way so that each branch runs its own tests, or am I missing anything?

[jameslamb]

CI runs against the merger of your PR and its target branch (in this case master). You can see that in the "Checkout repository" step in the CI jobs: https://github.com/microsoft/LightGBM/pull/3703/checks?check_run_id=1639476038.

/usr/bin/git checkout --progress --force refs/remotes/pull/3703/merge

This is the behavior of most CI services, and it's done to test that master would still be a working branch after your changes are merged.

This is also buried deep in the git docs, at https://git-scm.com/book/id/v2/GitHub-Maintaining-a-Project.

The eagle-eyed among you would note the head on the end of the remote portion of the refspec. There’s also a refs/pull/#/merge ref on the GitHub side, which represents the commit that would result if you push the “merge” button on the site

So those build failures are desirable. They're telling us that CI would start to fail if we merged this PR into master as-is.

Please merge master into this branch and test. Let me know if you have other questions!

[jameslamb]

I think this place MIGHT be related to the test failures you saw. env$model$reset_parameter(params = pars) returns a Booster, so the current behavior of this function is that that booster is returned if this code runs:

    if (!is.null(env$model)) {
      env$model$reset_parameter(params = pars)
    }

This return(invisible(NULL)) is correct, but please also add a return() inside that if statement above.

This kind of thing is why this PR is so valuable! To find places like this where {lightgbm} functions' return values are non-obvious. So thank you!

[zenggyu]

It turns out that the test failures were caused by a misplaced return() statement resulted from the merge. I have fixed it in commit 1f8661a .

[jameslamb]

Thanks for the changes @zenggyu . Can you please update to master again? We just merged a PR that removes the flaky GitHub Actions job R GitHub Actions / r-package (windows-latest, MINGW, R 3.6, cmake) (pull_request) , which is failing here for reasons unrelated to your code.

To replicate the linting errors you're facing on Travis, you can run Rscript .ci/lint_r_code.R $(pwd)/ from the root of this repo.

[jameslamb]

This is great, thanks so much for contributing!

[github-actions]

This pull request 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.