Bug 170897 – test262: test262/test/language/computed-property-names/class/static/getter-prototype.js

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

test262/test/language/computed-property-names/class/static/getter-prototype.js

Test:

    class X { static get ["prototype"]() {} }

Expected:
TypeError: Cannot declare a static method named 'prototype'

Actual:
No error.

Notes:
- Chrome: TypeError: Classes may not have static property named prototype
- Firefox: TypeError: can't redefine non-configurable property "prototype"

Spec:

https://tc39.github.io/ecma262/#sec-class-definitions-static-semantics-early-errors
14.5.1 Static Semantics: Early Errors

>   ClassElement : MethodDefinition
>  
>     • It is a Syntax Error if PropName of MethodDefinition is not "constructor" and HasDirectSuper of MethodDefinition is true.
>     • It is a Syntax Error if PropName of MethodDefinition is "constructor" and SpecialMethod of MethodDefinition is true.
>  
>   ClassElement:staticMethodDefinition
>  
>     • It is a Syntax Error if HasDirectSuper of MethodDefinition is true.
>     • It is a Syntax Error if PropName of MethodDefinition is "prototype".

[Joseph Pecoraro]

Created attachment 307257 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 307257 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=307257&action=review

> LayoutTests/js/script-tests/class-syntax-declaration.js:87
>  shouldThrow("class X { constructor() {} static set prototype() {} }", "'SyntaxError: Cannot declare a static method named \\'prototype\\'.'");
> +shouldThrow("class X { constructor() {} static get ['prototype']() {} }", "'TypeError: Cannot declare a static method named \\'prototype\\''");

It is peculiar that SyntaxErrors get a period appended to their error string (part of Parser's internalFailWithMessage).

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:566
> +                            OperandTypes(ResultType::unknownType(), ResultType::stringType()));

I originally tried `m_node->m_expression->resultDescriptor()` on the left hand side but this gave me some crashes which I didn't investigate further.

[Joseph Pecoraro]

Created attachment 307344 [details]
[PATCH] Proposed Fix - Rebaselined

Same comments as the previous still apply, however I missed marking some more test262 tests in the yams.

[Joseph Pecoraro]

Created attachment 307345 [details]
[PATCH] Proposed Fix

[Saam Barati]

Comment on attachment 307345 [details]
[PATCH] Proposed Fix

Just talked to Joe in person. I think the deeper problem here is we're trying to Put the property instead of DefineOwnProperty it.

[Joseph Pecoraro]

