From 6a219bdfe6fcf86aae28158e0d855f87a8bac719 Mon Sep 17 00:00:00 2001 From: James <5511220+Zamiell@users.noreply.github.com> Date: Fri, 29 Dec 2023 13:19:16 -0500 Subject: [PATCH] feat(eslint-plugin): [switch-exhaustiveness-check] add an option to warn against a `default` case on an already exhaustive `switch` (#7539) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: switch-exhaustiveness-check checks for dangerous default case * fix: spelling * fix: comment * fix: docs * feat: allowDefaultCase option * fix: tests * fix: lint * fix: prettier * refactor: finish merge * fix: format * fix: lint * chore: update docs * chore: update docs * chore: format * fix: test * fix: tests * fix: tests * fix: tests * fix: test * fix: test * fix: tests * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ * fix: double options in docs * Update packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md Co-authored-by: Josh Goldberg ✨ * feat: simplify code flow * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ * fix: grammar * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ * fix: wording on option * Update packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md Co-authored-by: Josh Goldberg ✨ * docs: add playground link * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ * chore: add punctuation * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ * chore: remove comment * refactor: rename option * fix: prettier * fix: lint * fix: tests * refactor: better metadata * fix: tests * refactor: rename interface * refactor: make interface readonly --------- Co-authored-by: Josh Goldberg ✨ --- .../docs/rules/switch-exhaustiveness-check.md | 64 +++-- .../src/rules/switch-exhaustiveness-check.ts | 246 ++++++++++++------ .../rules/switch-exhaustiveness-check.test.ts | 43 ++- .../switch-exhaustiveness-check.shot | 6 + 4 files changed, 259 insertions(+), 100 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md b/packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md index b362e97e1cf..b0cbaf1c47d 100644 --- a/packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md +++ b/packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md @@ -6,12 +6,51 @@ description: 'Require switch-case statements to be exhaustive.' > > See **https://typescript-eslint.io/rules/switch-exhaustiveness-check** for documentation. -When working with union types or enums in TypeScript, it's common to want to write a `switch` statement intended to contain a `case` for each constituent (possible type in the union or the enum). +When working with union types or enums in TypeScript, it's common to want to write a `switch` statement intended to contain a `case` for each possible type in the union or the enum. However, if the union type or the enum changes, it's easy to forget to modify the cases to account for any new types. This rule reports when a `switch` statement over a value typed as a union of literals or as an enum is missing a case for any of those literal types and does not have a `default` clause. -There is also an option to check the exhaustiveness of switches on non-union types by requiring a default clause. +## Options + +### `"allowDefaultCaseForExhaustiveSwitch"` + +Defaults to true. If set to false, this rule will also report when a `switch` statement has a case for everything in a union and _also_ contains a `default` case. Thus, by setting this option to false, the rule becomes stricter. + +When a `switch` statement over a union type is exhaustive, a final `default` case would be a form of dead code. +Additionally, if a new value is added to the union type, a `default` would prevent the `switch-exhaustiveness-check` rule from reporting on the new case not being handled in the `switch` statement. + +#### `"allowDefaultCaseForExhaustiveSwitch"` Caveats + +It can sometimes be useful to include a redundant `default` case on an exhaustive `switch` statement if it's possible for values to have types not represented by the union type. +For example, in applications that can have version mismatches between clients and servers, it's possible for a server running a newer software version to send a value not recognized by the client's older typings. + +If your project has a small number of intentionally redundant `default` cases, you might want to use an [inline ESLint disable comment](https://eslint.org/docs/latest/use/configure/rules#using-configuration-comments-1) for each of them. + +If your project has many intentionally redundant `default` cases, you may want to disable `allowDefaultCaseForExhaustiveSwitch` and use the [`default-case` core ESLint rule](https://eslint.org/docs/latest/rules/default-case) along with [a `satisfies never` check](https://www.typescriptlang.org/play?#code/C4TwDgpgBAYgTgVwJbCgXigcgIZjAGwkygB8sAjbAO2u0wG4AoRgMwSoGNgkB7KqBAGcI8ZMAAULRCgBcsacACUcwcDhIqAcygBvRlCiCA7ig4ALKJIWLd+g1A7ZhWXASJy99+3AjAEcfhw8QgApZA4iJi8AX2YvR2dMShoaTA87Lx8-AIpaGjCkCIYMqFiSgBMIFmwEfGB0rwMpMUNsbkEWJAhBKCoIADcIOCjGrP9A9gBrKh4jKgKikYNY5cZYoA). + +### `requireDefaultForNonUnion` + +Defaults to false. It set to true, this rule will also report when a `switch` statement switches over a non-union type (like a `number` or `string`, for example) and that `switch` statement does not have a `default` case. Thus, by setting this option to true, the rule becomes stricter. + +This is generally desirable so that `number` and `string` switches will be subject to the same exhaustive checks that your other switches are. + +Examples of additional **incorrect** code for this rule with `{ requireDefaultForNonUnion: true }`: + +```ts option='{ "requireDefaultForNonUnion": true }' showPlaygroundButton +const value: number = Math.floor(Math.random() * 3); + +switch (value) { + case 0: + return 0; + case 1: + return 1; +} +``` + +Since `value` is a non-union type it requires the switch case to have a default clause only with `requireDefaultForNonUnion` enabled. + + ## Examples @@ -181,27 +220,6 @@ switch (fruit) { -## Options - -### `requireDefaultForNonUnion` - -Examples of additional **incorrect** code for this rule with `{ requireDefaultForNonUnion: true }`: - -```ts option='{ "requireDefaultForNonUnion": true }' showPlaygroundButton -const value: number = Math.floor(Math.random() * 3); - -switch (value) { - case 0: - return 0; - case 1: - return 1; -} -``` - -Since `value` is a non-union type it requires the switch case to have a default clause only with `requireDefaultForNonUnion` enabled. - - - ## When Not To Use It If you don't frequently `switch` over union types or enums with many parts, or intentionally wish to leave out some parts, this rule may not be for you. diff --git a/packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts b/packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts index 90172006589..64ab5ef6b2e 100644 --- a/packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts +++ b/packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts @@ -12,9 +12,22 @@ import { requiresQuoting, } from '../util'; -type MessageIds = 'switchIsNotExhaustive' | 'addMissingCases'; +interface SwitchMetadata { + readonly symbolName: string | undefined; + readonly missingBranchTypes: ts.Type[]; + readonly defaultCase: TSESTree.SwitchCase | undefined; + readonly isUnion: boolean; +} + type Options = [ { + /** + * If `true`, allow `default` cases on switch statements with exhaustive cases. + * + * @default true + */ + allowDefaultCaseForExhaustiveSwitch?: boolean; + /** * If `true`, require a `default` clause for switches on non-union types. * @@ -24,6 +37,11 @@ type Options = [ }, ]; +type MessageIds = + | 'switchIsNotExhaustive' + | 'dangerousDefaultCase' + | 'addMissingCases'; + export default createRule({ name: 'switch-exhaustiveness-check', meta: { @@ -36,28 +54,132 @@ export default createRule({ schema: [ { type: 'object', - additionalProperties: false, properties: { + allowDefaultCaseForExhaustiveSwitch: { + description: `If 'true', allow 'default' cases on switch statements with exhaustive cases.`, + type: 'boolean', + }, requireDefaultForNonUnion: { description: `If 'true', require a 'default' clause for switches on non-union types.`, type: 'boolean', }, }, + additionalProperties: false, }, ], messages: { switchIsNotExhaustive: 'Switch is not exhaustive. Cases not matched: {{missingBranches}}', + dangerousDefaultCase: + 'The switch statement is exhaustive, so the default case is unnecessary.', addMissingCases: 'Add branches for missing cases.', }, }, - defaultOptions: [{ requireDefaultForNonUnion: false }], - create(context, [{ requireDefaultForNonUnion }]) { + defaultOptions: [ + { + allowDefaultCaseForExhaustiveSwitch: true, + requireDefaultForNonUnion: false, + }, + ], + create( + context, + [{ allowDefaultCaseForExhaustiveSwitch, requireDefaultForNonUnion }], + ) { const sourceCode = getSourceCode(context); const services = getParserServices(context); const checker = services.program.getTypeChecker(); const compilerOptions = services.program.getCompilerOptions(); + function getSwitchMetadata(node: TSESTree.SwitchStatement): SwitchMetadata { + const defaultCase = node.cases.find( + switchCase => switchCase.test == null, + ); + + const discriminantType = getConstrainedTypeAtLocation( + services, + node.discriminant, + ); + + const symbolName = discriminantType.getSymbol()?.escapedName as + | string + | undefined; + + if (!discriminantType.isUnion()) { + return { + symbolName, + missingBranchTypes: [], + defaultCase, + isUnion: false, + }; + } + + const caseTypes = new Set(); + for (const switchCase of node.cases) { + // If the `test` property of the switch case is `null`, then we are on a + // `default` case. + if (switchCase.test == null) { + continue; + } + + const caseType = getConstrainedTypeAtLocation( + services, + switchCase.test, + ); + caseTypes.add(caseType); + } + + const unionTypes = tsutils.unionTypeParts(discriminantType); + const missingBranchTypes = unionTypes.filter( + unionType => !caseTypes.has(unionType), + ); + + return { + symbolName, + missingBranchTypes, + defaultCase, + isUnion: true, + }; + } + + function checkSwitchExhaustive( + node: TSESTree.SwitchStatement, + switchMetadata: SwitchMetadata, + ): void { + const { missingBranchTypes, symbolName, defaultCase } = switchMetadata; + + // We only trigger the rule if a `default` case does not exist, since that + // would disqualify the switch statement from having cases that exactly + // match the members of a union. + if (missingBranchTypes.length > 0 && defaultCase === undefined) { + context.report({ + node: node.discriminant, + messageId: 'switchIsNotExhaustive', + data: { + missingBranches: missingBranchTypes + .map(missingType => + tsutils.isTypeFlagSet(missingType, ts.TypeFlags.ESSymbolLike) + ? `typeof ${missingType.getSymbol()?.escapedName as string}` + : checker.typeToString(missingType), + ) + .join(' | '), + }, + suggest: [ + { + messageId: 'addMissingCases', + fix(fixer): TSESLint.RuleFix | null { + return fixSwitch( + fixer, + node, + missingBranchTypes, + symbolName?.toString(), + ); + }, + }, + ], + }); + } + } + function fixSwitch( fixer: TSESLint.RuleFixer, node: TSESTree.SwitchStatement, @@ -68,8 +190,8 @@ export default createRule({ node.cases.length > 0 ? node.cases[node.cases.length - 1] : null; const caseIndent = lastCase ? ' '.repeat(lastCase.loc.start.column) - : // if there are no cases, use indentation of the switch statement - // and leave it to user to format it correctly + : // If there are no cases, use indentation of the switch statement and + // leave it to the user to format it correctly. ' '.repeat(node.loc.start.column); const missingCases = []; @@ -78,14 +200,17 @@ export default createRule({ missingCases.push(`default: { throw new Error('default case') }`); continue; } - // While running this rule on checker.ts of TypeScript project + + // While running this rule on the "checker.ts" file of TypeScript, the // the fix introduced a compiler error due to: // + // ```ts // type __String = (string & { - // __escapedIdentifier: void; - // }) | (void & { - // __escapedIdentifier: void; - // }) | InternalSymbolName; + // __escapedIdentifier: void; + // }) | (void & { + // __escapedIdentifier: void; + // }) | InternalSymbolName; + // ``` // // The following check fixes it. if (missingBranchType.isIntersection()) { @@ -124,7 +249,7 @@ export default createRule({ return fixer.insertTextAfter(lastCase, `\n${fixString}`); } - // there were no existing cases + // There were no existing cases. const openingBrace = sourceCode.getTokenAfter( node.discriminant, isOpeningBraceToken, @@ -140,89 +265,60 @@ export default createRule({ ); } - function checkSwitchExhaustive(node: TSESTree.SwitchStatement): void { - const discriminantType = getConstrainedTypeAtLocation( - services, - node.discriminant, - ); - const symbolName = discriminantType.getSymbol()?.escapedName; - - if (discriminantType.isUnion()) { - const unionTypes = tsutils.unionTypeParts(discriminantType); - const caseTypes = new Set(); - for (const switchCase of node.cases) { - if (switchCase.test == null) { - // Switch has 'default' branch - do nothing. - return; - } - - caseTypes.add( - getConstrainedTypeAtLocation(services, switchCase.test), - ); - } + function checkSwitchUnnecessaryDefaultCase( + switchMetadata: SwitchMetadata, + ): void { + if (allowDefaultCaseForExhaustiveSwitch) { + return; + } - const missingBranchTypes = unionTypes.filter( - unionType => !caseTypes.has(unionType), - ); + const { missingBranchTypes, defaultCase } = switchMetadata; - if (missingBranchTypes.length === 0) { - // All cases matched - do nothing. - return; - } + if (missingBranchTypes.length === 0 && defaultCase !== undefined) { + context.report({ + node: defaultCase, + messageId: 'dangerousDefaultCase', + }); + } + } + function checkSwitchNoUnionDefaultCase( + node: TSESTree.SwitchStatement, + switchMetadata: SwitchMetadata, + ): void { + if (!requireDefaultForNonUnion) { + return; + } + + const { isUnion, defaultCase } = switchMetadata; + + if (!isUnion && defaultCase === undefined) { context.report({ node: node.discriminant, messageId: 'switchIsNotExhaustive', data: { - missingBranches: missingBranchTypes - .map(missingType => - tsutils.isTypeFlagSet(missingType, ts.TypeFlags.ESSymbolLike) - ? `typeof ${missingType.getSymbol()?.escapedName as string}` - : checker.typeToString(missingType), - ) - .join(' | '), + missingBranches: 'default', }, suggest: [ { messageId: 'addMissingCases', fix(fixer): TSESLint.RuleFix { - return fixSwitch( - fixer, - node, - missingBranchTypes, - symbolName?.toString(), - ); + return fixSwitch(fixer, node, [null]); }, }, ], }); - } else if (requireDefaultForNonUnion) { - const hasDefault = node.cases.some( - switchCase => switchCase.test == null, - ); - - if (!hasDefault) { - context.report({ - node: node.discriminant, - messageId: 'switchIsNotExhaustive', - data: { - missingBranches: 'default', - }, - suggest: [ - { - messageId: 'addMissingCases', - fix(fixer): TSESLint.RuleFix { - return fixSwitch(fixer, node, [null]); - }, - }, - ], - }); - } } } return { - SwitchStatement: checkSwitchExhaustive, + SwitchStatement(node): void { + const switchMetadata = getSwitchMetadata(node); + + checkSwitchExhaustive(node, switchMetadata); + checkSwitchUnnecessaryDefaultCase(switchMetadata); + checkSwitchNoUnionDefaultCase(node, switchMetadata); + }, }; }, }); diff --git a/packages/eslint-plugin/tests/rules/switch-exhaustiveness-check.test.ts b/packages/eslint-plugin/tests/rules/switch-exhaustiveness-check.test.ts index cf7f6791753..9a06ace1e0a 100644 --- a/packages/eslint-plugin/tests/rules/switch-exhaustiveness-check.test.ts +++ b/packages/eslint-plugin/tests/rules/switch-exhaustiveness-check.test.ts @@ -222,7 +222,12 @@ switch (value) { return -1; } `, - options: [{ requireDefaultForNonUnion: true }], + options: [ + { + allowDefaultCaseForExhaustiveSwitch: true, + requireDefaultForNonUnion: true, + }, + ], }, ], invalid: [ @@ -627,7 +632,12 @@ switch (value) { return 1; } `, - options: [{ requireDefaultForNonUnion: true }], + options: [ + { + allowDefaultCaseForExhaustiveSwitch: true, + requireDefaultForNonUnion: true, + }, + ], errors: [ { messageId: 'switchIsNotExhaustive', @@ -724,5 +734,34 @@ switch (value) { }, ], }, + { + code: ` +type MyUnion = 'foo' | 'bar' | 'baz'; + +declare const myUnion: MyUnion; + +switch (myUnion) { + case 'foo': + case 'bar': + case 'baz': { + break; + } + default: { + break; + } +} + `, + options: [ + { + allowDefaultCaseForExhaustiveSwitch: false, + requireDefaultForNonUnion: false, + }, + ], + errors: [ + { + messageId: 'dangerousDefaultCase', + }, + ], + }, ], }); diff --git a/packages/eslint-plugin/tests/schema-snapshots/switch-exhaustiveness-check.shot b/packages/eslint-plugin/tests/schema-snapshots/switch-exhaustiveness-check.shot index 10996a21371..6146dadc128 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/switch-exhaustiveness-check.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/switch-exhaustiveness-check.shot @@ -8,6 +8,10 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos { "additionalProperties": false, "properties": { + "allowDefaultCaseForExhaustiveSwitch": { + "description": "If 'true', allow 'default' cases on switch statements with exhaustive cases.", + "type": "boolean" + }, "requireDefaultForNonUnion": { "description": "If 'true', require a 'default' clause for switches on non-union types.", "type": "boolean" @@ -22,6 +26,8 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos type Options = [ { + /** If 'true', allow 'default' cases on switch statements with exhaustive cases. */ + allowDefaultCaseForExhaustiveSwitch?: boolean; /** If 'true', require a 'default' clause for switches on non-union types. */ requireDefaultForNonUnion?: boolean; },