Extend syntax to check return types as well

[jimhester]

Not exactly sure of the format, perhaps just extending to replace any call of ? with the checking code would work. I can't really think of a time where ? is used within a function, so I don't think it would conflict with any existing code.

fun <- function(x) (if (isTRUE(x)) 1 else "1") ? is.character

fun(TRUE)
# Error: is.character((if (isTRUE(x)) 1 else "1"))) is not TRUE

[gaborcsardi]

Good idea, and I think it would be great to define it up front, so that one can see what to expect just by looking at the function header.

How this could be implemented is another question. With an extra function call it is easy I guess, without that we might redefine return, and also extend the last expression of the function, but this looks messy.

[jimhester]

Possible syntax ideas

# this does not parse
f <- function(x) ? is.numeric { 1 }
#> Error: unexpected '{' in "f <- function(x) ? is.numeric {"

# This does though
f <- function(x) ? is.numeric ? { 1 }

# Also this does
f <- function(x) { 1 } ? is.numeric

# But that might be weird for 1 expression functions?
f <- function(x) 1 ? is.numeric

I think some meta-programming would work well for this. Search through the AST and replace any return() calls with the checking code followed by the return. Then replace the last call in the function with the checking code (unless it is an explicit return and has already been done).

I may try an implementation tomorrow if I can get some time.

[gaborcsardi]

This sounds a little scary. :) Isn't it simpler to redefine return()? Plus deal with the last expression separately.

As for syntax, the second one is not bad. The third one puts the return type after the function body, so I don't really like that.

[jimhester]

Maybe just redefining return() is simpler, I guess there would be slightly more overhead in the case where there were no returns in the functions.

Another syntax alternative is putting the ? after the check (which does parse). Not sure which I like better though...

f1 <- function(x) is.numeric ? { 1 }

f1 <- function(x) is.numeric ? 1

[jimhester]

One downside to both the approaches with the check right after the arguments is our definition of ? when not using argufy has to change because the real statements will be the second argument rather than the first.

Right now given a function f1()

f1 <- function(x = ? is.numeric) as.character(x)

If you define ? as follows it works without modification (and without argufying)

`?` <- function(e1, e2) e1

This allows you to easily write argufyable functions that work (without the checks) even if argufy is not installed

if (requireNamespace("argufy"), quietly = TRUE) argufy::argufy_package() else `?` <- function(e1, e2) e1

However the replacement function would have to get more complicated if we were to use the first argument for the checks in some cases.

Therefore I think it is actually best to put it at the end of the definition.

f1 <- function(x = ? is.numeric) as.character(x) ? is.character

So that this function still works when argufy does not exist.

[gaborcsardi]

Now that the assertions are in the docs, this is easy, of course, as least the syntax. You can just do something like:

#' @return \assert{is.integer(.)} The number of rows in the database.

Do we want to allow coercions as well?

As for the implementation, we can take a look at Duncan's BioC package mentioned in #21, and then come up with something reasonable.

[jimhester]

The easiest thing to do is just construct a call to the checking function with whatever is in the body. If it is bare it is directly passed, if it uses {} than the return value will be passed to the function.

fun <- function(x) x
b <- body(fun)
chk_fun <- quote(is.numeric)
chk <- bquote(.(fun)(.(body)), as.environment(list(fun = chk_fun, body = b)))
body(fun) <- chk
fun
#> function (x) 
#> is.numeric(x)
#> <environment: 0x7ff1a357b5f0>

[gaborcsardi]

I think this could work, although it is somewhat more difficult, because you want to return x, not the logical from the assertion:

function(x) {
  if (is.numeric(`_` <- (x))) `_`
}

It might also mess up some functions that we pollute the stack. E.g. if a function calls parent.frame, we might break that, right?

It might mess up visibility of return values as well, I think.

[jimhester]

Good points, using withVisible should allow us to pass on the visibility. The parent.frame should be OK, but I the call stack will be affected of course. I don't see a way to avoid that.

fun <- function(x) {str(parent.frame()); invisible(x)}
fun2 <- fun
b <- body(fun)
chk_fun <- quote(is.numeric)
chk <- bquote({
  `_` <- withVisible(.(body))
  stopifnot(.(fun)(`_`$value))
  if (`_`$visible) `_` else invisible(`_`$value)
}, as.environment(list(fun = chk_fun, body = b)))
body(fun) <- chk
fun
#> function (x) 
#> {
#>     `_` <- withVisible({
#>         str(parent.frame())
#>         invisible(x)
#>     })
#>     stopifnot(is.numeric(`_`$value))
#>     if (`_`$visible) 
#>         `_`
#>     else invisible(`_`$value)
#> }
#> <environment: 0x7ff1abc0aa68>

fun(5)
#> <environment: 0x7ff1abc0aa68>
(fun(5))
#> <environment: 0x7ff1abc0aa68>
#> [1] 5
fun2(5)
#> <environment: 0x7ff1abc0aa68>
(fun2(5))
#> <environment: 0x7ff1abc0aa68>
#> [1] 5

[gaborcsardi]

This is quite good. My only very minor complaint is that it messes up the code of the function somewhat, it will be harder to read, especially smaller functions.

I'll add this soon.

[jimhester]

If we implement it like previously and copy the srcref attribute of the function the srcref will be used for display, so the checking code won't be visible unless you explicitly print it.

[gaborcsardi]

Unfortunately there is no srcref by default because packages are parsed without it.

In theory we could re-parse and add it, but I am not sure if we want this, as it'll make the installed packages significantly bigger.

[jimhester]

We could define a print method for argufied functions that prints the original function definition rather than including the argufied expressions. It would have to store the original definition in an attribute.

[gaborcsardi]

Certainly a possibility to keep in mind.

TBH I am not so keen on doing it, I feel that the less extra we put on the function the better, and adding a class is a big change IMO.

I think for now, the default print method will do, even if not ideal.