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.

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

Fix false positive with Set/Map-initialized *public* class properties in no-array-prototype-extensions rule #1543

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Fix false positive with Set/Map-initialized *public* class properties in no-array-prototype-extensions rule #1543

merged 1 commit into from
Jul 26, 2022
+248 −34

Conversation

bmish
Copy link
Member

@bmish bmish commented Jul 25, 2022 •
edited
Loading

Partially fixes #1537.

CC: @gilest @smilland

@bmish bmish added the bug label Jul 25, 2022
@bmish bmish force-pushed the no-array-prototype-extensions-class-props branch 5 times, most recently from 9eb696a to 224b18a Compare July 25, 2022 22:42
@gilest
Copy link
Contributor

gilest commented Jul 26, 2022 •
edited
Loading

Thanks for staging this @bmish 🙇🏻‍♂️ I'm not familiar with the implementation but the test cases look thorough!

I tried running it for you by installing it in ember-file-upload here.

Unfortunately yarn workspace ember-file-upload lint:js causes ESLint to throw an exception when it encounters this line in repo while building a ClassDeclaration.

export default class FileDropzoneComponent extends Component<FileDropzoneArgs> {

Perhaps we should add spec examples covering classes with TypeScript syntax? Or have I run ESLint wrong somehow?

TypeError: Cannot read properties of null (reading 'type')
Occurred while linting /Users/Giles/code/ember-file-upload/ember-file-upload/src/components/file-dropzone.ts:45
    at /Users/Giles/code/ember-file-upload/node_modules/eslint-plugin-ember/lib/rules/no-array-prototype-extensions.js:246:25
    at Array.filter (<anonymous>)
    at ClassDeclaration (/Users/Giles/code/ember-file-upload/node_modules/eslint-plugin-ember/lib/rules/no-array-prototype-extensions.js:242:14)
    at /Users/Giles/code/ember-file-upload/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/Giles/code/ember-file-upload/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/Users/Giles/code/ember-file-upload/node_modules/eslint/lib/linter/node-event-generator.js:293:26)
    at NodeEventGenerator.applySelectors (/Users/Giles/code/ember-file-upload/node_modules/eslint/lib/linter/node-event-generator.js:322:22)
    at NodeEventGenerator.enterNode (/Users/Giles/code/ember-file-upload/node_modules/eslint/lib/linter/node-event-generator.js:336:14)
    at CodePathAnalyzer.enterNode (/Users/Giles/code/ember-file-upload/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)

Let me know if I can help with more reproductions.

@bmish bmish force-pushed the no-array-prototype-extensions-class-props branch from 224b18a to 6cf5850 Compare July 26, 2022 14:46
@bmish bmish force-pushed the no-array-prototype-extensions-class-props branch from 6cf5850 to 71f5cc5 Compare July 26, 2022 14:51
@bmish
Copy link
Member Author

bmish commented Jul 26, 2022

@gilest thanks so much for testing this! I fixed the bug with the value-less property definition that was causing a crash for you. I also added some basic TypeScript test cases.

After this PR, there should only be one remaining issue, which is handling private identifiers as I mentioned in #1537 (comment). I'll work on that next.

@bmish bmish merged commit ae70c0a into ember-cli:master Jul 26, 2022
@bmish bmish changed the title Fix false positive with Set/Map-initialized class properties in no-array-prototype-extensions rule Fix false positive with Set/Map-initialized public class properties in no-array-prototype-extensions rule Jul 26, 2022
@bmish bmish changed the title Fix false positive with Set/Map-initialized public class properties in no-array-prototype-extensions rule Fix false positive with Set/Map-initialized *public* class properties in no-array-prototype-extensions rule Jul 26, 2022
ef4 added a commit that referenced this pull request Aug 10, 2022
@ef4
Remove no-array-prototype-extensions from recommended
a888fbf 
The `no-array-prototype-extensions` rule is trying to do an impossible thing. It's going to produce a never-ending stream of false positives. You cannot statically tell which calls are actually relying on the array prototype extensions.

A correct implementation is probably possible, but only in typescript (where eslint rules can take advantage of type information), not in javascript.

I'm all in favor of pushing people away from using array prototype extensions, but that is going to require runtime implementation. It cannot be checked statically. A rule that is so often wrong will just encourage people not to trust it anyway. I don't think it should be in the recommended set.

Examples:

#1561
#1552
#1547
#1546
#1544
#1543
#1538
#1539
#1536
@ef4 ef4 mentioned this pull request Aug 10, 2022
Merged
bmish added a commit that referenced this pull request Aug 18, 2022
@ef4 @bmish
Remove no-array-prototype-extensions rule from recommended config (…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
e7ffcb0 
…#1562)

* Remove no-array-prototype-extensions from recommended

The `no-array-prototype-extensions` rule is trying to do an impossible thing. It's going to produce a never-ending stream of false positives. You cannot statically tell which calls are actually relying on the array prototype extensions.

A correct implementation is probably possible, but only in typescript (where eslint rules can take advantage of type information), not in javascript.

I'm all in favor of pushing people away from using array prototype extensions, but that is going to require runtime implementation. It cannot be checked statically. A rule that is so often wrong will just encourage people not to trust it anyway. I don't think it should be in the recommended set.

Examples:

#1561
#1552
#1547
#1546
#1544
#1543
#1538
#1539
#1536

* docs: mentioned recommend rule removal in changelog

* docs: explain why no-array-prototype-extensions is not in recommended config

Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Additional false positives with no-array-prototype-extensions rule
2 participants
@bmish @gilest