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.
<rdar://problem/20027306>
Created attachment 247791 [details] patch
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 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".
(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".
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.
Created attachment 248117 [details] patch
Created attachment 248120 [details] patch Should apply to ToT now.
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 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"
landed in: http://trac.webkit.org/changeset/181331