.throw_error() and .throw_warning()

[mcol]

In 0.9.25 .throw_error() and .throw_warning() would sometimes choose the wrong frame in looking for the name of the calling function to report. This could happen when the throwing functions were nested in an lapply(), do.call() or similar and nframe was not correctly set. However, this was guaranteed to happen when a function was called via an S3 method, as in that case the nframe specified in the real function would not account for the fact that that function was called via S3 dispatch.

In those cases we reported something like this:

# run on v0.9.25
data(ExampleData.RLum.Analysis, envir = environment())
temp <- IRSAR.RF.Data
subset(temp, LTYPE == "RF")
# Error: [(new("nonstandardGenericFunction", .Data = function (object, ] Invalid subset expression,
# valid terms are:  curveType, recordType[    ...) ] Invalid subset expression, valid terms are: curveType,
# recordType[{] Invalid subset expression, valid terms are: curveType, recordType[    
# standardGeneric("get_RLum")] Invalid subset expression, valid terms are: curveType, recordType[}, 
# generic = structure("get_RLum", package = "Luminescence"), ] Invalid subset expression, valid terms
# are: curveType, recordType[    package = "Luminescence", group = list(), valueClass = character(0), ] 
# Invalid subset expression, valid terms are: curveType, recordType[    signature = "object", default = 
# NULL, skeleton = (function (object, ] Invalid subset expression, valid terms are: curveType, recordType[ 
# ...) ] Invalid subset expression, valid terms are: curveType, recordType[    stop(gettextf("invalid call
# in method dispatch to '%s' (no default method)", ] Invalid subset expression, valid terms are:

The changes in 57c549a have fixed such cases, but now the code in those two functions is much more complicated and it can result in "unknown()" being reported:

# run on current master
data(ExampleData.RLum.Analysis, envir = environment())
temp <- IRSAR.RF.Data
subset(temp, LTYPE == "RF")
# Error: [unknown()] Invalid subset expression, valid terms are: curveType, recordType

We need to bypass the automatic resolution of the name of calling function.

[mcol]

Unfortunately, playing around with sys.call(), sys.calls() and friends hasn't led me to improve the situation in those edge cases.

After much experimenting, I'm thinking that we should simply accept the function name as first argument :

.throw_error <- function(fn, ...) {
  stop("[", fn, "()] ", ..., call. = FALSE)
}

This removes all complexity and guarantees that we always return the correct name. It's still better and allows for more consistent results than using stop() directly. However, it means fixing all the calling sites and adding code like this:

## set function name for .throw_error/.throw_warning # <- these lines would appear just
fn <- "get_RLum"                                     # <- once at the start of the function
<...>
.throw_error(fn, "'subset' must contain a logical expression")

We could stick with using fn as shorthand for function name, as it's a short and memorable combination of letters that is not used anywhere in our package (well, apart from one case in fit_OSLLifeTimes(), easily fixed).

[mcol]

There's a problem with this approach: an internal function such as .validate_positive_scalar() (which calls .throw_error()) would have to accept the function name as well, and I think that's really invasive.

I'm now thinking that every time we enter a function, we could store the function name inside the .LuminescenceEnv environment, so that the .throw_* functions will be able to use that value without requiring the extra parameter. Functions like .validate_positive_scalar() will work directly without changes.

So whenever a function is defined, we do:

.set_function_name("name_of_function_being_defined")
<...>
.throw_error("error message")

where the internal functions are:

.set_function_name <- function(name) {
  .LuminescenceEnv$fn <- name
}

.throw_error <- function(...) {
  stop("[", .LuminescenceEnv$fn, "()] ", ..., call. = FALSE)
}

Overall this is a much less invasive change that seems to work in the situations I've encountered so far. I'll keep exploring and testing this to see if this is safe and reliable.

[RLumSK]

Thanks @mcol , perhaps that is the best idea to invoke this and to not further bother. It was just an idea, but apparently it is currently too complicated to figure out the function name otherwise.

[mcol]

The last approach I proposed needed some tweaking for functions that self-call or call other functions, such as in this case:

inner <- function(x) {
  .set_function_name("inner")
}
outer <- function(x) {
  .set_function_name("outer")
  if (x == "before") .throw_error("before")
  inner(x)
  if (x == "after") .throw_error("after")
}
outer("before")
# Error: [outer()] before
outer("after")
# Error: [inner()] after   # <- this is wrong

But anyway, I have a working solution based on setting on.exit() to restore the correct function names.