From 8a5cb0d418338c48d370c493048e7b579d548e7a Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Mon, 16 Sep 2024 20:59:55 -0500 Subject: [PATCH 01/11] properly handle everything --- packages/eslint-plugin/src/util/misc.ts | 13 +++++-------- .../rules/explicit-module-boundary-types.test.ts | 3 ++- .../rules/prefer-promise-reject-errors.test.ts | 1 + 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index aeed9f96d039..450fc5aa5890 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -242,7 +242,7 @@ type NodeWithKey = function getStaticMemberAccessValue( node: NodeWithKey, { sourceCode }: RuleContext, -): string | undefined { +): string | null { const key = node.type === AST_NODE_TYPES.MemberExpression ? node.property : node.key; if (!node.computed) { @@ -250,12 +250,9 @@ function getStaticMemberAccessValue( ? `${key.value}` : (key as TSESTree.Identifier | TSESTree.PrivateIdentifier).name; } - const value = getStaticValue(key, sourceCode.getScope(node))?.value as - | string - | number - | null - | undefined; - return value == null ? undefined : `${value}`; + const result = getStaticValue(key, sourceCode.getScope(node)); + // we must use `String(...)` rather than template literal interpolation, because interpolation throws a runtime error if `value` is a `symbol` + return result && String(result.value); } /** @@ -268,7 +265,7 @@ const isStaticMemberAccessOfValue = ( context: RuleContext, ...values: string[] ): boolean => - (values as (string | undefined)[]).includes( + (values as (string | null)[]).includes( getStaticMemberAccessValue(memberExpression, context), ); diff --git a/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts b/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts index eda213576ff8..3bff869ae8b1 100644 --- a/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts +++ b/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts @@ -407,6 +407,7 @@ export class Test { 'method'() {} ['prop']() {} [\`prop\`]() {} + [null]() {} [\`\${v}\`](): void {} foo = () => { @@ -416,7 +417,7 @@ export class Test { `, options: [ { - allowedNames: ['prop', 'method', 'foo'], + allowedNames: ['prop', 'method', 'null', 'foo'], }, ], }, diff --git a/packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts b/packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts index d890886eff15..5c020813bb01 100644 --- a/packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts @@ -266,6 +266,7 @@ ruleTester.run('prefer-promise-reject-errors', rule, { declare const foo: PromiseConstructor; foo.reject(new Error()); `, + 'console[Symbol.iterator]();', ], invalid: [ { From 0c863c199189af29161405185956ba691414139b Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Mon, 16 Sep 2024 21:02:17 -0500 Subject: [PATCH 02/11] reformat comment --- packages/eslint-plugin/src/util/misc.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index 450fc5aa5890..5b4943c1a115 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -251,7 +251,8 @@ function getStaticMemberAccessValue( : (key as TSESTree.Identifier | TSESTree.PrivateIdentifier).name; } const result = getStaticValue(key, sourceCode.getScope(node)); - // we must use `String(...)` rather than template literal interpolation, because interpolation throws a runtime error if `value` is a `symbol` + /* we must use `String(...)` rather than template literal interpolation, because interpolation throws a runtime error + if `value` is a `symbol` */ return result && String(result.value); } From d88ad418285bd8fe358f2a46724f3f6455e75be0 Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Mon, 16 Sep 2024 21:04:16 -0500 Subject: [PATCH 03/11] update comment --- packages/eslint-plugin/src/util/misc.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index 5b4943c1a115..cfc1f06dba44 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -251,8 +251,9 @@ function getStaticMemberAccessValue( : (key as TSESTree.Identifier | TSESTree.PrivateIdentifier).name; } const result = getStaticValue(key, sourceCode.getScope(node)); - /* we must use `String(...)` rather than template literal interpolation, because interpolation throws a runtime error - if `value` is a `symbol` */ + /* If a value is returned, coerce it to a string, because that's what JavaScript does with any data type in a + `MemberAccess`. We must use `String(...)` rather than template literal interpolation, because interpolation throws + a runtime error if `result.value` is a `symbol` */ return result && String(result.value); } From 60df6250cdcccb797ccca2ee9774b48ceb88e7f4 Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Mon, 16 Sep 2024 21:11:40 -0500 Subject: [PATCH 04/11] add a test more similar to the original issue --- .../tests/rules/prefer-promise-reject-errors.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts b/packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts index 5c020813bb01..e78575881255 100644 --- a/packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts @@ -267,6 +267,14 @@ ruleTester.run('prefer-promise-reject-errors', rule, { foo.reject(new Error()); `, 'console[Symbol.iterator]();', + ` + class A { + a = []; + [Symbol.iterator]() { + return this.a[Symbol.iterator](); + } + } + `, ], invalid: [ { From f9c243dbe65357266c0c1802e3289dec39362e48 Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Tue, 17 Sep 2024 07:20:56 -0500 Subject: [PATCH 05/11] be more correct, less practical, with symbols --- .../src/rules/class-literal-property-style.ts | 6 ++---- .../src/rules/class-methods-use-this.ts | 4 +++- .../util/isArrayMethodCallWithPredicate.ts | 5 ++++- packages/eslint-plugin/src/util/misc.ts | 20 ++++++++++++------- 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin/src/rules/class-literal-property-style.ts b/packages/eslint-plugin/src/rules/class-literal-property-style.ts index 28d4a5cc78ba..8bc274b1974b 100644 --- a/packages/eslint-plugin/src/rules/class-literal-property-style.ts +++ b/packages/eslint-plugin/src/rules/class-literal-property-style.ts @@ -25,7 +25,7 @@ interface NodeWithModifiers { interface PropertiesInfo { properties: TSESTree.PropertyDefinition[]; - excludeSet: Set; + excludeSet: Set; } const printNodeModifiers = ( @@ -132,9 +132,7 @@ export default createRule({ const { excludeSet } = propertiesInfoStack[propertiesInfoStack.length - 1]; - const name = - getStaticStringValue(node.property) ?? - context.sourceCode.getText(node.property); + const name = getStaticMemberAccessValue(node, context); if (name) { excludeSet.add(name); diff --git a/packages/eslint-plugin/src/rules/class-methods-use-this.ts b/packages/eslint-plugin/src/rules/class-methods-use-this.ts index 6e27a6229f12..4b369d7a51be 100644 --- a/packages/eslint-plugin/src/rules/class-methods-use-this.ts +++ b/packages/eslint-plugin/src/rules/class-methods-use-this.ts @@ -184,7 +184,9 @@ export default createRule({ node.key.type === AST_NODE_TYPES.PrivateIdentifier ? '#' : ''; const name = getStaticMemberAccessValue(node, context); - return !exceptMethods.has(hashIfNeeded + (name ?? '')); + return ( + typeof name !== 'string' || !exceptMethods.has(hashIfNeeded + name) + ); } /** diff --git a/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts b/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts index 746e9003722c..dfcd6ef26ab0 100644 --- a/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts +++ b/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts @@ -30,7 +30,10 @@ export function isArrayMethodCallWithPredicate( const staticAccessValue = getStaticMemberAccessValue(node.callee, context); - if (!staticAccessValue || !ARRAY_PREDICATE_FUNCTIONS.has(staticAccessValue)) { + if ( + typeof staticAccessValue !== 'string' || + !ARRAY_PREDICATE_FUNCTIONS.has(staticAccessValue) + ) { return false; } diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index cfc1f06dba44..219ef3ad380d 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -242,7 +242,7 @@ type NodeWithKey = function getStaticMemberAccessValue( node: NodeWithKey, { sourceCode }: RuleContext, -): string | null { +): string | symbol | undefined { const key = node.type === AST_NODE_TYPES.MemberExpression ? node.property : node.key; if (!node.computed) { @@ -251,10 +251,16 @@ function getStaticMemberAccessValue( : (key as TSESTree.Identifier | TSESTree.PrivateIdentifier).name; } const result = getStaticValue(key, sourceCode.getScope(node)); - /* If a value is returned, coerce it to a string, because that's what JavaScript does with any data type in a - `MemberAccess`. We must use `String(...)` rather than template literal interpolation, because interpolation throws - a runtime error if `result.value` is a `symbol` */ - return result && String(result.value); + if (!result) { + return; + } + const { value } = result; + return typeof value === 'symbol' + ? value + : /* intentional interpolation of an `unknown` value, because any non-Symbol value is coerced to a string by + JavaScript when in a `MemberAccess`. */ + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + `${value}`; } /** @@ -265,9 +271,9 @@ function getStaticMemberAccessValue( const isStaticMemberAccessOfValue = ( memberExpression: NodeWithKey, context: RuleContext, - ...values: string[] + ...values: (string | symbol)[] ): boolean => - (values as (string | null)[]).includes( + (values as (string | symbol | undefined)[]).includes( getStaticMemberAccessValue(memberExpression, context), ); From 9338c16369050a27116fad9491434502cf450184 Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Tue, 17 Sep 2024 12:33:47 -0500 Subject: [PATCH 06/11] add jsdoc --- packages/eslint-plugin/src/util/misc.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index 219ef3ad380d..2cc9fa46e89f 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -239,6 +239,12 @@ type NodeWithKey = | TSESTree.PropertyDefinition | TSESTree.TSAbstractMethodDefinition | TSESTree.TSAbstractPropertyDefinition; + +/** + * Gets the key being accessed or declared, such as `value` in + * `x.value`, `x['value']`, + * or even `const v = 'value'; x[v]` (or optional variants thereof). + */ function getStaticMemberAccessValue( node: NodeWithKey, { sourceCode }: RuleContext, @@ -265,8 +271,8 @@ function getStaticMemberAccessValue( /** * Answers whether the member expression looks like - * `x.memberName`, `x['memberName']`, - * or even `const mn = 'memberName'; x[mn]` (or optional variants thereof). + * `x.value`, `x['value']`, + * or even `const v = 'value'; x[v]` (or optional variants thereof). */ const isStaticMemberAccessOfValue = ( memberExpression: NodeWithKey, From 8abca66eedc867f53fb587d3b35db04094394657 Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Tue, 17 Sep 2024 12:43:50 -0500 Subject: [PATCH 07/11] clean up a couple things --- .../src/util/isArrayMethodCallWithPredicate.ts | 7 ++----- packages/eslint-plugin/src/util/misc.ts | 11 +++-------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts b/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts index dfcd6ef26ab0..39e46d9dda39 100644 --- a/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts +++ b/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts @@ -9,7 +9,7 @@ import * as tsutils from 'ts-api-utils'; import { getStaticMemberAccessValue } from './misc'; -const ARRAY_PREDICATE_FUNCTIONS = new Set([ +const ARRAY_PREDICATE_FUNCTIONS = new Set([ 'filter', 'find', 'findIndex', @@ -30,10 +30,7 @@ export function isArrayMethodCallWithPredicate( const staticAccessValue = getStaticMemberAccessValue(node.callee, context); - if ( - typeof staticAccessValue !== 'string' || - !ARRAY_PREDICATE_FUNCTIONS.has(staticAccessValue) - ) { + if (!ARRAY_PREDICATE_FUNCTIONS.has(staticAccessValue)) { return false; } diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index 2cc9fa46e89f..85748f9755d7 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -253,20 +253,15 @@ function getStaticMemberAccessValue( node.type === AST_NODE_TYPES.MemberExpression ? node.property : node.key; if (!node.computed) { return key.type === AST_NODE_TYPES.Literal - ? `${key.value}` + ? String(key.value) : (key as TSESTree.Identifier | TSESTree.PrivateIdentifier).name; } const result = getStaticValue(key, sourceCode.getScope(node)); if (!result) { - return; + return undefined; } const { value } = result; - return typeof value === 'symbol' - ? value - : /* intentional interpolation of an `unknown` value, because any non-Symbol value is coerced to a string by - JavaScript when in a `MemberAccess`. */ - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - `${value}`; + return typeof value === 'symbol' ? value : String(value); } /** From 2f1b690af94080a9e27f9c88db1760b8d73758dc Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Tue, 17 Sep 2024 17:25:14 -0500 Subject: [PATCH 08/11] refactor to not use type assertion --- packages/eslint-plugin/src/util/misc.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index 85748f9755d7..fa5f8398bc2e 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -2,8 +2,7 @@ * @fileoverview Really small utility functions that didn't deserve their own files */ import { requiresQuoting } from '@typescript-eslint/type-utils'; -import type { TSESLint, TSESTree } from '@typescript-eslint/utils'; -import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES, TSESLint, TSESTree } from '@typescript-eslint/utils'; import type { RuleContext } from '@typescript-eslint/utils/ts-eslint'; import * as ts from 'typescript'; @@ -251,10 +250,15 @@ function getStaticMemberAccessValue( ): string | symbol | undefined { const key = node.type === AST_NODE_TYPES.MemberExpression ? node.property : node.key; - if (!node.computed) { - return key.type === AST_NODE_TYPES.Literal - ? String(key.value) - : (key as TSESTree.Identifier | TSESTree.PrivateIdentifier).name; + const { type } = key; + if (type === AST_NODE_TYPES.Literal) { + return String(key.value); + } + if ( + type === AST_NODE_TYPES.Identifier || + type === AST_NODE_TYPES.PrivateIdentifier + ) { + return key.name; } const result = getStaticValue(key, sourceCode.getScope(node)); if (!result) { From 33bf910b2dd3c0c60b0c4eaf769e924550e4aa9c Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Tue, 17 Sep 2024 17:54:06 -0500 Subject: [PATCH 09/11] fix comment --- packages/eslint-plugin/src/util/misc.ts | 51 ++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index fa5f8398bc2e..f85ce4ac2b4c 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -240,9 +240,49 @@ type NodeWithKey = | TSESTree.TSAbstractPropertyDefinition; /** - * Gets the key being accessed or declared, such as `value` in - * `x.value`, `x['value']`, - * or even `const v = 'value'; x[v]` (or optional variants thereof). + * Gets a member being accessed or declared if its value can be determined statically, and + * resolves it to the string or symbol value that will be used as the actual member + * access key at runtime. Otherwise, returns `undefined`. + * + * ```ts + * x.member // returns 'member' + * ^^^^^^^^ + * + * x?.member // returns 'member' (optional chaining is treated the same) + * ^^^^^^^^^ + * + * x['value'] // returns 'value' + * ^^^^^^^^^^ + * + * x[Math.random()] // returns undefined (not a static value) + * ^^^^^^^^^^^^^^^^ + * + * arr[0] // returns '0' (NOT 0) + * ^^^^^^ + * + * arr[0n] // returns '0' (NOT 0n) + * ^^^^^^^ + * + * const s = Symbol.for('symbolName') + * x[s] // returns `Symbol.for('symbolName')` (since it's a static/global symbol) + * ^^^^ + * + * const us = Symbol('symbolName') + * x[us] // returns undefined (since it's a unique symbol, so not statically analyzable) + * ^^^^^ + * + * var object = { + * 1234: '4567', // returns '1234' (NOT 1234) + * ^^^^^^^^^^^^ + * method() { } // returns 'method' + * ^^^^^^^^^^^^ + * } + * + * class WithMembers { + * foo: string // returns 'foo' + * ^^^^^^^^^^^ + * } + * ``` */ function getStaticMemberAccessValue( node: NodeWithKey, @@ -255,8 +295,9 @@ function getStaticMemberAccessValue( return String(key.value); } if ( - type === AST_NODE_TYPES.Identifier || - type === AST_NODE_TYPES.PrivateIdentifier + !node.computed && + (type === AST_NODE_TYPES.Identifier || + type === AST_NODE_TYPES.PrivateIdentifier) ) { return key.name; } From 515ebbfb1a532ff9ee164e71a232f0cd9c91e2da Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Tue, 17 Sep 2024 18:07:41 -0500 Subject: [PATCH 10/11] lint --- .../eslint-plugin/src/rules/class-literal-property-style.ts | 1 - packages/eslint-plugin/src/util/misc.ts | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/class-literal-property-style.ts b/packages/eslint-plugin/src/rules/class-literal-property-style.ts index 8bc274b1974b..9f02142f4d6e 100644 --- a/packages/eslint-plugin/src/rules/class-literal-property-style.ts +++ b/packages/eslint-plugin/src/rules/class-literal-property-style.ts @@ -4,7 +4,6 @@ import { AST_NODE_TYPES } from '@typescript-eslint/utils'; import { createRule, getStaticMemberAccessValue, - getStaticStringValue, isAssignee, isFunction, isStaticMemberAccessOfValue, diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index f85ce4ac2b4c..cf47ef9156d2 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -2,7 +2,8 @@ * @fileoverview Really small utility functions that didn't deserve their own files */ import { requiresQuoting } from '@typescript-eslint/type-utils'; -import { AST_NODE_TYPES, TSESLint, TSESTree } from '@typescript-eslint/utils'; +import type { TSESLint, TSESTree } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; import type { RuleContext } from '@typescript-eslint/utils/ts-eslint'; import * as ts from 'typescript'; From 54ababe445850dc422b799d54a7d66b99805ad73 Mon Sep 17 00:00:00 2001 From: Abraham Guo Date: Fri, 20 Sep 2024 20:55:24 -1000 Subject: [PATCH 11/11] remove unnecessary check --- packages/eslint-plugin/src/util/misc.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index cf47ef9156d2..41933dc551c2 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -292,9 +292,6 @@ function getStaticMemberAccessValue( const key = node.type === AST_NODE_TYPES.MemberExpression ? node.property : node.key; const { type } = key; - if (type === AST_NODE_TYPES.Literal) { - return String(key.value); - } if ( !node.computed && (type === AST_NODE_TYPES.Identifier ||