New allow_implicit_else argument for return_linter

[MichaelChirico]

Part of #884

With this, all of the internal linters we plan to "upstream" are now filed on the tracker 🎉

[AshesITR]

The linter should probably be made configurable. I can see initialize(...) as defined within R6 classes for example.

Also, the message implies explicit returns. Should be reworded to be agnostic of the return style if possible.

[MichaelChirico]

the message implies explicit returns

Right, that's why I'm thinking it may be best to subsume into return_linter(). Something like return_style = c("implicit", "allow_implicit_else", "explicit").

The linter should probably be made configurable

Not seeing the connection with R6, could you elaborate?

[AshesITR]

For implicit returns, the lint could also be desired because the implicit NULL might surprise.

Regarding R6:

Foo <- R6::R6Class(
  "Foo",
  public = list(
    initialize = function(x) {
      self$x <- x
    },
    x = NULL
  )
)

Shouldn't lint.

[AshesITR]

I think for implicit return style it could also be a good feature to lint terminal if without else.

Suggestion: allow_implicit_else = TRUE (default FALSE would need justification from the styleguide since return_linter() is a default linter)

[MichaelChirico]

I think for implicit return style it could also be a good feature to lint terminal if without else.

Suggestion: allow_implicit_else = TRUE (default FALSE would need justification from the styleguide since return_linter() is a default linter)

what is the lint for the implicit return case? that else NULL should be explicit?

[MichaelChirico]

Implemented under return_linter() with new argument allow_implicit_else, PTAL.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fbadcfc) 99.10% compared to head (ce5a9c7) 99.10%.

❗ Current head ce5a9c7 differs from pull request most recent head 743322d. Consider uploading reports for the commit 743322d to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2321   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files         125      125           
  Lines        5671     5689   +18     
=======================================
+ Hits         5620     5638   +18     
  Misses         51       51           

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

[MEO265]

I think there is still a bug in the marking in the new functionality.
The lint is displayed for line 1 in this example:

foo <- function(x, y = 3) {
  if(x) {
    return(x)
  }
}

with return_linter(return_style = "explicit", allow_implicit_else = FALSE).

And in this case there are two separate lints for the if:

foo <- function(x, y = 3) {
  if(x) {
    x
  }
}

Would it be ok if I add some test cases?

[MEO265]

Also, the message implies explicit returns. Should be reworded to be agnostic of the return style if possible.

@AshesITR unfortunately, I don't understand how this parameter should be understood in implicit style. Can you give an example that should be linted without this parameter but not?

I actually found the idea of ​​@MichaelChirico with the different types with gradations of explicitness very elegant

[MichaelChirico]

I think this double lint is not correct either you see the if after an else as a new expression, then only the second one should be marked, or you understand an if with a lot of else as one expression (that's how I would see it) and then only the first one would have to be marked.

lint(
  trim_some("
  function(x, y) {
    if(x) {
      1
    } else if(y) {
      2
    }
  }
  "),
  return_linter(return_style = "explicit", allow_implicit_else = FALSE)
)

<text>:2:9: style: [return_linter] All functions must have an explicit return().
  if(x) {
        ^
<text>:4:10: style: [return_linter] All functions must have an explicit return().
  } else if(y) {
         ^~~~~~~

I'm not entirely sure if it's a bug of this PR or the first one. If it's one of the first, feel free to convert this message into an issue, because then I'll probably have a (simple) solution to fix it and properly handle control statements of any depth.

Thanks @MEO265, arbitrary nesting is now handled in #2362 and adapted here -- your example now catches 3 lints (two missing return() calls, one missing else).

[MichaelChirico]

NB this is based against #2362 for now. That means we don't get GHA...

[MichaelChirico]

I was a bit worried about the preceding-sibling::expr and parent::expr here in the context of = or -> assignment. Added a test for the former; the latter is not relevant I think, because of operator precedence:

function() {
  ...
} -> .onLoad

Does not work as "intended" -- it actually parses like:

function() ( foo <- { } )

So right-assignment of a function would actually have to look like:

(function() {
  ...
}) -> .onLoad

That seems pretty unlikely to me, so ignoring it for now unless there's a user request.

[MichaelChirico]

I do see a handful of such assignments, looks like exclusively in the package {symbol.equation.gpt}, but I'm happy to ignore them, e.g.

https://github.com/cran/symbol.equation.gpt/blob/4c2d44d5f175f5ea5bbd540a1fd56d8cf0736f4f/R/aaa.R#L15-L22

https://github.com/cran/symbol.equation.gpt/blob/4c2d44d5f175f5ea5bbd540a1fd56d8cf0736f4f/R/main.R#L14-L20

https://github.com/search?q=lang%3AR+%2F%5B%7D%5D%5B%29%5D%5Cs*%5B-%5D%5B%3E%5D%2F+%2F%5E%5B%28%5D%5Cs*function%5Cs*%5B%28%5D%2F&type=code