Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

findGlobals(~NULL) errors #59

Closed
DavisVaughan opened this issue Aug 11, 2020 · 6 comments
Closed

findGlobals(~NULL) errors #59

DavisVaughan opened this issue Aug 11, 2020 · 6 comments
Labels
Milestone

Comments

@DavisVaughan
Copy link
Contributor

Simplest possible reprex:

library(globals)

globals::findGlobals(~NULL)
#> Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") {: missing value where TRUE/FALSE needed

More complex reprex with some details. This came up when running revdeps for furrr. The baguette package failed because it has quosure objects (which are formulas) that get searched by getGlobalsAndPackages() and one of the quosures had NULL in it.

library(globals)
library(rlang)

x <- quo(NULL)

globals::findGlobals(x)
#> Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") {: missing value where TRUE/FALSE needed

# Oh...
x[-1]
#> Warning: Subsetting quosures with `[` is deprecated as of rlang 0.4.0
#> Please use `quo_get_expr()` instead.
#> This warning is displayed once per session.
#> Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") {: missing value where TRUE/FALSE needed

# The problem:
# rlang:::`[.quosure` hands of to NextMethod
# which gets us into stats:::`[.formula`
# which fails in the second half of the `if()` statement there
stats:::`[.formula`
#> function (x, i) 
#> {
#>     ans <- NextMethod("[")
#>     if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") {
#>         class(ans) <- "formula"
#>         environment(ans) <- environment(x)
#>     }
#>     ans
#> }
#> <bytecode: 0x7f9a455f8a28>
#> <environment: namespace:stats>

Using [ and [[ on quosures are both soft deprecated in rlang, so perhaps the formula could be unclassed at some point? Not sure. Unfortunately I think this will block a furrr release as well since there isn't anything that I'd change in baguette to fix it. :/

@DavisVaughan
Copy link
Contributor Author

CC @lionel-. Not sure if you knew that quo(NULL)[-1] would error like this (even though that behavior is soft deprecated)

@lionel-
Copy link

lionel- commented Aug 12, 2020

@lionel-. Not sure if you knew that quo(NULL)[-1] would error like this (even though that behavior is soft deprecated)

It looks like it's coming from the formula method to which [.quosure delegates.

@HenrikBengtsson
Copy link
Owner

For the purpose of fixing this in 'globals', this bug affects also:

globals::findGlobals(~NULL)
globals::findGlobals(NULL~rhs)
globals::findGlobals(NULL~NULL)

but not

globals::findGlobals(lhs~NULL)

@HenrikBengtsson
Copy link
Owner

HenrikBengtsson commented Aug 14, 2020

This happens because internally we end up doing:

> e <- ~ NULL
> str(e)
Class 'formula'  language ~NULL
  ..- attr(*, ".Environment")=<environment: R_GlobalEnv> 
> length(e)
[1] 2
> e[-1]
Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") { : 
  missing value where TRUE/FALSE needed
> traceback()
2: `[.formula`(e, -1)
1: e[-1]

where

> stats:::`[.formula`
function (x, i) 
{
    ans <- NextMethod("[")
    if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") {
        class(ans) <- "formula"
        environment(ans) <- environment(x)
    }
    ans
}
<bytecode: 0x55a10bdca238>
<environment: namespace:stats>

where ans becomes:

Browse[2]> str(ans)
 language NULL()
Browse[2]> length(ans)
[1] 1
Browse[2]> ans[[1L]]
NULL
Browse[2]> as.character(ans[[1L]])
character(0)
Browse[2]> as.character(ans[[1L]])[1L]
[1] NA
Browse[2]> as.character(ans[[1L]])[1L] == "~"
[1] NA
Browse[2]> length(ans) == 0L || as.character(ans[[1L]])[1L] == "~"
[1] NA

Hmm... I wonder if this is a bug in stats:::[.formula. This bug is not only when we try to drop the first element but also when we attempt to access any NULL element in a formula using [();

> e <- ~ NULL
> length(e)
[1] 2
> str(as.list(e))
List of 2
 $ : symbol ~
 $ : NULL
 - attr(*, "class")= chr "formula"
 - attr(*, ".Environment")=<environment: R_GlobalEnv>
> e[2]
Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") { : 
  missing value where TRUE/FALSE needed

> e <- NULL ~ NULL
> length(e)
[1] 3
List of 3
 $ : symbol ~
 $ : NULL
 $ : NULL
 - attr(*, "class")= chr "formula"
 - attr(*, ".Environment")=<environment: R_GlobalEnv>
> e[2]
Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") { : 
  missing value where TRUE/FALSE needed
> e[3]
Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") { : 
  missing value where TRUE/FALSE needed

> e <- NULL ~ rhs
> length(e)
[1] 3
> str(as.list(e))
List of 3
 $ : symbol ~
 $ : NULL
 $ : symbol rhs
 - attr(*, "class")= chr "formula"
 - attr(*, ".Environment")=<environment: R_GlobalEnv>
> e[1]
`~`()
> e[2]
Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") { : 
  missing value where TRUE/FALSE needed
> e[3]
rhs()

I might be able to work around it in findGlobals() - need to think about it.

HenrikBengtsson added a commit that referenced this issue Aug 14, 2020
@HenrikBengtsson
Copy link
Owner

I rewrote the code so it avoids using the problematic e[-1] on formula objects to instead uses e[[kk]] on formula objects;

         stop_if_not(identical(e[[1]], as.symbol("~")))
-        expr <- e[-1]
-        for (kk in seq_along(expr)) {
-          globals <- find_globals_ordered(expr = expr[[kk]], envir = w$env)
+        stop_if_not(length(e) >= 2L, identical(e[[1]], as.symbol("~")))
+        for (kk in 2:length(e)) {
+          globals <- find_globals_ordered(expr = e[[kk]], envir = w$env)

@HenrikBengtsson
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants