[parser] Disallow numeric separator in unicode scape sequences#10468
[parser] Disallow numeric separator in unicode scape sequences#10468nicolo-ribaudo merged 7 commits intobabel:masterfrom
Conversation
ivandevp
commented
Sep 19, 2019
| Q | A |
|---|---|
| Fixed Issues? | Fixes #10460 |
| Patch: Bug Fix? | Yes |
| Major: Breaking Change? | No |
| Minor: New Feature? | No |
| Tests Added + Pass? | Yes |
| Any Dependency Changes? | No |
| License | MIT |
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11657/ |
|
|
||
| let total = 0; | ||
|
|
||
| // Called from readHexChar |
There was a problem hiding this comment.
Other than readCodePoint, readHexChar could also be called from readRadixNumber. Did you checked whether 0xf_f can be successfully parsed?
We could add an extra allowNumSeparator: boolean to readHexChar
readHexChar(len: number, throwOnInvalid: boolean, allowNumSeparator: boolean): number | nulland set allowNumSeparator = true when calling from readCodePoint.
|
|
||
| if (unicode.includes("_")) { | ||
| const numSeparatorPos = this.input.indexOf("_"); | ||
| this.raise( |
There was a problem hiding this comment.
I don't think we should throw an error like this if user does not enable numericSeparator parser plugin. Consider moving the error logic inside this.hasPlugin("numericSeparator") branch.
| const unicode = this.input.slice(this.state.pos, this.state.pos + len); | ||
|
|
||
| if (unicode.includes("_")) { | ||
| const numSeparatorPos = this.input.indexOf("_"); |
There was a problem hiding this comment.
This doesn't work in case there is another _ in the input somewhere before the number. e.g.
_;
"\x1_2"It would be better to move this check inside the for loop right after, where there already are the other errors about numeric separators.
| this.raise(this.state.pos, "Invalid or unexpected token"); | ||
| } | ||
|
|
||
| if (allowNumSeparator) { |
There was a problem hiding this comment.
Shouldn't it only throw if it is not allowed?
There was a problem hiding this comment.
Oh... My bad. I'll fix it right now.
| if (!allowNumSeparator) { | ||
| this.raise( | ||
| this.state.pos, | ||
| "Numeric separators are not allowed inside unicode escape sequences", |
There was a problem hiding this comment.
The Numeric separators are also not allowed in hex escape sequences:
var c = \x1_0| "Numeric separators are not allowed inside unicode escape sequences", | |
| "Numeric separators are not allowed inside unicode escape sequences or hex escape sequences", |
Could you also add a test case on hex escape sequences?
| // Used to read character escape sequences ('\x', '\u'). | ||
|
|
||
| readHexChar(len: number, throwOnInvalid: boolean): number | null { | ||
| readHexChar( |
There was a problem hiding this comment.
readHexChar is only used for \x and \u, so we don't have to expose allowNumSeparator to its caller.
There was a problem hiding this comment.
🤔 Good catch. So, now it is exposed to readInt's callers and changed to false when called from readHexChar.