Skip to content

fix(eslint-plugin): [no-base-to-string] handle more robustly when multiple toString() declarations are present for a type #10432

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

Merged
merged 9 commits into from
Dec 7, 2024
Merged

fix(eslint-plugin): [no-base-to-string] handle more robustly when multiple toString() declarations are present for a type #10432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e224e60
remove confusing spread element test cases
kirkwaiblinger Dec 1, 2024
1fc3f23
fix intersection checking
kirkwaiblinger Dec 1, 2024
01f6a24
fix for old versions of TS
kirkwaiblinger Dec 1, 2024
132bb4f
lintfix
kirkwaiblinger Dec 1, 2024
2309fe3
readability
kirkwaiblinger Dec 1, 2024
d2b7ca7
isTypeParameter comment
kirkwaiblinger Dec 1, 2024
2a943bd
derp
kirkwaiblinger Dec 1, 2024
dfd3b70
ignored types
kirkwaiblinger Dec 1, 2024
f323fdc
expression
kirkwaiblinger Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 42 additions & 21 deletions packages/eslint-plugin/src/rules/no-base-to-string.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -69,7 +68,7 @@
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;
}
Expand Down Expand Up @@ -176,15 +175,17 @@
}

function collectToStringCertainty(type: ts.Type): Usefulness {
const toString =
checker.getPropertyOfType(type, 'toString') ??
checker.getPropertyOfType(type, 'toLocaleString');
const declarations = toString?.getDeclarations();
if (!toString || !declarations || declarations.length === 0) {
// https://github.com/JoshuaKGoldberg/ts-api-utils/issues/382
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;
}

// Patch for old version TypeScript, the Boolean type definition missing toString()
// the Boolean type definition missing toString()
if (
type.flags & ts.TypeFlags.Boolean ||
type.flags & ts.TypeFlags.BooleanLiteral
Expand All @@ -196,32 +197,49 @@
return Usefulness.Always;
}

if (
declarations.every(
({ parent }) =>
!ts.isInterfaceDeclaration(parent) || parent.name.text !== 'Object',
)
) {
return Usefulness.Always;
}

if (type.isIntersection()) {
return collectIntersectionTypeCertainty(type, collectToStringCertainty);
}

if (!type.isUnion()) {
return Usefulness.Never;
if (type.isUnion()) {
return collectUnionTypeCertainty(type, collectToStringCertainty);
}

const toString =
checker.getPropertyOfType(type, 'toString') ??
checker.getPropertyOfType(type, 'toLocaleString');
if (!toString) {
// e.g. any/unknown
return Usefulness.Always;
}
return collectUnionTypeCertainty(type, collectToStringCertainty);

const declarations = toString.getDeclarations();

if (declarations == null || 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;

Check warning on line 224 in packages/eslint-plugin/src/rules/no-base-to-string.ts

View check run for this annotation

Codecov / codecov/patch

packages/eslint-plugin/src/rules/no-base-to-string.ts#L224 

Added line #L224 was not covered by tests
}

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 {
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;
}
Expand All @@ -245,7 +263,10 @@
}
},
CallExpression(node: TSESTree.CallExpression): void {
if (isBuiltInStringCall(node)) {
if (
isBuiltInStringCall(node) &&
node.arguments[0].type !== AST_NODE_TYPES.SpreadElement
) {
checkExpression(node.arguments[0]);
}
},
Expand Down
119 changes: 99 additions & 20 deletions packages/eslint-plugin/tests/rules/no-base-to-string.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,20 @@ tag\`\${{}}\`;
"'' += new URL();",
"'' += new URLSearchParams();",
`
let numbers = [1, 2, 3];
String(...a);
`,
`
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;
Expand Down Expand Up @@ -215,6 +219,46 @@ class Foo {}
declare const tuple: [string] & [Foo];
tuple.join('');
`,
// don't bother trying to interpret spread args.
`
let objects = [{}, {}];
String(...objects);
`,
// https://github.com/typescript-eslint/typescript-eslint/issues/8585
`
type Constructable<Entity> = abstract new (...args: any[]) => Entity;

interface GuildChannel {
toString(): \`<#\${string}>\`;
}

declare const foo: Constructable<GuildChannel & { bar: 1 }>;
class ExtendedGuildChannel extends foo {}
declare const bb: ExtendedGuildChannel;
bb.toString();
`,
// https://github.com/typescript-eslint/typescript-eslint/issues/8585 with intersection order reversed.
Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this relevant, you might ask? Because it affects the order in which the overloads are declared (and therefore resolved), at least for TS < 5.4.

(playground)

type Constructable<Entity> = abstract new (...args: any[]) => Entity;
interface GuildChannel {
  toString(): `<#${string}>`;
}
declare const foo: Constructable<GuildChannel & { bar: 1 }>;
class ExtendedGuildChannel extends foo {}
declare const bb: ExtendedGuildChannel;
bb.toString() satisfies `<#${string}>`;

but this errors
(playground)

type Constructable<Entity> = 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() satisfies `<#${string}>`;

`
type Constructable<Entity> = 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();
`,
`
function foo<T>(x: T) {
String(x);
}
`,
`
declare const u: unknown;
String(u);
`,
],
invalid: [
{
Expand Down Expand Up @@ -277,21 +321,6 @@ tuple.join('');
},
],
},
{
code: `
let objects = [{}, {}];
String(...objects);
`,
errors: [
{
data: {
certainty: 'will',
name: '...objects',
},
messageId: 'baseToString',
},
],
},
{
code: "'' += {};",
errors: [
Expand Down Expand Up @@ -682,13 +711,63 @@ declare const foo: Bar & Foo;
errors: [
{
data: {
certainty: 'will',
certainty: 'may',
Copy link
Member Author

Choose a reason for hiding this comment

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

this is just an existing bug of imprecise handling of generics.

name: 'array',
},
messageId: 'baseArrayJoin',
},
],
},
{
code: `
type Bar = Record<string, string>;
function foo<T extends string | Bar>(array: T[]) {
array[0].toString();
}
`,
errors: [
{
data: {
certainty: 'may',
name: 'array[0]',
},
messageId: 'baseToString',
},
],
},
{
code: `
type Bar = Record<string, string>;
function foo<T extends string | Bar>(value: T) {
value.toString();
}
`,
errors: [
{
data: {
certainty: 'may',
name: 'value',
},
messageId: 'baseToString',
},
],
},
{
code: `
type Bar = Record<string, string>;
declare const foo: Bar | string;
foo.toString();
`,
errors: [
{
data: {
certainty: 'may',
name: 'foo',
},
messageId: 'baseToString',
},
],
},
{
code: `
type Bar = Record<string, string>;
Expand Down
Loading