Devtools: check_dots_used() gives false errors in install_deps()

[kenahoo]

If I call devtools::install_deps() and pass some INSTALL_opts arguments (which would be used by utils::install.packages()), but there's no installing to do, then I get an error:

> devtools::install_deps('.', upgrade=FALSE, INSTALL_opts=c('--no-docs', '--no-help'))
Error: 1 components of `...` were not used.

We detected these problematic arguments:
* `INSTALL_opts`

Did you misspecify an argument?
Call `rlang::last_error()` to see a backtrace

IMO that shouldn't throw an error.

I don't see how you'd solve this problem without changing the approach used by check_dots_used(), though - maybe it also needs to check whether ... itself was ever evaluated?

[kenahoo]

A workaround (or permanent fix) is to use remotes::install_deps() instead of devtools::install_deps(), but of course there's a lot of code out there calling the latter.

[jimhester]

I don't really see a solution either, this will also happen with the install_*() functions when you are using something in ... and the packages are already installed.

[kenahoo]

I'd suggest removing the check_dots_used() call if it can't be made to work reliably - in an automated testing situation, it will throw errors when everything's just fine. That's how I came across this, my builds started failing.

[jennybc]

Coming at this as an idealist, the conclusion that remotes::install_deps() is a superior choice for automated testing and programmatic usage, over devtools::install_deps(), is basically the party line. I realize pragmatism might ultimately point to a different solution.

But I feel like devtools wrappers are generally supposed to be the safer, more interactively-focused version of functions like remotes::install_deps().

[kenahoo]

I guess I feel like this is simply a regression, and a bug - devtools::install_deps('.', upgrade=FALSE, INSTALL_opts=blah) is a perfectly valid invocation, and it used to work just fine, but now it dies. Is that essentially what you're saying too @jennybc ? Or just thinking things through?

[jennybc]

I'm saying that, in an ideal world, we (in the vague R sense) wouldn't have embraced a convention (...) that is capable of silently swallowing mis-spelled argument names: they are neither used nor do they emit a message, warning, or error. But this is the situation with utils::install.packages(). So any attempt to wrap it and check for this phenomenon with ... is going to require some pragmatic compromise.

So, yes, your call devtools::install_deps('.', upgrade=FALSE, INSTALL_opts=blah) is perfectly valid. How aboutdevtools::install_deps('.', upgrade=FALSE, INSTAL_opts=blah)? Can you spot the typo 🧐? The phenomenon you're seeing is a consequence of trying to detect and message about the latter.

[jimhester]

So the planned workaround is to add a function equivalent to check_dots_used() to ellipsis that issues a warning rather than an error.

We can then use this in devtools. This will still alert the user that something might be fishy if they have misspelled arguments, but it won't fail for cases where we return early like this one.

[kenahoo]

I'm not sure that's the right workaround. The caller of check_dots_used() can't know whether to call the warning-version or the dying-version because they won't know what's happening downstream of their function, and how severe it is when there's an (apparent, but sometimes false) problem. Even if you read the source of everything downstream (which is what would be necessary, because they might be using ... implicitly, or doing something like args <- list(...)), the downstream functions should be free to change their internals later.

Is it not possible to modify check_dots_used() to check whether ... itself was never evaluated? That would be a step in the right direction, I think it would solve this particular case, but it would still have a fair number of false positives of course.

And even if this workaround is implemented, people won't want warnings issued when they're not warranted. I coach my team to not leave warnings unfixed, or else we stop paying attention to all the noise - stuff like this makes that harder for us to do.

[ashiklom]

I hit this problem in our project as well, and I agree with @kenahoo that false "Warnings" are not a great workaround.

Could you capture the ..., use formals to grab the named arguments of the downstream functions (utils::update.packages, utils::install.packages, and utils::download.file) and force evaluation of the ones that are actually real? AFAICT, utils::download.file is the last function in the installation chain and its ... arguments are not actually passed on to anything, so I think it's safe to grab only its named arguments.

This would allow you to preserve the typo-checking behavior @jennybc was describing while also avoiding false positives.

[jimhester]

You can now control the behavior of ellipsis in devtools by setting the option devtools.ellipsis_action to a function to be used by ellipsis, one of rlang::abort(), rlang::warn(), rlang::message() or rlang::signal().

If you want devtools to produce no output when you specify arguments that are not used you can set options(devtools.ellipsis_action = rlang::signal).

[kenahoo]

Okay, that will mask the issue (unless someone else upstream does the same thing you did and turns warnings into errors...) and get builds succeeding again. I'd still be happier if you just removed check_dots_used completely, though - it seems to cause more harm & workarounds than actual good in this case.

IMO the right place to check for unused arguments is at the leaves of the call tree. I'd say this is a (small) bug in install.packages and download.file, not something for devtools to try to compensate for.

Anyway, since I can go around and change all our code to use remotes::install_deps instead (if you're not going to make the same change there...) and ask our team to do that in their code too, I can work around it. Still not clear on why this is done for devtools::install_deps and not remotes::install_deps, though - or are you planning to add it to remotes too?

[jimhester]

remotes has no hard dependencies and cannot have dependencies, we can't do it in remotes.

[lock[bot]]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/