From 6de210cdff22013c47a67fd1accd93f99c8b03a3 Mon Sep 17 00:00:00 2001 From: fisker Cheung Date: Wed, 14 Apr 2021 09:35:46 +0800 Subject: [PATCH 1/2] Create rule --- docs/rules/proper-object-iterable-methods.md | 17 ++++++++ index.js | 1 + readme.md | 2 + rules/proper-object-iterable-methods.js | 43 ++++++++++++++++++++ test/proper-object-iterable-methods.mjs | 13 ++++++ 5 files changed, 76 insertions(+) create mode 100644 docs/rules/proper-object-iterable-methods.md create mode 100644 rules/proper-object-iterable-methods.js create mode 100644 test/proper-object-iterable-methods.mjs diff --git a/docs/rules/proper-object-iterable-methods.md b/docs/rules/proper-object-iterable-methods.md new file mode 100644 index 0000000000..c679d6be84 --- /dev/null +++ b/docs/rules/proper-object-iterable-methods.md @@ -0,0 +1,17 @@ +# Enforce proper methods when iterate through objects. + + + +This rule is partly fixable. + +## Fail + +```js +const foo = 'unicorn'; +``` + +## Pass + +```js +const foo = '🦄'; +``` diff --git a/index.js b/index.js index c873a76bd5..3c8dbc0bd1 100644 --- a/index.js +++ b/index.js @@ -115,6 +115,7 @@ module.exports = { 'unicorn/prefer-ternary': 'error', 'unicorn/prefer-type-error': 'error', 'unicorn/prevent-abbreviations': 'error', + 'unicorn/proper-object-iterable-methods': 'error', 'unicorn/string-content': 'off', 'unicorn/throw-new-error': 'error' } diff --git a/readme.md b/readme.md index c569eca7c1..8ba08601fe 100644 --- a/readme.md +++ b/readme.md @@ -105,6 +105,7 @@ Configure it in `package.json`. "unicorn/prefer-ternary": "off", "unicorn/prefer-type-error": "error", "unicorn/prevent-abbreviations": "error", + "unicorn/proper-object-iterable-methods": "error", "unicorn/string-content": "off", "unicorn/throw-new-error": "error" } @@ -195,6 +196,7 @@ Each rule has emojis denoting: | [prefer-ternary](docs/rules/prefer-ternary.md) | Prefer ternary expressions over simple `if-else` statements. | ✅ | 🔧 | | [prefer-type-error](docs/rules/prefer-type-error.md) | Enforce throwing `TypeError` in type checking conditions. | ✅ | 🔧 | | [prevent-abbreviations](docs/rules/prevent-abbreviations.md) | Prevent abbreviations. | ✅ | 🔧 | +| [proper-object-iterable-methods](docs/rules/proper-object-iterable-methods.md) | Enforce proper methods when iterate through objects. | ✅ | 🔧 | | [string-content](docs/rules/string-content.md) | Enforce better string content. | | 🔧 | | [throw-new-error](docs/rules/throw-new-error.md) | Require `new` when throwing an error. | ✅ | 🔧 | diff --git a/rules/proper-object-iterable-methods.js b/rules/proper-object-iterable-methods.js new file mode 100644 index 0000000000..61190daba5 --- /dev/null +++ b/rules/proper-object-iterable-methods.js @@ -0,0 +1,43 @@ +'use strict'; +const getDocumentationUrl = require('./utils/get-documentation-url'); + +const MESSAGE_ID = 'proper-object-iterable-methods'; +const messages = { + [MESSAGE_ID]: 'Prefer `{{replacement}}` over `{{value}}`.' +}; + +const create = context => { + return { + Literal(node) { + if (node.value !== 'unicorn') { + return; + } + + context.report({ + node, + messageId: MESSAGE_ID, + data: { + value: 'unicorn', + replacement: '🦄' + }, + fix: fixer => fixer.replaceText(node, '\'🦄\'') + }); + } + } +}; + +const schema = []; + +module.exports = { + create, + meta: { + type: 'suggestion', + docs: { + description: 'Enforce proper methods when iterate through objects.', + url: getDocumentationUrl(__filename) + }, + fixable: 'code', + schema, + messages + } +}; diff --git a/test/proper-object-iterable-methods.mjs b/test/proper-object-iterable-methods.mjs new file mode 100644 index 0000000000..02595297ba --- /dev/null +++ b/test/proper-object-iterable-methods.mjs @@ -0,0 +1,13 @@ +import outdent from 'outdent'; +import {getTester} from './utils/test.mjs'; + +const {test} = getTester(import.meta); + +test.snapshot({ + valid: [ + 'const foo = "🦄";' + ], + invalid: [ + 'const foo = "unicorn";' + ] +}); From a4e19a97f44dbb86b5ec4fe619167808ecbc73f0 Mon Sep 17 00:00:00 2001 From: fisker Cheung Date: Wed, 14 Apr 2021 12:48:00 +0800 Subject: [PATCH 2/2] Init --- rules/proper-object-iterable-methods.js | 211 +++++++++++++++++- test/proper-object-iterable-methods.mjs | 15 +- .../proper-object-iterable-methods.mjs.md | 53 +++++ .../proper-object-iterable-methods.mjs.snap | Bin 0 -> 335 bytes 4 files changed, 266 insertions(+), 13 deletions(-) create mode 100644 test/snapshots/proper-object-iterable-methods.mjs.md create mode 100644 test/snapshots/proper-object-iterable-methods.mjs.snap diff --git a/rules/proper-object-iterable-methods.js b/rules/proper-object-iterable-methods.js index 61190daba5..f1bcc87724 100644 --- a/rules/proper-object-iterable-methods.js +++ b/rules/proper-object-iterable-methods.js @@ -1,27 +1,216 @@ 'use strict'; +const {findVariable} = require('eslint-utils'); const getDocumentationUrl = require('./utils/get-documentation-url'); +const methodSelector = require('./utils/method-selector'); +const isSameReference = require('./utils/is-same-reference'); const MESSAGE_ID = 'proper-object-iterable-methods'; const messages = { - [MESSAGE_ID]: 'Prefer `{{replacement}}` over `{{value}}`.' + [MESSAGE_ID]: 'Prefer `Object.{{replacement}}()` over `Object.{{method}}()`.' }; +const objectStaticMethodSelector = property => methodSelector({ + object: 'Object', + names: ['keys', 'entries'], + length: 1, + property, +}); + +const forOfSelector = [ + 'ForOfStatement', + '[await!=true]', + objectStaticMethodSelector('right'), + '> .body' +].join('') + +const arrayMethodSelector = [ + methodSelector({ + names: [ + 'every', + 'filter', + 'find', + 'findIndex', + 'flatMap', + 'forEach', + 'map', + 'reduce', + 'reduceRight', + 'some' + ], + min: 1 + }), + objectStaticMethodSelector('callee.object'), + '> :function.arguments' +].join('') + +function getVariablesNode(node, method) { + const parameters = node.params; + const [firstParameter, secondParameter] = parameters; + + if (method !== 'reduce' && method !== 'reduceRight') { + return firstParameter; + } + + if (firstParameter && variablesNode.type !== 'RestElement') { + return secondParameter; + } +} + +function getDeclaredVariables(node, method) { + if (!node) { + return; + } + + let isConst = true; + if ( + node.type === 'VariableDeclaration' && + node.declarations.length === 1 + ) { + isConst = node.kind === 'const' + node = node.declarations[0].id; + } + + if (method === 'entries' && node.type === 'ArrayPattern') { + const [key, value] = node.elements; + const result = {isConst}; + if (key && key.type === 'Identifier') { + result.key = key; + } + if (value && value.type === 'Identifier') { + result.value = value; + } + + return result; + } + + if (node.type === 'Identifier') { + const type = method === 'keys' ? 'key' : 'pairs'; + + return {isConst, [type]: node} + } +} + +function isSame(nodeA, nodeB, scope) { + if (!isSameReference(nodeA, nodeB)) { + return false; + } + + if (nodeA.type === 'Identifier') { + // Check variable + } + + return true; +} + +function getProblem({node, block, scope, memberExpressions}) { + const {type} = node; + let variablesNode; + let objectMethodCallNode; + if (node.type === 'ForOfStatement') { + variablesNode = node.left; + objectMethodCallNode = node.right; + } else { + variablesNode = getVariablesNode(node.arguments[0], node.callee.property.name); + objectMethodCallNode = node.callee.object; + } + + const methodNode = objectMethodCallNode.callee.property; + const method = methodNode.name; + const object = objectMethodCallNode.arguments[0]; + + const {key, pairs} = getDeclaredVariables(variablesNode, method) || {}; + const accessingValue = memberExpressions.some( + node => { + const {object: memberObject, property, range, computed} = node; + if ( + range[0] < block.range[0] || + range[1] > block.range[1] || + !computed + ) { + return false; + } + + if (!isSame(memberObject, object, scope)) { + return false; + } + + if ( + key && + property.type === 'Identifier' && + isSame(property, key, scope) + ) { + return true; + } + + if ( + pairs && + property.type === 'MemberExpression' && + property.computed && + isSame(property.object, pairs, scope) && + property.property.type === 'Literal' && + property.property.value === 0 + ) { + return true; + } + + return false; + } + ); + + if (method === 'keys' && accessingValue) { + return { + node: methodNode, + messageId: MESSAGE_ID, + // TODO: if `key` is not used in other places, should suggest `.values()` + data: {method, replacement: 'entries'} + } + } + + if (method === 'entries' && !accessingValue) { + return { + node: methodNode, + messageId: MESSAGE_ID, + data: {method, replacement: 'keys'} + } + } +} + const create = context => { + const nodes = new Set(); + const memberExpressions = []; + return { - Literal(node) { - if (node.value !== 'unicorn') { + MemberExpression(node) { + memberExpressions.push(node); + }, + [forOfSelector](node) { + nodes.add({ + node: node.parent, + block: node, + scope: context.getScope() + }); + }, + [arrayMethodSelector](node) { + // Check function is the first argument (callback) of array method calls, + // Can't check this in selector + const arrayCall = node.parent; + if (arrayCall.arguments[0] !== node) { return; } - context.report({ - node, - messageId: MESSAGE_ID, - data: { - value: 'unicorn', - replacement: '🦄' - }, - fix: fixer => fixer.replaceText(node, '\'🦄\'') + nodes.add({ + node: node.parent, + block: node.body, + scope: context.getScope() }); + }, + 'Program:exit'() { + for (const nodeInfo of nodes) { + const problem = getProblem({...nodeInfo, memberExpressions}); + if (problem) { + context.report(problem); + } + } } } }; diff --git a/test/proper-object-iterable-methods.mjs b/test/proper-object-iterable-methods.mjs index 02595297ba..34f4268b4b 100644 --- a/test/proper-object-iterable-methods.mjs +++ b/test/proper-object-iterable-methods.mjs @@ -3,11 +3,22 @@ import {getTester} from './utils/test.mjs'; const {test} = getTester(import.meta); +// `Object.keys` test.snapshot({ valid: [ - 'const foo = "🦄";' ], invalid: [ - 'const foo = "unicorn";' + outdent` + for (const key of Object.keys(foo)) { + bar(foo[key]); + } + `, + outdent` + for (const key of Object.keys(foo)) { + bar(foo[key], key); + } + `, + 'const foo = Object.keys(bar).map(key => baz(bar[key]));', + 'const foo = Object.keys(bar).map(key => baz(bar[key], key));', ] }); diff --git a/test/snapshots/proper-object-iterable-methods.mjs.md b/test/snapshots/proper-object-iterable-methods.mjs.md new file mode 100644 index 0000000000..3b461d8284 --- /dev/null +++ b/test/snapshots/proper-object-iterable-methods.mjs.md @@ -0,0 +1,53 @@ +# Snapshot report for `test/proper-object-iterable-methods.mjs` + +The actual snapshot is saved in `proper-object-iterable-methods.mjs.snap`. + +Generated by [AVA](https://avajs.dev). + +## Invalid #1 + 1 | for (const key of Object.keys(foo)) { + 2 | bar(foo[key]); + 3 | } + +> Error 1/1 + + `␊ + > 1 | for (const key of Object.keys(foo)) {␊ + | ^^^^ Prefer \`Object.entries()\` over \`Object.keys()\`.␊ + 2 | bar(foo[key]);␊ + 3 | }␊ + ` + +## Invalid #2 + 1 | for (const key of Object.keys(foo)) { + 2 | bar(foo[key], key); + 3 | } + +> Error 1/1 + + `␊ + > 1 | for (const key of Object.keys(foo)) {␊ + | ^^^^ Prefer \`Object.entries()\` over \`Object.keys()\`.␊ + 2 | bar(foo[key], key);␊ + 3 | }␊ + ` + +## Invalid #3 + 1 | const foo = Object.keys(bar).map(key => baz(bar[key])); + +> Error 1/1 + + `␊ + > 1 | const foo = Object.keys(bar).map(key => baz(bar[key]));␊ + | ^^^^ Prefer \`Object.entries()\` over \`Object.keys()\`.␊ + ` + +## Invalid #4 + 1 | const foo = Object.keys(bar).map(key => baz(bar[key], key)); + +> Error 1/1 + + `␊ + > 1 | const foo = Object.keys(bar).map(key => baz(bar[key], key));␊ + | ^^^^ Prefer \`Object.entries()\` over \`Object.keys()\`.␊ + ` diff --git a/test/snapshots/proper-object-iterable-methods.mjs.snap b/test/snapshots/proper-object-iterable-methods.mjs.snap new file mode 100644 index 0000000000000000000000000000000000000000..f61ac8e3924c8382837815eb48cdd7be0aee2d5a GIT binary patch literal 335 zcmV-V0kHl-RzV~G zJ)>G$bpKQ7u{Jw<)dgR=8Ns5jf%u=tn;FerylvBzRh+~!a?Ub>MdvXwFfcQ)gH2{+ zWe{YV#K2{zV5m@|ker`aT%wScpRZu6;GdM0np~onomyF}k(5}Zsh68rpaJA5*xD&1 zC02pNqJi949R(n%scFrn00cD(SU?;QC?EjTc!*H006zlk&^%b literal 0 HcmV?d00001