diff --git a/rules/no-array-push-push.js b/rules/no-array-push-push.js index 7b5c9e6894..f5f7c13da2 100644 --- a/rules/no-array-push-push.js +++ b/rules/no-array-push-push.js @@ -3,6 +3,7 @@ const {hasSideEffect, isCommaToken, isSemicolonToken} = require('eslint-utils'); const getDocumentationUrl = require('./utils/get-documentation-url'); const methodSelector = require('./utils/method-selector'); const getCallExpressionArgumentsText = require('./utils/get-call-expression-arguments-text'); +const isSameReference = require('./utils/is-same-reference'); const ERROR = 'error'; const SUGGESTION = 'suggestion'; @@ -53,7 +54,7 @@ function create(context) { const secondCallArray = secondCall.callee.object; // Not same array - if (sourceCode.getText(firstCallArray) !== sourceCode.getText(secondCallArray)) { + if (!isSameReference(firstCallArray, secondCallArray)) { return; } @@ -83,10 +84,7 @@ function create(context) { ); }; - if ( - hasSideEffect(firstCallArray, sourceCode) || - secondCallArguments.some(element => hasSideEffect(element, sourceCode)) - ) { + if (secondCallArguments.some(element => hasSideEffect(element, sourceCode))) { problem.suggest = [ { messageId: SUGGESTION, diff --git a/rules/prefer-ternary.js b/rules/prefer-ternary.js index 69b5641416..0a81645bc5 100644 --- a/rules/prefer-ternary.js +++ b/rules/prefer-ternary.js @@ -4,6 +4,8 @@ const {flatten} = require('lodash'); const getDocumentationUrl = require('./utils/get-documentation-url'); const avoidCapture = require('./utils/avoid-capture'); const extendFixRange = require('./utils/extend-fix-range'); +const needsSemicolon = require('./utils/needs-semicolon'); +const isSameReference = require('./utils/is-same-reference'); const messageId = 'prefer-ternary'; @@ -37,11 +39,6 @@ function getNodeBody(node) { return node; } -function isSameAssignmentLeft(node1, node2) { - // [TODO]: Allow more types of left - return node1.type === node2.type && node1.type === 'Identifier' && node1.name === node2.name; -} - const getIndentString = (node, sourceCode) => { const {line, column} = sourceCode.getLocFromIndex(node.range[0]); const lines = sourceCode.getLines(); @@ -171,12 +168,12 @@ const create = context => { if ( type === 'AssignmentExpression' && - isSameAssignmentLeft(left, alternate.left) && operator === alternate.operator && !isTernary(left) && !isTernary(alternate.left) && !isTernary(right) && - !isTernary(alternate.right) + !isTernary(alternate.right) && + isSameReference(left, alternate.left) ) { return merge({ before: `${before}${sourceCode.getText(left)} ${operator} `, @@ -252,7 +249,13 @@ const create = context => { generateNewVariables = true; } - const fixed = `${before}${testText} ? ${consequentText} : ${alternateText}${after}`; + let fixed = `${before}${testText} ? ${consequentText} : ${alternateText}${after}`; + const tokenBefore = sourceCode.getTokenBefore(node); + const shouldAddSemicolonBefore = needsSemicolon(tokenBefore, sourceCode, fixed); + if (shouldAddSemicolonBefore) { + fixed = `;${fixed}`; + } + yield fixer.replaceText(node, fixed); if (generateNewVariables) { diff --git a/rules/utils/is-same-reference.js b/rules/utils/is-same-reference.js new file mode 100644 index 0000000000..9b1a275b74 --- /dev/null +++ b/rules/utils/is-same-reference.js @@ -0,0 +1,152 @@ +'use strict'; +const {getStaticValue} = require('eslint-utils'); + +// Copied from https://github.com/eslint/eslint/blob/c3e9accce2f61b04ab699fd37c90703305281aa3/lib/rules/utils/ast-utils.js#L379 + +/** +Gets the property name of a given node. +The node can be a MemberExpression, a Property, or a MethodDefinition. + +If the name is dynamic, this returns `null`. + +For examples: + + a.b // => "b" + a["b"] // => "b" + a['b'] // => "b" + a[`b`] // => "b" + a[100] // => "100" + a[b] // => null + a["a" + "b"] // => null + a[tag`b`] // => null + a[`${b}`] // => null + + let a = {b: 1} // => "b" + let a = {["b"]: 1} // => "b" + let a = {['b']: 1} // => "b" + let a = {[`b`]: 1} // => "b" + let a = {[100]: 1} // => "100" + let a = {[b]: 1} // => null + let a = {["a" + "b"]: 1} // => null + let a = {[tag`b`]: 1} // => null + let a = {[`${b}`]: 1} // => null +@param {ASTNode} node The node to get. +@returns {string|undefined} The property name if static. Otherwise, undefined. +*/ +function getStaticPropertyName(node) { + let property; + + switch (node && node.type) { + case 'MemberExpression': + property = node.property; + break; + + /* istanbul ignore next: Hard to test */ + case 'ChainExpression': + return getStaticPropertyName(node.expression); + + /* istanbul ignore next: Only reachable when use this to get class/object member key */ + case 'Property': + case 'MethodDefinition': + /* istanbul ignore next */ + property = node.key; + /* istanbul ignore next */ + break; + + // No default + } + + if (property) { + if (property.type === 'Identifier' && !node.computed) { + return property.name; + } + + const staticResult = getStaticValue(property); + if (!staticResult) { + return; + } + + return String(staticResult.value); + } +} + +/** +Check if two literal nodes are the same value. +@param {ASTNode} left The Literal node to compare. +@param {ASTNode} right The other Literal node to compare. +@returns {boolean} `true` if the two literal nodes are the same value. +*/ +function equalLiteralValue(left, right) { + // RegExp literal. + if (left.regex || right.regex) { + return Boolean( + left.regex && + right.regex && + left.regex.pattern === right.regex.pattern && + left.regex.flags === right.regex.flags + ); + } + + // BigInt literal. + if (left.bigint || right.bigint) { + return left.bigint === right.bigint; + } + + return left.value === right.value; +} + +/** +Check if two expressions reference the same value. For example: + a = a + a.b = a.b + a[0] = a[0] + a['b'] = a['b'] +@param {ASTNode} left The left side of the comparison. +@param {ASTNode} right The right side of the comparison. +@returns {boolean} `true` if both sides match and reference the same value. +*/ +function isSameReference(left, right) { + if (left.type !== right.type) { + // Handle `a.b` and `a?.b` are samely. + if (left.type === 'ChainExpression') { + return isSameReference(left.expression, right); + } + + if (right.type === 'ChainExpression') { + return isSameReference(left, right.expression); + } + + return false; + } + + switch (left.type) { + case 'Super': + case 'ThisExpression': + return true; + + case 'Identifier': + return left.name === right.name; + + case 'Literal': + return equalLiteralValue(left, right); + + case 'ChainExpression': + return isSameReference(left.expression, right.expression); + + case 'MemberExpression': { + const nameA = getStaticPropertyName(left); + + // X.y = x["y"] + return ( + typeof nameA !== 'undefined' && + isSameReference(left.object, right.object) && + nameA === getStaticPropertyName(right) + ); + } + + default: + return false; + } +} + +module.exports = isSameReference; diff --git a/test/no-array-push-push.mjs b/test/no-array-push-push.mjs index 1e47869821..ab72db7fed 100644 --- a/test/no-array-push-push.mjs +++ b/test/no-array-push-push.mjs @@ -40,10 +40,14 @@ test.snapshot({ foo.push(1); const length = foo.push(2); `, - // We are comparing array with source code + // Not considered same array outdent` - foo.bar.push(1); - (foo).bar.push(2); + foo().push(1); + foo().push(2); + `, + outdent` + foo().bar.push(1); + foo().bar.push(2); ` ], invalid: [ @@ -104,15 +108,6 @@ test.snapshot({ foo.push(1); foo.push(bar()); `, - // Array has side effect - outdent` - foo().push(1); - foo().push(2); - `, - outdent` - foo().bar.push(1); - foo().bar.push(2); - `, // Multiple outdent` foo.push(1,); @@ -144,6 +139,93 @@ test.snapshot({ foo.push(1) foo.push(2) ;[foo].forEach(bar) + `, + // Still same array + outdent` + foo.bar.push(1); + (foo)['bar'].push(2); + ` + ] +}); + +// `isSameReference` coverage +test({ + valid: [ + outdent` + a[x].push(1); + a[x].push(2); + + '1'.someMagicPropertyReturnsAnArray.push(1); + (1).someMagicPropertyReturnsAnArray.push(2); + + /a/i.someMagicPropertyReturnsAnArray.push(1); + /b/g.someMagicPropertyReturnsAnArray.push(2); + + 1n.someMagicPropertyReturnsAnArray.push(1); + 2n.someMagicPropertyReturnsAnArray.push(2); + + (true).someMagicPropertyReturnsAnArray.push(1); + (false).someMagicPropertyReturnsAnArray.push(2); ` + ], + invalid: [ + { + code: outdent` + class A extends B { + foo() { + this.push(1); + this.push(2); + + super.x.push(1); + super.x.push(2); + + ((a?.x).y).push(1); + (a.x?.y).push(1); + + ((a?.x.y).z).push(1); + ((a.x?.y).z).push(1); + + a[null].push(1); + a['null'].push(1); + + '1'.someMagicPropertyReturnsAnArray.push(1); + '1'.someMagicPropertyReturnsAnArray.push(2); + + /a/i.someMagicPropertyReturnsAnArray.push(1); + /a/i.someMagicPropertyReturnsAnArray.push(2); + + 1n.someMagicPropertyReturnsAnArray.push(1); + 1n.someMagicPropertyReturnsAnArray.push(2); + + (true).someMagicPropertyReturnsAnArray.push(1); + (true).someMagicPropertyReturnsAnArray.push(2); + } + } + `, + output: outdent` + class A extends B { + foo() { + this.push(1, 2); + + super.x.push(1, 2); + + ((a?.x).y).push(1, 1); + + ((a?.x.y).z).push(1, 1); + + a[null].push(1, 1); + + '1'.someMagicPropertyReturnsAnArray.push(1, 2); + + /a/i.someMagicPropertyReturnsAnArray.push(1, 2); + + 1n.someMagicPropertyReturnsAnArray.push(1, 2); + + (true).someMagicPropertyReturnsAnArray.push(1, 2); + } + } + `, + errors: 9 + } ] }); diff --git a/test/prefer-ternary.mjs b/test/prefer-ternary.mjs index 323e32151a..5ada75400d 100644 --- a/test/prefer-ternary.mjs +++ b/test/prefer-ternary.mjs @@ -807,13 +807,13 @@ test({ } } `, - // Same `left`, but not handled + // Not same `left` outdent` function unicorn() { if(test){ - foo.bar = a; + foo().bar = a; } else{ - foo.bar = b; + foo().bar = b; } } `, @@ -917,6 +917,43 @@ test({ `, errors }, + // Same `left` + { + code: outdent` + function unicorn() { + if (test) { + foo.bar = a; + } else{ + foo.bar = b; + } + } + `, + output: outdent` + function unicorn() { + foo.bar = test ? a : b; + } + `, + errors + }, + { + code: outdent` + function unicorn() { + a() + if (test) { + (foo)['b' + 'ar'] = a + } else{ + foo.bar = b + } + } + `, + output: outdent` + function unicorn() { + a() + ;(foo)['b' + 'ar'] = test ? a : b; + } + `, + errors + }, // Crazy nested { code: outdent` diff --git a/test/snapshots/no-array-push-push.mjs.md b/test/snapshots/no-array-push-push.mjs.md index 41cb8bc1bc..7e65927f52 100644 --- a/test/snapshots/no-array-push-push.mjs.md +++ b/test/snapshots/no-array-push-push.mjs.md @@ -255,38 +255,6 @@ Generated by [AVA](https://avajs.dev). ` ## Invalid #15 - 1 | foo().push(1); - 2 | foo().push(2); - -> Error 1/1 - - `␊ - 1 | foo().push(1);␊ - > 2 | foo().push(2);␊ - | ^^^^ Do not call `Array#push()` multiple times.␊ - ␊ - --------------------------------------------------------------------------------␊ - Suggestion 1/1: Merge with previous one.␊ - 1 | foo().push(1, 2);␊ - ` - -## Invalid #16 - 1 | foo().bar.push(1); - 2 | foo().bar.push(2); - -> Error 1/1 - - `␊ - 1 | foo().bar.push(1);␊ - > 2 | foo().bar.push(2);␊ - | ^^^^ Do not call `Array#push()` multiple times.␊ - ␊ - --------------------------------------------------------------------------------␊ - Suggestion 1/1: Merge with previous one.␊ - 1 | foo().bar.push(1, 2);␊ - ` - -## Invalid #17 1 | foo.push(1,); 2 | foo.push(2,); 3 | foo.push(3,); @@ -315,7 +283,7 @@ Generated by [AVA](https://avajs.dev). | ^^^^ Do not call `Array#push()` multiple times.␊ ` -## Invalid #18 +## Invalid #16 1 | if (a) { 2 | foo.push(1); 3 | foo.push(2); @@ -339,7 +307,7 @@ Generated by [AVA](https://avajs.dev). 4 | }␊ ` -## Invalid #19 +## Invalid #17 1 | switch (a) { 2 | default: 3 | foo.push(1); @@ -366,7 +334,7 @@ Generated by [AVA](https://avajs.dev). 5 | }␊ ` -## Invalid #20 +## Invalid #18 1 | function a() { 2 | foo.push(1); 3 | foo.push(2); @@ -390,7 +358,7 @@ Generated by [AVA](https://avajs.dev). 4 | }␊ ` -## Invalid #21 +## Invalid #19 1 | foo.push(1) 2 | foo.push(2) 3 | ;[foo].forEach(bar) @@ -409,3 +377,21 @@ Generated by [AVA](https://avajs.dev). | ^^^^ Do not call `Array#push()` multiple times.␊ 3 | ;[foo].forEach(bar)␊ ` + +## Invalid #20 + 1 | foo.bar.push(1); + 2 | (foo)['bar'].push(2); + +> Output + + `␊ + 1 | foo.bar.push(1, 2);␊ + ` + +> Error 1/1 + + `␊ + 1 | foo.bar.push(1);␊ + > 2 | (foo)['bar'].push(2);␊ + | ^^^^ Do not call `Array#push()` multiple times.␊ + ` diff --git a/test/snapshots/no-array-push-push.mjs.snap b/test/snapshots/no-array-push-push.mjs.snap index 351a9e0c19..3a2ce56c13 100644 Binary files a/test/snapshots/no-array-push-push.mjs.snap and b/test/snapshots/no-array-push-push.mjs.snap differ