Skip to content

Commit

Permalink
no-array-push-push and prefer-ternary: Improve same reference det…
Browse files Browse the repository at this point in the history
…ection (#1123)
  • Loading branch information
fisker authored Feb 22, 2021
1 parent f3b2441 commit c2c28a6
Show file tree
Hide file tree
Showing 7 changed files with 322 additions and 64 deletions.
8 changes: 3 additions & 5 deletions rules/no-array-push-push.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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,
Expand Down
19 changes: 11 additions & 8 deletions rules/prefer-ternary.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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} `,
Expand Down Expand Up @@ -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) {
Expand Down
152 changes: 152 additions & 0 deletions rules/utils/is-same-reference.js
Original file line number Diff line number Diff line change
@@ -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;
106 changes: 94 additions & 12 deletions test/no-array-push-push.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
Expand Down Expand Up @@ -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,);
Expand Down Expand Up @@ -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
}
]
});
Loading

0 comments on commit c2c28a6

Please sign in to comment.