It is necessary to add lexical bind of the "arguments", "super", "new.target"
Created attachment 259264 [details] Patch init version
Comment on attachment 259264 [details] Patch Attachment 259264 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/71653 New failing tests: js/arrowfunction-lexical-bind-arguments.html
Created attachment 259265 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 259264 [details] Patch Attachment 259264 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/71661 New failing tests: js/arrowfunction-lexical-bind-arguments.html
Created attachment 259266 [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
Created attachment 259267 [details] Patch Fix tests
Comment on attachment 259267 [details] Patch Attachment 259267 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/72002 New failing tests: js/arrowfunction-lexical-bind-arguments.html
Created attachment 259268 [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
Comment on attachment 259267 [details] Patch Attachment 259267 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/72012 New failing tests: js/arrowfunction-lexical-bind-arguments.html
Created attachment 259269 [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
Created attachment 259270 [details] Patch Fix layout tests
Comment on attachment 259270 [details] Patch Attachment 259270 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/72159 New failing tests: js/arrowfunction-lexical-bind-arguments.html
Created attachment 259274 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 259270 [details] Patch Attachment 259270 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/72163 New failing tests: js/arrowfunction-lexical-bind-arguments.html
Created attachment 259275 [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
Created attachment 259279 [details] Patch Fix layout tests
Created attachment 259292 [details] Patch Small changes
Created attachment 259366 [details] Patch Small improvementes
Comment on attachment 259366 [details] Patch Looks like something wrong with build
(In reply to comment #19) > Comment on attachment 259366 [details] > Patch > > Looks like something wrong with build ../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp: In function 'void JSC::computeBytecodeBasicBlocks(JSC::CodeBlock*, WTF::Vector<WTF::RefPtr<JSC::BytecodeBasicBlock> >&)': ../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:229:1: internal compiler error: Segmentation fault For GTK & EFL, this is almost certainly the result of the EWS machine running out of disk space, memory, or some other system resource. It's a known sporadic issue. You can ignore it.
(In reply to comment #20) > (In reply to comment #19) > > Comment on attachment 259366 [details] > > Patch > > > > Looks like something wrong with build > > ../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp: In function > 'void JSC::computeBytecodeBasicBlocks(JSC::CodeBlock*, > WTF::Vector<WTF::RefPtr<JSC::BytecodeBasicBlock> >&)': > ../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:229:1: internal > compiler error: Segmentation fault > > For GTK & EFL, this is almost certainly the result of the EWS machine > running out of disk space, memory, or some other system resource. It's a > known sporadic issue. You can ignore it. OK. Thanks for info
Comment on attachment 259366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259366&action=review The overall approach here is good. There needs to be some significant reworking in the bytecode generator (and maybe the parser) here. Also, we need many more tests. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:70 > + if (m_needToInitializeArguments) { It doesn't seem like you're doing any prevention of allocating the different arguments object while inside an arrow function. This is bad. We need to prevent those allocations. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:73 > + initializeVariable(variable(propertyNames().arguments), m_arrowFunctionArguments.get()); You can just use m_argumentsRegister here. Also, we don't prefix local variables with "m_". > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2305 > + RegisterID* m_tmpArgumentsRegister = emitGetFromScope(newTemporary(), scope, var, DoNotThrowIfNotFound); This is pretty shady. We should just be using m_argumentsRegister here. We shouldn't resolve anything. What this requires is that we now must note that an arguments object must be created for functions that have arrow functions nested inside them. We can get smart here, and we should if it's not too difficult, and only allocate these arguments objects as being needed, i.e it the arrow function uses "arguments" or eval. Maybe a good first take before optimizing is just report that any function with an arrow function nested inside of it needs to allocate an arguments object (unless the parent function is an arrow function itself, in which case it should get its arguments object through op_get_arrowfunction_arguments). Once you get this working, you can optimize cases like this: "function foo() { var x = () => 20; }". Obviously foo doesn't need an arguments object "function foo() { var x = (p) => eval(p); }". foo does need arguments object "function foo() { var x = () => { var x = () => arguments; } }". foo does need an arguments object if there is a chain from a non-arrow-function through only arrow functions that use arguments or eval. "function foo() { var x = () => { var x = () { function bar() { var x = () => arguments; } } } }". bar function does need arguments. foo doesn't. > Source/JavaScriptCore/tests/stress/arrowfunction-lexical-arguments.js:1 > +var testCase = function (actual, expected, message) { I don't think we're covering enough tests here. JSC has three different arguments objects it allocates. We should be writing tests to target each of these. We also need to ensure different properties of the arguments objects hold. For example, we shouldn't be able to overwrite local variables for strict mode arguments object. Does lexical binding of arguments object impose any restrictions on the type of arguments object? Or does it not matter. If it doesn't matter, we need tests for sloppy mode argument assignment inside an arrow function which change "local" arguments parameters.
Created attachment 268011 [details] Patch Upload new version of the patch
Comment on attachment 268011 [details] Patch Attachment 268011 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/627910 Number of test failures exceeded the failure limit.
Created attachment 268017 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268011 [details] Patch Attachment 268011 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/627923 Number of test failures exceeded the failure limit.
Created attachment 268018 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268011 [details] Patch Attachment 268011 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/627927 Number of test failures exceeded the failure limit.
Created attachment 268019 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 268098 [details] Patch Fix tests
Comment on attachment 268098 [details] Patch Attachment 268098 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/638982 New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-events-inspector-stylesheet.html imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html inspector/console/messageRepeatCountUpdated.html imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html inspector/css/stylesheet-events-basic.html imported/w3c/web-platform-tests/streams-api/readable-streams/general.html imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html inspector/console/command-line-api.html inspector/css/stylesheet-events-multiple-documents.html imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html inspector/network/client-blocked-load.html inspector/model/stack-trace.html
Created attachment 268100 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268098 [details] Patch Attachment 268098 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/638988 New failing tests: inspector/css/stylesheet-events-inspector-stylesheet.html imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html inspector/css/stylesheet-events-multiple-documents.html imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html inspector/console/messageRepeatCountUpdated.html inspector/css/stylesheet-events-basic.html imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html inspector/console/command-line-api.html imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html inspector/dom/setOuterHTML.html imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html imported/w3c/web-platform-tests/streams-api/readable-streams/general.html imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html inspector/model/stack-trace.html inspector/css/generate-css-rule-string.html imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html inspector/network/client-blocked-load.html
Created attachment 268101 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268098 [details] Patch Attachment 268098 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/638994 New failing tests: inspector/css/stylesheet-events-inspector-stylesheet.html imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html inspector/css/stylesheet-events-multiple-documents.html imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html inspector/console/messageRepeatCountUpdated.html inspector/css/stylesheet-events-basic.html imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html inspector/console/command-line-api.html imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html inspector/dom/setOuterHTML.html imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html imported/w3c/web-platform-tests/streams-api/readable-streams/general.html imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html inspector/model/stack-trace.html inspector/css/generate-css-rule-string.html imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html inspector/network/client-blocked-load.html
Created attachment 268102 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 268144 [details] Patch Fix tests
Comment on attachment 268144 [details] Patch Attachment 268144 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/644887 Number of test failures exceeded the failure limit.
Created attachment 268147 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268144 [details] Patch Attachment 268144 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/644890 Number of test failures exceeded the failure limit.
Created attachment 268148 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268144 [details] Patch Attachment 268144 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/644899 Number of test failures exceeded the failure limit.
Created attachment 268149 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 268192 [details] Patch Fix WebKit tests
Comment on attachment 268192 [details] Patch Attachment 268192 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/648878 New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-events-inspector-stylesheet.html inspector/dom/setOuterHTML.html inspector/css/stylesheet-events-basic.html inspector/console/command-line-api.html inspector/model/stack-trace.html inspector/css/stylesheet-events-multiple-documents.html inspector/console/messageRepeatCountUpdated.html inspector/network/client-blocked-load.html
Created attachment 268196 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268192 [details] Patch Attachment 268192 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/648877 New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-events-inspector-stylesheet.html inspector/dom/setOuterHTML.html inspector/css/stylesheet-events-basic.html inspector/network/client-blocked-load.html inspector/model/stack-trace.html inspector/css/stylesheet-events-multiple-documents.html inspector/console/messageRepeatCountUpdated.html inspector/console/command-line-api.html
Created attachment 268198 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 268192 [details] Patch Attachment 268192 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/648887 New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-events-inspector-stylesheet.html inspector/dom/setOuterHTML.html inspector/css/stylesheet-events-basic.html inspector/console/command-line-api.html inspector/model/stack-trace.html inspector/css/stylesheet-events-multiple-documents.html inspector/console/messageRepeatCountUpdated.html inspector/network/client-blocked-load.html
Created attachment 268199 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 268326 [details] Patch Fix WebKit tests#2
Comment on attachment 268326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268326&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:83 > + if (m_codeBlock->isStrictMode()) > + emitTDZCheck(argumentsRegister); why would we ever need a TDZ check here? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4123 > +RegisterID* BytecodeGenerator::emitPutArgumentsToArrowFunctionContextBlockScope() I see what you're going for here in this function, but this seems a bit hacky to me. I think there might be an alternate solution that might be cleaner: We currently don't allow functions to declare the variable named "arguments" as being captured (you can see this in the "collectFreeVariables" function inside Parser.h). But, in the case of an arrow function, we really do want "arguments" to act like a captured variable. If we teach the parser we can really capture "arguments", then we could probably do away with a lot of this specialized code. We wouldn't need this function (because "arguments" would already be in whatever environment is represented by scopeRegister()). We probably wouldn't need a specialized argumentsLocalPrivateName either. We would need some BytecodeGenerator code just to teach it that we don't have a local "arguments" that we created, but that's about it. I think most of this change could be a parser change. You would basically have to teach the parser that any arrow function implicitly captures the name "arguments". Or we could be smart about it, and only capture "arguments" if we use "arguments" or "eval" inside the arrow function.
Created attachment 269291 [details] Patch Fix comments
Created attachment 269298 [details] Patch Remove redundant code
Comment on attachment 268326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268326&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4123 >> +RegisterID* BytecodeGenerator::emitPutArgumentsToArrowFunctionContextBlockScope() > > I see what you're going for here in this function, but this seems a bit hacky to me. > I think there might be an alternate solution that might be cleaner: > > We currently don't allow functions to declare the variable named "arguments" > as being captured (you can see this in the "collectFreeVariables" function inside Parser.h). > But, in the case of an arrow function, we really do want "arguments" to act like a captured variable. > If we teach the parser we can really capture "arguments", then we could probably do away > with a lot of this specialized code. We wouldn't need this function (because "arguments" would already > be in whatever environment is represented by scopeRegister()). We probably wouldn't need a specialized > argumentsLocalPrivateName either. We would need some BytecodeGenerator code just to teach it > that we don't have a local "arguments" that we created, but that's about it. I think most of this change > could be a parser change. > > You would basically have to teach the parser that any arrow function implicitly captures the name "arguments". > Or we could be smart about it, and only capture "arguments" if we use "arguments" or "eval" inside the > arrow function. Fixed. Oh I did not know about this collectFreeVariables. This approach is much better because allow to decrease amount of code
Comment on attachment 269298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269298&action=review r=me with one suggestion and a few comments: Can you add a test to make sure it works with generators: function* foo(a, b, c) { yield () => arguments; } foo(10, 20).value[0] === 10 foo(10, 20).value[1] === 20 > Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-arguments-strict.js:32 > + // https://bugs.webkit.org/show_bug.cgi?id=152570 why does it lead to a reference error? > Source/WebInspectorUI/UserInterface/Base/Object.js:63 > - let wrappedCallback = () => { > - this.removeEventListener(eventType, wrappedCallback, null); > + let that = this; > + let wrappedCallback = function () { > + that.removeEventListener(eventType, wrappedCallback, null); why is this necessary?
Created attachment 269693 [details] Patch Fix comments
Comment on attachment 269298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269298&action=review Also I've added test of Generator >> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-arguments-strict.js:32 >> + // https://bugs.webkit.org/show_bug.cgi?id=152570 > > why does it lead to a reference error? Now I understand that it is wrong place for this test. I've removed it. The idea is that Global scope does not contain variable 'arguments', and access to the 'arguments' from global scope should lead to ReferenceError. Current test is executed in JSC app where arguments is empty array. >> Source/WebInspectorUI/UserInterface/Base/Object.js:63 >> + that.removeEventListener(eventType, wrappedCallback, null); > > why is this necessary? See line below. Without this change, but with current patch - arguments will start contain values of singleFireEventListener instead of values that is passed during wrappedCallback invocation.
Comment on attachment 269693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269693&action=review > Source/WebInspectorUI/UserInterface/Base/Object.js:61 > + let that = this; It's inspector style to use .bind(function, this).
Created attachment 269753 [details] Patch Fix comments
Comment on attachment 269753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269753&action=review > Source/WebInspectorUI/UserInterface/Base/Object.js:61 > - let wrappedCallback = () => { > + let wrappedCallback = function () { Style Nit: "function ()" => "function()" Good find!
Created attachment 269784 [details] Patch Fix style comments[D
Comment on attachment 269753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269753&action=review >> Source/WebInspectorUI/UserInterface/Base/Object.js:61 >> + let wrappedCallback = function () { > > Style Nit: "function ()" => "function()" > > Good find! Fixed
Comment on attachment 269784 [details] Patch Clearing flags on attachment: 269784 Committed r195581: <http://trac.webkit.org/changeset/195581>
All reviewed patches have been landed. Closing bug.