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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): support BigInt in restrict-plus-operands rule #344

Merged
merged 6 commits into from Apr 11, 2019
Merged

fix(eslint-plugin): support BigInt in restrict-plus-operands rule #344

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
719c905
fix(eslint-plugin): support BigInt in restrict-plus-operands rule (#309)
webschik Mar 9, 2019
6d58d97
Merge branch 'master' into issue-309
webschik Mar 11, 2019
fd0dad6
Merge branch 'master' into issue-309
bradzacher Apr 7, 2019
c76059b
Merge branch 'master' into issue-309
JamesHenry Apr 7, 2019
5b2f89a
Merge branch 'master' into issue-309
bradzacher Apr 11, 2019
0498060
Merge branch 'master' into issue-309
bradzacher Apr 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/eslint-plugin/src/rules/restrict-plus-operands.ts
View file
Open in desktop
Expand Up @@ -54,7 +54,11 @@ export default util.createRule({

const stringType = typeChecker.typeToString(type);

if (stringType === 'number' || stringType === 'string') {
if (
stringType === 'number' ||
stringType === 'string' ||
stringType === 'bigint'
) {
return stringType;
}
return 'invalid';
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/tests/fixtures/tsconfig.json
View file
Open in desktop
Expand Up @@ -4,6 +4,6 @@
"module": "commonjs",
"strict": true,
"esModuleInterop": true,
"lib": ["es2015", "es2017"]
"lib": ["es2015", "es2017", "esnext"]
}
}
22 changes: 22 additions & 0 deletions packages/eslint-plugin/tests/rules/restrict-plus-operands.test.ts
View file
Open in desktop
Expand Up @@ -23,6 +23,8 @@ ruleTester.run('restrict-plus-operands', rule, {
`var foo = parseInt("5.5", 10) + 10;`,
`var foo = parseFloat("5.5", 10) + 10;`,
`var foo = 1n + 1n;`,
`var foo = BigInt(1) + 1n`,
`var foo = 1n; foo + 2n`,
`
function test () : number { return 2; }
var foo = test("5.5", 10) + 10;
Expand Down Expand Up @@ -283,5 +285,25 @@ var foo = pair + pair;
},
],
},
{
code: `var foo = 1n; foo + 1`,
Copy link
Contributor

@mohsen1 mohsen1 Mar 10, 2019 •
edited

Choose a reason for hiding this comment

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

This is already an error thrown by TypeScript. Why you need a lint rule for it?

Operator '+' cannot be applied to types 'bigint' and '1'.

https://www.typescriptlang.org/play/index.html#src=var%20foo%20%3D%201n%3B%20foo%20%2B%201

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mohsen1 , good point. Maybe it's really an overhead and this rule makes sense only for strings and numbers.

I've implemented this to fix issue #309, but you're right that TypeScript does the same check already.

@novemberborn , @j-f1 , what do you think?

Copy link

Choose a reason for hiding this comment

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

Ah yes, it would do that. However the rule then needs to ensure either operand is actually a string or number before complaining (or any / unknown). The problem in #309 was that it complained about bigint even though bigints can be summed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I'd like to propose the next behaviour for this rule:

(any or unknown) + (string or number or bigint) - rule triggers an error, as it does now
all other cases - let TS decide what error to show

Copy link

Choose a reason for hiding this comment

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

That seems reasonable. You may want to pull that suggestion into its own issue for wider discussion, if you haven't already.

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a table to show the cases this rule should cover in my opinion:

+ number string BigInt any unknown
number rpo TS nua TS
string rpo rpo nua TS
BigInt TS rpo nua TS
any nua nua nua nua TS
unknown TS TS TS TS TS

Legend:

  • rpo: The behavior is invalid and should be caught by restrict-plus-operands.
  • TS: The behavior is invalid and is already caught by Typescript.
  • nua: The behavior coerces any into a stricter type and should thus be caught by no-unsafe-any.
  • An empty cell means valid behavior.

errors: [
{
messageId: 'notBigInts',
line: 1,
column: 15,
},
],
},
{
code: `var foo = 1; foo + 1n`,
errors: [
{
messageId: 'notBigInts',
line: 1,
column: 14,
},
],
},
],
});