[parser] Invalid NonOctal Decimal by zant · Pull Request #10467 · babel/babel

zant · 2019-09-19T14:43:22Z

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

babel-bot · 2019-09-19T14:51:17Z

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

JLHwung · 2019-09-19T15:36:48Z

packages/babel-parser/src/tokenizer/index.js

-        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) {


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

JLHwung · 2019-09-19T15:44:45Z

packages/babel-parser/src/tokenizer/index.js

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


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;

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"

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!

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

The spec calls it NonOctalDecimalIntegerLiteral, so maybe nonOctalDecimalInt?

But if you prefer isNonOctal, it's ok 👍

nicolo-ribaudo · 2019-09-21T00:23:16Z

packages/babel-parser/src/tokenizer/index.js

-        if (isFloat || octal) this.raise(start, "Invalid BigIntLiteral");
+        // disallow numeric separators in non octal decimals
+        if (this.hasPlugin("numericSeparator") && isNonOctalDecimalInt) {
+          const underscorePos = this.input


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

0_1;

nicolo-ribaudo

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 · 2019-09-22T23:07:40Z

Now all green again 🎉

JLHwung · 2019-09-23T19:17:29Z

packages/babel-parser/src/tokenizer/index.js

+        .indexOf("_");
+      if (underscorePos > 0) {
+        this.raise(
+          underscorePos,


Shouldn't it be start + underscorePos?

void 0_n;

JLHwung

@nicolo-ribaudo will resolve the conflicts.

nicolo-ribaudo · 2019-09-23T21:08:26Z

scripts/tests/test262/test262_whitelist.txt

 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)


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

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

Naa, just a PR

zant · 2019-09-23T21:12:56Z

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

zant changed the title ~~Invalid NonOctal Decimal~~ [parser] Invalid NonOctal Decimal Sep 19, 2019

JLHwung reviewed Sep 19, 2019

View reviewed changes

existentialism added pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: BigInt labels Sep 19, 2019

zant marked this pull request as ready for review September 20, 2019 20:56

nicolo-ribaudo reviewed Sep 21, 2019

View reviewed changes

nicolo-ribaudo approved these changes Sep 22, 2019

View reviewed changes

JLHwung reviewed Sep 23, 2019

View reviewed changes

JLHwung approved these changes Sep 23, 2019

View reviewed changes

gonza added 12 commits September 23, 2019 23:05

Add test

a4bcbaf

Add nonOctalDecimal verification

2ac7281

Update regex and code style

36b23ff

Refactor non octal detection

6bf61b6

Add numeric separator test

63288b3

Disallow numeric separators in non octals

ad99d8d

Update whitelist

9c26846

Better test naming

dbaf284

Disallow numeric separators in non oct for all numbers

a4c9bcf

Specific error above the general error

ae6025e

Add test for invalid leading zero in num separator

4b8e265

Add start position to error

5283e6c

nicolo-ribaudo merged commit 69d00dc into babel:master Sep 23, 2019

nicolo-ribaudo reviewed Sep 23, 2019

View reviewed changes

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

snyk-bot mentioned this pull request Oct 22, 2019

[Snyk] Upgrade @babel/runtime from 7.6.2 to 7.6.3 cds-snc/report-a-cybercrime#834

Merged

zant deleted the bigint-non-octal-decimal branch October 23, 2019 03:38

snyk-bot mentioned this pull request Oct 24, 2019

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.6.3 Matthelonianxl/drizzle#9

Open

snyk-bot mentioned this pull request Nov 1, 2019

[Snyk] Upgrade @babel/core from 7.2.2 to 7.6.4 nbv1982/material-ui#5

Open

snyk-bot mentioned this pull request Nov 12, 2019

[Snyk] Upgrade @babel/runtime from 7.1.5 to 7.6.3 ajesse11x/Parse-SDK-JS#1

Open

This was referenced Nov 28, 2019

[Snyk] Upgrade @babel/runtime from 7.0.0-beta.42 to 7.7.2 ali8889/mist#4

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 dyna-dot/CyberChef#3

Open

This was referenced Dec 5, 2019

[Snyk] Upgrade @babel/runtime from 7.3.1 to 7.7.4 dyna-dot/bit-javascript#5

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 jakeherp/todo-list#1

Merged

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 bot locked as resolved and limited conversation to collaborators Jan 24, 2020

Uh oh!

Conversation

zant commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

zant commented Sep 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zant commented Sep 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zant commented Sep 19, 2019 •

edited

Loading

babel-bot commented Sep 19, 2019 •

edited

Loading