Provide private symbols through internal APIs

Provide private symbols through internal APIs

Adds a notion of private symbols, mainly intended for internal use, especially, self-hosting of built-in types that would otherwise require new C++ classes.

On the JS side (i.e., in built-ins), private properties can be created and accessed through a set of macros:

  NEW_PRIVATE(print_name)
  HAS_PRIVATE(obj, sym)
  GET_PRIVATE(obj, sym)
  SET_PRIVATE(obj, sym, val)
  DELETE_PRIVATE(obj, sym)

In the V8 API, they are accessible via a new class Private, and respective HasPrivate/Get/Private/SetPrivate/DeletePrivate methods on calss Object.

These APIs are designed and restricted such that their implementation can later be replaced by whatever ES7+ will officially provide.

R=yangguo@chromium.org
BUG=

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

[arv (Not doing code reviews), 2013-10-28 21:58:19]

This is really great. Thanks for doing this.

Did you forget these ones?

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/array-itera...

Should there be a comment somewhere explaining that reads will read through to
the [[Prototype]]? One possible implementation of GET_PRIVATE would be to use a
side table but then the reads would not read through.

[rossberg, 2013-10-29 12:01:16]

On 2013/10/28 21:58:19, arv wrote:
> Did you forget these ones?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/array-itera...

Ah, yes, I did.

> Should there be a comment somewhere explaining that reads will read through to
> the [[Prototype]]? One possible implementation of GET_PRIVATE would be to use
a
> side table but then the reads would not read through.

Good point. I considered making them non-inheriting, but that would be costly on
the JS side.

Patch set 3 also makes Private a non-Value type, to prevent accidental uses with
the plain Get/Has/etc methods. In other words, from the API's perspective,
private symbols are entirely second-class now. Let me know if you think that's
too restrictive.

What's currently missing is the ability to install private properties on object
templates. I can add that if it's required.

[Yang, 2013-10-29 12:47:25]

We got a bunch of call sites to Symbol::name that does not check for private,
for example in objects-printer.cc, log-utils.cc, log.cc, etc. It would return
the box in case of a private (not sure whether that could happen at all).

I suggest creating a Symbol::Name() method that encapsulates the private-check
and does the correct thing to return a string, and use that instead of
Symbol::name() in places where we do not care about the distinction, and make
Symbol::name() a protected method so that only Private::name can get access to
the box.

https://codereview.chromium.org/48923002/diff/70001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/48923002/diff/70001/src/api.cc#newcode6201
src/api.cc:6201: return Utils::ToLocal(result);
I think you could unify those two New functions by having a default argument for
data like NULL or "\0".

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

https://codereview.chromium.org/48923002/diff/70001/src/objects-inl.h#newcode...
src/objects-inl.h:2703: return Box::cast(Symbol::name())->value();
could we assert in those accessors that the value is either undefined or a
string?

https://codereview.chromium.org/48923002/diff/70001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/48923002/diff/70001/src/objects.h#newcode363
src/objects.h:363: \
stray edit?

[arv (Not doing code reviews), 2013-10-29 13:20:43]

On 2013/10/29 12:01:16, rossberg wrote:

> What's currently missing is the ability to install private properties on
object
> templates. I can add that if it's required.

That seems useful.

My main concern is that we probably need to do cross realm registration of these
as well (we need that for unique symbols too so we should keep this in mind when
we do that). Today Blink bindings uses hidden properties, which are just strings
so they work in any realm/isolate. When we change these to private state we need
to coordinate between the realms/isolates.

[rossberg, 2013-10-29 15:08:19]

https://codereview.chromium.org/48923002/diff/70001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/48923002/diff/70001/src/api.cc#newcode6201
src/api.cc:6201: return Utils::ToLocal(result);
On 2013/10/29 12:47:26, Yang wrote:
> I think you could unify those two New functions by having a default argument
for
> data like NULL or "\0".

Done (also for the Symbol class).

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

https://codereview.chromium.org/48923002/diff/70001/src/objects-inl.h#newcode...
src/objects-inl.h:2703: return Box::cast(Symbol::name())->value();
On 2013/10/29 12:47:26, Yang wrote:
> could we assert in those accessors that the value is either undefined or a
> string?

Yeah, that is nasty. I decided to remove the whole Box hack and simply add a
flags field to the Symbol representation. Screw premature optimisation.

https://codereview.chromium.org/48923002/diff/70001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/48923002/diff/70001/src/objects.h#newcode363
src/objects.h:363: \
On 2013/10/29 12:47:26, Yang wrote:
> stray edit?

