Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support autofix of numerical property access and ternary expressions in no-get rule #1865

Merged
merged 7 commits into from
May 18, 2023
Merged

Support autofix of numerical property access and ternary expressions in no-get rule #1865

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9a88fa6
Fix numerical and identifier args with get
May 16, 2023
c49e954
Clarified comment verbiage
May 16, 2023
481272c
Remove identifier
May 17, 2023
bf1b091
Add test cases for condiitonal and logical expressions
May 18, 2023
f2957c8
Autofix ternary expressions with literal consequent and alternate
May 18, 2023
81dc15e
Add test cases
May 18, 2023
85c5e3d
Remove number check from types
May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
225 changes: 182 additions & 43 deletions lib/rules/no-get.js
View file
Open in desktop
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,88 @@ 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 &&
types.isNumber(node.arguments[0])
) {
// 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) &&
types.isNumber(node.arguments[1])
) {
// 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 +453,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}`;
}
5 changes: 5 additions & 0 deletions lib/utils/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ module.exports = {
isMemberExpression,
isMethodDefinition,
isNewExpression,
isNumber,
isObjectExpression,
isObjectPattern,
isOptionalCallExpression,
Expand Down Expand Up @@ -399,6 +400,10 @@ function isStringLiteral(node) {
return isLiteral(node) && typeof node.value === 'string';
}

function isNumber(node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid adding trivial helpers like this per the comment at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed trivial helper

return node !== undefined && typeof node.value === 'number';
}

/**
* Check whether or not a node is a TaggedTemplateExpression.
*
Expand Down