[proxies] Implement [[PreventExtensions]] and [[IsExtensible]].

[proxies] Implement [[PreventExtensions]] and [[IsExtensible]].

BUG=

Committed: https://crrev.com/a25018905df5f41e279403423eef4fc8b463ab49
Cr-Commit-Position: refs/heads/master@{#32212}

[neis, 2015-11-12 17:28:47]

Description was changed from

==========
[proxies] Implement [[PreventExtensions]] and [[IsExtensible]].

BUG=
==========

to

==========
[proxies] Implement [[PreventExtensions]] and [[IsExtensible]].

BUG=
==========

[neis, 2015-11-12 17:29:34]

Not for review yet.

[neis, 2015-11-16 22:37:46]

neis@chromium.org changed reviewers:
+ rossberg@chromium.org

[neis, 2015-11-16 22:38:37]

PTAL.

In order to write meaningful tests, we first need [[Set]] to work properly.

[neis, 2015-11-16 22:39:32]

neis@chromium.org changed reviewers:
+ mcilroy@chromium.org

[neis, 2015-11-16 22:40:23]

mcilroy: please have a look at the changes to test-bytecode-generator.cc.

[Camillo Bruni, 2015-11-17 08:57:16]

cbruni@chromium.org changed reviewers:
+ cbruni@chromium.org

[Camillo Bruni, 2015-11-17 08:57:17]

LGTM with nits (mostly missing comments).

I know that we probably need the full set of proxy traps to make real tests, but
I would nevertheless add some basic tests to see whether the fallbacks work.

Typically I just went through the spec and made sure I hit each top-level case
once. Like that I am at least sure I fully end up in all the places.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode4751
src/objects.cc:4751: Isolate* isolate = proxy->GetIsolate();
I would pass the isolate along, since we kept it in a local variable in all the
trap methods so far.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7062
src/objects.cc:7062: 
Please add a tiny header:
// ES6 9.5.?
// static

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7064
src/objects.cc:7064: ShouldThrow should_throw) {
Could you copy in the spec steps? (see the other proxy traps)
We did that to make sure that we can easily compare the steps if they change in
the future.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7069
src/objects.cc:7069: if (IsRevoked(proxy)) {
nice ;) we were too lazy so far to introduce this helper.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7095
src/objects.cc:7095: factory->preventExtensions_string()));
you could use trap_name here.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7175
src/objects.cc:7175: 
ditto: missing spec chapter comment.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7176
src/objects.cc:7176: Maybe<bool> JSProxy::IsExtensible(Handle<JSProxy> proxy) {
ditto: spec steps.

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

https://codereview.chromium.org/1441043002/diff/20001/src/objects.h#newcode9525
src/objects.h:9525: MUST_USE_RESULT static Maybe<bool>
IsExtensible(Handle<JSProxy> proxy);
can you put the ES6 section? 9.5.?

[Jakob Kummerow, 2015-11-17 12:12:35]

DBC.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode910
src/objects.cc:910: // TODO(neis): Decide on how to represent revocation.  For
now, revocation is
The spec for various internal methods keeps checking for handler/target being
null, which IIUC is the case after revocation. So how about the following
implementation:

DCHECK(proxy->handler()->IsNull() == proxy->target()->IsNull());
return proxy->handler()->IsNull();

[rossberg, 2015-11-17 13:29:09]

lgtm

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7171
src/objects.cc:7171: DCHECK(object->IsJSObject());
Nit: this check is redundant, since it's implied by the cast below.

[neis, 2015-11-17 18:16:21]

neis@chromium.org changed reviewers:
- mcilroy@chromium.org

[neis, 2015-11-17 23:19:03]

I added tests and addressed some comments.  Please have another look.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7062
src/objects.cc:7062: 
On 2015/11/17 08:57:16, cbruni wrote:
> Please add a tiny header:
> // ES6 9.5.?
> // static

Added spec reference to the header file.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7064
src/objects.cc:7064: ShouldThrow should_throw) {
On 2015/11/17 08:57:17, cbruni wrote:
> Could you copy in the spec steps? (see the other proxy traps)
> We did that to make sure that we can easily compare the steps if they change
in
> the future.

I don't know what others think about this, but for me copying the spec into the
code makes it harder to read the code because because it's just so cluttered.  I
prefer to have the spec in a separate window.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7095
src/objects.cc:7095: factory->preventExtensions_string()));
On 2015/11/17 08:57:16, cbruni wrote:
> you could use trap_name here.

