Bug 148148 – Function.prototype.toString is incorrect for ArrowFunction

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Blaze Burg]

It should be possible to define an arrow function, call toString on it, and evaluate the result without a parse error.

TEST CASE:

1    let a = () => {};
2    a.toString(); // returns function () => {}


Evaluating line 2's result will cause a parse error.

(Relevant spec: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function.prototype.tostring)

[GSkachkov]

Created attachment 259368 [details]
Patch

Fix

[GSkachkov]

I've uploaded small patch to fix this issue, but exists one more open issue 
https://bugs.webkit.org/show_bug.cgi?id=146507 that is related to toString method. I'll fix later

[Blaze Burg]

Comment on attachment 259368 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [ES6] Arrow function syntax. Arrow function specific features. Implement correct toString method

This bug title doesn't match the one in the link.

> Source/JavaScriptCore/ChangeLog:6
> +        Added correct support of toString() method for arrow function

Nit: trailing period.

> Source/JavaScriptCore/tests/stress/arrowfunction-tostring.js:8
> +var af2 = (a) => {a + 1};

This test is missing several syntax cases.

> LayoutTests/ChangeLog:6
> +        Added test of toString() method

Nit: trailing period.

[GSkachkov]

Created attachment 259403 [details]
Patch

Fix review comments

[GSkachkov]

Created attachment 259404 [details]
Patch

Fix review comments & reupload

[GSkachkov]

Comment on attachment 259368 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:3
>> +        [ES6] Arrow function syntax. Arrow function specific features. Implement correct toString method
> 
> This bug title doesn't match the one in the link.

Done

>> Source/JavaScriptCore/tests/stress/arrowfunction-tostring.js:8
>> +var af2 = (a) => {a + 1};
> 
> This test is missing several syntax cases.

I've added more tests, but I had to fix some issue in parser, so we need to review it as well

[Saam Barati]

Comment on attachment 259404 [details]
Patch

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

r=me
Please reupload with fixed Nit and added test cases for arrow function body expressions that don't end in ";" and I will r+/cq+

> Source/JavaScriptCore/parser/ASTBuilder.h:375
> +        SourceCode source = m_sourceCode->subExpression(functionInfo.startOffset, functionInfo.body->isArrowFunctionBodyExpression() ? functionInfo.endOffset -1 : functionInfo.endOffset, functionInfo.startLine, functionInfo.bodyStartColumn);

Nit: space between "-" and "1"

> Source/JavaScriptCore/parser/ASTBuilder.h:376
>          ArrowFuncExprNode* result = new (m_parserArena) ArrowFuncExprNode(location, *functionInfo.name, functionInfo.body, source);

The - 1 worries me. 
Does this work:
function foo(x) { print(x.toString() }
foo((y)=>y)

[GSkachkov]

Created attachment 259831 [details]
Patch

Fix review comments & reupload

[GSkachkov]

Created attachment 259833 [details]
Patch

Add one more test

[GSkachkov]

Comment on attachment 259404 [details]
Patch

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

>> Source/JavaScriptCore/parser/ASTBuilder.h:375
>> +        SourceCode source = m_sourceCode->subExpression(functionInfo.startOffset, functionInfo.body->isArrowFunctionBodyExpression() ? functionInfo.endOffset -1 : functionInfo.endOffset, functionInfo.startLine, functionInfo.bodyStartColumn);
> 
> Nit: space between "-" and "1"

Done

>> Source/JavaScriptCore/parser/ASTBuilder.h:376
>>          ArrowFuncExprNode* result = new (m_parserArena) ArrowFuncExprNode(location, *functionInfo.name, functionInfo.body, source);
> 
> The - 1 worries me. 
> Does this work:
> function foo(x) { print(x.toString() }
> foo((y)=>y)

I've added more tests, check in LayoutTests/js/script-tests/arrowfunction-tostring.js is it enough.

[Saam Barati]

Comment on attachment 259833 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 259833 [details]
Patch

Clearing flags on attachment: 259833

Committed r188928: <http://trac.webkit.org/changeset/188928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.