Skip to content

Commit

Permalink
fix(eslint-plugin): [prefer-optional-chain] handle more cases (#1261)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bradzacher
bradzacher committed Nov 26, 2019
1 parent 6daff10 commit 57ddba3
Show file tree
Hide file tree
Showing 5 changed files with 444 additions and 139 deletions.
255 changes: 225 additions & 30 deletions packages/eslint-plugin/src/rules/prefer-optional-chain.ts
View file
Expand Up @@ -2,9 +2,17 @@ import {
AST_NODE_TYPES,
TSESTree,
} from '@typescript-eslint/experimental-utils';
import { isOpeningParenToken } from 'eslint-utils';
import * as util from '../util';

const WHITESPACE_REGEX = /\s/g;
type ValidChainTarget =
| TSESTree.BinaryExpression
| TSESTree.CallExpression
| TSESTree.MemberExpression
| TSESTree.OptionalCallExpression
| TSESTree.OptionalMemberExpression
| TSESTree.Identifier
| TSESTree.ThisExpression;

/*
The AST is always constructed such the first element is always the deepest element.
Expand Down Expand Up @@ -70,19 +78,36 @@ export default util.createRule({
let optionallyChainedCode = previousLeftText;
let expressionCount = 1;
while (current.type === AST_NODE_TYPES.LogicalExpression) {
if (!isValidChainTarget(current.right)) {
if (
!isValidChainTarget(
current.right,
// only allow identifiers for the first chain - foo && foo()
expressionCount === 1,
)
) {
break;
}

const leftText = previousLeftText;
const rightText = getText(current.right);
if (!rightText.startsWith(leftText)) {
// can't just use startsWith because of cases like foo && fooBar.baz;
const matchRegex = new RegExp(
`^${
// escape regex characters
leftText.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
}[^a-zA-Z0-9_]`,
);
if (
!matchRegex.test(rightText) &&
// handle redundant cases like foo.bar && foo.bar
leftText !== rightText
) {
break;
}
expressionCount += 1;

// omit weird doubled up expression that make no sense like foo.bar && foo.bar
if (rightText !== leftText) {
expressionCount += 1;
previousLeftText = rightText;

/*
Expand All @@ -108,21 +133,37 @@ export default util.createRule({
rightText === 'foo.bar.baz[buzz]'
leftText === 'foo.bar.baz'
diff === '[buzz]'
5)
rightText === 'foo.bar.baz?.buzz'
leftText === 'foo.bar.baz'
diff === '?.buzz'
*/
const diff = rightText.replace(leftText, '');
const needsDot = diff.startsWith('(') || diff.startsWith('[');
optionallyChainedCode += `?${needsDot ? '.' : ''}${diff}`;
if (diff.startsWith('?')) {
// item was "pre optional chained"
optionallyChainedCode += diff;
} else {
const needsDot = diff.startsWith('(') || diff.startsWith('[');
optionallyChainedCode += `?${needsDot ? '.' : ''}${diff}`;
}
}

/* istanbul ignore if: this shouldn't ever happen, but types */
if (!current.parent) {
break;
}
previous = current;
current = current.parent;
current = util.nullThrows(
current.parent,
util.NullThrowsReasons.MissingParent,
);
}

