Skip to content

[prefer-includes] Should not auto fix simple Regex #2349

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
banyudu opened this issue Aug 2, 2020 · 6 comments · Fixed by #2391
Closed

[prefer-includes] Should not auto fix simple Regex #2349

banyudu opened this issue Aug 2, 2020 · 6 comments · Fixed by #2391
Labels
bug Something isn't working good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@banyudu
Copy link

banyudu commented Aug 2, 2020

Repro

// original
const pattern = new RegExp("bar")
function f(a) {
  return pattern.test(a)
}
// after fix
function f(a) {
  return a.includes("bar")
}
// f({}) is false before fix, while throw exception after fix
{
  "rules": {
    "@typescript-eslint/prefer-includes": "error"
  }
}
const pattern = new RegExp("bar")
function f(a) {
  return pattern.test(a)
}
console.log(f({}))

Expected Result
false

Actual Result
TypeError: xxx.includes is not a function

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 3.7.1
@typescript-eslint/parser 3.7.1
TypeScript 3.9.7
ESLint 7.6.0
node 12.13.1
npm 6.14.5
@banyudu banyudu added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Aug 2, 2020
@bradzacher
Copy link
Member

This rule probably should just be restricted to string types.

@bradzacher bradzacher added bug Something isn't working good first issue Good for newcomers and removed triage Waiting for team members to take a look labels Aug 2, 2020
@sudalqueen
Copy link
Contributor

I'll try this!😊

@bradzacher
Copy link
Member

@sudalqueen - awesome thanks!

To be clear: the bug here is not that it's running on a non-string type.
The bug here is that it's fixing without making sure the fix is correct.

Before reporting an error for .test, the rule should first ensure that the type has a .includes method on it, so that we can make sure the fix is safe.

@sudalqueen
Copy link
Contributor

sudalqueen commented Aug 4, 2020 •
edited
Loading

@bradzacher Thank you for the kind explanation😊

So to fix the bug, What I'm have to do is make this rule report and fix a error when the type has the .includes method, right?
I'll do my best!

@banyudu
Copy link
Author

banyudu commented Aug 4, 2020

I think !!a.includes?.("bar") is good for regex replacement.

@sudalqueen sudalqueen mentioned this issue Aug 13, 2020
Merged
@sudalqueen
Copy link
Contributor

I made PR! 😊

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
3 participants
@banyudu @bradzacher @sudalqueen