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

BigInt support #25886

Merged
merged 13 commits into from
Nov 5, 2018
Merged

BigInt support #25886

merged 13 commits into from
Nov 5, 2018

Conversation

calebsander
Copy link
Contributor

@calebsander calebsander commented Jul 24, 2018

Fixes #15096.

Additions:

  • The bigint primitive type, which is analogous to but distinct from number
  • Parsing of bigint literals, e.g. 123n. Per the ECMAScript proposal, literals can be expressed in binary (0b), octal (0o), hex (0x), or decimal formats. bigint literals, like number, string, or boolean literals, are valid types. The config option experimentalBigInt must be set in order to use bigint literals.
  • Arithmetic operators work on bigint values and return bigint results
  • Updated the return type of the (JavaScript) typeof operator to include "bigint"
  • esnext.bigint lib file, which defines the BigInt, BigInt64Array, and BigUint64Array types, the BigInt global, and the *int64* methods of DataView
  • 6 tests:
    • parseBigInt to test parsing bigint literals
    • numberVsBigIntOperations to test that arithmetic operations accept and return the correct types
    • warnExperimentalBigIntLiteral to test that using bigint literals without the --experimentalBigInt option throws an error
    • bigintWithLib and bigintWithoutLib to test the esnext.bigint lib file
    • bigintIndex to assert that bigint values can't be used as indices

Notes:

bigint is now a reserved word.

The type of arithmetic operations with any types, e.g. <any>1 * <any>2, was left as number instead of changing it to the more accurate number | bigint in order to maintain compatibility. Binary arithmetic operations are not allowed on operands of number | bigint type because it is possible that their runtime types conflict.

Error messages referring to the type of arithmetic operands were updated, as was the type of typeof expressions, which required updating many reference baselines. Other than that, no existing tests' expected behaviors were changed. (See commits fd5771a and 73d0fbf.)

bigint is not allowed as an index type. Use a string index instead and explicitly convert the bigint to a string.

@msftclas
Copy link

msftclas commented Jul 24, 2018

CLA assistant check
All CLA requirements met.

@calebsander
Copy link
Contributor Author

calebsander commented Jul 24, 2018

The lint is failing because tslint doesn't know about the esnext.bigint library. Seems like a chicken-and-egg situation: this push is needed to add the esnext.bigint library, but tslint needs to already have the changes in order to lint successfully.

I removed the esnext.bigint dependency by declaring the BigInt global in types.ts instead. This seems a bit of a hacky fix, so I would appreciate any suggestions for a better way to do it.

@calebsander
Copy link
Contributor Author

calebsander commented Jul 24, 2018

Also not sure how to avoid running the bigint tests on versions of Node.js below 10, as they will fail to compile since BigInt is not defined. As far as I can tell, there is no way to support compiling TypeScript code containing bigint literals in environments where BigInt is not defined, aside from including some JS/TS arbitrary-precision integer library. At the very least, the checker would need some way to convert arbtrary-length bigint literals to their decimal string representation, so e.g. 3n and 0b11n can be resolved to the same type.

@Kingwl
Copy link
Contributor

Kingwl commented Jul 24, 2018

cool, a big work

@DanielRosenwasser
Copy link
Member

Hey @calebsander, thanks for the PR! However there are a couple of things that we should be up-front with you about:

  1. Support for TC39 "BigInt: Arbitrary precision integers in JavaScript" proposal #15096 was never marked as help wanted, and was already assigned to someone on our team. As a result someone on our team has also been working on BigInt support. We'll have to choose one or the other, or find a way to reconcile both changes.
  2. This is a fairly large change, so it may be difficult to review over time.

I think @mhegazy and @sheetalkamat will be able to provide more detail here, but hopefully we can still leverage the work from both sides.

@calebsander
Copy link
Contributor Author

calebsander commented Jul 24, 2018

It is actually only a ~1000 line change (600 of which are from the esnext.bigint lib), ignoring the updates to the reference baselines (due to the changed type of typeof x and the changed error messages about arithmetic operands). It would be possible to merge the esnext.bigint lib separately if desired, since it is not currently being used in the compiler.

@calebsander
Copy link
Contributor Author

I reverted the LKG for now and removed references to bigint in the compiler code. The compiler should emit the same JS, but its typings are not as accurate. Once the LKG is rebuilt with this code, 74fd6d3 should be reverted so that the compiler code has the correct types.

@weswigham
Copy link
Member

Ah, another general remark: It looks like your parse handles it, but you need tests of bigint literals of all shapes and sizes with various configurations of numeric separators. I'd just add new entries to the numeric separator tests, myself, since those contain all kinds of literals with numeric separators in them.

@sheetalkamat
Copy link
Member

@mhegazy had mentioned that we should enable bigInt only under --experimentalBigInt compiler options flag.

@calebsander
Copy link
Contributor Author

What features would you imagine being restricted without the flag? Just bigint literals? The fact that typeof can return "bigint"? The bigint type keyword? Compiling the esnext.bigint lib requires the bigint keyword to be recognized, so if that is disabled without the flag, the tsconfig.json file will have to be modified in order to build the lib. I had issues with tslint failing on new options in tsconfig.json, so I'm not sure how that would work.

I'm not sure an experimental flag is necessary; no existing code should break with the change unless it is using the bigint identifier. I'm also not convinced "experimental" really makes sense; this doesn't seem like a feature whose behavior is likely to change.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 25, 2018

What features would you imagine being restricted without the flag? Just bigint literals?

yes. basically any expression of type bigint should be flagged.
I should add that we want the error reported only once in a compilation to avoid noise.

I'm not sure an experimental flag is necessary;

