Skip to content

feat(eslint-plugin): [prefer-nullish-coalescing] add support for assignment expressions #10152

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

Merged
merged 12 commits into from
Oct 21, 2024
Merged

feat(eslint-plugin): [prefer-nullish-coalescing] add support for assignment expressions #10152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1e3281a
extract current LogicalExpression logic into function
abrahamguo Oct 15, 2024
3d6a209
pull in other changes to rule file without edits
abrahamguo Oct 15, 2024
df78ea3
pull over genericizations for checkAssignmentOrLogicalExpression
abrahamguo Oct 15, 2024
ff5db69
lint autofix
abrahamguo Oct 15, 2024
2a705a3
port over test changes
abrahamguo Oct 15, 2024
4c3fb24
change all declare consts to declare lets
abrahamguo Oct 15, 2024
731cb4a
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into p…
abrahamguo Oct 16, 2024
52066e0
apply lint rule
abrahamguo Oct 16, 2024
da2aeb8
reduce the number of duplicate tests
abrahamguo Oct 16, 2024
de4fb70
revert incorrect unrelated changes
abrahamguo Oct 16, 2024
bc0786e
Merge branch 'main'
JoshuaKGoldberg Oct 21, 2024
22841da
fix lint complaints
JoshuaKGoldberg Oct 21, 2024
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
199 changes: 112 additions & 87 deletions packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@ export default createRule<Options, MessageIds>({
noStrictNullCheck:
'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.',
preferNullishOverOr:
'Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.',
'Prefer using nullish coalescing operator (`??{{ equals }}`) instead of a logical {{ description }} (`||{{ equals }}`), as it is a safer operator.',
preferNullishOverTernary:
'Prefer using nullish coalescing operator (`??`) instead of a ternary expression, as it is simpler to read.',
suggestNullish: 'Fix to nullish coalescing operator (`??`).',
'Prefer using nullish coalescing operator (`??{{ equals }}`) instead of a ternary expression, as it is simpler to read.',
suggestNullish: 'Fix to nullish coalescing operator (`??{{ equals }}`).',
},
schema: [
{
Expand Down Expand Up @@ -171,7 +171,107 @@ export default createRule<Options, MessageIds>({
});
}

// todo: rename to something more specific?
Copy link
Member

Choose a reason for hiding this comment

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

todo

What's the resolution here? FWIW I don't mind the name as-is.

function checkAssignmentOrLogicalExpression(
node: TSESTree.AssignmentExpression | TSESTree.LogicalExpression,
description: string,
equals: string,
): void {
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
const type = checker.getTypeAtLocation(tsNode.left);
if (!isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined)) {
return;
}

if (ignoreConditionalTests === true && isConditionalTest(node)) {
return;
}

if (
ignoreMixedLogicalExpressions === true &&
isMixedLogicalExpression(node)
) {
return;
}

// https://github.com/typescript-eslint/typescript-eslint/issues/5439
/* eslint-disable @typescript-eslint/no-non-null-assertion */
const ignorableFlags = [
(ignorePrimitives === true || ignorePrimitives!.bigint) &&
ts.TypeFlags.BigIntLike,
(ignorePrimitives === true || ignorePrimitives!.boolean) &&
ts.TypeFlags.BooleanLike,
(ignorePrimitives === true || ignorePrimitives!.number) &&
ts.TypeFlags.NumberLike,
(ignorePrimitives === true || ignorePrimitives!.string) &&
ts.TypeFlags.StringLike,
]
.filter((flag): flag is number => typeof flag === 'number')
.reduce((previous, flag) => previous | flag, 0);
if (
type.flags !== ts.TypeFlags.Null &&
type.flags !== ts.TypeFlags.Undefined &&
(type as ts.UnionOrIntersectionType).types.some(t =>
tsutils
.intersectionTypeParts(t)
.some(t => tsutils.isTypeFlagSet(t, ignorableFlags)),
)
) {
return;
}
/* eslint-enable @typescript-eslint/no-non-null-assertion */

const barBarOperator = nullThrows(
context.sourceCode.getTokenAfter(
node.left,
token =>
token.type === AST_TOKEN_TYPES.Punctuator &&
token.value === node.operator,
),
NullThrowsReasons.MissingToken('operator', node.type),
);

function* fix(
fixer: TSESLint.RuleFixer,
): IterableIterator<TSESLint.RuleFix> {
if (isLogicalOrOperator(node.parent)) {
// '&&' and '??' operations cannot be mixed without parentheses (e.g. a && b ?? c)
if (
node.left.type === AST_NODE_TYPES.LogicalExpression &&
!isLogicalOrOperator(node.left.left)
) {
yield fixer.insertTextBefore(node.left.right, '(');
} else {
yield fixer.insertTextBefore(node.left, '(');
}
yield fixer.insertTextAfter(node.right, ')');
}
yield fixer.replaceText(
barBarOperator,
node.operator.replace('||', '??'),
);
}

context.report({
node: barBarOperator,
messageId: 'preferNullishOverOr',
data: { description, equals },
suggest: [
{
messageId: 'suggestNullish',
data: { equals },
fix,
},
],
});
}

return {
'AssignmentExpression[operator = "||="]'(
node: TSESTree.AssignmentExpression,
): void {
checkAssignmentOrLogicalExpression(node, 'assignment', '=');
},
ConditionalExpression(node: TSESTree.ConditionalExpression): void {
if (ignoreTernaryTests) {
return;
Expand Down Expand Up @@ -200,7 +300,7 @@ export default createRule<Options, MessageIds>({
node.test.right.left,
node.test.right.right,
];
if (node.test.operator === '||') {
if (['||', '||='].includes(node.test.operator)) {
if (
node.test.left.operator === '===' &&
node.test.right.operator === '==='
Expand Down Expand Up @@ -304,9 +404,12 @@ export default createRule<Options, MessageIds>({
context.report({
node,
messageId: 'preferNullishOverTernary',
// TODO: also account for = in the ternary clause
data: { equals: '' },
suggest: [
{
messageId: 'suggestNullish',
data: { equals: '' },
fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix {
const [left, right] =
operator === '===' || operator === '=='
Expand All @@ -325,90 +428,10 @@ export default createRule<Options, MessageIds>({
});
}
},

'LogicalExpression[operator = "||"]'(
node: TSESTree.LogicalExpression,
): void {
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
const type = checker.getTypeAtLocation(tsNode.left);
if (!isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined)) {
return;
}

if (ignoreConditionalTests === true && isConditionalTest(node)) {
return;
}

const isMixedLogical = isMixedLogicalExpression(node);
if (ignoreMixedLogicalExpressions === true && isMixedLogical) {
return;
}

// https://github.com/typescript-eslint/typescript-eslint/issues/5439
/* eslint-disable @typescript-eslint/no-non-null-assertion */
const ignorableFlags = [
(ignorePrimitives === true || ignorePrimitives!.bigint) &&
ts.TypeFlags.BigIntLike,
(ignorePrimitives === true || ignorePrimitives!.boolean) &&
ts.TypeFlags.BooleanLike,
(ignorePrimitives === true || ignorePrimitives!.number) &&
ts.TypeFlags.NumberLike,
(ignorePrimitives === true || ignorePrimitives!.string) &&
ts.TypeFlags.StringLike,
]
.filter((flag): flag is number => typeof flag === 'number')
.reduce((previous, flag) => previous | flag, 0);
if (
type.flags !== ts.TypeFlags.Null &&
type.flags !== ts.TypeFlags.Undefined &&
(type as ts.UnionOrIntersectionType).types.some(t =>
tsutils
.intersectionTypeParts(t)
.some(t => tsutils.isTypeFlagSet(t, ignorableFlags)),
)
) {
return;
}
/* eslint-enable @typescript-eslint/no-non-null-assertion */

const barBarOperator = nullThrows(
context.sourceCode.getTokenAfter(
node.left,
token =>
token.type === AST_TOKEN_TYPES.Punctuator &&
token.value === node.operator,
),
NullThrowsReasons.MissingToken('operator', node.type),
);

function* fix(
fixer: TSESLint.RuleFixer,
): IterableIterator<TSESLint.RuleFix> {
if (isLogicalOrOperator(node.parent)) {
// '&&' and '??' operations cannot be mixed without parentheses (e.g. a && b ?? c)
if (
node.left.type === AST_NODE_TYPES.LogicalExpression &&
!isLogicalOrOperator(node.left.left)
) {
yield fixer.insertTextBefore(node.left.right, '(');
} else {
yield fixer.insertTextBefore(node.left, '(');
}
yield fixer.insertTextAfter(node.right, ')');
}
yield fixer.replaceText(barBarOperator, '??');
}

context.report({
node: barBarOperator,
messageId: 'preferNullishOverOr',
suggest: [
{
messageId: 'suggestNullish',
fix,
},
],
});
checkAssignmentOrLogicalExpression(node, 'or', '');
},
};
},
Expand Down Expand Up @@ -451,7 +474,9 @@ function isConditionalTest(node: TSESTree.Node): boolean {
return false;
}

function isMixedLogicalExpression(node: TSESTree.LogicalExpression): boolean {
function isMixedLogicalExpression(
node: TSESTree.AssignmentExpression | TSESTree.LogicalExpression,
): boolean {
const seen = new Set<TSESTree.Node | undefined>();
const queue = [node.parent, node.left, node.right];
for (const current of queue) {
Expand All @@ -463,7 +488,7 @@ function isMixedLogicalExpression(node: TSESTree.LogicalExpression): boolean {
if (current.type === AST_NODE_TYPES.LogicalExpression) {
if (current.operator === '&&') {
return true;
} else if (current.operator === '||') {
} else if (['||', '||='].includes(current.operator)) {
// check the pieces of the node to catch cases like `a || b || c && d`
queue.push(current.parent, current.left, current.right);
}
Expand Down
Loading
Loading