Stage 2: BigInt#588

wdhorton · 2017-06-18T23:45:25Z

Q	A
Bug fix?	no
Breaking change?	no
New feature?	yes
Deprecations?	no
Spec compliancy?	no
Tests added/pass?	yes
Fixed tickets	#569
License	MIT

For the tokenizer, check if the last character is n, in which case it should be treated as a BigInt. If it has a dot or an E, throw, otherwise, advance the position one character. Replace the literal n with "" when parsing the value, and create a new token type bigint.

For the parser, when handling a bigint token, create a node of the new type BigIntLiteral with the given value.

wdhorton · 2017-06-18T23:45:55Z

@littledan would love for you to take a look, let me know what other test cases I should add

hzoo · 2017-06-18T23:50:19Z

Awesome @wdhorton!!

As for test cases can you add hex, octal, binary and then error for exponential form (you already have the error for decimals)?

hzoo · 2017-06-18T23:54:12Z

test/fixtures/experimental/bigint/invalid-0/actual.js

@@ -0,0 +1 @@
+1.0n


can rename the invalid-0 to just invalid-decimal?

hzoo · 2017-06-18T23:54:37Z

README.md

What do we think about making the plugin name bigInt with camelCase?

I actually started with the plugin named bigInt. But then when it came time to make a new token type, I saw that the RegExp constructor mapped to regex so I figured that BigInt should map to bigint. So then I changed the plugin to match the token type.

But I'm ok with bigInt if you think that's better

Yeah I just meant for the plugin string not the tt.bigint in the code if that makes sense (I wasn't clear earlier 😄)

bigInt seems right for the plugin if the convention is camelCase, while bigint is a pretty reasonable name for a tag IMO, as typeof 1n is "bigint".

Yep that's what I meant!

hzoo · 2017-06-19T00:00:04Z

src/tokenizer/index.js

    if (isIdentifierStart(this.fullCharCodeAtPos())) this.raise(this.state.pos, "Identifier directly after number");

-    const str = this.input.slice(start, this.state.pos).replace(/_/g, "");
+    const str = this.input.slice(start, this.state.pos).replace(/[_n]/g, "");


Is it worth putting a comment here to explain we are removing the _ for numeric separators and n for BigInt?

sound like a good idea

littledan

For missing test cases: It'd also be good to have a BigInt that isn't exactly representable as a Number, and check that the output is as expected (and the test expectation shouldn't contain any rounded version).

littledan · 2017-06-19T10:40:28Z

test/fixtures/experimental/bigint/valid-0/expected.json

Maybe the "value" (and possibly "rawValue", but I don't know much about what that field is for) fields should contain a string with the value of the BigInt as a decimal number (taking care of other bases). Using a Number as the value seems suboptimal as it'll be rounded; no one should use such a value.

rawValue is basically the same as value and raw is just the actual string of the code

this.addExtra(node, "rawValue", value); this.addExtra(node, "raw", this.input.slice(startPos, this.state.end));

@littledan I realized after I pushed this up that using a number as the value would defeat the purpose, since as you said we still couldn't represent 64-bit ints without rounding. I'll change it to strings

littledan · 2017-06-19T10:54:48Z

README.md

bigInt seems right for the plugin if the convention is camelCase, while bigint is a pretty reasonable name for a tag IMO, as typeof 1n is "bigint".

littledan · 2017-06-19T13:41:09Z

src/tokenizer/index.js

@@ -678,7 +687,7 @@ export default class Tokenizer extends LocationParser {
    } else {
      val = parseInt(str, 8);


None of these parseInt calls are appropriate if isBigInt, as they will round away part of the answer.

you're very right, that was an oversight on my part

mathiasbynens · 2017-06-22T07:56:52Z

src/tokenizer/index.js

How about using 0x6E instead of 110? It makes it more obvious that this refers to U+006E LATIN SMALL LETTER N.

The existing code was already checking the charCodes using non-hex integers, so I was just trying to stay consistent with that. I can go ahead and change it if you think that's best

mathiasbynens · 2017-06-22T07:57:23Z

src/tokenizer/index.js

Same could be done here (0x30 instead of 48)

mathiasbynens · 2017-06-22T07:57:59Z

src/tokenizer/index.js

if (next === 0x2B || next === 0x2D)

mathiasbynens · 2017-06-22T07:58:09Z

src/tokenizer/index.js

littledan · 2017-06-22T08:17:01Z

test/fixtures/experimental/bigint/valid-binary/expected.json

+            "rawValue": "0b101011101",
+            "raw": "0b101011101n"
+          },
+          "value": "0b101011101"


Should Babylon handle converting this binary value into a decimal value, or should the transform be responsible for that/

I was thinking about that, and on a practical level I think it makes sense to delay that until the transform. If there was a binary value that exceeded the max value for a number, then in order to convert it we'd have to import a bigint library here. Whereas with the transform we're already going to need to use a library for the operations, so it makes sense to have it do the conversions as well

Would this be the same question as for Numeric Separators? We have a babel-plugin-transform-es2015-literals that does binary conversion but only for regular numbers.

https://github.com/babel/babel/pull/5793/files#diff-16974ce04e951262c4c093ce8d8a149cR94

I guess in this case since it's a BigInt, we'd make the new transform handle it. I'd say the transform.

Oh that makes sense; I didn't realize this was typically handled in a separate transform.

Does it make sense to have the 'value' field at all, though? Why bother, when you have the rawValue?

Not sure, I guess most of the time value works for all Literals depending on what you want via Boolean, Null, String, etc. I guess for this it's the same thing and can't actually be represented as a number

I think it's fine to include it in the BigInt transform itself if es2015-literals are only going to handle binary, octal, etc for regular numbers.

wdhorton · 2017-06-27T23:30:14Z

just added @mathiasbynens suggestions

wdhorton · 2017-06-27T23:30:36Z

@hzoo @littledan is there anything keeping this from getting merged now?

hzoo · 2017-06-28T03:32:04Z

Yeah we can iterate when we get to the transform part if it doesn't fit, tests wise it's fine to me!

hzoo · 2017-06-28T03:36:17Z

Thanks @wdhorton for the PR, let us know if you want help with the transform as well

wdhorton · 2017-06-28T03:40:47Z

@hzoo i have one starting question on the transform—is it ok to pull in an external lib to handle the math operations? idk what babel's policy is on adding 3rd-party dependencies, but it seems unavoidable in this case

existentialism · 2017-06-28T03:50:07Z

@wdhorton shouldn't be an issue (guess it depends on the dep?)... we pull in regexpu for transform-es2015-unicode-regex for instance.

hzoo · 2017-06-28T03:50:11Z

Yeah it needs a runtime library like described in #569. It's basically the syntax wrapper -> those calls to the library. It's similar to transform-runtime/regenerator in that way

Stage 2: BigInt

06cea2d

hzoo requested a review from littledan June 18, 2017 23:50

hzoo added spec-new Spec: BigInt labels Jun 18, 2017

hzoo reviewed Jun 18, 2017

View reviewed changes

test/fixtures/experimental/bigint/invalid-0/actual.js

@@ -0,0 +1 @@

1.0n

Copy link

Member

hzoo Jun 18, 2017

Choose a reason for hiding this comment

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

can rename the invalid-0 to just invalid-decimal?

hzoo reviewed Jun 18, 2017

View reviewed changes

hzoo reviewed Jun 19, 2017

View reviewed changes

babel deleted a comment from codecov bot Jun 19, 2017

littledan suggested changes Jun 19, 2017

View reviewed changes

wdhorton added 2 commits June 19, 2017 20:40

Change plugin name to bigInt (camelcase).

1062e15

Update based on PR review, add test cases.

771bc1b

mathiasbynens reviewed Jun 22, 2017

View reviewed changes

src/tokenizer/index.js Outdated

Copy link

Contributor

mathiasbynens Jun 22, 2017

Choose a reason for hiding this comment

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

Same could be done here (0x30 instead of 48)

mathiasbynens reviewed Jun 22, 2017

View reviewed changes

src/tokenizer/index.js Outdated

Copy link

Contributor

mathiasbynens Jun 22, 2017

Choose a reason for hiding this comment

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

if (next === 0x2B || next === 0x2D)

mathiasbynens reviewed Jun 22, 2017

View reviewed changes

src/tokenizer/index.js Outdated

Copy link

Contributor

mathiasbynens Jun 22, 2017

Choose a reason for hiding this comment

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

Same here

littledan reviewed Jun 22, 2017

View reviewed changes

Use hex for charCodes.

5ac96e0

hzoo requested a review from danez June 28, 2017 03:29

hzoo merged commit baa5f4d into babel:master Jun 28, 2017

hzoo added the Tag: New Feature label Jun 28, 2017

jamiebuilds mentioned this pull request Sep 13, 2017

BigInt estree/estree#169

Closed

		@@ -678,7 +687,7 @@ export default class Tokenizer extends LocationParser {
		} else {
		val = parseInt(str, 8);

Uh oh!

Conversation

wdhorton commented Jun 18, 2017

Uh oh!

wdhorton commented Jun 18, 2017

Uh oh!

hzoo commented Jun 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

littledan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdhorton commented Jun 27, 2017

Uh oh!

wdhorton commented Jun 27, 2017

Uh oh!

hzoo commented Jun 28, 2017

Uh oh!

hzoo commented Jun 28, 2017

Uh oh!

wdhorton commented Jun 28, 2017

Uh oh!

existentialism commented Jun 28, 2017

Uh oh!

hzoo commented Jun 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!