Bug 142233 - Web Inspector: Destructuring function parameters should show type information
Summary: Web Inspector: Destructuring function parameters should show type information
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-03 13:45 PST by Saam Barati
Modified: 2022-03-01 02:47 PST (History)
7 users (show)

See Also:


Attachments
patch (9.97 KB, patch)
2015-03-03 14:03 PST, Saam Barati
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
patch (10.64 KB, patch)
2015-03-06 17:27 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (10.60 KB, patch)
2015-03-06 17:49 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (10.60 KB, patch)
2015-03-07 10:12 PST, Saam Barati
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-03-03 13:45:39 PST
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.
Comment 1 Radar WebKit Bug Importer 2015-03-03 13:46:03 PST
<rdar://problem/20027306>
Comment 2 Saam Barati 2015-03-03 14:03:57 PST
Created attachment 247791 [details]
patch
Comment 3 Brian Burg 2015-03-03 14:06:50 PST
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
Comment 4 Joseph Pecoraro 2015-03-03 14:26:01 PST
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".
Comment 5 Saam Barati 2015-03-06 16:45:30 PST
(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".
Comment 6 Saam Barati 2015-03-06 16:57:59 PST
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.
Comment 7 Saam Barati 2015-03-06 17:27:18 PST
Created attachment 248117 [details]
patch
Comment 8 Saam Barati 2015-03-06 17:49:24 PST
Created attachment 248120 [details]
patch

Should apply to ToT now.
Comment 9 Saam Barati 2015-03-07 10:12:03 PST
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.
Comment 10 Joseph Pecoraro 2015-03-09 20:51:48 PDT
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"
Comment 11 Saam Barati 2015-03-10 11:48:01 PDT
landed in:
http://trac.webkit.org/changeset/181331