Change global const handling to silently ignore redeclarations

src/runtime.cc

Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index 9d3bb1d83b567480be63d4e4a355beecc636441e..2f8f8a299720ac7a82a72cd0a72503952d4cb2a3 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -1184,44 +1184,12 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DeclareGlobals) {
       LookupResult lookup;
       global->Lookup(*name, &lookup);
       if (lookup.IsProperty()) {
-        // Determine if the property is local by comparing the holder
-        // against the global object. The information will be used to
-        // avoid throwing re-declaration errors when declaring
-        // variables or constants that exist in the prototype chain.
-        bool is_local = (*global == lookup.holder());
-        // Get the property attributes and determine if the property is
-        // read-only.
         PropertyAttributes attributes = global->GetPropertyAttribute(*name);

[Kevin Millikin (Chromium), 2011/09/08 12:06:02]

I think you could probably simplify this code a bit (and avoid calling
GetPropertyAttributes unless you need to, because it will repeat the lookup).

// We found an existing property.  Unless it was an interceptor
// that claims the property is absent, skip this declaration.
if (lookup.type() != INTERCEPTOR) continue;
PropertyAttributes attributes = ...;
if (attributes != ABSENT) continue;

Or some such.

[Jakob Kummerow, 2011/09/09 15:42:43]

Done.

-        bool is_read_only = (attributes & READ_ONLY) != 0;
-        if (lookup.type() == INTERCEPTOR) {
-          // If the interceptor says the property is there, we
-          // just return undefined without overwriting the property.
-          // Otherwise, we continue to setting the property.
-          if (attributes != ABSENT) {
-            // Check if the existing property conflicts with regards to const.
-            if (is_local && (is_read_only || is_const_property)) {
-              const char* type = (is_read_only) ? "const" : "var";
-              return ThrowRedeclarationError(isolate, type, name);
-            };
-            // The property already exists without conflicting: Go to
-            // the next declaration.
-            continue;
-          }
+        if (lookup.type() == INTERCEPTOR && attributes == ABSENT) {
           // Fall-through and introduce the absent property by using
           // SetProperty.
         } else {
-          // For const properties, we treat a callback with this name
-          // even in the prototype as a conflicting declaration.
-          if (is_const_property && (lookup.type() == CALLBACKS)) {
-            return ThrowRedeclarationError(isolate, "const", name);
-          }
-          // Otherwise, we check for locally conflicting declarations.
-          if (is_local && (is_read_only || is_const_property)) {
-            const char* type = (is_read_only) ? "const" : "var";
-            return ThrowRedeclarationError(isolate, type, name);
-          }
-          // The property already exists without conflicting: Go to
-          // the next declaration.
+          // The property already exists: Go to the next declaration.
           continue;
         }
       }
@@ -1253,8 +1221,7 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DeclareGlobals) {
     if (lookup.IsProperty() &&
         (lookup.type() != INTERCEPTOR) &&
         (lookup.IsReadOnly() || is_const_property)) {
-      const char* type = (lookup.IsReadOnly()) ? "const" : "var";
-      return ThrowRedeclarationError(isolate, type, name);
+      continue;

[Kevin Millikin (Chromium), 2011/09/08 12:06:02]

I don't think this is what we intend.  Won't we skip a function declaration
following a const declaration?

const f = 0;
function f() { ... }

Because when processing the function declaration, there is a local property with
that name and it's read only?

I think we can get rid of this whole test+continue (and the completely confusing
comments before it).  In the declaring a var/const case, we've already skipped
these (the global lookup would have found this local property, and we skip all
where != INTERCEPTOR, not just some of them like here).  In the declaring a
function case, we don't want to skip them.

[Jakob Kummerow, 2011/09/09 15:42:43]

Done.

     }
 
     // Safari does not allow the invocation of callback setters for
@@ -1434,7 +1401,7 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_InitializeVarGlobal) {
         // If we found readonly property on one of hidden prototypes,
         // just shadow it.
         if (real_holder != isolate->context()->global()) break;
-        return ThrowRedeclarationError(isolate, "const", name);
+        return isolate->heap()->undefined_value();
       }
 
       // Determine if this is a redeclaration of an intercepted read-only
@@ -1451,12 +1418,11 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_InitializeVarGlobal) {
           // make sure to introduce it.
           found = false;
         } else if ((intercepted & READ_ONLY) != 0) {
-          // The property is present, but read-only. Since we're trying to
-          // overwrite it with a variable declaration we must throw a
-          // re-declaration error.  However if we found readonly property
+          // The property is present, but read-only, so we ignore the
+          // re-declaration.  However if we found readonly property
           // on one of hidden prototypes, just shadow it.
           if (real_holder != isolate->context()->global()) break;
-          return ThrowRedeclarationError(isolate, "const", name);
+          return isolate->heap()->undefined_value();
         }
       }
 
@@ -1524,15 +1490,15 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_InitializeConstGlobal) {
   // need to ask it for the property attributes.
   if (!lookup.IsReadOnly()) {
     if (lookup.type() != INTERCEPTOR) {
-      return ThrowRedeclarationError(isolate, "var", name);
+      return isolate->heap()->undefined_value();

[Kevin Millikin (Chromium), 2011/09/08 12:06:02]

I'm not sure this is our intended semantics.  Do we really want to ignore the
const initialization below?

eval("Object.setOwnPropertyDescriptor(this, 'x', { writable: true });"
   + "const x = 7; x");

The const declaration will introduce a property x with value undefined, the
const initialization will be ignored because the property is not read only. 
Hmmm.

[Jakob Kummerow, 2011/09/09 15:42:43]

Done.

     }
 
     PropertyAttributes intercepted = global->GetPropertyAttribute(*name);
 
-    // Throw re-declaration error if the intercepted property is present
+    // Ignore the re-declaration if the intercepted property is present
     // but not read-only.
     if (intercepted != ABSENT && (intercepted & READ_ONLY) == 0) {
-      return ThrowRedeclarationError(isolate, "var", name);
+      return isolate->heap()->undefined_value();
     }
 
     // Restore global object from context (in case of GC) and continue