<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Arrow function created before super() causes TDZ, should it?"
href="https://bugs.webkit.org/show_bug.cgi?id=149338">bug 149338</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #265001 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Arrow function created before super() causes TDZ, should it?"
href="https://bugs.webkit.org/show_bug.cgi?id=149338#c72">Comment # 72</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Arrow function created before super() causes TDZ, should it?"
href="https://bugs.webkit.org/show_bug.cgi?id=149338">bug 149338</a>
from <span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=265001&action=diff" name="attach_265001" title="Patch">attachment 265001</a> <a href="attachment.cgi?id=265001&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=265001&action=review">https://bugs.webkit.org/attachment.cgi?id=265001&action=review</a>
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.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:833
> + addResult.iterator->value.setIsConst();</span >
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;
}
}
```
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3863
> +</span >
nit: only one newline is needed.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3876
> + emitPutToScope(m_arrowFunctionContextLexicalEnvironmentRegister, newTargetVar, newTarget(), DoNotThrowIfNotFound, NotInitialization);</span >
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.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3885
> + emitPutToScope(m_arrowFunctionContextLexicalEnvironmentRegister, protoScope, &m_calleeRegister, DoNotThrowIfNotFound, NotInitialization);</span >
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.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3894
> + emitPutToScope(isDerivedConstructorContext() ? emitResolveScope(nullptr, thisVar) : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, NotInitialization);</span >
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);
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:830
> + RefPtr<RegisterID> m_resolvedArrowFunctionScopeContextRegister { nullptr };</span >
you don't need the nullptr initializer here, RefPtrs are by default initialized to null.
<span class="quote">> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3003
> + bool usesArrowOrEval = generator.usesArrowFunction() || generator.usesEval();</span >
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.
<span class="quote">> Source/JavaScriptCore/runtime/Executable.h:348
> + bool isArrowFunctionContext() const { return m_isArrowFunctionContext; }</span >
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?
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:1
> +var testCase = function (actual, expected, message) {</span >
style: Let's make this file 4-space indented throughout
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:13
> +for (var i=0; i<10000; i++) {</span >
I think we can get away w/ 1000 iterations for all the loops in this test.
10000 seems overkill.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:27
> + passed = new.target === B;</span >
Shouldn't this be '&=' and the below just be "=" since below "passed &= new.target === B" is executed first?
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:50
> + passed &= new.target === B;</span >
Why would this be "B"?
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:75
> +</span >
I take my previous comment back, I don't think we really need a test for this, it's just confusing.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:1
> +var testCase = function (actual, expected, message) {</span >
style: You should make all your test files have a consistent 4-space indent.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:16
> + constructor (inArrowFuction, inConstructor, repeatInArrowFunction) {</span >
"repeatInArrowFunction" is unused, maybe remove it or were you planning on calling super() twice?
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:35
> +for (var i=0; i < 10000; i++) {</span >
I think 1000 iterations is also good for tests in this file.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:17
> + var arrow = () => () => () => {</span >
👍
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:34
> +for (var i=0; i < 10000; i++) {</span >
1000 iterations is probably good for this file too.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:39
> +var testException = function (value1, value2, value3, index) {</span >
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
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:95
> + E.__proto__ = function () {};</span >
Might be worth having a test that sets __proto__ to "null" and make sure that we throw an error.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-3.js:31
> +for (var i=0; i < 10000; i++) {</span >
I think all your tests can just be 1000 iterations.
<span class="quote">> 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:</span >
Is this fixed?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>