(In reply to Saam Barati from comment #5)
> Comment on attachment 307345 [details]
> [PATCH] Proposed Fix
> 
> Just talked to Joe in person. I think the deeper problem here is we're
> trying to Put the property instead of DefineOwnProperty it.

Hmm, this path is the path that puts a single getter or a single setter adding on to the existing descriptor instead of replacing it. So I'm not sure we may not be able to  just switch to DefineOwnProperty, however we should be falling through to:
https://tc39.github.io/ecma262/#sec-validateandapplypropertydescriptor

Which I assume would throw an exception in this case. I'll have to read through it carefully.

[Joseph Pecoraro]

Oh, heh JSObject::putGetter and JSObject::putSetter call JSObject::defineOwnProperty with throwException false. I wonder why. I'll compile overnight.

[Joseph Pecoraro]

Created attachment 307698 [details]
[PATCH] Proposed Fix

Improved patch.

We actually continue down the same code path:

    op_put_getter_by_val
      => JSObject::putGetter
        => defineOwnProperty
          => validateAndApplyPropertyDescriptor

Previously we were muting the exception, now we throw it.

[Build Bot]

Comment on attachment 307698 [details]
[PATCH] Proposed Fix

Attachment 307698 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3574791

New failing tests:
webrtc/datachannel/bufferedAmountLowThreshold.html

[Build Bot]

Created attachment 307710 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 307710 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

Unreleated test failure. This patch is good for review.

[JF Bastien]

Comment on attachment 307698 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=307698&action=review

Looks good, but I don't feel super qualified to review :(

> Source/JavaScriptCore/parser/Parser.h:1577
> +    template <class TreeBuilder> TreeProperty parseGetterSetter(TreeBuilder&, bool strict, PropertyNode::Type, unsigned getterOrSetterStartOffset, ConstructorKind, bool isClassProperty, bool isStaticMethod);

I find multiple bool params confusing. Could you make them each their own enum class (here or in a follow-up is fine).

> Source/JavaScriptCore/runtime/JSObject.cpp:1710
> +    return defineOwnProperty(this, exec, propertyName, descriptor, true);

Like this. Out of context I have no clue that this is about throwing. In fact, the definition is super unhelpful also:
  typedef bool (*DefineOwnPropertyFunctionPtr)(JSObject*, ExecState*, PropertyName, const PropertyDescriptor&, bool);
!!!

[Saam Barati]

Comment on attachment 307698 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=307698&action=review

r=me

>> Source/JavaScriptCore/runtime/JSObject.cpp:1710
>> +    return defineOwnProperty(this, exec, propertyName, descriptor, true);
> 
> Like this. Out of context I have no clue that this is about throwing. In fact, the definition is super unhelpful also:
>   typedef bool (*DefineOwnPropertyFunctionPtr)(JSObject*, ExecState*, PropertyName, const PropertyDescriptor&, bool);
> !!!

Agreed. We really need to move to an enum here at some point.

[WebKit Commit Bot]

Comment on attachment 307698 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 307698

Committed r215689: <http://trac.webkit.org/changeset/215689>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

> > Like this. Out of context I have no clue that this is about throwing. In fact, the definition is super unhelpful also:
> >   typedef bool (*DefineOwnPropertyFunctionPtr)(JSObject*, ExecState*, PropertyName, const PropertyDescriptor&, bool);
> > !!!
> 
> Agreed. We really need to move to an enum here at some point.

The only caller who now passes false (meaning do not throw an exception) is the JSC C API. Which honestly should probably throw an exception since it has an exception out parameter. Maybe we should eliminate the boolean by just eliminating this last unique caller.

[Saam Barati]

(In reply to Joseph Pecoraro from comment #16)
> > > Like this. Out of context I have no clue that this is about throwing. In fact, the definition is super unhelpful also:
> > >   typedef bool (*DefineOwnPropertyFunctionPtr)(JSObject*, ExecState*, PropertyName, const PropertyDescriptor&, bool);
> > > !!!
> > 
> > Agreed. We really need to move to an enum here at some point.
> 
> The only caller who now passes false (meaning do not throw an exception) is
> the JSC C API. Which honestly should probably throw an exception since it
> has an exception out parameter. Maybe we should eliminate the boolean by
> just eliminating this last unique caller.

Seems reasonable to me. I guess the worry about changing the C api is breaking things that rely on an exception not being thrown.

In general, I think we booleans to represent Throw or DontThrow. We should probably switch all such uses of a boolean to an enum.

[Joseph Pecoraro]

> > The only caller who now passes false (meaning do not throw an exception) is
> > the JSC C API. Which honestly should probably throw an exception since it
> > has an exception out parameter. Maybe we should eliminate the boolean by
> > just eliminating this last unique caller.
> 
> Seems reasonable to me. I guess the worry about changing the C api is
> breaking things that rely on an exception not being thrown.

After getting nearly to the end, it turns out there was one other user with false.

    Reflect.defineProperty

Which doesn't throw a type error if [[DefineOwnProperty]] fails, it only returns the boolean success/failure of the operation.

--

Before I eliminate the changes in my tree, maybe we can still eliminate the boolean, and just clear any exception in Reflect, and just propagate a false return value. I'll ask later.

[Joseph Pecoraro]

> Before I eliminate the changes in my tree, maybe we can still eliminate the
> boolean, and just clear any exception in Reflect, and just propagate a false
> return value.

It seems DECLARE_CATCH_SCOPE might be appropriate for this after reading CatchScope.h.