Bug 148960 – Web Inspector: update Esprima to latest version

• Component: Web Inspector
• Status: RESOLVED FIXED

[Saam Barati]

...

[Radar WebKit Bug Importer]

<rdar://problem/22611037>

[Saam Barati]

*** Bug 148972 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 261698 [details]
patch

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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>

[Ariya Hidayat]

(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

[Saam Barati]

Created attachment 261717 [details]
patch

[Joseph Pecoraro]

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.

[Saam Barati]

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.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/190110