Implement the optional space parameter in JSON.stringify

[tofpie]

This Pull Request fixes/closes #346 .

It changes the following:

• JSON.stringify() now supports space parameter

[codecov]

Codecov Report

Merging #961 (4aec630) into master (8f388d5) will increase coverage by 0.09%.
The diff coverage is 87.17%.

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
+ Coverage   59.40%   59.49%   +0.09%     
==========================================
  Files         166      166              
  Lines       10717    10752      +35     
==========================================
+ Hits         6366     6397      +31     
- Misses       4351     4355       +4     

Impacted Files	Coverage Δ
boa/src/builtins/json/mod.rs	84.07% <87.17%> (+2.01%)	⬆️

---

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 8f388d5...4aec630. Read the comment docs.

[RageKnify]

Looks good, but needs a fix:
JSON.stringify({a: "b", b: "c"}, undefined, 4) will return {"a":"b","b":"c"} when it should not.
The code relative to replacer needs to not return immediately, the implementation doesn't follow the spec closely, so I'm not sure as to how we should aproach, but it's a test case to add (I'd say to change your tests with {} as the replacer to use undefined instead).

#919 implements ToIntegerOrInfinity, which could be used to more closely follow the spec, but I don't think it's a big problem.

[tofpie]

Thanks for your review and comments. Let's wait for #919 to be merged, so that ToIntegerOrInfinity can be used to follow the spec more closely.

[RageKnify]

Sorry to bother, but I do think we should use undefined in all of these cases, the spec only uses the 2nd argument when it is a function or array, I'd rather we leave it as undefined to be clear that it is not used.

[RageKnify]

Looks good, we'll just wait for #919 to use ToIntegerOrInfinity

[tofpie]

I have run the test space-string-object.js. It passes on this branch but it also passes on master. I have used the following command:

cargo run --release --bin boa_tester -- run -s test/built-ins/JSON/stringify/space-string-object.js -vv

It is probably a dumb question, and I might have missed something, but why is it pass on master?

[RageKnify]

That test only checks if pairs of invocations return the same result, it never checks if the results follow the spec. So we get:

>> JSON.stringify(obj, null, 'xxx')
"{"a1":{"b1":[1,2,3,4],"b2":{"c1":1,"c2":2}},"a2":"a2"}"
>> JSON.stringify(obj, null, new String('xxx'))
"{"a1":{"b1":[1,2,3,4],"b2":{"c1":1,"c2":2}},"a2":"a2"}"

Being equal and we pass the test.

edit: space-number.js is an example that should failt in master and pass in your branch.

[tofpie]

Thanks for the explanation. I have run space-number.js and also space-string.js on master, and they are both passed.

[RageKnify]

I have run space-number.js and also space-string.js on master, and they are both passed.

My bad for not reading the test carefully, just like the other one, it only checks if the results match, since we never use the arguments it passes the test.

[tofpie]

It is right for space-number.js, but space-string.js makes an actual comparison with the expected indented string in the second assert. It should probably be failed on master.

[tofpie]

I finally found why the tests are passed on master on my computer. It seems that there is a bug in the parsing of single line comments that occurs when the files use CRLF line endings. The end of line is not detected, so the single line comment extends until the end of the file.

In cursor.rs when a CR is detected, the following LF is swallowed, and only the CR is returned.

boa/boa/src/syntax/lexer/cursor.rs

Lines 222 to 230 in 0c068cb

	match byte {
	Some(b'\r') => {
	// Try to take a newline if it's next, for windows "\r\n" newlines
	// Otherwise, treat as a Mac OS9 bare '\r' newline
	if self.peek()? == Some(b'\n') {
	let _ = self.iter.next_byte();
	}
	self.next_line();
	}

But the tokenizer for single line comments expects a LF to end a comment, and not a CR.

boa/boa/src/syntax/lexer/comment.rs

Lines 33 to 40 in 0c068cb

	while let Some(ch) = cursor.peek()? {
	if ch == b'\n' {
	break;
	} else {
	// Consume char.
	cursor.next_byte()?.expect("Comment character vansihed");
	}
	}

As I work on Windows, and the test262 repository is cloned with CRLF endings on Windows, so all tests starting with single line comments are passed, as they are considered empty.

Fixing cursor.rs so that it always returns LF for end of lines would probably be the best solution, but it is also possible to fix the single line comment parser so that it detects both CR and LF as line endings. What do you think?

[Razican]

Fixing cursor.rs so that it always returns LF for end of lines would probably be the best solution, but it is also possible to fix the single line comment parser so that it detects both CR and LF as line endings. What do you think?

I have merged the PR, so a rebase should fix that.

Test262 conformance changes:

Test result	master count	PR count	difference
Total	78,460	78,460	0
Passed	19,374	19,376	+2
Ignored	15,559	15,559	0
Failed	43,527	43,525	-2
Panics	465	465	0
Conformance	24.69	24.70	+0.00%

[Razican]

This looks good to me, It seems it follows the spec correctly, even though it's not following it step by step exactly. Still, I think it makes sense to do it this way, since the result is the same and it might be a bit more optimized.

One less panic, 3 new extra tests passing, seems nice :)

[tofpie]

In fact, using to_integer_or_infinity would convert a boolean to 1, and so generate an indentation with one space, and that does not follow the spec. Maybe it should stay as it is.

[RageKnify]

In fact, using to_integer_or_infinity would convert a boolean to 1, and so generate an indentation with one space, and that does not follow the spec. Maybe it should stay as it is.

I don't think that would be the case @tofpie the spec only enters step 6. if the type is Number and that wouldn't be the case for booleans.

[tofpie]

Thanks for the clarification. I understand now. Sorry I did not get it earlier.
I will propose something that follows the spec more closely.

[RageKnify]

LGTM, I'll let @Razican have a final look at the code

[Razican]

Looks good to me :) thanks!