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".
Created attachment 307257 [details] [PATCH] Proposed Fix
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.
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.
Created attachment 307345 [details] [PATCH] Proposed Fix
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.
(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.
Oh, heh JSObject::putGetter and JSObject::putSetter call JSObject::defineOwnProperty with throwException false. I wonder why. I'll compile overnight.
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 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
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 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 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 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 on attachment 307698 [details] [PATCH] Proposed Fix Clearing flags on attachment: 307698 Committed r215689: <http://trac.webkit.org/changeset/215689>
All reviewed patches have been landed. Closing bug.
> > 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.
(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.
> > 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.
> 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.