Bug 142233 – Web Inspector: Destructuring function parameters should show type information

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

[Saam Barati]

Currently, we don't recursively gather identifiers for restructuring assignments as function arguments.

Also, there is some duplicated code between the ScriptSyntaxTree and TypeTokenAnnotator for how they
traverse the AST and decide which nodes to gather type information for. There is no need for this duplication. 
ScriptSyntaxTree should be the canonical version that decides which nodes to gather type information for, and 
the type token annotator should just receive an array of such nodes that it should update from ScriptSyntaxTree.

Also, there was a subtle bug that will be fixed. Currently, if a function returns some type, say an Int, and we display
in the UI that it displays an Int, if that function ever returns a type other than an Int, the UI will not update because
there was a faulty condition inside an if statement in type token annotator. This is taken care of in this patch too.

[Radar WebKit Bug Importer]

<rdar://problem/20027306>

[Saam Barati]

Created attachment 247791 [details]
patch

[Brian Burg]

Comment on attachment 247791 [details]
patch

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

> Source/WebInspectorUI/ChangeLog:9
> +        just an oversight not have this in bug 141215.

Grammar

[Joseph Pecoraro]

Comment on attachment 247791 [details]
patch

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

Is it possible / worth writing a test for this?

> Source/WebInspectorUI/ChangeLog:8
> +        JSC supports destructuring arguments, and so should we. This was

What is destructuring arguments? Is this supposed to be destructuring assignments?

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:110
> +        if (!node.attachments.__typeToken
> +                && (scriptSyntaxTree.containsNonEmptyReturnStatement(node.body) || !WebInspector.TypeSet.fromPayload(functionReturnType).isContainedIn(WebInspector.TypeSet.TypeBit.Undefined))) {

Style: Can just b done line. The line-break doesn't help much.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:237
> +                for (var identifier of this._gatherIdentifiersInDeclaration(node.id)) {

Err, shouldn't this be calling with "node" and not "node.id"?

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:275
> +    _gatherIdentifiersInDeclaration: function (node) {

Style: "function (node)" => "function(node)".
Style: Brace should be on its own line.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:300
>              }

Typo on line 298: "Unexecpted" => "Unexpected".

[Saam Barati]

(In reply to comment #4)
> Comment on attachment 247791 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247791&action=review
> 
> Is it possible / worth writing a test for this?
> 
It's both possible and probably worth doing inside JSC. This way we can ensure
the data JSC sends the inspector is correct. I think it's possible from within
the inspector to write tests too, but it's more difficult and would require some
scaffolding to be created.

> > Source/WebInspectorUI/ChangeLog:8
> > +        JSC supports destructuring arguments, and so should we. This was
> 
> What is destructuring arguments? Is this supposed to be destructuring
> assignments?

Not sure what the proper ECMAScript name for it is, but basically:

function foo({x, y: hello}) { return x + hello }
foo({x:20, y: 40})

or

function foo([a, b]) { return a * b }
foo([10, 20])

> 
> > Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:110
> > +        if (!node.attachments.__typeToken
> > +                && (scriptSyntaxTree.containsNonEmptyReturnStatement(node.body) || !WebInspector.TypeSet.fromPayload(functionReturnType).isContainedIn(WebInspector.TypeSet.TypeBit.Undefined))) {
> 
> Style: Can just b done line. The line-break doesn't help much.
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:237
> > +                for (var identifier of this._gatherIdentifiersInDeclaration(node.id)) {
> 
> Err, shouldn't this be calling with "node" and not "node.id"?

No, I've changed the function to recurse on a node that's an
"Identifier". I'm not sure Identifier is the proper name
here, I'm sure there is some other name in ECMAScript's grammar,
but basically an expression that can be the X in "var X = expr".
So, X can be:
var ident = expr
var {y: yyy} = expo
var [x,y] = expr
etc.

Previously, this function would receive a VariableDeclarator AST node and
recurse on that node's node.id property, but, that was too specific 
because it rules out recursing on a function's formal parameters which
can also be an "Identifier" in the sense defined above for "X".

I'm going to add an assert to make sure the node passed into this function
meets my above definition of "Identifier".

[Saam Barati]

Looks like ECMA grammar has some names for it based on context,
but basically, X in var X can be:
"BindingIdentifier" or "BindingPattern" which is basically a name
or a pattern.

and function foo(X) {}, X can be a
"BindingElement" which is either a pattern or a name.

We will have to support more AST nodes when JSC gets
ES6 function rest parameter.

[Saam Barati]

Created attachment 248117 [details]
patch

[Saam Barati]

Created attachment 248120 [details]
patch

Should apply to ToT now.

[Saam Barati]

Created attachment 248157 [details]
patch

Change misleading name of bug to indicate that this is a function definition's parameters
and not a function call's arguments.

[Joseph Pecoraro]

Comment on attachment 248157 [details]
patch

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

r=me

> Source/WebInspectorUI/ChangeLog:10
> +        and so should the type profiler in the Inspector. This was just an oversight 
> +        not have this in bug 141215.

Grammar: "an oversight not have this"

> Source/WebInspectorUI/ChangeLog:14
> +        Before, both these classes were responsible for traversing the AST

Grammar: "both these classes" => "both of these classes"

[Saam Barati]

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