Skip to content

[restrict-plus-operands] false positive on types like {} & string #2506

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
Closed
3 tasks done
papb opened this issue Sep 7, 2020 · 5 comments · Fixed by #2628
Closed
3 tasks done

[restrict-plus-operands] false positive on types like {} & string #2506

papb opened this issue Sep 7, 2020 · 5 comments · Fixed by #2628
Labels
enhancement New feature or request good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@papb
Copy link
Contributor

papb commented Sep 7, 2020

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/restrict-plus-operands": "error"
  }
}
declare const a: {} & string;
declare const b: string;
const x = a + b; // Operands of '+' operation must either be both strings or both numbers. Consider using a template literal. (@typescript-eslint/restrict-plus-operands)

Expected Result

No error

Actual Result

Operands of '+' operation must either be both strings or both numbers. Consider using a template literal. (@typescript-eslint/restrict-plus-operands)

Additional Info

The minimal example above might seem nonsense, using {} & string, but it's to be minimal. I encountered a situation in real code involving complex type inferences in which one of the operands apparently ended up being inferred as string | (Buffer & string) (which also seems nonsense, maybe I should open an issue at TS too... Regardless, WhateverType & string is a string, so the linter shouldn't complain).

Versions

package version (attempt 1, with XO) version (attempt 2, minimal, without XO)
@typescript-eslint/eslint-plugin 3.10.1 4.0.1
@typescript-eslint/parser 3.10.1 4.0.1
TypeScript 4.0.2 4.0.2
ESLint 7.7.0 7.8.1
node 12.14.1 12.14.1
@papb papb added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Sep 7, 2020
@bradzacher bradzacher added enhancement New feature or request good first issue Good for newcomers and removed triage Waiting for team members to take a look labels Sep 8, 2020
@bradzacher
Copy link
Member

Regardless, WhateverType & string is a string, so the linter shouldn't complain).

That's not strictly true.
boolean & string === never
number & string === never
symbol & string === never
object & string === never
never & string === never
any & string === any

The only cases that it holds true are:
unknown & string === string
string & string === string
'string literal' & string === 'string literal'

The cases of { someObjectType } & string can dubiously be considered strings.

This rule just hasn't had it added.
Happy to accept a PR.

There's some prior art in the restrict-template-expressions rule.

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/restrict-template-expressions.ts

@sunghyunjo
Copy link
Contributor

I will try this!

@papb
Copy link
Contributor Author

papb commented Sep 12, 2020

@sunghyunjo Awesome!!

@sunghyunjo
Copy link
Contributor

@bradzacher
I have one question. I think there is no rule that restricts the intersection type to string, so I think this rule needs to be added.
ex) create rules/restrict-intersection-types

Is this correct approach, or does it need to be fixed by modifying the existing restrict-plus-operands?

I am suddenly confused and ask a question. Sorry for the delay.

@bradzacher
Copy link
Member

A new rule isn't a good idea to solve this problem. The problem isn't that people are able to create these branded types, the problem is that this rule doesn't support them.

Supporting them should be pretty straightforward. You can use restrict-template-expressions for inspiration here.

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/restrict-template-expressions.ts

It has handling built in for branded types and does similar checks to this rule.

@sunghyunjo sunghyunjo mentioned this issue Oct 2, 2020
Merged
sunghyunjo added a commit to sunghyunjo/typescript-eslint that referenced this issue Oct 2, 2020
@sunghyunjo
Merge branch 'master' into issue/typescript-eslint#2506
7b5f7ee
sunghyunjo added a commit to sunghyunjo/typescript-eslint that referenced this issue Oct 3, 2020
@sunghyunjo
Merge branch 'issue/typescript-eslint#2506' of https://github.com/sun…
6bc4c39 
…ghyunjo/typescript-eslint into issue/typescript-eslint#2506
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
3 participants
@bradzacher @sunghyunjo @papb