ES6 Unscopables

This implements unscopables

The unscobables allow us to black list properties from showing up in
with statements.

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-object-environment-records-hasbinding-n

The spec draft is not fully up to date.

https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-07/jul-29.md#conclusionresolution

BUG=v8:3401
LOG=Y
R=rossberg@chromium.org, verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22942

[arv (Not doing code reviews), 2014-07-10 23:13:11]

Hi Andreas,

As requested, I'm uploading this patch now that I've implemented the
GetBindingValue part.

Does this look like a good approach?

I think it is doable to refactor the code to share the prototype walking between
the two functions.

UnscopableLookupHelper res(isolate, name, object);
if (res.HasException()) { ... }
else if (res.FoundProperty()) { ... }
else { ... }

https://codereview.chromium.org/384963002/diff/1/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/384963002/diff/1/src/contexts.cc#newcode80
src/contexts.cc:80: while (true) {
I'll switch to use the new PrototypeIterator.

[arv (Not doing code reviews), 2014-07-10 23:13:12]

Hi Andreas,

As requested, I'm uploading this patch now that I've implemented the
GetBindingValue part.

Does this look like a good approach?

I think it is doable to refactor the code to share the prototype walking between
the two functions.

UnscopableLookupHelper res(isolate, name, object);
if (res.HasException()) { ... }
else if (res.FoundProperty()) { ... }
else { ... }

[rossberg, 2014-07-11 11:59:07]

Thanks Arv, looks good so far. Besides the refactoring you suggest I'd mainly
like to have a few additional tests (see below).

https://codereview.chromium.org/384963002/diff/1/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/384963002/diff/1/src/contexts.cc#newcode80
src/contexts.cc:80: while (true) {
On 2014/07/10 23:13:11, arv wrote:
> I'll switch to use the new PrototypeIterator.

Yes, that would be better.

https://codereview.chromium.org/384963002/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/384963002/diff/20001/src/objects-inl.h#newcod...
src/objects-inl.h:1149: MaybeHandle<Object>
Object::GetPropertyWithReceiver(Handle<JSReceiver> holder,
I'd prefer to inline this function, it doesn't seem worth polluting the object
API with.

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
File test/mjsunit/harmony/unscopables.js (right):

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
test/mjsunit/harmony/unscopables.js:8: (function TestBasics() {
Some of these tests should probably be run on different kinds of objects
(normal, array, function, the global, etc.). And in particular, proxies... ;)
(that one maybe in a separate file, to allow separate shipping).

Also, include a test that not just reads but writes some variables, as that can
take quite different code paths. Likewise, a test where the properties in
question (on the with'ed object) are accessors.

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
test/mjsunit/harmony/unscopables.js:103: (function TestChangeDuringWith() {
It would be good to have a variation of this test where the _same_ variable
reference is evaluated several times (e.g. using a loop) with different results.
Just to ensure that no overoptimistic optimisation can screw anything up.

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
test/mjsunit/harmony/unscopables.js:134: (function TestAccessorReceiver() {
Hm, this test doesn't use unscopables at all...

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
test/mjsunit/harmony/unscopables.js:176: Object.defineProperty(object,
Symbol.unscopables, {
It would definitely be good to have variants of the following two tests using
assignments to x. I'm not even sure where those assignment are supposed to end
up... :)

[arv (Not doing code reviews), 2014-07-11 21:21:42]

Add confusing test

[arv (Not doing code reviews), 2014-07-11 21:45:27]

Run all with tests with unscopables turned on

[arv (Not doing code reviews), 2014-07-11 21:47:29]

Updated but still not done.

I'm seeing failures in test/mjsunit/with-leave.js. I'll look into that next
week.

https://codereview.chromium.org/384963002/diff/1/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/384963002/diff/1/src/contexts.cc#newcode80
src/contexts.cc:80: while (true) {
On 2014/07/11 11:59:06, rossberg wrote:
> On 2014/07/10 23:13:11, arv wrote:
> > I'll switch to use the new PrototypeIterator.
> 
> Yes, that would be better.

Waiting for that CL to land...

https://codereview.chromium.org/384963002/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/384963002/diff/20001/src/objects-inl.h#newcod...
src/objects-inl.h:1149: MaybeHandle<Object>
Object::GetPropertyWithReceiver(Handle<JSReceiver> holder,
On 2014/07/11 11:59:06, rossberg wrote:
> I'd prefer to inline this function, it doesn't seem worth polluting the object
> API with.

Done.

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
File test/mjsunit/harmony/unscopables.js (right):

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
test/mjsunit/harmony/unscopables.js:8: (function TestBasics() {
On 2014/07/11 11:59:07, rossberg wrote:
> Some of these tests should probably be run on different kinds of objects
> (normal, array, function, the global, etc.). And in particular, proxies... ;)
> (that one maybe in a separate file, to allow separate shipping).
> 
> Also, include a test that not just reads but writes some variables, as that
can
> take quite different code paths. Likewise, a test where the properties in
> question (on the with'ed object) are accessors.

Our proxies do not support symbol property names so we cannot put a
Symbol.unscopables on the actual proxy... but I can do some basic tests since
with a proxy it is possible to have an inconsistent getOwnPropertyDescriptor
which is used for Lookup and then later for GetProperty.

For setting, unscopables only comes in for the first pass when we do HasBinding.
If that is true, then a normal Put is used.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-object-environment-r...

This leads to pretty strange behavior and I commented on the bug about it.

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
test/mjsunit/harmony/unscopables.js:103: (function TestChangeDuringWith() {
On 2014/07/11 11:59:07, rossberg wrote:
> It would be good to have a variation of this test where the _same_ variable
> reference is evaluated several times (e.g. using a loop) with different
results.
> Just to ensure that no overoptimistic optimisation can screw anything up.

Done.

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
test/mjsunit/harmony/unscopables.js:134: (function TestAccessorReceiver() {
On 2014/07/11 11:59:07, rossberg wrote:
> Hm, this test doesn't use unscopables at all...

Since --harmony-unscopables changes how with works I need to test this code path
too. I'm also adding a test that runs all of the existing tests with unscopables
turned on so maybe this can be removed.

https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/uns...
test/mjsunit/harmony/unscopables.js:176: Object.defineProperty(object,
Symbol.unscopables, {
On 2014/07/11 11:59:07, rossberg wrote:
> It would definitely be good to have variants of the following two tests using
> assignments to x. I'm not even sure where those assignment are supposed to end
> up... :)

Done.

[arv (Not doing code reviews), 2014-07-16 22:57:02]

Changed tests to test multiple object types

[arv (Not doing code reviews), 2014-07-16 23:01:14]

This is now ready for review. PTAL.

[rossberg, 2014-07-17 15:12:14]

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h
File src/unscopables.h (right):

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h#newco...
src/unscopables.h:20: class UnscopableLookup {
Hm, this class seems overkill to me. It should be adequate enough to just make
it a static function like

  bool UnscopableLookup(Isolate*,
    Handle<JSReceiver> object, Handle<String> name,
    Handle<JSReceiver>* holder, PropertyAttributes* attr, bool* ok)

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h#newco...
src/unscopables.h:55: JSReceiver::GetOwnPropertyAttributes(object, name_);
Shouldn't this call HasOwnProperty?

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h#newco...
src/unscopables.h:59: JSReceiver::GetOwnPropertyAttributes(object,
unscopables_symbol);
Same here?

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h#newco...
src/unscopables.h:72: blocked_attrs = JSReceiver::GetOwnPropertyAttributes(
And here?

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/pr...
File test/mjsunit/harmony/proxies-with-unscopables.js (right):

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/pr...
test/mjsunit/harmony/proxies-with-unscopables.js:6: // Flags: --harmony-proxies
Hm, shouldn't there also be some tests that actually define @@unscopables on a
proxy, and one that uses a proxy as @@unscopables?

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
File test/mjsunit/harmony/unscopables.js (right):

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:11: function runtTest(f) {
Nit: s/runtTest/runTest/

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:23: case 0: return {};
Why not simply use an array? Then you don't need to hardcode 4 below either.

Perhaps add a couple more cases, and maybe a few different cases of property
names as well. For example, the matrix that we test for O.o:

var objects = [
  {},
  [],
  function(){},
  (function(){ return arguments })(),
  (function(){ "use strict"; return arguments })(),
  Object(1), Object(true), Object("bla"),
  new Date(),
  Object, Function, Date, RegExp,
  new Set, new Map, new WeakMap,
  new ArrayBuffer(10), new Int32Array(5),
  global
];
var properties = ["a", "1", 1, "length", "prototype", "name", "caller"];

Would also be good to have the @@unscopable object itself iterate through the
`objects` list. And to install the properties on the @@unscopables as accessors
as well (particular ones without getters). :)

You may think that this should obviously work, but you never know in V8...

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:39: var proto = getObject(i);
getObject(j) ? Otherwise, you don't execute anything. ;)

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:43: f(getObject(i), getObject(j));
f(object, proto) ?

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:180: function
TestChangeDuringWithWithPossibleOptimization(object) {
I had in mind a test like the following:

var x = 1
object.x = 2
with (object) {
  for (var i = 0; i < 10000; i++) {
    if (i === 500) object[Symbol.unscopables] = {x: true}
    assertEquals(i < 500 ? 1 : 2, x);
  }
}

and dually,

var x = 1
object.x = 2
object[Symbol.unscopables] = {x: true}
with (object) {
  for (var i = 0; i < 10000; i++) {
    if (i === 500) delete object[Symbol.unscopables]
    assertEquals(i < 500 ? 2 : 1, x);
  }
}

and perhaps also the variants

var x = 1
object.x = 2
object[Symbol.unscopables] = {}
with (object) {
  for (var i = 0; i < 10000; i++) {
    if (i === 500) object[Symbol.unscopables].x = true
    assertEquals(i < 500 ? 2 : 1, x);
  }
}

and

var x = 1
object.x = 2
object[Symbol.unscopables] = {x: true}
with (object) {
  for (var i = 0; i < 10000; i++) {
    if (i === 500) delete object[Symbol.unscopables].x
    assertEquals(i < 500 ? 2 : 1, x);
  }
}

The point being that lookup is always for the same variable reference `x` in the
code -- which changes its meaning over time, however.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:354: assertTrue(false);
assertUnreachable()

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:383: x = 3;
Yeah, what this does is really weird...

[arv (Not doing code reviews), 2014-07-17 23:34:37]

I haven't uploaded a new snapshot yet. I got stuck on the following:

var length = 2;
var a = [1];
assertEquals(0, Array.prototype.length);
a[Symbol.unscopables] = {length: true};
with (a) {
  assertEquals(0, length);  // FAILS
}

The problem seems to come down to length being implemented as an ACCESSOR and it
seems to always return the instance (receiver) length even when:

LookupIterator it(receiver, name, holder);
// (arrayInstance, "length", ArrayPrototype)

I guess I can detect if the ACCESSOR is backed by a getter or not and change to
get the property direct from the holder if it isn't.

This feels hacky to me. Maybe I am missing something?

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h
File src/unscopables.h (right):

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h#newco...
src/unscopables.h:20: class UnscopableLookup {
On 2014/07/17 15:12:13, rossberg wrote:
> Hm, this class seems overkill to me. It should be adequate enough to just make
> it a static function like
> 
>   bool UnscopableLookup(Isolate*,
>     Handle<JSReceiver> object, Handle<String> name,
>     Handle<JSReceiver>* holder, PropertyAttributes* attr, bool* ok)

Done.

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h#newco...
src/unscopables.h:55: JSReceiver::GetOwnPropertyAttributes(object, name_);
On 2014/07/17 15:12:13, rossberg wrote:
> Shouldn't this call HasOwnProperty?

The caller needs the PropertyAttributes.

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h#newco...
src/unscopables.h:59: JSReceiver::GetOwnPropertyAttributes(object,
unscopables_symbol);
On 2014/07/17 15:12:13, rossberg wrote:
> Same here?

This one can be changed though.

https://codereview.chromium.org/384963002/diff/100001/src/unscopables.h#newco...
src/unscopables.h:72: blocked_attrs = JSReceiver::GetOwnPropertyAttributes(
On 2014/07/17 15:12:13, rossberg wrote:
> And here?

This can also be changed.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/pr...
File test/mjsunit/harmony/proxies-with-unscopables.js (right):

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/pr...
test/mjsunit/harmony/proxies-with-unscopables.js:6: // Flags: --harmony-proxies
On 2014/07/17 15:12:13, rossberg wrote:
> Hm, shouldn't there also be some tests that actually define @@unscopables on a
> proxy, and one that uses a proxy as @@unscopables?

Like I said in a comment on the CL. Proxies cannot have symbols as property
names.

I'll add a TODO.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
File test/mjsunit/harmony/unscopables.js (right):

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:11: function runtTest(f) {
On 2014/07/17 15:12:13, rossberg wrote:
> Nit: s/runtTest/runTest/

Done.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:23: case 0: return {};
On 2014/07/17 15:12:13, rossberg wrote:
> Why not simply use an array? Then you don't need to hardcode 4 below either.

These needs to be new objects every time but I agree that the switch is an
unnecessary optimization.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:39: var proto = getObject(i);
On 2014/07/17 15:12:13, rossberg wrote:
> getObject(j) ? Otherwise, you don't execute anything. ;)

wow. That refactoring just completely failed.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:43: f(getObject(i), getObject(j));
On 2014/07/17 15:12:13, rossberg wrote:
> f(object, proto) ?

Done.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:180: function
TestChangeDuringWithWithPossibleOptimization(object) {
On 2014/07/17 15:12:13, rossberg wrote:
> I had in mind a test like the following:
> 
> var x = 1
> object.x = 2
> with (object) {
>   for (var i = 0; i < 10000; i++) {
>     if (i === 500) object[Symbol.unscopables] = {x: true}
>     assertEquals(i < 500 ? 1 : 2, x);
>   }
> }
> 
> and dually,
> 
> var x = 1
> object.x = 2
> object[Symbol.unscopables] = {x: true}
> with (object) {
>   for (var i = 0; i < 10000; i++) {
>     if (i === 500) delete object[Symbol.unscopables]
>     assertEquals(i < 500 ? 2 : 1, x);
>   }
> }
> 
> and perhaps also the variants
> 
> var x = 1
> object.x = 2
> object[Symbol.unscopables] = {}
> with (object) {
>   for (var i = 0; i < 10000; i++) {
>     if (i === 500) object[Symbol.unscopables].x = true
>     assertEquals(i < 500 ? 2 : 1, x);
>   }
> }
> 
> and
> 
> var x = 1
> object.x = 2
> object[Symbol.unscopables] = {x: true}
> with (object) {
>   for (var i = 0; i < 10000; i++) {
>     if (i === 500) delete object[Symbol.unscopables].x
>     assertEquals(i < 500 ? 2 : 1, x);
>   }
> }
> 
> The point being that lookup is always for the same variable reference `x` in
the
> code -- which changes its meaning over time, however.

Thanks. Added.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:354: assertTrue(false);
On 2014/07/17 15:12:13, rossberg wrote:
> assertUnreachable()

Done.

https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unscopables.js:383: x = 3;
On 2014/07/17 15:12:13, rossberg wrote:
> Yeah, what this does is really weird...

Acknowledged.

[rossberg, 2014-07-21 09:16:15]

On 2014/07/17 23:34:37, arv wrote:
> I haven't uploaded a new snapshot yet. I got stuck on the following:
> 
> var length = 2;
> var a = [1];
> assertEquals(0, Array.prototype.length);
> a[Symbol.unscopables] = {length: true};
> with (a) {
>   assertEquals(0, length);  // FAILS
> }
> 
> The problem seems to come down to length being implemented as an ACCESSOR and
it
> seems to always return the instance (receiver) length even when:
> 
> LookupIterator it(receiver, name, holder);
> // (arrayInstance, "length", ArrayPrototype)

Argh, bummer. Talking to Toon, that's a known problem with native accessors. He
has a plan for cleaning this up, but there is quite a bit of Blink code relying
on the current behaviour, so it will take some time.

In the meantime, it's probably best to punt and comment out this case in the
test. But please open an issue tracking it, and add the failing test to
test/mjsunit/bugs, respectively.

>
https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/pr...
> File test/mjsunit/harmony/proxies-with-unscopables.js (right):
> 
>
https://codereview.chromium.org/384963002/diff/100001/test/mjsunit/harmony/pr...
> test/mjsunit/harmony/proxies-with-unscopables.js:6: // Flags:
--harmony-proxies
> On 2014/07/17 15:12:13, rossberg wrote:
> > Hm, shouldn't there also be some tests that actually define @@unscopables on
a
> > proxy, and one that uses a proxy as @@unscopables?
> 
> Like I said in a comment on the CL. Proxies cannot have symbols as property
> names.

Right, sorry. But it should still be possible to have a test using a proxy for
the @@unscopables object.

[arv (Not doing code reviews), 2014-07-21 15:27:29]

On 2014/07/21 at 09:16:15, rossberg wrote:
> On 2014/07/17 23:34:37, arv wrote:
> > I haven't uploaded a new snapshot yet. I got stuck on the following:
> > 
> > var length = 2;
> > var a = [1];
> > assertEquals(0, Array.prototype.length);
> > a[Symbol.unscopables] = {length: true};
> > with (a) {
> >   assertEquals(0, length);  // FAILS
> > }
> > 
> > The problem seems to come down to length being implemented as an ACCESSOR
and it
> > seems to always return the instance (receiver) length even when:
> > 
> > LookupIterator it(receiver, name, holder);
> > // (arrayInstance, "length", ArrayPrototype)
> 
> Argh, bummer. Talking to Toon, that's a known problem with native accessors.
He has a plan for cleaning this up, but there is quite a bit of Blink code
relying on the current behaviour, so it will take some time.
> 
> In the meantime, it's probably best to punt and comment out this case in the
test. But please open an issue tracking it, and add the failing test to
test/mjsunit/bugs, respectively.

I can get length etc to work but it does require one more lookup which is
visible using proxies.

I also got it working by copy/pasting Object::GetProperty and make it
differentiate whether the accessor is an AccessorPair or not. This approach is
not working in general but it does work for unscopables.

I'm not sure what is worse here?


I'll upload what I have so you can see how ugly it gets.

[rossberg, 2014-07-21 16:29:31]

On 2014/07/21 15:27:29, arv wrote:
> On 2014/07/21 at 09:16:15, rossberg wrote:
> > On 2014/07/17 23:34:37, arv wrote:
> > > I haven't uploaded a new snapshot yet. I got stuck on the following:
> > > 
> > > var length = 2;
> > > var a = [1];
> > > assertEquals(0, Array.prototype.length);
> > > a[Symbol.unscopables] = {length: true};
> > > with (a) {
> > >   assertEquals(0, length);  // FAILS
> > > }
> > > 
> > > The problem seems to come down to length being implemented as an ACCESSOR
> and it
> > > seems to always return the instance (receiver) length even when:
> > > 
> > > LookupIterator it(receiver, name, holder);
> > > // (arrayInstance, "length", ArrayPrototype)
> > 
> > Argh, bummer. Talking to Toon, that's a known problem with native accessors.
> He has a plan for cleaning this up, but there is quite a bit of Blink code
> relying on the current behaviour, so it will take some time.
> > 
> > In the meantime, it's probably best to punt and comment out this case in the
> test. But please open an issue tracking it, and add the failing test to
> test/mjsunit/bugs, respectively.
> 
> I can get length etc to work but it does require one more lookup which is
> visible using proxies.
> 
> I also got it working by copy/pasting Object::GetProperty and make it
> differentiate whether the accessor is an AccessorPair or not. This approach is
> not working in general but it does work for unscopables.
> 
> I'm not sure what is worse here?

I'm fine with not doing either workaround, and wait till the actual cause of the
problem is fixed. I don't think array.length will be relevant in practice.

But if you're keen on doing it, then I think I like the first option better --
esp since symbols don't work for proxies right now anyway. Be sure to add a TODO
that this is a temporary hack, though.

[arv (Not doing code reviews), 2014-07-21 19:23:47]

I refactored GetProperty to force usage of the holder as the receiver when we
have a non JS accessor callback.

PTAL

[rossberg, 2014-07-22 17:03:19]

On 2014/07/21 19:23:47, arv wrote:
> I refactored GetProperty to force usage of the holder as the receiver when we
> have a non JS accessor callback.

OK, Toon should have a look then.

[Toon Verwaest, 2014-07-23 12:08:39]

My comment below is based on a short explanation of the problem by rossberg. Let
me know if some things are still unclear.

Overall: Please don't work around "bugs" (wasn't really a bug until unscopable
since it wasn't observable in any way) in one part of the system by obfuscating
another part.

https://codereview.chromium.org/384963002/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/384963002/diff/120001/src/objects.cc#newcode165
src/objects.cc:165: }
Euh, no.

What you want to do is fix a bug in Array.length; but what you do instead is
modify get-property to change the semantics of all API accessors...

The right fix is to change ArrayLengthGetter to not use This() (via
GetThisFrom), but rather read the value from the Holder(). That also removes the
need for FindInstanceOf.

[arv (Not doing code reviews), 2014-07-23 15:19:37]

Revert changes to objects and comment out failing tests

[arv (Not doing code reviews), 2014-07-23 18:13:41]

This is now blocked by https://codereview.chromium.org/410923003

[arv (Not doing code reviews), 2014-07-23 20:55:03]

merge with accessors fixes in

[arv (Not doing code reviews), 2014-07-23 20:58:22]

PTAL

[arv (Not doing code reviews), 2014-07-23 23:06:20]

Cleanup code using LookupIterator

[arv (Not doing code reviews), 2014-07-23 23:10:41]

Switching to use a LookupIterator cleaned this up a bit.

https://codereview.chromium.org/384963002/diff/190001/src/unscopables.h
File src/unscopables.h (right):

https://codereview.chromium.org/384963002/diff/190001/src/unscopables.h#newco...
src/unscopables.h:24: if (it->state() == LookupIterator::JSPROXY) {
Toon, this is a bit ugly. I was considering adding a GetHolderOrJSProxy but
changing GetHolder to return a JSReceiver seems reasonable to me as well.

I'll look at the hidden prototype issue tomorrow.

[arv (Not doing code reviews), 2014-07-24 21:32:16]

Use new GetHolder

[Toon Verwaest, 2014-07-25 10:09:41]

https://codereview.chromium.org/384963002/diff/200001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/384963002/diff/200001/src/runtime.cc#newcode9140
src/runtime.cc:9140: PropertyAttributes attrs = UnscopableLookup(&it);
An exception could already have been thrown in UnscopableLookup. Check for
has_pending_exception. I suggest returning (at least) Maybe<PropertyAttributes>
to indicate this possibility.

https://codereview.chromium.org/384963002/diff/200001/src/unscopables.h
File src/unscopables.h (right):

https://codereview.chromium.org/384963002/diff/200001/src/unscopables.h#newco...
src/unscopables.h:28: if (!maybe_unscopables.ToHandle(&unscopables)) return
false;
Maybe add a comment here saying that you return false to flag a failure? OTOH,
what about returning a Maybe<bool> and using that to indicate that you may have
a failure rather than just a result?

https://codereview.chromium.org/384963002/diff/200001/src/unscopables.h#newco...
src/unscopables.h:30: return
JSReceiver::HasOwnProperty(Handle<JSReceiver>::cast(unscopables),
This HasOwnProperty can cause an exception at least in the case where the
unscopables is a JSProxy. I wouldn't rely on the returned bool to indicate
whether there is an exception. Check has_pending_exception() instead. (See my
comment below)

https://codereview.chromium.org/384963002/diff/200001/src/unscopables.h#newco...
src/unscopables.h:42: while ((attrs = JSReceiver::GetPropertyAttributes(it)) !=
ABSENT) {
At least GetPropertyAttributesWithHandler will return NONE (in at least some
cases) if GetPropertyAttributes causes an exception to be thrown while getting
the attributes. GetPropertyAttributes currently has no other way of flagging
that it may fail. The client has to check for has_pending_exception(), and
manually propagate the exception up if so.

(This is obviously very error-prone; and making this explicit is on our TODO
list).

[arv (Not doing code reviews), 2014-07-25 15:25:41]

On 2014/07/25 at 10:09:41, verwaest wrote:
> https://codereview.chromium.org/384963002/diff/200001/src/runtime.cc
> File src/runtime.cc (right):
> 
>
https://codereview.chromium.org/384963002/diff/200001/src/runtime.cc#newcode9140
> src/runtime.cc:9140: PropertyAttributes attrs = UnscopableLookup(&it);
> An exception could already have been thrown in UnscopableLookup. Check for
has_pending_exception. I suggest returning (at least) Maybe<PropertyAttributes>
to indicate this possibility.
> 
> https://codereview.chromium.org/384963002/diff/200001/src/unscopables.h
> File src/unscopables.h (right):
> 
>
https://codereview.chromium.org/384963002/diff/200001/src/unscopables.h#newco...
> src/unscopables.h:28: if (!maybe_unscopables.ToHandle(&unscopables)) return
false;
> Maybe add a comment here saying that you return false to flag a failure? OTOH,
what about returning a Maybe<bool> and using that to indicate that you may have
a failure rather than just a result?
> 
>
https://codereview.chromium.org/384963002/diff/200001/src/unscopables.h#newco...
> src/unscopables.h:30: return
JSReceiver::HasOwnProperty(Handle<JSReceiver>::cast(unscopables),
> This HasOwnProperty can cause an exception at least in the case where the
unscopables is a JSProxy. I wouldn't rely on the returned bool to indicate
whether there is an exception. Check has_pending_exception() instead. (See my
comment below)
> 
>
https://codereview.chromium.org/384963002/diff/200001/src/unscopables.h#newco...
> src/unscopables.h:42: while ((attrs = JSReceiver::GetPropertyAttributes(it))
!= ABSENT) {
> At least GetPropertyAttributesWithHandler will return NONE (in at least some
cases) if GetPropertyAttributes causes an exception to be thrown while getting
the attributes. GetPropertyAttributes currently has no other way of flagging
that it may fail. The client has to check for has_pending_exception(), and
manually propagate the exception up if so.
> 
> (This is obviously very error-prone; and making this explicit is on our TODO
list).

It seems like exceptions are handled at a higher level already (I'm trying to
find where). All exceptions I throw at this are being reported as far as I can
tell.

[arv (Not doing code reviews), 2014-07-25 15:33:26]

Never mind... I found a repro where the error is not propagated.

https://codereview.chromium.org/384963002/diff/200001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/384963002/diff/200001/src/contexts.cc#newcode120
src/contexts.cc:120: if (isolate->has_pending_exception()) return
Handle<Object>();
Exception is handled here.

[arv (Not doing code reviews), 2014-07-25 16:45:42]

Call me Maybe?

[Toon Verwaest, 2014-07-25 18:34:26]

I just landed a CL that changes the return type to Maybe<> for those methods, so
we won't get this wrong anymore:
https://code.google.com/p/v8/source/detail?r=22624

[Toon Verwaest, 2014-07-25 18:52:11]

https://codereview.chromium.org/384963002/diff/200001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/384963002/diff/200001/src/contexts.cc#newcode120
src/contexts.cc:120: if (isolate->has_pending_exception()) return
Handle<Object>();
The problem is that it was looking for pending exceptions, but it may just have
been scheduled. Anyway, after my CL that distinction is now irrelevant, and
pending is all you need to care about.

On 2014/07/25 15:33:26, arv wrote:
> Exception is handled here.

[arv (Not doing code reviews), 2014-07-25 21:03:09]

Added cctest to test hidden prototypes

[arv (Not doing code reviews), 2014-07-25 21:08:33]

Toon, I added a cctest that tests the behavior with hidden prototypes. It seems
like we are getting the right behavior already which is kind of surprising.

[Toon Verwaest, 2014-07-29 15:09:41]

If your sixth test are the expected semantics, then you indeed don't need to do
anything special.

I was just wondering what was expected. I presumed that if you want to hide
something from an object, it should also hide it from the hidden prototype;
otherwise you can't (from userland) ignore properties that are installed on
hidden prototypes (through the API).

[Toon Verwaest, 2014-07-29 15:12:39]

lgtm. just a last comment.

https://codereview.chromium.org/384963002/diff/260001/src/unscopables.h
File src/unscopables.h (right):

https://codereview.chromium.org/384963002/diff/260001/src/unscopables.h#newco...
src/unscopables.h:25: } while (false)
Can you please move these macros next to the other macros so we can use them as
well? Maybe even put them there in a separate CL?

[arv (Not doing code reviews), 2014-07-29 15:19:09]

On 2014/07/29 at 15:12:39, verwaest wrote:
> lgtm. just a last comment.
> 
> https://codereview.chromium.org/384963002/diff/260001/src/unscopables.h
> File src/unscopables.h (right):
> 
>
https://codereview.chromium.org/384963002/diff/260001/src/unscopables.h#newco...
> src/unscopables.h:25: } while (false)
> Can you please move these macros next to the other macros so we can use them
as well? Maybe even put them there in a separate CL?

I have some issues with this CL as is so: Not LGTM

The test is flawed... I fixed the CL yesterday on my flight and I also extended
the LookupIterator to keep track of the last non hidden prototype.

I'm having some technical difficulties with git cl upload on this new laptop.
I'll ping you when I resolve them.

[Toon Verwaest, 2014-07-29 16:01:33]

Ok, sounds good.

[arv (Not doing code reviews), 2014-07-29 19:26:03]

We talked about @@unscopables today at TC39 and we are going back to "Plan A"
which means that we will do a object.[[Get]](@@unscopables) and then a
unscopableObject.[[Has]](name). This should simplify this CL quite a bit.

[arv (Not doing code reviews), 2014-07-30 15:39:38]

https://codereview.chromium.org/384963002/diff/260001/src/unscopables.h
File src/unscopables.h (right):

https://codereview.chromium.org/384963002/diff/260001/src/unscopables.h#newco...
src/unscopables.h:25: } while (false)
On 2014/07/29 15:12:39, Toon Verwaest wrote:
> Can you please move these macros next to the other macros so we can use them
as
> well? Maybe even put them there in a separate CL?

It feels kind of wrong to assume that all usages of Maybe that returns a no
value was due to an exception. That means the ASSERT needs to go away.

The other similar macros do not do the declaration in the macro. The usage would
then be more like:

bool has_own;
RETURN_ON_NO_VALUE(has_own,
    JSReceiver::HasOwnProperty(object, unscopables_symbol),
    Maybe<bool>());

But that lost the assert...

My current thinking is that the following would be the cleanest

bool has_own;
if (!JSReceiver::HasOwnProperty(object, unscopables_symbol).ToValue(&has_own)) {
  ASSERT((isolate)->has_pending_exception());
  return Maybe<bool>();
}

This matches ToHandle we have for MaybeHandle and does not depend on macros.

WDYT?

[arv (Not doing code reviews), 2014-08-05 16:47:44]

This now reflects the JUly 2014 consesus.

PTAL

[arv (Not doing code reviews), 2014-08-05 16:52:01]

Andreas, https://codereview.chromium.org/441943002/ removes Symbol.unscopables
so once that lands this will have to add that back

[rossberg, 2014-08-06 09:53:02]

https://codereview.chromium.org/384963002/diff/280001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/384963002/diff/280001/include/v8.h#newcode892
include/v8.h:892: bool ToValue(T* t) {
Can we not introduce this? Maybe intentionally is a simple struct, using this
method actually makes code more verbose and less readable.

https://codereview.chromium.org/384963002/diff/280001/src/unscopables.h
File src/unscopables.h (right):

https://codereview.chromium.org/384963002/diff/280001/src/unscopables.h#newcode5
src/unscopables.h:5: #ifndef V8_UNSCOPABLES_H_
I don't think it's worth putting this into an extra file anymore. I'd even
prefer just inlining the function into Context::Lookup directly.

https://codereview.chromium.org/384963002/diff/280001/src/unscopables.h#newco...
src/unscopables.h:23: if
(!JSReceiver::GetPropertyAttributes(it).ToValue(&attrs)) {
I think it's clearer and simpler to avoid ToValue and write this e.g. as

  Maybe<PropertyAttributes> attrs = JSReceiver::GetPropertyAttributes(it);
  DCHECK(isolate->has_pending_exception() || attrs.has_value());
  if (!attrs.has_value() || attrs.value == ABSENT) return attrs;

You then can simply return attrs below.

[arv (Not doing code reviews), 2014-08-06 15:03:45]

Remove unscopables.h. Add unscopables.js and set up the array unscopables

[arv (Not doing code reviews), 2014-08-06 15:08:29]

Thanks Andreas.

I added a src/unscopables.js that conditionally adds the Symbol.unscopables.
Would you prefer that Symbol.unscopables is always defined no matter if
--harmony-unscopables is turned on or not?

This also adds Array.prototype[Symbol.unscopables] with tests.

https://codereview.chromium.org/384963002/diff/280001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/384963002/diff/280001/include/v8.h#newcode892
include/v8.h:892: bool ToValue(T* t) {
On 2014/08/06 09:53:02, rossberg wrote:
> Can we not introduce this? Maybe intentionally is a simple struct, using this
> method actually makes code more verbose and less readable.

Reverted this.

https://codereview.chromium.org/384963002/diff/280001/src/unscopables.h
File src/unscopables.h (right):

https://codereview.chromium.org/384963002/diff/280001/src/unscopables.h#newcode5
src/unscopables.h:5: #ifndef V8_UNSCOPABLES_H_
On 2014/08/06 09:53:02, rossberg wrote:
> I don't think it's worth putting this into an extra file anymore. I'd even
> prefer just inlining the function into Context::Lookup directly.

Agreed. Now that it is only used in one place.

I moved it to contexts.cc but I decided to keep it as a function for
readability's sake.

https://codereview.chromium.org/384963002/diff/280001/src/unscopables.h#newco...
src/unscopables.h:23: if
(!JSReceiver::GetPropertyAttributes(it).ToValue(&attrs)) {
On 2014/08/06 09:53:02, rossberg wrote:
> I think it's clearer and simpler to avoid ToValue and write this e.g. as
> 
>   Maybe<PropertyAttributes> attrs = JSReceiver::GetPropertyAttributes(it);
>   DCHECK(isolate->has_pending_exception() || attrs.has_value());
>   if (!attrs.has_value() || attrs.value == ABSENT) return attrs;
> 
> You then can simply return attrs below.

Done.

[rossberg, 2014-08-06 15:13:46]

LGTM

On 2014/08/06 15:08:29, arv wrote:
> I added a src/unscopables.js that conditionally adds the Symbol.unscopables.
> Would you prefer that Symbol.unscopables is always defined no matter if
> --harmony-unscopables is turned on or not?

I'm fine either way.

[rossberg, 2014-08-06 15:15:56]

I'll land this.

[rossberg, 2014-08-06 15:50:55]

Committed patchset #18 manually as 22942 (tree was closed).