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

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

716096d

…es(), Ob

Copy link

Contributor

typescript-eslintbot commentedJun 17, 2024

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.

Copy link

netlifybot commentedJun 17, 2024•
edited
Loading

❌ Deploy Preview fortypescript-eslint failed.

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

Copy link

nx-cloudbot commentedJun 17, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit716096d. 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 💌 fromNxCloud.

Copy link

SimonSchick commentedJun 21, 2024•
edited
Loading

This is missing support forReadonlyMapReadonlySet,WeakMap andWeakSet (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 withHeaders 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 asno-misused-map-like-usage?

Copy link

github-actionsbot commentedJun 21, 2024

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 atBasic writing and formatting syntax: images on GitHub Docs.

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

SimonSchick suggested changes

Jun 21, 2024

View reviewed changes

Copy link

SimonSchick left a comment

Choose a reason for hiding this comment

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

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

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

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

Copy link

SimonSchickJun 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 forSetLike andMapLike is likely sufficient since it would be weird to allow callingObject.values( on a Map but banningObject.keys

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,

Copy link

SimonSchickJun 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

JoshuaKGoldbergJun 27, 2024

Choose a reason for hiding this comment

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

Yup, theservices.getTypeAtLocation below necessitates it.

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

		}

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

Copy link

SimonSchickJun 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 asObject.assign/groupBy/hasOwn?, it might be better to move this check into thecheckObjectMethodCall function and have a list of banned methods there.

JoshuaKGoldberg reviewed

Jun 27, 2024

View reviewed changes

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

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

Copy link

Member

JoshuaKGoldbergJun 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, onlyObject.values?

JoshuaKGoldberg requested changes

Jun 27, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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! 🙌

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') {

Copy link

Member

JoshuaKGoldbergJun 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 ownMap class, this will falsely report:

classMap{value=1;}constmap=newMap();Object.values(map);

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

Comment on lines +95 to +96

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

Copy link

Member

JoshuaKGoldbergJun 27, 2024

Choose a reason for hiding this comment

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

[Bug] Using anas 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

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

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

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

		}

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

Copy link

Member

JoshuaKGoldbergJun 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:

constObject={keys:(value)=>[value]};exportconstkeys=Object.keys(newMap());

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

Copy link

Member

JoshuaKGoldbergJun 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: withMap,Set, and/or other types
Readonlies:ReadonlyMap,ReadonlySet

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

		}
		}

		return {

Copy link

Member

JoshuaKGoldbergJun 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 inhttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects#keyed_collections, I think at leastWeakMap andWeakSet should also be handled by this rule.

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Jun 27, 2024

Copy link

SimonSchick commentedJul 10, 2024

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

Copy link

Author

gibson commentedJul 10, 2024

@SimonSchick will continue in closes time

Copy link

Member

JoshuaKGoldberg commentedAug 12, 2024

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

JoshuaKGoldberg added the stalePRs 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

Copy link

Member

JoshuaKGoldberg commentedAug 25, 2024

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 addingCo-authored-by: @gibson at the end of your PR description. Thanks! 😊

JoshuaKGoldberg closed this

Aug 25, 2024

Copy link

SimonSchick commentedAug 25, 2024

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

github-actionsbot locked asresolvedand limited conversation to collaborators

Sep 2, 2024

Labels

awaiting response

Issues waiting for a reply from the OP or another party

stale

PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period

Movatterモバイル変換

Uh oh!

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

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

Conversation

gibson commentedJun 17, 2024

PR Checklist

Overview

Uh oh!

typescript-eslintbot commentedJun 17, 2024

Uh oh!

netlifybot commentedJun 17, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

❌ Deploy Preview fortypescript-eslint failed.

Uh oh!

nx-cloudbot commentedJun 17, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

SimonSchick commentedJun 21, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

github-actionsbot commentedJun 21, 2024

Uh oh!

SimonSchick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonSchick commentedJul 10, 2024

Uh oh!

gibson commentedJul 10, 2024

Uh oh!

JoshuaKGoldberg commentedAug 12, 2024

Uh oh!

JoshuaKGoldberg commentedAug 25, 2024

Uh oh!

SimonSchick commentedAug 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlifybot commentedJun 17, 2024•
edited
Loading

nx-cloudbot commentedJun 17, 2024•
edited
Loading

SimonSchick commentedJun 21, 2024•
edited
Loading