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

Need methods package on the search path #1760

Closed
krlmlr opened this issue Apr 5, 2016 · 12 comments
Closed

Need methods package on the search path #1760

krlmlr opened this issue Apr 5, 2016 · 12 comments

Comments

@krlmlr
Copy link
Member

krlmlr commented Apr 5, 2016

> Rscript -e "dplyr::memdb_frame(a=1)"
Error in UseMethod("db_has_table") : 
  no applicable method for 'db_has_table' applied to an object of class "SQLiteConnection"
Calls: <Anonymous> ... copy_to -> copy_to.src_sql -> isTRUE -> db_has_table
5: db_has_table(dest$con, name)
4: isTRUE(db_has_table(dest$con, name)) at tbl-sql.r#334
3: copy_to.src_sql(src_memdb(), data_frame(...), name = .name) at copy-to.r#14
2: copy_to(src_memdb(), data_frame(...), name = .name) at src-sqlite.r#135
1: dplyr::memdb_frame(a = 1)
> Rscript -e "library(methods); dplyr::memdb_frame(a=1)"
Source:   query [?? x 1]
Database: sqlite 3.11.1 [:memory:]

      a
  <dbl>
1     1

Importing all symbols from methods in the NAMESPACE does not seem to change this behavior.

@hadley
Copy link
Member

hadley commented Apr 6, 2016

This came up with another package I was debugging in person - I could've sworn this used to work. Is it a regression in R?

@krlmlr
Copy link
Member Author

krlmlr commented Apr 6, 2016

I'll bisect R later today.

@krlmlr
Copy link
Member Author

krlmlr commented Apr 6, 2016

Looks like this particular problem is not a regression with R. I tried trunk1000 (mid-August last year), with the same symptoms. Now trying trunk3000 (from 2014), will report here if anything changes.

This will be a lot of fun to track down :-o

I can replicate in a similar setting when implementing an S3 method for class "Matrix"; it is also not dispatched for Matrix::Matrix() (which has class "lsyMatrix"):

#' @export
test <- function(x) UseMethod("test", x)

#' @export
test.Matrix <- function(x) "Hi!"

@kevinushey
Copy link
Contributor

Why not just add methods to the Depends: field?

@krlmlr
Copy link
Member Author

krlmlr commented Apr 6, 2016

Will this fix the example where dplyr is loaded not attached?

@kevinushey
Copy link
Contributor

Ah, you're right :/ It would need to be more aggressive, e.g. call library(methods) in .onLoad().

FWIW, Rscript is weird compared to e.g. R -e in that it explicitly does not attach methods, but I'm guessing you've bumped into this before as well?

@krlmlr
Copy link
Member Author

krlmlr commented Apr 6, 2016

Yes, even asked on SO a couple of years ago ;-)

A workaround would be to up the affected methods to S4 generics. Still, I'm curious why the current code doesn't work, even when all of "methods" is imported into the NAMESPACE.

@krlmlr
Copy link
Member Author

krlmlr commented Apr 6, 2016

After adding this for db_has_table() and importing "methods", I'm getting an error about another method:

#' @export
setGeneric("db_has_table",
           def = function(con, ...) standardGeneric("db_has_table")
)

#' @export
setMethod("db_has_table", signature("DBIConnection"),
          function(con, table) dbExistsTable(con, table))

In the C code for UseMethod(), there are too many uses of R_GlobalEnv for my taste, but I haven't investigated further. (Oops: R_GlobalContext.)

@krlmlr
Copy link
Member Author

krlmlr commented Apr 18, 2016

It works flawlessly if methods::extends is copied into the global environment:

Rscript -e "extends <- methods::extends; dplyr::memdb_frame(a = 1)"

But it also works if extends is assigned an arbitrary value:

Rscript -e "extends <- 42; dplyr::memdb_frame(a = 1)"

To me, it looks like (at least) this code in R is responsible for this behavior. Weird...

@kevinushey
Copy link
Contributor

It sounds like it's worth reporting to R-devel, or filing a bug -- likely that sanity check could just check that methods is within the set of loaded namespaces?

@krlmlr
Copy link
Member Author

krlmlr commented Apr 18, 2016

@krlmlr
Copy link
Member Author

krlmlr commented Apr 19, 2016

Should be fixed upstream eventually.

@krlmlr krlmlr closed this as completed Apr 19, 2016
shntnu added a commit to cytomining/cytotools that referenced this issue Sep 28, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jun 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants