R devel - Oct 2014 - Making parent.env<- an error for package namespaces and package imports

I'd like to propose a change to the R language so that calling
'parent.env<-' on a package namespace or package imports is a runtime
error.

Currently the documentation warns that it's dangerous behaviour and
might go away:
     The replacement function ?parent.env<-? is extremely dangerous as
     it can be used to destructively change environments in ways that
     violate assumptions made by the internal C code.  It may be
     removed in the near future.

This change would both eliminate some potential dangerous behaviours,
and make it significantly easier for runtime compilation systems to
optimize symbol lookups for code in packages.

The following patch against current svn implements this functionality.
It allows calls to 'parent.env<-' only until the namespace is locked,
allowing the namespace to be built correctly while preventing user
code from subsequently messing with it.

I'd also like to make calling parent.env<- on an environment on the
call stack an error, for the same reasons, but it's not so obvious to
me how to implement that efficiently right now.  Could we at least
document that as being 'undefined behaviour'?

Thanks,

Karl


Index: src/main/builtin.c
==================================================================---
src/main/builtin.c (revision 66783)
+++ src/main/builtin.c (working copy)
@@ -356,6 +356,24 @@
     return( ENCLOS(arg) );
 }

+static Rboolean R_IsImportsEnv(SEXP env)
+{
+    if (isNull(env) || !isEnvironment(env))
+        return FALSE;
+    if (ENCLOS(env) != R_BaseNamespace)
+        return FALSE;
+    SEXP name = getAttrib(env, R_NameSymbol);
+    if (!isString(name) || length(name) != 1)
+        return FALSE;
+
+    const char *imports_prefix = "imports:";
+    const char *name_string = CHAR(STRING_ELT(name, 0));
+    if (!strncmp(name_string, imports_prefix, strlen(imports_prefix)))
+        return TRUE;
+    else
+        return FALSE;
+}
+
 SEXP attribute_hidden do_parentenvgets(SEXP call, SEXP op, SEXP args, SEXP rho)
 {
     SEXP env, parent;
@@ -371,6 +389,10 @@
  error(_("argument is not an environment"));
     if( env == R_EmptyEnv )
  error(_("can not set parent of the empty environment"));
+    if (R_EnvironmentIsLocked(env) && R_IsNamespaceEnv(env))
+      error(_("can not set the parent environment of a namespace"));
+    if (R_EnvironmentIsLocked(env) && R_IsImportsEnv(env))
+      error(_("can not set the parent environment of package
imports"));
     parent = CADR(args);
     if (isNull(parent)) {
  error(_("use of NULL environment is defunct"));
?

I'll look into it

Sent from my iPhone
> On Oct 17, 2014, at 1:13 AM, "Karl Millar" <kmillar at
google.com> wrote:
> 
> I'd like to propose a change to the R language so that calling
> 'parent.env<-' on a package namespace or package imports is a
runtime
> error.
> 
> Currently the documentation warns that it's dangerous behaviour and
> might go away:
>     The replacement function ?parent.env<-? is extremely dangerous as
>     it can be used to destructively change environments in ways that
>     violate assumptions made by the internal C code.  It may be
>     removed in the near future.
> 
> This change would both eliminate some potential dangerous behaviours,
> and make it significantly easier for runtime compilation systems to
> optimize symbol lookups for code in packages.
> 
> The following patch against current svn implements this functionality.
> It allows calls to 'parent.env<-' only until the namespace is
locked,
> allowing the namespace to be built correctly while preventing user
> code from subsequently messing with it.
> 
> I'd also like to make calling parent.env<- on an environment on the
> call stack an error, for the same reasons, but it's not so obvious to
> me how to implement that efficiently right now.  Could we at least
> document that as being 'undefined behaviour'?
> 
> Thanks,
> 
> Karl
> 
> 
> Index: src/main/builtin.c
> ==================================================================> ---
src/main/builtin.c (revision 66783)
> +++ src/main/builtin.c (working copy)
> @@ -356,6 +356,24 @@
>     return( ENCLOS(arg) );
> }
> 
> +static Rboolean R_IsImportsEnv(SEXP env)
> +{
> +    if (isNull(env) || !isEnvironment(env))
> +        return FALSE;
> +    if (ENCLOS(env) != R_BaseNamespace)
> +        return FALSE;
> +    SEXP name = getAttrib(env, R_NameSymbol);
> +    if (!isString(name) || length(name) != 1)
> +        return FALSE;
> +
> +    const char *imports_prefix = "imports:";
> +    const char *name_string = CHAR(STRING_ELT(name, 0));
> +    if (!strncmp(name_string, imports_prefix, strlen(imports_prefix)))
> +        return TRUE;
> +    else
> +        return FALSE;
> +}
> +
> SEXP attribute_hidden do_parentenvgets(SEXP call, SEXP op, SEXP args, SEXP
rho)
> {
>     SEXP env, parent;
> @@ -371,6 +389,10 @@
>  error(_("argument is not an environment"));
>     if( env == R_EmptyEnv )
>  error(_("can not set parent of the empty environment"));
> +    if (R_EnvironmentIsLocked(env) && R_IsNamespaceEnv(env))
> +      error(_("can not set the parent environment of a
namespace"));
> +    if (R_EnvironmentIsLocked(env) && R_IsImportsEnv(env))
> +      error(_("can not set the parent environment of package
imports"));
>     parent = CADR(args);
>     if (isNull(parent)) {
>  error(_("use of NULL environment is defunct"));
> ?
> 
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel