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 1166 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();
 
     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);

[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;
         }
       }
     } else {
       // 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,
@@ skipping 11 matching lines @@
     // 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);
+      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
     // function declarations. To mimic this behavior, we do not allow
     // the invocation of setters for function values. This makes a
     // difference for global functions with the same names as event
     // handlers such as "function onload() {}". Firefox does call the
     // onload setter in those case and Safari does not. We follow
     // Safari for compatibility.
     if (value->IsJSFunction()) {
@@ skipping 154 matching lines @@
   // 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;
   LookupResult lookup;
   while (true) {
     real_holder->LocalLookup(*name, &lookup);
     if (lookup.IsProperty()) {
       // Determine if this is a redeclaration of something read-only.

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

Let's change all the mentions of declaration, redeclaration, etc. in the
comments to say 'initialization', 're-initialization', etc.

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

Done.

       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);
+        return isolate->heap()->undefined_value();
       }
 
       // Determine if this is a redeclaration of an intercepted read-only
       // property and figure out if the property exists at all.

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

This code is pretty complicated.  I think what it's trying to do (after the
redeclaration stuff is removed) is see if there is a non-ABSENT, non-READ_ONLY
interceptor on the global object or one of its hidden prototypes.  If so, set
the property on that hidden prototype.

If not, set the property on the global object itself.  I think it should be fine
to go ahead and try to do that even for READ_ONLY properties on the global
object itself, they should just be ignored by SetProperty.

Could we rewrite it as something more direct?  We have to be very careful
because there is no handle scope except in the case we see an interceptor, and
so we have to "export" handles from that handle scope in case there was a GC
(e.g., real_holder in the existing code) or look them up repeatedly (e.g.
isolate->context()->global()).

Object* object = global;
while (object->IsJSObject() &&
       JSObject::cast(object)->map()->is_hidden_prototype()) {
  real_holder->LocalLookup(*name, &lookup);
  if (lookup.IsProperty() && lookup.type() == INTERCEPTOR) {
    HandleScope ...
    // etc.;
    if (intercepted != ABSENT && (intercepted & READ_ONLY) == 0) {
      // Found an interceptor that's not read only.
      if (assign) {
        return real_holder->SetProperty(...);
      } else {
        return <undefined>;
      }
  object = JSObject::cast(object)->GetPrototoype();
}
// Here you can set the property on global itself.

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

Done.

       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
+          // 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();
         }
       }
 
       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.
@@ skipping 42 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

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

Not your code, but the comments that mention 'declaration' when this function is
doing initialization are confusing and need to be changed.

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

Done.

   // 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);
+      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
     // 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...

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

I'm not sure how to interpret "nirk", but it doesn't inspire much confidence.

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

I could change it to "nurk", which I think sounds more confident.

     // Passing non-strict mode because the property is writable.
     RETURN_IF_EMPTY_HANDLE(isolate,
                            SetProperty(global,
                                        name,
                                        value,
                                        attributes,
                                        kNonStrictMode));
     return *value;
   }
 
@@ skipping 11430 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