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): [no-magic-numbers] handle UnaryExpression for enums #1415

Merged
merged 1 commit into from Jan 8, 2020
Merged

fix(eslint-plugin): [no-magic-numbers] handle UnaryExpression for enums #1415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
245 changes: 117 additions & 128 deletions packages/eslint-plugin/src/rules/no-magic-numbers.ts
View file
Open in desktop
Expand Up @@ -55,130 +55,6 @@ export default util.createRule<Options, MessageIds>({
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
Expand All @@ -189,14 +65,17 @@ export default util.createRule<Options, MessageIds>({
// 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;
}
Expand All @@ -207,8 +86,10 @@ export default util.createRule<Options, MessageIds>({
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}`;
Expand All @@ -229,3 +110,111 @@ export default util.createRule<Options, MessageIds>({
};
},
});

/**
* 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 &&
Copy link
Member

@armano2 armano2 Jan 8, 2020 •
edited

Choose a reason for hiding this comment

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

hmm, i was thinking what to do with node.parent?

parent is always defined in eslint context for nodes (with exception for Program).

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we should probably fix the types.
I was considering just adding a new type to ts-estree:

type KnownParent<TNode extends Node, TParent extends Node> = TNode & { parent: TParent };

But that type wouldn't doesn't fix cases like this. It would let us express direct parent selectors like VariableDeclaration > VariableDeclarator, so it's probably a nice thing to add as a non-breaking feature.

I was considering doing this via generics to make it even more explicit. But this only works if we make it non-optional.

interface BaseNode<TParent extends Node = Node> {
  // ...
  parent: TParent;
}

interface VariableDeclarator<TParent extends Node = Node> extends BaseNode<TParent> {}

interface Program extends BaseNode {
  parent?: never;
}

The problem with making it non-optional is that this complicates the converter code because the converter isn't the one that adds the parent, eslint is.

Copy link
Member

@armano2 armano2 Jan 8, 2020 •
edited

Choose a reason for hiding this comment

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

q: what do you think maybe about adding d.ts with augmentation for type in eslint-utils or plugin itself?

import { Node } from "@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree";

declare module '@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree' {
  export interface BaseNode {
    parent: Node;
  }

  export interface Program {
    parent: never;
  }
}

and dropping parent from ts-estree completely?

this is just an idea (should work, but i haven't tested it)

['-', '+'].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;
}