Unexpected result with isNaN method

[CaledoniaProject]

I'm using Mozilla Rhino engine version 1.7.7.2 with OpenRASP project.

The following statement returns expected result false

isNaN('0x7f.0x0.0x0.0x1')

At lease in Chrome console, it returns true

Can you verify that?

[sainaen]

Yep. Right now isNaN() uses the same conversion method as parseInt(), so it erroneously finds 0x7f prefix of the string to be a proper Number, not a NaN.

For the curious, it's org.mozilla.javascript.ScriptRuntime#stringToNumber, which is caled from ScriptRuntime#toNumber. There are other misalignments with the spec, for example, isNaN("-0x1") should be true, but returns false in Rhino.

[sainaen]

Ok, some more details:

• stringToNumber() supposed to work only with integers; it starts from given position and doesn't “consume” anything but chars that are expected to be part of a number. When it finds something else, it just returns what it got at that point (if it's at the beginning of the string, it will return NaN)
• this behavior is fine for the parseInt() because it supposed to work this way (see step 11)
• it's also fine for the TokenStream.getToken(), because there stringToNumber() is called on the previously collected characters of the number and only when it's an integer, so nothing else is in the string
• toNumber() passes its input string to stringToNumber() with a start position adjusted (whitespace and 0x prefix are “consumed”) when it sees a 0x prefix (note that 010 is not an octal number for toNumber()), but it doesn't check that the whole string was “consumed” ← the problem

toNumber() is very important conversion function (with 40 usages in a few Native* classes, Interpreter, Context, and ScriptRuntime). As far as I can tell, this behavior was there since at least 2001, so changing it could be considered a breaking change. If I tried to fix this and a couple of other things around it, what would be the best way to proceed?

[gbrail]

In general -- We can be a little bit more aggressive with more recent versions. So if you want to change this to match the latest ES specs but not break existing code, then it'd be safest to make the change only if: Context.getLanguageVersion() > Context.VERSION_ES6 In addition, it'd be great to re-use some tests from the ES6 test suite -- either: - look at "test262.properties" and enable some related tests that might currently be disabled - or, copy a test suite from the v8 project and put it in testsrc/jstests/es6 or testsrc/jstests/harmony (depending on where you got it in V8) with a corresponding test class in org.mozilla.javascript.tests.{whatever you picked} That way we're using a "known" test suite that accounts for the tons of corner cases that come with every single thing about JavaScript ;-)

…

On Thu, Dec 21, 2017 at 10:08 AM, Ivan Vyshnevskyi ***@***.*** > wrote: Ok, some more details: - stringToNumber() supposed to work only with integers; it starts from given position and doesn't “consume” anything but chars that are expected to be part of a number. When it finds something else, it just returns what it got at that point (if it's at the beginning of the string, it will return NaN) - this behavior is fine for the parseInt() because it supposed to work this way (see step 11 <https://tc39.github.io/ecma262/#sec-parseint-string-radix>) - it's also fine for the TokenStream.getToken(), because there stringToNumber() is called on the previously collected characters of the number and only when it's an integer, so nothing else is in the string - toNumber() passes its input string to stringToNumber() with a start position adjusted (whitespace and 0x prefix are “consumed”) when it sees a 0x prefix (note that 010 is *not* an octal number for toNumber()), but it doesn't check that the whole string was “consumed” ← the problem As far as I can tell, this behavior was there since at least 2001, so changing it could be considered a breaking change. If I tried to fix this and a couple of other things around it, what would be the best way to proceed? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#368 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAf0ayHiDBGpnnrdDqwxf1qzLB-lMgG_ks5tCp6WgaJpZM4RJlYH> .

[sainaen]

Context.getLanguageVersion() > Context.VERSION_ES6

Sure, will go with this.

• look at "test262.properties" and enable some related tests that might currently be disabled

Yeah, well, I went to test262 first and they don't seem to have cases covering this. I'll probably try to submit some, based on the spec, so I'm more or less sure that I got it right before doing fix here. (Even did a first step. 🙂 )

• or, copy a test suite from the v8 project and put it in testsrc/jstests/es6 or testsrc/jstests/harmony (depending on where you got it in V8) with a corresponding test class in org.mozilla.javascript.tests.{whatever you picked}

Again, I can't seem to find anything interesting. All of the isNaN uses GitHub's search finds, except for weird regressions tests, are for checking that other things return NaNs correctly. 😕
I'm cloning v8 right now to look more thoroughly with grep, but it is slooow.

SpiderMonkey has basic tests and I'll definitely borrow some.
Update: the next day after this message, the patch landed that removed this file since the ToNumber conversion is covered in test262 by the Number constructor tests. Here's a link to it in some older revision: GlobalObject/15.1.2.6.js

[gbrail]

Sorry -- I meant ">= Context.VERSION_ES6." Otherwise your code will never be executed ;-) And yeah, it's probably worth a careful reading of the ECMAScript spec to make sure that we're following the spec and not just what we observe in Chrome or wherever.

…

On Thu, Dec 21, 2017 at 11:06 AM, Ivan Vyshnevskyi ***@***.*** > wrote: Context.getLanguageVersion() > Context.VERSION_ES6 Sure, will go with this. - look at "test262.properties" and enable some related tests that might currently be disabled Yeah, well, I went to test262 first and they don't seem to have cases covering this. I'll probably try to submit some, based on the spec, so I'm more or less sure that I got it right before doing fix here. (Even did a first step. <tc39/test262#1378> 🙂 ) - or, copy a test suite from the v8 project and put it in testsrc/jstests/es6 or testsrc/jstests/harmony (depending on where you got it in V8) with a corresponding test class in org.mozilla.javascript.tests.{whatever you picked} Again, I can't seem to find anything interesting. All of the isNaN uses GitHub's search finds, except for weird regressions tests, are for checking that other things return NaNs correctly. 😕 I'm cloning v8 right now to look more thoroughly with grep, but it is slooow. SpiderMonkey has basic tests <https://searchfox.org/mozilla-central/source/js/src/tests/ecma/GlobalObject/15.1.2.6.js> and I'll definitely borrow some. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#368 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAf0a6TuNB-ttpYANnIzLQu53MAvw1geks5tCqw8gaJpZM4RJlYH> .

[sainaen]

Sorry -- I meant ">= Context.VERSION_ES6." Otherwise your code will never be executed ;-)

Oh, no worries, I didn't even notice the lack of = and understood it like >= :)

Progress update: I've submitted PR adding this case to test262, but since it's holidays I don't expect it will be merged in or even looked at this week/year. In the meantime, I'll prepare the patch based on my current understanding.

[sainaen]

I wrote a patch in the d2ac3ae that we could discuss, but it depends on both tc39/test262#1382 and #372.

Update: One of the test262 maintainers just merged my PR. 🎉
I'm going to update submodule to the latest version in the #372 and will wait for the merge.

Update 2: I've updated #372 to include latest test262 master at the moment.

[CaledoniaProject]

I have problems with Number.isInteger

Number.isInteger(1)   ==> true
Number.isInteger('1') ==> false

The only thing works for now is parseInt ... is that expected?

[sainaen]

Yes, @CaledoniaProject, that's how Number.isInteger() is supposed to work.
See, for example, MDN docs on this.