Breaking: some rules recognize bigint literals (fixes #11803)

[mysticatea]

Blocked by #12700.

What is the purpose of this pull request? (put an "X" next to item)

[X] Changes existing rules (fixes #11803)

What changes did you make? (Give an overview)

This PR adds BigInt support to some rules with the manner that increases warnings.

From the list of #11803:

• no-compare-neg-zero rule: I didn't include the rule in this PR because of this question. Indeed, reporting BigInt literals is different from the purpose of this rule.
• no-extend-native rule: As the above comment mentioned, the rule has been updated via a minor update of globals package. I just added a test case. I'm wondering if we should update this rule to not use globals package.
• no-magic-number rule: This rule got recognizing bigint literals.
• yoda rule: This rule got recognizing ranges even if there are negative bigint literals. This change requires Node.js 10.4.0 or later. Therefore, this PR is blocked by Breaking: drop Node.js 8 support (refs eslint/rfcs#44) #12700.

Is there anything you'd like reviewers to focus on?

• Should we update no-extend-native to prevent breaking changes via the minor updates of globals package?
• Are there other rules to need updates?

[mdjermanovic]

• no-compare-neg-zero rule: I didn't include the rule in this PR because of this question. Indeed, reporting BigInt literals is different from the purpose of this rule.

Does a separate new rule to disallow -0n anywhere in the code make sense? If it looks useful, I could prepare a proposal.

• Should we update no-extend-native to prevent breaking changes via the minor updates of globals package?

Can this happen with some other rules as well? For example, it looks like if some new global gets added to browser environment in a minor globals package version, it could be a new error for rules such as no-shadow, no-redeclare etc. A solution could be to pin globals version. On the other hand, it might be useful to warn the user when they're rewriting some newly added built-in/platform global, although it's breaking the build.

• Are there other rules to need updates?

Just some thoughts, but probably neither of the following:

• no-implicit-coercion could warn on 1n * foo. But this doesn't 'convert value types' as noted in the documentation. It throws if foo or foo.valueOf isn't a bigint value, so this might be more suitable for a separate rule as a possible error.
• no-new-wrappers could warn on new BigInt. But this produces a runtime error, not a 'wrapper'.

Also some rules if var foo = { 1n: bar} is a valid syntax, though I believe it's a bug that Acorn allows this.

[mysticatea]

Does a separate new rule to disallow -0n anywhere in the code make sense? If it looks useful, I could prepare a proposal.

I'm not sure if disallowing -0n is worthful or not. We can say that such a rule is similar to no-unused-* or no-useless-* rules because that minus sign does nothing. But we can say that it's too rare for core. That said, I don't oppose it actively.

Can this happen with some other rules as well? For example, it looks like if some new global gets added to browser environment in a minor globals package version, it could be a new error for rules such as no-shadow, no-redeclare etc. A solution could be to pin globals version. On the other hand, it might be useful to warn the user when they're rewriting some newly added built-in/platform global, although it's breaking the build.

Yes. Pinning the globals package sounds like a good idea.

In fact, I like RFC9's solution; deprecating env feature and users use the globals package directly. Because if users depend on the package directly, they can update/pin it easily in their own decision. (However, it makes user-land too complex if users require other packages than eslint to configure eslint for the standard JavaScript. It's not good.)

Just some thoughts, ...

Thank you!

• no-implicit-coercion could warn on 1n * foo. But this doesn't 'convert value types' as noted in the documentation. It throws if foo or foo.valueOf isn't a bigint value, so this might be more suitable for a separate rule as a possible error.

Right. It's not implicit type conversion.

• no-new-wrappers could warn on new BigInt. But this produces a runtime error, not a 'wrapper'.

Right. Probably, no-new-bigint or something like may be nice because we have no-new-symbol already. But, actually Symbol and BigInt are wrappers (e.g. (1n).constructor === BigInt is true), so also it may be good if we should merge no-new-symbol to no-new-wrappers.

Also some rules if var foo = { 1n: bar} is a valid syntax, though I believe it's a bug that Acorn allows this.

Wait. Indeed, all of Node.js, Chrome, and Firefox throw syntax error. But I'm not sure if it's a defined behavior in the spec. PropertyName > LiteralPropertyName > NumericLiteral, then it contains BigInt literals. And I couldn't find any early errors nor runtime errors for BigInt literals.

[mdjermanovic]

Wait. Indeed, all of Node.js, Chrome, and Firefox throw syntax error. But I'm not sure if it's a defined behavior in the spec. PropertyName > LiteralPropertyName > NumericLiteral, then it contains BigInt literals. And I couldn't find any early errors nor runtime errors for BigInt literals.

I think you're right, there is nothing to prevent a BigInt literal from being a property name in an object literal. What's an appropriate place to report an issue/question about the specification, such as this one?

[kaicataldo]

LGTM, thanks!

[kaicataldo]

Not a blocker: For my own understanding, is there a reason we're checking the presence of this property versus using typeof like we are below? I wonder if it would be nice to add astUtils.isNumber() and use it everywhere?

[mysticatea]

I have noticed after I wrote this, it's no problem if it's typeof node.value === "number" || typeof node.value === "bigint".

The reason for checking node.bigint was that node.value will be null if the runtime doesn't support bigint natively. However, Node.js 10.4.0 supports bigint and ESLint 7 requires 10.11.0, so it's not a problem.

astUtils.isNumeric() or someting like is a good idea.

[ljharb]

Why does bigint support matter tho? typeof x === 'bigint' will just be false in an engine that doesn't support bigint.

[mysticatea]

When a source code 1n is given, ESLint expects the AST to be { type: "Literal", value: 1n }. However, if the runtime doesn't support BigInt natively, the AST will be { type: "Literal", value: null } then typeof node.value becomes a wrong value.

[ljharb]

Ah, that seems like a pretty big flaw in the thing that generates the AST - it should probably supply enough information to know that the value isn't a literal null.

[kaicataldo]

I think you're right, there is nothing to prevent a BigInt literal from being a property name in an object literal. What's an appropriate place to report an issue/question about the specification, such as this one?

Maybe @ljharb could help us or point us to where we might be able to find this out?

[ljharb]

It would be treated as a string in the literal case, i believe - only strings can be non computed property keys.

Why would the yoda rule changes require node 10.4? Syntactic bigint support should not be necessary for anything.

[mysticatea]

@ljharb

It would be treated as a string in the literal case, i believe - only strings can be non computed property keys.

As the language spec, if a property name is a NumericLiteral, the runtime uses ! ToString(nbr) (nbr is the NumericValue of the NumericLiteral). (See this section)

In fact, ({ 1e+3: 1 }) makes { "1000": 1 } object in existing browsers.

Following the spec, it should allow { 1n: 1n } as well.

Why would the yoda rule changes require node 10.4? Syntactic bigint support should not be necessary for anything.

Because the exceptRange option of yoda rule needs to compare the literal values. Because ESLint 7 requires Node.js 10.11.0 at least, I don't think it's a problem.

[ljharb]

Indeed, it would toString them, but since the toString of 123n is '123', it seems pretty trivial to handle.

[mdjermanovic]

If ({ 1n: foo }) is valid code, no-useless-computed-key looks like a rule that should be updated.

A problem is that the rule would start to require (and produce in autofix) code that seems to be valid by the spec but doesn't work in real engines.

[mdjermanovic]

Re no-magic-numbers, I'm not sure if it is a big issue, but it seems that the ignore option can't accept bigints.

[mdjermanovic]

In the quote-props rule, numbers: true option might apply to bigint literals as well?

/*eslint quote-props: ["error", "as-needed", { "numbers": true }]*/

var foo = {
    123n: bar // no error at the moment
}

[mysticatea]

• 339f932 updated no-useless-computed-key rule to recognize bigint literals. The autofix of the rule fixes bigint literals of computed properties to string literal property names for now because well-known browsers don't accord to the language spec and throwing syntax errors. (e.g., { [1n]: 1 } → { "1": 1 })
• 4bfdcb1 updated no-magic-number to recognize bigint literals. And the ignores option of the rule now accepts bigint literals from .eslintrc.js. Is this direction good?
• 7ae3a85 just fixed some rules to use astUtils.getStaticPropertyName() instead of their own similar logic.

I'm thinking about quote-props.

I'm leaning toward that quote-props and no-useless-computed-key (and other rules that handle property names) should not touch bigint literals for now because many runtimes throw syntax error as against the language spec... 🤔

[mysticatea]

• 0921b79 updated quote-props for adding quotes. Adding quotes for bigint literal property names is just safe. There is not the opposite direction because the string representation of BigInt doesn't contain suffix n.

[mdjermanovic]

4bfdcb1 updated no-magic-number to recognize bigint literals. And the ignores option of the rule now accepts bigint literals from .eslintrc.js. Is this direction good?

Looks good except for JSON config files. I'm also not sure about yml and config comments, e.g., it throws a config error when I try this test case on this branch:

{
    code: "/*eslint no-magic-numbers: ['error', { 'ignore': [100n] }]*/ f(100n)",
    parserOptions: { ecmaVersion: 2020 }
}

Perhaps we could just say that the feature to ignore some bigints is supported in js configs only? Or maybe provide some alternative.

7ae3a85 just fixed some rules to use astUtils.getStaticPropertyName() instead of their own similar logic.

👍 major version looks like a good time to do this.

I'm leaning toward that quote-props and no-useless-computed-key (and other rules that handle property names) should not touch bigint literals for now because many runtimes throw syntax error as against the language spec...

I agree that no-useless-computed-key should not touch bigint literals for now. This rule is supposed to just remove [] brackets, not to add quotes on top.

As for quote-props, adding quotes is okay. Removing quotes and adding n to a long sequence of digits might be unexpected behavior anyway, regardless of the fact that it wouldn't presently work in runtime?

[platinumazure]

Just leaving one question about adopting the JSON metaschema, otherwise LGTM. Thanks!

[ljharb]

For json configs, could the ignore list accept string versions of bigint literals? Like ’0n’.

[mysticatea]

• 97f38f1 updated no-useless-computed-key rule to not touch bigint literals.
• fbcddfc reverted the meta JSON Schema change because I think we need an RFC for that. Instead, updated no-magic-number rule to accept "3n"-like strings in ignores option.

Removing quotes and adding n to a long sequence of digits might be unexpected behavior anyway, regardless of the fact that it wouldn't presently work in runtime?

I think so. It's surprising a bit to me.

[mysticatea]

• 3695321 updated no-magic-numbers rule to allow strings of negative bigint values in ignores option.
• I removed blocked label because Breaking: drop Node.js 8 support (refs eslint/rfcs#44) #12700 has been merged.

---

A side note: I reported about BigInt literals as LiteralPropertyName: V8, Firefox, and test262.

[mdjermanovic]

LGTM, thanks!

Unrelated bug, no-magic-number allows 100 .toString() as array index.