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

explicit stats::setNames #734

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@robertzk
Contributor

robertzk commented Mar 8, 2015

This has been giving me some trouble when calling install_github from within a package namespace.

@wch

This comment has been minimized.

Member

wch commented Mar 8, 2015

Does your package have a function called setNames? Since stats isn't listed as an import for devtools, I guess it would make sense that this would happen if you have another function named setNames that's on the search path.

@hadley

This comment has been minimized.

Member

hadley commented Mar 8, 2015

@wch but we need to fix this anyway - I think there's a bug in R CMD check where it doesn't pick up missing namespace imports for recommended packages.

@wch

This comment has been minimized.

Member

wch commented Mar 8, 2015

I agree that it should be fixed. I suspect there are also lots of functions from utils that are called without explicit namespacing.

@robertzk

This comment has been minimized.

Contributor

robertzk commented Mar 9, 2015

How do I fix CI? Add "stats" to Imports?

The command "Rscript -e 'options(repos = "http://cran.rstudio.com"); tryCatch({   deps <- devtools::install_deps(dependencies = TRUE) }, error = function(e) {   message(e);   q(status=1) }); if (!all(deps %in% installed.packages())) {  q(status = 1, save = "no") }'" failed and exited with 1 during .
@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Mar 9, 2015

@hadley: nitpicking, but stats is a base package. It seems that you need to declare imports even from base packages, otherwise another package might just override a base function for you.

I am not sure why it is this way, but it seems rather silly. Maybe historical reasons, or I am missing something.

@Geoff99

This comment has been minimized.

Contributor

Geoff99 commented Mar 12, 2015

Hi @gaborcsardi

I'm still getting up to speed on all this, but I think I read in Hadley's Advanced R book that only the base package itself is included in the chain of enclosing environments above a package's own namespace environment, and before the global environment, "as a convenience". All the other recommended packages are on the search path, between the global environment and the empty environment (and hence subject to the vagaries of whatever a user has installed on their search path, and in which order, in a particular session). If I understand properly, that would explain why it would be safer to @import or use the :: approach to calling those functions, pain though it is!

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Mar 12, 2015

@Geoff99

only the base package itself is included in the chain of enclosing environments above a package's own namespace environment,

I don't think this is true. Surely, all base and recommended packages are included there. At least you don't need to explicitly import (say) plot, even if plot is in graphics, not base. It seems that recommended packages are also included (head is in utils, which is not a base package):

library(disposables)
pkg <- make_packages(foo = { h <- function(...) head(...) })
foo::h(1:100)
#> [1] 1 2 3 4 5 6
dispose_packages(pkg)

To make things even worse, it seems attached packages are also searched, even before the recommended and base packages.

Consider the following example, in which package bar calls density from the stats base package. If you have another package (foo) loaded, and foo redefines density, then bar calls foo::density. Which IMO does not make much sense.

p1 <- make_packages(foo = { density <- function(...) print("cocooo!") })
p2 <- make_packages(bar = { f <- function() density(1:10) })
bar::f()
#> [1] "cocooo!"

dispose_packages(p1)
dispose_packages(p2)

Calling density explicitly from stats solves the problem.

p1 <- make_packages(foo = { density <- function(...) print("cocooo!") })
p2 <- make_packages(bar = { f <- function() stats::density(1:10) })
bar::f()

#> Call:
#>  density.default(x = 1:10)
#>
#> Data: 1:10 (10 obs.);    Bandwidth 'bw' = 1.719
#> 
#>        x                 y            
#>  Min.   :-4.1579   Min.   :0.0003034  
#>  1st Qu.: 0.6711   1st Qu.:0.0092711  
#>  Median : 5.5000   Median :0.0538486  
#>  Mean   : 5.5000   Mean   :0.0517052  
#>  3rd Qu.:10.3289   3rd Qu.:0.0936045  
#>  Max.   :15.1579   Max.   :0.0997741  

dispose_packages(p1)
dispose_packages(p2)

If you explicitly import stats (I guess importing just density is enough, too), that also solves the problem:

p1 <- make_packages(foo = { density <- function(...) print("cocooo!") })
p2 <- make_packages(bar = { f <- function() density(1:10) }, imports = "stats")
bar::f()

#> Call:
#>  density.default(x = 1:10)
#> 
#> Data: 1:10 (10 obs.);    Bandwidth 'bw' = 1.719
#> 
#>        x                 y            
#>  Min.   :-4.1579   Min.   :0.0003034  
#>  1st Qu.: 0.6711   1st Qu.:0.0092711  
#>  Median : 5.5000   Median :0.0538486  
#>  Mean   : 5.5000   Mean   :0.0517052  
#>  3rd Qu.:10.3289   3rd Qu.:0.0936045  
#>  Max.   :15.1579   Max.   :0.0997741  

dispose_packages(p1)
dispose_packages(p2)

So, unless I am missing something, the best is to import everything, even stuff from base and certainly from recommended packages. But hopefully I am missing something.

@wch

This comment has been minimized.

Member

wch commented Mar 12, 2015

The parent environment of a package namespace is its imports environment, which contains imported objects listed in the package's NAMESPACE file. The parent of that is the base namespace, and the parent of that is the global environment.

From there, you have the usual search path, of package environments for packages that are attached. You can look at this with pryr::parenvs(). In this case, it'll show all the ancestor environments of the devtools namespace:

> pryr::parenvs(e = asNamespace("devtools"), all = TRUE)
   label                             name               
1  <environment: namespace:devtools> ""                 
2  <environment: 0x106368e38>        "imports:devtools" 
3  <environment: namespace:base>     ""                 
4  <environment: R_GlobalEnv>        ""                 
5  <environment: 0x101e1b908>        "tools:rstudio"    
6  <environment: package:stats>      "package:stats"    
7  <environment: package:graphics>   "package:graphics" 
8  <environment: package:grDevices>  "package:grDevices"
9  <environment: package:utils>      "package:utils"    
10 <environment: package:datasets>   "package:datasets" 
11 <environment: 0x101cea790>        "custom"           
12 <environment: package:methods>    "package:methods"  
13 <environment: 0x1038206a8>        "Autoloads"        
14 <environment: base>               ""                 
15 <environment: R_EmptyEnv>         ""                 

@Geoff99 is right that it would be safer to import objects from stats or use :: -- it would be safest not to rely on the search path (i.e., attached packages) to find functions.

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Mar 12, 2015

@Geoff99 is right that it would be safer to import objects from stats or use :: -- it would be safest not to rely on the search path (i.e., attached packages) to find functions.

Agreed. All I am saying is that you need to do this for base packages as well (except maybe base itself), not only for recommended ones. E.g. stats is a base package.

@Geoff99

This comment has been minimized.

Contributor

Geoff99 commented Mar 13, 2015

Hi @gaborcsardi

I think we are all in violent agreement about what we need to do to be absolutely safe :-)

I willingly confess I am vague about the difference between :

  • the base package (singular),
  • the base packages (plural), and
  • whatever the name is for the set of packages which are automatically attached between R_GlobalEnv and R_EmptyEnv in an interactive session of RStudio (and I suspect in normal R as well, though I use RStudio pretty well exclusively).

so I have resorted to writing myself little recursive programs using parent.env to see what is actually where (now @wch has drawn my attention to pryr::parenvs() my life will be that bit easier - thanks!)

@hadley

This comment has been minimized.

Member

hadley commented Apr 21, 2015

Could you please add a bullet point to NEWS.md?

@hadley

This comment has been minimized.

Member

hadley commented Apr 28, 2015

Bump - let me know if you don't have time, and I can manually rebase & update news.

@hadley

This comment has been minimized.

Member

hadley commented Apr 28, 2015

I can't seem to merge this myself, so I'm just going to redo.

@gaborcsardi any interesting in reporting this problem to R-devel? I think it's a problem that R CMD check doesn't detect missing imports from base packages (except base).

@hadley hadley closed this in 9969462 Apr 28, 2015

@robertzk

This comment has been minimized.

Contributor

robertzk commented Apr 28, 2015

I think I deleted my fork of devtools, and now I can't access it either. Sorry about that.

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Apr 28, 2015

@hadley Sure, I can report it. I am pretty sure that they know about it. I am also pretty sure that they won't fix it, because it would generate NOTEs for many-many packages. It really should be fixed, though, so I'll give it a try later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment