[Rd] [External] Time to drop globalenv() from searches in package code?

[Kurt Hornik]

>>>>> luke-tierney  writes:

> On Thu, 15 Sep 2022, Duncan Murdoch wrote:
>> The author of this Stackoverflow question 
>> https://stackoverflow.com/q/73722496/2554330 got confused because a typo in 
>> his code didn't trigger an error in normal circumstances, but it did when he 
>> ran his code in pkgdown.
>> 
>> The typo was to use "x" in a test, when the local variable was named ".x". 
>> There was no "x" defined locally or in the package or its imports, so the 
>> search got all the way to the global environment and found one.  (The very 
>> confusing part for this user was that it found the right variable.)
>> 
>> This author had suppressed the "R CMD check" check for use of global 
>> variables.  Obviously he shouldn't have done that, but he's working with 
>> tidyverse NSE, and that causes so many false positives that it is somewhat 
>> understandable he would suppress one too many.
>> 
>> The pkgdown simulation of code in examples doesn't do perfect mimicry of 
>> running it at top level; the fake global environment never makes it onto the 
>> search list.  Some might call this a bug, but I'd call it the right search 
>> strategy.
>> 
>> My suggestion is that the search for variables in package code should never 
>> get to globalenv().  The chain of environments should stop after handling the 
>> imports.  (Probably base package functions should also be implicitly 
>> imported, but nothing else.)
>> 

> This was considered and discussed when I added namespaces. Basically
> it would mean making the parent of the base namespace environment be
> the empty environment instead of the global environment. As a design
> this is cleaner, and it would be a one-line change in eval.c.  But
> there were technical reasons this was not a viable option at the time,
> also a few political reasons. The technical reasons mostly had to do
> with S3 dispatch.

> Changes over the years, mostly from work Kurt has done, to S3 dispatch
> for methods defined and registered in packages might make this more
> viable in principle, but there would still be a lot of existing code
> that would stop working. For example, 'make check' with the one-line
> change fails in a base example that defines an S3 method. It might be
> possible to fiddle with the dispatch to keep most of that code
> working, but I suspect that would be a lot of work. Seeing what it
> would take to get 'make check' to succeed would be a first step if
> anyone wants to take a crack at it.

Luke,

Can you please share the one-line change so that I can take a closer
look?

Best
-k

>> I suspect this change would reveal errors in lots of packages, but the number 
>> of legitimate uses of the current search strategy has got to be pretty small 
>> nowadays, since we've been getting warnings for years about implicit imports 
>> from other standard packages.

> Your definition of 'legitimate' is probably quite similar to mine, but
> there is likely to be a small but vocal minority with very different
> views :-).

> Best,

> luke

>> Duncan Murdoch
>> 
>> ______________________________________________
>> R-devel using r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>> 

> -- 
> Luke Tierney
> Ralph E. Wareham Professor of Mathematical Sciences
> University of Iowa                  Phone:             319-335-3386
> Department of Statistics and        Fax:               319-335-3017
>     Actuarial Science
> 241 Schaeffer Hall                  email:   luke-tierney using uiowa.edu
> Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu

> ______________________________________________
> R-devel using r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel