moodymudskipper/typed

Assertions failing on libs using devtools

Closed this issue · 5 comments

latot commented

Hi, I was trying to use typed from libs, until now, the basic types seems to work, just there is some very weird things.

With a simple package, where after load with devtools, the assertions can't be executed, even including library(typed) inside or outside the library, we will be unable to use it.

bug_typed.zip

> devtools::load_all("bug_typed")
> typed::Data.frame() ? a
Error in type == "package" : 
  comparison (==) is possible only for atomic and list types
> traceback()
4: (function (e1, e2) 
   {
       if (missing(e2)) {
           type <- NULL
           topicExpr <- substitute(e1)
       }
       else {
           type <- substitute(e1)
           topicExpr <- substitute(e2)
       }
       search <- (is.call(topicExpr) && topicExpr[[1L]] == "?")
       if (search) {
           topicExpr <- topicExpr[[2L]]
           if (is.call(te <- topicExpr) && te[[1L]] == "?" && is.call(te <- topicExpr[[2L]]) && 
               te[[1L]] == "?") {
               cat("Contacting Delphi...")
               flush.console()
               Sys.sleep(2 + stats::rpois(1, 2))
               cat("the oracle is unavailable.\nWe apologize for any inconvenience.\n")
               return(invisible())
           }
       }
       if (is.call(topicExpr) && (topicExpr[[1L]] == "::" || topicExpr[[1L]] == 
           ":::")) {
           package <- as.character(topicExpr[[2L]])
           topicExpr <- topicExpr[[3L]]
       }
       else package <- NULL
       if (search) {
           if (is.null(type)) 
               return(eval(substitute(help.search(TOPIC, package = PACKAGE), 
                   list(TOPIC = as.character(topicExpr), PACKAGE = package))))
           else return(eval(substitute(help.search(TOPIC, fields = FIELD, 
               package = PACKAGE), list(TOPIC = as.character(topicExpr), 
               FIELD = as.character(type), PACKAGE = package))))
       }
       else {
           if (is.null(type)) {
               if (is.call(topicExpr)) 
                   return(.helpForCall(topicExpr, parent.frame()))
               topic <- if (is.name(topicExpr)) 
                   as.character(topicExpr)
               else e1
               return(eval(substitute(help(TOPIC, package = PACKAGE), 
                   list(TOPIC = topic, PACKAGE = package))))
           }
           else {
               type <- if (is.name(type)) 
                   as.character(type)
               else e1
               topic <- if (is.name(topicExpr)) 
                   as.character(topicExpr)
               else {
                   if (is.call(topicExpr) && identical(type, "method")) 
                     return(.helpForCall(topicExpr, parent.frame(), 
                       FALSE))
                   e2
               }
               if (type == "package") 
                   package <- topic
               h <- .tryHelp(topicName(type, topic), package = package)
               if (is.null(h)) {
                   if (is.language(topicExpr)) 
                     topicExpr <- deparse(topicExpr)
                   stop(gettextf("no documentation of type %s and topic %s (or error in processing help)", 
                     sQuote(type), sQuote(topicExpr)), domain = NA)
               }
               h
           }
       }
   })(typed::Data.frame(), a)
3: eval(as.call(list(utils::`?`, substitute(e1), substitute(e2))))
2: eval(as.call(list(utils::`?`, substitute(e1), substitute(e2))))
1: `?`(typed::Data.frame(), a)

This bug is pretty weird, the main reason, is because if we use it inside a library will works, is only after load the library with devtools.

Maybe is a devtools bug.

Thx!

Have you reexported ?, if you want to have it available without attaching {typed} you'll need to reexport it, just importing as you showed in the other issue makes it available to the functions of your package only.

To reexport you need :

#' @export
typed::`?`

Then document(), install and try again.

The reason why it worked with load_all() is that by default it exports everything (doesn't look at the NAMESPACE to choose what to export or not), and it includes imports such as ?.

Alternately, use declare(), which works like ? but without the binary op syntax:

typed::declare("a", typed::Data.frame())
latot commented

Hi, yes, ? is exported.

The code works fine inside the lib, only fails when is called the env from devtools.

Oh yes I remember now I had this issue. devtools places a shim of ? in the "devtools_shims" environment, this is so we can have development help, so that's not a devtools bug.

as.environment("devtools_shims")$`?`
#> function (e1, e2) 
#> {
#>     e1_expr <- substitute(e1)
#>     if (is.name(e1_expr)) {
#>         topic <- as.character(e1_expr)
#>         pkg <- NULL
#>     }
#>     else if (is.call(e1_expr)) {
#>         if (identical(e1_expr[[1]], quote(`?`))) {
#>             topic <- NULL
#>             pkg <- NULL
#>         }
#>         else if (identical(e1_expr[[1]], quote(`::`))) {
#>             topic <- as.character(e1_expr[[3]])
#>             pkg <- as.character(e1_expr[[2]])
#>         }
#>         else {
#>             topic <- deparse(e1_expr[[1]])
#>             pkg <- NULL
#>         }
#>     }
#>     else if (is.character(e1_expr)) {
#>         topic <- e1
#>         pkg <- NULL
#>     }
#>     else {
#>         cli::cli_abort("Unknown input.")
#>     }
#>     if (!is.null(topic) && !is.null(dev_topic_find(topic, pkg))) {
#>         dev_help(topic, pkg)
#>     }
#>     else {
#>         eval(as.call(list(utils::`?`, substitute(e1), substitute(e2))))
#>     }
#> }
#> <bytecode: 0x1175286a8>
#> <environment: namespace:pkgload>

I think if you'll need to do this again for now, I don't see another way:

`?` <- typed::`?`

Maybe I can work around it by creating a hook when this is attached, we'll need to modify our own ? function so it falls back on the pkgload version instead of the base version when not used in as in {typed}. Let's have a good explicit message too about what we'll be doing since it's a bit "rude" to hijack someone else's function.

latot commented

Hi!, mmm, if is a change on devtools....

mmmmm!!!!

Maybe the best would be recommend to use ? on non-lib things, and use declare when is inside a lib.

But there should do some tests, because I don't know how works functions and function's params behavior with all this.

No idea the equivalent with declare, or use other symbol, is possible to have more than one symbol, or one symbol out of a lib, and one to inside.

There is two points here, one is ideally a dev section, add this details.
Second, how devtools override ? it also breaks the functions, which is hard to know why and how happens, well except if you ask here.

Thx!

Follow up in #49