Normative: Throw TypeError if proxy [[OwnPropertyKeys]] returns duplicate entries #833
Conversation
bterlson
added
the
needs consensus
| @@ -16564,7 +16572,7 @@ | |||
| for (const key of Reflect.ownKeys(obj)) { | |||
| if (typeof key === "symbol") continue; | |||
| const desc = Reflect.getOwnPropertyDescriptor(obj, key); | |||
| if (desc && !visited.has(key)) { | |||
| if (desc) { | |||
There was a problem hiding this comment.
I think this polyfill code might need more tweaking - does it still need visited at all?
There was a problem hiding this comment.
We still need to track when the same property key is also present in the prototype chain.
|
Assuming this can only happen for proxy objects, this should be no problem from HTML's perspective. |
|
Does anyone remember why we call [[GetOwnProperty]] for every property key instead of throwing an exception when we detected the first invariant violation? Was this added just to make sure the possible side-effects of [[GetOwnProperty]] are executed? |
|
@annevk just to make double-sure, you mean "duplicate keys" can only happen with proxies, or making duplicate keys a type error is only for proxies? just want to make sure HTML doesn't depend on duplicate keys anywhere... |
|
@bterlson the former. I think we already made sure that proxy-using objects never returned duplicate keys, though maybe now this change is made we should start enforcing that in IDL too more clearly. |
leobalter
added
the
needs test262 tests
|
We need volunteers to update the current tests on test262. I'm willing to support w/ reviews |
|
What if the text said "If trapResult contains any entries a and b for which SameValue(a, b) is true, throw a TypeError". That's still one line, but at least refers to the right spec machinery. |
|
Removing needs-consensus as we got consensus in March. Still need tests though I think. |
bterlson
removed
the
needs consensus
…ies, throw a TypeError Ref: tc39/ecma262#833
…ies, throw a TypeError Ref: tc39/ecma262#833
leobalter
removed
the
needs test262 tests
|
Tests are up and landed into Test262 |
|
YES! |
[JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys https://bugs.webkit.org/show_bug.cgi?id=185211 Reviewed by Saam Barati. This is for the normative spec change in tc39/ecma262#833 This changes several assertions to expect a TypeError to be thrown (in some cases, changing thee expected message). * es6/Proxy_ownKeys_duplicates.js: (handler): (shouldThrow): (test): * stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js: (shouldThrow): * stress/proxy-own-keys.js: (i.catch): (assert): LayoutTests/imported/w3c: [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys https://bugs.webkit.org/show_bug.cgi?id=185211 Reviewed by Saam Barati. This is for the normative spec change in tc39/ecma262#833 Change some test expectations which were previously expected to fail. * web-platform-tests/fetch/api/headers/headers-record-expected.txt: Source/JavaScriptCore: [JSC] throw if ownKeys Proxy trap result contains duplicate keys https://bugs.webkit.org/show_bug.cgi?id=185211 Reviewed by Saam Barati. Implements the normative spec change in tc39/ecma262#833 This involves tracking duplicate keys returned from the ownKeys trap in yet another HashTable, and may incur a minor performance penalty in some cases. This is not expected to significantly affect web performance. * runtime/ProxyObject.cpp: (JSC::ProxyObject::performGetOwnPropertyNames): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@243933 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Proxy [[OwnPropertyKeys]] now throws if the trap returns an array with duplicate entries and reverts a few of the changes made to handle duplicate property keys in a few places. See also discussion in #461. I believe this is needs-consensus, though I haven't heard arguments against this.
/cc @anba