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

globalenv on search path of a module #151

Closed
amyncarol opened this issue Apr 2, 2020 · 6 comments
Closed

globalenv on search path of a module #151

amyncarol opened this issue Apr 2, 2020 · 6 comments
Assignees

Comments

@amyncarol
Copy link

I tried to use modules to manage my R codebase. It seems that each module will have base namespace on its search path, since global env is parent of base namespace, global env will also be on the modules's search path. This might cause some problem as shown below:

Say for example, I need to run a R script called some.Rscript, and it imports module1.R:
some.Rscript:

m1 = modules::import("module1")
m_pryr = modules::import_package("pryr")

module1.R

m_pryr$where("search")

module1 can actually see m_pryr since m_pryr will be put into the global env. Generally, I would like to avoid such unsafe behaviors. module1.R shouldn't have access to m_pryr, in my opinion.

I am not sure why we need the base namespace as parent instead of the baseenv. If we have baseenv as parent of the module, global env will not be on any module search path. I am not sure if this is related to S3 method? As I found this about base namespace:

The parent of the base namespace is the global environment. This means that if a binding isn’t defined in the imports environment the package will look for it in the usual way. This is usually a bad idea (because it makes code depend on other loaded packages), so R CMD check automatically warns about such code. It is needed primarily for historical reasons, particularly due to how S3 method dispatch works.

If this is the reason we need to use base namespace instead of base env, is there any way to work around this?

@klmr
Copy link
Owner

klmr commented Apr 4, 2020

module1.R shouldn't have access to m_pryr, in my opinion.

That’s entirely correct, and this is in fact the intended behaviour of ‘modules’.

I am not sure why we need the base namespace as parent instead of the baseenv.

We … don’t. That’s a bug.

@klmr
Copy link
Owner

klmr commented Apr 4, 2020

Hmm. Actually, there is a reason why the behaviour is as currently seen; namely, to make ease of migration from plain scripts to modules easier: since many plain R scripts use objects from the default attached packages (‘stats’, ‘grDevices’, ‘methods’ etc.), we need to ensure that these objects continue to be available without explicitly qualifying their package name.

There are two possible ways of ensuring this:

  1. Using .BaseNamespaceEnv as the namespace parent
  2. Using as.environment(2L) as the namespace parent.

The second way would ensure that packages can continue to be used, but skip the global environment for lookup.

klmr added a commit that referenced this issue Apr 4, 2020
@klmr klmr closed this as completed in bca2574 Apr 4, 2020
@klmr
Copy link
Owner

klmr commented Apr 4, 2020

Check out the new release, 0.9.13. It should be fixed now.

Incidentally version 1.0 of modules (if I ever get around to releasing it …), which is a backwards incompatible rewrite, will handle this way stricter: no (non-‘base’) package will be attached inside a module any more, even other default packages. Every object access must be either fully qualified or explicitly use-attached.

@jonnybaik
Copy link

jonnybaik commented Apr 7, 2020

Incidentally version 1.0 of modules (if I ever get around to releasing it …), which is a backwards incompatible rewrite, will handle this way stricter: no (non-‘base’) package will be attached inside a module any more, even other default packages. Every object access must be either fully qualified or explicitly use-attached.

@klmr This might not be the best place to make this comment, but I just wanted to say that I am eagerly awaiting the CRAN release of version 1.0 with the new API. I hope you get around to releasing it :)

@amyncarol
Copy link
Author

Thanks for the quick fix.

Consider the following senario: what if a careless user accidentally use library(somepackage) in some.Rscript, then somepackage will be on the search path of module1.R.

Not sure if it is appropriate to instead use as.environment("package:stats") as parent? It seems a little hardcoded, and I am not sure if these defaults will change between versions of R.

@klmr
Copy link
Owner

klmr commented Apr 8, 2020

what if a careless user accidentally use library(somepackage) in some.Rscript, then somepackage will be on the search path of module1.R

Yes. This is unfortunate, and why future versions of modules will behave differently. But in the current version of modules, this is by design because many existing modules (which were created from existing R scripts) rely on this behaviour. It can’t be changed without breaking backwards compatibility in a big way.

klmr added a commit that referenced this issue Apr 11, 2020
radbasa pushed a commit to Appsilon/box that referenced this issue Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants