Add support for line and column number context for errors: #86

[amb26]

Hi - here is my proposed fix for #86 introducing support for line and column numbers.

I've introduced a new "errorSpec" system which allows us to write little JSON5 files with the "errorSpec" suffix alongside parse fail ".txt" files holding assertions about the error contents - this will allow us to write tests for #69 easily if we want to address it later.

I'm sorry I had to subvert the "exports" style for mocha slightly since it didn't sit nicely with the dynamism relating to whether we have an errorSpec file or not - I could try harder to preserve it, but I think it might be a little messy either way.

Thanks for any feedback!

[jordanbtucker]

This is some good work!

I do see one issue just from looking at the code. It only treats \n as a new line. In the vast majority of cases, this will suffice, but in the rare case where only \r is used for new lines, your code will always report errors as appearing on line 1.

Please see the ES5 spec on Line Terminators and the JSON5 code that handles new lines in strings.

As for cosmetics, can you please convert the prefixed increment operators ++ to postfix and remove the space between them and their variables, e.g. lineNumber++ instead of ++ lineNumber. Thanks.

[amb26]

Hi - thanks for the speedy and helpful review! I've added support for all the line terminators in the ES5 spec - I didn't update the code for handling newlines in strings to match since this looked like a potentially breaking strategy change that presumably we should discuss separately. Addressed cosmetics also - ready for another round of feedback, cheers!

PS - I thought it a bit less fussy to test with internal strings for these cases rather than lug around a set of very hard-to-edit files containing nonstandard line terminators, see what you think

[amb26]

I see indeed that the "nonstandard line terminators within strings" issue is being tracked separately at #70 so just as well I left it alone :)

[amb26]

OK - looks like I have indeed violated the published grammar in a minor way, in that U+2028 and U+2029 are now at least recognised as whitespace even though they are not recognised as line terminators (for the purpose of continuation). This seemed better than the current behaviour of throwing a syntax error when encountering these characters. I guess if this PR is accepted, this would require us to update the JSON5Whitespace production in our grammar.

[jordanbtucker]

I've thought about this for a while today, and I came to the conclusion that determining what constitutes a new line is not the job of ES5, JSON, or JSON5, but it is a job for Unicode.

Regardless of what whitespace and new lines are allowed in JSON5, errors should report line numbers based on what the JSON5 document (valid or not) would look like in a text editor.

We should use Unicode's Newline Guidelines (Section 5.8) for determining what characters (or sequence of characters) are new lines. Namely \r\n, \r, \n, \x85, \v, \f, \u2028, and \u2029.

All that said, I think we should remove the changes to whitespace from this PR and keep that as a separate issue. I've got more to say on that matter anyway.

[jordanbtucker]

Let's use peek() here instead of ch2. No need to introduce a one-time variable.

[jordanbtucker]

Also, \r by itself is a valid new line.

[amb26]

The rule here is simply taken from the ES5 LineTerminatorSequence production - https://es5.github.io/#x7.3 - we do indeed recognise \r as a valid newline as the test case verified, but we need to ensure not to recognise \r\n as two newlines.

[amb26]

Thanks for the feedback, @jordanbtucker - I've reverted the changes to the whitespace rules as you suggested - this involved axing our two testcases for the PS and LS line terminators since these now represent syntax errors. I think your plan for universally moving to support for Unicode line terminators both within and outside strings is a reasonable one. Ready for another round - cheers.

[jasonswearingen]

it would be very nice if we got this merged in. non-useful error messages are a very big pain point.