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

Disallow octal literals in strict mode and ES5 #385

Merged
merged 7 commits into from Aug 7, 2014
Merged

Disallow octal literals in strict mode and ES5 #385

merged 7 commits into from Aug 7, 2014

Conversation

JsonFreeman
Copy link
Contributor

This change alters the way we parse octal literals to match the behavior of the old compiler. We also error now on octal literals in strict mode and -t es5.

There is one breaking change from the old compiler. Namely, in strict mode in ES3 (which doesn't exist in javascript anyway), we will error on octal literals, whereas the old compiler did not.

Next work item is octal escape sequences, which the old compiler did not detect.

@vladima
Copy link
Contributor

vladima commented Aug 7, 2014

👍

@JsonFreeman
Copy link
Contributor Author

I will add tests for those cases, but they do seem to behave correctly.

-03 does fail. The parser does not need to check negatives because the minus sign is part of the parent node (prefix unary expression). Same for +03 and ++03.

009 has two errors, which matches the behavior of the old compiler. The scanner yields 00 and 9 as 2 separate numbers, so even though the regex would match 009, it will never see a number with this spelling. Maybe I should add a comment to that effect.

The only deviation from the ecmascript spec is that 09 succeeds, whereas the spec considers it malformed. I did this to align with the old compiler's behavior.

// we would get 00 and 9 as two separate tokens.
// We also do not need to check for negatives because any prefix operator would be part of a
// parent unary expression.
if (/0[0-7]+/.test(numberLiteralSource)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just test the first and second char in the source instead of a regex

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, I'd simply do this check first and do the ES5/strict mode checking in the body, since you're less likely to be writing octal literals and that'll short-circuit the check.

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
Contributor

Choose a reason for hiding this comment

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

V8 allows 009 in strict mode, and treats it as 9.

Perhaps TS should either follow the same behaviour, or generate a warning.

@DanielRosenwasser
Copy link
Member

👍

@mihailik
Copy link
Contributor

mihailik commented Aug 7, 2014

V8 considers 009 a valid literal for number 9 (both strict and non-strict). I wonder whether TS should follow, or at least give a warning if it deviates. After all TS itself runs in node half the time.

@JsonFreeman
Copy link
Contributor Author

That is true, but we'd like to align as best we can with the ecmaScript 5 spec, and with the TypeScript 1.0 compiler. That is the rationale for these rules. Rejecting 009 is consistent with both the spec and the old compiler, so it would not break any existing code.

JsonFreeman added a commit that referenced this pull request Aug 7, 2014
Disallow octal literals in strict mode and ES5
@JsonFreeman JsonFreeman merged commit b9124a7 into master Aug 7, 2014
@JsonFreeman JsonFreeman deleted the octal branch August 7, 2014 20:07
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants