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

[parser] Invalid NonOctal Decimal #10467

Merged
merged 12 commits into from Sep 23, 2019
Merged

[parser] Invalid NonOctal Decimal #10467

merged 12 commits into from Sep 23, 2019

Conversation

zant
Copy link
Contributor

@zant zant commented Sep 19, 2019

Q                       A
Fixed Issues? Fixes #10465
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Any Dependency Changes? No
License MIT

I added a regex verification for a NonOctal Decimal Integer, defined as a sequence of decimal digits starting with 0 and with at least two characters, i did it within the scope of the if (octal) because octal ensure two of the requirements, and there was a similar octal verification.

Todo:

  • Update the whitelist

@zant zant changed the title Invalid NonOctal Decimal [parser] Invalid NonOctal Decimal Sep 19, 2019
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 19, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11647/

if (isFloat || octal) this.raise(start, "Invalid BigIntLiteral");
// disallow floats, legacy octal syntax and non octal decimals
// new style octal ("0o") is handled in this.readRadixNumber
if (isFloat || octal || isNonOctalDecimal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does isNonOctalDecimal == true imply (octal || isNonOctalDecimal) == true ? If so we could consider removing octal check here.

octal = false;
}
if (/[89]/.test(number)) octal = false;
if (/^[0-9]*$/.test(number)) isNonOctalDecimal = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

octal == true implies that startsWithDot == false (because it starts with zero), which in turn means that this.readInt(10) != null, or otherwise we would throw Invalid number error here.

if (!startsWithDot && this.readInt(10) === null) {
      this.raise(start, "Invalid number");
}

IIRC this.readInt(10) would ensure the input from start to this.state.pos to always be [0-9], if so we don't have to check [0-9] again, which means isNonOctalDecimal equals to the previous definition of octal:

this.state.pos - start >= 2 && this.input.charCodeAt(start) === charCodes.digit0;

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so it would be enought to do

if (/[89]/.test(number)) {
  octal = false;
  isNonOctalDecimal = true;
}

So that isNonOctalDecimal literally means "we tought that it was an octal number, but it turns out that it isn't"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, i didn't see it in that way, it took me a while to realize why the test i added was passing, but it's because when octal is set to false here:

if (/[89]/.test(number)) octal = false;

isNonOctalDecimal remains true, so it can throw the error here:

 if (isFloat || octal || isNonOctalDecimal) {...}

It's confusing, thanks for the review, i'll come back with some changes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo Nice! Also, the decimal part isn't confusing? Sounds like fractional numbers, maybe it can only be isNonOctal?

Copy link
Member

Choose a reason for hiding this comment

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

The spec calls it NonOctalDecimalIntegerLiteral, so maybe nonOctalDecimalInt?

But if you prefer isNonOctal, it's ok 👍

@existentialism existentialism added pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: BigInt labels Sep 19, 2019
@zant zant marked this pull request as ready for review September 20, 2019 20:56
if (isFloat || octal) this.raise(start, "Invalid BigIntLiteral");
// disallow numeric separators in non octal decimals
if (this.hasPlugin("numericSeparator") && isNonOctalDecimalInt) {
const underscorePos = this.input
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this whole if out of the if (this.hasPlugin("bigInt"))? It also applies to normal numbers:

0_1;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

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.

Could you run make test-test262-update-whitelist again and add a test for 0_8 which is now correctly disallowed by this PR?

@zant
Copy link
Contributor Author

zant commented Sep 22, 2019

Now all green again 🎉

.indexOf("_");
if (underscorePos > 0) {
this.raise(
underscorePos,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be start + underscorePos?

void 0_n;

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

@nicolo-ribaudo will resolve the conflicts.

@nicolo-ribaudo nicolo-ribaudo merged commit 69d00dc into babel:master Sep 23, 2019
language/literals/bigint/numeric-separators/numeric-separator-literal-nonoctal-09-err.js(default)
language/literals/bigint/numeric-separators/numeric-separator-literal-nonoctal-0_8-err.js(default)
language/literals/bigint/numeric-separators/numeric-separator-literal-nonoctal-0_9-err.js(default)
language/literals/bigint/numeric-separators/numeric-separator-literal-nzd-nsl-dds-leading-zero-err.js(default)
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-00-err.js(default)
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-01-err.js(default)
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-07-err.js(default)
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-0_0-err.js(default)
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-0_1-err.js(default)
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-0_7-err.js(default)
Copy link
Member

Choose a reason for hiding this comment

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

If you want to open another PR to fix these tests, you are welcome to do so!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, i would like to do so! Should i make an issue or just as a follow up of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Naa, just a PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Goodd 👌

@zant
Copy link
Contributor Author

zant commented Sep 23, 2019

Thanks guys! Appreciate the review, I learned a lot 🙏

@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 Sep 23, 2019
@zant zant deleted the bigint-non-octal-decimal branch October 23, 2019 03:38
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 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 pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: BigInt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The BigInt suffix should be disallowed in NonOctal Decimal Integer Literal
5 participants