From e224e60cdada8183be6b44f138eeeda1fb435a2c Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 30 Nov 2024 20:38:32 -0700 Subject: [PATCH 1/9] remove confusing spread element test cases --- .../src/rules/no-base-to-string.ts | 8 +++++-- .../tests/rules/no-base-to-string.test.ts | 24 ++++--------------- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index f7c012eeb21f..1dca360133a3 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -1,4 +1,3 @@ -/* eslint-disable @typescript-eslint/internal/prefer-ast-types-enum */ import type { TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; @@ -218,10 +217,12 @@ export default createRule({ function isBuiltInStringCall(node: TSESTree.CallExpression): boolean { if ( node.callee.type === AST_NODE_TYPES.Identifier && + // eslint-disable-next-line @typescript-eslint/internal/prefer-ast-types-enum node.callee.name === 'String' && node.arguments[0] ) { const scope = context.sourceCode.getScope(node); + // eslint-disable-next-line @typescript-eslint/internal/prefer-ast-types-enum const variable = scope.set.get('String'); return !variable?.defs.length; } @@ -245,7 +246,10 @@ export default createRule({ } }, CallExpression(node: TSESTree.CallExpression): void { - if (isBuiltInStringCall(node)) { + if ( + isBuiltInStringCall(node) && + node.arguments[0].type !== AST_NODE_TYPES.SpreadElement + ) { checkExpression(node.arguments[0]); } }, diff --git a/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts b/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts index df1f3d4a6979..966aab5ab38e 100644 --- a/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts +++ b/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts @@ -135,10 +135,6 @@ tag\`\${{}}\`; "'' += new URL();", "'' += new URLSearchParams();", ` -let numbers = [1, 2, 3]; -String(...a); - `, - ` Number(1); `, { @@ -215,6 +211,11 @@ class Foo {} declare const tuple: [string] & [Foo]; tuple.join(''); `, + // don't bother trying to interpret spread args. + ` +let objects = [{}, {}]; +String(...objects); + `, ], invalid: [ { @@ -277,21 +278,6 @@ tuple.join(''); }, ], }, - { - code: ` -let objects = [{}, {}]; -String(...objects); - `, - errors: [ - { - data: { - certainty: 'will', - name: '...objects', - }, - messageId: 'baseToString', - }, - ], - }, { code: "'' += {};", errors: [ From 1fc3f231b72678a8f214f8b47a9462fddb8b2bef Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 30 Nov 2024 21:04:16 -0700 Subject: [PATCH 2/9] fix intersection checking --- .../src/rules/no-base-to-string.ts | 40 ++++++++++--------- .../tests/rules/no-base-to-string.test.ts | 26 ++++++++++++ 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index 1dca360133a3..721b7ca17a76 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -9,6 +9,7 @@ import { getConstrainedTypeAtLocation, getParserServices, getTypeName, + isTypeAnyType, nullThrows, } from '../util'; @@ -175,11 +176,25 @@ export default createRule({ } function collectToStringCertainty(type: ts.Type): Usefulness { + if (type.isIntersection()) { + return collectIntersectionTypeCertainty(type, collectToStringCertainty); + } + + if (type.isUnion()) { + return collectUnionTypeCertainty(type, collectToStringCertainty); + } + const toString = checker.getPropertyOfType(type, 'toString') ?? checker.getPropertyOfType(type, 'toLocaleString'); - const declarations = toString?.getDeclarations(); - if (!toString || !declarations || declarations.length === 0) { + if (!toString) { + // This is essentially just `any`s. + return Usefulness.Always; + } + + const declarations = toString.getDeclarations(); + if (!declarations || declarations.length === 0) { + // not clear how to reach this. return Usefulness.Always; } @@ -195,23 +210,12 @@ export default createRule({ return Usefulness.Always; } - if ( - declarations.every( - ({ parent }) => - !ts.isInterfaceDeclaration(parent) || parent.name.text !== 'Object', - ) - ) { - return Usefulness.Always; - } - - if (type.isIntersection()) { - return collectIntersectionTypeCertainty(type, collectToStringCertainty); - } + const canBeObjectToString = declarations.some( + ({ parent }) => + ts.isInterfaceDeclaration(parent) && parent.name.text === 'Object', + ); - if (!type.isUnion()) { - return Usefulness.Never; - } - return collectUnionTypeCertainty(type, collectToStringCertainty); + return canBeObjectToString ? Usefulness.Never : Usefulness.Always; } function isBuiltInStringCall(node: TSESTree.CallExpression): boolean { diff --git a/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts b/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts index 966aab5ab38e..1c33ccd27a21 100644 --- a/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts +++ b/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts @@ -216,6 +216,32 @@ tuple.join(''); let objects = [{}, {}]; String(...objects); `, + // https://github.com/typescript-eslint/typescript-eslint/issues/8585 + ` +type Constructable = abstract new (...args: any[]) => Entity; + +interface GuildChannel { + toString(): \`<#\${string}>\`; +} + +declare const foo: Constructable; +class ExtendedGuildChannel extends foo {} +declare const bb: ExtendedGuildChannel; +bb.toString(); + `, + // https://github.com/typescript-eslint/typescript-eslint/issues/8585 with intersection order reversed. + ` +type Constructable = abstract new (...args: any[]) => Entity; + +interface GuildChannel { + toString(): \`<#\${string}>\`; +} + +declare const foo: Constructable<{ bar: 1 } & GuildChannel>; +class ExtendedGuildChannel extends foo {} +declare const bb: ExtendedGuildChannel; +bb.toString(); + `, ], invalid: [ { From 01f6a24835f2fc7f5dd7be85c314a94b7fc42ced Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 30 Nov 2024 22:30:30 -0700 Subject: [PATCH 3/9] fix for old versions of TS --- .../src/rules/no-base-to-string.ts | 46 +++++++++----- .../tests/rules/no-base-to-string.test.ts | 61 ++++++++++++++++++- 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index 721b7ca17a76..e8bf4dd89a14 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -10,6 +10,7 @@ import { getParserServices, getTypeName, isTypeAnyType, + isTypeUnknownType, nullThrows, } from '../util'; @@ -176,10 +177,27 @@ export default createRule({ } function collectToStringCertainty(type: ts.Type): Usefulness { + if ((tsutils.isTypeParameter as (t: ts.Type) => boolean)(type)) { + const constraint = type.getConstraint(); + if (constraint) { + return collectToStringCertainty(constraint); + } + // unconstrained generic means `unknown` + return Usefulness.Always; + } + if (type.isIntersection()) { return collectIntersectionTypeCertainty(type, collectToStringCertainty); } + // the Boolean type definition missing toString() + if ( + type.flags & ts.TypeFlags.Boolean || + type.flags & ts.TypeFlags.BooleanLiteral + ) { + return Usefulness.Always; + } + if (type.isUnion()) { return collectUnionTypeCertainty(type, collectToStringCertainty); } @@ -188,21 +206,18 @@ export default createRule({ checker.getPropertyOfType(type, 'toString') ?? checker.getPropertyOfType(type, 'toLocaleString'); if (!toString) { - // This is essentially just `any`s. + // e.g. any/unknown return Usefulness.Always; } const declarations = toString.getDeclarations(); - if (!declarations || declarations.length === 0) { - // not clear how to reach this. - return Usefulness.Always; - } - // Patch for old version TypeScript, the Boolean type definition missing toString() - if ( - type.flags & ts.TypeFlags.Boolean || - type.flags & ts.TypeFlags.BooleanLiteral - ) { + if (!(declarations?.length === 1)) { + // If there are multiple declarations, at least one of them must not be + // the default object toString. + // + // This may only matter for older versions of TS + // see https://github.com/typescript-eslint/typescript-eslint/issues/8585 return Usefulness.Always; } @@ -210,12 +225,11 @@ export default createRule({ return Usefulness.Always; } - const canBeObjectToString = declarations.some( - ({ parent }) => - ts.isInterfaceDeclaration(parent) && parent.name.text === 'Object', - ); - - return canBeObjectToString ? Usefulness.Never : Usefulness.Always; + const declaration = declarations[0]; + const isBaseToString = + ts.isInterfaceDeclaration(declaration.parent) && + declaration.parent.name.text === 'Object'; + return isBaseToString ? Usefulness.Never : Usefulness.Always; } function isBuiltInStringCall(node: TSESTree.CallExpression): boolean { diff --git a/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts b/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts index 1c33ccd27a21..30156c528fd3 100644 --- a/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts +++ b/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts @@ -242,6 +242,15 @@ class ExtendedGuildChannel extends foo {} declare const bb: ExtendedGuildChannel; bb.toString(); `, + ` +function foo(x: T) { + String(x); +} + `, + ` +declare const u: unknown; +String(u); + `, ], invalid: [ { @@ -694,13 +703,63 @@ declare const foo: Bar & Foo; errors: [ { data: { - certainty: 'will', + certainty: 'may', name: 'array', }, messageId: 'baseArrayJoin', }, ], }, + { + code: ` + type Bar = Record; + function foo(array: T[]) { + array[0].toString(); + } + `, + errors: [ + { + data: { + certainty: 'may', + name: 'array[0]', + }, + messageId: 'baseToString', + }, + ], + }, + { + code: ` + type Bar = Record; + function foo(value: T) { + value.toString(); + } + `, + errors: [ + { + data: { + certainty: 'may', + name: 'value', + }, + messageId: 'baseToString', + }, + ], + }, + { + code: ` +type Bar = Record; +declare const foo: Bar | string; +foo.toString(); + `, + errors: [ + { + data: { + certainty: 'may', + name: 'foo', + }, + messageId: 'baseToString', + }, + ], + }, { code: ` type Bar = Record; From 132bb4f325426954290c764224e98696b15c1efd Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 30 Nov 2024 22:34:33 -0700 Subject: [PATCH 4/9] lintfix --- packages/eslint-plugin/src/rules/no-base-to-string.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index e8bf4dd89a14..99a75f3bf07c 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -9,8 +9,6 @@ import { getConstrainedTypeAtLocation, getParserServices, getTypeName, - isTypeAnyType, - isTypeUnknownType, nullThrows, } from '../util'; From 2309fe3e04d45d12cea2217945479f7092b6849e Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 30 Nov 2024 22:55:24 -0700 Subject: [PATCH 5/9] readability --- packages/eslint-plugin/src/rules/no-base-to-string.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index 99a75f3bf07c..e8c09e5c08e6 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -210,7 +210,7 @@ export default createRule({ const declarations = toString.getDeclarations(); - if (!(declarations?.length === 1)) { + if (declarations == null || declarations.length !== 1) { // If there are multiple declarations, at least one of them must not be // the default object toString. // From d2b7ca74cbbb1c9e20c86e2c53c698c31ea4708a Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 30 Nov 2024 22:57:02 -0700 Subject: [PATCH 6/9] isTypeParameter comment --- packages/eslint-plugin/src/rules/no-base-to-string.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index e8c09e5c08e6..4b4dcbc3e15d 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -175,6 +175,7 @@ export default createRule({ } function collectToStringCertainty(type: ts.Type): Usefulness { + // https://github.com/JoshuaKGoldberg/ts-api-utils/issues/382 if ((tsutils.isTypeParameter as (t: ts.Type) => boolean)(type)) { const constraint = type.getConstraint(); if (constraint) { From 2a943bd021c678ff6a857b2138ad331838fdef62 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 30 Nov 2024 22:58:58 -0700 Subject: [PATCH 7/9] derp --- packages/eslint-plugin/src/rules/no-base-to-string.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index 4b4dcbc3e15d..0537a60fa288 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -189,6 +189,10 @@ export default createRule({ return collectIntersectionTypeCertainty(type, collectToStringCertainty); } + if (type.isUnion()) { + return collectUnionTypeCertainty(type, collectToStringCertainty); + } + // the Boolean type definition missing toString() if ( type.flags & ts.TypeFlags.Boolean || @@ -197,10 +201,6 @@ export default createRule({ return Usefulness.Always; } - if (type.isUnion()) { - return collectUnionTypeCertainty(type, collectToStringCertainty); - } - const toString = checker.getPropertyOfType(type, 'toString') ?? checker.getPropertyOfType(type, 'toLocaleString'); From dfd3b705cfe655fd3c0634931794f2ada98754c6 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 30 Nov 2024 23:04:53 -0700 Subject: [PATCH 8/9] ignored types --- .../src/rules/no-base-to-string.ts | 24 +++++++++---------- .../tests/rules/no-base-to-string.test.ts | 8 +++++++ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index 0537a60fa288..951a9d78be5f 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -185,14 +185,6 @@ export default createRule({ return Usefulness.Always; } - if (type.isIntersection()) { - return collectIntersectionTypeCertainty(type, collectToStringCertainty); - } - - if (type.isUnion()) { - return collectUnionTypeCertainty(type, collectToStringCertainty); - } - // the Boolean type definition missing toString() if ( type.flags & ts.TypeFlags.Boolean || @@ -201,6 +193,18 @@ export default createRule({ return Usefulness.Always; } + if (ignoredTypeNames.includes(getTypeName(checker, type))) { + return Usefulness.Always; + } + + if (type.isIntersection()) { + return collectIntersectionTypeCertainty(type, collectToStringCertainty); + } + + if (type.isUnion()) { + return collectUnionTypeCertainty(type, collectToStringCertainty); + } + const toString = checker.getPropertyOfType(type, 'toString') ?? checker.getPropertyOfType(type, 'toLocaleString'); @@ -220,10 +224,6 @@ export default createRule({ return Usefulness.Always; } - if (ignoredTypeNames.includes(getTypeName(checker, type))) { - return Usefulness.Always; - } - const declaration = declarations[0]; const isBaseToString = ts.isInterfaceDeclaration(declaration.parent) && diff --git a/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts b/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts index 30156c528fd3..5b73b1e371e5 100644 --- a/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts +++ b/packages/eslint-plugin/tests/rules/no-base-to-string.test.ts @@ -141,6 +141,14 @@ Number(1); code: 'String(/regex/);', options: [{ ignoredTypeNames: ['RegExp'] }], }, + { + code: ` +type Foo = { a: string } | { b: string }; +declare const foo: Foo; +String(foo); + `, + options: [{ ignoredTypeNames: ['Foo'] }], + }, ` function String(value) { return value; From f323fdc8b7deb88052495c8d2b1324ed218a71f8 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 1 Dec 2024 00:18:36 -0700 Subject: [PATCH 9/9] expression --- packages/eslint-plugin/src/rules/no-base-to-string.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/no-base-to-string.ts b/packages/eslint-plugin/src/rules/no-base-to-string.ts index 951a9d78be5f..b681f820a3b0 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -68,7 +68,7 @@ export default createRule({ const checker = services.program.getTypeChecker(); const ignoredTypeNames = option.ignoredTypeNames ?? []; - function checkExpression(node: TSESTree.Node, type?: ts.Type): void { + function checkExpression(node: TSESTree.Expression, type?: ts.Type): void { if (node.type === AST_NODE_TYPES.Literal) { return; }