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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint-plugin): [prefer-nullish-coalescing] ignoreStrings option #1569

Closed
wants to merge 2 commits into from
Closed
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
36 changes: 36 additions & 0 deletions packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ type Options = [
{
ignoreConditionalTests?: boolean;
ignoreMixedLogicalExpressions?: boolean;
ignoreStrings?: boolean;
forceSuggestionFixer?: boolean;
},
];
Expand All @@ -54,6 +55,7 @@ const defaultOptions = [
{
ignoreConditionalTests: true,
ignoreMixedLogicalExpressions: true,
ignoreStrings: false,
forceSuggestionFixer: false,
},
];
Expand Down Expand Up @@ -133,6 +135,40 @@ a ?? (b && c && d);

**_NOTE:_** Errors for this specific case will be presented as suggestions (see below), instead of fixes. This is because it is not always safe to automatically convert `||` to `??` within a mixed logical expression, as we cannot tell the intended precedence of the operator. Note that by design, `??` requires parentheses when used with `&&` or `||` in the same expression.

### `ignoreStrings`

Setting this option to `true` will cause the rule to be ignored when the left-hand's TypeScript type is `string`, whether it may be nullish or not.

This can be useful if you want to enforce the use of `??` on every types but `string`, because you want to handle empty strings.

Incorrect code for `ignoreStrings: false`, and correct code for `ignoreStrings: true`:

```ts
declare const nullString: string | null;
const emptyString = '';

const a = nullString || 'fallback';
const b = emptyString || 'fallback';

// a === 'fallback'
// b === 'fallback'
```

Correct code for `ignoreStrings: false`:

```ts
declare const nullString: string | null;
const emptyString = '';

const a = nullString ?? 'fallback';
const b = emptyString ?? 'fallback';
const c = !emptyString ? 'fallback' : emptyString;

// a === 'fallback'
// b === ''
// c === 'fallback'
```

### `forceSuggestionFixer`

Setting this option to `true` will cause the rule to use ESLint's "suggested fix" mode for all fixes. _This option is provided as to aid in transitioning your codebase onto this rule_.
Expand Down
20 changes: 20 additions & 0 deletions packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import {
TSESTree,
} from '@typescript-eslint/experimental-utils';
import * as util from '../util';
import * as ts from 'typescript';

export type Options = [
{
ignoreConditionalTests?: boolean;
ignoreMixedLogicalExpressions?: boolean;
ignoreStrings?: boolean;
forceSuggestionFixer?: boolean;
},
];
Expand Down Expand Up @@ -41,6 +43,9 @@ export default util.createRule<Options, MessageIds>({
ignoreMixedLogicalExpressions: {
type: 'boolean',
},
ignoreStrings: {
type: 'boolean',
},
forceSuggestionFixer: {
type: 'boolean',
},
Expand All @@ -53,6 +58,7 @@ export default util.createRule<Options, MessageIds>({
{
ignoreConditionalTests: true,
ignoreMixedLogicalExpressions: true,
ignoreStrings: false,
forceSuggestionFixer: false,
},
],
Expand All @@ -62,6 +68,7 @@ export default util.createRule<Options, MessageIds>({
{
ignoreConditionalTests,
ignoreMixedLogicalExpressions,
ignoreStrings,
forceSuggestionFixer,
},
],
Expand Down Expand Up @@ -90,6 +97,10 @@ export default util.createRule<Options, MessageIds>({
return;
}

if (ignoreStrings === true && isStringOrNullish(checker, type)) {
return;
}

const barBarOperator = util.nullThrows(
sourceCode.getTokenAfter(
node.left,
Expand Down Expand Up @@ -199,3 +210,12 @@ function isMixedLogicalExpression(node: TSESTree.LogicalExpression): boolean {

return false;
}

function isStringOrNullish(checker: ts.TypeChecker, type: ts.Type): boolean {
return util
.getTypeName(checker, type)
.split(' ')
.every(resolvedType =>
['string', 'undefined', 'null', '|', '&'].includes(resolvedType),
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ const ruleTester = new RuleTester({
},
});

const types = ['string', 'number', 'boolean', 'object'];
const allTypesButString = ['number', 'boolean', 'object'];
const types = ['string', ...allTypesButString];
const nullishTypes = ['null', 'undefined', 'null | undefined'];

function typeValidTest(
Expand All @@ -28,10 +29,11 @@ function nullishTypeValidTest(
nullish: string,
type: string,
) => TSESLint.ValidTestCase<Options> | string,
testedTypes: string[] = types,
): (TSESLint.ValidTestCase<Options> | string)[] {
return nullishTypes.reduce<(TSESLint.ValidTestCase<Options> | string)[]>(
(acc, nullish) => {
types.forEach(type => {
testedTypes.forEach(type => {
acc.push(cb(nullish, type));
});
return acc;
Expand All @@ -44,10 +46,11 @@ function nullishTypeInvalidTest(
nullish: string,
type: string,
) => TSESLint.InvalidTestCase<MessageIds, Options>,
testedTypes: string[] = types,
): TSESLint.InvalidTestCase<MessageIds, Options>[] {
return nullishTypes.reduce<TSESLint.InvalidTestCase<MessageIds, Options>[]>(
(acc, nullish) => {
types.forEach(type => {
testedTypes.forEach(type => {
acc.push(cb(nullish, type));
});
return acc;
Expand Down Expand Up @@ -138,6 +141,21 @@ a && b || c || d;
`,
options: [{ ignoreMixedLogicalExpressions: true }],
})),

// ignoreStrings
...nullishTypeValidTest(
(nullish, type) => ({
code: `
declare const a: ${type} | ${nullish};
declare const b: ${type} & ${nullish};
declare const c: ${type};
a || b || c;
c || b || a;
`,
options: [{ ignoreStrings: true }],
}),
['string'],
),
],
invalid: [
...nullishTypeInvalidTest((nullish, type) => ({
Expand Down Expand Up @@ -393,6 +411,80 @@ a && b || c ?? d;
],
})),

// ignoreStrings
...nullishTypeInvalidTest((nullish, type) => ({
code: `
declare const a: ${type} | ${nullish};
declare const b: ${type};
a || b;
`,
output: `
declare const a: ${type} | ${nullish};
declare const b: ${type};
a ?? b;
`,
options: [{ ignoreStrings: false }],
errors: [
{
messageId: 'preferNullish',
line: 4,
column: 3,
endLine: 4,
endColumn: 5,
},
],
})),
...nullishTypeInvalidTest(
(nullish, type) => ({
code: `
declare const a: ${type} | ${nullish};
declare const b: ${type};
a || b;
`,
output: `
declare const a: ${type} | ${nullish};
declare const b: ${type};
a ?? b;
`,
options: [{ ignoreStrings: true }],
errors: [
{
messageId: 'preferNullish',
line: 4,
column: 3,
endLine: 4,
endColumn: 5,
},
],
}),
allTypesButString,
),
...nullishTypeInvalidTest(
(nullish, type) => ({
code: `
declare const a: string | ${type} | ${nullish};
declare const b: string;
a || b;
`,
output: `
declare const a: string | ${type} | ${nullish};
declare const b: string;
a ?? b;
`,
options: [{ ignoreStrings: true }],
errors: [
{
messageId: 'preferNullish',
line: 4,
column: 3,
endLine: 4,
endColumn: 5,
},
],
}),
allTypesButString,
),

// should not false positive for functions inside conditional tests
...nullishTypeInvalidTest((nullish, type) => ({
code: `
Expand Down