Add a pretty printer to improve the error message non-function calls

Add a pretty printer to improve the error message non-function calls

BUG=259443
LOG=y

Committed: https://crrev.com/ac2b1cea9718d35ffddac5e5a656e6669296e404
Cr-Commit-Position: refs/heads/master@{#26189}

[Toon Verwaest, 2015-01-19 16:34:28]

verwaest@chromium.org changed reviewers:
+ yangguo@chromium.org

[Toon Verwaest, 2015-01-19 16:34:29]

PTAL

[caitp (gmail), 2015-01-19 16:44:57]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2015-01-19 16:44:58]

https://codereview.chromium.org/861623002/diff/120001/test/webkit/fast/js/toS...
File test/webkit/fast/js/toString-overrides-expected.txt (right):

https://codereview.chromium.org/861623002/diff/120001/test/webkit/fast/js/toS...
test/webkit/fast/js/toString-overrides-expected.txt:31: FAIL
[1].toLocaleString() should be 1. Threw exception TypeError: (intermediate
value).toLocaleString is not a function
Just my opinion, but I think `(intermediate value).toLocaleString is not a
function` feels less informative than the old error

For code like `a.b()` where b is a string, it would be nice if the error was
more like "TypeError() `a.b` (string "toLocaleString") is not a function".

[Toon Verwaest, 2015-01-19 17:42:26]

I'm not sure what you are asking exactly. The previous error was most of the
time "undefined is not a function". Now we match what Firefox does.

[caitp (gmail), 2015-01-19 17:52:41]

On 2015/01/19 17:42:26, Toon Verwaest wrote:
> I'm not sure what you are asking exactly. The previous error was most of the
> time "undefined is not a function". Now we match what Firefox does.

I'm saying a few things:

1. having `(intermediate value).` in the message looks kind of confusing, I'm
not sure this would be more helpful for web developers (this might not actually
be in the produced error message, but that's what it looks like from the tests)

2. It would be really nice if the identifier / properties where the call
occurred was deserialized and included in the message (ie, "a.b (<<type>>
<<pretty value>>) is not a function"more informative than "<<pretty value>> is
not a function". It would be nice

[Toon Verwaest, 2015-01-19 18:07:02]

The (intermediate value) occurs depending on the receiver expression. If the
receiver expression is, say, the variable a, and we call method b, it'll say a.b
is not a function.

Sure, we could still include the type of the function, but it's probably
undefined in most cases.

[paulirish, 2015-01-19 21:38:24]

Taking the example from the test here..

    RegExp.prototype.toLocaleString = "invalid";
    [/r/].toLocaleString()  

Currently, 

Chrome : TypeError: string is not a function
Firefox: TypeError: (intermediate value).toLocaleString is not a function
Safari : TypeError: invalid is not a function (evaluating
'[/r/].toLocaleString()')


Another example:

    i = 'invalid', window[i]();

Chrome : TypeError: undefined is not a function
Firefox: TypeError: window[i] is not a function
Safari : TypeError: undefined is not a function (evaluating 'window[i]()')


Example from http://crbug.com/406987

    var foo; foo.bar.baz();

Chrome : TypeError: Cannot read property 'bar' of undefined
Firefox: TypeError: TypeError: foo is undefined
Safari : TypeError: TypeError: undefined is not an object (evaluating 'foo.bar')


The developer's next step when getting a message like this 1) identifying what
value is currently undefined and 2) backtracking to why. 
For this CL we want to assist with this first step, so providing any context
about the original JS source would be most helpful.

So, is there any way to adapt the above work to include the original JS source,
like what Safari has done?

[Toon Verwaest, 2015-01-19 21:50:40]

The reason why I opted for Firefox style is that it limits the output to useful
info, rather than just a verbatim copy of the input source. Verbatim copy
doesn't work well with newlines and comments. Safari e.g., does the following:

TypeError: undefined is not a function (evaluating '(function() {

/* bla */

}).f()')

[paulirish1, 2015-01-19 22:08:39]

I understand the thrust, but I think the verbatim copy helps orient the
developer better than anything. Comments and newlines doesn't happen too often
inside of an expression, but regardless, that extra context helps me mentally
map the error back to my original source much easier than the basic
"(intermediate value).f is not a function". 


For this example, the DevTools could handle formatting so we could output
something like:

  TypeError: (intermediate value).f is not a function                
file.js:169
             Thrown while evaluating:
                 (function() {                         
                   /* bla */
                 }).f()

[Toon Verwaest, 2015-01-19 22:22:19]

So you suggest having both? Otherwise I can also rip out the ast visitor again
since that's more complex than just printing the source.

[paulirish1, 2015-01-20 00:57:21]

Yeah I suggest having both. The AST visitor gives much better immediate errors,
but I see the source as a necessary addition.

[Yang, 2015-01-20 08:19:07]

Looking good. I got some comments.

That aside, I got this weird behavior:

[1, 2][0]()
         ^
TypeError: (intermediate value)(intermediate value)[0] is not a function

https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc
File src/prettyprinter.cc (right):

https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc#ne...
src/prettyprinter.cc:47: Print("(intermediate value)");
I'd use square brackets like we do when we print source of native functions:

Math.sin.toString()
"function sin() { [native code] }"

https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc#ne...
src/prettyprinter.cc:201: if (node->init() != NULL) {
omitting curly brackets would look nicer :)

