Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Normative: Suppress source text for JSON parse data modified before access #41

Merged
merged 4 commits into from
Sep 18, 2023
Merged

Normative: Suppress source text for JSON parse data modified before access #41

merged 4 commits into from
Sep 18, 2023

Conversation

gibson042
Copy link
Collaborator

Snapshot the initial data structure before exposing it to ECMAScript code.
Fixes #35
Fixes #39

@gibson042 gibson042 force-pushed the gh-39-mutation branch 2 times, most recently from 646d90f to 7fc28f7 Compare January 27, 2023 16:24
@gibson042
Normative: Suppress source text for JSON parse data modified before a…
cc64b74 
…ccess

Snapshot the initial data structure before exposing it to ECMAScript code.
Fixes tc39#35
Fixes tc39#39
@gibson042 gibson042 mentioned this pull request Feb 3, 2023
Open
@gibson042
Copy link
Collaborator Author

@waldemarhorwat @michaelficarra @syg I welcome your reviews.

michaelficarra
michaelficarra reviewed Feb 7, 2023
View reviewed changes
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArrayLiteralContentNodes returns lists which may contain Elision nodes. These are in turn passed to CreateJSONParseRecord, followed by ShallowestContainedJSONValue. So by my reading, the assertion that is step 2 of CreateJSONParseRecord will fail.

michaelficarra
michaelficarra reviewed Feb 7, 2023
View reviewed changes
spec.html
1. Let _entryParseRecord_ be CreateJSONParseRecord(_propertyValueNode_, _P_, ! Get(_val_, _P_)).
1. Append _entryParseRecord_ to _entries_.
1. Else,
1. Assert: _typedValNode_ is not an |ArrayLiteral| Parse Node and not an |ObjectLiteral| Parse Node.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this assertion here? The invariant holds only for this step.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To assert agreement between parseNode and val: the latter is an object if and only if the former is an |ArrayLiteral| or |ObjectLiteral|.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 this assertion is helpful.

@michaelficarra
Copy link
Member

Please don't introduce an SDO with the same name as a nonterminal in the grammar (PropertyDefinitionList).

michaelficarra
michaelficarra reviewed Feb 7, 2023
View reviewed changes
spec.html Outdated Show resolved Hide resolved
@gibson042
Copy link
Collaborator Author

ArrayLiteralContentNodes returns lists which may contain Elision nodes. These are in turn passed to CreateJSONParseRecord, followed by ShallowestContainedJSONValue. So by my reading, the assertion that is step 2 of CreateJSONParseRecord will fail.

Elisions are not valid in JSON text.

@gibson042
Copy link
Collaborator Author

gibson042 commented Feb 7, 2023 •
edited
Loading

Please don't introduce an SDO with the same name as a nonterminal in the grammar (PropertyDefinitionList).

@michaelficarra Do you have an alternative name suggestion?

syg
syg approved these changes Feb 10, 2023
View reviewed changes
spec.html
1. Let _entryParseRecord_ be CreateJSONParseRecord(_propertyValueNode_, _P_, ! Get(_val_, _P_)).
1. Append _entryParseRecord_ to _entries_.
1. Else,
1. Assert: _typedValNode_ is not an |ArrayLiteral| Parse Node and not an |ObjectLiteral| Parse Node.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 this assertion is helpful.

spec.html Outdated Show resolved Hide resolved
@gibson042 gibson042 merged commit 513f373 into tc39:master Sep 18, 2023
1 check passed
gibson042 added a commit to gibson042/proposal-json-parse-with-source that referenced this pull request Sep 18, 2023
@gibson042
Editorial: Rename new SDO to distinguish it from grammar nonterminals
c4d5ef1 
tc39#41 (comment)
@gibson042 gibson042 mentioned this pull request Sep 18, 2023
Merged
gibson042 added a commit that referenced this pull request Sep 18, 2023
@gibson042
Editorial: Rename new SDO to distinguish it from grammar nonterminals
e9481f7 
#41 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelficarra michaelficarra michaelficarra left review comments

@syg syg syg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@gibson042 @michaelficarra @syg