Fix zero argument panic in JSON.parse()

[JohnDoneth]

This Pull Request closes #784.

It changes the following:
Instead of panicking when an argument is not provided to JSON.parse(), a syntax error is raised like other JavaScript implementations.

[codecov]

Codecov Report

Merging #785 into master will increase coverage by 0.01%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
+ Coverage   59.13%   59.15%   +0.01%     
==========================================
  Files         155      155              
  Lines        9838     9837       -1     
==========================================
+ Hits         5818     5819       +1     
+ Misses       4020     4018       -2     

Impacted Files	Coverage Δ
boa/src/builtins/json/mod.rs	82.43% <80.00%> (+2.43%)	⬆️
boa/src/syntax/ast/node/mod.rs	43.80% <0.00%> (ø)
boa/src/syntax/ast/node/expression/call/mod.rs
boa/src/syntax/ast/node/expression/new/mod.rs
boa/src/syntax/ast/node/new/mod.rs	72.22% <0.00%> (ø)
boa/src/syntax/ast/node/call/mod.rs	83.33% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a509de...24ad305. Read the comment docs.

[croraf]

@JohnDoneth Two things:

1. As far as I see, the spec (https://tc39.es/ecma262/#sec-json.parse) doesn't explicitly say what should happen when argument is not passed. I guess somewhere before it generally says that it is considered as undefined. Would like to find where is that said.

2. From what I see about how Chrome behaves, is that it doesn't have special handling of this case. It is just that no parameter is considered as "undefined" (what I mention in bullet 1) and then the JSON parser throws internally during parsing which gets propagated through the stack.

---

So something like this might be better, although it doesn't work out of the box:

let arg = match args.get(0) {
      Some(arg) => arg.to_string(ctx)?,
      None => Value::undefined().to_string(ctx)?,
    };

The Test-262 spec just asserts that SyntaxError is thrown, so I guess we are fine either way. https://github.com/tc39/test262/blob/main/test/built-ins/JSON/parse/text-non-string-primitive.js

[JohnDoneth]

New output with changes made:

>> JSON.parse()
Uncaught: "expected value at line 1 column 1"

[HalidOdat]

New output with changes made:

>> JSON.parse()
Uncaught: "expected value at line 1 column 1"

This matches more with what other engines are doing, and what the spec says :)

[HalidOdat]

Looks good to me! :)

[croraf]

@JohnDoneth Thanks. Perhaps not related to the task (but a general error), but in Firefox it shows:

Uncaught SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data

and in Chrome

Uncaught SyntaxError: Unexpected token u in JSON at position 0

It would be nice to have the error class and message in the output.
@HalidOdat Is this a common boa issue regarding printing of errors?

---

I actually don't know if the correct error type is thrown in current implementation.

[RageKnify]

I don't know if all errors in JSON::parse should be syntax errors (I'd appreciate if someone else can look into it), but for the Value that gets thrown to be an actual Error with a message this line: https://github.com/JohnDoneth/boa/blob/cfe64ae60065ca36e9bc45a7e7d46a29282abbfb/boa/src/builtins/json/mod.rs#L82
Needs to be changed to instead invoke ctx.throw_syntax_error(err.to_string).

[HalidOdat]

I don't know if all errors in JSON::parse should be syntax errors (I'd appreciate if someone else can look into it), but for the Value that gets thrown to be an actual Error with a message this line: JohnDoneth/boa@cfe64ae/boa/src/builtins/json/mod.rs#L82
Needs to be changed to instead invoke ctx.throw_syntax_error(err.to_string).

Good catch! @RageKnify, Yep. https://tc39.es/ecma262/#sec-json.parse step 2. SyntaxError should be thrown and that's why the name of the error, because an error is not being thrown, we are throwing a string.

[JohnDoneth]

There we go!

>> JSON.parse()
Uncaught: "SyntaxError": "expected value at line 1 column 1"

Thanks for spotting that @RageKnify

[croraf]

Also we should have a test for that :D

[croraf]

Although don't know what is the policy for the tests, now that we have Test-262 test suite, because this test will be a duplicate of https://github.com/tc39/test262/blob/main/test/built-ins/JSON/parse/text-non-string-primitive.js

I even think tests might be a loss of time now.

[HalidOdat]

Although don't know what is the policy for the tests, now that we have Test-262 test suite, because this test will be a duplicate of

Yes some tests are duplicates but that is fine the rust tests catch the bigger bugs but the test262 catch all bugs and is not practical to run the test262 tests for when you want rough idea of what is broken since takes like 10m for them to finish, compared to cargo test around 1 sec for the tests to finish.