Linter for explicit/implicit returns #2271

MEO265 · 2023-11-10T15:56:11Z

Closes #1100.

I have designed the implementation in such a way that as far as possible no false positive errors occur. This leads to a relatively large number of false negatives, but I would like to accept that.
If anyone can construct false positive cases, I would be grateful if you would write them in the comments.

Inline functions have been ignored so far, this is to a certain extent intentional, as I personally am a supporter of explicit returns, but find them unnecessary in inline functions. On the other hand, this case would have become much more complex.
If someone ever wants to implement it, I think an lint_inline parameter would be nice.

FYI:
There are 10 explicit returns (that the linter finds) in the linter package and although I prefer them, they should probably be removed for consistency.

codecov-commenter · 2023-11-10T16:00:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (97182b7) 99.40% compared to head (63e0e2b) 99.41%.

❗ Current head 63e0e2b differs from pull request most recent head 325205c. Consider uploading reports for the commit 325205c to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2271   +/-   ##
=======================================
  Coverage   99.40%   99.41%           
=======================================
  Files         123      124    +1     
  Lines        5590     5640   +50     
=======================================
+ Hits         5557     5607   +50     
  Misses         33       33

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AshesITR · 2023-11-10T17:22:32Z

The warnings stem from exclusions of linters that you removed by providing linters=.
To avoid them, add return_linter() to our .lintr instead.

MichaelChirico · 2023-11-10T18:20:30Z

Hmm, we do already have an explicit_return_linter() [our default conflicts with the Tidyverse guide & we require all returns to be explicit]. Seems like we've got some duplicate effort here. OTOH yours already goes further by trying to support both styles (implicit + explicit)

We have tested ours pretty extensively (no false positives on 10,000+ files) -- here's all the tests we've written:

https://gist.github.com/MichaelChirico/9164672a4e762627c09ad6b521e54964

Please ensure these tests pass under use_implicit_returns = FALSE

Here's our XPath:

https://gist.github.com/MichaelChirico/084bc163c01ad0debba8b1b945b75009

The major issue I'm aware of with our implementation is it doesn't handle arbitrary nesting in terminal if/else branches -- we only look at top-level if/else (i.e. depth=1).

See #884 for linters we've already written that are just a matter of porting over.

MichaelChirico · 2023-11-10T18:22:31Z

Inline functions have been ignored so far

I agree we shouldn't require return() on inline functions -- that's how we implemented it as well.

MichaelChirico · 2023-11-10T18:24:02Z

There are 10 explicit returns (that the linter finds) in the linter package and although I prefer them, they should probably be removed for consistency.

Most likely, these are from copy-pasting our internal code where the styles are opposite. Yes, we should remove them here for consistency (and because we're meant to enforce the tidyverse guide)

MEO265 · 2023-11-10T20:22:07Z

We have tested ours pretty extensively (no false positives on 10,000+ files) -- here's all the tests we've written:

https://gist.github.com/MichaelChirico/9164672a4e762627c09ad6b521e54964

Please ensure these tests pass under use_implicit_returns = FALSE

Just by looking at it I can tell that my linter would produce lots of false positives. Many that one should definitely pay attention to, e.g. invokeRestart, tryInvokeRestart, UseMethod, etc. and others that I would say are actually marked correctly, namely message, stopifnot and maybe even warning. And when it comes to controls like for and while, my code is also a little stricter.

Maybe in addition to a fixed number of basic functions that are taken into account in the code, there should be the option to accept more functions (from other packages).

Here's our XPath:

https://gist.github.com/MichaelChirico/084bc163c01ad0debba8b1b945b75009

The major issue I'm aware of with our implementation is it doesn't handle arbitrary nesting in terminal if/else branches -- we only look at top-level if/else (i.e. depth=1).

Thanks for access to the code. I see that you have already paid a lot more attention. What is the best way to proceed? I can try to bring the two together and make sure everything passes, although that might take a while (which doesn't bother me), but if you want to take over that would be ok too.

See #884 for linters we've already written that are just a matter of porting over.

Is there a way to see the code of all linters still to be transferred or to get access to certain ones? If so, I would like to try to support whenever my time allows.

MEO265 · 2023-11-10T20:29:03Z

The warnings stem from exclusions of linters that you removed by providing linters=. To avoid them, add return_linter() to our .lintr instead.

Thanks for the answer, what surprised me is that not a single warning came when I called up my linter without argument

MichaelChirico · 2023-11-10T20:51:40Z

Is there a way to see the code of all linters still to be transferred or to get access to certain ones?

Not easily... I have to strip some internal-only pieces from the code before uploading them here. If you'd like to see specific linters I am happy to do so one a one-by-one basis.

MichaelChirico · 2023-11-10T20:57:12Z

when it comes to controls like for and while, my code is also a little stricter.

I think I'd be open to simplifying things a bit -- always lint on a terminal for/while/repeat, requiring the function to terminate with return(invisible()) instead (or perhaps a stop() like you mentioned in case the function is meant to return early). I think I didn't realize at the time we initially implemented this that those control flows always return NULL implicitly:

foo <- function() {
  for (ii in 1:10) {
    ii
  }
}
x <- foo()
dput(x)
# NULL

So under explicit return style, IMO it's always preferable to make that behavior more explicit:

foo <- function() {
  for (ii in 1:10) {
    ii
  }
  return(invisible())
}

MichaelChirico · 2023-11-10T21:00:18Z

Maybe in addition to a fixed number of basic functions that are taken into account in the code, there should be the option to accept more functions (from other packages).

Yes, that's what I would do... one issue is the list is already a bit long, the signature will get pretty messy if the default list is huge. So I would propose the parameter being extra functions to skip.

Something like

# User has no control here; all of these come from base
default_allowed_functions <- c(
  # Normal calls
  "return", "stop", "warning", "message", "stopifnot", "q", "quit",
  "invokeRestart", "tryInvokeRestart",

  # Functions related to S3 methods
  "UseMethod", "NextMethod",

  # Functions related to S4 methods
  "standardGeneric", "callNextMethod",

  # Functions related to C interfaces
  ".C", ".Call", ".External", ".Fortran"
)

# User supplies these; internally we would use this
extra_allowed_functions <- c(
  # Normal calls from non-default libraries
  "LOG", "abort",

  # tests in the RUnit framework are functions ending with a call to one
  #   of the below. would rather users just use a different framework
  #   (e.g. testthat or tinytest), but already 250+ BUILD files depend
  #   on RUnit, so just cater to that. confirmed the efficiency impact
  #   of including these is minimal.
  # RUnit tests look like 'TestInCamelCase <- function()'
  #   NB: check for starts-with(text(), 'Test') below is not sufficient, e.g.
  #   in cases of a "driver" test function taking arguments and the main unit
  #   test iterating over those.
  "checkEquals", "checkEqualsNumeric", "checkException", "checkIdentical",
  "checkStop", "checkTrue", "checkWarnings",
)

AshesITR · 2023-11-11T08:39:28Z

I'm assuming "LOG" is internal to google?
In that case, I'd remove it from the official defaults.

Agree we should silently add all base R exceptions and maybe rlang (abort)?
Or keep "abort" in the signature to hint at the usage, dropping RUnit support out of the box?

MichaelChirico · 2023-11-11T14:50:01Z

I'm assuming "LOG" is internal to google? In that case, I'd remove it from the official defaults.

Agree we should silently add all base R exceptions and maybe rlang (abort)? Or keep "abort" in the signature to hint at the usage, dropping RUnit support out of the box?

SGTM, though maybe should add any other signalling calls from rlang as well? not super familiar with what's offered, I think warn()?

AshesITR · 2023-11-12T08:48:16Z

Actually, why allow warning() as the last call? It quietly returns the message as a string.

MichaelChirico · 2023-11-12T17:23:34Z

Actually, why allow warning() as the last call? It quietly returns the message as a string.

Hmm, not sure we noticed that at the time. I do find it very strange that behavior differs from message() (which returns NULL).

Anyway, for our case I would still skip return(). I think requiring it for warning() would make tryCatch()/withCallingHandlers() usage overly verbose, e.g.

tryCatch(
  foo(),
  error = \(e) {
    cat(Sys.time(), file="log")
    cat(e$message, file="log", append=TRUE)
    warning(e)
  }
)

Treating all signalling functions the same makes more sense to me.

That said, for {lintr}'s defaults, we can consider something different.

R/return_linter.R

AshesITR · 2023-11-16T23:35:12Z

Maybe instead of allowing warning() etc. we should carve out functions passed to withCallingHandlers() and friends?

MEO265 · 2023-11-20T12:11:21Z

@MichaelChirico I think it would be really useful, even outside of this PR, if we included some of the functions you use for glue, like .XpTextInTable, in the package here too. Would make some linter easier to read.
Or is it already in the package under a different name and I just overlooked it?

MichaelChirico · 2023-11-20T14:53:42Z

@MichaelChirico I think it would be really useful, even outside of this PR, if we included some of the functions you use for glue, like .XpTextInTable, in the package here too. Would make some linter easier to read. Or is it already in the package under a different name and I just overlooked it?

in {lintr} that's xp_text_in_table, see xp_utils.R for similar helpers

…feature/return_linter

MichaelChirico · 2023-11-23T03:19:31Z

Dropped RUnit support for now, to be handled in follow-up
Renamed all three parameters
- use_implicit_returns ➡️ return_style. This already reads better to me, and as hinted in New allow_implicit_else argument for return_linter #2321, might get more options in the future
- additional_allowed_func ➡️ return_functions
- additional_side_effect_func ➡️ except

MEO265 · 2023-11-23T06:22:09Z

@MichaelChirico as I see it, you've already done everything or is there still something to do for me?
Thanks anyway.

MichaelChirico · 2023-11-23T06:29:52Z

@MichaelChirico as I see it, you've already done everything or is there still something to do for me? Thanks anyway.

yes, I tried to clear the pending tasks. good for another round of review in case there's anything pending needed for initial merge.

MEO265 · 2023-11-23T06:41:38Z

@MichaelChirico as I see it, you've already done everything or is there still something to do for me? Thanks anyway.

yes, I tried to clear the pending tasks. good for another round of review in case there's anything pending needed for initial merge.

Okay, I don't have any more comments from my side

AshesITR · 2023-11-23T06:53:39Z

Nice. I'll try to review later this day.

MichaelChirico · 2023-11-23T16:32:58Z

R/return_linter.R

+  } else {
+    # See `?.onAttach`; these functions are all exclusively used for their
+    #   side-effects, so implicit return is generally acceptable
+    default_except <- c(".onLoad", ".onUnload", ".onAttach", ".onDetach", ".Last.lib")


just noticed we could probably re-use this:

lintr/R/object_name_linter.R

Lines 186 to 194 in 1c36e0d

special_funs <- c(

".onLoad",

".onAttach",

".onUnload",

".onDetach",

".Last.lib",

".First",

".Last"

)

* remove incorrect comment in default_linter_testcode.R * fix NEWS entry (argument is called `return_style`) * reuse `special_funs` constant * convert all tests from lines <- c(...) to trim_some()

AshesITR · 2023-11-23T20:36:47Z

@MichaelChirico PTAL, I've fixed what I found directly on the branch.
I think we can merge now.

AshesITR · 2023-11-23T20:08:14Z

tests/testthat/default_linter_testcode.R

@@ -20,6 +25,7 @@ f = function (x,y = 1){}
 # object_name
 # object_usage
 # open_curly
+# return


This is inaccurate. return_linter() doesn't lint the following function.

MichaelChirico · 2023-11-24T01:47:43Z

tests/testthat/default_linter_testcode.R

@@ -25,7 +25,6 @@ g <- function(x) {
 # object_name
 # object_usage
 # open_curly
-# return


oh! in some commit I added an explicit return here. Must not have survived. I see someone (possibly me?) added a return_linter() test case to this file in another commit.

MichaelChirico · 2023-11-24T01:48:19Z

tests/testthat/test-return_linter.R

-    "  FALSE",
-    "}"
+  expect_lint(
+    trim_some("


thanks for cleaning these up to use trim_some()!

MichaelChirico · 2023-11-24T01:50:01Z

Great work everyone and thanks for the patience! 70 comments, that may be a record :)

MEO265 added 7 commits November 9, 2023 07:10

feat: Add return_linter

bc9f197

feat: Dont lint deterministically returning control statements

2a54612

test: Accept false negatives

293e4a2

feat: Do not lint stop

842a006

feat: Refined lint of switch

cda5823

test: Add line tests

c091d1f

doc: Mark as configurable

067a33d

MEO265 added 2 commits November 10, 2023 17:14

mnt: Add terminal new lines

94955ed

Merge branch 'main' into feature/return_linter

8a27435

MichaelChirico reviewed Nov 16, 2023

View reviewed changes

R/return_linter.R Outdated Show resolved Hide resolved

MichaelChirico added the google-linters label Nov 18, 2023

This was referenced Nov 20, 2023

Upstreaming linters from Google's internal linting suite #884

Closed

New allow_implicit_else argument for return_linter #2321

Merged

AshesITR mentioned this pull request Nov 22, 2023

Extend return_linter to allow prefix exclusions #2335

Closed

MichaelChirico added 7 commits November 23, 2023 02:44

Merge branch 'main' into feature/return_linter

ca945bc

Merge branch 'feature/return_linter' of github.com:MEO265/lintr into …

f7afa54

…feature/return_linter

drop runit support for now

91820bd

style

e1961d5

rename parameter to accept "implicit"/"explicit"

5248af9

rename other parameters

8612b70

corresponding changes to tests

aff7765

MichaelChirico mentioned this pull request Nov 23, 2023

return_linter() should allow switch() where every entry has an exit call #2343

Closed

dont link R4.0+ tryInvokeRestart, which is in linked page already anyway

28b51f4

Merge branch 'main' into feature/return_linter

cb2d9c4

MichaelChirico reviewed Nov 23, 2023

View reviewed changes

AshesITR added 2 commits November 23, 2023 21:06

Merge branch 'main' into MEO265-feature/return_linter

8a0ea77

review and fixes

63eba24

* remove incorrect comment in default_linter_testcode.R * fix NEWS entry (argument is called `return_style`) * reuse `special_funs` constant * convert all tests from lines <- c(...) to trim_some()

AshesITR previously approved these changes Nov 23, 2023

View reviewed changes

document()

f89c8bc

AshesITR dismissed their stale review via f89c8bc November 23, 2023 20:58

AshesITR approved these changes Nov 23, 2023

View reviewed changes

Merge branch 'main' into feature/return_linter

325205c

MichaelChirico reviewed Nov 24, 2023

View reviewed changes

MichaelChirico merged commit 30c7d70 into r-lib:main Nov 24, 2023
20 checks passed

MEO265 deleted the feature/return_linter branch November 25, 2023 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linter for explicit/implicit returns #2271

Linter for explicit/implicit returns #2271

MEO265 commented Nov 10, 2023 •

edited

Loading

codecov-commenter commented Nov 10, 2023 •

edited

Loading

AshesITR commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023 •

edited

Loading

MichaelChirico commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023

MEO265 commented Nov 10, 2023 •

edited

Loading

MEO265 commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023

AshesITR commented Nov 11, 2023

MichaelChirico commented Nov 11, 2023

AshesITR commented Nov 12, 2023

MichaelChirico commented Nov 12, 2023 •

edited

Loading

AshesITR commented Nov 16, 2023

MEO265 commented Nov 20, 2023

MichaelChirico commented Nov 20, 2023

MichaelChirico commented Nov 23, 2023

MEO265 commented Nov 23, 2023

MichaelChirico commented Nov 23, 2023

MEO265 commented Nov 23, 2023

AshesITR commented Nov 23, 2023

MichaelChirico Nov 23, 2023

AshesITR commented Nov 23, 2023

AshesITR Nov 23, 2023

MichaelChirico Nov 24, 2023

MichaelChirico Nov 24, 2023

MichaelChirico commented Nov 24, 2023

	special_funs <- c(
	".onLoad",
	".onAttach",
	".onUnload",
	".onDetach",
	".Last.lib",
	".First",
	".Last"
	)

Linter for explicit/implicit returns #2271

Linter for explicit/implicit returns #2271

Conversation

MEO265 commented Nov 10, 2023 • edited Loading

codecov-commenter commented Nov 10, 2023 • edited Loading

Codecov Report

AshesITR commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023 • edited Loading

MichaelChirico commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023

MEO265 commented Nov 10, 2023 • edited Loading

MEO265 commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023

MichaelChirico commented Nov 10, 2023

AshesITR commented Nov 11, 2023

MichaelChirico commented Nov 11, 2023

AshesITR commented Nov 12, 2023

MichaelChirico commented Nov 12, 2023 • edited Loading

AshesITR commented Nov 16, 2023

MEO265 commented Nov 20, 2023

MichaelChirico commented Nov 20, 2023

MichaelChirico commented Nov 23, 2023

MEO265 commented Nov 23, 2023

MichaelChirico commented Nov 23, 2023

MEO265 commented Nov 23, 2023

AshesITR commented Nov 23, 2023

MichaelChirico Nov 23, 2023

Choose a reason for hiding this comment

AshesITR commented Nov 23, 2023

AshesITR Nov 23, 2023

Choose a reason for hiding this comment

MichaelChirico Nov 24, 2023

Choose a reason for hiding this comment

MichaelChirico Nov 24, 2023

Choose a reason for hiding this comment

MichaelChirico commented Nov 24, 2023

MEO265 commented Nov 10, 2023 •

edited

Loading

codecov-commenter commented Nov 10, 2023 •

edited

Loading

MichaelChirico commented Nov 10, 2023 •

edited

Loading

MEO265 commented Nov 10, 2023 •

edited

Loading

MichaelChirico commented Nov 12, 2023 •

edited

Loading