Web Inspector: Type bubbles missing for computed methods and methods on object literals

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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

[Radar WebKit Bug Importer]

<rdar://problem/22470287>

[Joseph Pecoraro]

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());

[Joseph Pecoraro]

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).

[Saam Barati]

(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 "[".

[Saam Barati]

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

[Saam Barati]

Created attachment 261788 [details]
patch

[Saam Barati]

Created attachment 261789 [details]
patch

[Joseph Pecoraro]

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.

[Saam Barati]

(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.

[Saam Barati]

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