Skip to content

fix(eslint-plugin): [prefer-nullish-coalescing] report on chain expressions in a ternary #10708

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.

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
200e6e2
handle ChainExpression in preferNullishOverTernary
OlivierZal Jan 24, 2025
d0a06a7
simplify
OlivierZal Jan 27, 2025
58eabb3
naming
OlivierZal Jan 27, 2025
5af7ed9
Move block which comes too early
OlivierZal Jan 28, 2025
dd90ef8
add tests
OlivierZal Feb 4, 2025
bc3ff36
handle use case where 2 conditions
OlivierZal Feb 4, 2025
6a207b6
renaming
OlivierZal Feb 4, 2025
3ec45e5
Merge branch 'main' into prefer-nullish-coalescing-10531
OlivierZal Feb 5, 2025
aee2f23
changes after review
OlivierZal Feb 13, 2025
f8cc815
Merge branch 'main' into prefer-nullish-coalescing-10531
OlivierZal Feb 13, 2025
833b967
use new API
OlivierZal Feb 16, 2025
628c69d
move skipChainExpression to utils
OlivierZal Feb 16, 2025
3c14cf6
use skipChainExpression util for no-floating-promises as well
OlivierZal Feb 16, 2025
3b6430a
Merge branch 'main' into prefer-nullish-coalescing-10531
OlivierZal Feb 16, 2025
201e196
use skipChainExpression wherever it's possible
OlivierZal Feb 17, 2025
0318385
move line
OlivierZal Feb 17, 2025
259a4a1
simplify
OlivierZal Feb 17, 2025
97fa249
add tests
OlivierZal Feb 18, 2025
1eba720
add symetric node equivalence
OlivierZal Feb 18, 2025
23d52a5
simplify
OlivierZal Feb 18, 2025
53dd427
simplify
OlivierZal Feb 18, 2025
7114958
Simplify
OlivierZal Feb 18, 2025
b5e3fcd
Merge branch 'main' into prefer-nullish-coalescing-10531
OlivierZal Feb 18, 2025
8eed250
Typo
OlivierZal Feb 18, 2025
a1674cf
Simplify
OlivierZal Feb 18, 2025
b31b29e
renaming and adding JS doc
OlivierZal Feb 20, 2025
c131cad
simplify js doc
OlivierZal Feb 20, 2025
7970bfa
Merge branch 'main' into prefer-nullish-coalescing-10531
OlivierZal Feb 22, 2025
2c2f3ec
Merge branch 'main' into prefer-nullish-coalescing-10531
JoshuaKGoldberg Feb 24, 2025
e800c2a
Merge branch 'main' into prefer-nullish-coalescing-10531
OlivierZal Feb 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions packages/eslint-plugin/src/rules/no-floating-promises.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
OperatorPrecedence,
readonlynessOptionsDefaults,
readonlynessOptionsSchema,
skipChainExpression,
typeMatchesSomeSpecifier,
} from '../util';

