From 852fc3143cd287c396562fb72b6f6b97ad730281 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Wed, 8 Jan 2020 14:30:01 -0800 Subject: [PATCH] fix(eslint-plugin): [no-magic-numbers] handle UnaryExpression for enums (#1415) --- .../src/rules/no-magic-numbers.ts | 245 +++++++++--------- .../tests/rules/no-magic-numbers.test.ts | 74 ++++-- 2 files changed, 169 insertions(+), 150 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-magic-numbers.ts b/packages/eslint-plugin/src/rules/no-magic-numbers.ts index b4791614cda..6576ad4b49f 100644 --- a/packages/eslint-plugin/src/rules/no-magic-numbers.ts +++ b/packages/eslint-plugin/src/rules/no-magic-numbers.ts @@ -55,130 +55,6 @@ export default util.createRule({ 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 union 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 enum member - * @param node the node to be validated. - * @returns true if the node parent is a Typescript enum member - * @private - */ - function isParentTSEnumDeclaration(node: TSESTree.Node): boolean { - return ( - typeof node.parent !== 'undefined' && - node.parent.type === AST_NODE_TYPES.TSEnumMember - ); - } - - /** - * 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; - } - - /** - * Checks if the node parent is a readonly class property - * @param node the node to be validated. - * @returns true if the node parent is a readonly class property - * @private - */ - function isParentTSReadonlyClassProperty(node: TSESTree.Node): boolean { - if ( - node.parent && - node.parent.type === AST_NODE_TYPES.UnaryExpression && - ['-', '+'].includes(node.parent.operator) - ) { - node = node.parent; - } - - if ( - node.parent && - node.parent.type === AST_NODE_TYPES.ClassProperty && - node.parent.readonly - ) { - return true; - } - - return false; - } - return { Literal(node): void { // Check if the node is a TypeScript enum declaration @@ -189,14 +65,17 @@ export default util.createRule({ // Check TypeScript specific nodes for Numeric Literal if ( options.ignoreNumericLiteralTypes && - isNumber(node) && + typeof node.value === 'number' && isTSNumericLiteralType(node) ) { return; } // Check if the node is a readonly class property - if (isNumber(node) && isParentTSReadonlyClassProperty(node)) { + if ( + typeof node.value === 'number' && + isParentTSReadonlyClassProperty(node) + ) { if (options.ignoreReadonlyClassProperties) { return; } @@ -207,8 +86,10 @@ export default util.createRule({ let raw = node.raw; if ( - node.parent && - node.parent.type === AST_NODE_TYPES.UnaryExpression + node.parent?.type === AST_NODE_TYPES.UnaryExpression && + // the base rule only shows the operator for negative numbers + // https://github.com/eslint/eslint/blob/9dfc8501fb1956c90dc11e6377b4cb38a6bea65d/lib/rules/no-magic-numbers.js#L126 + node.parent.operator === '-' ) { fullNumberNode = node.parent; raw = `${node.parent.operator}${node.raw}`; @@ -229,3 +110,111 @@ export default util.createRule({ }; }, }); + +/** + * Gets the true parent of the literal, handling prefixed numbers (-1 / +1) + */ +function getLiteralParent(node: TSESTree.Literal): TSESTree.Node | undefined { + if ( + node.parent?.type === AST_NODE_TYPES.UnaryExpression && + ['-', '+'].includes(node.parent.operator) + ) { + return node.parent.parent; + } + + return node.parent; +} + +/** + * 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?.parent?.type === AST_NODE_TYPES.TSTypeAliasDeclaration; +} + +/** + * 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 union type and its parent is a type alias declaration + * @private + */ +function isGrandparentTSUnionType(node: TSESTree.Node): boolean { + if (node.parent?.parent?.type === AST_NODE_TYPES.TSUnionType) { + return isGrandparentTSTypeAliasDeclaration(node.parent); + } + + return false; +} + +/** + * Checks if the node parent is a Typescript enum member + * @param node the node to be validated. + * @returns true if the node parent is a Typescript enum member + * @private + */ +function isParentTSEnumDeclaration(node: TSESTree.Literal): boolean { + const parent = getLiteralParent(node); + return parent?.type === AST_NODE_TYPES.TSEnumMember; +} + +/** + * 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?.type === AST_NODE_TYPES.TSLiteralType; +} + +/** + * 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, use the parent node + if ( + 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; +} + +/** + * Checks if the node parent is a readonly class property + * @param node the node to be validated. + * @returns true if the node parent is a readonly class property + * @private + */ +function isParentTSReadonlyClassProperty(node: TSESTree.Literal): boolean { + const parent = getLiteralParent(node); + + if (parent?.type === AST_NODE_TYPES.ClassProperty && parent.readonly) { + return true; + } + + return false; +} diff --git a/packages/eslint-plugin/tests/rules/no-magic-numbers.test.ts b/packages/eslint-plugin/tests/rules/no-magic-numbers.test.ts index f65610ea3f4..9c8bdeae10a 100644 --- a/packages/eslint-plugin/tests/rules/no-magic-numbers.test.ts +++ b/packages/eslint-plugin/tests/rules/no-magic-numbers.test.ts @@ -34,11 +34,14 @@ ruleTester.run('no-magic-numbers', rule, { options: [{ ignoreNumericLiteralTypes: true }], }, { - code: 'enum foo { SECOND = 1000 }', - options: [{ ignoreEnums: true }], - }, - { - code: 'enum foo { SECOND = 1000, NUM = "0123456789" }', + code: ` + enum foo { + SECOND = 1000, + NUM = "0123456789", + NEG = -1, + POS = +1, + } + `, options: [{ ignoreEnums: true }], }, { @@ -152,7 +155,14 @@ class Foo { ], }, { - code: 'enum foo { SECOND = 1000 }', + code: ` +enum foo { + SECOND = 1000, + NUM = "0123456789", + NEG = -1, + POS = +1, +} + `, options: [{ ignoreEnums: false }], errors: [ { @@ -160,22 +170,24 @@ class Foo { data: { raw: '1000', }, - line: 1, - column: 21, + line: 3, + column: 12, }, - ], - }, - { - code: 'enum foo { SECOND = 1000, NUM = "0123456789" }', - options: [{ ignoreEnums: false }], - errors: [ { messageId: 'noMagic', data: { - raw: '1000', + raw: '-1', }, - line: 1, - column: 21, + line: 5, + column: 9, + }, + { + messageId: 'noMagic', + data: { + raw: '1', + }, + line: 6, + column: 10, }, ], }, @@ -184,43 +196,61 @@ class Foo { class Foo { readonly A = 1; readonly B = 2; - public static readonly C = 1; - static readonly D = 1; - readonly E = -1; - readonly F = +1; + public static readonly C = 3; + static readonly D = 4; + readonly E = -5; + readonly F = +6; } `, options: [{ ignoreReadonlyClassProperties: false }], errors: [ { messageId: 'noMagic', + data: { + raw: '1', + }, line: 3, column: 16, }, { messageId: 'noMagic', + data: { + raw: '2', + }, line: 4, column: 16, }, { messageId: 'noMagic', + data: { + raw: '3', + }, line: 5, column: 30, }, { messageId: 'noMagic', + data: { + raw: '4', + }, line: 6, column: 23, }, { messageId: 'noMagic', + data: { + raw: '-5', + }, line: 7, column: 16, }, { messageId: 'noMagic', + data: { + raw: '6', + }, line: 8, - column: 16, + column: 17, }, ], },