1860887 - Provide JSON.parse with source for primitives

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Bryan Thrall [:bthrall]]

Support the JSON.parse with source proposal behind a shell option for parsing primitives (object and array support will come later).

This will require:

• A shell option "--enable-json-parse-with-source" to enable the proposal support
• Implement the CreateParseRecord() algorithm from the proposal with support for JSON primitive types
• Update InternalizeJSONProperty() and JSON.parse() implementations to use the Parse Record information according to the proposal

Comment 1

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Add --enable-json-parse-with-source shell option r=jandem

Comment 2

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Begin implementing ParseRecordObject r=jandem

I haven't decided on the representation for ParseRecordObject.[[ParseNode]]
yet.

The slot enum is public for testing purposes, but will be made private as the
implementation matures.

Depends on D193469

Comment 3

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Pass ParseRecordObject parameter to InternalizeJSONProperty and create context object r=jandem

Depends on D193470

Comment 4

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Add parseNode parameter to CreateJSONParseRecord r=jandem!

Using JSString as a placeholder for the type because I think we will only need
to store the source content for use. We can probably end up using a
JSDependentString to minimize memory and copies.

The missing piece right now is how to provide the parseNode info to the
CreateJSONParseRecord since JSON parsing currently doesn't keep that info
around.

Depends on D193471

Comment 5

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Construct ParseRecordObjects for primitives r=jandem

This is easier to do at parse time than in a separate pass using
CreateJSONParseRecord because we don't have a formal JSON syntax tree. The
downside is it's harder to make sure I am implementing things exactly as the
spec requires.

This implementation always constructs the ParseRecordObjects, even if they
aren't needed; we should avoid that cost when we can.

I also am creating whole new strings for the ParseNodeSlot, when I should
probably be using dependent strings.

Depends on D193472

Comment 6

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Move ParseRecordObject creation into numberValue() and stringValue() r=jandem!

This makes source handling consistent between number, string, boolean, and
null.

Some functions (numberValue, stringValue, atomValue, objectPropertyName) now
use the source's character type, so they had to move to JSONFullParseHandler.

Depends on D193474

Comment 7

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Avoid creating ParseRecordObject when not enabled r=jandem!

Depends on D193475

Comment 8

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Trace JSONFullParseHandler::parseRecord r=jandem

Depends on D193476

Comment 9

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Add tests r=jandem

These tests cover the progress of implementing JSON.parse with source so far,
so we don't have to modify
js/src/tests/test262/staging/JSON/json-parse-with-source.js while some parts
aren't done yet.

This file can also include tests that aren't part of test262, or can be removed
after the proposal is completely implemented.

Depends on D193477

Comment 10

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Construct ParseRecordObject with parseNode and value r=jandem

Depends on D193479

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The following patches are waiting for review from a reviewer who resigned from the review:

ID	Title	Author	Reviewer Status
D193469	Bug 1860887 - Add --enable-json-parse-with-source shell option r=allstarschh!	bthrall	allstarschh: Resigned from review
D193470	Bug 1860887 - Begin implementing CreateJSONParseRecord r=allstarschh!	bthrall	allstarschh: Resigned from review
D193471	Bug 1860887 - Pass ParseRecordObject parameter to InternalizeJSONProperty and create context object r=allstarschh!	bthrall	allstarschh: Resigned from review
D193472	Bug 1860887 - Add parseNode parameter to CreateJSONParseRecord r=allstarschh!	bthrall	allstarschh: Resigned from review
D193474	Bug 1860887 - Construct ParseRecordObjects for primitives r=allstarschh!	bthrall	allstarschh: Resigned from review
D193475	Bug 1860887 - Move ParseRecordObject creation into numberValue() and stringValue() r=allstarschh!	bthrall	allstarschh: Resigned from review
D193476	Bug 1860887 - Avoid creating ParseRecordObject when not enabled r=allstarschh!	bthrall	allstarschh: Resigned from review
D193477	Bug 1860887 - Trace JSONFullParseHandler::parseRecord r=allstarschh!	bthrall	allstarschh: Resigned from review
D193479	Bug 1860887 - Add tests r=allstarschh!	bthrall	allstarschh: Resigned from review
D193480	Bug 1860887 - Add setter/getters instead of using slot ids directly r=allstarschh!	bthrall	allstarschh: Resigned from review

:bthrall, could you please find another reviewer?

For more information, please visit BugBot documentation.

Comment 12

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Enable JSON.parse with source when configured r=jandem

Comment 13

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Move functions down to JSONFullParseHandler r=jandem

When we add ParseRecord support for objects and arrays, these functions will
depend on the character type, so they can't be in JSONFullParseHandlerAnyChar.

Depends on D193479

Comment 14

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Reorder JSON functions r=jandem

Follow the order in the header file, and group by class to make them easier to
find.

Depends on D198320

Comment 15

[Bryan Thrall [:bthrall]]

Attachment: Bug 1860887 - Fix autospider WASI unified compile r=jandem!

Adding ParseRecordObject.cpp to js/src/moz.build changes the unified file that
includes jsdate.cpp so that it also includes WrappedFunctionObject.cpp, which
contains 'using namespace JS;'. This makes several static functions in
jsdate.cpp ambiguous.

jsdate.cpp was not expecting the JS namespace to be in scope, so it is clear
that the ambiguous function references should all be for the static functions.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2f70eb90b3c
Enable JSON.parse with source when configured r=jandem
https://hg.mozilla.org/integration/autoland/rev/b806178221aa
Add --enable-json-parse-with-source shell option r=jandem
https://hg.mozilla.org/integration/autoland/rev/6a8f5054d8d5
Begin implementing ParseRecordObject r=jandem
https://hg.mozilla.org/integration/autoland/rev/bce38792bb60
Fix autospider WASI unified compile r=jandem
https://hg.mozilla.org/integration/autoland/rev/751289257f92
Pass ParseRecordObject parameter to InternalizeJSONProperty and create context object r=jandem
https://hg.mozilla.org/integration/autoland/rev/0c08191bfac1
Construct ParseRecordObjects for primitives r=jandem
https://hg.mozilla.org/integration/autoland/rev/41a49eb63e56
Trace JSONFullParseHandler::parseRecord r=jandem
https://hg.mozilla.org/integration/autoland/rev/de041895ff59
Add tests r=jandem
https://hg.mozilla.org/integration/autoland/rev/7a609350fd11
Reorder JSON functions r=jandem

Comment 17

[Natalia Csoregi [:nataliaCs]]

Backed out for causing py3 failure on lint.py

• backout: https://hg.mozilla.org/integration/autoland/rev/90b599e79c1978332a10ff2488aefb0661e3c342
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=7a609350fd1141b84a9e3a8b006df4d6eb94abbf&selectedTaskRun=Va1D8r-vRkKhZGL8oH9p6A.0
• failure log: https://treeherder.mozilla.org/logviewer?job_id=445739497&repo=autoland&lineNumber=2342

[task 2024-02-02T20:18:02.278Z] ======================== 83 passed in 123.18s (0:02:03) ========================
[task 2024-02-02T20:18:02.278Z] 
[task 2024-02-02T20:18:02.278Z] 
[task 2024-02-02T20:18:02.278Z] Tests Completed: 100%|██████████| 81/81 [02:15<00:00, 27.71s/Test]
[task 2024-02-02T20:18:02.278Z]                                                                   
[task 2024-02-02T20:18:02.278Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/test/configure/lint.py
[task 2024-02-02T20:18:02.278Z] ============================= test session starts ==============================
[task 2024-02-02T20:18:02.278Z] platform linux -- Python 3.8.10, pytest-7.0.1, pluggy-1.4.0 -- /builds/worker/.mozbuild/srcdirs/gecko-8a5b87fe5d69/_virtualenvs/python-test/bin/python
[task 2024-02-02T20:18:02.279Z] rootdir: /builds/worker/checkouts/gecko, configfile: config/mozunit/mozunit/pytest.ini
[task 2024-02-02T20:18:02.279Z] collecting ... collected 4 items
[task 2024-02-02T20:18:02.279Z] 
[task 2024-02-02T20:18:02.279Z] python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_browser TEST-UNEXPECTED-FAIL
[task 2024-02-02T20:18:02.279Z] python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_js TEST-UNEXPECTED-FAIL
[task 2024-02-02T20:18:02.279Z] python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_memory PASSED
[task 2024-02-02T20:18:02.279Z] python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_mobile_android TEST-UNEXPECTED-FAIL
[task 2024-02-02T20:18:02.279Z] 

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b49045beeaf3
Enable JSON.parse with source when configured r=jandem
https://hg.mozilla.org/integration/autoland/rev/aa9d9ac352af
Add --enable-json-parse-with-source shell option r=jandem
https://hg.mozilla.org/integration/autoland/rev/ba7e3130ed8a
Begin implementing ParseRecordObject r=jandem
https://hg.mozilla.org/integration/autoland/rev/c7e519735961
Fix autospider WASI unified compile r=jandem
https://hg.mozilla.org/integration/autoland/rev/c92998c43c3b
Pass ParseRecordObject parameter to InternalizeJSONProperty and create context object r=jandem
https://hg.mozilla.org/integration/autoland/rev/e97bc86aaf2b
Construct ParseRecordObjects for primitives r=jandem
https://hg.mozilla.org/integration/autoland/rev/523a63b359ed
Trace JSONFullParseHandler::parseRecord r=jandem
https://hg.mozilla.org/integration/autoland/rev/1cffabd5b348
Add tests r=jandem
https://hg.mozilla.org/integration/autoland/rev/aa071cec54c8
Reorder JSON functions r=jandem

Comment 19

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/b49045beeaf3
https://hg.mozilla.org/mozilla-central/rev/aa9d9ac352af
https://hg.mozilla.org/mozilla-central/rev/ba7e3130ed8a
https://hg.mozilla.org/mozilla-central/rev/c7e519735961
https://hg.mozilla.org/mozilla-central/rev/c92998c43c3b
https://hg.mozilla.org/mozilla-central/rev/e97bc86aaf2b
https://hg.mozilla.org/mozilla-central/rev/523a63b359ed
https://hg.mozilla.org/mozilla-central/rev/1cffabd5b348
https://hg.mozilla.org/mozilla-central/rev/aa071cec54c8

Comment 20

[Hamish Willee]

Looking at the code, this is behind a build-time flag right? I.e. it is not released by default anywhere?

FYI Docs work for this being tracked in https://github.com/mdn/content/issues/32387

Comment 21

[Bryan Thrall [:bthrall]]

Yes, it is behind both a build-time flag and a runtime flag. It is not released by default anywhere yet.