Bug 170897 - test262: test262/test/language/computed-property-names/class/static/getter-prototype.js
Summary: test262: test262/test/language/computed-property-names/class/static/getter-pr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-17 00:17 PDT by Joseph Pecoraro
Modified: 2017-04-26 01:28 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (17.69 KB, patch)
2017-04-17 00:21 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix - Rebaselined (18.85 KB, patch)
2017-04-17 22:38 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (20.03 KB, patch)
2017-04-17 22:43 PDT, Joseph Pecoraro
sbarati: review-
sbarati: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (20.98 KB, patch)
2017-04-20 23:27 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (843.04 KB, application/zip)
2017-04-21 01:31 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-04-17 00:17:23 PDT
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".
Comment 1 Joseph Pecoraro 2017-04-17 00:21:58 PDT
Created attachment 307257 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2017-04-17 00:28:00 PDT
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.
Comment 3 Joseph Pecoraro 2017-04-17 22:38:51 PDT
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.
Comment 4 Joseph Pecoraro 2017-04-17 22:43:17 PDT
Created attachment 307345 [details]
[PATCH] Proposed Fix
Comment 5 Saam Barati 2017-04-18 15:27:22 PDT
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.
Comment 6 Joseph Pecoraro 2017-04-19 01:41:03 PDT
(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.
Comment 7 Joseph Pecoraro 2017-04-19 01:49:15 PDT
Oh, heh JSObject::putGetter and JSObject::putSetter call JSObject::defineOwnProperty with throwException false. I wonder why. I'll compile overnight.
Comment 8 Joseph Pecoraro 2017-04-20 23:27:14 PDT
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.
Comment 9 Build Bot 2017-04-21 01:31:24 PDT
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
Comment 10 Build Bot 2017-04-21 01:31:25 PDT
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
Comment 11 Joseph Pecoraro 2017-04-23 00:10:04 PDT
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.
Comment 12 JF Bastien 2017-04-24 09:29:11 PDT
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);
!!!
Comment 13 Saam Barati 2017-04-24 11:55:47 PDT
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.
Comment 14 WebKit Commit Bot 2017-04-24 12:25:05 PDT
Comment on attachment 307698 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 307698

Committed r215689: <http://trac.webkit.org/changeset/215689>
Comment 15 WebKit Commit Bot 2017-04-24 12:25:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Joseph Pecoraro 2017-04-24 12:46:29 PDT
> > 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.
Comment 17 Saam Barati 2017-04-24 17:14:34 PDT
(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.
Comment 18 Joseph Pecoraro 2017-04-26 00:58:57 PDT
> > 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.
Comment 19 Joseph Pecoraro 2017-04-26 01:28:26 PDT
> 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.