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): add no-magic-numbers rule #373

Merged
merged 6 commits into from May 8, 2019
Merged
Show file tree
Hide file tree
Changes from 4 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 @@ -131,6 +131,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: | | |
Expand Down
44 changes: 44 additions & 0 deletions 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>
152 changes: 152 additions & 0 deletions 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,
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
},
],
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);
},
};
},
});
134 changes: 134 additions & 0 deletions 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,
},
],
},
],
});
22 changes: 22 additions & 0 deletions packages/eslint-plugin/typings/eslint-rules.d.ts
Expand Up @@ -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';
Expand Down