diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dea710..862d852 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- `noUnnecessaryCondition`: Catch useless `?? null` and `?? undefined` coalescing and add "Remove unnecessary nullish coalescing" suggestion. + ## 1.0.27 - `preferOptionalChain`: fix chain analysis when the chain ends with `=== null` diff --git a/src/rules/noUnnecessaryCondition/noUnnecessaryCondition.test.ts b/src/rules/noUnnecessaryCondition/noUnnecessaryCondition.test.ts index 9f7a42c..71e019e 100644 --- a/src/rules/noUnnecessaryCondition/noUnnecessaryCondition.test.ts +++ b/src/rules/noUnnecessaryCondition/noUnnecessaryCondition.test.ts @@ -934,6 +934,7 @@ function test(arg: T | { value: string }) { } } `, + `declare const foo: string | undefined; const bar = foo ?? null;`, ], invalid: [ // Ensure that it's checking in all the right places @@ -1556,14 +1557,31 @@ function test(a: T) { column: 15, }, ], - }, // Nullish coalescing operator + }, + // Nullish coalescing operator { code: ` function test(a: string) { return a ?? 'default'; } `, - errors: [{ message: messages.neverNullish, line: 3, column: 10 }], + errors: [ + { + message: messages.neverNullish, + line: 3, + column: 12, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +function test(a: string) { + return a; +} + `, + }, + ], + }, + ], }, { code: ` @@ -1571,7 +1589,23 @@ function test(a: string | false) { return a ?? 'default'; } `, - errors: [{ message: messages.neverNullish, line: 3, column: 10 }], + errors: [ + { + message: messages.neverNullish, + line: 3, + column: 12, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +function test(a: string | false) { + return a; +} + `, + }, + ], + }, + ], }, { code: ` @@ -1579,15 +1613,48 @@ function test(a: T) { return a ?? 'default'; } `, - errors: [{ message: messages.neverNullish, line: 3, column: 10 }], - }, // nullish + array index without optional chaining + errors: [ + { + message: messages.neverNullish, + line: 3, + column: 12, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +function test(a: T) { + return a; +} + `, + }, + ], + }, + ], + }, + // nullish + array index without optional chaining { code: ` function test(a: { foo: string }[]) { return a[0].foo ?? 'default'; } `, - errors: [{ message: messages.neverNullish, line: 3, column: 10 }], + errors: [ + { + message: messages.neverNullish, + line: 3, + column: 19, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +function test(a: { foo: string }[]) { + return a[0].foo; +} + `, + }, + ], + }, + ], }, { code: ` @@ -1619,7 +1686,23 @@ function test(a: never) { return a ?? 'default'; } `, - errors: [{ message: messages.never, line: 3, column: 10 }], + errors: [ + { + message: messages.never, + line: 3, + column: 12, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +function test(a: never) { + return a; +} + `, + }, + ], + }, + ], }, { code: ` @@ -1627,8 +1710,25 @@ function test(num: T[K]) { num ?? 'default'; } `, - errors: [{ message: messages.neverNullish, line: 3, column: 3 }], - }, // Predicate functions + errors: [ + { + message: messages.neverNullish, + line: 3, + column: 7, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +function test(num: T[K]) { + num; +} + `, + }, + ], + }, + ], + }, + // Predicate functions { code: ` [1, 3, 5].filter(() => true); @@ -2757,9 +2857,18 @@ foo ??= 1; { message: messages.neverNullish, line: 3, - column: 1, - endColumn: 4, + column: 5, endLine: 3, + endColumn: 10, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +declare let foo: {}; +foo; + `, + }, + ], }, ], }, @@ -2772,9 +2881,18 @@ foo ??= 1; { message: messages.neverNullish, line: 3, - column: 1, - endColumn: 4, + column: 5, endLine: 3, + endColumn: 10, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +declare let foo: number; +foo; + `, + }, + ], }, ], }, @@ -2863,9 +2981,18 @@ foo.bar ??= 1; { message: messages.neverNullish, line: 3, - column: 1, - endColumn: 8, + column: 9, endLine: 3, + endColumn: 14, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +declare const foo: { bar: number }; +foo.bar; + `, + }, + ], }, ], }, @@ -3185,5 +3312,49 @@ if (arr[42] && arr[42]) {} `, errors: [{ message: messages.alwaysTruthy, line: 3, column: 16 }], }, + { + code: ` +declare const foo: string | null; +const bar = foo ?? null; + `, + errors: [ + { + message: messages.uselessNullCoalescing, + line: 3, + column: 17, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +declare const foo: string | null; +const bar = foo; + `, + }, + ], + }, + ], + }, + { + code: ` +declare const foo: string | undefined; +const bar = foo ?? undefined; + `, + errors: [ + { + message: messages.uselessUndefinedCoalescing, + line: 3, + column: 17, + suggestions: [ + { + message: messages.removeNullishCoalescing, + output: ` +declare const foo: string | undefined; +const bar = foo; + `, + }, + ], + }, + ], + }, ], }); diff --git a/src/rules/noUnnecessaryCondition/noUnnecessaryCondition.ts b/src/rules/noUnnecessaryCondition/noUnnecessaryCondition.ts index eb0ab10..6ae58a2 100644 --- a/src/rules/noUnnecessaryCondition/noUnnecessaryCondition.ts +++ b/src/rules/noUnnecessaryCondition/noUnnecessaryCondition.ts @@ -37,15 +37,20 @@ export const messages = { right: string; }) => `Unnecessary conditional, comparison is always ${params.trueOrFalse}, since ${params.left} ${params.operator} ${params.right} is ${params.trueOrFalse}.`, - never: "Unnecessary conditional, value is `never`.", + never: "Unnecessary conditional, left-hand side of `??` operator is `never`.", neverNullish: - "Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined.", + "Unnecessary conditional, left-hand side of `??` operator is not possibly null or undefined.", + uselessNullCoalescing: + "Unnecessary coalescing, null is already included in the left-hand side of the `??` operator.", + uselessUndefinedCoalescing: + "Unnecessary coalescing, undefined is already included in the left-hand side of the `??` operator.", neverOptionalChain: "Unnecessary optional chain on a non-nullish value.", noOverlapBooleanExpression: "Unnecessary conditional, the types have no overlap.", typeGuardAlreadyIsType: (params: { typeGuardOrAssertionFunction: string }) => `Unnecessary conditional, expression already has the type being checked by the ${params.typeGuardOrAssertionFunction}.`, removeOptionalChain: "Remove unnecessary optional chain.", + removeNullishCoalescing: "Remove unnecessary nullish coalescing.", }; export type NoUnnecessaryConditionOptions = { @@ -85,10 +90,8 @@ export const noUnnecessaryCondition = defineRule( checkNode(context, node.left); break; case SyntaxKind.QuestionQuestionEqualsToken: - checkNodeForNullish(context, node.left); - break; case SyntaxKind.QuestionQuestionToken: - checkNodeForNullish(context, node.left); + checkNodeForNullish(context, node); break; case SyntaxKind.BarBarToken: case SyntaxKind.AmpersandAmpersandToken: @@ -305,13 +308,16 @@ function checkNode( } } -function checkNodeForNullish(context: Context, node: AST.Expression): void { - const type = context.utils.getConstrainedTypeAtLocation(node); +function checkNodeForNullish( + context: Context, + node: AST.BinaryExpression, +): void { + const leftType = context.utils.getConstrainedTypeAtLocation(node.left); // Conditional is always necessary if it involves `any`, `unknown` or a naked type parameter if ( context.utils.typeOrUnionHasFlag( - type, + leftType, TypeFlags.Any | TypeFlags.Unknown | TypeFlags.TypeParameter @@ -321,15 +327,34 @@ function checkNodeForNullish(context: Context, node: AST.Expression): void { return; } - let message: string | null = null; - if (context.utils.typeOrUnionHasFlag(type, TypeFlags.Never)) { - message = messages.never; + function reportUselessCoalescing(message: string) { + context.report({ + start: node.operatorToken.getStart(), + end: node.right.getEnd(), + message, + suggestions: [ + { + message: messages.removeNullishCoalescing, + changes: [ + { + start: node.left.getEnd(), + end: node.right.getEnd(), + newText: "", + }, + ], + }, + ], + }); + } + + if (context.utils.typeOrUnionHasFlag(leftType, TypeFlags.Never)) { + reportUselessCoalescing(messages.never); } else if ( - !context.utils.typeOrUnionHasFlag(type, nullishFlag) + !context.utils.typeOrUnionHasFlag(leftType, nullishFlag) && !( - (node.kind === SyntaxKind.PropertyAccessExpression - || node.kind === SyntaxKind.ElementAccessExpression) - && isNullableMemberExpression(context, node) + (node.left.kind === SyntaxKind.PropertyAccessExpression + || node.left.kind === SyntaxKind.ElementAccessExpression) + && isNullableMemberExpression(context, node.left) ) ) { // Since typescript array index signature types don't represent the @@ -337,20 +362,29 @@ function checkNodeForNullish(context: Context, node: AST.Expression): void { // just skip the check, to avoid false positives if ( context.compilerOptions.noUncheckedIndexedAccess === true - || (!isArrayIndexExpression(context, node) + || (!isArrayIndexExpression(context, node.left) && !( - node.kind === SyntaxKind.PropertyAccessExpression - && optionChainContainsOptionArrayIndex(context, node) + node.left.kind === SyntaxKind.PropertyAccessExpression + && optionChainContainsOptionArrayIndex(context, node.left) )) ) { - message = messages.neverNullish; + reportUselessCoalescing(messages.neverNullish); + } + } else if (isAlwaysNullish(context, leftType)) { + context.report({ node: node.left, message: messages.alwaysNullish }); + } else { + if ( + context.utils.typeOrUnionHasFlag(leftType, TypeFlags.Null) + && node.right.kind === SyntaxKind.NullKeyword + ) { + reportUselessCoalescing(messages.uselessNullCoalescing); + } else if ( + context.utils.typeOrUnionHasFlag(leftType, TypeFlags.Undefined) + && node.right.kind === SyntaxKind.Identifier + && node.right.text === "undefined" + ) { + reportUselessCoalescing(messages.uselessUndefinedCoalescing); } - } else if (isAlwaysNullish(context, type)) { - message = messages.alwaysNullish; - } - - if (message) { - context.report({ node, message }); } }