Improve JSON syntax validation messaging

[jdesrosiers]

We currently use jsonc.printParseErrorCode to produce JSON syntax error messages. As shown below, that just returns the error code variable name.

export function printParseErrorCode(code) {
    switch (code) {
        case 1 /* ParseErrorCode.InvalidSymbol */: return 'InvalidSymbol';
        case 2 /* ParseErrorCode.InvalidNumberFormat */: return 'InvalidNumberFormat';
        case 3 /* ParseErrorCode.PropertyNameExpected */: return 'PropertyNameExpected';
        case 4 /* ParseErrorCode.ValueExpected */: return 'ValueExpected';
        case 5 /* ParseErrorCode.ColonExpected */: return 'ColonExpected';
        case 6 /* ParseErrorCode.CommaExpected */: return 'CommaExpected';
        case 7 /* ParseErrorCode.CloseBraceExpected */: return 'CloseBraceExpected';
        case 8 /* ParseErrorCode.CloseBracketExpected */: return 'CloseBracketExpected';
        case 9 /* ParseErrorCode.EndOfFileExpected */: return 'EndOfFileExpected';
        case 10 /* ParseErrorCode.InvalidCommentToken */: return 'InvalidCommentToken';
        case 11 /* ParseErrorCode.UnexpectedEndOfComment */: return 'UnexpectedEndOfComment';
        case 12 /* ParseErrorCode.UnexpectedEndOfString */: return 'UnexpectedEndOfString';
        case 13 /* ParseErrorCode.UnexpectedEndOfNumber */: return 'UnexpectedEndOfNumber';
        case 14 /* ParseErrorCode.InvalidUnicode */: return 'InvalidUnicode';
        case 15 /* ParseErrorCode.InvalidEscapeCharacter */: return 'InvalidEscapeCharacter';
        case 16 /* ParseErrorCode.InvalidCharacter */: return 'InvalidCharacter';
    }
    return '<unknown ParseErrorCode>';
}

In practice, the message gets across sufficiently, but we should provide a better experience. I think we should use a similar approach to what we used for @hyperjump/json-schema-errors, which uses fluent for language translations. But, I'm open to other suggestions.

[Ahmedmhmud]

Hi Jason,
I agree that adding Fluent logic for error message translation would be great.
Following the @hyperjump/json-schema-errors approach seems like the best fit for our case, we can set up a Localization class with caching and map our error codes directly to Fluent message keys.
I addition, we can wire it with the server logic by checking the params in server initialization if there is any locale and pass it to the constructor of SyntaxValidation.
If you're good with that direction, let me know! I'd be happy to take this on and implement it.

[srivastava-diya]

I was looking through this and I think the messages might be a bit worse off than "gets across sufficiently" at least for one common case.

Here's an example with our server running alongside VS Code's built-in JSON support:

The built-in says Trailing comma and points at the comma on line 2. We say PropertyNameExpected and ValueExpected, two errors, both at line 3 col 1, which is the closing brace. Neither mentions a comma.

So I went back and looked at how @hyperjump/json-schema-errors does it. and i could instantly recall that we don't translate the raw validation output there, there's a whole layer first.

That feels like the bit we would want to copy here. Right now the parse codes tell us what the parser wanted next, not what the person did wrong so if we key straight off them, we'd be translating "Property name expected" into every language when the message we want is the one the built-in is already showing.

Could we do the same first step for syntax errors, like work out the actual mistake (trailing comma, unquoted key, missing bracket), collapse the duplicates and hand those to Fluent as something like trailing-comma-message along with an unknown-message style fallback to today's behaviour for anything we don't recognise yet, same as json-schema-errors does.

[srivastava-diya]

we can set up a Localization class with caching and map our error codes directly to Fluent message keys.

hey @Ahmedmhmud thanks for picking this up, this is something I bumped into while looking at it. the tricky bit is that PropertyNameExpected turns up for {"a": 1,}, {a: 1} and {'a': 1} a trailing comma, an unquoted key, and the wrong quote marks. three totally different mistakes, same code.

so if the keys map 1:1 onto the raw codes, the best message we can write for that key is something vague like "property name expected", and there'd be no way to say "trailing comma" for the case that actually is one.

what if we worked out the real mistake first and used those as the keys instead, like trailing-comma-message? am i right about this @jdesrosiers or am i overcomplicating it?

[Ahmedmhmud]

Yup, @srivastava-diya you're right thanks!
this is a jsonc-parser limitation. We can add a small classification layer on top of the ParseError list before we map errors to Fluent keys.
We can use the document text and the error offset to detect each case, and the logic should stay simple since it only needs lightweight text inspection around the error position.
I’d be happy to implement that if we all agree to move forward with this approach.

[jdesrosiers]

Great insight! I'm sure this is why vscode implements it's own JSON parsing. It's still based on jsonc-parser but uses some of the low level tools to parse. It might be a good idea to use that as a starting point and modify for our needs. That way we can provide the same messaging they do. One modification I think we would want to make is that vscode defines its own JSON AST. I think we would want to stick with the AST jsonc-parser uses. That way we can make use of the utilities it provides and we don't have to change all of the code we've already written that expects that AST.

[srivastava-diya]

Could this be two PRs/issues? There's a natural split, the parsing decides which mistake it is, and the messaging turns that into text in the language. They're sort of independent once we agree what passes between them.

• So PR 1 could be the parser, we can use jsonc-parser's low level pieces the way vscode does, and emit a specific mistake like trailing comma, unquoted key, missing comma etc with a range, instead of the current code.
• PR 2 could be the Fluent side, message keys per mistake, translations etc.

@jdesrosiers does that split look right to you, or would you rather keep it as one PR? if the split does seem right to you then I'd be up for the parser half, and if it makes more sense for @Ahmedmhmud to take the whole thing as one piece, that works just as well, I don't mind either way.

[Ahmedmhmud]

I find the splitting seems like a good idea, and I looked into how VS Code handles this and I agree that we should follow the same approach: use jsonc-parser’s scanner for the tokens, adapt VS Code’s parse() logic for more specific errors, and keep jsonc-parser’s existing AST. We can also preserve the generic diagnostics through a callback when the new parser cannot provide a more specific message.

And like @srivastava-diya says I don't mind the both choices of me implementing the Fluent issue or taking this as one piece, as you prefer.

[jdesrosiers]

Splitting the work sounds fine to me. I just assigned @Ahmedmhmud another task, so it makes sense for @srivastava-diya to take at least the first part (the parser) while @Ahmedmhmud is working on the other issue.

@srivastava-diya could you also create a couple of sub-issues so we can track each step individually?