From luke-tierney at uiowa.edu  Thu Jan 13 23:14:18 2011
From: luke-tierney at uiowa.edu (luke-tierney at uiowa.edu)
Date: Thu, 13 Jan 2011 16:14:18 -0600
Subject: [Rd] as.environment.list provides inconsistent results under
 torture
In-Reply-To: <alpine.DEB.2.00.1101120638320.1812@luke-inspiron>
References: <4D2CB092.6000303@r-enthusiasts.com>
	<792C6196-A236-44AC-B734-E38A09791CB4@r-project.org>
	<alpine.DEB.2.00.1101111751380.1812@luke-inspiron>
	<21D8CF42-9C05-4EC0-A6CD-0C697C81D69A@r-project.org>
	<alpine.DEB.2.00.1101120638320.1812@luke-inspiron>
Message-ID: <alpine.DEB.2.00.1101131613530.1812@luke-inspiron>

The fixes are committed as 53967.

luke

On Wed, 12 Jan 2011, luke-tierney at uiowa.edu wrote:

> On Tue, 11 Jan 2011, Simon Urbanek wrote:
>
>> 
>> On Jan 11, 2011, at 6:55 PM, <luke-tierney at uiowa.edu> 
>> <luke-tierney at uiowa.edu> wrote:
>> 
>>> No. Lots of internal functions expect their callers to protect their 
>>> arguments, for efficiency reasons. eval is called very often and almost 
>>> always with argument that are protected because they are in the evaluation 
>>> engine, so it would be wasteful and potentially very costly if eval 
>>> protected its arguments every time it is called. (I don't tknow what the 
>>> cost would be to do so in the current implementation but it could be 
>>> prohibitive if we moved to some different approaches, so for now we whould 
>>> continue to expect callers of eval to make sure the argumetns are 
>>> protected.)
>>> 
>> 
>> 
>> Fair enough. It would be nice if this was explicitly documented since 
>> eval() is part of the API and I see several packages on CRAN using 
>> eval(LCONS(..),..) and eval(listX(...),...) - and I don't blame them 
>> (partly because one of them is mine ;)).
>
> There are lots of functions in the internals, API or not, that do not
> protect their arguments. I would argue that the right way to think
> about this is: unless a function is explicitly documented to protect
> its arguments you have to assume it either does not now or might not
> in the future.
>
> In any case, with functions of more than one SEXP argument, like eval,
> protecting the arguments might give inexperienced programmers a false
> sense of security: if f, g, and h all allocate and f protects its
> arguments they might then be tempted to write
>
>     f(g(), h())
>
> But that would be wrong since one of the values of g() or h() would be
> unprotected while the other one is computed and before f() has a
> chance to do its protecting (which one is unprotected depends on the
> argument evaluation order the C compiler chooses for the expression).
>
> One of the few lower level functions that protects its arguments is
> CONS (and some of its cousins like LCONS) both because it is cheap to
> do so because of the integration with the GC and because it makes
> certain kinds of code a bit more readable. But again if you want to
> take advantage of this only one of the arguments can allocate. For
> example, in the first case below there is
>
>    LCONS(install("library"),CONS(install("grDevices"),R_NilValue))
>
> Both LCONS and CONS protect their arguments, but one of the two
> arguments to LCONS is unprotected while the other is being
> computed. (Of course the install calls only allocate if the symbols
> are not in the symbol table and even then those allocations are
> protected by being put in the symbol table, and as library will be
> there once base is loaded this is safe for all practical purposes, but
> it would still be better to write it as
>
>    lang2(install("library"), install("grDevices"))
>
> or
>
>     SEXP librarySymbol = install("library");
>     SEXP grDevicesSymbol = install("grDevices");
>     ...
>     lang2(librarySymbol, grDevicesSymbol)
>
> or something like that.)
>
> I suppose we could have more warnings about this sort of thing in the
> extensions manual.
>
> I did do a quick scan of R-devel for this issue with eval and found these:
>
>    ./unix/aqua.c: 
> eval(LCONS(install("library"),CONS(install("grDevices"),R_NilValue)),R_GlobalEnv);
>    ./unix/sys-std.c:    infile = PROTECT(eval(lang1(RComp_getFileCompSym), 
> rcompgen_rho));
>    ./modules/X11/dataentry.c:       newval <- eval(parse(text=newval))
>    ./main/envir.c: 
> return(eval(lang4(install("list2env"), arg,
>    ./gnuwin32/dataentry.c:       newval <- eval(parse(text=newval))
>
> I'll fix them in the next couple of days if no one else gets there
> first (but I'm not set up to test the aqua or gnuwin32 ones).
>
> Best,
>
> luke
>
>> Unfortunately all the examples in R-ext use implicitly protected arguments 
>> (as function arguments or parts of larger already protected constructs) so 
>> it's not obvious from that, either.
>> 
>> Thanks,
>> Simon
>> 
>> 
>> 
>> 
>>> 
>>> 
>>> On Tue, 11 Jan 2011, Simon Urbanek wrote:
>>> 
>>>> Interesting, I'd argue that the bug is in eval() not protecting its 
>>>> arguments since the usual convention is for functions to protect its 
>>>> arguments...
>>>> 
>>>> On Jan 11, 2011, at 2:33 PM, Romain Francois wrote:
>>>> 
>>>>> Hello,
>>>>> 
>>>>> Using R-devel (rev 53950), I get inconsistent results with 
>>>>> as.environment( VECSXP ) when gctorture is on.
>>>>> 
>>>>> Consider:
>>>>> 
>>>>> a <- list( aa = rnorm, bb = runif )
>>>>> gctorture(TRUE)
>>>>> as.environment( a )
>>>>> 
>>>>> The last line sometimes produces the correct environment, but sometimes 
>>>>> I get errors. Here are the three situations:
>>>>> 
>>>>> # good
>>>>>> as.environment( a )
>>>>> <environment: 0x100b1c978>
>>>>> 
>>>>> # not good
>>>>>> as.environment( a )
>>>>> Erreur dans length(x) : 'x' est manquant
>>>>> 
>>>>> # not good either
>>>>>> as.environment( a )
>>>>> Erreur dans list(NULL, list(aa = function (n, mean = 0, sd = 1)  :
>>>>> correspondance partielle de cha?nes de caract?res incorrecte
>>>>> 
>>>>> 
>>>>> Is it because the call made by lang4 is not protected while evaluated in 
>>>>> this line :
>>>>>
>>>>>   case VECSXP: {
>>>>> 	/* implement as.environment.list() {isObject(.) is false for a list} 
>>>>> */
>>>>> 	return(eval(lang4(install("list2env"), arg,
>>>>> 			  /*envir = */R_NilValue, /* parent = */R_EmptyEnv),
>>>>> 		    rho));
>>>>>   }
>>>>> 
>>>>> 
>>>>> (BTW, this was detected in a looooooooong Rcpp-devel thread. See 
>>>>> http://comments.gmane.org/gmane.comp.lang.r.rcpp/1336)
>>>>> 
>>>>> Romain
>>>>> 
>>>>> --
>>>>> Romain Francois
>>>>> Professional R Enthusiast
>>>>> +33(0) 6 28 91 30 30
>>>>> http://romainfrancois.blog.free.fr
>>>>> |- http://bit.ly/fT2rZM : highlight 0.2-5
>>>>> |- http://bit.ly/gpCSpH : Evolution of Rcpp code size
>>>>> `- http://bit.ly/hovakS : RcppGSL initial release
>>>>> 
>>>>> ______________________________________________
>>>>> R-devel at r-project.org mailing list
>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>> 
>>>>> 
>>>> 
>>>> ______________________________________________
>>>> R-devel at r-project.org mailing list
>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>> 
>>> 
>>> --
>>> Luke Tierney
>>> Statistics and Actuarial Science
>>> 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 at stat.uiowa.edu
>>> Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu
>> 
>> 
>
>

-- 
Luke Tierney
Statistics and Actuarial Science
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 at stat.uiowa.edu
Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu


[Rd] as.environment.list provides inconsistent results under torture
luke-tierney at uiowa.edu