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

NumericLiteral text property strips the number of separators #53182

Closed
tmcw opened this issue Mar 9, 2023 · 11 comments · Fixed by #57144
Closed

NumericLiteral text property strips the number of separators #53182

tmcw opened this issue Mar 9, 2023 · 11 comments · Fixed by #57144
Labels
Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript

Comments

@tmcw
Copy link

tmcw commented Mar 9, 2023

Bug Report

The code:

1_0

Is parsed into a NumericLiteral expression with text "10". Example on astexplorer: https://astexplorer.net/#/gist/6cb9fb64646ae587070a8a15083a46fb/4143393d63a1ce5d1e6076bd690af26b56dfa625

The ramification of this is that if you parse a document with typescript's parser and print it with typescript's printer, any numbers with separators are transformed into versions without separators.

Ideally, the text of NumericLiterals would reflect the text in the document, or if there's a separate text stored somewhere, then ts.createPrinter would use that more accurate version.

🔎 Search Terms

  • NumericLiteral
  • separator
  • Number

🕗 Version & Regression Information

Exists at least from 4.7.4 to 4.9

⏯ Playground Link

Playground link with relevant code

💻 Code

1_0

🙁 Actual behavior

Numeric separators are not included in the text of parsed numbers.

🙂 Expected behavior

They should be, so that parsing & re-printing can maintain fidelity.

@fatcerberus
Copy link

The separator is retained when targeting esnext: Playground

@MartinJohns
Copy link
Contributor

MartinJohns commented Mar 9, 2023

It keeps the separator when targeting ESNext. I'm not sure which ECMAScript version it's officially added, figuring this out always seems close to impossible to me.

edit: Too slow, the Cerberus was faster once again.

@fatcerberus
Copy link

I'm not sure which ECMAScript version it's officially added, figuring this out always seems close to impossible to me.

Good to know I'm not the only one.

@MartinJohns
Copy link
Contributor

This StackOverflow post and this v8 documentation claim it's part of ES2021, so it would be a bug in TypeScript.

@tmcw
Copy link
Author

tmcw commented Mar 9, 2023

Wow, that was fast 😆 Looks like indeed this is supported for transpilation with latest (though it might want to be part of 2021), and I'm still not sure if it's a thing or not if you got through the createSourceFile / createPrinter flow:

import ts from "typescript";

const printer = ts.createPrinter({});
const sourceFile = ts.createSourceFile(
  "index.ts",
  "1_0",
  ts.ScriptTarget.ESNext,
  true
);
const p = printer.printFile(sourceFile);
console.log(p); // 10

@RyanCavanaugh
Copy link
Member

Printback is only guaranteed to be semantics-preserving, not format-preserving.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Mar 9, 2023
@DanielRosenwasser
Copy link
Member

FWIW the ECMA-262 spec tries to do a better job about this these days.

image

You also can view the multipage version (https://tc39.es/ecma262/multipage/) instead of the ginormous single page spec. 😄

@fatcerberus
Copy link

@DanielRosenwasser Hmm, so numeric separators are ES2021. Is it a bug then that TS doesn't preserve them in the output unless you target ESNext?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 10, 2023

Ryan's right that we can't always guarantee preservation, but I kind of feel like it is a bug - we do keep 0b110101 and 0o1234567 preserved if we can.

I don't know what's causing us to lose it - I'm having a hard time finding that.

It's at the very least likely that the following code needs to be changed:

if (numericLiteralFlags & TokenFlags.BinaryOrOctalSpecifier) node.transformFlags |= TransformFlags.ContainsES2015;

Basically, I'd rewrite this as

- if (numericLiteralFlags & TokenFlags.BinaryOrOctalSpecifier) node.transformFlags |= TransformFlags.ContainsES2015;
+ if (numericLiteralFlags & TokenFlags.NumericLiteralFlags) {
+     if (numericLiteralFlags & TokenFlags.BinaryOrOctalSpecifier) node.transformFlags |= node.transformFlags |= TransformFlags.ContainsES2015;
+     if (numericLiteralFlags & TokenFlags.ContainsSeparator) node.transformFlags |= node.transformFlags |= TransformFlags.ContainsES2021;
+ }

But first you'd need to find where that lossiness is happening.

@fatcerberus
Copy link

Ryan's right that we can't always guarantee preservation

Yeah, I got that, it's the fact that it's preserved under ESNext that made me suspect "bug" - this suggests it can be preserved but it's deliberately dropping it for non-ESNext targets.

@RyanCavanaugh
Copy link
Member

IOW if someone wants to change this they can, it's just upholding an invariant that we don't actually have.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Experience Enhancement Noncontroversial enhancements Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". and removed Not a Defect This behavior is one of several equally-correct options labels Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants