* SUMMARY ES6 allows duplicate property names where ES5 (strict mode) did not: <https://people.mozilla.org/~jorendorff/es6-draft.html#sec-additions-and-changes-that-introduce-incompatibilities-with-prior-editions> > Annex E: Additions and Changes That Introduce Incompatibilities with Prior Editions > 12.2.5.1: In Edition 6, it is no longer an early error to have duplicate property names in Object Initializers. > July 18, 2014 Draft Rev 26 > Eliminated duplicate property name restrictions on object literals and class definitions > Deleted: > ObjectLiteral : { PropertyDefinitionList } > ObjectLiteral : { PropertyDefinitionList , } > > <#>It is a Syntax Error if PropertyNameList of PropertyDefinitionList contains any duplicate entries, unless one of the following conditions aretrueforeachduplicateentry: > <#>The source code corresponding to PropertyDefinitionList is not strict code and all occurrences in the list of the duplicated entry were obtained from productions of the form PropertyDefinition : PropertyName : AssignmentExpression. > <#>The duplicated entry occurs exactly twice in the list and one occurrence was obtained from a get accessor MethodDefinition and the other occurrence was obtained from a set accessor MethodDefinition. Removing duplicate detection offers us an opportunity to also fix these cases: * TESTS /* Test 1 - ensure computed property eliminates the getter, and then the setter eliminates the property */ x = 0; var o = { ["foo"]:++x, get foo() { return -2; }, ["foo"]:++x, set foo(x) { return 0; }, }; console.log(Object.getOwnPropertyDescriptor(o)); // only setter console.log(o.foo); // undefined console.log(x); // 2 /* Test 2 - ensure getter doesn't get erased when setting setter*/ x = 0; var o = { ["foo"]:++x, get foo() { return -2; }, set foo(x) { return 0; }, }; console.log(Object.getOwnPropertyDescriptor(o)); // getter and setter console.log(o.foo); // -2 console.log(x); // 1 /* Test 3 - ensure getter doesn't get erased */ x = 0; var o = { ["foo"]:++x, get foo() { return -2; }, set foo(x) { return 0; }, foo:99, }; console.log(Object.getOwnPropertyDescriptor(o)); // value console.log(o.foo); // 99 console.log(x); // 1
Created attachment 249079 [details] [PATCH] Work in Progress - Numerous issues I took the naive approach, and just "emit everything" if there is a computed property. And for getters/setters instead of emitting in pairs, just emit them individually. This ran into two issues: - emitting an individual getter/setter individually with emitPutGetterSetter wipes out any preexisting getter/setter - trying to putDirect a constant if a getter/setter is not supported (ASSERTs in JSC::PropertyDescriptor::setDescriptor) If anyone wants to move this forward, please do!
> If anyone wants to move this forward, please do! Looking into this more I think I can manage this. I'll take this and give it a shot.
Created attachment 249166 [details] [PATCH] Proposed Fix This works! A few pre-existing issues were discovered while writing tests that should be addressed separately.
Comment on attachment 249166 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=249166&action=review > Source/JavaScriptCore/runtime/JSObject.h:1342 > + if ((attributes & Accessor) != (currentAttributes & Accessor)) { I suppose this could be an xor. I wasn't sure if that would be frowned on: ((attributes ^ currentAttributes) & Accessor)
Created attachment 249167 [details] [PATCH] For Bots 1
Comment on attachment 249167 [details] [PATCH] For Bots 1 Attachment 249167 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6007228142190592 New failing tests: ietestcenter/Javascript/11.1.5_4-4-c-2.html js/mozilla/strict/11.1.5.html ietestcenter/Javascript/11.1.5_4-4-d-1.html ietestcenter/Javascript/11.1.5_4-4-b-1.html ietestcenter/Javascript/11.1.5_4-4-d-4.html ietestcenter/Javascript/11.1.5_4-4-d-3.html ietestcenter/Javascript/11.1.5_4-4-b-2.html ietestcenter/Javascript/11.1.5_4-4-c-1.html ietestcenter/Javascript/11.1.5_4-4-d-2.html
Created attachment 249169 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249167 [details] [PATCH] For Bots 1 Attachment 249167 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4900378562789376 New failing tests: ietestcenter/Javascript/11.1.5_4-4-c-2.html js/mozilla/strict/11.1.5.html ietestcenter/Javascript/11.1.5_4-4-d-1.html ietestcenter/Javascript/11.1.5_4-4-b-1.html ietestcenter/Javascript/11.1.5_4-4-d-4.html ietestcenter/Javascript/11.1.5_4-4-d-3.html ietestcenter/Javascript/11.1.5_4-4-b-2.html ietestcenter/Javascript/11.1.5_4-4-c-1.html ietestcenter/Javascript/11.1.5_4-4-d-2.html
Created attachment 249170 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249166 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=249166&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:334 > for (; p && p->m_node->m_type == PropertyNode::Constant; p = p->m_next) > emitPutConstantProperty(generator, dst, *p->m_node); Nit: This fast path can still be for Constant | Computed. Since the slow path is to handle getters/setters getting overriden by computed properties, we only care if there is a computed property once we've seen a getter/setter.
I have not looked yet, but these tests are likely failing because they expect ES5 behavior that is changing, so the should just need updated results.
Created attachment 249179 [details] [PATCH] Proposed Fix
Comment on attachment 249179 [details] [PATCH] Proposed Fix Attachment 249179 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6292978221973504 New failing tests: js/basic-computed-property-name.html
Created attachment 249183 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249179 [details] [PATCH] Proposed Fix Attachment 249179 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5300034396487680 New failing tests: js/basic-computed-property-name.html
Created attachment 249185 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
> New failing tests: > js/basic-computed-property-name.html Heh, this is interesting. It is actually a bug in the existing computed properties implementation. I'll file a separate bug to address this separately.
Created attachment 249186 [details] [PATCH] Proposed Fix This also tried to fix the other builds!
Created attachment 249188 [details] [PATCH] Proposed Fix Try to fix Windows.
This patch probably needs to be rebased at this point, but can it get a review to see if it was the correct direction? I'm willing to rebaseline it and make further changes.
Comment on attachment 249188 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=249188&action=review > LayoutTests/js/mozilla/strict/11.1.5-expected.txt:1 > -PASS Function("'use strict'; ({x:1, x:1})") threw exception of type SyntaxError. > +FAIL Function("'use strict'; ({x:1, x:1})") should throw an instance of SyntaxError I thought duplication in strict code is not allowed. Your description (with excerpt of Annex E) said: "It is a Syntax Error if PropertyNameList of PropertyDefinitionList contains any duplicate entries, unless … The source code corresponding to PropertyDefinitionList is not strict code”.
(In reply to comment #21) > Comment on attachment 249188 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249188&action=review > > > LayoutTests/js/mozilla/strict/11.1.5-expected.txt:1 > > -PASS Function("'use strict'; ({x:1, x:1})") threw exception of type SyntaxError. > > +FAIL Function("'use strict'; ({x:1, x:1})") should throw an instance of SyntaxError > > I thought duplication in strict code is not allowed. Your description (with > excerpt of Annex E) said: "It is a Syntax Error if PropertyNameList of > PropertyDefinitionList contains any duplicate entries, unless … The source > code corresponding to PropertyDefinitionList is not strict code”. You are quoting the part of the spec that was "Deleted". The top sentence says, "it is no longer an early error to have duplicate property names in Object Initializers."
(In reply to comment #22) > (In reply to comment #21) > > Comment on attachment 249188 [details] > > [PATCH] Proposed Fix > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=249188&action=review > > > > > LayoutTests/js/mozilla/strict/11.1.5-expected.txt:1 > > > -PASS Function("'use strict'; ({x:1, x:1})") threw exception of type SyntaxError. > > > +FAIL Function("'use strict'; ({x:1, x:1})") should throw an instance of SyntaxError > > > > I thought duplication in strict code is not allowed. Your description (with > > excerpt of Annex E) said: "It is a Syntax Error if PropertyNameList of > > PropertyDefinitionList contains any duplicate entries, unless … The source > > code corresponding to PropertyDefinitionList is not strict code”. > > You are quoting the part of the spec that was "Deleted". The top sentence > says, "it is no longer an early error to have duplicate property names in > Object Initializers." I see. Thanks for clarifying.
Bump!
Comment on attachment 249188 [details] [PATCH] Proposed Fix Does this patch mean that functions that initialize objects with getters and setters will no longer compile in DFG or FTL?
(In reply to comment #25) > Comment on attachment 249188 [details] > [PATCH] Proposed Fix > > Does this patch mean that functions that initialize objects with getters and > setters will no longer compile in DFG or FTL? The existing op_put_getter_setter is not compiled to the DFG or FTL, so there is no change in capabilities for object literals with getters/setters.
Comment on attachment 249188 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=249188&action=review r=me > Source/JavaScriptCore/runtime/JSObject.cpp:1253 > +void JSObject::putGetter(ExecState* exec, PropertyName propertyName, JSValue getter) > +{ > + PropertyDescriptor descriptor; > + descriptor.setGetter(getter); > + descriptor.setEnumerable(true); > + descriptor.setConfigurable(true); > + defineOwnProperty(this, exec, propertyName, descriptor, false); > +} > + > +void JSObject::putSetter(ExecState* exec, PropertyName propertyName, JSValue setter) > +{ > + PropertyDescriptor descriptor; > + descriptor.setSetter(setter); > + descriptor.setEnumerable(true); > + descriptor.setConfigurable(true); > + defineOwnProperty(this, exec, propertyName, descriptor, false); > +} Is this really better than just emitting bytecode to call defineOwnProperty?
> Is this really better than just emitting bytecode to call defineOwnProperty? Do you mean adding a single opcode with more flags that calls defineOwnProperty or something else?
Created attachment 253064 [details] [PATCH] For Landing
Comment on attachment 253064 [details] [PATCH] For Landing Clearing flags on attachment: 253064 Committed r184324: <http://trac.webkit.org/changeset/184324>