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

StringNumericLiteral does not include NumericLiteralSeparator #10938

Merged
merged 1 commit into from Jan 10, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Dec 29, 2019

Q                       A
Fixed Issues? proposal-numeric-separator incorrectly removes _ in Number argument.
Patch: Bug Fix? Yes, but given that it does change the behaviour, minor is expected.
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link Nope, Number("4_2") is never mentioned in the docs.
License MIT

The String Type Interpretation Grammar of ToNumber was updated in tc39/proposal-numeric-separator@fc53cdb, the StringNumericLiteral does not allow a NumericLiteralSeparator. Therefore the following example should return NaN

new Number("4_2")

but Babel will transform Number("4_2") into Number("42"), which is incorrect.
This PR removes the transformation on Number arguments.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Numeric Separator labels Dec 29, 2019
@nicolo-ribaudo nicolo-ribaudo added PR: Spec Compliance 👓 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Dec 29, 2019
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Out of curiosity, did you find this in a bug report outside of GitHub?

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Dec 29, 2019
@JLHwung
Copy link
Contributor Author

JLHwung commented Dec 29, 2019

@nicolo-ribaudo Nope. I find it by searching CallExpression: in the code base.

Background: I was reviewing an estree AST design PR and inspired from https://github.com/estree/estree/pull/204#issuecomment-565686514 (prevent GitHub backtrace)

It's a severe concern that non-optional member accesses and function calls are getting two different representations.

It is exactly how things are done in babel-parser, so I feel like there will be some potential bugs when a CallExpression visitor does not take OptionalCallExpression into account. But in this case I fix this bug by removing the code lol.

@JLHwung JLHwung added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Dec 30, 2019
@JLHwung JLHwung added this to the v7.8.0 milestone Dec 30, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit 7bc22e4 into babel:master Jan 10, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the fix-numeric-separator branch January 10, 2020 01:28
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 10, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Numeric Separator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants