Skip to content

Commit

Permalink
Support autofix of numerical property access and ternary expressions …
Browse files Browse the repository at this point in the history 
…in `no-get` rule (#1865)

* Fix numerical and identifier args with get

* Clarified comment verbiage

* Remove identifier

* Add test cases for condiitonal and logical expressions

* Autofix ternary expressions with literal consequent and alternate

* Add test cases

* Remove number check from types

---------

Co-authored-by: Evan Jehl <ejehl@ejehl-mn1.linkedin.biz>
  • Loading branch information
@evanjehl
evanjehl and Evan Jehl committed May 18, 2023
1 parent e1b5d6c commit a8efed7
Show file tree
Hide file tree
Showing 2 changed files with 366 additions and 46 deletions.
227 changes: 184 additions & 43 deletions lib/rules/no-get.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function isValidJSPath(str) {
return str.split('.').every((part) => isValidJSVariableName(part) || isValidJSArrayIndex(part));
}

function reportGet({ node, context, path, useOptionalChaining, objectText }) {
function reportGet({ node, context, path, useOptionalChaining, objectText, sourceCode }) {
const isInLeftSideOfAssignmentExpression = utils.isInLeftSideOfAssignmentExpression(node);
context.report({
node,
Expand All @@ -36,6 +36,7 @@ function reportGet({ node, context, path, useOptionalChaining, objectText }) {
useOptionalChaining,
isInLeftSideOfAssignmentExpression,
objectText,
sourceCode,
});
},
});
Expand All @@ -48,61 +49,60 @@ function fixGet({
useOptionalChaining,
isInLeftSideOfAssignmentExpression,
objectText,
sourceCode,
}) {
// Add parenthesis around the object text in case of something like this: get(foo || {}, 'bar')
const objectTextSafe = isValidJSPath(objectText) ? objectText : `(${objectText})`;

const getResultIsChained = node.parent.type === 'MemberExpression' && node.parent.object === node;

// If the result of get is chained, we can safely autofix nests paths without using optional chaining.
// In the left side of an assignment, we can safely autofix nested paths without using optional chaining.
const shouldIgnoreOptionalChaining = getResultIsChained || isInLeftSideOfAssignmentExpression;

if (path.includes('.') && !useOptionalChaining && !shouldIgnoreOptionalChaining) {
// Not safe to autofix nested properties because some properties in the path might be null or undefined.
return null;
}

if (!isValidJSPath(path)) {
// Do not autofix since the path would not be a valid JS path.
return null;
}

if (path.match(/lastObject/g)?.length > 1) {
// Do not autofix when multiple `lastObject` are chained.
return null;
}
if (types.isConditionalExpression(path)) {
const newConsequentExpression = convertLiteralTypePath({
path: path.consequent.value,
useOptionalChaining,
shouldIgnoreOptionalChaining,
objectText,
});
const newAlternateExpression = convertLiteralTypePath({
path: path.alternate.value,
useOptionalChaining,
shouldIgnoreOptionalChaining,
objectText,
});

// this means the overall expression can't be fixed
if (newConsequentExpression === null || newAlternateExpression === null) {
return null;
}

let replacementPath = shouldIgnoreOptionalChaining ? path : path.replace(/\./g, '?.');
let replacementText = `${sourceCode.getText(
path.test
)} ? ${objectTextSafe}${newConsequentExpression} : ${objectTextSafe}${newAlternateExpression}`;

// Replace any array element access (foo.1 => foo[1] or foo?.[1]).
replacementPath = replacementPath
.replace(/\.(\d+)/g, shouldIgnoreOptionalChaining ? '[$1]' : '.[$1]') // Usages in middle of path.
.replace(/^(\d+)\??\./, shouldIgnoreOptionalChaining ? '[$1].' : '[$1]?.') // Usage at beginning of path.
.replace(/^(\d+)$/, '[$1]'); // Usage as entire string.
if (shouldIgnoreOptionalChaining) {
replacementText = `(${replacementText})`;
}

// Replace any array element access using `firstObject` and `lastObject` (foo.firstObject => foo[0] or foo?.[0]).
replacementPath = replacementPath
.replace(/\.firstObject/g, shouldIgnoreOptionalChaining ? '[0]' : '.[0]') // When `firstObject` is used in the middle of the path. e.g. foo.firstObject
.replace(/^firstObject\??\./, shouldIgnoreOptionalChaining ? '[0].' : '[0]?.') // When `firstObject` is used at the beginning of the path. e.g. firstObject.bar
.replace(/^firstObject$/, '[0]') // When `firstObject` is used as the entire path.
.replace(
/\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
(_, offset) =>
`${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
0,
offset
)}.length - 1]`
)
.replace(
/^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
`[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
)
.replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.
return fixer.replaceText(node, replacementText);
}

// Add parenthesis around the object text in case of something like this: get(foo || {}, 'bar')
const objectTextSafe = isValidJSPath(objectText) ? objectText : `(${objectText})`;
const replacementPath = convertLiteralTypePath({
path,
useOptionalChaining,
shouldIgnoreOptionalChaining,
objectText,
});

const objectPathSeparator = replacementPath.startsWith('[') ? '' : '.';
// null means it can't be fixed
if (replacementPath === null) {
return null;
}

return fixer.replaceText(node, `${objectTextSafe}${objectPathSeparator}${replacementPath}`);
return fixer.replaceText(node, `${objectTextSafe}${replacementPath}`);
}

function reportGetProperties({ context, node, objectText, properties }) {
Expand Down Expand Up @@ -319,6 +319,90 @@ module.exports = {
});
}

if (
types.isMemberExpression(node.callee) &&
(types.isThisExpression(node.callee.object) || catchUnsafeObjects) &&
types.isIdentifier(node.callee.property) &&
node.callee.property.name === 'get' &&
node.arguments.length === 1 &&
node.arguments[0].type === 'Literal' &&
typeof node.arguments[0].value === 'number'
) {
// Example: this.get(5);
const sourceCode = context.getSourceCode();
reportGet({
node,
context,
path: node.arguments[0].value,
isImportedGet: false,
objectText: sourceCode.getText(node.callee.object),
});
}

if (
types.isIdentifier(node.callee) &&
node.callee.name === importedGetName &&
node.arguments.length === 2 &&
(types.isThisExpression(node.arguments[0]) || catchSafeObjects) &&
node.arguments[1].type === 'Literal' &&
typeof node.arguments[1].value === 'number'
) {
// Example: get(this, 5);
const sourceCode = context.getSourceCode();
reportGet({
node,
context,
path: node.arguments[1].value,
isImportedGet: true,
objectText: sourceCode.getText(node.arguments[0]),
});
}

if (
types.isMemberExpression(node.callee) &&
(types.isThisExpression(node.callee.object) || catchUnsafeObjects) &&
types.isIdentifier(node.callee.property) &&
node.callee.property.name === 'get' &&
node.arguments.length === 1 &&
types.isConditionalExpression(node.arguments[0]) &&
types.isLiteral(node.arguments[0].consequent) &&
types.isLiteral(node.arguments[0].alternate)
) {
// Example: this.get(foo ? 'bar' : 'baz');
const sourceCode = context.getSourceCode();
reportGet({
node,
context,
path: node.arguments[0],
isImportedGet: false,
objectText: sourceCode.getText(node.callee.object),
useOptionalChaining,
sourceCode,
});
}

if (
types.isIdentifier(node.callee) &&
node.callee.name === importedGetName &&
node.arguments.length === 2 &&
(types.isThisExpression(node.arguments[0]) || catchSafeObjects) &&
types.isConditionalExpression(node.arguments[1]) &&
types.isLiteral(node.arguments[1].consequent) &&
types.isLiteral(node.arguments[1].alternate)
) {
// Example: get(foo, bar ? 'baz' : 'biz');
const sourceCode = context.getSourceCode();
reportGet({
node,
context,
path: node.arguments[1],
isImportedGet: true,
objectText: sourceCode.getText(node.arguments[0]),
useOptionalChaining,
sourceCode,
});
}

// **************************
// getProperties
// **************************
Expand Down Expand Up @@ -371,3 +455,60 @@ function validateGetPropertiesArguments(args, ignoreNestedPaths) {
types.isStringLiteral(argument) && (!argument.value.includes('.') || !ignoreNestedPaths)
);
}

function convertLiteralTypePath({
path,
useOptionalChaining,
shouldIgnoreOptionalChaining,
objectText,
}) {
if (typeof path === 'number') {
return `[${path}]`;
}

if (path.includes('.') && !useOptionalChaining && !shouldIgnoreOptionalChaining) {
// Not safe to autofix nested properties because some properties in the path might be null or undefined.
return null;
}

if (!isValidJSPath(path)) {
// Do not autofix since the path would not be a valid JS path.
return null;
}

if (path.match(/lastObject/g)?.length > 1) {
// Do not autofix when multiple `lastObject` are chained.
return null;
}

let replacementPath = shouldIgnoreOptionalChaining ? path : path.replace(/\./g, '?.');

// Replace any array element access (foo.1 => foo[1] or foo?.[1]).
replacementPath = replacementPath
.replace(/\.(\d+)/g, shouldIgnoreOptionalChaining ? '[$1]' : '.[$1]') // Usages in middle of path.
.replace(/^(\d+)\??\./, shouldIgnoreOptionalChaining ? '[$1].' : '[$1]?.') // Usage at beginning of path.
.replace(/^(\d+)$/, '[$1]'); // Usage as entire string.

// Replace any array element access using `firstObject` and `lastObject` (foo.firstObject => foo[0] or foo?.[0]).
replacementPath = replacementPath
.replace(/\.firstObject/g, shouldIgnoreOptionalChaining ? '[0]' : '.[0]') // When `firstObject` is used in the middle of the path. e.g. foo.firstObject
.replace(/^firstObject\??\./, shouldIgnoreOptionalChaining ? '[0].' : '[0]?.') // When `firstObject` is used at the beginning of the path. e.g. firstObject.bar
.replace(/^firstObject$/, '[0]') // When `firstObject` is used as the entire path.
.replace(
/\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
(_, offset) =>
`${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
0,
offset
)}.length - 1]`
)
.replace(
/^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
`[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
)
.replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.

const objectPathSeparator = replacementPath.startsWith('[') ? '' : '.';

return `${objectPathSeparator}${replacementPath}`;
}

0 comments on commit a8efed7

Please sign in to comment.