report syntax errors instead of parser codes - #115
srivastava-diya wants to merge 2 commits into
Conversation
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
Looks great! I make a couple suggestions inline.
A truncated document like
{"a":{"b":{"c":[1,2reports every container left open, so four errors.
👍 These are all distinct problems and we should show them all.
An empty file gives an error with no length
Does an error with no length even show up? Not sure what we could do about it in any case, so it probably doesn't matter.
A number put against a string,
{ "a": 1"b" }, still gives three errors. Odd input, but it's the one case I didn't get down to one.
I think that makes sense. It looks simple, but it's actually multiple errors. There should be a comma after the number, then the string is a property name, so it needs a colon and a value.
So, there's actually a simpler case where there's more than one error: {"a"}. This produces two errors: colon-expected and value-expected. I think that's fine too. There really are two things missing here. The property name has two required things that must always come after it.
comment-not-closed exists but is unreachable today, because we skip comments while scanning. It becomes live if we start reporting comments.
This language server should support JSONC as well as JSON. I think the parser needs to have an option whether or not to allow comments based on the file extension. If it's .json, it should report a syntax error. If it's .jsonc, it should catch things like comment-not-closed.
| test("an unclosed array points at the bracket that was never closed", () => { | ||
| const { errors } = parse(`{ "a": [1, 2 }`); | ||
|
|
||
| expect(errors).toEqual([ | ||
| { code: "bracket-not-closed", offset: 7, length: 1 } | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Could this point to the whole array, or maybe the last item? My thought is that if I have a large array covering many lines, knowing where the array started doesn't do much to help me find where I need to close the array. It would be a lot more helpful if the error could tell me where the missing bracket should be.
There was a problem hiding this comment.
Went with the last item, since that's where the bracket goes. Did the same for unclosed objects, which point at the last property. If it's empty, it falls back to the opening bracket since there's nothing else to point at.
| test("a second top level value points at where the document should have ended", () => { | ||
| const { errors } = parse(`{"a":1} {"b":2}`); | ||
|
|
||
| expect(errors).toEqual([ | ||
| { code: "end-of-file-expected", offset: 8, length: 1 } | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Can we make this point at everything after the document should have ended? All of that is invalid, no just the first character.
There was a problem hiding this comment.
Done, it now covers everything from where the document should have ended to the end of the file, not counting trailing whitespace.
| test("a missing comma points at the member that needed one before it", () => { | ||
| const { errors } = parse(`{ "a": 1 "b": 2 }`); | ||
|
|
||
| expect(errors).toEqual([ | ||
| { code: "comma-expected", offset: 9, length: 3 } | ||
| ]); | ||
| }); | ||
|
|
||
| test("a missing colon points at the value that needed one before it", () => { | ||
| const { errors } = parse(`{ "a" 1 }`); | ||
|
|
||
| expect(errors).toEqual([ | ||
| { code: "colon-expected", offset: 6, length: 1 } | ||
| ]); | ||
| }); | ||
|
|
||
| test("a property with no value points at the token found instead", () => { | ||
| const { errors } = parse(`{ "a": }`); | ||
|
|
||
| expect(errors).toEqual([ | ||
| { code: "value-expected", offset: 7, length: 1 } | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
The positioning of these errors doesn't feel right to me. By positioning them after the error, it feels like it's not pointing at the thing that's actually wrong. I think my feeling is influenced by where whitespace usually goes. For example, when I see the first test, I think, "the 1 needs a comma". The comma is needed because "b" is present, but it's the 1 that gets the comma. Of course the comma isn't really part of the 1, but that's what it looks like with typical whitespace conventions.
I feel like the error should be on the thing before the error instead of after the error. In the first, there should be a comma after 1. In the second, there should be a colon after "a". In the third, there should be value after :.
I think the last test makes the case best if you put it on multiple lines.
{
"a":
}
The error would appear on the }, which means the error doesn't appear on the line you'd make the change on. If the error is on the :, it's more clear where the fix goes.
There was a problem hiding this comment.
Makes sense, moved them. Comma and colon now point at what they should follow, and a missing value points at the colon. I added your multiline example as a test too. I did the same for gaps like [1, , 2], which now point at the first comma.
5047a1a to
0136bac
Compare
2 participants
Closes #109.
What this does
We currently show the user whatever
jsonc-parsercalled the errorValueExpected,PropertyNameExpectedwhich isn't friendly, and often isn't even what they did wrong. The same code covers several different mistakes, so there's no way to turn it into a helpful message.This replaces
jsonc.parseTreewith a parser that reads the document token by token usingjsonc-parser's scanner underneath, and reports what the user actually did. It still produces the same treeparseTreedid, so nothing else in the codebase changes.The problems it fixes
One mistake used to produce several errors.
{ "a": [1, 2 }Four errors before, one now, pointing at the
[that was never closed.{ a: 1 }Three before, one now, pointing at the key.
Errors landed on the wrong thing. A trailing comma was reported at the closing brace. Now it points at the comma.
Some errors had no length, so there was nothing for the editor to underline. Unclosed brackets and braces now point at the opening bracket, which also tells you which one is unclosed. The only one left is an empty file, where there's genuinely nothing to point at.
The error didn't say what was wrong.
ValueExpectedalone covered trailing comma, single-quoted value, missing value, unclosed bracket,NaN,Trueand an empty file. There are now 16 codes that each name one thingtrailing-comma,property-key-not-quoted,string-not-closed,number-invalidand so on.Errors can also carry the text that caused them, so a message can say
"0x1F" is not a valid numberrather than something generic.Multiple genuine mistakes still give multiple errors
{ a: 1, "b" 2, 'c': 3 }gives three.What this doesn't do
No message text or translation. The diagnostic currently shows the code itself as a placeholder, which is what #110 replaces.
Two things I found along the way and deliberately left, since neither needs parser work: we still accept
//comments in.jsonfiles, and we still don't report duplicate keys. I'll raise those separately.Notes
A few judgement calls worth a second opinion:
{"a":{"b":{"c":[1,2reports every container left open, so four errors. Arguably right, since four closers really are missing, but it's the one place "one mistake, one error" doesn't hold. Same count as before.{ "a": 1"b" }, still gives three errors. Odd input, but it's the one case I didn't get down to one.comment-not-closedexists but is unreachable today, because we skip comments while scanning. It becomes live if we start reporting comments.tests
This is a first pass and I expect to restructure it based on feedback. I wrote the cases first as a way of pinning down what each mistake should produce, so the 40 tests are organised by mistake rather than by anything about how the parser is built. Happy to reorganise, rename, split or trim I wanted the behaviour agreed before polishing the layout.