Done.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7171
src/objects.cc:7171: DCHECK(object->IsJSObject());
On 2015/11/17 13:29:09, rossberg wrote:
> Nit: this check is redundant, since it's implied by the cast below.

Done.

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7175
src/objects.cc:7175: 
On 2015/11/17 08:57:16, cbruni wrote:
> ditto: missing spec chapter comment.

Done.

[Jakob Kummerow, 2015-11-18 13:58:27]

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7064
src/objects.cc:7064: ShouldThrow should_throw) {
On 2015/11/17 23:19:03, neis wrote:
> On 2015/11/17 08:57:17, cbruni wrote:
> > Could you copy in the spec steps? (see the other proxy traps)
> > We did that to make sure that we can easily compare the steps if they change
> in
> > the future.
> 
> I don't know what others think about this, but for me copying the spec into
the
> code makes it harder to read the code because because it's just so cluttered. 
I
> prefer to have the spec in a separate window.

I'm strongly in favor of adding the spec steps as comments, for the following
reasons:

(1) Doing so makes it easier to verify that the code is correct, 
  (a) while writing it [you may disagree],
  (b) when reviewing it,
  (c) when chasing bugs months later.

(2) It documents why the code does what it does, in particular distinguishes
implementation details that can be refactored at will vs. behavior that's
mandated by the spec. Side effects:
  (a) forces us to be strict about following the spec step by step; any
shortcuts or abstractions or convenience helpers are easy to find and their
validity is easy to reason about
  (b) helps prevent growth of a big mess of similarly named functions that
implement various parts or variants of a spec'ed algorithm (which we've seen
happening way too often)

(3) As the spec evolves, it documents clearly what semantics we've implemented.
  (a) This helps figure out what's going on when bugs are filed in the future
claiming spec violations.
  (b) This also makes it easier to proactively find code that needs updating
after spec changes (by comparing the comments with the new spec).

So yeah, please add those comments :-)

[rossberg, 2015-11-18 14:31:22]

On 2015/11/18 13:58:27, Jakob wrote:
> https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/1441043002/diff/20001/src/objects.cc#newcode7064
> src/objects.cc:7064: ShouldThrow should_throw) {
> On 2015/11/17 23:19:03, neis wrote:
> > On 2015/11/17 08:57:17, cbruni wrote:
> > > Could you copy in the spec steps? (see the other proxy traps)
> > > We did that to make sure that we can easily compare the steps if they
change
> > in
> > > the future.
> > 
> > I don't know what others think about this, but for me copying the spec into
> the
> > code makes it harder to read the code because because it's just so
cluttered. 
> I
> > prefer to have the spec in a separate window.
> 
> I'm strongly in favor of adding the spec steps as comments, for the following
> reasons:
> 
> (1) Doing so makes it easier to verify that the code is correct, 
>   (a) while writing it [you may disagree],
>   (b) when reviewing it,
>   (c) when chasing bugs months later.
> 
> (2) It documents why the code does what it does, in particular distinguishes
> implementation details that can be refactored at will vs. behavior that's
> mandated by the spec. Side effects:
>   (a) forces us to be strict about following the spec step by step; any
> shortcuts or abstractions or convenience helpers are easy to find and their
> validity is easy to reason about
>   (b) helps prevent growth of a big mess of similarly named functions that
> implement various parts or variants of a spec'ed algorithm (which we've seen
> happening way too often)
> 
> (3) As the spec evolves, it documents clearly what semantics we've
implemented.
>   (a) This helps figure out what's going on when bugs are filed in the future
> claiming spec violations.
>   (b) This also makes it easier to proactively find code that needs updating
> after spec changes (by comparing the comments with the new spec).
> 
> So yeah, please add those comments :-)

The problem is that such cross refs will get out of date quickly, and at that
point might be more harmful than helpful. Remember that we will have a new spec
every year.

[rossberg, 2015-11-24 09:33:46]

Still LGTM.

I don't see the necessity to comment every trivial micro step. May not match
what we are doing, is bound to get out of date quickly and at that point might
potentially cause more harm than benefit. It is worth commenting functions and
larger parts of non-trivial spec function implementations, though.

[neis, 2015-11-24 13:45:57]

The CQ bit was checked by neis@chromium.org

[neis, 2015-11-24 13:45:57]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1441043002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-11-24 13:46:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441043002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441043002/60001

[commit-bot: I haz the power, 2015-11-24 14:16:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-24 14:16:32]

Patchset 4 (id:??) landed as
https://crrev.com/a25018905df5f41e279403423eef4fc8b463ab49
Cr-Commit-Position: refs/heads/master@{#32212}