Bug 164830 – Web Inspector: Update Esprima to support new features / syntax (**, async/await, trailing comma)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Summary:
Update Esprima to support new features / syntax (**, async/await, trailing comma)

Currently we fail to parse / pretty print code that has ES2017 features which JavaScriptCore supports:

  * Exponentiation operators (**, **=)
  * async/await functions
  * Trailing commas

All of which are supported in Esprima trunk (4.0.0-dev after its latest release 3.1.1).

[Radar WebKit Bug Importer]

<rdar://problem/29293814>

[Joseph Pecoraro]

Created attachment 294963 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 294963 [details]
[PATCH] Proposed Fix

Err some tests need an update.

[Joseph Pecoraro]

Created attachment 294969 [details]
[PATCH] Proposed Fix

[Saam Barati]

Comment on attachment 294969 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/formatting/resources/javascript-tests/classes-expected.js:121
> +    async a()

what about adding tests for:

1. async computed properties
2. async string literal methods
3. async number literal methods

also, all of the above, but in an object literal instead of a class.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:-537
> -                defaults: node.defaults.map(this._createInternalSyntaxTree, this),

Why did this go away?

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:-682
> -                defaults: node.defaults.map(this._createInternalSyntaxTree, this),

Ditto. Was this replaced with something else?

[Joseph Pecoraro]

Comment on attachment 294969 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:-682
>> -                defaults: node.defaults.map(this._createInternalSyntaxTree, this),
> 
> Ditto. Was this replaced with something else?

Yes this was mentioned in the ChangeLog. Now, `params` has an AssignmentPattern if there are default values, instead of this separate, out of flow `defaults` list.

[Joseph Pecoraro]

> What about adding tests for:
> 
> 1. async computed properties
> 2. async string literal methods
> 3. async number literal methods

It turns out Esprima fails to parse each of these obscure cases. I filed:
https://github.com/jquery/esprima/issues/1626

[Joseph Pecoraro]

Comment on attachment 294969 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:220
> +            if (tokenValue === "async" && node.parent.type === "Property" && node.parent.value.async && token.range[1] !== node.range[1])

I made an adjustment and dropped the node.parent.value.async case in order to handle this case properly:

    o = { async "foo": function(){} }

I'll likewise add a test for it.

[Joseph Pecoraro]

(In reply to comment #8)
> Comment on attachment 294969 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294969&action=review
> 
> > Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:220
> > +            if (tokenValue === "async" && node.parent.type === "Property" && node.parent.value.async && token.range[1] !== node.range[1])
> 
> I made an adjustment and dropped the node.parent.value.async case in order
> to handle this case properly:
> 
>     o = { async "foo": function(){} }

Err on second thought this should not be valid.

[Joseph Pecoraro]

Comment on attachment 294969 [details]
[PATCH] Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:220
>>> +            if (tokenValue === "async" && node.parent.type === "Property" && node.parent.value.async && token.range[1] !== node.range[1])
>> 
>> I made an adjustment and dropped the node.parent.value.async case in order to handle this case properly:
>> 
>>     o = { async "foo": function(){} }
>> 
>> I'll likewise add a test for it.
> 
> Err on second thought this should not be valid.

Esprima incorrectly allows:

    o = {async foo:function(){}}

So I'll revert back to what was in this patch.

[Joseph Pecoraro]

Went with: Esprima@7219731 which fixed a few issues around async function syntax.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/209491