[Webkit-unassigned] [Bug 149338] [ES6] "super" and "this" should be lexically bound inside an arrow function and should live in a JSLexicalEnvironment
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 11 11:06:59 PST 2015
https://bugs.webkit.org/show_bug.cgi?id=149338
GSkachkov <gskachkov at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #265001| |review-, commit-queue?
Flags| |
--- Comment #90 from GSkachkov <gskachkov at gmail.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
>> 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;
> }
> }
> ```
I've changed to 'let' variable type. I've added one test case to cover going down the slow path.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3863
>> +
>
> nit: only one newline is needed.
Removed
>> 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.
Done
>> 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.
NP. I've changed name to derivedConstructorPrivateName
>> 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);
Changed const to 'let', and used your snipped.
I'm sure that we can't run super() twice in constructor, second call should lead to RuntimeException. See tred on es6-discuss https://esdiscuss.org/topic/duplicate-super-call-behaviour.
But this behavior is not related to the arrow function.
>> 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.
Done
>> 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.
I've created property needsToUpdateArrowFunctionContext in generator, and now it is used in several placed
>> 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?
I've added CodeFeature. To force it works I made small 'trick'. Please take a look if it look ok.
>> 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
Done
>> 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.
Done
>> 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?
Ohh, my fault.
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:50
>> + passed &= new.target === B;
>
> Why would this be "B"?
Removed this condition
>> 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.
Done
>> 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?
I've removed. I thought about this, but I'm going to call super() twice in another bug.
>> 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.
done
>> 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.
done
>> 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
Done
>> 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.
Added new tests
>> 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.
Done
>> 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?
Now it works, I'll rollback soon to check this error again.
--
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/20151111/5b3a7d7c/attachment-0001.html>
More information about the webkit-unassigned
mailing list