Bug 148445 - [ES6] Arrow function syntax. Add missed tests for invoking the arrow function that created by 'eval' statement.
Summary: [ES6] Arrow function syntax. Add missed tests for invoking the arrow function...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 148148
Blocks: 140855
  Show dependency treegraph
 
Reported: 2015-08-25 13:36 PDT by GSkachkov
Modified: 2015-08-31 23:44 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.06 KB, patch)
2015-08-25 13:59 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (2.88 KB, patch)
2015-08-31 12:31 PDT, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2015-08-25 13:36:55 PDT
eval('var x=x=>x+10');
x(111); // Returns Exception: SyntaxError: Invalid character: '\0'
Comment 1 GSkachkov 2015-08-25 13:59:21 PDT
Created attachment 259876 [details]
Patch

Added test for this case. Issue was fixed by following patch https://bugs.webkit.org/show_bug.cgi?id=148148
Comment 2 Yusuke Suzuki 2015-08-25 19:32:46 PDT
Comment on attachment 259876 [details]
Patch

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

> LayoutTests/ChangeLog:5
> +

Could you tell me which part fixed this issue in the original patch? http://trac.webkit.org/changeset/188928
Describing it here is nice.

And could you tell me why `functionInfo.endOffset - 1` is needed in the original patch?
Comment 3 GSkachkov 2015-08-26 09:10:12 PDT
(In reply to comment #2)
> Comment on attachment 259876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259876&action=review
> 
> > LayoutTests/ChangeLog:5
> > +
> 
> Could you tell me which part fixed this issue in the original patch?
All changes, that were made in 'parser' module, fix current issue. 
> http://trac.webkit.org/changeset/188928
> Describing it here is nice.
> 
> And could you tell me why `functionInfo.endOffset - 1` is needed in the
> original patch?
The issue was that during first parsing arrow function, with body as expression (z=>z*2), has redundant symbol at the end, i.e. var f = x => x+1; and after parsing it has own source as 'x => x+1;' with redundant ';' at the end. It is OK for all cases except case for eval, when it leads to error during invoking function. I found this issue when was fixing issue with toString method, because toString returns extra ';' but should not. So I decided to add test that will prevent this issue with eval in future. 
 
Possible it would be better to split original patch into two patches, 'fix parsing' and add 'correct toString' to avoid misunderstanding, also possible I need to rename current issue to just 'adding additional tests for arrow function'.

What do you think I need to do to make this issue more clear?
Comment 4 Yusuke Suzuki 2015-08-27 16:57:19 PDT
(In reply to comment #3)
> The issue was that during first parsing arrow function, with body as
> expression (z=>z*2), has redundant symbol at the end, i.e. var f = x => x+1;
> and after parsing it has own source as 'x => x+1;' with redundant ';' at the
> end. It is OK for all cases except case for eval, when it leads to error
> during invoking function. I found this issue when was fixing issue with
> toString method, because toString returns extra ';' but should not. So I
> decided to add test that will prevent this issue with eval in future. 
>  
> Possible it would be better to split original patch into two patches, 'fix
> parsing' and add 'correct toString' to avoid misunderstanding, also possible
> I need to rename current issue to just 'adding additional tests for arrow
> function'.
> 
> What do you think I need to do to make this issue more clear?

Yeah. I think describing the details in ChangeLog (and issue title) is preferable :)
Comment 5 GSkachkov 2015-08-31 12:31:22 PDT
Created attachment 260307 [details]
Patch

Updated patch
Comment 6 Yusuke Suzuki 2015-08-31 14:41:08 PDT
Comment on attachment 260307 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2015-08-31 15:29:54 PDT
Comment on attachment 260307 [details]
Patch

Clearing flags on attachment: 260307

Committed r189187: <http://trac.webkit.org/changeset/189187>
Comment 8 WebKit Commit Bot 2015-08-31 15:29:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 GSkachkov 2015-08-31 23:44:09 PDT
(In reply to comment #6)
> Comment on attachment 260307 [details]
> Patch
> 
> r=me

Thanks!