654685 - Remove Boolean.prototype.toJSON

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Boolean.prototype.toJSON is not in ES5. Bug 557371 removed {Number, String}.prototype.toJSON.

Test case that triggered this was something like this:
--
JSON.stringify(new Boolean(false), function(k, v) { 
    assertEq(typeof v, "object"); 
});
--

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: remove boolean toJSON

Not sure what to test here?

Comment 2

[Jason Orendorff [:jorendorff]]

Tom, there's a test case in comment 0.  :)

Comment 3

[Paul Biggar]

Comment on attachment 530015 [details] [diff] [review]
remove boolean toJSON

Review of attachment 530015 [details] [diff] [review]:

Have you checked that jstests doesn't fail? I'd be surprised if there wasn't something different didn't.

I'd add the test from jandem's comment, and a test that toJSON isn't in Boolean.prototype. Looking at the standard, there's not much room to go wrong here (it's either "true" or "false"), but you might take a look at the code paths and make sure there's nothing else lurking like jandem's test.

Comment 4

[Paul Biggar]

(In reply to comment #3)
> Have you checked that jstests doesn't fail? I'd be surprised if there wasn't
> something different didn't.

Hmmm, that's not english. I meant that we should check jstests don't fail (and fix them if they do) - I would have though they would.

Comment 5

[Jeff Walden [:Waldo]]

I doubt one fails.  JSON.stringify extracts the primitive boolean from Boolean objects without this method, and it's the exact same behavior you'd get with this method, so the only way I can think of that this would be visible is if you defined a custom toJSON method on Object.prototype that would be invoked but for Boolean.prototype.toJSON existing.  And no one's going to do that because it'd mess up stringification of basically every object.

Here's a test which I think we'd fail currently and which we'd pass without the Boolean.prototype.toJSON method.  It's the only test I can think of which the presence of the method would affect, other than one directly testing for the presence of the method.

Object.prototype.toJSON = function() { return 2; };
assertEq(JSON.stringify(new Boolean(true)), "2");

Comment 6

[Jason Orendorff [:jorendorff]]

Jeff: There's also the test case in comment 0.

Comment 7

[Tom S. (please needinfo tschuster)]

Jeff: don't worry, i didn't notice it either :P

Comment 8

[Jeff Walden [:Waldo]]

Ha, right.  That's what I get for going from memory and not looking at spec steps.

Comment 9

[Paul Biggar]

Comment on attachment 530015 [details] [diff] [review]
remove boolean toJSON

Review of attachment 530015 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you include the tests from comments 0 and 5.

Comment 10

[Tom S. (please needinfo tschuster)]

Added the tests.
https://hg.mozilla.org/tracemonkey/rev/22acbcf01a8f

Comment 11

[Tom S. (please needinfo tschuster)]

Fixed warnings on tinderbox
https://hg.mozilla.org/tracemonkey/rev/ec82b6f20c8f

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/22acbcf01a8f
http://hg.mozilla.org/mozilla-central/rev/ec82b6f20c8f