I get weird behavior when I use a negative integer as an object property. js 1.5 rc2 treats the property name differently as a string than it does as an integer. The following js shell session illustrates. Note that this behavior doesn not occur when I use non-negative integers, or when I use a positive or negative floating-point value as the array index js> var o = new Object(); js> o[-1] = "number"; // is this even legal syntax? number js> o[-1] number js> o["-1"] js> o["-1"] = "string"; string js> o[-1] number js> o["-1"] string js> for(i in o) print(i + ": " + typeof i + ": " + o[i]); -1: string: string -1: string: string js> o[-1] number

Looking into this - somehow I remember that only string literals may be used to define object property names. In the meantime, here is an experiment in the JS Shell using the number 1 instead of -1 : js> var o = {1 : "one - as Number"} js> o[1] one - as Number js> o["1"] one - as Number js> var o = {"1" : "one - as String"} js> o[1] one - as String js> o["1"] one - as String js> var o = {1 : "one - as Number", "1" : "one - as String"} js> o[1] one - as String js> o["1"] one - as String

I get an error if I attempt the same experiment with -1 : js> var o = {-1 : "minus one - as Number", "-1" : "minus one - as String"} 110: invalid property id: 110: var o = {-1 : "minus one - as Number", "-1" : "minus one - as String"} 110: .........^ "invalid property id" is the error JSMSG_BAD_PROP_ID in js.msg. It occurs in a switch case default in jsparse.c: default: js_ReportCompileErrorNumber(cx, ts, JSREPORT_ERROR, JSMSG_BAD_PROP_ID);

From the ECMA3 document: 11.2.1 Property Accessors Properties are accessed by name, using either the dot notation: MemberExpression . Identifier CallExpression . Identifier or the bracket notation: MemberExpression [ Expression ] CallExpression [ Expression ] The dot notation is explained by the following syntactic conversion: MemberExpression . Identifier is identical in its behaviour to MemberExpression [ <identifier-string> ] and similarly CallExpression . Identifier is identical in its behaviour to CallExpression [ <identifier-string> ] where <identifier-string> is a string literal containing the same sequence of characters as the Identifier. The production MemberExpression : MemberExpression [ Expression ] is evaluated as follows: 1. Evaluate MemberExpression. 2. Call GetValue(Result(1)). 3. Evaluate Expression. 4. Call GetValue(Result(3)). 5. Call ToObject(Result(2)). 6. Call ToString(Result(4)). 7. Return a value of type Reference whose base object is Result(5) and whose property name is Result(6). The production CallExpression : CallExpression [ Expression ] is evaluated in exactly the same manner, except that the contained CallExpression is evaluated in step 1.

Now trying an experiment closer to David's, and comparing 1 vs. -1 There is a clear difference in the behavior: js> var o = new Object(); js> o[1] = "one - as Number" one - as Number js> o["1"] = "one - as String" one - as String js> for(i in o) print(i + ": " + typeof i + ": " + o[i]); 1: string: one - as String js> o[-1] = "minus one - as Number" minus one - as Number js> o["-1"] = "minus one - as String" minus one - as String js> for(i in o) print(i + ": " + typeof i + ": " + o[i]); 1: string: one - as String -1: string: minus one - as String -1: string: minus one - as String

CHECK_FOR_FUNNY_INDEX got some ECMA love from mccabe, but it doesn't handle ToString(-1) or other strings containing negative literals that fit in a jsval. Mike, can you field this one fast for js1.5 (and maybe, hope against hope, for Netscape 6 rtm)? Please reassign to me if not. /be

Reassigning to Brendan (sorry) because Mike is away -

Patch coming up. Will try for [rtm+] on account of standards purity. /be

jsints are symmetric around 0, so we don't need to test for a different "max" value when accumulating digits. The greatest jsint that fits in a jsval is (1<<30) - 1 or 1073741823. The least is 1 - (1<<30) or -1073741823. The only thing we need to do is remember whether the property id string began with a '-', and if so, use the two's complement of the accumulated _index. Looking for r= and sr= this weekend. /be

Fix makes sense. Have you run the ECMA suite? Also, there are some tab characters visible, even in lines that you added. Fix the tab damage and verify with the ECMA suite, and r=shaver shall be your reward.

shaver: my editor never adds tabs, but unless I shift or otherwise reformat a whole line, it leaves any existing tabs alone. I did run the JS testsuite, which needs to cover these cases! Same results as before my change, all passed (once you suppress warnings, which now are on by default in the js shell, but which confuse the test driver). /be

sr=jband I'll by this.

Checked into trunk. Among the bugs david@oreilly.com has reported (which I learned of only yesterday, and which show holes in our ECMA testsuite), this is one of only a few that I think netscape.com should take for Netscape 6 RTM. The change allows a leading '-' sign, and remembers (for a short section of code) that it saw that character in order to pass a length check that weeds out numeric-literal string identifiers too big to fit in a jsval (a signed machine word used to hold JS identifiers and values), and finally to negate the converted integer before tagging it as a jsval. The consequences of not fixing this bug are that: (a) We fail to conform to the ECMA-262 Edition 3 spec for the crucial rule that property identifiers are strings, and indexing a property by an expression looks up the identifier given by evaluating the expression and converting it to a string. (b) JS that depends on index-converted-to-string equivalence for negative indexes expressed as strings and as numbers will fail (I don't know of web examples, or of a good way to search for any using string matching). (c) David Flanagan, the author of the O'Reilly "Rhino" JS book -- the top JS book on the market, and a bestseller for O'Reilly -- will have to update that book for Netscape 6 to document this bug. PDT: If you have any questions, please feel free to ask here, via e-mail, on IRC #mozilla, or by phone. Thanks, /be

rtm keyword for foolish consistency. /be

FYI only - I get 'unary minus operator applied to unsigned type, result still unsigned' warnings from VC++.

That warning is silly, it's easily avoided via 0 - _index instead of -_index. Fixed in trunk. /be

PDT: I hope it's clear from the patch itself, and from my earlier comment: >The change allows a leading '-' sign, and remembers (for a short section of >code) that it saw that character in order to pass a length check that weeds out >numeric-literal string identifiers too big to fit in a jsval (a signed machine >word used to hold JS identifiers and values), and finally to negate the >converted integer before tagging it as a jsval. plus from the testing David has done, that *should this fix have a bug*, it will affect only negative indexes expressed as strings, which were already broken. It won't run off the zero-terminated end of a string, any more than the code could before. It won't crash on a bad address, any more than the code could before. The fix does not alter behavior for unsigned numeric literals, it pellucidly works for "-1", and it can be demonstrated to work for "-1073741823" (the least integer that fits in a jsval, converted to a string) and "-1073741824" (which can't fit, and so remains a string. /be

r=mccabe. + JSBool _negative = (*_cp == '-'); + if (_negative) _cp++; Would it be a micro-erg better to say _cp += _negative, and avoid the branch? Or is this too fine an optimization? Similarly, but less clearly, > if (_negative) _index = -_index;

An unconditional add instead of a predicted-taken branch around the then statement might be better, yeah. I'll play around on popular architectures, look at code, pipeline it through the superscalar simulator that MicroUnity left in my brain. Already did the _index = 0 - _index to abate an inconsistent Windows warning. PDT: this fussing doesn't matter for the branch, although the warning fix is free if you want it. /be

This seems release-notable to us. [rtm-]

Fix in trunk, and not wanted in N6 branch, so closing. /be

Verified on Linux, Mac and WinNT via this testcase added to JS test suite: js/tests/js1_5/Regress/regress-57043.js