Bug 142895 – ES6: Allow duplicate property names

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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

[Joseph Pecoraro]

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!

[Joseph Pecoraro]

> 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.

[Joseph Pecoraro]

Created attachment 249166 [details]
[PATCH] Proposed Fix

This works! A few pre-existing issues were discovered while writing tests that should be addressed separately.

[Joseph Pecoraro]

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)

[Joseph Pecoraro]

Created attachment 249167 [details]
[PATCH] For Bots 1

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 249179 [details]
[PATCH] Proposed Fix

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

> 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.

[Joseph Pecoraro]

Created attachment 249186 [details]
[PATCH] Proposed Fix

This also tried to fix the other builds!

[Joseph Pecoraro]

Created attachment 249188 [details]
[PATCH] Proposed Fix

Try to fix Windows.

[Joseph Pecoraro]

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.

[Mark Lam]

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”.

[Joseph Pecoraro]

(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."

[Mark Lam]

(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.

[Joseph Pecoraro]

Bump!

[Geoffrey Garen]

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?

[Joseph Pecoraro]

(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.

[Geoffrey Garen]

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?

[Joseph Pecoraro]

> 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?

[Joseph Pecoraro]

Created attachment 253064 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 253064 [details]
[PATCH] For Landing

Clearing flags on attachment: 253064

Committed r184324: <http://trac.webkit.org/changeset/184324>