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.

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

Improve JSON parser conformity #303

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Conversation

chqrlie
Copy link
Collaborator

@chqrlie chqrlie commented Mar 11, 2024

  • add JSON specific parsers for strings and numbers
  • update JSON parse error messages
  • fix JSON.stringify handling of boxed objects
  • parse Flags in v8 mjsunit test files
  • update v8.txt

- add JSON specific parsers for strings and numbers
- update JSON parse error messages
- fix `JSON.stringify` handling of boxed objects
- parse Flags in v8 mjsunit test files
- update v8.txt
@chqrlie chqrlie mentioned this pull request Mar 11, 2024
Comment on lines +18718 to +18719
if (do_throw)
js_parse_error(s, "octal escape sequences are not allowed in strict mode");
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message should perhaps distinguish between strict mode and template strings? E.g.:

Suggested change
if (do_throw)
js_parse_error(s, "octal escape sequences are not allowed in strict mode");
if (do_throw) {
js_parse_error(s, "octal escape sequences are not allowed in %s",
sep == '`' ? "template strings" : "strict mode");
}

I have a hunch that might fix even more V8 test cases.

This function is never used for JSON strings anymore now, right?

Copy link
Collaborator Author

@chqrlie chqrlie Mar 12, 2024

Choose a reason for hiding this comment

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

This function is never used for JSON strings anymore now, right?

Not used anymore for JSON.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding the V8 test cases, those in test262/implementation-contributed/v8/mjsunit/ are outdated. The current version of v8 has different error messages and more test cases. Of course the error messages are implementation defined so they can change at will...
The v8 tests are useful for conformity because they complement the test262 suite. We might want to create a repository that loosely tracks the test/mjsunit directory of the v8 repository and use that as a submodule. The reports should also be improved to help track the actual arguments for which the failures occur.
I am going to make a new issue from this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The error message should perhaps distinguish between strict mode and template strings?

Yes, and also different error causes: invalid hexadecimal/octal/Unicode escape sequences.
I shall submit a different patch for this.

@chqrlie chqrlie merged commit 45f8dc2 into quickjs-ng:master Mar 14, 2024
36 checks passed
@chqrlie chqrlie deleted the json-fixes branch March 15, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants