Re: [PATCH 5/5] json-parser: add location to JSON parsing errors

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Now that all calls to parse_error have a token, add the line and column

Enabled by PATCH 2 pushing the JSON_END_OF_INPUT to the parser.  Before,
the parser detected end of input by running out of tokens.  It passed a
null token to parse_error() then.

> to the message.  As far as I can see the two important TODOs (better
> errors and better EOI handling) are done, and the others (token range
> information and "parsed size"?) do not really matter or are handled
> better by json-streamer.c.  So remove the list, which had sat unchanged
> since 2009.

Adding my remarks to the patch hunk below.  May lead to commit message
adjustments.

An example showing error before and after would be nice.

> Note however that right now the x and y are those of the *last* character
> in the token.  Modify json-lexer.c to freeze tok->x and tok->y at the
> first character added to the GString.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qobject/json-parser.h |  1 +
>  qobject/json-lexer.c          | 11 +++++++----
>  qobject/json-parser.c         | 12 ++----------
>  3 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/include/qobject/json-parser.h b/include/qobject/json-parser.h
> index 923eb74ca00..98fe1371a85 100644
> --- a/include/qobject/json-parser.h
> +++ b/include/qobject/json-parser.h
> @@ -17,6 +17,7 @@
>  typedef struct JSONLexer {
>      int start_state, state;
>      GString *token;
> +    int cur_x, cur_y;
>      int x, y;
>  } JSONLexer;
>  
> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
> index 51341d96e49..7753ba6c092 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -277,7 +277,8 @@ void json_lexer_init(JSONLexer *lexer, bool enable_interpolation)
>      lexer->start_state = lexer->state = enable_interpolation
>          ? IN_START_INTERP : IN_START;
>      lexer->token = g_string_sized_new(3);
> -    lexer->x = lexer->y = 0;
> +    lexer->cur_x = lexer->cur_y = 1;
> +    lexer->x = lexer->y = 1;

Not mentioned in the commit message: we switch @ and @y from zero-based
to one-based.  Pefectly safe, because they're both unused before this
patch.

Suggest to rename to line and col.

>  }
>  
>  static void json_lexer_feed_char(JSONLexer *lexer, char ch, bool flush)
> @@ -285,10 +286,10 @@ static void json_lexer_feed_char(JSONLexer *lexer, char ch, bool flush)
>      int new_state;
>      bool char_consumed = false;
>  
> -    lexer->x++;
> +    lexer->cur_x++;
>      if (ch == '\n') {
> -        lexer->x = 0;
> -        lexer->y++;
> +        lexer->cur_x = 1;
> +        lexer->cur_y++;
>      }
>  
>      while (flush ? lexer->state != lexer->start_state : !char_consumed) {
> @@ -316,6 +317,8 @@ static void json_lexer_feed_char(JSONLexer *lexer, char ch, bool flush)
>          case IN_START:
>              g_string_truncate(lexer->token, 0);
>              new_state = lexer->start_state;
> +            lexer->x = lexer->cur_x;
> +            lexer->y = lexer->cur_y;
>              break;
>          case JSON_ERROR:
>              json_message_process_token(lexer, lexer->token, JSON_ERROR,
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 2fede59842f..93b737511d1 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -57,15 +57,6 @@ typedef struct JSONParserStackEntry {
>  
>  #define BUG_ON(cond) assert(!(cond))
>  
> -/**
> - * TODO

These go back all the way to the initial commit.

> - *
> - * 0) make errors meaningful again

The errors weren't meaningless, just bad.  The "again" is kind of funny,
considering it's the initial commit.

I agree with removing the line, but i wonder what exactly improved
beyond line and column ("geometry information").

> - * 1) add geometry information to tokens

Tokens had "geometry information" x, y since day one.  Delete.

> - * 3) should we return a parsed size?

Why we'd be interesed in the parsed size we no longer know.  Delete.

> - * 4) deal with premature EOI

Fixed in commit 11e8a46cc35 (json-parser: detect premature EOI) almost
fourteen years ago.  Delete.

> - */
> -
>  static inline JSONParserStackEntry *current_entry(JSONParserContext *ctxt)
>  {
>      return g_queue_peek_tail(ctxt->stack);
> @@ -105,7 +96,8 @@ static void G_GNUC_PRINTF(3, 4) parse_error(JSONParserContext *ctxt,
>      va_start(ap, msg);
>      vsnprintf(message, sizeof(message), msg, ap);
>      va_end(ap);
> -    error_setg(&ctxt->err, "JSON parse error, %s", message);
> +    error_setg(&ctxt->err, "JSON parse error at line %d, column %d, %s",
> +	       token->y, token->x, message);

The usual format used by code parsing tools such as compilers is like

   "%d:%d: JSON parse error: %s, token->x, token->y, message

Should we use it?

>  }
>  
>  static int cvt4hex(const char *s)