Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat(eslint-plugin): [no-misused-object-like] add rule#9369
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| import type { Identifier, MemberExpression } from '@typescript-eslint/ast-spec'; | ||
| import type { TSESTree } from '@typescript-eslint/utils'; | ||
| import { createRule, getParserServices } from '../util'; | ||
| export type Options = [ | ||
| { | ||
| checkObjectKeysForMap?: boolean; | ||
| checkObjectValuesForMap?: boolean; | ||
| checkObjectEntriesForMap?: boolean; | ||
| checkObjectKeysForSet?: boolean; | ||
| checkObjectValuesForSet?: boolean; | ||
| checkObjectEntriesForSet?: boolean; | ||
| }, | ||
| ]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. [Question] Why allow such a granular set of options? Is there a known use case where someone would want to skip, say, only | ||
| export type MessageIds = | ||
| | 'objectKeysForMap' | ||
| | 'objectValuesForMap' | ||
| | 'objectEntriesForMap' | ||
| | 'objectKeysForSet' | ||
| | 'objectValuesForSet' | ||
| | 'objectEntriesForSet'; | ||
| export default createRule<Options, MessageIds>({ | ||
| name: 'no-misused-object-likes', | ||
| defaultOptions: [ | ||
| { | ||
| checkObjectKeysForMap: true, | ||
| checkObjectValuesForMap: true, | ||
| checkObjectEntriesForMap: true, | ||
| checkObjectKeysForSet: true, | ||
| checkObjectValuesForSet: true, | ||
| checkObjectEntriesForSet: true, | ||
| }, | ||
| ], | ||
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: | ||
| 'Enforce check `Object.values(...)`, `Object.keys(...)`, `Object.entries(...)` usage with Map/Set objects', | ||
| requiresTypeChecking: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Pretty sure this is required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yup, the | ||
| }, | ||
| messages: { | ||
| objectKeysForMap: "Don't use `Object.keys()` for Map objects", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I think this may be overly specific, granularity for | ||
| objectValuesForMap: "Don't use `Object.values()` for Map objects", | ||
| objectEntriesForMap: "Don't use `Object.entries()` for Map objects", | ||
| objectKeysForSet: "Don't use `Object.keys()` for Set", | ||
| objectValuesForSet: "Don't use `Object.values()` for Set", | ||
| objectEntriesForSet: "Don't use `Object.entries()` for Set", | ||
| }, | ||
| schema: [ | ||
| { | ||
| type: 'object', | ||
| additionalProperties: false, | ||
| properties: { | ||
| checkObjectKeysForMap: { | ||
| description: 'Check usage Object.keys for Map object', | ||
| type: 'boolean', | ||
| }, | ||
| checkObjectValuesForMap: { | ||
| description: 'Check usage Object.values for Map object', | ||
| type: 'boolean', | ||
| }, | ||
| checkObjectEntriesForMap: { | ||
| description: 'Check usage Object.entries for Map object', | ||
| type: 'boolean', | ||
| }, | ||
| checkObjectKeysForSet: { | ||
| description: 'Check usage Object.keys for Set object', | ||
| type: 'boolean', | ||
| }, | ||
| checkObjectValuesForSet: { | ||
| description: 'Check usage Object.values for Set object', | ||
| type: 'boolean', | ||
| }, | ||
| checkObjectEntriesForSet: { | ||
| description: 'Check usage Object.entries for Set object', | ||
| type: 'boolean', | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| create(context, [options]) { | ||
| const services = getParserServices(context); | ||
| function checkObjectMethodCall( | ||
| callExpression: TSESTree.CallExpression, | ||
| ): void { | ||
| const argument = callExpression.arguments[0]; | ||
| const type = services.getTypeAtLocation(argument); | ||
| const argumentTypeName = type.getSymbol()?.name; | ||
| const callee = callExpression.callee as MemberExpression; | ||
| const objectMethod = (callee.property as Identifier).name; | ||
Comment on lines +95 to +96 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. [Bug] Using an
The typeCallExpressionWithStuff=TSESTree.CallExpression&{callee:{object:{name:'Object';// ... | ||
| if (argumentTypeName === 'Map') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. [Bug] If a user defines their own classMap{value=1;}constmap=newMap();Object.values(map); | ||
| if (objectMethod === 'keys' && options.checkObjectKeysForMap) { | ||
| context.report({ | ||
| node: callExpression, | ||
| messageId: 'objectKeysForMap', | ||
| }); | ||
| } | ||
| if (objectMethod === 'values' && options.checkObjectValuesForMap) { | ||
| context.report({ | ||
| node: callExpression, | ||
| messageId: 'objectValuesForMap', | ||
| }); | ||
| } | ||
| if (objectMethod === 'entries' && options.checkObjectEntriesForMap) { | ||
| context.report({ | ||
| node: callExpression, | ||
| messageId: 'objectEntriesForMap', | ||
| }); | ||
| } | ||
| } | ||
| if (argumentTypeName === 'Set') { | ||
| if (objectMethod === 'keys' && options.checkObjectKeysForSet) { | ||
| context.report({ | ||
| node: callExpression, | ||
| messageId: 'objectKeysForSet', | ||
| }); | ||
| } | ||
| if (objectMethod === 'values' && options.checkObjectValuesForSet) { | ||
| context.report({ | ||
| node: callExpression, | ||
| messageId: 'objectValuesForSet', | ||
| }); | ||
| } | ||
| if (objectMethod === 'entries' && options.checkObjectEntriesForSet) { | ||
| context.report({ | ||
| node: callExpression, | ||
| messageId: 'objectEntriesForSet', | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| return { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. [Bug] Looking at built-in global objects inhttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects#keyed_collections, I think at least | ||
| 'CallExpression[callee.object.name=Object][callee.property.name=keys][arguments.length=1]': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This is missing various other ones such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more.
[Bug] False report case: constObject={keys:(value)=>[value]};exportconstkeys=Object.keys(newMap()); | ||
| checkObjectMethodCall, | ||
| 'CallExpression[callee.object.name=Object][callee.property.name=values][arguments.length=1]': | ||
| checkObjectMethodCall, | ||
| 'CallExpression[callee.object.name=Object][callee.property.name=entries][arguments.length=1]': | ||
| checkObjectMethodCall, | ||
| }; | ||
| }, | ||
| }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. [Testing] Some missing test cases:
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| import { RuleTester } from '@typescript-eslint/rule-tester'; | ||
| import rule from '../../src/rules/no-misused-object-likes'; | ||
| import { getFixturesRootDir } from '../RuleTester'; | ||
| const rootPath = getFixturesRootDir(); | ||
| const ruleTester = new RuleTester({ | ||
| parser: '@typescript-eslint/parser', | ||
| parserOptions: { | ||
| tsconfigRootDir: rootPath, | ||
| project: './tsconfig.json', | ||
| }, | ||
| }); | ||
| ruleTester.run('no-misused-object-likes', rule, { | ||
| valid: [ | ||
| { | ||
| code: ` | ||
| class ExMap extends Map {} | ||
| const map = new ExMap(); | ||
| Object.keys(map); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| class ExMap extends Map {} | ||
| const map = new ExMap(); | ||
| Object.values(map); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| class ExMap extends Map {} | ||
| const map = new ExMap(); | ||
| Object.entries(map); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| const test = {}; | ||
| Object.entries(test); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| const test = {}; | ||
| Object.keys(test); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| const test = {}; | ||
| Object.values(test); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| const test = []; | ||
| Object.keys(test); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| const test = []; | ||
| Object.values(test); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| const test = []; | ||
| Object.entries(test); | ||
| `, | ||
| }, | ||
| { | ||
| options: [{ checkObjectKeysForMap: false }], | ||
| code: ` | ||
| const map = new Map(); | ||
| const result = Object.keys(map); | ||
| `, | ||
| }, | ||
| { | ||
| options: [{ checkObjectEntriesForMap: false }], | ||
| code: ` | ||
| const map = new Map(); | ||
| const result = Object.entries(map); | ||
| `, | ||
| }, | ||
| { | ||
| options: [{ checkObjectValuesForMap: false }], | ||
| code: ` | ||
| const map = new Map(); | ||
| const result = Object.values(map); | ||
| `, | ||
| }, | ||
| { | ||
| options: [{ checkObjectKeysForSet: false }], | ||
| code: ` | ||
| const set = new Set(); | ||
| const result = Object.keys(set); | ||
| `, | ||
| }, | ||
| { | ||
| options: [{ checkObjectEntriesForSet: false }], | ||
| code: ` | ||
| const set = new Set(); | ||
| const result = Object.entries(set); | ||
| `, | ||
| }, | ||
| { | ||
| options: [{ checkObjectValuesForSet: false }], | ||
| code: ` | ||
| const set = new Set(); | ||
| const result = Object.values(set); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| const test = 123; | ||
| Object.keys(test); | ||
| `, | ||
| }, | ||
| { | ||
| code: ` | ||
| const test = new WeakMap(); | ||
| Object.keys(test); | ||
| `, | ||
| }, | ||
| ], | ||
| invalid: [ | ||
| { | ||
| code: ` | ||
| const map = new Map(); | ||
| const result = Object.keys(map); | ||
| `, | ||
| errors: [{ messageId: 'objectKeysForMap' }], | ||
| }, | ||
| { | ||
| code: ` | ||
| const map = new Map(); | ||
| const result = Object.entries(map); | ||
| `, | ||
| errors: [{ messageId: 'objectEntriesForMap' }], | ||
| }, | ||
| { | ||
| code: ` | ||
| const map = new Map(); | ||
| const result = Object.values(map); | ||
| `, | ||
| errors: [{ messageId: 'objectValuesForMap' }], | ||
| }, | ||
| { | ||
| code: ` | ||
| const set = new Set(); | ||
| const result = Object.keys(set); | ||
| `, | ||
| errors: [{ messageId: 'objectKeysForSet' }], | ||
| }, | ||
| { | ||
| code: ` | ||
| const set = new Set(); | ||
| const result = Object.entries(set); | ||
| `, | ||
| errors: [{ messageId: 'objectEntriesForSet' }], | ||
| }, | ||
| { | ||
| code: ` | ||
| const set = new Set(); | ||
| const result = Object.values(set); | ||
| `, | ||
| errors: [{ messageId: 'objectValuesForSet' }], | ||
| }, | ||
| ], | ||
| }); |