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

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index fefdc5be48dde724d2eeff54368985392a6dc7b3..9a6e48ebaf247ad83405d0c2f7dbd48d77480df1 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -906,6 +906,15 @@ MaybeHandle<Object> JSProxy::GetPrototype(Handle<JSProxy> proxy) {
 }
 
 
+bool JSProxy::IsRevoked(Handle<JSProxy> proxy) {
+  // TODO(neis): Decide on how to represent revocation.  For now, revocation is

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

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

+  // unsupported.
+  DCHECK(proxy->target()->IsJSReceiver());
+  DCHECK(proxy->handler()->IsJSReceiver());
+  return false;
+}
+
+
 MaybeHandle<Object> JSProxy::GetPropertyWithHandler(Handle<JSProxy> proxy,
                                                     Handle<Object> receiver,
                                                     Handle<Name> name) {
@@ -4736,6 +4745,15 @@ Maybe<PropertyAttributes> JSProxy::GetPropertyAttributesWithHandler(
 }
 
 
+MaybeHandle<Object> JSProxy::GetTrap(Handle<JSProxy> proxy,
+                                     Handle<String> trap) {
+  DCHECK(!IsRevoked(proxy));
+  Isolate* isolate = proxy->GetIsolate();

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

I would pass the isolate along, since we kept it in a local variable in all the
trap methods so far.

+  Handle<JSReceiver> handler(JSReceiver::cast(proxy->handler()), isolate);
+  return Object::GetMethod(handler, trap);
+}
+
+
 MaybeHandle<Object> JSProxy::CallTrap(Handle<JSProxy> proxy,
                                       const char* name,
                                       Handle<Object> derived,
@@ -4743,13 +4761,9 @@ MaybeHandle<Object> JSProxy::CallTrap(Handle<JSProxy> proxy,
                                       Handle<Object> argv[]) {
   Isolate* isolate = proxy->GetIsolate();
   Handle<Object> handler(proxy->handler(), isolate);
-
   Handle<String> trap_name = isolate->factory()->InternalizeUtf8String(name);
   Handle<Object> trap;
-  ASSIGN_RETURN_ON_EXCEPTION(
-      isolate, trap,
-      Object::GetPropertyOrElement(handler, trap_name),
-      Object);
+  ASSIGN_RETURN_ON_EXCEPTION(isolate, trap, GetTrap(proxy, trap_name), Object);
 
   if (trap->IsUndefined()) {
     if (derived.is_null()) {
@@ -7036,13 +7050,63 @@ bool JSObject::ReferencesObject(Object* obj) {
 
 Maybe<bool> JSReceiver::PreventExtensions(Handle<JSReceiver> object,
                                           ShouldThrow should_throw) {
-  if (!object->IsJSObject()) return Just(false);
-  // TODO(neis): Deal with proxies.
+  if (object->IsJSProxy()) {
+    return JSProxy::PreventExtensions(Handle<JSProxy>::cast(object),
+                                      should_throw);
+  }
+  DCHECK(object->IsJSObject());
   return JSObject::PreventExtensions(Handle<JSObject>::cast(object),
                                      should_throw);
 }
 
 

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

Please add a tiny header:
// ES6 9.5.?
// static

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

Added spec reference to the header file.

+Maybe<bool> JSProxy::PreventExtensions(Handle<JSProxy> proxy,
+                                       ShouldThrow should_throw) {

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

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.

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

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.

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

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 :-)

+  Isolate* isolate = proxy->GetIsolate();
+  Factory* factory = isolate->factory();
+  Handle<String> trap_name = factory->preventExtensions_string();
+
+  if (IsRevoked(proxy)) {

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

nice ;) we were too lazy so far to introduce this helper.

+    isolate->Throw(
+        *factory->NewTypeError(MessageTemplate::kProxyRevoked, trap_name));
+    return Nothing<bool>();
+  }
+
+  Handle<JSReceiver> target(JSReceiver::cast(proxy->target()), isolate);
+  Handle<JSReceiver> handler(JSReceiver::cast(proxy->handler()), isolate);
+
+  Handle<Object> trap;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, trap, GetTrap(proxy, trap_name),
+                                   Nothing<bool>());
+  if (trap->IsUndefined()) {
+    return JSReceiver::PreventExtensions(target, should_throw);
+  }
+
+  Handle<Object> trap_result;
+  Handle<Object> args[] = {target};
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, trap_result,
+      Execution::Call(isolate, trap, handler, arraysize(args), args),
+      Nothing<bool>());
+  if (!trap_result->BooleanValue()) {
+    RETURN_FAILURE(isolate, should_throw,
+                   NewTypeError(MessageTemplate::kProxyHandlerReturned, handler,
+                                factory->false_string(),
+                                factory->preventExtensions_string()));

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

you could use trap_name here.

[neis, 2015/11/17 23:19:02]

Done.

+  }
+
+  // Enforce the invariant.
+  Maybe<bool> target_result = JSReceiver::IsExtensible(target);
+  MAYBE_RETURN(target_result, Nothing<bool>());
+  if (target_result.FromJust()) {
+    isolate->Throw(*factory->NewTypeError(
+        MessageTemplate::kProxyPreventExtensionsViolatesInvariant));
+    return Nothing<bool>();
+  }
+  return Just(true);
+}
+
+
 Maybe<bool> JSObject::PreventExtensions(Handle<JSObject> object,
                                         ShouldThrow should_throw) {
   Isolate* isolate = object->GetIsolate();
@@ -7100,16 +7164,55 @@ Maybe<bool> JSObject::PreventExtensions(Handle<JSObject> object,
 }
 
 
-// static
 Maybe<bool> JSReceiver::IsExtensible(Handle<JSReceiver> object) {
   if (object->IsJSProxy()) {
-    // TODO(neis,cbruni): Redirect to the trap on JSProxy.
-    return Just(true);
+    return JSProxy::IsExtensible(Handle<JSProxy>::cast(object));
   }
+  DCHECK(object->IsJSObject());

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

Nit: this check is redundant, since it's implied by the cast below.

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

Done.

   return Just(JSObject::IsExtensible(Handle<JSObject>::cast(object)));
 }
 
 

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

ditto: missing spec chapter comment.

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

Done.

+Maybe<bool> JSProxy::IsExtensible(Handle<JSProxy> proxy) {

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

ditto: spec steps.

+  Isolate* isolate = proxy->GetIsolate();
+  Factory* factory = isolate->factory();
+  Handle<String> trap_name = factory->isExtensible_string();
+
+  if (IsRevoked(proxy)) {
+    isolate->Throw(
+        *factory->NewTypeError(MessageTemplate::kProxyRevoked, trap_name));
+    return Nothing<bool>();
+  }
+
+  Handle<JSReceiver> target(JSReceiver::cast(proxy->target()), isolate);
+  Handle<JSReceiver> handler(JSReceiver::cast(proxy->handler()), isolate);
+
+  Handle<Object> trap;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, trap, GetTrap(proxy, trap_name),
+                                   Nothing<bool>());
+  if (trap->IsUndefined()) {
+    return JSReceiver::IsExtensible(target);
+  }
+
+  Handle<Object> trap_result;
+  Handle<Object> args[] = {target};
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, trap_result,
+      Execution::Call(isolate, trap, handler, arraysize(args), args),
+      Nothing<bool>());
+
+  // Enforce the invariant.
+  Maybe<bool> target_result = JSReceiver::IsExtensible(target);
+  MAYBE_RETURN(target_result, Nothing<bool>());
+  if (target_result.FromJust() != trap_result->BooleanValue()) {
+    isolate->Throw(*factory->NewTypeError(
+        MessageTemplate::kProxyIsExtensibleViolatesInvariant));
+    return Nothing<bool>();
+  }
+  return target_result;
+}
+
+
 bool JSObject::IsExtensible(Handle<JSObject> object) {
   Isolate* isolate = object->GetIsolate();
   if (object->IsAccessCheckNeeded() &&