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

gibson · 2024-06-17T08:12:39Z

PR Checklist

Addresses an existing open issue: fixes Rule proposal: Prevent Object.values(...) usage with Map #6807
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This MR will close the problem with usage Object.keys(), Object.values() and Object.entries() for object like classes Map and Set. On this classes that methods always returns empty array instead of any meaningful data and doesn't cause and warnings or errors

…es(), Ob

typescript-eslint · 2024-06-17T08:12:43Z

Thanks for the PR, @gibson!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2024-06-17T08:12:55Z

❌ Deploy Preview for typescript-eslint failed.

Name	Link
🔨 Latest commit	`716096d`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/666feffa052e5400085851b4

nx-cloud · 2024-06-17T08:14:00Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 716096d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run repo-tools:postinstall-script

Sent with 💌 from NxCloud.

SimonSchick · 2024-06-21T17:47:16Z

This is missing support for ReadonlyMap ReadonlySet, WeakMap and WeakSet (and possibly others) and will likely not work with inheritance?

It may be worth-while to check if the subject of the check has any .get(arg) and .set(arg1, arg2) methods to catch issues with Headers and friends as well, this may be slow however.

> new Headers({a:1})
Headers { a: '1' }
> Object.values(new Headers({a:1}))
[]
> Object.entries(new Headers({a:1}))
[]

Overall this is a good PR, it might also be good to integrate #5677 into this and generalize this rule as no-misused-map-like-usage?

github-actions · 2024-06-21T17:47:39Z

Uh oh! @SimonSchick, the image you shared is missing helpful alt text. Check #9369 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

SimonSchick

This would be extremely useful when migrating from object dictionary use to map/set use!

SimonSchick · 2024-06-21T17:50:10Z

packages/eslint-plugin/src/rules/no-misused-object-likes.ts

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


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

SimonSchick · 2024-06-21T17:50:39Z

packages/eslint-plugin/src/rules/no-misused-object-likes.ts

+    docs: {
+      description:
+        'Enforce check `Object.values(...)`, `Object.keys(...)`, `Object.entries(...)` usage with Map/Set objects',
+      requiresTypeChecking: false,


Pretty sure this is required?

Yup, the services.getTypeAtLocation below necessitates it.

SimonSchick · 2024-06-21T17:55:50Z

packages/eslint-plugin/src/rules/no-misused-object-likes.ts

+    }
+
+    return {
+      'CallExpression[callee.object.name=Object][callee.property.name=keys][arguments.length=1]':


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.

JoshuaKGoldberg · 2024-06-27T13:00:37Z

packages/eslint-plugin/src/rules/no-misused-object-likes.ts

+    checkObjectValuesForSet?: boolean;
+    checkObjectEntriesForSet?: boolean;
+  },
+];


[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?

JoshuaKGoldberg

A good start! There are some functional gaps that'll need to be filled in around more object support and resiliency to user shenanigans. Excited to see this - thanks for getting it started! 🙌

JoshuaKGoldberg · 2024-06-27T13:01:44Z

packages/eslint-plugin/src/rules/no-misused-object-likes.ts

+      const callee = callExpression.callee as MemberExpression;
+      const objectMethod = (callee.property as Identifier).name;
+
+      if (argumentTypeName === 'Map') {


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

class Map { value = 1; } const map = new Map(); Object.values(map);

JoshuaKGoldberg · 2024-06-27T13:07:36Z

packages/eslint-plugin/src/rules/no-misused-object-likes.ts

+      const callee = callExpression.callee as MemberExpression;
+      const objectMethod = (callee.property as Identifier).name;


[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'; // ...

JoshuaKGoldberg · 2024-06-27T13:08:33Z

packages/eslint-plugin/src/rules/no-misused-object-likes.ts

+    }
+
+    return {
+      'CallExpression[callee.object.name=Object][callee.property.name=keys][arguments.length=1]':


object.name=Object

[Bug] False report case:

const Object = { keys: (value) => [value] }; export const keys = Object.keys(new Map());

JoshuaKGoldberg · 2024-06-27T13:09:56Z

packages/eslint-plugin/tests/rules/no-misused-object-likes.test.ts

[Testing] Some missing test cases:

Intersections and unions: with Map, Set, and/or other types

Readonlies: ReadonlyMap, ReadonlySet

JoshuaKGoldberg · 2024-06-27T13:14:15Z

packages/eslint-plugin/src/rules/no-misused-object-likes.ts

+      }
+    }
+
+    return {


[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.

SimonSchick · 2024-07-10T04:43:39Z

@gibson are you interested in continuing work on this? Otherwise I may be inclined to take a stab at this myself.

gibson · 2024-07-10T07:14:54Z

@SimonSchick will continue in closes time

JoshuaKGoldberg · 2024-08-12T16:27:44Z

👋 @gibson just checking in, do you still plan on working on this?

JoshuaKGoldberg · 2024-08-25T17:07:03Z

Closing this PR as it's been stale for ~6 weeks without activity. Feel free to reopen @gibson if you have time - but no worries if not! If anybody wants to drive it forward, please do post your own PR - and if you use this as a start, consider adding Co-authored-by: @gibson at the end of your PR description. Thanks! 😊

SimonSchick · 2024-08-25T20:49:04Z

I might pick this up, I ran into some scenarious/migrations where this rule would've been incredibly valuable.

typescript-eslint#6807 Prevent misusage of Object.keys(), Object.valu…

716096d

…es(), Ob

SimonSchick suggested changes Jun 21, 2024

View reviewed changes

JoshuaKGoldberg reviewed Jun 27, 2024

View reviewed changes

JoshuaKGoldberg requested changes Jun 27, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jun 27, 2024

JoshuaKGoldberg added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Aug 12, 2024

bradzacher changed the title ~~#6807 Prevent misusage of Object.keys(), Object.values(), Ob~~ feat(eslint-plugin): [no-misused-object-like] add rule Aug 25, 2024

JoshuaKGoldberg closed this Aug 25, 2024

github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gibson commented Jun 17, 2024

typescript-eslint bot commented Jun 17, 2024

netlify bot commented Jun 17, 2024 •

edited

Loading

nx-cloud bot commented Jun 17, 2024 •

edited

Loading

SimonSchick commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024

SimonSchick left a comment

SimonSchick Jun 21, 2024

SimonSchick Jun 21, 2024

JoshuaKGoldberg Jun 27, 2024

SimonSchick Jun 21, 2024

JoshuaKGoldberg Jun 27, 2024

JoshuaKGoldberg left a comment

JoshuaKGoldberg Jun 27, 2024

JoshuaKGoldberg Jun 27, 2024

JoshuaKGoldberg Jun 27, 2024

JoshuaKGoldberg Jun 27, 2024

JoshuaKGoldberg Jun 27, 2024

SimonSchick commented Jul 10, 2024

gibson commented Jul 10, 2024

JoshuaKGoldberg commented Aug 12, 2024

JoshuaKGoldberg commented Aug 25, 2024

SimonSchick commented Aug 25, 2024

		const callee = callExpression.callee as MemberExpression;
		const objectMethod = (callee.property as Identifier).name;

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

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

Conversation

gibson commented Jun 17, 2024

PR Checklist

Overview

typescript-eslint bot commented Jun 17, 2024

netlify bot commented Jun 17, 2024 • edited Loading

❌ Deploy Preview for typescript-eslint failed.

nx-cloud bot commented Jun 17, 2024 • edited Loading

☁️ Nx Cloud Report

SimonSchick commented Jun 21, 2024 • edited Loading

github-actions bot commented Jun 21, 2024

SimonSchick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonSchick commented Jul 10, 2024

gibson commented Jul 10, 2024

JoshuaKGoldberg commented Aug 12, 2024

JoshuaKGoldberg commented Aug 25, 2024

SimonSchick commented Aug 25, 2024

netlify bot commented Jun 17, 2024 •

edited

Loading

nx-cloud bot commented Jun 17, 2024 •

edited

Loading

SimonSchick commented Jun 21, 2024 •

edited

Loading