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

feat(eslint-plugin): [no-duplicate-enum-values] add rule #4833

Merged
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
1 change: 1 addition & 0 deletions packages/eslint-plugin/README.md
Expand Up @@ -119,6 +119,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int
| [`@typescript-eslint/no-base-to-string`](./docs/rules/no-base-to-string.md) | Requires that `.toString()` is only called on objects which provide useful information when stringified | | | :thought_balloon: |
| [`@typescript-eslint/no-confusing-non-null-assertion`](./docs/rules/no-confusing-non-null-assertion.md) | Disallow non-null assertion in locations that may be confusing | | :wrench: | |
| [`@typescript-eslint/no-confusing-void-expression`](./docs/rules/no-confusing-void-expression.md) | Requires expressions of type void to appear in statement position | | :wrench: | :thought_balloon: |
| [`@typescript-eslint/no-duplicate-enum-values`](./docs/rules/no-duplicate-enum-values.md) | Disallow duplicate enum member values | | | |
| [`@typescript-eslint/no-dynamic-delete`](./docs/rules/no-dynamic-delete.md) | Disallow the delete operator with computed key expressions | | :wrench: | |
| [`@typescript-eslint/no-empty-interface`](./docs/rules/no-empty-interface.md) | Disallow the declaration of empty interfaces | :white_check_mark: | :wrench: | |
| [`@typescript-eslint/no-explicit-any`](./docs/rules/no-explicit-any.md) | Disallow usage of the `any` type | :white_check_mark: | :wrench: | |
Expand Down
51 changes: 51 additions & 0 deletions packages/eslint-plugin/docs/rules/no-duplicate-enum-values.md
@@ -0,0 +1,51 @@
# `no-duplicate-enum-values`

Disallow duplicate enum member values.

Although TypeScript supports duplicate enum member values, people usually expect members to have unique values within the same enum. Duplicate values can lead to bugs that are hard to track down.

## Rule Details

This rule disallows defining an enum with multiple members initialized to the same value. Now it only enforces on enum members initialized with String or Number literals. Members without initializer or initialized with an expression are not checked by this rule.

<!--tabs-->

### ❌ Incorrect

```ts
enum E {
A = 0,
B = 0,
}
```

```ts
enum E {
A = 'A'
B = 'A'
}
```

### ✅ Correct

```ts
enum E {
A = 0,
B = 1,
}
```

```ts
enum E {
A = 'A'
B = 'B'
}
```

This rule is not configurable.

## Attributes

- [ ] ✅ Recommended
- [ ] 🔧 Fixable
- [ ] 💭 Requires type information
9 changes: 5 additions & 4 deletions packages/eslint-plugin/src/configs/all.ts
Expand Up @@ -21,8 +21,8 @@ export = {
'@typescript-eslint/consistent-indexed-object-style': 'error',
'@typescript-eslint/consistent-type-assertions': 'error',
'@typescript-eslint/consistent-type-definitions': 'error',
'@typescript-eslint/consistent-type-imports': 'error',
'@typescript-eslint/consistent-type-exports': 'error',
'@typescript-eslint/consistent-type-imports': 'error',
'default-param-last': 'off',
'@typescript-eslint/default-param-last': 'error',
'dot-notation': 'off',
Expand Down Expand Up @@ -51,6 +51,7 @@ export = {
'@typescript-eslint/no-confusing-void-expression': 'error',
'no-dupe-class-members': 'off',
'@typescript-eslint/no-dupe-class-members': 'error',
'@typescript-eslint/no-duplicate-enum-values': 'error',
'no-duplicate-imports': 'off',
'@typescript-eslint/no-duplicate-imports': 'error',
'@typescript-eslint/no-dynamic-delete': 'error',
Expand Down Expand Up @@ -115,9 +116,9 @@ export = {
'@typescript-eslint/no-unused-vars': 'error',
'no-use-before-define': 'off',
'@typescript-eslint/no-use-before-define': 'error',
'@typescript-eslint/no-useless-empty-export': 'error',
'no-useless-constructor': 'off',
'@typescript-eslint/no-useless-constructor': 'error',
'@typescript-eslint/no-useless-empty-export': 'error',
'@typescript-eslint/no-var-requires': 'error',
'@typescript-eslint/non-nullable-type-assertion-style': 'error',
'object-curly-spacing': 'off',
Expand Down Expand Up @@ -153,12 +154,12 @@ export = {
semi: 'off',
'@typescript-eslint/semi': 'error',
'@typescript-eslint/sort-type-union-intersection-members': 'error',
'space-before-blocks': 'off',
'@typescript-eslint/space-before-blocks': 'error',
'space-before-function-paren': 'off',
'@typescript-eslint/space-before-function-paren': 'error',
'space-infix-ops': 'off',
'@typescript-eslint/space-infix-ops': 'error',
'space-before-blocks': 'off',
'@typescript-eslint/space-before-blocks': 'error',
'@typescript-eslint/strict-boolean-expressions': 'error',
'@typescript-eslint/switch-exhaustiveness-check': 'error',
'@typescript-eslint/triple-slash-reference': 'error',
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Expand Up @@ -32,6 +32,7 @@ import noBaseToString from './no-base-to-string';
import confusingNonNullAssertionLikeNotEqual from './no-confusing-non-null-assertion';
import noConfusingVoidExpression from './no-confusing-void-expression';
import noDupeClassMembers from './no-dupe-class-members';
import noDuplicateEnumValues from './no-duplicate-enum-values';
import noDuplicateImports from './no-duplicate-imports';
import noDynamicDelete from './no-dynamic-delete';
import noEmptyFunction from './no-empty-function';
Expand Down Expand Up @@ -159,6 +160,7 @@ export default {
'no-confusing-non-null-assertion': confusingNonNullAssertionLikeNotEqual,
'no-confusing-void-expression': noConfusingVoidExpression,
'no-dupe-class-members': noDupeClassMembers,
'no-duplicate-enum-values': noDuplicateEnumValues,
'no-duplicate-imports': noDuplicateImports,
'no-dynamic-delete': noDynamicDelete,
'no-empty-function': noEmptyFunction,
Expand Down
72 changes: 72 additions & 0 deletions packages/eslint-plugin/src/rules/no-duplicate-enum-values.ts
@@ -0,0 +1,72 @@
import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils';
import * as util from '../util';

export default util.createRule({
name: 'no-duplicate-enum-values',
meta: {
type: 'problem',
docs: {
description: 'Disallow duplicate enum member values',
recommended: false,
},
hasSuggestions: true,
messages: {
duplicateValue: 'Duplicate enum member value {{value}}.',
},
schema: [],
},
defaultOptions: [],
create(context) {
function isStringLiteral(
node: TSESTree.Expression,
): node is TSESTree.StringLiteral {
return (
node.type === AST_NODE_TYPES.Literal && typeof node.value === 'string'
);
}

function isNumberLiteral(
node: TSESTree.Expression,
): node is TSESTree.NumberLiteral {
return (
node.type === AST_NODE_TYPES.Literal && typeof node.value === 'number'
);
}
Copy link
Member

Choose a reason for hiding this comment

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

[Nitpicking] Is there a need to separate these two functions out? They both check that node.type === AST_NODE_TYPES.Literal.

I'd suggest either making a single separate function:

function getExpressionLiteralValue(node: TSESTree.Expression) {
  if (node.type === AST_NODE_TYPES.Literal) {
    switch (typeof node.value) {
      // ...
    }  
  }
}

...or inlining them below:

```ts
let value: string | number | undefined;

if (node.type === AST_NODE_TYPES.Literal) {
  // ...
}

Just suggestions, feel free to ignore if you don't like either of these changes 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to do this because typescript doesn't know the type of member.initializer without the util functions, and would complain when I cast its value to String or Number.


return {
TSEnumDeclaration(node: TSESTree.TSEnumDeclaration): void {
const enumMembers = node.members;
const seenValues = new Set<number | string>();

enumMembers.forEach(member => {
if (member.initializer === undefined) {
return;
}

let value: string | number | undefined;
if (isStringLiteral(member.initializer)) {
value = String(member.initializer.value);
} else if (isNumberLiteral(member.initializer)) {
value = Number(member.initializer.value);
}

if (value === undefined) {
return;
}

if (seenValues.has(value)) {
context.report({
node: member,
messageId: 'duplicateValue',
data: {
value,
},
});
} else {
seenValues.add(value);
}
});
},
};
},
});
136 changes: 136 additions & 0 deletions packages/eslint-plugin/tests/rules/no-duplicate-enum-values.test.ts
@@ -0,0 +1,136 @@
import rule from '../../src/rules/no-duplicate-enum-values';
import { RuleTester } from '../RuleTester';

const ruleTester = new RuleTester({
parser: '@typescript-eslint/parser',
});

ruleTester.run('no-duplicate-enum-values', rule, {
valid: [
`
enum E {
A,
B,
}
`,
`
enum E {
A = 1,
B,
}
`,
`
enum E {
A = 1,
B = 2,
}
`,
`
enum E {
A = 'A',
B = 'B',
}
`,
`
enum E {
A = 'A',
B = 'B',
C,
}
`,
`
enum E {
A = 'A',
B = 'B',
C = 2,
D = 1 + 1,
}
`,
`
enum E {
A = 3,
B = 2,
C,
}
`,
`
enum E {
A = 'A',
B = 'B',
C = 2,
D = foo(),
}
`,
`
enum E {
A = '',
B = 0,
}
`,
`
enum E {
A = 0,
B = -0,
C = NaN,
}
`,
],
invalid: [
{
code: `
enum E {
A = 1,
B = 1,
}
`,
errors: [
{
line: 4,
column: 3,
messageId: 'duplicateValue',
data: { value: 1 },
},
],
},
{
code: `
enum E {
A = 'A',
B = 'A',
}
`,
errors: [
{
line: 4,
column: 3,
messageId: 'duplicateValue',
data: { value: 'A' },
},
],
},
{
code: `
enum E {
A = 'A',
B = 'A',
C = 1,
D = 1,
}
`,
errors: [
{
line: 4,
column: 3,
messageId: 'duplicateValue',
data: { value: 'A' },
},
{
line: 6,
column: 3,
messageId: 'duplicateValue',
data: { value: 1 },
},
],
},
],
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
});