The flag is not necessary for valid code generation. the flag is needed to inform the user that there is potential change in semantics that could happen in a future release. that is the same as decorators.
The alternative here is not to include this feature until it reaches stage 4.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 26, 2018

Just chipping in from the sides. While the proposal is at Stage 3, by TC39's definition, there is a risk of implementation challenges which can effect the spec. For example, this behaviour isn't fully settled, while it unlikely to impact the TypeScript implementation, the flag is the only way to underline to users that it isn't fully baked yet and until it has an ES release there is a danger of how it gets aligned. It should be ES2019 but...

For something like this, it feels fairly important to flag it until the proposal reaches Stage 4.

@calebsander
Copy link
Contributor Author

Sure, that makes sense. I already added the flag. It will warn on any bigint literals, or if it sees the bigint keyword. I think I've addressed all the comments on the PR, with the exception of TypeFlags exceeding 30 bits. I looked into trying to fix it by factoring out the literal flags, but it looks like there are hundreds of locations that would have to be updated. I would rather keep that for a separate PR. I think this PR should be ready to merge if there are no further comments.

The Node.js 8 Travis job seems to have failed on the last build because npm couldn't resolve the jake executable. Not sure what would cause that. Otherwise, the builds are all passing.

@weswigham
Copy link
Member

weswigham commented Jul 26, 2018

I already added the flag. It will warn on any bigint literals, or if it sees the bigint keyword.

I think we don't want to error in the keyword, just the literals. We don't want to error when the node .d.ts updates to use bigints in stats, for example - just on usages we won't be downleveling. Although @mhegazy has final say there.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 28, 2018

I think we don't want to error in the keyword, just the literals. We don't want to error when the node .d.ts updates to use bigints in stats, for example - just on usages we won't be downleveling.

That sounds right to me. in general we should not error in an ambient context any ways..

@calebsander
Copy link
Contributor Author

calebsander commented Jul 28, 2018

I removed the error on the bigint keyword and made every bigint literal error.

@calebsander
Copy link
Contributor Author

Hmm, it seems like the Travis build failure on Node 8 is due to jake not being installed in the node_modules cache, since it went away when I removed the cache field from .travis.yml. Not sure what can be done to fix that, but I don't think there is any problem with the PR code.

@mhegazy mhegazy requested a review from sheetalkamat July 31, 2018 18:30
@sheetalkamat
Copy link
Member

Also need testcase for usage of bigint literals, type and BigInt() when esNext and when less than esNext with and without experimentalBigInt

@calebsander
Copy link
Contributor Author

Also need testcase for usage of bigint literals, type and BigInt() when esNext and when less than esNext with and without experimentalBigInt

experimentalBigInt and "target": "esnext" vs. pre-esnext are currently orthogonal options: experimentalBigInt controls whether errors are raised on bigint literals, whereas target only affects whether the esnext.bigint lib is included. Regardless of both options, bigint can be used as a type without raising any errors. (I'm not sure whether it makes sense to allow the bigint keyword without experimentalBigInt, but I think it should behave the same in ambient and non-ambient settings. I think @weswigham's point about not throwing errors just because @types/node is included is really more about included libraries vs. user-written code than about ambient vs. non-ambient contexts.)

The parseBigInt and warnExperimentalBigIntLiteral tests already verify the behavior of the having experimentalBigInt set and unset. I added tests verifying the behavior of variables and types declared in esnext.bigint with and without "target": "esnext", including the non-primitive resolution of bigint (to BigInt with "target": "esnext", and to {} without esnext).

@sheetalkamat sheetalkamat merged commit 576fdf9 into microsoft:master Nov 5, 2018
@sheetalkamat
Copy link
Member

Thank you @calebsander for this contribution. It should be available in tonight's nightly build.

@joshuajbouw
Copy link

joshuajbouw commented Nov 15, 2018

Hey @calebsander, I just updated to the Nightly from your own repo and now I am having TSLint issues, but it is compiling correctly. What were the flags to remove the warnings again? I think I noticed that you deleted them, or removed them.

@calebsander
Copy link
Contributor Author

I'm not sure what is required for TSLint to work, but for tsc to succeed you should only have to set "target": "esnext". This will prevent warnings for BigInt literals and include the esnext.bigint standard library declarations. The developers removed the experimentalBigInt flag; it's now indicated by setting the target version.

@joshuajbouw
Copy link

joshuajbouw commented Nov 15, 2018

"esnext.bigint", where would I find these? I do have my target already set at esnext, still not fixed on Atom or VS Code. So, I guess that I'm just missing the standard library declarations but, I didn't understand exactly where to put them.

So to clarify, tsc does work. Its just TSLint that is showing that Typescript doesn't know what the BigInt features are. Not TSLint itself.

Thanks.

@weswigham
Copy link
Member

I do have my target already set at esnext, still not fixed on Atom or VS Code.

You need to set vscode to use your workspace version of typescript and install the @next version in your workspace - this hasn't shipped bundled with the editor yet, and won't until after the 3.2 release in the coming few weeks.

@calebsander
Copy link
Contributor Author

"esnext.bigint", where would I find these?

This is just the declaration file that specifies BigInt, BigInt64Array, etc. If you click Go to Definition in VS Code on the BigInt constructor, it should take you to the esnext.bigint file where it is defined (which should be node_modules/typescript/lib/lib.esnext.bigint.d.ts). As @weswigham said, you have to make VS Code use your workspace version of TypeScript. This esnext.bigint declaration file is automatically included when you target esnext, but you can also include it manually by adding it to lib in your tsconfig.json.

@joshuajbouw
Copy link

Saviours, thank you. I hope your information will help the next person.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.