From f6e51182bfbb24d92348a1c5ce76465d34ee0d41 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 8 Mar 2019 10:05:54 -0800 Subject: [PATCH] fix(eslint-plugin): fix false positives for adjacent-overload-signatures regarding computed property names (#340) Currently, the name of a method definition or a class property is always assumed to be a literal or an identifier. However, this fails to account for computed property names. In those cases, we currently sometimes give the name for a method definition with a computed name as `undefined`, which as a result causes methods with differing computed property names to be marked as overloads of each other. In this PR, we slice the text of the computed property name out to handle these cases. --- .../src/rules/adjacent-overload-signatures.ts | 4 ++- .../rules/explicit-member-accessibility.ts | 3 +- .../eslint-plugin/src/rules/member-naming.ts | 4 ++- .../src/rules/member-ordering.ts | 4 ++- packages/eslint-plugin/src/util/misc.ts | 29 +++++++++++++++++++ .../adjacent-overload-signatures.test.ts | 5 ++++ packages/eslint-plugin/typings/ts-eslint.d.ts | 1 + .../src/ts-estree/ts-estree.ts | 2 +- 8 files changed, 47 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/src/rules/adjacent-overload-signatures.ts b/packages/eslint-plugin/src/rules/adjacent-overload-signatures.ts index 7007b1c3bdf..8e864c9c4a2 100644 --- a/packages/eslint-plugin/src/rules/adjacent-overload-signatures.ts +++ b/packages/eslint-plugin/src/rules/adjacent-overload-signatures.ts @@ -26,6 +26,8 @@ export default util.createRule({ }, defaultOptions: [], create(context) { + const sourceCode = context.getSourceCode(); + /** * Gets the name of the member being processed. * @param member the member being processed. @@ -57,7 +59,7 @@ export default util.createRule({ case AST_NODE_TYPES.TSConstructSignatureDeclaration: return 'new'; case AST_NODE_TYPES.MethodDefinition: - return util.getNameFromPropertyName(member.key); + return util.getNameFromClassMember(member, sourceCode); } return null; diff --git a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts index afae022c262..66029faa501 100644 --- a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts +++ b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts @@ -20,6 +20,7 @@ export default util.createRule({ }, defaultOptions: [], create(context) { + const sourceCode = context.getSourceCode(); /** * Checks if a method declaration has an accessibility modifier. * @param methodDefinition The node representing a MethodDefinition. @@ -36,7 +37,7 @@ export default util.createRule({ messageId: 'missingAccessibility', data: { type: 'method definition', - name: util.getNameFromPropertyName(methodDefinition.key), + name: util.getNameFromClassMember(methodDefinition, sourceCode), }, }); } diff --git a/packages/eslint-plugin/src/rules/member-naming.ts b/packages/eslint-plugin/src/rules/member-naming.ts index 23c14c8b39c..2efdf6ea307 100644 --- a/packages/eslint-plugin/src/rules/member-naming.ts +++ b/packages/eslint-plugin/src/rules/member-naming.ts @@ -51,6 +51,8 @@ export default util.createRule({ }, defaultOptions: [{}], create(context, [config]) { + const sourceCode = context.getSourceCode(); + const conventions = (Object.keys(config) as Modifiers[]).reduce< Config >((acc, accessibility) => { @@ -69,7 +71,7 @@ export default util.createRule({ function validateName( node: TSESTree.MethodDefinition | TSESTree.ClassProperty, ): void { - const name = util.getNameFromPropertyName(node.key); + const name = util.getNameFromClassMember(node, sourceCode); const accessibility: Modifiers = node.accessibility || 'public'; const convention = conventions[accessibility]; diff --git a/packages/eslint-plugin/src/rules/member-ordering.ts b/packages/eslint-plugin/src/rules/member-ordering.ts index dff4752bab4..f3ae6a439bd 100644 --- a/packages/eslint-plugin/src/rules/member-ordering.ts +++ b/packages/eslint-plugin/src/rules/member-ordering.ts @@ -164,6 +164,8 @@ export default util.createRule({ }, ], create(context, [options]) { + const sourceCode = context.getSourceCode(); + const functionExpressions = [ AST_NODE_TYPES.FunctionExpression, AST_NODE_TYPES.ArrowFunctionExpression, @@ -213,7 +215,7 @@ export default util.createRule({ case AST_NODE_TYPES.MethodDefinition: return node.kind === 'constructor' ? 'constructor' - : util.getNameFromPropertyName(node.key); + : util.getNameFromClassMember(node, sourceCode); case AST_NODE_TYPES.TSConstructSignatureDeclaration: return 'new'; default: diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index dbaaebc3f7a..ab56cd4e694 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -4,6 +4,7 @@ import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/typescript-estree'; import RuleModule from 'ts-eslint'; +import { SourceCode } from 'ts-eslint'; /** * Check if the context file name is *.ts or *.tsx @@ -63,3 +64,31 @@ export function getNameFromPropertyName( } return `${propertyName.value}`; } + +/** + * Gets a string name representation of the name of the given MethodDefinition + * or ClassProperty node, with handling for computed property names. + */ +export function getNameFromClassMember( + methodDefinition: TSESTree.MethodDefinition | TSESTree.ClassProperty, + sourceCode: SourceCode, +): string { + if (keyCanBeReadAsPropertyName(methodDefinition.key)) { + return getNameFromPropertyName(methodDefinition.key); + } + + return sourceCode.text.slice(...methodDefinition.key.range); +} + +/** + * This covers both actual property names, as well as computed properties that are either + * an identifier or a literal at the top level. + */ +function keyCanBeReadAsPropertyName( + node: TSESTree.Expression, +): node is TSESTree.PropertyName { + return ( + node.type === AST_NODE_TYPES.Literal || + node.type === AST_NODE_TYPES.Identifier + ); +} diff --git a/packages/eslint-plugin/tests/rules/adjacent-overload-signatures.test.ts b/packages/eslint-plugin/tests/rules/adjacent-overload-signatures.test.ts index 01680fef838..c040362a128 100644 --- a/packages/eslint-plugin/tests/rules/adjacent-overload-signatures.test.ts +++ b/packages/eslint-plugin/tests/rules/adjacent-overload-signatures.test.ts @@ -217,6 +217,11 @@ class Test { // examples from https://github.com/nzakas/eslint-plugin-typescript/issues/138 'export default function(foo : T) {}', 'export default function named(foo : T) {}', + ` +interface Foo { + [Symbol.toStringTag](): void; + [Symbol.iterator](): void; +}`, ], invalid: [ { diff --git a/packages/eslint-plugin/typings/ts-eslint.d.ts b/packages/eslint-plugin/typings/ts-eslint.d.ts index 998c6958342..b757a1b5982 100644 --- a/packages/eslint-plugin/typings/ts-eslint.d.ts +++ b/packages/eslint-plugin/typings/ts-eslint.d.ts @@ -683,6 +683,7 @@ declare module 'ts-eslint' { RuleMetaData, RuleMetaDataDocs, Scope, + SourceCode, }; export default RuleModule; } diff --git a/packages/typescript-estree/src/ts-estree/ts-estree.ts b/packages/typescript-estree/src/ts-estree/ts-estree.ts index 4a9c7b91ae1..f15f50abca3 100644 --- a/packages/typescript-estree/src/ts-estree/ts-estree.ts +++ b/packages/typescript-estree/src/ts-estree/ts-estree.ts @@ -500,7 +500,7 @@ interface LiteralBase extends BaseNode { } interface MethodDefinitionBase extends BaseNode { - key: PropertyName; + key: Expression; value: FunctionExpression | TSEmptyBodyFunctionExpression; computed: boolean; static: boolean;