Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: Use a non-frozen object to represent Private Names #88

Merged
merged 2 commits into from Apr 18, 2018

Conversation

littledan
Copy link
Member

This patch changes PrivateName from a primitive type to a non-frozen
object. Rather than passing the get and set functions to decorators,
the PrivateName constructor is instead passed, and is no longer present
as a property of the global object.

The change is hoped to reduce implementation complexity compared to the
previous primitive type semantics, and describes a possibility which
was discussed at the March 2018 TC39 meeting.

Applications which need an unmodified version of PrivateName's methods
are recommended to make a copy of them early on before anything modifies
them, similarly to any other JS built-in.

Addresses #68

@littledan
Copy link
Member Author

cc @bmeck @erights @gsathya @akroshg

@@ -67,7 +67,7 @@ function bound(elementDescriptor) {
// Whenever a read or write is done to a field, call the render()
// method afterwards. Implement this by replacing the field with
// a getter/setter pair.
function observed({kind, key, placement, descriptor, initializer}, get, set) {
function observed({kind, key, placement, descriptor, initializer}, PrivateName) {
assert(kind == "field");
assert(placement == "own");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec text says that this must be new PrivateName (on line 74)

spec.html Outdated
<p>The following steps are taken:</p>
<emu-alg>
1. Return PrivateNameDescriptiveString(? ThisPrivateName()).
1. Let _pn_ be ? ThisPrivateName().
1. Let _desc_ be _sym_'s [[Description]] value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is “sym”?

spec.html Outdated
1. Return PrivateNameDescriptiveString(? ThisPrivateName()).
1. Let _pn_ be ? ThisPrivateName().
1. Let _desc_ be _sym_'s [[Description]] value.
1. If _desc_ is *undefined*, return _desc_ be the empty string.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return the empty string?

spec.html Outdated
<p>This property has the attributes { [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *true* }.</p>
</emu-clause>

<emu-clause id="sec-private-name-this-private-name" aoid=ThisPrivateName>
<h1>ThisPrivateName()</h1>
<emu-alg>
1. Let _p_ be the *this* value.
1. Return ToPrivateName(_p_).
1. Let _O_ be the *this* value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can abstract operations access the receiver in this manner? I feel like what I’ve typically seen is that API methods extract the receiver and pass it to abstract ops as an argument.

@rbuckton
Copy link
Collaborator

rbuckton commented Apr 7, 2018

I'm concerned about the security hazard of a non-frozen prototype. A bad actor could patch get and set to steal private names. While "monkey patching" is a nice to have, it completely breaks hard privacy. I could understand having new PrivateName return an object with own get and set methods bound to runtime-created functions, so that they cannot be intercepted, but this would remove the need for get and set on the prototype and wouldn't be monkey-patchable. I don't know if monkey-patching aligns with the hard privacy tenent.

@littledan
Copy link
Member Author

Thanks for the review, @ljharb. I've fixed the issues you raised; let me know if you see any other problems.

@rbuckton We'll have to discuss further whether this should be frozen or not frozen.

@littledan
Copy link
Member Author

OK, I'm going to merge this patch since it gets us closer to where we are agreed we want to go. Frozen vs not frozen is a smaller change that we can iterate on, compared to primitive vs object (which this patch does in a way that I don't think we'll go back on).

This patch changes PrivateName from a primitive type to a non-frozen
object. Rather than passing the `get` and `set` functions to decorators,
the PrivateName constructor is instead passed, and is no longer present
as a property of the global object.

The change is hoped to reduce implementation complexity compared to the
previous primitive type semantics, and describes a possibility which
was discussed at the March 2018 TC39 meeting.

Applications which need an unmodified version of PrivateName's methods
are recommended to make a copy of them early on before anything modifies
them, similarly to any other JS built-in.

Addresses #68
@littledan littledan merged commit 6ab21b8 into master Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants