Bug 143171 – Web Inspector: [ES6] Improve Type Profiler Support for Arrow Functions

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

[Joseph Pecoraro]

* SUMMARY
Improve Type Profiler Support for ES6 Syntax.

  • ScriptSyntaxTree should support ES6 syntax that Esprima produces
    - template strings
    - modules, packages (export/import)
    - arrow functions
    - binding patterns
    - comprehensions? (ES7?)

  • BasicBlockAnnotator for method syntax is a bit off
    - closing brace doesn't appear to be included

[Radar WebKit Bug Importer]

<rdar://problem/20335738>

[Joseph Pecoraro]

Retitling to be specifically about Arrow Functions. I think we've handled the rest for ES6, and Block Annotations is aa separate issue.

[Saam Barati]

Created attachment 261768 [details]
patch

[WebKit Commit Bot]

Attachment 261768 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/TypeProfiler.h:107:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 261768 [details]
patch

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

> Source/JavaScriptCore/runtime/TypeProfiler.h:73
> +    }
> +    bool operator==(const QueryKey& other) const

Style: Normally there are newlines between multi-line member functions.

> Source/JavaScriptCore/tests/typeProfiler/arrow-functions.js:4
> +let foo = (x) => x;
> +let bar = abc => abc;

Might want to also test a case with {}s.

    let baz = x => { return x }

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:152
> +        console.assert( node.type === WebInspector.ScriptSyntaxTree.NodeType.FunctionDeclaration || node.type === WebInspector.ScriptSyntaxTree.NodeType.FunctionExpression || node.type === WebInspector.ScriptSyntaxTree.NodeType.MethodDefinition || node.type === WebInspector.ScriptSyntaxTree.NodeType.ArrowFunctionExpression); 

Style: Unnecessary leading space inside the assert(...).

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:372
> +        case WebInspector.ScriptSyntaxTree.NodeType.ArrowFunctionExpression:
>              callback(node, state);
>              this._recurse(node.id, callback, state);
>              this._recurseArray(node.params, callback, state);

r- ArrowFunctionExpression has "expression" but FunctionExpression/FunctionDeclaration don't. We'll need to walk over that. Either a new case, or check `if (node.expression)`.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:955
> +    ArrowFunctionExpression: Symbol("arrow-function-expression"),

Nit: Alphabetical order (`sort`)

We should add a basic test for the frontend's new node type in:
LayoutTests/inspector/model/parse-script-syntax-tree.html

[Joseph Pecoraro]

Comment on attachment 261768 [details]
patch

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

>> Source/JavaScriptCore/tests/typeProfiler/arrow-functions.js:4
>> +let bar = abc => abc;
> 
> Might want to also test a case with {}s.
> 
>     let baz = x => { return x }

And maybe even an empty case? () => {}

[Saam Barati]

Created attachment 261782 [details]
patch

Fixed comments.
Also noted in comments that the "expression" field is just a boolean. Not an AST node.

[Joseph Pecoraro]

Comment on attachment 261782 [details]
patch

r=me!

[Saam Barati]

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