931475 - Make error message of JSON.parse() more informative

Status: NEW

(Core :: JavaScript Engine, enhancement)

Description

[Masaya Iseki[:isk](UTC+9)]

This bug is derived from bug507998.
Bug507998 indicate position which caused error.
Next step is to improve error message.

Comment 1

[Masaya Iseki[:isk](UTC+9)]

Attachment: Bug931475.patch

I change error message and collect error message using macro.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8355201 [details] [diff] [review]
Bug931475.patch

Review of attachment 8355201 [details] [diff] [review]:
-----------------------------------------------------------------

I don't believe we want this.  Changing from passing readable-in-context strings to passing numbers-that-associate-with-strings whose strings you have to look up elsewhere is a regression in readability.  And it's not clear to me there's some special value to using numbers instead of strings.  (As noted on IRC, identical strings will be folded into a single string by all reasonable compilers/linkers.)  So unless I'm missing some other benefit, this patch isn't desirable.

Comment 3

[Masaya Iseki[:isk](UTC+9)]

Attachment: Bug931475.patch

Thank you for your reviewing.

I change error *number* into error *message*.

Comment 4

[Masaya Iseki[:isk](UTC+9)]

Attachment: IRC-Waldo-isk.txt

This text is IRC log with Waldo.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8356362 [details] [diff] [review]
Bug931475.patch

Review of attachment 8356362 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsonparser.cpp
@@ +429,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected ',' or '}' after property value in object");

Wait, this isn't right.  We're just past an object-open, so aren't we looking for either '}' or '"' to start a property name?  So maybe "expected '\"' or '}' within object but ran out of data" instead.

@@ +480,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected ',' or ']' end of data");

"expected ',' or ']' after array element but ran out of data" perhaps.  Certainly "end of data" there doesn't quite follow in English after "expected foo or bar".

@@ +506,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected property name end of data");

Same English thing here, too.  Maybe "expected '\"' and a property name but ran out of data"?

@@ +525,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected ':' after property name in object");

"expected ':' after property name in object but ran out of data"?

Comment 6

[Masaya Iseki[:isk](UTC+9)]

Attachment: Bug931475.patch

Thank you for your reviewing.

Sorry, I sent incomplete patch.
New error string is added in this patch.

I think error message should indicate what programmer do to pass compiler such as "expcted ...".
Only when there are many pattern that programmer do to pass compiler, error message indicate why can't compile such as "unexpected keyword".

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8356905 [details] [diff] [review]
Bug931475.patch

Review of attachment 8356905 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the horrendous delay here.  :-(  Distraction by other things, then a horrible security issue that took up multiple months of my life, got in the way.

I kind of hate to nitpick much when this has already been iterated on a few times.  But it occurs to me, looking at this fresh, that when end of data is reached, the exact thing expected next is often less important than that end of data was reached.  For simple cases like '"fooo' adding the expected '"' will fix things.  But if the string is in the middle of an array, in the middle of an object, etc. then really hit-end-of-data is the important part.  So we should probably emphasize the end-of-data issue more than the immediate apparent concern.  Do you agree?

I promise to be much, much faster the next time around here.  :-(  Give me your thoughts, even if not in the form of a revised patch, and I'll make sure this gets done quickly.

::: js/src/jsonparser.cpp
@@ +114,5 @@
>       *   /^"([^\u0000-\u001F"\\]|\\(["/\\bfnrt]|u[0-9a-fA-F]{4}))*"$/
>       */
>  
>      if (++current == end) {
> +        error("expected \" at end of string");

So I wonder: in some cases this is perfectly informative, like:

  JSON.parse('"foobar');

But it seems likely to me that often, the unterminated string literal here will actually be because the incoming data got truncated somehow.  So the situation might more often be:

  JSON.parse('{"prop1":15,"prop2":25,"pr');

It's technically true that a terminating " was expected.  But it doesn't really illuminate the actual problem.

What do you think about, instead, having the error be

  error("hit end of JSON data in the middle of a string");

?

@@ +231,5 @@
>                  break;
>          }
>      } while (current < end);
>  
> +    error("expected \" at end of string");

This is only hit if the string is unterminated, again.  And actually, I think we could totally *remove* the end-of-string test you previously changed, replacing it with just a plain old

   ++current;

because subsequent code never assumes |*current| is valid.  Seems worth doing, to not have to repeat this error message.

@@ +250,5 @@
>      bool negative = *current == '-';
>  
>      /* -? */
>      if (negative && ++current == end) {
> +        error("expected number after minus sign");

error("hit end of data after the negative sign in a number");

@@ +291,5 @@
>  
>      /* (\.[0-9]+)? */
>      if (current < end && *current == '.') {
>          if (++current == end) {
> +            error("expected digits after decimal point");

error("hit end of data after the decimal point in a number");

@@ +307,5 @@
>  
>      /* ([eE][\+\-]?[0-9]+)? */
>      if (current < end && (*current == 'e' || *current == 'E')) {
>          if (++current == end) {
> +            error("expected digits after exponent indicator");

error("hit end of data after the exponent indicator in a number");

@@ +312,5 @@
>              return token(Error);
>          }
>          if (*current == '+' || *current == '-') {
>              if (++current == end) {
> +                error("expected digits after exponent sign");

error("hit end of data after the exponent sign in a number");

@@ +429,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected '\"' or '}' within object but ran out of data");

error("hit end of data inside object literal");

@@ +480,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected ',' or ']' after array element but ran out of data");

error("hit end of data after an element in an array");

@@ +506,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected '\"' and a property name but ran out of data");

error("hit end of data after a comma in an object literal");

@@ +525,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected ':' after property name in object");

error("hit end of data while looking for ':' after a property name in an object literal");

@@ +546,5 @@
>  
>      while (current < end && IsJSONWhitespace(*current))
>          current++;
>      if (current >= end) {
> +        error("expected ',' or '}' after property value in object");

error("hit end of data after a property value in an object, while looking for ',' or '}'");

Comment 8

[jamie.fallon]

Trying to debug a JSON.parse error(). However the error does not happen when I step through code. So this change would be useful if I could knew how to use it!

Comment 9

[Sebastian Zartner [:sebo]]

Is this still being worked on?

Sebastian

Comment 10

[Masaya Iseki[:isk](UTC+9)]

Sorry, now I don't have a time to work on this task.