[Webkit-unassigned] [Bug 149338] [ES6] Arrow function created before super() causes TDZ, should it?

[bugzilla-daemon at webkit.org]

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

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #265001|review?                     |review-
              Flags|                            |

--- Comment #72 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 265001
  --> https://bugs.webkit.org/attachment.cgi?id=265001
Patch

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

This patch is just about done. It's in really good shape. I just have a few final comments and then I think it's ready to land.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:833
> +    addResult.iterator->value.setIsConst();

Why is "this" modeled as a "const" variable?
I think it's better to just model them as normal variables because
we write to them more than once. Modeling them as
"const" will probably make the put_to_scope code fail when
it goes down the slow path. Unless we always make sure the put_to_scope
passes in the Initialize flag.

It's worth having tests to ensure the code works going
down the slow path. One way to write such a test is to
make the eval var injection watchpoint fire. I think something like this should make
the "this"/"new.target" go down the slow path (you should verify):
```
class C {
    constructor() {
        eval("var x = 20");
        super();
        let f = () => this;
    }
}
```

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3863
> +

nit: only one newline is needed.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3876
> +    emitPutToScope(m_arrowFunctionContextLexicalEnvironmentRegister, newTargetVar, newTarget(), DoNotThrowIfNotFound, NotInitialization);

This should be "Initialization". As I commented above, I think this code would fail if it went down the slow path. We should ensure it works going down the slow path of put_to_scope.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3885
> +        emitPutToScope(m_arrowFunctionContextLexicalEnvironmentRegister, protoScope, &m_calleeRegister, DoNotThrowIfNotFound, NotInitialization);

Ditto: should be "Initialization".
Also, I was totally wrong about naming this "super...", this is really the derived constructor that we're storing in the environemnt
and then later loading __proto__ on. We should name it like "derivedConstructorPrivateName" or "derivedClassPrivateName". Sorry about that.
I had thought we were eagerly storing the __proto__ of the derived constructor.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3894
> +    emitPutToScope(isDerivedConstructorContext() ? emitResolveScope(nullptr, thisVar) : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, NotInitialization);

I think it's arguable whether or not this should be "Initialization" or "NotInitialization". I'd say it should be "Initialization" even though it may execute more than once.
Either way, I think the "this" variable probably shouldn't be marked as being like "const".

Also, we don't want to have more than one op_resolve_scope here because it will always resolve to the same thing. I'm not sure if this code
will run more than once unless we call "super()" more than once in a constructor. This seems really unlikely in real code (but I think it's legal in ES6 to do so),
so it's cleaner to ensure we never emit op_resolve_scope unnecessarily by doing something like this:

if (isDerivedConstructorContext())
    emitLoadArrowFunctionLexicalEnvironment()
emitPutToScope(isDerivedConstructorContext() ? m_resolvedArrowFunctionScopeContextRegister.get() : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, Initialization);

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:830
> +        RefPtr<RegisterID> m_resolvedArrowFunctionScopeContextRegister { nullptr };

you don't need the nullptr initializer here, RefPtrs are by default initialized to null.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3003
> +        bool usesArrowOrEval = generator.usesArrowFunction() || generator.usesEval();

There are a lot of places where you do usesArrow() || usesEval(), I wonder if it's worth giving this
condition a more descriptive name in the various bytecodegenerator constructors.
Maybe like "m_needsToUpdateArrowFunctionContext(generator.usesArrowFunction() || generator.usesEval())"
Just a thought, I'm also okay with having this condition tested at all the interesting places.

> Source/JavaScriptCore/runtime/Executable.h:348
> +    bool isArrowFunctionContext() const { return m_isArrowFunctionContext; }

Could we get rid of these properties (not these methods) and just ask the unlinked code block for this data or get at it through CodeFeatures?

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:1
> +var testCase = function (actual, expected, message) {

style: Let's make this file 4-space indented throughout

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:13
> +for (var i=0; i<10000; i++) {

I think we can get away w/ 1000 iterations for all the loops in this test.
10000 seems overkill.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:27
> +        passed = new.target === B;

Shouldn't this be '&=' and the below just be "=" since below "passed &= new.target === B" is executed first?

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:50
> +      passed &= new.target === B;

Why would this be "B"?

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:75
> +

I take my previous comment back, I don't think we really need a test for this, it's just confusing.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:1
> +var testCase = function (actual, expected, message) {

style: You should make all your test files have a consistent 4-space indent.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:16
> +  constructor (inArrowFuction, inConstructor, repeatInArrowFunction) {

"repeatInArrowFunction" is unused, maybe remove it or were you planning on calling super() twice?

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:35
> +for (var i=0; i < 10000; i++) {

I think 1000 iterations is also good for tests in this file.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:17
> +    var arrow = () => () => () => {

👍

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:34
> +for (var i=0; i < 10000; i++) {

1000 iterations is probably good for this file too.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:39
> +var testException = function (value1, value2, value3, index) {

value3 is unused.
I would make this function take only the index parameter
because value1 and value2 are always false. It's easier
to just pass in "false" yourself

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:95
> +        E.__proto__ = function () {};

Might be worth having a test that sets __proto__ to "null" and make sure that we throw an error.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-3.js:31
> +for (var i=0; i < 10000; i++) {

I think all your tests can just be 1000 iterations.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-3.js:35
> +// Fixme: Failed test with 'Segmentation fault: 11' error in release mode in 6 cases from 14. List of failed case:

Is this fixed?

-- 
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/20151108/4edafed6/attachment-0001.html>