Handle functions referenced by name as a string when using do.call()

[bburns632]

(I'm reposting a great question I received via email)

Does the pkgnet pick up function calls that use do.call()? It seems it doesn't. For example, within a function in a package I am working on, I have this call

nuis <- do.call(".estimate_nuisance",
args = c(data = list(data),
.key.inputs,
.nuis.inputs))

In this case pkgnet doesn't pick up the dependence on the function .estimate_nuisance(). In this particular package, this happens to be one of the key dependences, and we need to use do.call in order to tailor a bunch of options based on context, which we put in .nuis.inputs.

Is there an argument I can provide to CreatePackageReport() that would allow picking up dependencies through do.call()? If not, would you consider adding this?

[jameslamb]

I think this could be a useful feature, but I personally am not committing to working on it.

For anyone reading this post... I'd be happy to review a pull request proposing such a change. @bburns632 could you encourage whoever emailed you directly to come talk to us here, if they're interested in contributing that feature?

[bburns632]

Of course. This was posted as a general question / call to action for the pkgnet community. To that end, if anyone is interested in developing this feature, please comment on this ticket then start a PR.

…

On Sat, Apr 29, 2023, 10:48 PM James Lamb ***@***.***> wrote: I think this could be a useful feature, but I personally am not committing to working on it. For anyone reading this post... I'd be happy to review a pull request proposing such a change. @bburns632 <https://github.com/bburns632> could you encourage whoever emailed you directly to come talk to us here, if they're interested in contributing that feature? — Reply to this email directly, view it on GitHub <#302 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF3FDS4YSEECBTG4HN45DJDXDXOHNANCNFSM6AAAAAAW6PIOUY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bburns632]

OK. Looking into this feature. Would like to bounce this idea off you @jayqi as I recall much of the current function network code originally written by you.

Why do this?
do.call is a fairly widely used function. Here's a quick search on github: https://github.com/search?q=do.call&type=code. Accommodating this will produce more correct function graphs for packages that rely on it (like the original requester's).

Why doesn't it work now?
do.call is a base function. Presently, our scope for nodes is limited to the target package namespace (e.g. names(pkgnet)). Base functions that are used within functions are overlooked.

Plan of attack
After brushing up on how we extract the function network (which ultimately a string search sorta thing), I think adding some code after this matching section to look for do.call specifically and do some special node additions to the edges & node list is the best place. Keep it similarly regex based with base functions.

https://github.com/uptake/pkgnet/blob/main/R/FunctionReporter.R#L361-L374

Design Decisions:

1. How to reflect this in the graph?
   Do we include a do.call node that links to its argument or just link the function directly to the argument? For do.call, I could see either.

2. Do we want to check for other base functions as well?
   We would need to limit expansion to those base functions directly referenced. We would still need special accommodation for do.call. This opens us up to a whole bunch of edgecases as well.

3. Nested do.call: where there's a function in the list of arguments. Accommodate or not?
   Do not look recursively seems like a half measure, but that could get ugly. This is similar to Missing dependency link, if function call happens within the arguments of a function #304.

[jayqi]

With the caveat that I remember only like 5% of what we're doing here, how it works, why it works a certain way:

• I don't think considering the target to be through do.call is the correct semantic representation here. The target function is directly referenced in the body of the source function, but it's just an argument of do.call rather than being called directly. Having the target be linked through do.call would be as if the body of do.call itself contained the target function.
  • As such, I don't think this is a case of "we exclude base functions but we should include them". I think this is a case of "do.call is a unique way of invoking a function that we currently miss".
  • Additionally IMO, do.call itself is not that interesting and is just machinery for calling the target function being used. I don't think adding a node for do.call would add any value to the graph.
• Is this actually a case of whether or not the function is specified as a string reference to a name? i.e., is there a difference between do.call(my_func, args = ...) vs. do.call("my_func", args = ...)? I'm wondering if the former currently works fine, and it's just the latter that doesn't work. If so, this is more like a bug/missed edge case, and not really a change in functionality. Then we basically just need to special case the latter, because IIRC we currently discard atomic strings, but do.call is special because it will look up the string reference.

[bburns632]

@jayqi, on your second point, here are some quick edits on baseballstats. Looks like we currently support the case with a function argument to do.call. It's the function text name case that is not supported.

Normal (main branch):

With text as a param:

With function as a param:

Between each, I am quitting R without memory and recreating with this code chunk:

library(devtools)
remove.packages("pkgnet")
devtools::install_local(".", force = TRUE, upgrade = "never")
devtools::install_local("inst/baseballstats", force = TRUE,upgrade = "never")
library(pkgnet)
t <- CreatePackageReport("baseballstats")

[bburns632]

Working on the edge case handling now