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

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

Merged
merged 6 commits into from Apr 11, 2019
Merged

Conversation

webschik
Copy link
Contributor

@webschik webschik commented Mar 9, 2019

Fixes #309

@@ -283,5 +285,25 @@ var foo = pair + pair;
},
],
},
{
code: `var foo = 1n; foo + 1`,
Copy link
Contributor

@mohsen1 mohsen1 Mar 10, 2019

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?

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

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.

@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #344 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #344   +/-   ##
=======================================
  Coverage   97.14%   97.14%           
=======================================
  Files          74       74           
  Lines        2875     2875           
  Branches      473      473           
=======================================
  Hits         2793     2793           
  Misses         49       49           
  Partials       33       33
Impacted Files Coverage Δ
.../eslint-plugin/src/rules/restrict-plus-operands.ts 96.15% <100%> (ø) ⬆️

@bradzacher bradzacher merged commit eee6d49 into typescript-eslint:master Apr 11, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[restrict-plus-operands] rule doesn't support bigint
7 participants