Bug 148960

Summary: Web Inspector: update Esprima to latest version
Product: WebKit Reporter: Saam Barati <sbarati>
Component: Web InspectorAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: ariya.hidayat, bburg, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
joepeck: review+
patch joepeck: review+

Description Saam Barati 2015-09-08 09:09:38 PDT
...
Comment 1 Radar WebKit Bug Importer 2015-09-08 09:15:31 PDT
<rdar://problem/22611037>
Comment 2 Saam Barati 2015-09-08 13:43:11 PDT
*** Bug 148972 has been marked as a duplicate of this bug. ***
Comment 3 Saam Barati 2015-09-21 16:04:22 PDT
Created attachment 261698 [details]
patch
Comment 4 Joseph Pecoraro 2015-09-21 16:31:08 PDT
Comment on attachment 261698 [details]
patch

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

r=me, but there are some changes that I think deserve to be made to ScriptSyntaxTree at the same time.

> Source/WebInspectorUI/ChangeLog:17
> +        * UserInterface/External/Esprima/esprima.js:
> +        (assert):
> +        (isDecimalDigit):
> +        (isOctalDigit):
> +        (octalToDecimal):

You can drop everything past esprima.js in the ChangeLog, it is not particularly useful.

> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:108
> +        AssignmentPattern: 'AssignmentPattern',

Of the new Nodes, there are three that WebInspector.ScriptSyntaxTree does not support but probably should:

    AssignmentPattern (destructuring)
    MetaProperty (new.target?)
    Super (super?)

The rest are things we know about and haven't yet supported:

    ArrowFunctionExpression
    ExportAllDeclaration
    ExportDefaultDeclaration
    ExportNamedDeclaration
    ExportSpecifier
    ImportDeclaration
    ImportDefaultSpecifier
    ImportNamespaceSpecifier
    ImportSpecifier
    RestElement
    YieldStatement

----

It seems that, at least for this patch ScriptSyntaxTree should:

  1. add support for AssignmentPattern to prevent breaking type profiling with destructuring assignments.
  2. Cover MetaProperty and Super, which should be rather basic, to avoid potential issues parsing classes

Do you agree, or were things working in these cases?

> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:2137
> +        finishProgram: function (body, sourceType) {
> +            this.type = Syntax.Program;
> +            this.body = body;
> +            this.sourceType = sourceType;

The "sourceType" property of Program is new. Maybe we should reflect that in ScriptSyntaxTree. It seems sourceType can be the string 'script' or 'module'.

> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:2246
> +        finishTryStatement: function (block, handler, finalizer) {
> +            this.type = Syntax.TryStatement;
> +            this.block = block;
> +            this.guardedHandlers = [];
> +            this.handlers = handler ? [handler] : [];
> +            this.handler = handler;

This is a particularly interesting change. There appears to be a single 'handler' property alongside 'handlers' which is either a single element array or empty array. Perhaps we should also move to a single handler as well.
Comment 5 Joseph Pecoraro 2015-09-21 16:33:13 PDT
Comment on attachment 261698 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:2246
>> +            this.handler = handler;
> 
> This is a particularly interesting change. There appears to be a single 'handler' property alongside 'handlers' which is either a single element array or empty array. Perhaps we should also move to a single handler as well.

Yes, further discussion on ESTree. Funny, this was the first ESTree issue!
<https://github.com/estree/estree/issues/1>
Comment 6 Ariya Hidayat 2015-09-21 17:03:02 PDT
(In reply to comment #5)
> Comment on attachment 261698 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261698&action=review
> 
> >> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:2246
> >> +            this.handler = handler;
> > 
> > This is a particularly interesting change. There appears to be a single 'handler' property alongside 'handlers' which is either a single element array or empty array. Perhaps we should also move to a single handler as well.
> 
> Yes, further discussion on ESTree. Funny, this was the first ESTree issue!
> <https://github.com/estree/estree/issues/1>

And we will drop that in Esprima 3.0 (major version bump, breaking change):
https://github.com/jquery/esprima/issues/1030
Comment 7 Saam Barati 2015-09-21 19:40:25 PDT
Created attachment 261717 [details]
patch
Comment 8 Joseph Pecoraro 2015-09-21 19:49:33 PDT
Comment on attachment 261717 [details]
patch

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

r=me, nice!

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:449
> +        case WebInspector.ScriptSyntaxTree.NodeType.Super:

You could share the implementation of Meta/Super/This, since they are so super similar. Or we could try to keep this mostly alphabetical. Either way works!

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:556
> +

Style: Unnecessary newline.

> LayoutTests/inspector/model/parse-script-syntax-tree.html:449
> +    node = makeNode("class C { constructor() { super(); new.target; } }", false);

Oh snap, I forgot about this test. It would also be awesome to have some tests for template strings / default arguments (which I recently added but did not test). But that is totally up to you.
Comment 9 Saam Barati 2015-09-22 00:47:14 PDT
Comment on attachment 261717 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:449
>> +        case WebInspector.ScriptSyntaxTree.NodeType.Super:
> 
> You could share the implementation of Meta/Super/This, since they are so super similar. Or we could try to keep this mostly alphabetical. Either way works!

I'll collect all leaf nodes into one section.

>> LayoutTests/inspector/model/parse-script-syntax-tree.html:449
>> +    node = makeNode("class C { constructor() { super(); new.target; } }", false);
> 
> Oh snap, I forgot about this test. It would also be awesome to have some tests for template strings / default arguments (which I recently added but did not test). But that is totally up to you.

Lets do this in another patch.
Comment 10 Saam Barati 2015-09-22 01:27:35 PDT
landed in:
http://trac.webkit.org/changeset/190110