[Webkit-unassigned] [Bug 145132] [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "arguments"

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=145132

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #259366|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #22 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 259366
  --> https://bugs.webkit.org/attachment.cgi?id=259366
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150821/723c7f7e/attachment-0001.html>