Changeset 289166 in webkit


Ignore:
Timestamp:
Feb 6, 2022 4:34:45 AM (5 months ago)
Author:
commit-queue@webkit.org
Message:

Object literal doesn't properly resolve name clash between an accessor and a constant property
​https://bugs.webkit.org/show_bug.cgi?id=220574

Patch by Alexey Shvayka <​ashvayka@apple.com> on 2022-02-06
Reviewed by Yusuke Suzuki.

JSTests:

  • stress/class-static-accessor-name-clash-with-field.js: Added.
  • stress/object-literal-accessor-name-clash-with-constant.js: Added.

Source/JavaScriptCore:

The spec [1] calls DefineOwnProperty? for every property node, whether it's a
getter, a setter, or a value. JSC attempts to reduce emitted bytecodes by setting
up a getter and a setter at once.

However, there is a slower path that exactly matches the spec, which was called only
if a spread syntax or a computed property was encountered. With this patch, the slower
path is also taken in case of a constant property (including a shorthand) with the
same name as an accessor.

That causes an incomplete accessor descriptor to correctly overwrite the existing
data one, which aligns JSC with V8 and SpiderMonkey.

This bug doesn't exist for static class fields and accessors because initialization
of class fields is deferred [2] and they always overwrite eponymous static methods /
accessors, no matter the order in source code. No reproduction for private elements either.

[1]: ​https://tc39.es/ecma262/#sec-runtime-semantics-methoddefinitionevaluation (step 11 of "get", step 10 of "set")
[2]: ​https://tc39.es/ecma262/#sec-runtime-semantics-classdefinitionevaluation (step 31.a)

  • bytecompiler/NodesCodegen.cpp:

(JSC::PropertyListNode::emitBytecode):

LayoutTests:

Adjusted test now passes on V8 and SpiderMonkey as well.

  • js/class-syntax-method-names-expected.txt:
  • js/script-tests/class-syntax-method-names.js:
Location:
trunk
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r289164 r289166  
     12022-02-06  Alexey Shvayka  <ashvayka@apple.com>
     2
     3        Object literal doesn't properly resolve name clash between an accessor and a constant property
     4        https://bugs.webkit.org/show_bug.cgi?id=220574
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        * stress/class-static-accessor-name-clash-with-field.js: Added.
     9        * stress/object-literal-accessor-name-clash-with-constant.js: Added.
     10
    1112022-02-05  Alexey Shvayka  <ashvayka@apple.com>
    212

  • trunk/LayoutTests/ChangeLog

    r289165 r289166  
     12022-02-06  Alexey Shvayka  <ashvayka@apple.com>
     2
     3        Object literal doesn't properly resolve name clash between an accessor and a constant property
     4        https://bugs.webkit.org/show_bug.cgi?id=220574
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        Adjusted test now passes on V8 and SpiderMonkey as well.
     9
     10        * js/class-syntax-method-names-expected.txt:
     11        * js/script-tests/class-syntax-method-names.js:
     12
    1132022-02-05  Chris Dumez  <cdumez@apple.com>
    214

  • trunk/LayoutTests/js/class-syntax-method-names-expected.txt

    r259676 r289166  
    8888PASS setterValue = undefined; class A { set a(x) { setterValue = x } a() { return 425 } }; (new A).a() is 425
    8989PASS setterValue = undefined; class A { get foo() { return 426 } set foo(x) { setterValue = x; } }; a = new A; a.foo = a.foo; setterValue is 426
    90 PASS class A { get foo() { } foo() { } set foo(x) { } }; valueTypes((new A).__proto__, 'foo') is ['value']
    91 PASS class A { set foo(x) { } foo() { } get foo() { } }; valueTypes((new A).__proto__, 'foo') is ['value']
     90PASS class A { get foo() { } foo() { } set foo(x) { } }; valueTypes((new A).__proto__, 'foo') is ['get', 'set']
     91PASS class A { set foo(x) { } foo() { } get foo() { } }; valueTypes((new A).__proto__, 'foo') is ['get', 'set']
    9292PASS class A { foo() { } get foo() { } set foo(x) { } }; valueTypes((new A).__proto__, 'foo') is ['get', 'set']
    9393PASS class A { foo() { } set foo(x) { } get foo() { } }; valueTypes((new A).__proto__, 'foo') is ['get', 'set']
    … …  
    109109PASS setterValue = undefined; class A { static set a(x) { setterValue = x } static a() { return 525 } }; A.a() is 525
    110110PASS setterValue = undefined; class A { static get foo() { return 526 } static set foo(x) { setterValue = x; } }; A.foo = A.foo; setterValue is 526
    111 PASS class A { static get foo() { } static foo() { } static set foo(x) { } }; valueTypes(A, 'foo') is ['value']
    112 PASS class A { static set foo(x) { } static foo() { } static get foo() { } }; valueTypes(A, 'foo') is ['value']
     111PASS class A { static get foo() { } static foo() { } static set foo(x) { } }; valueTypes(A, 'foo') is ['get', 'set']
     112PASS class A { static set foo(x) { } static foo() { } static get foo() { } }; valueTypes(A, 'foo') is ['get', 'set']
    113113PASS class A { static foo() { } static get foo() { } static set foo(x) { } }; valueTypes(A, 'foo') is ['get', 'set']
    114114PASS class A { static foo() { } static set foo(x) { } static get foo() { } }; valueTypes(A, 'foo') is ['get', 'set']

  • trunk/LayoutTests/js/script-tests/class-syntax-method-names.js

    r259676 r289166  
    9696    return ['value', 'get', 'set'].filter(function (name) { return name in descriptor; });
    9797}
    98 shouldBe("class A { get foo() { } foo() { } set foo(x) { } }; valueTypes((new A).__proto__, 'foo')", "['value']");
    99 shouldBe("class A { set foo(x) { } foo() { } get foo() { } }; valueTypes((new A).__proto__, 'foo')", "['value']");
     98shouldBe("class A { get foo() { } foo() { } set foo(x) { } }; valueTypes((new A).__proto__, 'foo')", "['get', 'set']");
     99shouldBe("class A { set foo(x) { } foo() { } get foo() { } }; valueTypes((new A).__proto__, 'foo')", "['get', 'set']");
    100100shouldBe("class A { foo() { } get foo() { } set foo(x) { } }; valueTypes((new A).__proto__, 'foo')", "['get', 'set']");
    101101shouldBe("class A { foo() { } set foo(x) { } get foo() { } }; valueTypes((new A).__proto__, 'foo')", "['get', 'set']");
    … …  
    118118shouldBe("setterValue = undefined; class A { static set a(x) { setterValue = x } static a() { return 525 } }; A.a()", "525");
    119119shouldBe("setterValue = undefined; class A { static get foo() { return 526 } static set foo(x) { setterValue = x; } }; A.foo = A.foo; setterValue", "526");
    120 shouldBe("class A { static get foo() { } static foo() { } static set foo(x) { } }; valueTypes(A, 'foo')", "['value']");
    121 shouldBe("class A { static set foo(x) { } static foo() { } static get foo() { } }; valueTypes(A, 'foo')", "['value']");
     120shouldBe("class A { static get foo() { } static foo() { } static set foo(x) { } }; valueTypes(A, 'foo')", "['get', 'set']");
     121shouldBe("class A { static set foo(x) { } static foo() { } static get foo() { } }; valueTypes(A, 'foo')", "['get', 'set']");
    122122shouldBe("class A { static foo() { } static get foo() { } static set foo(x) { } }; valueTypes(A, 'foo')", "['get', 'set']");
    123123shouldBe("class A { static foo() { } static set foo(x) { } static get foo() { } }; valueTypes(A, 'foo')", "['get', 'set']");

  • trunk/Source/JavaScriptCore/ChangeLog

    r289164 r289166  
     12022-02-06  Alexey Shvayka  <ashvayka@apple.com>
     2
     3        Object literal doesn't properly resolve name clash between an accessor and a constant property
     4        https://bugs.webkit.org/show_bug.cgi?id=220574
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        The spec [1] calls [[DefineOwnProperty]] for every property node, whether it's a
     9        getter, a setter, or a value. JSC attempts to reduce emitted bytecodes by setting
     10        up a getter and a setter at once.
     11
     12        However, there is a slower path that exactly matches the spec, which was called only
     13        if a spread syntax or a computed property was encountered. With this patch, the slower
     14        path is also taken in case of a constant property (including a shorthand) with the
     15        same name as an accessor.
     16
     17        That causes an incomplete accessor descriptor to correctly overwrite the existing
     18        data one, which aligns JSC with V8 and SpiderMonkey.
     19
     20        This bug doesn't exist for static class fields and accessors because initialization
     21        of class fields is deferred [2] and they always overwrite eponymous static methods /
     22        accessors, no matter the order in source code. No reproduction for private elements either.
     23
     24        [1]: https://tc39.es/ecma262/#sec-runtime-semantics-methoddefinitionevaluation (step 11 of "get", step 10 of "set")
     25        [2]: https://tc39.es/ecma262/#sec-runtime-semantics-classdefinitionevaluation (step 31.a)
     26
     27        * bytecompiler/NodesCodegen.cpp:
     28        (JSC::PropertyListNode::emitBytecode):
     29
    1302022-02-05  Alexey Shvayka  <ashvayka@apple.com>
    231

  • trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

    r288541 r289166  
    662662    if (p) {
    663663        // Build a list of getter/setter pairs to try to put them at the same time. If we encounter
    664         // a computed property or a spread, just emit everything as that may override previous values.
     664        // a constant property by the same name as accessor or a computed property or a spread,
     665        // just emit everything as that may override previous values.
    665666        bool canOverrideProperties = false;
    666667
    … …  
    676677            }
    677678
    678             if (node->m_type & PropertyNode::Constant)
     679            GetterSetterMap& map = node->isStaticClassProperty() ? staticMap : instanceMap;
     680            if (node->m_type & PropertyNode::Constant) {
     681                if (map.contains(node->name()->impl())) {
     682                    canOverrideProperties = true;
     683                    break;
     684                }
    679685                continue;
     686            }
    680687
    681688            // Duplicates are possible.
    682689            GetterSetterPair pair(node, static_cast<PropertyNode*>(nullptr));
    683             GetterSetterMap& map = node->isStaticClassProperty() ? staticMap : instanceMap;
    684690            GetterSetterMap::AddResult result = map.add(node->name()->impl(), pair);
    685691            auto& resultPair = result.iterator->value;
Note: See TracChangeset for help on using the changeset viewer.