diff --git a/docs/rules/prefer-prototype-methods.md b/docs/rules/prefer-prototype-methods.md index 097647f04d..0b618f5d5c 100644 --- a/docs/rules/prefer-prototype-methods.md +++ b/docs/rules/prefer-prototype-methods.md @@ -1,8 +1,8 @@ -# Prefer borrowing methods from the prototype instead of methods from an instance. +# Prefer borrowing methods from the prototype instead of methods from an instance -When “borrowing” a method from a different object (especially generic methods from `Array`), it‘s clearer to get it from the prototype than from an instance. +When “borrowing” a method from `Array` or `Object`, it‘s clearer to get it from the prototype than from an instance. -This rule is fixable when using method from `[]` or `{}`. +This rule is fixable. ## Fail @@ -14,10 +14,6 @@ const array = [].slice.apply(bar); const hasProperty = {}.hasOwnProperty.call(foo, 'property'); ``` -```js -foo.bar.call(baz); -``` - ```js Reflect.apply([].forEach, arrayLike, [callback]); ``` @@ -32,14 +28,6 @@ const array = Array.prototype.slice.apply(bar); const hasProperty = Object.prototype.hasOwnProperty.call(foo, 'property'); ``` -```js -Foo.prototype.bar.call(baz); -``` - -```js -foo.constructor.prototype.bar.call(baz); -``` - ```js Reflect.apply(Array.prototype.forEach, arrayLike, [callback]); ``` diff --git a/rules/prefer-prototype-methods.js b/rules/prefer-prototype-methods.js index 1e98493c25..7d36ac3f4c 100644 --- a/rules/prefer-prototype-methods.js +++ b/rules/prefer-prototype-methods.js @@ -1,155 +1,54 @@ 'use strict'; -const {findVariable} = require('eslint-utils'); const getDocumentationUrl = require('./utils/get-documentation-url.js'); -const {methodCallSelector} = require('./selectors/index.js'); +const { + methodCallSelector, + emptyObjectSelector, + emptyArraySelector, + matches +} = require('./selectors/index.js'); const getPropertyName = require('./utils/get-property-name.js'); const messages = { - 'known-constructor-known-method': 'Prefer using `{{constructorName}}.prototype.{{methodName}}`.', - 'known-constructor-unknown-method': 'Prefer using method from `{{constructorName}}.prototype`.', - 'unknown-constructor-known-method': 'Prefer using `{{methodName}}` method from the constructor prototype.', - 'unknown-constructor-unknown-method': 'Prefer using method from the constructor prototype.' + 'known-method': 'Prefer using `{{constructorName}}.prototype.{{methodName}}`.', + 'unknown-method': 'Prefer using method from `{{constructorName}}.prototype`.' }; -const functionMethodsSelector = [ - methodCallSelector(['apply', 'bind', 'call']), - ' > ', - '.callee', - ' > ', - '.object' +const emptyObjectOrArrayMethodSelector = [ + 'MemberExpression', + matches([emptyObjectSelector('object'), emptyArraySelector('object')]) ].join(''); - -const reflectApplySelector = methodCallSelector({ - object: 'Reflect', - name: 'apply', - min: 1 -}); - -function getConstructorName(node) { - switch (node.type) { - case 'ArrayExpression': - return 'Array'; - case 'ObjectExpression': - return 'Object'; - // No default - } -} - -function isSafeToFix(node) { - switch (node.type) { - case 'ArrayExpression': - return node.elements.length === 0; - case 'ObjectExpression': - return node.properties.length === 0; - // No default - } -} - -function isObjectOwnProperty(property) { - // eslint-disable-next-line no-useless-call - return Object.prototype.hasOwnProperty.call(Object.prototype, property); -} +const selector = matches([ + // `[].foo.{apply,bind,call}(…)` + // `({}).foo.{apply,bind,call}(…)` + [ + methodCallSelector(['apply', 'bind', 'call']), + ' > ', + '.callee', + ' > ', + `${emptyObjectOrArrayMethodSelector}.object` + ].join(''), + // `Reflect.apply([].foo, …)` + // `Reflect.apply({}.foo, …)` + [ + methodCallSelector({object: 'Reflect', name: 'apply', min: 1}), + ' > ', + `${emptyObjectOrArrayMethodSelector}.arguments:first-child` + ].join('') +]); /** @param {import('eslint').Rule.RuleContext} context */ function create(context) { - const methods = new Set(); - const nonArrowFunctionStack = []; - const thisExpressions = new Map(); - - function check(method, scope) { - const {type, object} = method; - if ( - type !== 'MemberExpression' || - // Most likely it's a static method of a class - (object.type === 'Identifier' && /^[A-Z]/.test(object.name)) || - ( - object.type === 'MemberExpression' && - !object.computed && - !object.optional && - object.property.type === 'Identifier' && - object.property.name === 'prototype' - ) - ) { - return; - } - - const methodName = getPropertyName(method, scope); - if (!isObjectOwnProperty(methodName)) { - if (object.type === 'ObjectExpression' && object.properties.length > 0) { - return; - } - - if (object.type === 'ThisExpression') { - const functionNode = thisExpressions.get(object); - if ( - functionNode && - functionNode.parent.type === 'Property' && - functionNode.parent.value === functionNode && - functionNode.parent.parent.type === 'ObjectExpression' - ) { - return; - } - } - - if (object.type === 'Identifier') { - const variable = findVariable(scope, object); - if (variable && variable.defs.length === 1) { - const [definition] = variable.defs; - if ( - definition.type === 'Variable' && - definition.kind === 'const' && - definition.node.id === definition.name && - definition.node.type === 'VariableDeclarator' && - definition.node.init && - definition.node.init.type === 'ObjectExpression' - ) { - return; - } - } - } - } - - const constructorName = getConstructorName(object); - const messageId = [ - constructorName ? 'known' : 'unknown', - 'constructor', - methodName ? 'known' : 'unknown', - 'method' - ].join('-'); - - const problem = { - node: method, - messageId, - data: {constructorName, methodName: String(methodName)} - }; - - if (constructorName && isSafeToFix(object)) { - problem.fix = fixer => fixer.replaceText(object, `${constructorName}.prototype`); - } - - context.report(problem); - } - return { - 'FunctionExpression,FunctionDeclaration'(node) { - nonArrowFunctionStack.push(node); - }, - 'FunctionExpression,FunctionDeclaration:exit'() { - nonArrowFunctionStack.pop(); - }, - 'ThisExpression'(node) { - thisExpressions.set(node, nonArrowFunctionStack[nonArrowFunctionStack.length - 1]); - }, - [reflectApplySelector](node) { - methods.add({method: node.arguments[0], scope: context.getScope()}); - }, - [functionMethodsSelector](node) { - methods.add({method: node, scope: context.getScope()}); - }, - 'Program:exit'() { - for (const {method, scope} of methods) { - check(method, scope); - } + [selector](node) { + const constructorName = node.object.type === 'ArrayExpression' ? 'Array' : 'Object'; + const methodName = getPropertyName(node, context.getScope()); + + context.report({ + node, + messageId: methodName ? 'known-method' : 'unknown-method', + data: {constructorName, methodName: String(methodName)}, + fix: fixer => fixer.replaceText(node.object, `${constructorName}.prototype`) + }); } }; } diff --git a/rules/selectors/empty-array-selector.js b/rules/selectors/empty-array-selector.js index a0ca810155..8f48fc376c 100644 --- a/rules/selectors/empty-array-selector.js +++ b/rules/selectors/empty-array-selector.js @@ -1,7 +1,7 @@ 'use strict'; function emptyArraySelector(path) { - const prefix = `${path}.`; + const prefix = path ? `${path}.` : ''; return [ `[${prefix}type="ArrayExpression"]`, `[${prefix}elements.length=0]` diff --git a/rules/selectors/empty-object-selector.js b/rules/selectors/empty-object-selector.js index f4cd94e046..d231abdc1b 100644 --- a/rules/selectors/empty-object-selector.js +++ b/rules/selectors/empty-object-selector.js @@ -1,7 +1,7 @@ 'use strict'; function emptyObjectSelector(path) { - const prefix = `${path}.`; + const prefix = path ? `${path}.` : ''; return [ `[${prefix}type="ObjectExpression"]`, `[${prefix}properties.length=0]` diff --git a/rules/selectors/index.js b/rules/selectors/index.js index 1dbde3289d..294f39bac7 100644 --- a/rules/selectors/index.js +++ b/rules/selectors/index.js @@ -8,6 +8,7 @@ module.exports = { arrayPrototypeMethodSelector: require('./prototype-method-selector.js').arrayPrototypeMethodSelector, objectPrototypeMethodSelector: require('./prototype-method-selector.js').objectPrototypeMethodSelector, emptyArraySelector: require('./empty-array-selector.js'), + emptyObjectSelector: require('./empty-object-selector.js'), memberExpressionSelector: require('./member-expression-selector.js'), methodCallSelector: require('./method-call-selector.js'), notDomNodeSelector: require('./not-dom-node.js').notDomNodeSelector, diff --git a/test/prefer-prototype-methods.mjs b/test/prefer-prototype-methods.mjs index 582e1c4acb..33b82dfd10 100644 --- a/test/prefer-prototype-methods.mjs +++ b/test/prefer-prototype-methods.mjs @@ -1,4 +1,3 @@ -import outdent from 'outdent'; import {getTester} from './utils/test.mjs'; const {test} = getTester(import.meta); @@ -10,6 +9,9 @@ test.snapshot({ 'const foo = Object.prototype.toString.call(bar);', 'const foo = Object.prototype.hasOwnProperty.call(bar, "property");', 'const foo = Object.prototype.propertyIsEnumerable.call(bar, "property");', + 'const foo = "".charCodeAt.call(bar, 0);', + 'const foo = [notEmpty].push.apply(bar, elements);', + 'const foo = {notEmpty}.toString.call(bar);', 'Array.prototype.forEach.call(foo, () => {})', 'const push = Array.prototype.push.bind(foo)', 'const push = [].push', @@ -24,8 +26,12 @@ test.snapshot({ 'NotReflect.apply([].slice, foo, [])', 'Reflect.notApply([].slice, foo, [])', 'Reflect.apply([]?.slice, foo, [])', - // This better use `Foo.prototype.bar.call(baz)`, not handled - 'foo.constructor.prototype.bar.call(baz)' + 'Reflect.apply(foo, [].bar, [].bar)', + 'class Foo {bar() { this.baz.bind(this) }}', + 'const a = {bar() { this.baz.bind(this) }}', + 'foo.bar.bind(foo)', + 'foo.bar.bind(bar)', + 'foo[{}].call(bar)' ], invalid: [ 'const foo = [].push.apply(bar, elements);', @@ -35,98 +41,13 @@ test.snapshot({ 'const foo = {}.propertyIsEnumerable.call(bar, "property");', '[].forEach.call(foo, () => {})', 'const push = [].push.bind(foo)', - 'const foo = bar.method.call(foo)', - 'const foo = bar[method].call(foo)', - 'const method = "realMethodName";const foo = bar[method].call(foo)', 'const foo = [][method].call(foo)', 'const method = "realMethodName";const foo = [][method].call(foo)', - 'const foo = [1].push.apply(bar, elements);', 'const array = Reflect.apply([].slice, foo, [])', - 'Reflect.apply(foo.bar, baz, [])', - // False positive - 'Array["prototype"].slice.call();', - 'Array?.prototype.slice.call();', - 'window.Math.max.apply(null, numbers)' - ] -}); - -// Object method -test.snapshot({ - valid: [ - outdent` - const foo = { - a() { - this.method = this.method.bind(this); - }, - b: function() { - this.method = this.method.bind(this); - }, - c() { - const foo = () => this.method.bind(this); - }, - d: { - d1() { - this.method.call(this); - } - } - } - `, - outdent` - const foo = {}; - const bar = foo.method.call(foo, 'property'); - `, - '({method() {}}).method.call(foo)' - ], - invalid: [ - outdent` - const foo = { - a: () => { - this.method = this.method.bind(this); - } - } - `, - outdent` - const foo = { - [function() {this.method.bind(this);}]: 1 - } - `, - outdent` - const {foo} = {foo: {}}; - const bar = foo.method.call(foo, 'property'); - `, - outdent` - const [foo] = [{}]; - const bar = foo.method.call(foo, 'property'); - `, - outdent` - const foo = { - a() { - this.propertyIsEnumerable.apply(this, []); - } - } - `, - outdent` - const foo = { - a() { - function fn() { - // this is not the object foo - this.method.call(this); - } - return fn; - } - } - `, - 'this.method = this.method.bind(this)', - outdent` - const foo = {}; - const bar = foo.hasOwnProperty.call(foo, 'property'); - `, - 'for (const foo of []) foo.bar.call(foo)', - outdent` - let foo = {}; - const bar = foo.method.call(foo, 'property'); - `, - '({method() {}}).propertyIsEnumerable.call(foo)' + 'Reflect.apply([].bar, baz, [])', + 'const foo = ({}).toString.call(bar);', + 'const foo = ({}.toString).call(bar);', + 'const foo = ({}.toString.call)(bar);' ] }); @@ -137,12 +58,14 @@ test.babel({ valid: [], invalid: [ { - code: 'Reflect.apply(foo[Symbol()], baz, [])', - errors: [{message: 'Prefer using method from the constructor prototype.'}] + code: 'Reflect.apply({}[Symbol()], baz, [])', + output: 'Reflect.apply(Object.prototype[Symbol()], baz, [])', + errors: [{message: 'Prefer using method from `Object.prototype`.'}] }, { - code: 'Reflect.apply(foo[Symbol("symbol description")], baz, [])', - errors: [{message: 'Prefer using method from the constructor prototype.'}] + code: 'Reflect.apply({}[Symbol("symbol description")], baz, [])', + output: 'Reflect.apply(Object.prototype[Symbol("symbol description")], baz, [])', + errors: [{message: 'Prefer using method from `Object.prototype`.'}] }, { code: 'Reflect.apply([][Symbol()], baz, [])', diff --git a/test/snapshots/prefer-prototype-methods.mjs.md b/test/snapshots/prefer-prototype-methods.mjs.md index ce3c046f11..37be2cbe6f 100644 --- a/test/snapshots/prefer-prototype-methods.mjs.md +++ b/test/snapshots/prefer-prototype-methods.mjs.md @@ -117,36 +117,6 @@ Generated by [AVA](https://avajs.dev). ` ## Invalid #8 - 1 | const foo = bar.method.call(foo) - -> Error 1/1 - - `␊ - > 1 | const foo = bar.method.call(foo)␊ - | ^^^^^^^^^^ Prefer using \`method\` method from the constructor prototype.␊ - ` - -## Invalid #9 - 1 | const foo = bar[method].call(foo) - -> Error 1/1 - - `␊ - > 1 | const foo = bar[method].call(foo)␊ - | ^^^^^^^^^^^ Prefer using method from the constructor prototype.␊ - ` - -## Invalid #10 - 1 | const method = "realMethodName";const foo = bar[method].call(foo) - -> Error 1/1 - - `␊ - > 1 | const method = "realMethodName";const foo = bar[method].call(foo)␊ - | ^^^^^^^^^^^ Prefer using \`realMethodName\` method from the constructor prototype.␊ - ` - -## Invalid #11 1 | const foo = [][method].call(foo) > Output @@ -162,7 +132,7 @@ Generated by [AVA](https://avajs.dev). | ^^^^^^^^^^ Prefer using method from \`Array.prototype\`.␊ ` -## Invalid #12 +## Invalid #9 1 | const method = "realMethodName";const foo = [][method].call(foo) > Output @@ -178,17 +148,7 @@ Generated by [AVA](https://avajs.dev). | ^^^^^^^^^^ Prefer using \`Array.prototype.realMethodName\`.␊ ` -## Invalid #13 - 1 | const foo = [1].push.apply(bar, elements); - -> Error 1/1 - - `␊ - > 1 | const foo = [1].push.apply(bar, elements);␊ - | ^^^^^^^^ Prefer using \`Array.prototype.push\`.␊ - ` - -## Invalid #14 +## Invalid #10 1 | const array = Reflect.apply([].slice, foo, []) > Output @@ -204,196 +164,66 @@ Generated by [AVA](https://avajs.dev). | ^^^^^^^^ Prefer using \`Array.prototype.slice\`.␊ ` -## Invalid #15 - 1 | Reflect.apply(foo.bar, baz, []) - -> Error 1/1 - - `␊ - > 1 | Reflect.apply(foo.bar, baz, [])␊ - | ^^^^^^^ Prefer using \`bar\` method from the constructor prototype.␊ - ` - -## Invalid #16 - 1 | Array["prototype"].slice.call(); - -> Error 1/1 - - `␊ - > 1 | Array["prototype"].slice.call();␊ - | ^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using \`slice\` method from the constructor prototype.␊ - ` - -## Invalid #17 - 1 | Array?.prototype.slice.call(); - -> Error 1/1 - - `␊ - > 1 | Array?.prototype.slice.call();␊ - | ^^^^^^^^^^^^^^^^^^^^^^ Prefer using \`slice\` method from the constructor prototype.␊ - ` - -## Invalid #18 - 1 | window.Math.max.apply(null, numbers) - -> Error 1/1 - - `␊ - > 1 | window.Math.max.apply(null, numbers)␊ - | ^^^^^^^^^^^^^^^ Prefer using \`max\` method from the constructor prototype.␊ - ` - -## Invalid #1 - 1 | const foo = { - 2 | a: () => { - 3 | this.method = this.method.bind(this); - 4 | } - 5 | } - -> Error 1/1 - - `␊ - 1 | const foo = {␊ - 2 | a: () => {␊ - > 3 | this.method = this.method.bind(this);␊ - | ^^^^^^^^^^^ Prefer using \`method\` method from the constructor prototype.␊ - 4 | }␊ - 5 | }␊ - ` - -## Invalid #2 - 1 | const foo = { - 2 | [function() {this.method.bind(this);}]: 1 - 3 | } - -> Error 1/1 - - `␊ - 1 | const foo = {␊ - > 2 | [function() {this.method.bind(this);}]: 1␊ - | ^^^^^^^^^^^ Prefer using \`method\` method from the constructor prototype.␊ - 3 | }␊ - ` - -## Invalid #3 - 1 | const {foo} = {foo: {}}; - 2 | const bar = foo.method.call(foo, 'property'); - -> Error 1/1 - - `␊ - 1 | const {foo} = {foo: {}};␊ - > 2 | const bar = foo.method.call(foo, 'property');␊ - | ^^^^^^^^^^ Prefer using \`method\` method from the constructor prototype.␊ - ` - -## Invalid #4 - 1 | const [foo] = [{}]; - 2 | const bar = foo.method.call(foo, 'property'); +## Invalid #11 + 1 | Reflect.apply([].bar, baz, []) -> Error 1/1 +> Output `␊ - 1 | const [foo] = [{}];␊ - > 2 | const bar = foo.method.call(foo, 'property');␊ - | ^^^^^^^^^^ Prefer using \`method\` method from the constructor prototype.␊ + 1 | Reflect.apply(Array.prototype.bar, baz, [])␊ ` -## Invalid #5 - 1 | const foo = { - 2 | a() { - 3 | this.propertyIsEnumerable.apply(this, []); - 4 | } - 5 | } - > Error 1/1 `␊ - 1 | const foo = {␊ - 2 | a() {␊ - > 3 | this.propertyIsEnumerable.apply(this, []);␊ - | ^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using \`propertyIsEnumerable\` method from the constructor prototype.␊ - 4 | }␊ - 5 | }␊ + > 1 | Reflect.apply([].bar, baz, [])␊ + | ^^^^^^ Prefer using \`Array.prototype.bar\`.␊ ` -## Invalid #6 - 1 | const foo = { - 2 | a() { - 3 | function fn() { - 4 | // this is not the object foo - 5 | this.method.call(this); - 6 | } - 7 | return fn; - 8 | } - 9 | } +## Invalid #12 + 1 | const foo = ({}).toString.call(bar); -> Error 1/1 +> Output `␊ - 1 | const foo = {␊ - 2 | a() {␊ - 3 | function fn() {␊ - 4 | // this is not the object foo␊ - > 5 | this.method.call(this);␊ - | ^^^^^^^^^^^ Prefer using \`method\` method from the constructor prototype.␊ - 6 | }␊ - 7 | return fn;␊ - 8 | }␊ - 9 | }␊ + 1 | const foo = (Object.prototype).toString.call(bar);␊ ` -## Invalid #7 - 1 | this.method = this.method.bind(this) - > Error 1/1 `␊ - > 1 | this.method = this.method.bind(this)␊ - | ^^^^^^^^^^^ Prefer using \`method\` method from the constructor prototype.␊ + > 1 | const foo = ({}).toString.call(bar);␊ + | ^^^^^^^^^^^^^ Prefer using \`Object.prototype.toString\`.␊ ` -## Invalid #8 - 1 | const foo = {}; - 2 | const bar = foo.hasOwnProperty.call(foo, 'property'); +## Invalid #13 + 1 | const foo = ({}.toString).call(bar); -> Error 1/1 +> Output `␊ - 1 | const foo = {};␊ - > 2 | const bar = foo.hasOwnProperty.call(foo, 'property');␊ - | ^^^^^^^^^^^^^^^^^^ Prefer using \`hasOwnProperty\` method from the constructor prototype.␊ + 1 | const foo = (Object.prototype.toString).call(bar);␊ ` -## Invalid #9 - 1 | for (const foo of []) foo.bar.call(foo) - > Error 1/1 `␊ - > 1 | for (const foo of []) foo.bar.call(foo)␊ - | ^^^^^^^ Prefer using \`bar\` method from the constructor prototype.␊ + > 1 | const foo = ({}.toString).call(bar);␊ + | ^^^^^^^^^^^ Prefer using \`Object.prototype.toString\`.␊ ` -## Invalid #10 - 1 | let foo = {}; - 2 | const bar = foo.method.call(foo, 'property'); +## Invalid #14 + 1 | const foo = ({}.toString.call)(bar); -> Error 1/1 +> Output `␊ - 1 | let foo = {};␊ - > 2 | const bar = foo.method.call(foo, 'property');␊ - | ^^^^^^^^^^ Prefer using \`method\` method from the constructor prototype.␊ + 1 | const foo = (Object.prototype.toString.call)(bar);␊ ` -## Invalid #11 - 1 | ({method() {}}).propertyIsEnumerable.call(foo) - > Error 1/1 `␊ - > 1 | ({method() {}}).propertyIsEnumerable.call(foo)␊ - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using \`Object.prototype.propertyIsEnumerable\`.␊ + > 1 | const foo = ({}.toString.call)(bar);␊ + | ^^^^^^^^^^^ Prefer using \`Object.prototype.toString\`.␊ ` diff --git a/test/snapshots/prefer-prototype-methods.mjs.snap b/test/snapshots/prefer-prototype-methods.mjs.snap index 5615c4ef70..de1f4afd7f 100644 Binary files a/test/snapshots/prefer-prototype-methods.mjs.snap and b/test/snapshots/prefer-prototype-methods.mjs.snap differ