Bug 148562

Summary: Web Inspector: Type bubbles missing for computed methods and methods on object literals
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, graouts, joepeck, mattbaker, nvasilyev, sbarati, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
patch joepeck: review+

Description Joseph Pecoraro 2015-08-27 22:20:26 PDT
* SUMMARY
Type bubbles missing for computed property methods.

* TEST
1 var o = {
2     ["computed"]() {
3         return 10;
4     }
5 }
6 console.log(o.computed());

* RESULTS
- Expected type bubbles for the method "computed" but got none
Comment 1 Radar WebKit Bug Importer 2015-08-27 22:20:55 PDT
<rdar://problem/22470287>
Comment 2 Joseph Pecoraro 2015-08-27 22:25:01 PDT
Seems to be broken for computed and regular methods:

* TEST
var o = {
    ["computed"](){return 10},
    normal(){return 20}
}
console.log(o.computed());
console.log(o.normal());
Comment 3 Joseph Pecoraro 2015-08-28 00:00:04 PDT
There appear to be multiple issues here. I've broken them out into these test cases.

  1. I don't see any divot registered for computed property methods:
      TEST: o = { ["computed"]() { return 1; }; o.computed();
      TEST: o = { ["computed"]   () { return 1; }; o.computed();

  2. Object literal property methods.
      TEST: o = { method1() { return 1; }; o.method1();

  3. Object literal property methods with spaces in the declaration.
      TEST: o = { method1  () { return 1; }; o.method1();

The patch below gets (2), which I think is the common case of object literal property methods, working. It seems the divot for (2) is the last character of the method name, but the divot for (3) is the '(' character. All of these could likely benefit from cleanup in the backend.

> --- a/Source/JavaScriptCore/runtime/TypeProfiler.cpp
> +++ b/Source/JavaScriptCore/runtime/TypeProfiler.cpp
> @@ -783,6 +787,12 @@ WebInspector.ScriptSyntaxTree = class ScriptSyntaxTree extends WebInspector.Obje
>              if (result.kind === "get" || result.kind === "set") {
>                  result.value.isGetterOrSetter = true;
>                  result.value.getterOrSetterRange = result.key.range;
> +            } else if (node.method) {
> +                // FIXME: <https://webkit.org/b/143171> Web Inspector: Improve Type Profiler Support for ES6 Syntax
> +                // The backend has confusing handling of the divot here.
> +                // Though this is not always result.key.range[1], it often is.
> +                result.value.isGetterOrSetter = true;
> +                result.value.getterOrSetterRange = [result.key.range[1], result.value.range[1]];
>              }
>              break;
>          case "ReturnStatement":

Saam, maybe you can advise me here, since I understand this is a particular tricky area of Type Profiling (divots).
Comment 4 Saam Barati 2015-08-28 15:16:28 PDT
(In reply to comment #3)
> There appear to be multiple issues here. I've broken them out into these
> test cases.
> 
>   1. I don't see any divot registered for computed property methods:
>       TEST: o = { ["computed"]() { return 1; }; o.computed();
>       TEST: o = { ["computed"]   () { return 1; }; o.computed();
> 
>   2. Object literal property methods.
>       TEST: o = { method1() { return 1; }; o.method1();
> 
>   3. Object literal property methods with spaces in the declaration.
>       TEST: o = { method1  () { return 1; }; o.method1();
> 
> The patch below gets (2), which I think is the common case of object literal
> property methods, working. It seems the divot for (2) is the last character
> of the method name, but the divot for (3) is the '(' character. All of these
> could likely benefit from cleanup in the backend.
> 
> > --- a/Source/JavaScriptCore/runtime/TypeProfiler.cpp
> > +++ b/Source/JavaScriptCore/runtime/TypeProfiler.cpp
> > @@ -783,6 +787,12 @@ WebInspector.ScriptSyntaxTree = class ScriptSyntaxTree extends WebInspector.Obje
> >              if (result.kind === "get" || result.kind === "set") {
> >                  result.value.isGetterOrSetter = true;
> >                  result.value.getterOrSetterRange = result.key.range;
> > +            } else if (node.method) {
> > +                // FIXME: <https://webkit.org/b/143171> Web Inspector: Improve Type Profiler Support for ES6 Syntax
> > +                // The backend has confusing handling of the divot here.
> > +                // Though this is not always result.key.range[1], it often is.
> > +                result.value.isGetterOrSetter = true;
> > +                result.value.getterOrSetterRange = [result.key.range[1], result.value.range[1]];
> >              }
> >              break;
> >          case "ReturnStatement":
> 
> Saam, maybe you can advise me here, since I understand this is a particular
> tricky area of Type Profiling (divots).

Sure. There are a few things at play here:
1. We need a better name for isGetterOrSetter. I think this really means how things are formed
textually in the JS program: i.e, there is no "function" keyword, but just "get"/"set"/<methodName>. We should probably
come up with a good name the expresses this idea.

2. It looks like some divots are wrong and should be fixed. The new approach is to have these divots
on the earliest possible character that describes a function: "s" in "set", "g" in "get",
"m" in "method1", etc. So we should use the "[" for computed properties. We should
fix these divots in JSC to be consistent. It looks like (2)/(3) are not consistent.
I'm not sure about (1) but we should make it "[".
Comment 5 Saam Barati 2015-08-28 15:22:30 PDT
It looks like some of the interesting code that does this is in:
ScriptSynaxTree::static functionReturnDivot(node).

I've opened a JSC bug to fix what JSC does:
https://bugs.webkit.org/show_bug.cgi?id=148594
Comment 6 Saam Barati 2015-09-22 18:04:12 PDT
Created attachment 261788 [details]
patch
Comment 7 Saam Barati 2015-09-22 18:09:24 PDT
Created attachment 261789 [details]
patch
Comment 8 Joseph Pecoraro 2015-09-23 09:34:50 PDT
Comment on attachment 261789 [details]
patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:110
> +            var offset = node.typeProfilingReturnDivot;
>              this._insertToken(offset, node, true, WebInspector.TypeTokenView.TitleType.ReturnStatement, functionName);

Nit: Could eliminate the temporary variable now.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:160
> +        // "f" in "function", "s" in "set", "g" in "get", first letter in any method name for classes, the "[" for computed properties.

Nit: Methods can be in Object Literals as well as Classes.
Nit: So the "[" for computed is only for method syntax as well? ({ ["x"](){} }) versus ({ ["x"]: function(){} })

Having a test that covers just these cases (makeNode(...) check typeProfilingReturnDivot) would be fantastic, but I won't block this on a test.
Comment 9 Saam Barati 2015-09-23 10:12:45 PDT
(In reply to comment #8)
> Comment on attachment 261789 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261789&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:110
> > +            var offset = node.typeProfilingReturnDivot;
> >              this._insertToken(offset, node, true, WebInspector.TypeTokenView.TitleType.ReturnStatement, functionName);
> 
> Nit: Could eliminate the temporary variable now.
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:160
> > +        // "f" in "function", "s" in "set", "g" in "get", first letter in any method name for classes, the "[" for computed properties.
> 
> Nit: Methods can be in Object Literals as well as Classes.
> Nit: So the "[" for computed is only for method syntax as well? ({ ["x"](){}
> }) versus ({ ["x"]: function(){} })
> 
> Having a test that covers just these cases (makeNode(...) check
> typeProfilingReturnDivot) would be fantastic, but I won't block this on a
> test.
I actually only tested the computer method syntax. I'll make sure everything
works for both syntax types and write tests for them.
Comment 10 Saam Barati 2015-09-23 13:59:30 PDT
landed in:
http://trac.webkit.org/changeset/190184