375801 - uneval should use "(void 0)" instead of "undefined"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> uneval({a: undefined})
({a:undefined})

This is wrong because "undefined" is a mutable property of the global object.  In bug 351219, Brendan suggested using "(void 0)" instead.

Comment 1

[Brian Crowder]

I wonder if this too should be WONTFIXed.  How much of the web would be broken by this change?

Comment 2

[Jesse Ruderman]

How much of the web even uses uneval?  Only SpiderMonkey supports it.

Comment 3

[Brian Crowder]

Attachment: adds "(void 0)" atom and uses it for toString of JSVAL_VOID

Well, I think in our case toSource and toString are not so different (especially here).  This seems like a risky change, but here is what a patch would look like (I think) if we were to implement it.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 267471 [details] [diff] [review]
adds "(void 0)" atom and uses it for toString of JSVAL_VOID

Is it really worth it to add an atom for this? I feel like a static const char[] in jsstr.c would suffice here. Tell me if I'm wrong, though!

Comment 5

[Jesse Ruderman]

I think toString should return "undefined" for web compatibility and avoiding surprise, but toSource/uneval should return "(void 0)" for correctness.

Comment 6

[Brian Crowder]

Attachment: addressing Jesse's suggestion

Still using an atom here, for now, but I can be convinced otherwise.

Comment 7

[Brendan Eich [:brendan]]

I can see how using a C string constant and inflating it into a new JS string every time would be both (a) more code than this patch, and (b) costly in time and (if done often) space. Sometimes the smallest patch is the best, yay. Cc'ing Igor to get his thoughts.

/be

Comment 8

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-375801.js,v  <--  regress-375801.js
initial revision: 1.1

Comment 9

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.