Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for bad hex strings when using Number constructor. #369

Merged
merged 1 commit into from
Nov 4, 2015
Merged

Fix for bad hex strings when using Number constructor. #369

merged 1 commit into from
Nov 4, 2015

Conversation

Xotic750
Copy link
Contributor

@Xotic750 Xotic750 commented Nov 4, 2015

Ref #366

Node v0.8, old Chrome/Chromium and others.

@ljharb
Copy link
Collaborator

ljharb commented Nov 4, 2015

Can you please provide tests that would fail without your change?

Also, can you link to spec text that says that +'-0x1' should be NaN, when the literal -0x1 parses fine?

@Xotic750
Copy link
Contributor Author

Xotic750 commented Nov 4, 2015

I already provided the tests in #366 and that was the only failing test remaining in your latest push. If I can find something concrete in the spec I will link it, but otherwise Node v0.8, old Chrome and Chromium (possibly others) behaviour is different to all others in this respect. So someone's got it right and someone's got it wrong, not consistent.

@ljharb
Copy link
Collaborator

ljharb commented Nov 4, 2015

You're right about the tests - https://travis-ci.org/paulmillr/es6-shim/jobs/89172994 passes but https://travis-ci.org/paulmillr/es6-shim/jobs/88743727 fails.

It's worth noting that this appears to make things consistent, but I do want to know what the spec requires.

@Xotic750
Copy link
Contributor Author

Xotic750 commented Nov 4, 2015

i would guess these
http://www.ecma-international.org/ecma-262/6.0/#sec-tonumber-applied-to-the-string-type

StrNumericLiteral :::
StrDecimalLiteral
BinaryIntegerLiteral
OctalIntegerLiteral
HexIntegerLiteral

http://www.ecma-international.org/ecma-262/6.0/#sec-literals-numeric-literals

HexIntegerLiteral ::
0x HexDigits
0X HexDigits
HexDigits ::
HexDigit
HexDigits HexDigit
HexDigit :: one of
0 1 2 3 4 5 6 7 8 9 a b c d e f A B C D E F

http://www.ecma-international.org/ecma-262/6.0/#sec-number-conversions

HexIntegerLiteral :::
0x HexDigit
0X HexDigit
HexIntegerLiteral HexDigit

So, current environments seem to interpret this as '0x1' is valid but '-0x1' is invalid. There is no mention of +/-, so it seems fair to interpret it this way I think. In the same way binary and octal strings do not mention +/- and they also are interpreted '0b1' is valid and '-0b1' is invalid. :/

And of course native parseInt behaves differently again by accepting '-0x1' but not '-0b1', but the es5-shim doesn't appear to agree in its fix. :/

// ES-5 15.1.2.2
if (parseInt(ws + '08') !== 8 || parseInt(ws + '0x16') !== 22) {
    /* global parseInt: true */
    parseInt = (function (origParseInt) {
        var hexRegex = /^0[xX]/;
        return function parseInt(str, radix) {
            var string = $String(str).trim();
            var defaultedRadix = $Number(radix) || (hexRegex.test(string) ? 16 : 10);
            return origParseInt(string, defaultedRadix);
        };
    }(parseInt));
}

That's about as concrete as I can give you.

@@ -719,6 +719,7 @@
};
var nonWS = ['\u0085', '\u200b', '\ufffe'].join('');
var nonWSregex = new RegExp('[' + nonWS + ']', 'g');
var isBadHexRegex = new RegExp('^[-+]0x[0-9a-f]+$', 'i');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be a literal - /^[\-+]0x[0-9a-f]+$/i

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure on the preference of style.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In every language, using literals is preferred over more verbose forms when they're equivalent :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, but I seem to remember some older environments (probably Opera) had problems with certain literals, I think it was XRegExp that pointed these out. :)

ljharb added a commit that referenced this pull request Nov 4, 2015
[Fix] don't parse bad hex strings when using `Number` constructor
@ljharb ljharb merged commit 85051be into paulmillr:master Nov 4, 2015
@ljharb
Copy link
Collaborator

ljharb commented Nov 4, 2015

Please file a separate issue for parseInt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants