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

Additional false positives with no-array-prototype-extensions rule #1537

Closed
gilest opened this issue Jul 22, 2022 · 6 comments · Fixed by #1538, #1539, #1543, #1544 or #1546
Closed

Additional false positives with no-array-prototype-extensions rule #1537

gilest opened this issue Jul 22, 2022 · 6 comments · Fixed by #1538, #1539, #1543, #1544 or #1546
Labels

Comments

@gilest
Copy link
Contributor

gilest commented Jul 22, 2022

Further to #1534 there are additional false positives when this rule is run against ember-file-upload.

  • .clear() on a Set (probably on a Map, too)
  • RSVP.reject()

If I have time tomorrow I'll open a PR to address.

@bmish bmish added the bug label Jul 22, 2022
@bmish
Copy link
Member

bmish commented Jul 22, 2022

Thanks a lot for this report! Ignoring RSVP should be easy using the same technique as the last fix:

const KNOWN_NON_ARRAY_FUNCTION_CALLS = new Set([

For Map / Set / WeakMap / WeakSet / or the tracked-built-ins versions of those, we'll need to come up with some heuristics to detect when the variables are those data types. Some of my ideas:

  • Ignore variables with "map" or "set" in the name
  • Ignore variables where we can statically determine their value or that at least one of their initializations was to a map/set

@bmish
Copy link
Member

bmish commented Jul 22, 2022

I began a fix with one of the heuristics in #1538.

@gilest
Copy link
Contributor Author

gilest commented Jul 23, 2022

Attempted to cover the remaining case in #1539

Would appreciate your review @bmish

@bmish bmish reopened this Jul 23, 2022
@bmish bmish reopened this Jul 23, 2022
@bmish
Copy link
Member

bmish commented Jul 23, 2022

I'm reopening this because there are still some improvements needed to cover the original scenario:

  1. Class property definitions (eslint-utils' findVariable function doesn't handle class properties?)
class MyClass {
  foo = new Set();
  myFunc() { this.foo.clear() }
}
  1. TypeScript with private identifiers (TypeScript PrivateIdentifier node type not handled?)
class MyClass {
  #foo: Set<UploadFile> = new Set();
  myFunc() {
    this.#foo.clear();
  }
}

I'll try to look into how to improve upon my solution from #1538 in the coming week.

@bmish
Copy link
Member

bmish commented Jul 26, 2022

@gilest all the necessary fixes should now be available in v11.0.3. Let me know if you have any other issues. Thanks for testing and reporting!

@gilest
Copy link
Contributor Author

gilest commented Jul 26, 2022

Awesome work @bmish 🙌🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment