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.

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

Jump to bottom

fix(eslint-plugin): [switch-exhaustiveness-check] enum members with new line or single quotes are not being fixed correctly #7806

Merged
merged 7 commits into from
Nov 16, 2023
Merged

fix(eslint-plugin): [switch-exhaustiveness-check] enum members with new line or single quotes are not being fixed correctly #7806

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2b3789
feat(eslint-plugin): [switch-exhaustiveness-check] members with new l…
StyleShit Oct 20, 2023
dd729d1
cleanup
StyleShit Oct 20, 2023
fd21aa4
oops
StyleShit Oct 20, 2023
1acfe46
wip wip
StyleShit Oct 23, 2023
17c9ef7
Merge remote-tracking branch 'typescript-eslint/main' into fix/switch…
StyleShit Oct 23, 2023
c63f866
maybe?
StyleShit Oct 23, 2023
82d5b37
idk why it was missing
StyleShit Oct 23, 2023
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
94 changes: 86 additions & 8 deletions packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
---
description: 'Require switch-case statements to be exhaustive with union type.'
description: 'Require switch-case statements to be exhaustive with union types and enums.'
---

> 🛑 This file is source code, not the primary documentation location! 🛑
>
> See **https://typescript-eslint.io/rules/switch-exhaustiveness-check** for documentation.

When working with union types 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).
However, if the union type changes, it's easy to forget to modify the cases to account for any new types.
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).
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 is missing a case for any of those literal types and does not have a `default` clause.
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.

## Examples

When the switch doesn't have exhaustive cases, either filling them all out or adding a default will correct the rule's complaint.

Here are some examples of code working with a union of literals:

<!--tabs-->

### ❌ Incorrect
Expand All @@ -27,7 +31,7 @@ type Day =
| 'Saturday'
| 'Sunday';

const day = 'Monday' as Day;
declare const day: Day;
let result = 0;

switch (day) {
Expand All @@ -49,7 +53,7 @@ type Day =
| 'Saturday'
| 'Sunday';

const day = 'Monday' as Day;
declare const day: Day;
let result = 0;

switch (day) {
Expand Down Expand Up @@ -89,7 +93,7 @@ type Day =
| 'Saturday'
| 'Sunday';

const day = 'Monday' as Day;
declare const day: Day;
let result = 0;

switch (day) {
Expand All @@ -101,6 +105,80 @@ switch (day) {
}
```

<!--/tabs-->

Likewise, here are some examples of code working with an enum:

<!--tabs-->

### ❌ Incorrect

Comment on lines +112 to +115
Copy link
Contributor Author

@StyleShit StyleShit Oct 20, 2023 •
edited

Choose a reason for hiding this comment

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

it's probably not displayed correctly, would be happy to get some help here regarding how we should display this (maybe add a subtitle?):

https://deploy-preview-7806--typescript-eslint.netlify.app/rules/switch-exhaustiveness-check#examples

Copy link
Member

Choose a reason for hiding this comment

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

<!-- /tabs --> is what you're looking for.

I'd also request a brief sentence before each of the examples explaining what they're for. It's kind of hard to piece together just from the code.

Vague starting proposal (just off the top of my head, not attached to this):

When the switch doesn't have exhaustive cases, either filling them all out or adding a default will correct the rule's complaint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

```ts
enum Fruit {
Apple,
Banana,
Cherry,
}

declare const fruit: Fruit;

switch (fruit) {
case Fruit.Apple:
console.log('an apple');
break;
}
```

### ✅ Correct

```ts
enum Fruit {
Apple,
Banana,
Cherry,
}

declare const fruit: Fruit;

switch (fruit) {
case Fruit.Apple:
console.log('an apple');
break;

case Fruit.Banana:
console.log('a banana');
break;

case Fruit.Cherry:
console.log('a cherry');
break;
}
```

### ✅ Correct

```ts
enum Fruit {
Apple,
Banana,
Cherry,
}

declare const fruit: Fruit;

switch (fruit) {
case Fruit.Apple:
console.log('an apple');
break;

default:
console.log('a fruit');
break;
}
```

<!--/tabs-->

## When Not To Use It

If you don't frequently `switch` over union types with many parts, or intentionally wish to leave out some parts.
If you don't frequently `switch` over union types or enums with many parts, or intentionally wish to leave out some parts.
17 changes: 13 additions & 4 deletions packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export default createRule({
type: 'suggestion',
docs: {
description:
'Require switch-case statements to be exhaustive with union type',
'Require switch-case statements to be exhaustive with union types and enums',
requiresTypeChecking: true,
},
hasSuggestions: true,
Expand Down Expand Up @@ -69,14 +69,23 @@ export default createRule({
let caseTest = checker.typeToString(missingBranchType);

if (
symbolName &&
(missingBranchName || missingBranchName === '') &&
requiresQuoting(missingBranchName.toString(), compilerOptions.target)
) {
caseTest = `${symbolName}['${missingBranchName}']`;
const requiresBackticks = missingBranchName?.match(/[\r\n]/g);

caseTest = requiresBackticks
? `${symbolName}[\`${missingBranchName}\`]`
: `${symbolName}['${missingBranchName}']`;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's discussed yet, but if we are escaping characters in the error message anyway, I would prefer text here to also be escaped instead of wrapping in backticks. Otherwise, you would also need to deal with ` in the text. (Do we even deal with ' correctly?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so your suggested case breaks the code (nice catch!)
But I'm not sure that I understand what you mean in this comment, can you maybe show some pseudo code and/or cases?

Copy link
Member

@Josh-Cena Josh-Cena Oct 23, 2023 •
edited

Choose a reason for hiding this comment

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

It means we simply output case '${escapedCaseTest}': { throw new Error('Not implemented yet: "${escapedCaseTest}" case') }. One consistent string literal to be used in both places. Then, you would only need to take care of ', \n, and \r in your escaping logic. By the way, if you want to be pedantic, you also need to take care of \u2028 and \u2029, which are also line terminators.

In terms of cases, it means we output:

switch (a) {
  case 'a\nb': { throw new Error('Not implemented yet: "a\nb" case') }
}

Instead of:

switch (a) {
  case `a
b`: { throw new Error('Not implemented yet: "a\nb" case') }
}

I'm sure no one writes code like the latter, especially with deeper indentations.

Copy link
Contributor Author

@StyleShit StyleShit Oct 23, 2023 •
edited

Choose a reason for hiding this comment

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

I'm not sure that we can use the same escapedCaseTest variable, because we still need to re-escape It before using it in the error, right? Or am I missing something?

Copy link
Contributor Author

@StyleShit StyleShit Oct 23, 2023 •
edited

Choose a reason for hiding this comment

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

Have a look at this one

Is that what you mean?

}

const errorMessage = `Not implemented yet: ${caseTest} case`;
// escape single quotes and newlines, so that the error message is readable and valid code.
const escapedCaseTest = caseTest
.replace(/'/g, "\\'")
.replace(/\n/g, '\\n')
.replace(/\r/g, '\\r');

const errorMessage = `Not implemented yet: ${escapedCaseTest} case`;

missingCases.push(
`case ${caseTest}: { throw new Error('${errorMessage}') }`,
Expand Down
48 changes: 45 additions & 3 deletions packages/eslint-plugin/tests/rules/switch-exhaustiveness-check.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ export enum Enum {

function test(arg: Enum): string {
switch (arg) {
case Enum['test-test']: { throw new Error('Not implemented yet: Enum['test-test'] case') }
case Enum['test-test']: { throw new Error('Not implemented yet: Enum[\\'test-test\\'] case') }
StyleShit marked this conversation as resolved.
Show resolved Hide resolved
case Enum.test: { throw new Error('Not implemented yet: Enum.test case') }
}
}
Expand Down Expand Up @@ -555,7 +555,7 @@ export enum Enum {

function test(arg: Enum): string {
switch (arg) {
case Enum['']: { throw new Error('Not implemented yet: Enum[''] case') }
case Enum['']: { throw new Error('Not implemented yet: Enum[\\'\\'] case') }
case Enum.test: { throw new Error('Not implemented yet: Enum.test case') }
}
}
Expand Down Expand Up @@ -592,7 +592,7 @@ export enum Enum {

function test(arg: Enum): string {
switch (arg) {
case Enum['9test']: { throw new Error('Not implemented yet: Enum['9test'] case') }
case Enum['9test']: { throw new Error('Not implemented yet: Enum[\\'9test\\'] case') }
case Enum.test: { throw new Error('Not implemented yet: Enum.test case') }
}
}
Expand All @@ -602,5 +602,47 @@ function test(arg: Enum): string {
},
],
},
{
code: `
enum Enum {
'a' = 1,
[\`key-with

new-line\`] = 2,
}

declare const a: Enum;

switch (a) {
}
`,
errors: [
{
messageId: 'switchIsNotExhaustive',
suggestions: [
{
messageId: 'addMissingCases',
output: `
enum Enum {
'a' = 1,
[\`key-with

new-line\`] = 2,
}

declare const a: Enum;

switch (a) {
case Enum.a: { throw new Error('Not implemented yet: Enum.a case') }
case Enum[\`key-with

new-line\`]: { throw new Error('Not implemented yet: Enum[\`key-with\\n\\n new-line\`] case') }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasn't sure about this. should we throw an error with actual new lines or \n is fine?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah good question. It's such a rare case, I don't think it matters too much. But in general having error messages be on one line is easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we just strip out any spaces, tabs, and new lines?

OK... it might look weird in some cases... IDK

key = `key-with
          new-line`

error = "not implemented: 'key-withnew-line'";

anyway, as you said, it's a rare case that probably isn't worth the effort

}
`,
},
],
},
],
},
],
});