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

Numeric separated literals should disallow zero leading #39563

Closed
MaxGraey opened this issue Jul 11, 2020 · 7 comments · Fixed by #51837
Closed

Numeric separated literals should disallow zero leading #39563

MaxGraey opened this issue Jul 11, 2020 · 7 comments · Fixed by #51837
Labels
Bug A bug in TypeScript Help Wanted You can do this
Milestone

Comments

@MaxGraey
Copy link

MaxGraey commented Jul 11, 2020

Search Terms:
numeric separator

spec test for checking leading zero separated literal

Code

These literals should be invalid:

0_0
0_0.0
0_0e0_0
0_0_1

but this should valid:

1_0.0_1 // 10.01
1_0e0_1 // 100

Expected behavior:
SyntaxError: Numeric separator can not be used after leading 0.

Actual behavior:

0_0     // -> 0  valid but shouldn't
0_0.0   // -> 0  valid but shouldn't
0_0e0_0 // -> 0  valid but shouldn't
0_0_1   // -> 1  valid but shouldn't. Btw 001 is invalid in strict mode

1_0.0_1 // -> 10.01 ok
1_0e0_1 // -> 100 ok

playground

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jul 13, 2020
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jul 13, 2020
@RyanCavanaugh RyanCavanaugh added the Help Wanted You can do this label Jul 13, 2020
@IllusionMH
Copy link
Contributor

IllusionMH commented Jul 13, 2020

Browsers have different behavior in case of 00_1 or 077_77:
Chromium reports Uncaught SyntaxError: Invalid or unexpected token (for 0_1 reports Uncaught SyntaxError: Numeric separator can not be used after leading 0.)
SpiderMonkey reports Uncaught SyntaxError: numeric separators '_' are not allowed in numbers that start with '0'

Currently TS parses it as 2 tokens: NumericLiteral 00 (with Octal flag) and Identifier _1 (because scanOctalDigits will stop on _) and therefore reports 2 errors: Octal literals are not allowed in strict mode. and Cannot find name '_1'.

Question is: Should fix for this issue should handle only cases 0_ (and process 00_ as it currently does) or 00_ as well (and report error similar to one from SpiderMonkey)? If later - this might break bad code.

UPD. Strange that TS doesn't report An identifier or keyword cannot immediately follow a numeric literal. for 00_O1 but does for 0_O1.

@IllusionMH
Copy link
Contributor

@RyanCavanaugh I'd like to take on this issue.

According to spec test it should be an syntax error so I'll update scanOctalDigits to skip over _ or just use scanBinaryOrOctalDigits.
This will be "breaking change" for already invalid code.

Strange that TS doesn't report An identifier or keyword cannot immediately follow a numeric literal. for 00_O1 but does for 0_O1.

Will create separate issue to report errors in this case.

Do you have any objections?
If that "breaking change" is not feasible - fix for An identifier or keyword cannot immediately follow a numeric literal. error should be noticeable enough for cases like 00_1 or 077_77

P.S. I hope @a-tarasyuk doesn't work on this one 😄

@DanielRosenwasser
Copy link
Member

@IllusionMH I think that sounds reasonable, I don't expect it to be a large breaking change but we can run the test suite

@a-tarasyuk
Copy link
Contributor

a-tarasyuk commented Jul 17, 2020

Not sure that I understood the point to mention me in this issue 😕 . As I know, if the issue marked with the Backlog/Help Wanted/GFI label anyone can try to resolve it, maybe I'm wrong and @DanielRosenwasser 👈 can better clarify the current process. I do PRs from time to time (I can't even say that I did a lot of PRs 😟), however, I definitely cannot work on all issues 😂, unfortunately 😞 .

@IllusionMH
Copy link
Contributor

IllusionMH commented Jul 17, 2020

UPD. Somehow I managed to write this one ambiguous too 🤦‍♂️. Clarifications in cursive.

@a-tarasyuk It was some kind of positive joke and my real hope that I will have chance to submit PR this time because IMHO you fix a lot of issues and do it quickly (which is great, previously beat me by 2 minutes in other issue with a bit better solution 👍 and I learned new thing).
I also agree think that everyone can submit fix for these kinds of issues. Thank you for a bunch of submitted fixes, some of them I rely on every day! 🙇

I'm sorry if that mention was inappropriate and for ping.


As for this issue - I have fix for numbers starting with 08... and 09, and plan to finish "legacy octal literals" this weekend.

@DanielRosenwasser
Copy link
Member

Yeah, I understood it as a joke in a positive light - as in "@a-tarasyuk fixes a lot of issues really fast, I hope I can fix this one first!"

For what it's worth, we all really appreciate working with both of you on this issue tracker, so thank you both! Please don't feel bad about either of your contributions.

@evanw
Copy link
Contributor

evanw commented Jan 17, 2023

FWIW I have another data point for this: Someone recently reported this as a bug in esbuild's TypeScript-to-JavaScript transformer: evanw/esbuild#2833. The confusion is understandable because TypeScript still incorrectly considers this to be valid syntax. However, I'm planning to keep it a syntax error in esbuild because it's supposed to be an error in JavaScript. But it would be helpful if TypeScript also considered this to be a syntax error so that it would appear as an error when people write code like this in their IDE.

Edit: There are also additional bugs where TypeScript has a more permissive numeric parser than JavaScript. One such example is 089n which should be invalid syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
6 participants