https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:2732: RUNTIME_FUNCTION(Runtime_RenderCallSite) {
Nit: runtime-debug.cc is used for debugger related stuff. This would belong to
runtime-internal.cc where the other stack trace and message related stuff are.

https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:2738: CompilationInfoWithZone
info(location.script());
If you could get the JSFunction from within ComputeLocation, and use it to
create the CompilationInfo, I'd imagine that parsing and ast visiting can go
quite a lot faster since you would only be creating AST for that particular
function.

[Yang, 2015-01-20 08:19:36]

On 2015/01/20 08:19:07, Yang wrote:
> Looking good. I got some comments.
> 
> That aside, I got this weird behavior:
> 
> [1, 2][0]()
>          ^
> TypeError: (intermediate value)(intermediate value)[0] is not a function
> 
> https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc
> File src/prettyprinter.cc (right):
> 
>
https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc#ne...
> src/prettyprinter.cc:47: Print("(intermediate value)");
> I'd use square brackets like we do when we print source of native functions:
> 
> Math.sin.toString()
> "function sin() { [native code] }"
> 
>
https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc#ne...
> src/prettyprinter.cc:201: if (node->init() != NULL) {
> omitting curly brackets would look nicer :)
> 
>
https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
> File src/runtime/runtime-debug.cc (right):
> 
>
https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
> src/runtime/runtime-debug.cc:2732: RUNTIME_FUNCTION(Runtime_RenderCallSite) {
> Nit: runtime-debug.cc is used for debugger related stuff. This would belong to
> runtime-internal.cc where the other stack trace and message related stuff are.
> 
>
https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
> src/runtime/runtime-debug.cc:2738: CompilationInfoWithZone
> info(location.script());
> If you could get the JSFunction from within ComputeLocation, and use it to
> create the CompilationInfo, I'd imagine that parsing and ast visiting can go
> quite a lot faster since you would only be creating AST for that particular
> function.

Also a new set of test cases might be nice.

[Yang, 2015-01-20 08:31:03]

On 2015/01/20 08:19:36, Yang wrote:
> On 2015/01/20 08:19:07, Yang wrote:
> > Looking good. I got some comments.
> > 
> > That aside, I got this weird behavior:
> > 
> > [1, 2][0]()
> >          ^
> > TypeError: (intermediate value)(intermediate value)[0] is not a function
> > 
> > https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc
> > File src/prettyprinter.cc (right):
> > 
> >
>
https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc#ne...
> > src/prettyprinter.cc:47: Print("(intermediate value)");
> > I'd use square brackets like we do when we print source of native functions:
> > 
> > Math.sin.toString()
> > "function sin() { [native code] }"
> > 
> >
>
https://codereview.chromium.org/861623002/diff/120001/src/prettyprinter.cc#ne...
> > src/prettyprinter.cc:201: if (node->init() != NULL) {
> > omitting curly brackets would look nicer :)
> > 
> >
>
https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
> > File src/runtime/runtime-debug.cc (right):
> > 
> >
>
https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
> > src/runtime/runtime-debug.cc:2732: RUNTIME_FUNCTION(Runtime_RenderCallSite)
{
> > Nit: runtime-debug.cc is used for debugger related stuff. This would belong
to
> > runtime-internal.cc where the other stack trace and message related stuff
are.
> > 
> >
>
https://codereview.chromium.org/861623002/diff/120001/src/runtime/runtime-deb...
> > src/runtime/runtime-debug.cc:2738: CompilationInfoWithZone
> > info(location.script());
> > If you could get the JSFunction from within ComputeLocation, and use it to
> > create the CompilationInfo, I'd imagine that parsing and ast visiting can go
> > quite a lot faster since you would only be creating AST for that particular
> > function.
> 
> Also a new set of test cases might be nice.

There are some more places where we create that type error: all native functions
that take a closure as argument, for example Array.prototype.forEach and the
likes:

[0].forEach(this.a)
    ^
TypeError: undefined is not a function

[Yang, 2015-01-20 11:18:23]

LGTM with a nit.

The code duplication is a bit lame, but I guess the other option would to heap
allocate it :/

https://codereview.chromium.org/861623002/diff/180001/src/runtime/runtime-int...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/861623002/diff/180001/src/runtime/runtime-int...
src/runtime/runtime-internal.cc:175: CompilationInfoWithZone
info(location.script());
I guess you could also pass the zone into the CompilationInfo and not use the
..WithZone constructor.

[Toon Verwaest, 2015-01-21 10:31:59]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2015-01-21 10:33:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/861623002/200001

[commit-bot: I haz the power, 2015-01-21 10:48:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 10:48:25]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/105)

[Toon Verwaest, 2015-01-21 11:54:23]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2015-01-21 11:55:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/861623002/200001

[commit-bot: I haz the power, 2015-01-21 12:51:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 12:51:59]

Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/838)

[Toon Verwaest, 2015-01-21 13:07:10]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2015-01-21 13:07:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/861623002/200001

[commit-bot: I haz the power, 2015-01-21 13:40:38]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-01-21 13:40:56]

Patchset 11 (id:??) landed as
https://crrev.com/ac2b1cea9718d35ffddac5e5a656e6669296e404
Cr-Commit-Position: refs/heads/master@{#26189}

[Michael Achenbach, 2015-01-21 16:44:28]

This changes layout test expectations for win and mac. Please land an upstream
rebaseline request in blink.

http://build.chromium.org/p/client.v8/builders/V8-Blink%20Win/builds/1954
http://build.chromium.org/p/client.v8/builders/V8-Blink%20Mac/builds/1809

[Toon Verwaest, 2015-01-21 17:20:43]

I did, but I guess I forgot one. On its way:
https://codereview.chromium.org/856393003/