feat(eslint-plugin): add no-magic-numbers rule (#373)

typescript-eslint · May 8, 2019 · 43fa09c · 43fa09c
1 parent 665278f
commit 43fa09c
Show file tree

Hide file tree

Showing 5 changed files with 353 additions and 0 deletions.
diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md
@@ -134,6 +134,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
 | [`@typescript-eslint/no-extraneous-class`](./docs/rules/no-extraneous-class.md)                           | Forbids the use of classes as namespaces (`no-unnecessary-class` from TSLint)                                                                       |                    |          |                   |
 | [`@typescript-eslint/no-for-in-array`](./docs/rules/no-for-in-array.md)                                   | Disallow iterating over an array with a for-in loop (`no-for-in-array` from TSLint)                                                                 |                    |          | :thought_balloon: |
 | [`@typescript-eslint/no-inferrable-types`](./docs/rules/no-inferrable-types.md)                           | Disallows explicit type declarations for variables or parameters initialized to a number, string, or boolean. (`no-inferrable-types` from TSLint)   | :heavy_check_mark: | :wrench: |                   |
+| [`@typescript-eslint/no-magic-numbers`](./docs/rules/no-magic-numbers.md)                                 | Disallows magic numbers.                                                                                                                            | :heavy_check_mark: |          |
 | [`@typescript-eslint/no-misused-new`](./docs/rules/no-misused-new.md)                                     | Enforce valid definition of `new` and `constructor`. (`no-misused-new` from TSLint)                                                                 | :heavy_check_mark: |          |                   |
 | [`@typescript-eslint/no-namespace`](./docs/rules/no-namespace.md)                                         | Disallow the use of custom TypeScript modules and namespaces (`no-namespace` from TSLint)                                                           | :heavy_check_mark: |          |                   |
 | [`@typescript-eslint/no-non-null-assertion`](./docs/rules/no-non-null-assertion.md)                       | Disallows non-null assertions using the `!` postfix operator (`no-non-null-assertion` from TSLint)                                                  | :heavy_check_mark: |          |                   |

diff --git a/packages/eslint-plugin/docs/rules/no-magic-numbers.md b/packages/eslint-plugin/docs/rules/no-magic-numbers.md
@@ -0,0 +1,44 @@
+# Disallow Magic Numbers (@typescript-eslint/no-magic-numbers)
+
+'Magic numbers' are numbers that occur multiple times in code without an explicit meaning.
+They should preferably be replaced by named constants.
+
+## Rule Details
+
+The `@typescript-eslint/no-magic-numbers` rule extends the `no-magic-numbers` rule from ESLint core, and adds support for handling Typescript specific code that would otherwise trigger the rule.
+
+See the [ESLint documentation](https://eslint.org/docs/rules/no-magic-numbers) for more details on the `no-magic-numbers` rule.
+
+## Rule Changes
+
+```cjson
+{
+    // note you must disable the base rule as it can report incorrect errors
+    "no-magic-numbers": "off",
+    "@typescript-eslint/no-magic-numbers": ["error", { "ignoreNumericLiteralTypes": true }]
+}
+```
+
+In addition to the options supported by the `no-magic-numbers` rule in ESLint core, the rule adds the following options:
+
+### ignoreNumericLiteralTypes
+
+A boolean to specify if numbers used in Typescript numeric literal types are considered okay. `false` by default.
+
+Examples of **incorrect** code for the `{ "ignoreNumericLiteralTypes": false }` option:
+
+```ts
+/*eslint @typescript-eslint/no-magic-numbers: ["error", { "ignoreNumericLiteralTypes": false }]*/
+
+type SmallPrimes = 2 | 3 | 5 | 7 | 11;
+```
+
+Examples of **correct** code for the `{ "ignoreNumericLiteralTypes": true }` option:
+
+```ts
+/*eslint @typescript-eslint/no-magic-numbers: ["error", { "ignoreNumericLiteralTypes": true }]*/
+
+type SmallPrimes = 2 | 3 | 5 | 7 | 11;
+```
+
+<sup>Taken with ❤️ [from ESLint core](https://github.com/eslint/eslint/blob/master/docs/rules/no-magic-numbers.md)</sup>
diff --git a/packages/eslint-plugin/src/rules/no-magic-numbers.ts b/packages/eslint-plugin/src/rules/no-magic-numbers.ts
@@ -0,0 +1,152 @@
+/**
+ * @fileoverview Rule to flag statements that use magic numbers (adapted from https://github.com/danielstjules/buddy.js)
+ * @author Scott O'Hara
+ */
+
+import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/typescript-estree';
+import baseRule from 'eslint/lib/rules/no-magic-numbers';
+import * as util from '../util';
+import { JSONSchema4 } from 'json-schema';
+
+type Options = util.InferOptionsTypeFromRule<typeof baseRule>;
+type MessageIds = util.InferMessageIdsTypeFromRule<typeof baseRule>;
+
+const baseRuleSchema = (baseRule.meta.schema as JSONSchema4[])[0];
+
+export default util.createRule<Options, MessageIds>({
+  name: 'no-magic-numbers',
+  meta: {
+    type: 'suggestion',
+    docs: {
+      description: 'Disallow magic numbers',
+      category: 'Best Practices',
+      recommended: false,
+    },
+    // Extend base schema with additional property to ignore TS numeric literal types
+    schema: [
+      {
+        ...baseRuleSchema,
+        properties: {
+          ...baseRuleSchema.properties,
+          ignoreNumericLiteralTypes: {
+            type: 'boolean',
+          },
+        },
+      },
+    ],
+    messages: baseRule.meta.messages,
+  },
+  defaultOptions: [
+    {
+      ignore: [],
+      ignoreArrayIndexes: false,
+      enforceConst: false,
+      detectObjects: false,
+      ignoreNumericLiteralTypes: false,
+    },
+  ],
+  create(context, [options]) {
+    const rules = baseRule.create(context);
+
+    /**
+     * Returns whether the node is number literal
+     * @param node the node literal being evaluated
+     * @returns true if the node is a number literal
+     */
+    function isNumber(node: TSESTree.Literal): boolean {
+      return typeof node.value === 'number';
+    }
+
+    /**
+     * Checks if the node grandparent is a Typescript type alias declaration
+     * @param node the node to be validated.
+     * @returns true if the node grandparent is a Typescript type alias declaration
+     * @private
+     */
+    function isGrandparentTSTypeAliasDeclaration(node: TSESTree.Node): boolean {
+      return node.parent && node.parent.parent
+        ? node.parent.parent.type === AST_NODE_TYPES.TSTypeAliasDeclaration
+        : false;
+    }
+
+    /**
+     * Checks if the node grandparent is a Typescript union type and its parent is a type alias declaration
+     * @param node the node to be validated.
+     * @returns true if the node grandparent is a Typescript untion type and its parent is a type alias declaration
+     * @private
+     */
+    function isGrandparentTSUnionType(node: TSESTree.Node): boolean {
+      if (
+        node.parent &&
+        node.parent.parent &&
+        node.parent.parent.type === AST_NODE_TYPES.TSUnionType
+      ) {
+        return isGrandparentTSTypeAliasDeclaration(node.parent);
+      }
+
+      return false;
+    }
+
+    /**
+     * Checks if the node parent is a Typescript literal type
+     * @param node the node to be validated.
+     * @returns true if the node parent is a Typescript literal type
+     * @private
+     */
+    function isParentTSLiteralType(node: TSESTree.Node): boolean {
+      return node.parent
+        ? node.parent.type === AST_NODE_TYPES.TSLiteralType
+        : false;
+    }
+
+    /**
+     * Checks if the node is a valid TypeScript numeric literal type.
+     * @param node the node to be validated.
+     * @returns true if the node is a TypeScript numeric literal type.
+     * @private
+     */
+    function isTSNumericLiteralType(node: TSESTree.Node): boolean {
+      // For negative numbers, update the parent node
+      if (
+        node.parent &&
+        node.parent.type === AST_NODE_TYPES.UnaryExpression &&
+        node.parent.operator === '-'
+      ) {
+        node = node.parent;
+      }
+
+      // If the parent node is not a TSLiteralType, early return
+      if (!isParentTSLiteralType(node)) {
+        return false;
+      }
+
+      // If the grandparent is a TSTypeAliasDeclaration, ignore
+      if (isGrandparentTSTypeAliasDeclaration(node)) {
+        return true;
+      }
+
+      // If the grandparent is a TSUnionType and it's parent is a TSTypeAliasDeclaration, ignore
+      if (isGrandparentTSUnionType(node)) {
+        return true;
+      }
+
+      return false;
+    }
+
+    return {
+      Literal(node) {
+        // Check TypeScript specific nodes
+        if (
+          options.ignoreNumericLiteralTypes &&
+          isNumber(node) &&
+          isTSNumericLiteralType(node)
+        ) {
+          return;
+        }
+
+        // Let the base rule deal with the rest
+        rules.Literal(node);
+      },
+    };
+  },
+});
diff --git a/packages/eslint-plugin/tests/rules/no-magic-numbers.test.ts b/packages/eslint-plugin/tests/rules/no-magic-numbers.test.ts
@@ -0,0 +1,134 @@
+import rule from '../../src/rules/no-magic-numbers';
+import { RuleTester } from '../RuleTester';
+
+const ruleTester = new RuleTester({
+  parser: '@typescript-eslint/parser',
+});
+
+ruleTester.run('no-magic-numbers', rule, {
+  valid: [
+    {
+      code: 'const FOO = 10;',
+      options: [{ ignoreNumericLiteralTypes: true }],
+    },
+    {
+      code: 'type Foo = "bar";',
+    },
+    {
+      code: 'type Foo = true;',
+    },
+    {
+      code: 'type Foo = 1;',
+      options: [{ ignoreNumericLiteralTypes: true }],
+    },
+    {
+      code: 'type Foo = -1;',
+      options: [{ ignoreNumericLiteralTypes: true }],
+    },
+    {
+      code: 'type Foo = 1 | 2 | 3;',
+      options: [{ ignoreNumericLiteralTypes: true }],
+    },
+    {
+      code: 'type Foo = 1 | -1;',
+      options: [{ ignoreNumericLiteralTypes: true }],
+    },
+  ],
+
+  invalid: [
+    {
+      code: 'type Foo = 1;',
+      options: [{ ignoreNumericLiteralTypes: false }],
+      errors: [
+        {
+          messageId: 'noMagic',
+          data: {
+            raw: '1',
+          },
+          line: 1,
+          column: 12,
+        },
+      ],
+    },
+    {
+      code: 'type Foo = -1;',
+      options: [{ ignoreNumericLiteralTypes: false }],
+      errors: [
+        {
+          messageId: 'noMagic',
+          data: {
+            raw: '-1',
+          },
+          line: 1,
+          column: 12,
+        },
+      ],
+    },
+    {
+      code: 'type Foo = 1 | 2 | 3;',
+      options: [{ ignoreNumericLiteralTypes: false }],
+      errors: [
+        {
+          messageId: 'noMagic',
+          data: {
+            raw: '1',
+          },
+          line: 1,
+          column: 12,
+        },
+        {
+          messageId: 'noMagic',
+          data: {
+            raw: '2',
+          },
+          line: 1,
+          column: 16,
+        },
+        {
+          messageId: 'noMagic',
+          data: {
+            raw: '3',
+          },
+          line: 1,
+          column: 20,
+        },
+      ],
+    },
+    {
+      code: 'type Foo = 1 | -1;',
+      options: [{ ignoreNumericLiteralTypes: false }],
+      errors: [
+        {
+          messageId: 'noMagic',
+          data: {
+            raw: '1',
+          },
+          line: 1,
+          column: 12,
+        },
+        {
+          messageId: 'noMagic',
+          data: {
+            raw: '-1',
+          },
+          line: 1,
+          column: 16,
+        },
+      ],
+    },
+    {
+      code: 'interface Foo { bar: 1; }',
+      options: [{ ignoreNumericLiteralTypes: true }],
+      errors: [
+        {
+          messageId: 'noMagic',
+          data: {
+            raw: '1',
+          },
+          line: 1,
+          column: 22,
+        },
+      ],
+    },
+  ],
+});
diff --git a/packages/eslint-plugin/typings/eslint-rules.d.ts b/packages/eslint-plugin/typings/eslint-rules.d.ts
@@ -174,6 +174,28 @@ declare module 'eslint/lib/rules/no-implicit-globals' {
   export = rule;
 }
 
+declare module 'eslint/lib/rules/no-magic-numbers' {
+  import { TSESTree } from '@typescript-eslint/typescript-estree';
+  import RuleModule from 'ts-eslint';
+
+  const rule: RuleModule<
+    'noMagic',
+    [
+      {
+        ignore?: string[];
+        ignoreArrayIndexes?: boolean;
+        enforceConst?: boolean;
+        detectObjects?: boolean;
+        ignoreNumericLiteralTypes?: boolean;
+      }
+    ],
+    {
+      Literal(node: TSESTree.Literal): void;
+    }
+  >;
+  export = rule;
+}
+
 declare module 'eslint/lib/rules/no-redeclare' {
   import { TSESTree } from '@typescript-eslint/typescript-estree';
   import RuleModule from 'ts-eslint';