Change global const handling to silently ignore redeclarations

src/runtime.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 1159 matching lines @@
     // assign to it when evaluating the assignment for "const x =
     // <expr>" the initial value is the hole.
     bool is_const_property = value->IsTheHole();
     bool is_function_declaration = false;
     if (value->IsUndefined() || is_const_property) {
       // Lookup the property in the global object, and don't set the
       // value of the variable if the property is already there.
       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);
-        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;
-          }
-          // 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.
+        // 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 = global->GetPropertyAttribute(*name);
+        if (attributes != ABSENT) {
+          continue;
+        }
+        // Fall-through and introduce the absent property by using
+        // SetProperty.
       }
     } else {
       is_function_declaration = true;
       // Copy the function and update its context. Use it as value.
       Handle<SharedFunctionInfo> shared =
           Handle<SharedFunctionInfo>::cast(value);
       Handle<JSFunction> function =
           isolate->factory()->NewFunctionFromSharedFunctionInfo(shared,
                                                                 context,
                                                                 TENURED);
       value = function;
     }
 
     LookupResult lookup;
     global->LocalLookup(*name, &lookup);
 
-    // There's a local property that we need to overwrite because
-    // we're either declaring a function or there's an interceptor
-    // that claims the property is absent.
-    //
-    // Check for conflicting re-declarations. We cannot have
-    // conflicting types in case of intercepted properties because
-    // they are absent.
-    if (lookup.IsProperty() &&
-        (lookup.type() != INTERCEPTOR) &&
-        (lookup.IsReadOnly() || is_const_property)) {
-      const char* type = (lookup.IsReadOnly()) ? "const" : "var";
-      return ThrowRedeclarationError(isolate, type, name);
-    }
-
     // Compute the property attributes. According to ECMA-262, section
     // 13, page 71, the property must be read-only and
     // non-deletable. However, neither SpiderMonkey nor KJS creates the
     // property as read-only, so we don't either.
     int attr = NONE;
     if ((flags & kDeclareGlobalsEvalFlag) == 0) {
       attr |= DONT_DELETE;
     }
     bool is_native = (flags & kDeclareGlobalsNativeFlag) != 0;
     if (is_const_property || (is_native && is_function_declaration)) {
@@ skipping 164 matching lines @@
   // not be deletable.
   PropertyAttributes attributes = DONT_DELETE;
 
   // Lookup the property locally in the global object. If it isn't
   // there, there is a property with this name in the prototype chain.
   // We follow Safari and Firefox behavior and only set the property
   // locally if there is an explicit initialization value that we have
   // to assign to the property.
   // Note that objects can have hidden prototypes, so we need to traverse
   // the whole chain of hidden prototypes to do a 'local' lookup.
-  JSObject* real_holder = global;
+  Object* object = global;
   LookupResult lookup;
-  while (true) {
+  while (object->IsJSObject() &&
+         JSObject::cast(object)->map()->is_hidden_prototype()) {
+    JSObject* real_holder = JSObject::cast(object);

[Kevin Millikin (Chromium), 2011/09/13 11:09:15]

This code is still a bit confusing.  I suggest renaming "real_holder" to
"raw_holder" (I'm not sure what makes it more real than the handle version).

[Jakob Kummerow, 2011/09/13 13:21:38]

Done.

     real_holder->LocalLookup(*name, &lookup);
-    if (lookup.IsProperty()) {
-      // Determine if this is a redeclaration of something read-only.
-      if (lookup.IsReadOnly()) {
-        // 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);
-      }
-
-      // Determine if this is a redeclaration of an intercepted read-only
-      // property and figure out if the property exists at all.
-      bool found = true;
-      PropertyType type = lookup.type();
-      if (type == INTERCEPTOR) {
-        HandleScope handle_scope(isolate);
-        Handle<JSObject> holder(real_holder);
-        PropertyAttributes intercepted = holder->GetPropertyAttribute(*name);
-        real_holder = *holder;
-        if (intercepted == ABSENT) {
-          // The interceptor claims the property isn't there. We need to
-          // 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
-          // on one of hidden prototypes, just shadow it.
-          if (real_holder != isolate->context()->global()) break;
-          return ThrowRedeclarationError(isolate, "const", name);
+    if (lookup.IsProperty() && lookup.type() == INTERCEPTOR) {
+      HandleScope handle_scope(isolate);
+      Handle<JSObject> holder(real_holder);
+      PropertyAttributes intercepted = holder->GetPropertyAttribute(*name);
+      real_holder = *holder;

[Kevin Millikin (Chromium), 2011/09/13 11:09:15]

And I think this line just needs a short comment that says something like,
'Update the raw pointer in case it's changed due to GC.'

Otherwise, it's easy to miss this line.

[Jakob Kummerow, 2011/09/13 13:21:38]

Done.

+      if (intercepted != ABSENT && (intercepted & READ_ONLY) == 0) {
+        // Found an interceptor that's not read only.
+        if (assign) {
+          return real_holder->SetProperty(
+              &lookup, *name, args[2], attributes, strict_mode);
+        } else {
+          return isolate->heap()->undefined_value();
         }
       }
-
-      if (found && !assign) {
-        // The global property is there and we're not assigning any value
-        // to it. Just return.
-        return isolate->heap()->undefined_value();
-      }
-
-      // Assign the value (or undefined) to the property.
-      Object* value = (assign) ? args[2] : isolate->heap()->undefined_value();
-      return real_holder->SetProperty(
-          &lookup, *name, value, attributes, strict_mode);
     }
-
-    Object* proto = real_holder->GetPrototype();
-    if (!proto->IsJSObject())
-      break;
-
-    if (!JSObject::cast(proto)->map()->is_hidden_prototype())
-      break;
-
-    real_holder = JSObject::cast(proto);
+    object = real_holder->GetPrototype();
   }
 
   global = isolate->context()->global();

[Kevin Millikin (Chromium), 2011/09/13 11:09:15]

Also a comment here that says something like 'Reload global in case the loop
above performed a GC.'

[Jakob Kummerow, 2011/09/13 13:21:38]

Done.

   if (assign) {
     return global->SetProperty(*name, args[2], attributes, strict_mode);
   }
   return isolate->heap()->undefined_value();
 }
 
 
 RUNTIME_FUNCTION(MaybeObject*, Runtime_InitializeConstGlobal) {
   // All constants are declared with an initial value. The name
   // of the constant is the first argument and the initial value
@@ skipping 16 matching lines @@
   // prototype chain (this rules out using SetProperty).
   // We use SetLocalPropertyIgnoreAttributes instead
   LookupResult lookup;
   global->LocalLookup(*name, &lookup);
   if (!lookup.IsProperty()) {
     return global->SetLocalPropertyIgnoreAttributes(*name,
                                                     *value,
                                                     attributes);
   }
 
-  // Determine if this is a redeclaration of something not
+  // Determine if this is a re-initialization of something not
   // read-only. In case the result is hidden behind an interceptor we
   // need to ask it for the property attributes.
-  if (!lookup.IsReadOnly()) {
-    if (lookup.type() != INTERCEPTOR) {
-      return ThrowRedeclarationError(isolate, "var", name);
-    }
-
+  if (!lookup.IsReadOnly() && lookup.type() == INTERCEPTOR) {
     PropertyAttributes intercepted = global->GetPropertyAttribute(*name);
 
-    // Throw re-declaration error if the intercepted property is present
+    // Ignore the re-initialization 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
     // with setting the value because the property is either absent or
     // read-only. We also have to do redo the lookup.
     HandleScope handle_scope(isolate);
     Handle<GlobalObject> global(isolate->context()->global());
 
     // BUG 1213575: Handle the case where we have to set a read-only
     // property through an interceptor and only do it if it's
     // uninitialized, e.g. the hole. Nirk...
     // Passing non-strict mode because the property is writable.
     RETURN_IF_EMPTY_HANDLE(isolate,
                            SetProperty(global,
                                        name,
                                        value,
                                        attributes,
                                        kNonStrictMode));
     return *value;
   }
 
   // Set the value, but only we're assigning the initial value to a
   // constant. For now, we determine this by checking if the
   // current value is the hole.
   // Strict mode handling not needed (const disallowed in strict mode).
   PropertyType type = lookup.type();
   if (type == FIELD) {
     FixedArray* properties = global->properties();
     int index = lookup.GetFieldIndex();
-    if (properties->get(index)->IsTheHole()) {
+    if (properties->get(index)->IsTheHole() || !lookup.IsReadOnly()) {
       properties->set(index, *value);
     }
   } else if (type == NORMAL) {
-    if (global->GetNormalizedProperty(&lookup)->IsTheHole()) {
+    if (global->GetNormalizedProperty(&lookup)->IsTheHole() ||
+        !lookup.IsReadOnly()) {
       global->SetNormalizedProperty(&lookup, *value);
     }
   } else {
     // Ignore re-initialization of constants that have already been
     // assigned a function value.
     ASSERT(lookup.IsReadOnly() && type == CONSTANT_FUNCTION);
   }
 
   // Use the set value as the result of the operation.
   return *value;
@@ skipping 11547 matching lines @@
   } else {
     // Handle last resort GC and make sure to allow future allocations
     // to grow the heap without causing GCs (if possible).
     isolate->counters()->gc_last_resort_from_js()->Increment();
     isolate->heap()->CollectAllGarbage(false);
   }
 }
 
 
 } }  // namespace v8::internal