Expand Down Expand Up @@ -135,11 +136,7 @@ export default createRule<Options, MessageId>({
return;
}

let expression = node.expression;

if (expression.type === AST_NODE_TYPES.ChainExpression) {
expression = expression.expression;
}
const expression = skipChainExpression(node.expression);

if (isKnownSafePromiseReturn(expression)) {
return;
Expand Down
17 changes: 10 additions & 7 deletions packages/eslint-plugin/src/rules/no-inferrable-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ import type { TSESTree } from '@typescript-eslint/utils';

import { AST_NODE_TYPES } from '@typescript-eslint/utils';

import { createRule, nullThrows, NullThrowsReasons } from '../util';
import {
createRule,
nullThrows,
NullThrowsReasons,
skipChainExpression,
} from '../util';

export type Options = [
{
Expand Down Expand Up @@ -55,14 +60,12 @@ export default createRule<Options, MessageIds>({
init: TSESTree.Expression,
callName: string,
): boolean {
if (init.type === AST_NODE_TYPES.ChainExpression) {
return isFunctionCall(init.expression, callName);
}
const node = skipChainExpression(init);

return (
init.type === AST_NODE_TYPES.CallExpression &&
init.callee.type === AST_NODE_TYPES.Identifier &&
init.callee.name === callName
node.type === AST_NODE_TYPES.CallExpression &&
node.callee.type === AST_NODE_TYPES.Identifier &&
node.callee.name === callName
);
}
function isLiteral(init: TSESTree.Expression, typeName: string): boolean {
Expand Down
28 changes: 10 additions & 18 deletions packages/eslint-plugin/src/rules/prefer-find.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
getStaticValue,
isStaticMemberAccessOfValue,
nullThrows,
skipChainExpression,
} from '../util';

export default createRule({
Expand Down Expand Up @@ -47,32 +48,26 @@ export default createRule({
function parseArrayFilterExpressions(
expression: TSESTree.Expression,
): FilterExpressionData[] {
if (expression.type === AST_NODE_TYPES.SequenceExpression) {
const node = skipChainExpression(expression);

if (node.type === AST_NODE_TYPES.SequenceExpression) {
// Only the last expression in (a, b, [1, 2, 3].filter(condition))[0] matters
const lastExpression = nullThrows(
expression.expressions.at(-1),
node.expressions.at(-1),
'Expected to have more than zero expressions in a sequence expression',
);
return parseArrayFilterExpressions(lastExpression);
}

if (expression.type === AST_NODE_TYPES.ChainExpression) {
return parseArrayFilterExpressions(expression.expression);
}

// This is the only reason we're returning a list rather than a single value.
if (expression.type === AST_NODE_TYPES.ConditionalExpression) {
if (node.type === AST_NODE_TYPES.ConditionalExpression) {
// Both branches of the ternary _must_ return results.
const consequentResult = parseArrayFilterExpressions(
expression.consequent,
);
const consequentResult = parseArrayFilterExpressions(node.consequent);
if (consequentResult.length === 0) {
return [];
}

const alternateResult = parseArrayFilterExpressions(
expression.alternate,
);
const alternateResult = parseArrayFilterExpressions(node.alternate);
if (alternateResult.length === 0) {
return [];
}
Expand All @@ -82,11 +77,8 @@ export default createRule({
}

// Check if it looks like <<stuff>>(...), but not <<stuff>>?.(...)
if (
expression.type === AST_NODE_TYPES.CallExpression &&
!expression.optional
) {
const callee = expression.callee;
if (node.type === AST_NODE_TYPES.CallExpression && !node.optional) {
const callee = node.callee;
// Check if it looks like <<stuff>>.filter(...) or <<stuff>>['filter'](...),
// or the optional chaining variants.
if (callee.type === AST_NODE_TYPES.MemberExpression) {
Expand Down
133 changes: 98 additions & 35 deletions packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,17 @@ import {
isUndefinedIdentifier,
nullThrows,
NullThrowsReasons,
skipChainExpression,
} from '../util';

const isIdentifierOrMemberExpression = isNodeOfTypes([
const isIdentifierOrMemberOrChainExpression = isNodeOfTypes([
AST_NODE_TYPES.ChainExpression,
AST_NODE_TYPES.Identifier,
AST_NODE_TYPES.MemberExpression,
] as const);

type NullishCheckOperator = '!' | '!=' | '!==' | '==' | '===' | undefined;

export type Options = [
{
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean;
Expand Down Expand Up @@ -166,7 +170,6 @@ export default createRule<Options, MessageIds>({
const parserServices = getParserServices(context);
const compilerOptions = parserServices.program.getCompilerOptions();

const checker = parserServices.program.getTypeChecker();
const isStrictNullChecks = tsutils.isStrictCompilerOptionEnabled(
compilerOptions,
'strictNullChecks',
Expand Down Expand Up @@ -340,7 +343,7 @@ export default createRule<Options, MessageIds>({
return;
}

let operator: '!' | '!=' | '!==' | '==' | '===' | undefined;
let operator: NullishCheckOperator;
let nodesInsideTestExpression: TSESTree.Node[] = [];
if (node.test.type === AST_NODE_TYPES.BinaryExpression) {
nodesInsideTestExpression = [node.test.left, node.test.right];
Expand Down Expand Up @@ -398,28 +401,35 @@ export default createRule<Options, MessageIds>({
}
}

let identifierOrMemberExpression: TSESTree.Node | undefined;
let nullishCoalescingLeftNode: TSESTree.Node | undefined;
let hasTruthinessCheck = false;
let hasNullCheckWithoutTruthinessCheck = false;
let hasUndefinedCheckWithoutTruthinessCheck = false;

if (!operator) {
let testNode: TSESTree.Node | undefined;
hasTruthinessCheck = true;

if (
isIdentifierOrMemberExpression(node.test) &&
isNodeEqual(node.test, node.consequent)
) {
identifierOrMemberExpression = node.test;
if (isIdentifierOrMemberOrChainExpression(node.test)) {
testNode = node.test;
} else if (
node.test.type === AST_NODE_TYPES.UnaryExpression &&
node.test.operator === '!' &&
isIdentifierOrMemberExpression(node.test.argument) &&
isNodeEqual(node.test.argument, node.alternate)
isIdentifierOrMemberOrChainExpression(node.test.argument) &&
node.test.operator === '!'
) {
identifierOrMemberExpression = node.test.argument;
testNode = node.test.argument;
operator = '!';
}

if (
testNode &&
areNodesSimilarMemberAccess(
testNode,
getBranchNodes(node, operator).nonNullishBranch,
)
) {
nullishCoalescingLeftNode = testNode;
}
} else {
// we check that the test only contains null, undefined and the identifier
for (const testNode of nodesInsideTestExpression) {
Expand All @@ -428,22 +438,25 @@ export default createRule<Options, MessageIds>({
} else if (isUndefinedIdentifier(testNode)) {
hasUndefinedCheckWithoutTruthinessCheck = true;
} else if (
(operator === '!==' || operator === '!=') &&
isNodeEqual(testNode, node.consequent)
areNodesSimilarMemberAccess(
testNode,
getBranchNodes(node, operator).nonNullishBranch,
)
) {
identifierOrMemberExpression = testNode;
} else if (
(operator === '===' || operator === '==') &&
isNodeEqual(testNode, node.alternate)
) {
identifierOrMemberExpression = testNode;
// Only consider the first expression in a multi-part nullish check,
// as subsequent expressions might not require all the optional chaining operators.
// For example: a?.b?.c !== undefined && a.b.c !== null ? a.b.c : 'foo';
// This works because `node.test` is always evaluated first in the loop
// and has the same or more necessary optional chaining operators
// than `node.alternate` or `node.consequent`.
nullishCoalescingLeftNode ??= testNode;
} else {
return;
}
}
}

if (!identifierOrMemberExpression) {
if (!nullishCoalescingLeftNode) {
return;
}

Expand All @@ -452,16 +465,10 @@ export default createRule<Options, MessageIds>({
if (hasTruthinessCheck) {
return isTruthinessCheckEligibleForPreferNullish({
node,
testNode: identifierOrMemberExpression,
testNode: nullishCoalescingLeftNode,
});
}

const tsNode = parserServices.esTreeNodeToTSNodeMap.get(
identifierOrMemberExpression,
);
const type = checker.getTypeAtLocation(tsNode);
const flags = getTypeFlags(type);

// it is fixable if we check for both null and undefined, or not if neither
if (
hasUndefinedCheckWithoutTruthinessCheck ===
Expand All @@ -475,6 +482,11 @@ export default createRule<Options, MessageIds>({
return true;
}

const type = parserServices.getTypeAtLocation(
nullishCoalescingLeftNode,
);
const flags = getTypeFlags(type);

if (flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) {
return false;
}
Expand Down Expand Up @@ -503,15 +515,11 @@ export default createRule<Options, MessageIds>({
messageId: 'suggestNullish',
data: { equals: '' },
fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix {
const [left, right] =
operator === '===' || operator === '==' || operator === '!'
? [identifierOrMemberExpression, node.consequent]
: [identifierOrMemberExpression, node.alternate];
return fixer.replaceText(
node,
`${getTextWithParentheses(context.sourceCode, left)} ?? ${getTextWithParentheses(
`${getTextWithParentheses(context.sourceCode, nullishCoalescingLeftNode)} ?? ${getTextWithParentheses(
context.sourceCode,
right,
getBranchNodes(node, operator).nullishBranch,
)}`,
);
},
Expand Down Expand Up @@ -647,3 +655,58 @@ function isMixedLogicalExpression(

return false;
}

/**
* Checks if two TSESTree nodes have the same member access sequence,
* regardless of optional chaining differences.
*
* Note: This does not imply that the nodes are runtime-equivalent.
*
* Example: `a.b.c`, `a?.b.c`, `a.b?.c`, `(a?.b).c`, `(a.b)?.c` are considered similar.
*
* @param a First TSESTree node.
* @param b Second TSESTree node.
* @returns `true` if the nodes access members in the same order; otherwise, `false`.
*/
function areNodesSimilarMemberAccess(
a: TSESTree.Node,
b: TSESTree.Node,
): boolean {
if (
a.type === AST_NODE_TYPES.MemberExpression &&
b.type === AST_NODE_TYPES.MemberExpression
) {
return (
isNodeEqual(a.property, b.property) &&
areNodesSimilarMemberAccess(a.object, b.object)
);
}
if (
a.type === AST_NODE_TYPES.ChainExpression ||
b.type === AST_NODE_TYPES.ChainExpression
) {
return areNodesSimilarMemberAccess(
skipChainExpression(a),
skipChainExpression(b),
);
}
return isNodeEqual(a, b);
}

/**
* Returns the branch nodes of a conditional expression:
* - the "nonNullish branch" is the branch when test node is not nullish
* - the "nullish branch" is the branch when test node is nullish
*/
function getBranchNodes(
node: TSESTree.ConditionalExpression,
operator: NullishCheckOperator,
): {
nonNullishBranch: TSESTree.Expression;
nullishBranch: TSESTree.Expression;
} {
if (!operator || ['!=', '!=='].includes(operator)) {
return { nonNullishBranch: node.consequent, nullishBranch: node.alternate };
}
return { nonNullishBranch: node.alternate, nullishBranch: node.consequent };
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
isPromiseLike,
isReadonlyErrorLike,
isStaticMemberAccessOfValue,
skipChainExpression,
} from '../util';

export type MessageIds = 'rejectAnError';
Expand Down Expand Up @@ -102,14 +103,6 @@ export default createRule<Options, MessageIds>({
});
}

function skipChainExpression<T extends TSESTree.Node>(
node: T,
): T | TSESTree.ChainElement {
return node.type === AST_NODE_TYPES.ChainExpression
? node.expression
: node;
}

function typeAtLocationIsLikePromise(node: TSESTree.Node): boolean {
const type = services.getTypeAtLocation(node);
return (
Expand Down
16 changes: 5 additions & 11 deletions packages/eslint-plugin/src/rules/prefer-string-starts-ends-with.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
isStaticMemberAccessOfValue,
nullThrows,
NullThrowsReasons,
skipChainExpression,
} from '../util';

const EQ_OPERATORS = /^[=!]=/;
Expand Down Expand Up @@ -306,18 +307,11 @@ export default createRule<Options, MessageIds>({
}

function getLeftNode(
node: TSESTree.Expression | TSESTree.PrivateIdentifier,
init: TSESTree.Expression | TSESTree.PrivateIdentifier,
): TSESTree.MemberExpression {
if (node.type === AST_NODE_TYPES.ChainExpression) {
return getLeftNode(node.expression);
}

let leftNode;
if (node.type === AST_NODE_TYPES.CallExpression) {
leftNode = node.callee;
} else {
leftNode = node;
}
const node = skipChainExpression(init);
const leftNode =
node.type === AST_NODE_TYPES.CallExpression ? node.callee : node;

if (leftNode.type !== AST_NODE_TYPES.MemberExpression) {
throw new Error(`Expected a MemberExpression, got ${leftNode.type}`);
Expand Down
Loading
Loading