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.

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

fix(eslint-plugin): [prefer-null-coal] fixer w/ mixed logicals #1326

Merged
merged 2 commits into from Dec 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 20 additions & 8 deletions packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
Expand Up @@ -103,24 +103,36 @@ export default util.createRule<Options, MessageIds>({
util.NullThrowsReasons.MissingToken('operator', node.type),
);

function* fix(
fixer: TSESLint.RuleFixer,
): IterableIterator<TSESLint.RuleFix> {
if (node.parent && util.isLogicalOrOperator(node.parent)) {
// '&&' and '??' operations cannot be mixed without parentheses (e.g. a && b ?? c)
if (
node.left.type === AST_NODE_TYPES.LogicalExpression &&
!util.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, '??');
}

const fixer =
isMixedLogical || forceSuggestionFixer
? // suggestion instead for cases where we aren't sure if the fixer is completely safe
({
suggest: [
{
messageId: 'preferNullish',
fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix {
return fixer.replaceText(barBarOperator, '??');
},
fix,
},
],
} as const)
: {
fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix {
return fixer.replaceText(barBarOperator, '??');
},
};
: { fix };

context.report({
node: barBarOperator,
Expand Down
14 changes: 12 additions & 2 deletions packages/eslint-plugin/src/util/astUtils.ts
@@ -1,7 +1,7 @@
import {
TSESTree,
AST_TOKEN_TYPES,
AST_NODE_TYPES,
AST_TOKEN_TYPES,
TSESTree,
} from '@typescript-eslint/experimental-utils';

const LINEBREAK_MATCHER = /\r\n|[\r\n\u2028\u2029]/;
Expand Down Expand Up @@ -42,6 +42,15 @@ function isOptionalOptionalChain(
);
}

/**
* Returns true if and only if the node represents logical OR
*/
function isLogicalOrOperator(node: TSESTree.Node): boolean {
armano2 marked this conversation as resolved.
Show resolved Hide resolved
return (
node.type === AST_NODE_TYPES.LogicalExpression && node.operator === '||'
);
}

/**
* Determines whether two adjacent tokens are on the same line
*/
Expand All @@ -59,5 +68,6 @@ export {
isOptionalChainPunctuator,
isOptionalOptionalChain,
isTokenOnSameLine,
isLogicalOrOperator,
LINEBREAK_MATCHER,
};
Expand Up @@ -317,7 +317,7 @@ declare const a: ${type} | ${nullish};
declare const b: ${type} | ${nullish};
declare const c: ${type} | ${nullish};
declare const d: ${type} | ${nullish};
a ?? b || c && d;
dimabory marked this conversation as resolved.
Show resolved Hide resolved
(a ?? b) || c && d;
`.trimRight(),
},
],
Expand Down Expand Up @@ -367,7 +367,7 @@ declare const a: ${type} | ${nullish};
declare const b: ${type} | ${nullish};
declare const c: ${type} | ${nullish};
declare const d: ${type} | ${nullish};
a && b ?? c || d;
dimabory marked this conversation as resolved.
Show resolved Hide resolved
a && (b ?? c) || d;
`.trimRight(),
},
],
Expand Down Expand Up @@ -463,5 +463,30 @@ x ?? 'foo';
},
],
},

// https://github.com/typescript-eslint/typescript-eslint/issues/1290
...nullishTypeInvalidTest((nullish, type) => ({
code: `
declare const a: ${type} | ${nullish};
declare const b: ${type};
declare const c: ${type};
a || b || c;
`.trimRight(),
output: `
declare const a: ${type} | ${nullish};
declare const b: ${type};
declare const c: ${type};
(a ?? b) || c;
`.trimRight(),
errors: [
{
messageId: 'preferNullish',
line: 5,
column: 3,
endLine: 5,
endColumn: 5,
},
],
})),
],
});