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

@babel/eslint-parser: fix BigIntLiteral node to match ESTree spec #10827

Merged
merged 6 commits into from Dec 8, 2019

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Dec 6, 2019

Q                       A
Fixed Issues? Refs #10826
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? Yes, this adds the @babel/plugin-syntax-bigint plugin to the @babel/eslint-parser package.
License MIT

@babel/parser currently parses BigInt Literals (1n) as BigIntLiteral. The ESTree spec differs from this and gives it the type Literal and adds the bigint property (see spec here).

Additionally, Espree tokenizes BigInt Literals as type "Numeric".

I have tested this in Node 8 (no BigInt support) and Node 12, and these are the nodes that are created, respectively:

Node 8

{
  type: 'Literal',
  start: 10,
  end: 12,
  loc: {
    start: Position { line: 1, column: 10 },
    end: Position { line: 1, column: 12 } 
  },
  range: [ 10, 12 ],
  value: null,
  raw: '1n',
  bigint: '1' 
}

Node 12

{
  type: 'Literal',
  start: 10,
  end: 12,
  loc: {
    start: Position { line: 1, column: 10 },
    end: Position { line: 1, column: 12 }
  },
  range: [ 10, 12 ],
  value: 1n,
  raw: '1n',
  bigint: '1'
}

@kaicataldo kaicataldo changed the title WIP: eslint-parser supports BigInt Literal @babel/eslint-parser: fix BigIntLiteral node to ESTree spec Dec 6, 2019
@kaicataldo kaicataldo changed the title @babel/eslint-parser: fix BigIntLiteral node to ESTree spec @babel/eslint-parser: fix BigIntLiteral node to match ESTree spec Dec 6, 2019
@kaicataldo kaicataldo force-pushed the babel-eslint-bigint-literal branch 3 times, most recently from 2673b15 to 1e114a8 Compare December 6, 2019 06:40
@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser area: estree labels Dec 6, 2019
@@ -77,6 +77,9 @@ export default function(token, tt, source) {
flags: value.flags,
};
token.value = `/${value.pattern}/${value.flags}`;
} else if (type === tt.bigint) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Muuuuuch better 😄

@kaicataldo kaicataldo force-pushed the babel-eslint-bigint-literal branch 2 times, most recently from fe20f14 to cfb2ded Compare December 6, 2019 20:11
const { toJSON } = obj.prototype;
originalToJSONMap.set(obj, toJSON);
obj.prototype.toJSON = function() {
return this.toString();
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to ensure we're not passing parameters through here because BigInt.prototype.toString() takes a radix argument that must be between [2-36] and throws if it gets called with a different argument. It has a default value, though, so it's fine to not call it with anything.

@@ -1,3 +1,5 @@
/* global BigInt */
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll be able to remove this when we enable the latest version of @babel/eslint-config-internal.

"column": 12
}
},
"value": "1",
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't the actual correct value in the AST (it's the BigInt.prototype.toString() representation since BigInts can't be serialized to JSON), but it should be fine for testing purposes.

@kaicataldo
Copy link
Member Author

Hmm this is failing because BigInt doesn't appear to be defined in Flow. Should I just add a type for now?

@nicolo-ribaudo
Copy link
Member

It's better to add // $FlowIgnore before the line which is reporting an error so that it will tell us to remove it when we upgrade to a version that supports bigints.

@kaicataldo
Copy link
Member Author

Makes sense! Will do.

@nicolo-ribaudo nicolo-ribaudo merged commit fb100ee into babel:master Dec 8, 2019
@nicolo-ribaudo
Copy link
Member

✔️

estreeParseBigIntLiteral(value: any): N.Node {
// https://github.com/estree/estree/blob/master/es2020.md#bigintliteral
// $FlowIgnore
const bigInt = typeof BigInt !== "undefined" ? BigInt(value) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a test case to show that the parsed result is different on node.js 6 and 8, which does not have BigInt support.

https://github.com/estree/estree/blob/master/es2020.md#bigintliteral

In environments that don't support BigInt values, value property will be null as the BigInt value can't be represented natively.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, are we not testing in older Node versions at the moment? I tested it locally with older Node versions and assumed this was already being done in CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right - sorry, I was thinking of the @babel/eslint-parser tests. Yes, we should add a test for this in @babel/parser.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: estree outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants