Change global const handling to silently ignore redeclarations

Change global const handling to silently ignore redeclarations

and make window.{Infinity,NaN,undefined} read-only as per ES5


BUG=89490
TEST=mjsunit/const-redecl.js, mjsunit/undeletable-functions.js, es5conform, sputnik

Committed: http://code.google.com/p/v8/source/detail?r=9299

[Jakob Kummerow, 2011-08-31 09:25:10]

PTAL.

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

It's not your code, but the existing code is kind of convoluted.  I've
highlighted some parts where I don't think we're doing what we intend with this
change.

I'll try to figure out what we intend with the interceptors (it's a little
troubling at first glance that we don't have the same stuff for initializing
const context slots that end up hitting the global object).

Ultimately, but not as part of this change, we should change the whole thing to
more closely follow the algorithms in the 5.1 spec.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1187
src/runtime.cc:1187: PropertyAttributes attributes =
global->GetPropertyAttribute(*name);
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.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1224
src/runtime.cc:1224: continue;
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.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1399
src/runtime.cc:1399: // Determine if this is a redeclaration of something
read-only.
Let's change all the mentions of declaration, redeclaration, etc. in the
comments to say 'initialization', 're-initialization', etc.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1408
src/runtime.cc:1408: // property and figure out if the property exists at all.
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.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1488
src/runtime.cc:1488: // Determine if this is a redeclaration of something not
Not your code, but the comments that mention 'declaration' when this function is
doing initialization are confusing and need to be changed.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1493
src/runtime.cc:1493: return isolate->heap()->undefined_value();
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.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1512
src/runtime.cc:1512: // uninitialized, e.g. the hole. Nirk...
I'm not sure how to interpret "nirk", but it doesn't inspire much confidence.

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

Thanks for the feedback. Please take another look.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1187
src/runtime.cc:1187: PropertyAttributes attributes =
global->GetPropertyAttribute(*name);
On 2011/09/08 12:06:02, Kevin Millikin wrote:
> 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.

Done.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1224
src/runtime.cc:1224: continue;
On 2011/09/08 12:06:02, Kevin Millikin wrote:
> 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.

Done.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1399
src/runtime.cc:1399: // Determine if this is a redeclaration of something
read-only.
On 2011/09/08 12:06:02, Kevin Millikin wrote:
> Let's change all the mentions of declaration, redeclaration, etc. in the
> comments to say 'initialization', 're-initialization', etc.

Done.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1408
src/runtime.cc:1408: // property and figure out if the property exists at all.
On 2011/09/08 12:06:02, Kevin Millikin wrote:
> 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.

Done.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1488
src/runtime.cc:1488: // Determine if this is a redeclaration of something not
On 2011/09/08 12:06:02, Kevin Millikin wrote:
> Not your code, but the comments that mention 'declaration' when this function
is
> doing initialization are confusing and need to be changed.

Done.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1493
src/runtime.cc:1493: return isolate->heap()->undefined_value();
On 2011/09/08 12:06:02, Kevin Millikin wrote:
> 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.

Done.

http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1512
src/runtime.cc:1512: // uninitialized, e.g. the hole. Nirk...
On 2011/09/08 12:06:02, Kevin Millikin wrote:
> I'm not sure how to interpret "nirk", but it doesn't inspire much confidence.

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

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

OK, LGTM.  I have a few comments on the comments that you might incorporate.

http://codereview.chromium.org/7811015/diff/5001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7811015/diff/5001/src/runtime.cc#newcode1395
src/runtime.cc:1395: JSObject* real_holder = JSObject::cast(object);
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).

http://codereview.chromium.org/7811015/diff/5001/src/runtime.cc#newcode1401
src/runtime.cc:1401: real_holder = *holder;
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.

http://codereview.chromium.org/7811015/diff/5001/src/runtime.cc#newcode1415
src/runtime.cc:1415: global = isolate->context()->global();
Also a comment here that says something like 'Reload global in case the loop
above performed a GC.'

[Erik Corry, 2011-09-13 11:24:36]

What does Safari do?

[Jakob Kummerow, 2011-09-13 12:28:25]

On 2011/09/13 11:24:36, Erik Corry wrote:
> What does Safari do?

You're stirring a hornets' nest here... ;-)

Safari does almost the same thing, but it lets consts be overridden by
redeclarations. Example (from const-redecl.js):
const d = 1; const d = 2;
In Safari, this results in a == 2. We decided it would make more sense to treat
d as read-only after it has been initialized to 1.

From the tests I've added to const-redecl.js, also the test for "k" fails on
Safari (k is 1 rather than undefined); all the others pass.

[Jakob Kummerow, 2011-09-13 13:21:37]

Comments addressed, landing.

http://codereview.chromium.org/7811015/diff/5001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7811015/diff/5001/src/runtime.cc#newcode1395
src/runtime.cc:1395: JSObject* real_holder = JSObject::cast(object);
On 2011/09/13 11:09:15, Kevin Millikin wrote:
> 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).

Done.

http://codereview.chromium.org/7811015/diff/5001/src/runtime.cc#newcode1401
src/runtime.cc:1401: real_holder = *holder;
On 2011/09/13 11:09:15, Kevin Millikin wrote:
> 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.

Done.

http://codereview.chromium.org/7811015/diff/5001/src/runtime.cc#newcode1415
src/runtime.cc:1415: global = isolate->context()->global();
On 2011/09/13 11:09:15, Kevin Millikin wrote:
> Also a comment here that says something like 'Reload global in case the loop
> above performed a GC.'

Done.

[Jakob Kummerow, 2011-09-14 15:26:07]

On 2011/09/13 13:21:37, Jakob wrote:
> Comments addressed, landing.

Cancel that, found an assertion failure. Patch set 4 is the result from our
discussion how to fix it. Please take another look.

It is still unclear whether we need special rules for interceptors or not. The
plan is to land this and see if there are any problems.

[Kevin Millikin (Chromium), 2011-09-15 11:47:51]

LGTM.