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

Escape \0 followed by a number as a hex escape to avoid printing an octal #18026

Merged
merged 1 commit into from
Aug 24, 2017

Conversation

weswigham
Copy link
Member

Fixes #17226

@weswigham weswigham merged commit 2f1bd8c into master Aug 24, 2017
@weswigham weswigham deleted the decaffinate-octals branch August 24, 2017 22:52
@clydin
Copy link
Contributor

clydin commented Feb 8, 2018

This fix is breaking other use cases. For example, if there is a string literal node containing the text \0042, it will be emitted as \x00042 when it should not be altered (excepting the escape of the \) and definitely not include an extra 0.

@weswigham
Copy link
Member Author

TBQH, we should be flagging string literal nodes containing octal escapes as errors - see #396, the reason being that they're ambiguous and behavior depends on the runtime (old or new). Specifically, under strict mode \0042 is just an error (which is why we never want to emit them).

@clydin
Copy link
Contributor

clydin commented Feb 9, 2018

I agree, but the error (and any normalization) should probably be at the parsing stage.
If an actual StringLiteral node's text is \0042 then the output of that node should be "\\0042" not "\\x00042" or "\\x0042". Just as if the node's text is \n; it's output would be (and currently is) "\\n". The printer should ideally be limited to generating syntactically correct output and not correcting for ambiguous behavior in input user code as such code should never (barring some hypothetical unsafe option) get that far into the full compilation pipeline.

@clydin clydin mentioned this pull request Feb 9, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 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.

4 participants