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: Remove proxy OwnPropertyKeys error with duplicate keys #594

Merged
merged 1 commit into from Jun 3, 2016

Conversation

Projects
None yet
6 participants
@bterlson
Member

bterlson commented Jun 2, 2016

Previously, proxy OwnPropertyKeys returning duplicate keys would cause an error when the target object was non-extensible. The error does not seem deliberate but rather a result of not considering the possibility of duplicate keys. This PR handles that possibility and removes the error.

I think this is just a bug fix but if anyone thinks this needs consensus let me know. Also note that this is a breaking change for spidermonkey and chakra. V8 already implements these semantics. cc @efaust.

Normative: Remove proxy OwnPropertyKeys error with duplicate keys
Previously, proxy OwnPropertyKeys returning duplicate keys would
cause an error when the target object was non-extensible. The error
does not seem deliberate but rather a result of not considering the
possibility of duplicate keys. This PR handles that possibility and
removes the error.
@bterlson

This comment has been minimized.

Member

bterlson commented Jun 3, 2016

Also cc @msaboff.

@efaust

This comment has been minimized.

efaust commented Jun 3, 2016

No objections, your honor.

@littledan

This comment has been minimized.

Member

littledan commented Jun 3, 2016

Looks good to me; this is what we do now if I understand correctly.

@saambarati

This comment has been minimized.

saambarati commented Jun 3, 2016

@bterlson We currently throw an exception, but we can fix that.

@bterlson

This comment has been minimized.

Member

bterlson commented Jun 3, 2016

Alright, thanks! Since all the implementers that currently throw are on board with this change I'll go ahead and accept this as a bugfix. Thanks!

@bterlson bterlson merged commit 6d0fc45 into tc39:master Jun 3, 2016

@saambarati

This comment has been minimized.

saambarati commented Jun 3, 2016

@bterlson
The algorithm you wrote seems to have odd semantics when targetConfigurableKeys or targetNonConfigurableKeys itself has duplicate properties. I think this will always cause an exception in either loop. Is this intended? I haven't thought too hard about it, but I think it's possible for those lists to have duplicates.

@bterlson

This comment has been minimized.

Member

bterlson commented Jun 3, 2016

I am currently thinking hard about this as well. Starting by testing everyone's implementation (except JSC, which I don't have running on windows :()

@bterlson

This comment has been minimized.

Member

bterlson commented Jun 3, 2016

v8, sm, and Chakra all throw in this case per spec:

var obj = {};
Object.defineProperty(obj, 'a', { value: 42 });

var p1 = new Proxy(obj, {
  ownKeys() {
    print('getting p1 keys');
    return ["a", "a"]
  }
});

var p2 = new Proxy(p1, {
  ownKeys() {
    print('getting p2 keys');
    return ["a"]
  },
  getOwnPropertyDescriptor(t, p) {
    print('getting own property ' + p);
    return Reflect.getOwnPropertyDescriptor(t, p);
  }
});

print(Reflect.ownKeys(p2));

Removing the duplicate key from the p1 proxy doesn't result in an error. I think this is also a bug that should be fixed. Agree @littledan, @saambarati, @efaust, @evilpie?

@bterlson

This comment has been minimized.

Member

bterlson commented Jun 3, 2016

There are probably a bunch of possible fixes, but possibly easiest is to keep a list of seen keys in the step 14 loop and skip any seen keys. This will make sure that we only ever get the descriptor for a duplicated property once.

@saambarati

This comment has been minimized.

saambarati commented Jun 3, 2016

@bterlson Why not just only add things to every list if it's not already in the list?

@bterlson

This comment has been minimized.

Member

bterlson commented Jun 3, 2016

IIUC, then we'd do [[GetOwnProperty]] N times for N-duplicated keys, which seems possibly bad (you could get very strange behavior if the proxy decided that the key was configurable the first time and non-configurable the next time). This might be fine? But not processing the key at all seems easier.

@saambarati

This comment has been minimized.

saambarati commented Jun 3, 2016

@bterlson I wasn't suggesting that. I'm suggesting, for example, to change this line:
"Append key as an element of targetConfigurableKeys."
to
"Append key as an element of targetConfigurableKeys if key is not already in targetConfigurableKeys".

Also, thinking about this more, there are some other weird semantics. I think the intersection between targetConfigurableKeys and targetNonconfigurableKeys may be non empty. That seems weird. I wish we enforced them having an empty intersection. I'm not 100% sure if Proxy's would allow you to fake this, but I suspect it would and then one of those loops is guaranteed to throw an exception.

@saambarati

This comment has been minimized.

saambarati commented Jun 3, 2016

@bterlson To clarify on my first point, I don't think that'd change how many times [GetOwnProperty] is called. targetKeys will still be a list with maybe repeating elements.

@saambarati

This comment has been minimized.

saambarati commented Jun 3, 2016

@bterlson
Sorry, I just must have skipped over your comment about step 14. I see it now. Sorry for duplicating your proposal. I agree that this is probably the easiest thing to do, and it is what I was suggesting too.

kisg pushed a commit to paul99/webkit-mips that referenced this pull request Jun 4, 2016

sbarati@apple.com
Proxy.ownKeys should no longer throw an exception when duplicate keys…
… are returned and the target is non-extensible

https://bugs.webkit.org/show_bug.cgi?id=158350
<rdar://problem/26626211>

Reviewed by Michael Saboff.

The spec was recently changes in Proxy [[OwnPropertyKeys]]
to allow for duplicate property names under certain circumstances.
This patch fixes our implementation to match the spec.
See: tc39/ecma262#594

* runtime/ProxyObject.cpp:
(JSC::ProxyObject::performGetOwnPropertyNames):
* tests/stress/proxy-own-keys.js:
(i.catch):
(ownKeys):
(assert):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@201672 268f45cc-cd09-0410-ab3c-d52691b4dbfc

leirocks added a commit to leirocks/ChakraCore that referenced this pull request Jun 6, 2016

@raulsebastianmihaila

This comment has been minimized.

raulsebastianmihaila commented Jun 25, 2016

@bterlson It seems to me that the changes from this PR contradict what has been discussed in #461.

@bterlson

This comment has been minimized.

Member

bterlson commented Jun 28, 2016

@raulsebastianmihaila Thanks for pointing that out! I can whip up a PR removing duplicate keys (unless someone else is gung ho about it).

leirocks added a commit to leirocks/ChakraCore that referenced this pull request Aug 25, 2016

@anba

This comment has been minimized.

Contributor

anba commented Feb 28, 2017

Thanks for pointing that out! I can whip up a PR removing duplicate keys (unless someone else is gung ho about it).

@bterlson Ping?

@bterlson

This comment has been minimized.

Member

bterlson commented Mar 1, 2017

@anba gah I dropped this, thanks for pinging me. I have a start but it needs rebasing. Will work on this tomorrow.

@bterlson

This comment has been minimized.

Member

bterlson commented Mar 1, 2017

@anba I actually lied to you, sorry to say; I'm in an all-day meeting today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment