search

tc39 / proposal-json-parse-with-source

Proposal for extending JSON.parse to expose input source text.
https://tc39.github.io/proposal-json-parse-with-source
MIT License
213 stars 9 forks source link

Stage 3 Specification Review: Waldemar Horwat #29

Closed gibson042 closed 2 years ago

gibson042 commented 2 years ago

This is a placeholder task for the Stage 3 Specification Review feedback from @waldemarhorwat.

waldemarhorwat commented 2 years ago

Here's my review. I just have a couple editorial comments.

The ShallowestContained algorithm doesn't work in general. It may happen to work for the JSON subset, but it forms an attractive nuisance that hides land mines for others who might call it in the future. For example,

ShallowestContained called on the parse of "class C {x=4; [true]=8}" with types «NumericLiteral» doesn't find any numeric literals. But if we add searching for an unrelated nonterminal, it suddenly starts to find them: ShallowestContained called on the parse of "class C {x=4; [true]=8}" with types «NumericLiteral, BooleanLiteral» finds the numeric literal 4.

ArrayElementList is a bit broken in general too. If elisions are present, the length of the list generated by ArrayElementList is not necessarily equal to the array's length because a single Elision can contain multiple commas but ArrayElementList treats it as a single element. However, the JSON syntax doesn't allow elisions, so it happens to work on JSON arrays.

Possible solutions are to either fix these so they work in general or rename them so it's clear they are only usable on JSON objects.

gibson042 commented 2 years ago

The ShallowestContained algorithm doesn't work in general. It may happen to work for the JSON subset, but it forms an attractive nuisance that hides land mines for others who might call it in the future. For example,

ShallowestContained called on the parse of "class C {x=4; [true]=8}" with types «NumericLiteral» doesn't find any numeric literals. But if we add searching for an unrelated nonterminal, it suddenly starts to find them: ShallowestContained called on the parse of "class C {x=4; [true]=8}" with types «NumericLiteral, BooleanLiteral» finds the numeric literal 4.

Thank you, this is an excellent point. Please review #30, which renames the operation to ShallowestContainedJSONValue and narrows its use to JSON.parse.

ArrayElementList is a bit broken in general too. If elisions are present, the length of the list generated by ArrayElementList is not necessarily equal to the array's length because a single Elision can contain multiple commas but ArrayElementList treats it as a single element. However, the JSON syntax doesn't allow elisions, so it happens to work on JSON arrays.

This behavior is intentional, although the length assertion is indeed specific to JSON.parse. For clarity, I have renamed the operation to ArrayLiteralContentNodes in #31.