`get` being overridden by other packages

[awong234]

Problem

library(config)
#> 
#> Attaching package: 'config'
#> The following objects are masked from 'package:base':
#> 
#>     get, merge
import::from('R/make_import_call.R', make_import_call)
#> Error: unused arguments (envir = <environment>, inherits = FALSE)

^{Created on 2022-03-30 by the reprex package (v2.0.1)}

Any package that loads a function called get is used instead of base::get (I understand that config is meant to be used much like import, without calling library, but this is an example)

Expected output

Expected no error on load

Proposed Solution

In make_import_call, namespace get as base::get.

#' Make an import call object.
#'
#' The import call constructed by this function can be evaluated with
#' \code{eval} to perform the actual import.
#'
#' @param params list of parameters to substitute in the call.
#' @param exports_only logical indicating whether only exported objects are
#'   allowed.
#'
#' @return A call object.
#'
#' @noRd
make_import_call <- function(params, exports_only)
{
  cl <-
    if (exports_only)
       substitute(safe_assign(new, getExportedValue(nm, ns = ns), pos = pos),
                  params)
    else
       substitute(safe_assign(new, base::get(nm, envir = ns, inherits = inh), pos = pos),
                  params)

  cl[[1]] <- safe_assign

  cl
}

[torfason]

Reproduced locally. Masking base functions causes the masking function to be called (inside package functions) instead of the masked base function.

But this would be an issue not just with get, but with all base functions used in the package (just in this one function, it seems like the substitute() function would need to be prefixed as base::substitute() to prevent similar issues.

And, I would guess, in all packages? Just checked a few dplyr functions and they use base functions without prefix willy nilly. Same with digest. Is everyone doing it wrong or is there something different about import ???

[torfason]

Update: This is indeed a special case, because the call being created in this function ends up getting evaluated in parent.frame() which is probably the reason this happens here but not in general.

I agree with the proposed solution, prefixing get() in this function.

We should probably also prefix getExportedValue() in this function, but don't need to hunt down every call to base functions.

Thanks for the clear report!

[awong234]

Got it! Great -- while I was tracing make_import_call, I had suspected it was because it was being evaluated in the global env for this particular reprex but I couldn't offer an alternative way to evaluate it that resolved the issue. Glad to hear that this issue makes sense.

[torfason]

Checked this out, and it turns out that one doesn't even need another package. Overloading get with a user-defined function in .GlobalEnv is enough to break this. For example, defining

get <- function(...) {
  stop("You have been masked!")
}

Before an import seems to trigger this reliably.

@awong234, would you care to prepare a PR with some tests as well (and prefixing getExportedValue() is probably also needed although I have not checked to see how that gets triggered)?

If you find time for that, it's probably best to base the PR on the dev branch of torfason/import (the dev branch is the default), so that the GitHub check action fix gets incorporated and the checks pass.

[torfason]

Now fixed in rticulate/import@dev, thanks to @awong234!

[torfason]

All features/fixes targeted for v1.3.0 are now in dev and master has been forwarded to match dev, and the version bumped to correspond to that. All checks required for a CRAN release see to be passing, and the plan is to submit to CRAN after the weekend, unless any unknown issues are uncovered until then. In the meantime, it would be much appreciated if the people involved in the new features/fixes could check out this could install and try out the most recent version from GitHub, for example using:

pak::pak("rticulate/import")

(remotes::install_github("rticulate/import") or devtools::install_github("rticulate/import") should give the same result)

Thanks to everyone for their contributions!

[torfason]

Fixed in v1.3.0