Remove Boolean.prototype.toJSON

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: evilpie)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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"); 
});
--
(Assignee)

Updated

6 years ago
Assignee: general → evilpies
(Assignee)

Comment 1

6 years ago
Created attachment 530015 [details] [diff] [review]
remove boolean toJSON

Not sure what to test here?
Attachment #530015 - Flags: review?(pbiggar)
Tom, there's a test case in comment 0.  :)

Comment 3

6 years ago
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

6 years ago
(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.
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");
Jeff: There's also the test case in comment 0.
(Assignee)

Comment 7

6 years ago
Jeff: don't worry, i didn't notice it either :P
Ha, right.  That's what I get for going from memory and not looking at spec steps.

Comment 9

6 years ago
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.
Attachment #530015 - Flags: review?(pbiggar) → review+
(Assignee)

Comment 10

6 years ago
Added the tests.
https://hg.mozilla.org/tracemonkey/rev/22acbcf01a8f
(Assignee)

Updated

6 years ago
Whiteboard: [fixed-in-tracemonkey]
(Assignee)

Comment 11

6 years ago
Fixed warnings on tinderbox
https://hg.mozilla.org/tracemonkey/rev/ec82b6f20c8f
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/22acbcf01a8f
http://hg.mozilla.org/mozilla-central/rev/ec82b6f20c8f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.