if (expressionCount > 1) {
if (previous.right.type === AST_NODE_TYPES.BinaryExpression) {
// case like foo && foo.bar !== someValue
optionallyChainedCode += ` ${
previous.right.operator
} ${sourceCode.getText(previous.right.right)}`;
}

context.report({
node: previous,
messageId: 'preferOptionalChain',
Expand All @@ -134,37 +175,191 @@ export default util.createRule({
},
};

function getText(
node:
| TSESTree.BinaryExpression
| TSESTree.CallExpression
| TSESTree.Identifier
| TSESTree.MemberExpression,
function getText(node: ValidChainTarget): string {
if (node.type === AST_NODE_TYPES.BinaryExpression) {
return getText(
// isValidChainTarget ensures this is type safe
node.left as ValidChainTarget,
);
}

if (
node.type === AST_NODE_TYPES.CallExpression ||
node.type === AST_NODE_TYPES.OptionalCallExpression
) {
const calleeText = getText(
// isValidChainTarget ensures this is type safe
node.callee as ValidChainTarget,
);

// ensure that the call arguments are left untouched, or else we can break cases that _need_ whitespace:
// - JSX: <Foo Needs Space Between Attrs />
// - Unary Operators: typeof foo, await bar, delete baz
const closingParenToken = util.nullThrows(
sourceCode.getLastToken(node),
util.NullThrowsReasons.MissingToken('closing parenthesis', node.type),
);
const openingParenToken = util.nullThrows(
sourceCode.getFirstTokenBetween(
node.callee,
closingParenToken,
isOpeningParenToken,
),
util.NullThrowsReasons.MissingToken('opening parenthesis', node.type),
);

const argumentsText = sourceCode.text.substring(
openingParenToken.range[0],
closingParenToken.range[1],
);

return `${calleeText}${argumentsText}`;
}

if (node.type === AST_NODE_TYPES.Identifier) {
return node.name;
}

if (node.type === AST_NODE_TYPES.ThisExpression) {
return 'this';
}

return getMemberExpressionText(node);
}

/**
* Gets a normalised representation of the given MemberExpression
*/
function getMemberExpressionText(
node: TSESTree.MemberExpression | TSESTree.OptionalMemberExpression,
): string {
const text = sourceCode.getText(
node.type === AST_NODE_TYPES.BinaryExpression ? node.left : node,
);
let objectText: string;

// cases should match the list in ALLOWED_MEMBER_OBJECT_TYPES
switch (node.object.type) {
case AST_NODE_TYPES.CallExpression:
case AST_NODE_TYPES.OptionalCallExpression:
case AST_NODE_TYPES.Identifier:
objectText = getText(node.object);
break;

case AST_NODE_TYPES.MemberExpression:
case AST_NODE_TYPES.OptionalMemberExpression:
objectText = getMemberExpressionText(node.object);
break;

case AST_NODE_TYPES.ThisExpression:
objectText = getText(node.object);
break;

/* istanbul ignore next */
default:
throw new Error(`Unexpected member object type: ${node.object.type}`);
}

let propertyText: string;
if (node.computed) {
// cases should match the list in ALLOWED_COMPUTED_PROP_TYPES
switch (node.property.type) {
case AST_NODE_TYPES.Identifier:
propertyText = getText(node.property);
break;

// Removes spaces from the source code for the given node
return text.replace(WHITESPACE_REGEX, '');
case AST_NODE_TYPES.Literal:
case AST_NODE_TYPES.BigIntLiteral:
case AST_NODE_TYPES.TemplateLiteral:
propertyText = sourceCode.getText(node.property);
break;

case AST_NODE_TYPES.MemberExpression:
case AST_NODE_TYPES.OptionalMemberExpression:
propertyText = getMemberExpressionText(node.property);
break;

/* istanbul ignore next */
default:
throw new Error(
`Unexpected member property type: ${node.object.type}`,
);
}

return `${objectText}${node.optional ? '?.' : ''}[${propertyText}]`;
} else {
// cases should match the list in ALLOWED_NON_COMPUTED_PROP_TYPES
switch (node.property.type) {
case AST_NODE_TYPES.Identifier:
propertyText = getText(node.property);
break;

/* istanbul ignore next */
default:
throw new Error(
`Unexpected member property type: ${node.object.type}`,
);
}

return `${objectText}${node.optional ? '?.' : '.'}${propertyText}`;
}
}
},
});

const ALLOWED_MEMBER_OBJECT_TYPES: ReadonlySet<AST_NODE_TYPES> = new Set([
AST_NODE_TYPES.CallExpression,
AST_NODE_TYPES.Identifier,
AST_NODE_TYPES.MemberExpression,
AST_NODE_TYPES.OptionalCallExpression,
AST_NODE_TYPES.OptionalMemberExpression,
AST_NODE_TYPES.ThisExpression,
]);
const ALLOWED_COMPUTED_PROP_TYPES: ReadonlySet<AST_NODE_TYPES> = new Set([
AST_NODE_TYPES.BigIntLiteral,
AST_NODE_TYPES.Identifier,
AST_NODE_TYPES.Literal,
AST_NODE_TYPES.MemberExpression,
AST_NODE_TYPES.OptionalMemberExpression,
AST_NODE_TYPES.TemplateLiteral,
]);
const ALLOWED_NON_COMPUTED_PROP_TYPES: ReadonlySet<AST_NODE_TYPES> = new Set([
AST_NODE_TYPES.Identifier,
]);

function isValidChainTarget(
node: TSESTree.Node,
allowIdentifier = false,
): node is
| TSESTree.BinaryExpression
| TSESTree.CallExpression
| TSESTree.MemberExpression {
allowIdentifier: boolean,
): node is ValidChainTarget {
if (
node.type === AST_NODE_TYPES.MemberExpression ||
node.type === AST_NODE_TYPES.CallExpression
node.type === AST_NODE_TYPES.OptionalMemberExpression
) {
return true;
const isObjectValid =
ALLOWED_MEMBER_OBJECT_TYPES.has(node.object.type) &&
// make sure to validate the expression is of our expected structure
isValidChainTarget(node.object, true);
const isPropertyValid = node.computed
? ALLOWED_COMPUTED_PROP_TYPES.has(node.property.type) &&
// make sure to validate the member expression is of our expected structure
(node.property.type === AST_NODE_TYPES.MemberExpression ||
node.property.type === AST_NODE_TYPES.OptionalMemberExpression
? isValidChainTarget(node.property, allowIdentifier)
: true)
: ALLOWED_NON_COMPUTED_PROP_TYPES.has(node.property.type);

return isObjectValid && isPropertyValid;
}
if (allowIdentifier && node.type === AST_NODE_TYPES.Identifier) {

if (
node.type === AST_NODE_TYPES.CallExpression ||
node.type === AST_NODE_TYPES.OptionalCallExpression
) {
return isValidChainTarget(node.callee, allowIdentifier);
}

if (
allowIdentifier &&
(node.type === AST_NODE_TYPES.Identifier ||
node.type === AST_NODE_TYPES.ThisExpression)
) {
return true;
}

Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/util/index.ts
View file
Expand Up @@ -4,6 +4,7 @@ export * from './astUtils';
export * from './createRule';
export * from './getParserServices';
export * from './misc';
export * from './nullThrows';
export * from './types';

// this is done for convenience - saves migrating all of the old rules
Expand Down

0 comments on commit 57ddba3

Please sign in to comment.