🌐 AI搜索 & 代理 主页
Skip to content

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.

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

Closed
gibson wants to merge 1 commit into from
Closed

feat(eslint-plugin): [no-misused-object-like] add rule #9369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ import noLossOfPrecision from './no-loss-of-precision';
import noMagicNumbers from './no-magic-numbers';
import noMeaninglessVoidOperator from './no-meaningless-void-operator';
import noMisusedNew from './no-misused-new';
import noMisusedObjectLikes from './no-misused-object-likes';
import noMisusedPromises from './no-misused-promises';
import noMixedEnums from './no-mixed-enums';
import noNamespace from './no-namespace';
Expand Down Expand Up @@ -210,6 +211,7 @@ export default {
'no-magic-numbers': noMagicNumbers,
'no-meaningless-void-operator': noMeaninglessVoidOperator,
'no-misused-new': noMisusedNew,
'no-misused-object-likes': noMisusedObjectLikes,
'no-misused-promises': noMisusedPromises,
'no-mixed-enums': noMixedEnums,
'no-namespace': noNamespace,
Expand Down
149 changes: 149 additions & 0 deletions packages/eslint-plugin/src/rules/no-misused-object-likes.ts
View file
Open in desktop
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;
},
];
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jun 27, 2024

Choose a reason for hiding this comment

The 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 Object.values?

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,
Copy link

@SimonSchick SimonSchick Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this is required?

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the services.getTypeAtLocation below necessitates it.

},
messages: {
objectKeysForMap: "Don't use `Object.keys()` for Map objects",
Copy link

@SimonSchick SimonSchick Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may be overly specific, granularity for SetLike and MapLike is likely sufficient since it would be weird to allow calling Object.values( on a Map but banning Object.keys

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
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] Using an as assertion on AST nodes is generally a sign of either:

  • 🍎 The node is known to be a more specific type than what you've told the type system
  • 🍌 The code isn't properly handling edge cases

The CallExpression[callee.object.name=Object][callee.property.name=keys][arguments.length=1] below makes me think it's 🍎. Which means you'd want to use a type for the node.

    type CallExpressionWithStuff = TSESTree.CallExpression & {
      callee: {
        object: {
          name: 'Object';
      // ...


if (argumentTypeName === 'Map') {
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] If a user defines their own Map class, this will falsely report:

class Map {
  value = 1;
}

const map = new Map();
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 {
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] Looking at built-in global objects in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects#keyed_collections, I think at least WeakMap and WeakSet should also be handled by this rule.

'CallExpression[callee.object.name=Object][callee.property.name=keys][arguments.length=1]':
Copy link

@SimonSchick SimonSchick Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing various other ones such as Object.assign/groupBy/hasOwn?, it might be better to move this check into the checkObjectMethodCall function and have a list of banned methods there.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

object.name=Object

[Bug] False report case:

const Object = { keys: (value) => [value] };

export const keys = Object.keys(new Map());

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,
};
},
});
174 changes: 174 additions & 0 deletions packages/eslint-plugin/tests/rules/no-misused-object-likes.test.ts
View file
Open in desktop
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Testing] Some missing test cases:

  • Intersections and unions: with Map, Set, and/or other types
  • Readonlies: ReadonlyMap, ReadonlySet

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' }],
},
],
});