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

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

Merged
merged 14 commits into from Jan 17, 2020
Merged

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Dec 22, 2019

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 #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?

@mysticatea mysticatea added enhancement rule accepted blocked breaking do not merge labels Dec 22, 2019
@mysticatea mysticatea added this to Implemented, pending review in v7.0.0 Dec 22, 2019
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Dec 23, 2019

  • 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
Copy link
Member Author

@mysticatea mysticatea commented Dec 23, 2019

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
Copy link
Member

@mdjermanovic mdjermanovic commented Dec 23, 2019

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 kaicataldo removed the do not merge label Dec 23, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

LGTM, thanks!

@@ -65,7 +65,7 @@ module.exports = {
* @returns {boolean} true if the node is a number literal
*/
function isNumber(node) {
return typeof node.value === "number";
return typeof node.value === "number" || Boolean(node.bigint);
Copy link
Member

@kaicataldo kaicataldo Dec 24, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@mysticatea mysticatea Dec 24, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Contributor

@ljharb ljharb Dec 24, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@mysticatea mysticatea Dec 24, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Contributor

@ljharb ljharb Dec 24, 2019

Choose a reason for hiding this comment

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

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
Copy link
Member

@kaicataldo kaicataldo commented Dec 24, 2019

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
Copy link
Sponsor Contributor

@ljharb ljharb commented Dec 24, 2019

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
Copy link
Member Author

@mysticatea mysticatea commented Dec 24, 2019

@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
Copy link
Sponsor Contributor

@ljharb ljharb commented Dec 24, 2019

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

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Dec 24, 2019

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
Copy link
Member

@mdjermanovic mdjermanovic commented Dec 24, 2019

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.

mysticatea added 2 commits Dec 25, 2019
# Conflicts:
#	lib/rules/utils/ast-utils.js
#	tests/lib/rules/yoda.js
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Dec 25, 2019

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
Copy link
Member Author

@mysticatea mysticatea commented Dec 26, 2019

  • 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 mysticatea added the do not merge label Dec 26, 2019
@mysticatea mysticatea removed the do not merge label Dec 26, 2019
@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Dec 26, 2019

  • 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
Copy link
Member

@mdjermanovic mdjermanovic commented Dec 26, 2019

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?

Copy link
Member

@platinumazure platinumazure left a comment

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

conf/json-schema/README.md Outdated Show resolved Hide resolved
@ljharb
Copy link
Sponsor Contributor

@ljharb ljharb commented Dec 26, 2019

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

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Dec 27, 2019

  • 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.

lib/rules/no-magic-numbers.js Outdated Show resolved Hide resolved
@mysticatea mysticatea removed the blocked label Jan 7, 2020
@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Jan 7, 2020

  • 3695321 updated no-magic-numbers rule to allow strings of negative bigint values in ignores option.
  • I removed blocked label because #12700 has been merged.

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

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

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

@kaicataldo kaicataldo merged commit 7350589 into master Jan 17, 2020
11 checks passed
v7.0.0 automation moved this from Implemented, pending review to Done Jan 17, 2020
@kaicataldo kaicataldo deleted the issue11803 branch Jan 17, 2020
montmanu pushed a commit to montmanu/eslint that referenced this issue Mar 4, 2020
…slint#12701)

* update no-magic-numbers to recognize bigint

* update yoda to recognize bigint

* add a no-extend-native test

* update ci.yml temporary (this PR is blocked by eslint#12700)

* add astUtils.isNumericLiteral and use it in some rules

* update no-dupe-class-members

* update no-magic-number to support bigint in options

* update some rules to use getStaticPropertyName

* update quote-props

* revert no-useless-computed-key change

* revert "allowing {type: 'bigint'}" and update no-magic-number

* no-magic-number 'ignores' allows negative bigint
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 17, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age label Jul 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age breaking enhancement rule
Projects
No open projects
v7.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

5 participants