Internal error in 'ls' for pathological environments (PR#14036)

macrakis at alum.mit.edu wrote:

nchar(with(list(2),ls())) gives an internal error. This is of course
a peculiar call (no names in the list), but the error is not caught
cleanly.

It is not clear from the documentation whether with(list(2)...) is
allowable; if it is not, it should presumably give an error. If it is,
then
ls
shouldn't have problems with the resulting environment.

qq <- with(list(2),ls()) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =

names in list)

nchar(qq)

Error in nchar(qq) : 'getEncChar' must be called on a CHARSXP =A0# ls
returned
a bad object

qq

[1]Error: 'getEncChar' must be called on a CHARSXP

qq[1]

[1]Error: 'getEncChar' must be called on a CHARSXP

qq[2]

[1] NA

Apparently related:

with(list(a=3D1,2),ls())

Error in ls() : 'getEncChar' must be called on a CHARSXP

Thanks, yes, this looks wrong.

Also, closer to the root cause:

eval(quote(ls()),list(a=3D1,2))

Error in ls() : 'getEncChar' must be called on a CHARSXP

e <- evalq(environment(),list(2))
ls(e)

[1]Error: 'getEncChar' must be called on a CHARSXP

It is not quite clear that it should be allowed to have unnamed elements =

lists used by eval (which is what with() uses internally). I suppose it
should, since the intended semantics are unaffected in most cases. (I.e.,
there is nothing really wrong with eval(quote(a+b), list(a=3D1,b=3D2,3,4)=

people may have been using such code unwittingly all over.)

However, it IS a bug that we are creating ill-formed environments. The
culprit seems to be that NewEnvironment (memory.c) is getting called in
violation of its assumption that

" This definition allows
=A0the namelist argument to be shorter than the valuelist; in this
=A0case the remaining values must be named already. =A0(This is useful
=A0in cases where the entire valuelist is already named--namelist can
=A0then be R_NilValue.)
"

Removing the assumption from NewEnvironment looks like an efficiency sink=

so I would suggest that we fix do_eval (eval.c) instead, effectively doin=

<- l[names(l) !=3D ""]. =A0So in

=A0 =A0case LISTSXP:
=A0 =A0 =A0 =A0env =3D NewEnvironment(R_NilValue, duplicate(CADR(args)), =

=A0 =A0 =A0 =A0PROTECT(env);
=A0 =A0 =A0 =A0break;

we need to replace the duplicate() call with something that skips the
unnamed elements.

The only side effect I can see from such a change is the resulting
environments get a different length() than before. I'd say that if there =

coder who actually rely on the length of an invalid environment, then the=

deserve what they'd get...