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

Conversation

Projects
None yet
7 participants
@webschik
Copy link
Contributor

webschik commented Mar 9, 2019

Fixes #309

@j-f1

j-f1 approved these changes Mar 10, 2019

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

This comment has been minimized.

Copy link
@mohsen1

mohsen1 Mar 10, 2019

Contributor

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

This comment has been minimized.

Copy link
@webschik

webschik Mar 11, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@novemberborn

novemberborn Mar 11, 2019

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.

This comment has been minimized.

Copy link
@webschik

webschik Mar 11, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@novemberborn

novemberborn Mar 12, 2019

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

This comment has been minimized.

Copy link
@ThomasdenH

ThomasdenH Mar 12, 2019

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.

webschik and others added some commits Mar 11, 2019

@codecov

This comment has been minimized.

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 added some commits Apr 11, 2019

@bradzacher bradzacher merged commit eee6d49 into typescript-eslint:master Apr 11, 2019

3 checks passed

Semantic Pull Request ready to be squashed
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 97.14% (+0%) compared to ba0d524
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.