On purpose. :)

[rossberg, 2013-10-29 15:10:06]

On 2013/10/29 13:20:43, arv wrote:
> On 2013/10/29 12:01:16, rossberg wrote:
> 
> > What's currently missing is the ability to install private properties on
> object
> > templates. I can add that if it's required.
> 
> That seems useful.

That will require a bit of work, though. So I'll not do it in this CL.


> My main concern is that we probably need to do cross realm registration of
these
> as well (we need that for unique symbols too so we should keep this in mind
when
> we do that). Today Blink bindings uses hidden properties, which are just
strings
> so they work in any realm/isolate. When we change these to private state we
need
> to coordinate between the realms/isolates.

Yes. I don't really know what to do about that, but I'm open to ideas. :)

[Yang, 2013-11-05 13:50:54]

On 2013/10/29 15:10:06, rossberg wrote:
> On 2013/10/29 13:20:43, arv wrote:
> > On 2013/10/29 12:01:16, rossberg wrote:
> > 
> > > What's currently missing is the ability to install private properties on
> > object
> > > templates. I can add that if it's required.
> > 
> > That seems useful.
> 
> That will require a bit of work, though. So I'll not do it in this CL.
> 
> 
> > My main concern is that we probably need to do cross realm registration of
> these
> > as well (we need that for unique symbols too so we should keep this in mind
> when
> > we do that). Today Blink bindings uses hidden properties, which are just
> strings
> > so they work in any realm/isolate. When we change these to private state we
> need
> > to coordinate between the realms/isolates.
> 
> Yes. I don't really know what to do about that, but I'm open to ideas. :)

Instead of a flag field, why not a new map with the same instance type? We got a
symbol_map(), how about a private_name_map()? If the instance type is the same,
it would be treated the same way as a symbol in generated code and IsSymbol()
passes.

[Michael Starzinger, 2013-11-05 14:54:21]

Small drive-by. I like this change.

https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc#newcode4868
src/runtime.cc:4868: if (name.is_null()) return Failure::Exception();
nit: Please use RETURN_IF_EMPTY_HANDLE here.

https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc#newcode4902
src/runtime.cc:4902: if (name.is_null()) return Failure::Exception();
nit: Please use RETURN_IF_EMPTY_HANDLE here.

[rossberg, 2013-11-11 15:19:35]

On 2013/11/05 13:50:54, Yang wrote:
> Instead of a flag field, why not a new map with the same instance type? We got
a
> symbol_map(), how about a private_name_map()? If the instance type is the
same,
> it would be treated the same way as a symbol in generated code and IsSymbol()
> passes.

That's what I started out with first, actually, but it's _hugely_ more
complicated, brittle, and also less efficient, because certain type tests would
require additional comparisons. Moreover, there are only a couple of very
specific places (all having to do with reflection) where private and non-private
symbols will behave differently, so it seems wrong to factor the difference into
the instance type.

[rossberg, 2013-11-11 15:20:05]

https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc#newcode4868
src/runtime.cc:4868: if (name.is_null()) return Failure::Exception();
On 2013/11/05 14:54:21, Michael Starzinger wrote:
> nit: Please use RETURN_IF_EMPTY_HANDLE here.

Done.

https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc#newcode4902
src/runtime.cc:4902: if (name.is_null()) return Failure::Exception();
On 2013/11/05 14:54:21, Michael Starzinger wrote:
> nit: Please use RETURN_IF_EMPTY_HANDLE here.

Done.

[Yang, 2013-11-13 10:05:09]

On 2013/11/11 15:20:05, rossberg wrote:
> https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc
> File src/runtime.cc (right):
> 
>
https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc#newcode4868
> src/runtime.cc:4868: if (name.is_null()) return Failure::Exception();
> On 2013/11/05 14:54:21, Michael Starzinger wrote:
> > nit: Please use RETURN_IF_EMPTY_HANDLE here.
> 
> Done.
> 
>
https://codereview.chromium.org/48923002/diff/150001/src/runtime.cc#newcode4902
> src/runtime.cc:4902: if (name.is_null()) return Failure::Exception();
> On 2013/11/05 14:54:21, Michael Starzinger wrote:
> > nit: Please use RETURN_IF_EMPTY_HANDLE here.
> 
> Done.

LGTM.

[rossberg, 2013-11-13 10:34:21]

Committed patchset #5 manually as r17683 (presubmit successful).