From 5750ebce7fd303467b95f02cc5ae028eb26ddb9d Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Tue, 5 Nov 2024 00:16:08 +0900 Subject: [PATCH 1/7] feat(eslint-plugin): [no-base-to-string] add checkArrayJoin option --- .../docs/rules/no-base-to-string.mdx | 8 ++ .../src/rules/no-base-to-string.ts | 61 +++++++++- .../no-base-to-string.shot | 8 ++ .../tests/rules/no-base-to-string.test.ts | 109 ++++++++++++++++++ .../schema-snapshots/no-base-to-string.shot | 6 + 5 files changed, 188 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx index 1da882c1ebe4..28db5d47a2af 100644 --- a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx +++ b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx @@ -96,6 +96,14 @@ let text = `${value}`; String(/regex/); ``` +### `checkArrayJoin` + +{/* insert option description */} + +```ts option='{ "checkArrayJoin": true }' showPlaygroundButton +[{}, {}].join(); +``` + ## When Not To Use It If you don't mind a risk of `"[object Object]"` or incorrect type coercions in your values, then you will not need this rule. 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 7aad839a584a..815db1e2b3f8 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -2,6 +2,7 @@ import type { TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import * as tsutils from 'ts-api-utils'; import * as ts from 'typescript'; import { createRule, getParserServices, getTypeName } from '../util'; @@ -14,10 +15,11 @@ enum Usefulness { type Options = [ { + checkArrayJoin?: boolean; ignoredTypeNames?: string[]; }, ]; -type MessageIds = 'baseToString'; +type MessageIds = 'baseArrayJoin' | 'baseToString'; export default createRule({ name: 'no-base-to-string', @@ -30,6 +32,8 @@ export default createRule({ requiresTypeChecking: true, }, messages: { + baseArrayJoin: + "Using `join()` for {{name}} use Object's default stringification format ('[object Object]') when stringified.", baseToString: "'{{name}}' {{certainty}} use Object's default stringification format ('[object Object]') when stringified.", }, @@ -38,6 +42,11 @@ export default createRule({ type: 'object', additionalProperties: false, properties: { + checkArrayJoin: { + type: 'boolean', + description: + 'Whether to check for arrays that are stringified by `Array.prototype.join`.', + }, ignoredTypeNames: { type: 'array', description: @@ -52,6 +61,7 @@ export default createRule({ }, defaultOptions: [ { + checkArrayJoin: false, ignoredTypeNames: ['Error', 'RegExp', 'URL', 'URLSearchParams'], }, ], @@ -59,22 +69,35 @@ export default createRule({ const services = getParserServices(context); const checker = services.program.getTypeChecker(); const ignoredTypeNames = option.ignoredTypeNames ?? []; + const checkArrayJoin = option.checkArrayJoin ?? false; - function checkExpression(node: TSESTree.Node, type?: ts.Type): void { + function getToStringCertainty( + node: TSESTree.Node, + type?: ts.Type, + ): Usefulness { if (node.type === AST_NODE_TYPES.Literal) { - return; + return Usefulness.Always; } const certainty = collectToStringCertainty( type ?? services.getTypeAtLocation(node), ); + return certainty; + } + + function checkExpression( + node: TSESTree.Node, + type?: ts.Type, + messageId: MessageIds = 'baseToString', + ): void { + const certainty = getToStringCertainty(node, type); if (certainty === Usefulness.Always) { return; } context.report({ node, - messageId: 'baseToString', + messageId, data: { name: context.sourceCode.getText(node), certainty, @@ -202,6 +225,36 @@ export default createRule({ checkExpression(expression); } }, + ...(checkArrayJoin + ? { + 'CallExpression > MemberExpression.callee > Identifier[name = "join"].property'( + node: TSESTree.Expression, + ): void { + const memberExpr = node.parent as TSESTree.MemberExpression; + const type = services.getTypeAtLocation(memberExpr.object); + const typeParts = tsutils.typeParts(type); + const isArrayOrTuple = typeParts.every( + part => checker.isArrayType(part) || checker.isTupleType(part), + ); + if (!isArrayOrTuple) { + return; + } + + for (const part of typeParts) { + const typeArg = checker.getTypeArguments( + part as ts.TypeReference, + )[0]; + + const certainty = getToStringCertainty(node, typeArg); + + if (certainty !== Usefulness.Always) { + checkExpression(memberExpr.object, typeArg, 'baseArrayJoin'); + return; + } + } + }, + } + : undefined), }; }, }); diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot index 1226f0a5f7ff..fe192a7eaae7 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot @@ -63,3 +63,11 @@ let text = \`\${value}\`; String(/regex/); " `; + +exports[`Validating rule docs no-base-to-string.mdx code examples ESLint output 4`] = ` +"Options: { "checkArrayJoin": true } + +[{}, {}].join(); +~~~~~~~~ Using \`join()\` for [{}, {}] use Object's default stringification format ('[object Object]') when stringified. +" +`; 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 5aebfa112f9a..f7d6863ecadf 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 @@ -156,6 +156,14 @@ String(myValue); import { String } from 'foo'; String({}); `, + { + code: "['foo', 'bar'].join('');", + options: [{ checkArrayJoin: true }], + }, + { + code: "([{}, 'bar'] as string[]).join('');", + options: [{ checkArrayJoin: true }], + }, ], invalid: [ { @@ -353,5 +361,106 @@ String(...objects); }, ], }, + + { + code: ` + [{}, {}].join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: '[{}, {}]', + }, + messageId: 'baseArrayJoin', + }, + ], + options: [{ checkArrayJoin: true }], + }, + { + code: ` + class A {} + [{}, 'str'].join(''); + `, + errors: [ + { + data: { + certainty: 'may', + name: "[{}, 'str']", + }, + messageId: 'baseArrayJoin', + }, + ], + options: [{ checkArrayJoin: true }], + }, + { + code: ` + const array = [{}, {}]; + array.join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: 'array', + }, + messageId: 'baseArrayJoin', + }, + ], + options: [{ checkArrayJoin: true }], + }, + { + code: ` + class Foo {} + class Bar {} + const array: Foo[] | Bar[] = [new Foo(), new Bar()]; + array.join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: 'array', + }, + messageId: 'baseArrayJoin', + }, + ], + options: [{ checkArrayJoin: true }], + }, + { + code: ` + class Foo {} + class Bar {} + const array: Foo[] & Bar[] = [new Foo(), new Bar()]; + array.join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: 'array', + }, + messageId: 'baseArrayJoin', + }, + ], + options: [{ checkArrayJoin: true }], + }, + { + code: ` + class Foo {} + const tuple: [Foo, Foo] = [new Foo(), new Foo()]; + tuple.join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: 'tuple', + }, + messageId: 'baseArrayJoin', + }, + ], + options: [{ checkArrayJoin: true }], + }, ], }); diff --git a/packages/eslint-plugin/tests/schema-snapshots/no-base-to-string.shot b/packages/eslint-plugin/tests/schema-snapshots/no-base-to-string.shot index 08776052d51f..61552f5b5288 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/no-base-to-string.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/no-base-to-string.shot @@ -8,6 +8,10 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos { "additionalProperties": false, "properties": { + "checkArrayJoin": { + "description": "Whether to check for arrays that are stringified by \`Array.prototype.join\`.", + "type": "boolean" + }, "ignoredTypeNames": { "description": "Stringified regular expressions of type names to ignore.", "items": { @@ -25,6 +29,8 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos type Options = [ { + /** Whether to check for arrays that are stringified by \`Array.prototype.join\`. */ + checkArrayJoin?: boolean; /** Stringified regular expressions of type names to ignore. */ ignoredTypeNames?: string[]; }, From b7a0f17e769b6830d10053db0a37b275ec39ae18 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Thu, 7 Nov 2024 00:32:24 +0900 Subject: [PATCH 2/7] add test case --- .../tests/rules/no-base-to-string.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 f7d6863ecadf..54b8d41f03e4 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 @@ -164,6 +164,16 @@ String({}); code: "([{}, 'bar'] as string[]).join('');", options: [{ checkArrayJoin: true }], }, + { + code: ` + class Foo { + join() {} + } + const foo = new Foo(); + foo.join(); + `, + options: [{ checkArrayJoin: true }], + }, ], invalid: [ { From 68a3db2910c93d059543f332485c2ffba159b9b2 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Thu, 7 Nov 2024 01:55:33 +0900 Subject: [PATCH 3/7] update snapshot --- packages/eslint-plugin/docs/rules/no-base-to-string.mdx | 5 ++++- .../docs-eslint-output-snapshots/no-base-to-string.shot | 8 ++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx index 28db5d47a2af..0ef9c0ee254e 100644 --- a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx +++ b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx @@ -101,7 +101,10 @@ String(/regex/); {/* insert option description */} ```ts option='{ "checkArrayJoin": true }' showPlaygroundButton -[{}, {}].join(); +[{}, 'string'].join(); + +const array = ['string', {}]; +array.join(''); ``` ## When Not To Use It diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot index fe192a7eaae7..2d498325e95a 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot @@ -67,7 +67,11 @@ String(/regex/); exports[`Validating rule docs no-base-to-string.mdx code examples ESLint output 4`] = ` "Options: { "checkArrayJoin": true } -[{}, {}].join(); -~~~~~~~~ Using \`join()\` for [{}, {}] use Object's default stringification format ('[object Object]') when stringified. +[{}, 'string'].join(); +~~~~~~~~~~~~~~ Using \`join()\` for [{}, 'string'] use Object's default stringification format ('[object Object]') when stringified. + +const array = ['string', {}]; +array.join(''); +~~~~~ Using \`join()\` for array use Object's default stringification format ('[object Object]') when stringified. " `; From 1fafc446c053d0b4c8974fb20ba2c5c774bc49ab Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Sat, 9 Nov 2024 23:12:29 +0900 Subject: [PATCH 4/7] reviews --- .../src/rules/no-base-to-string.ts | 14 +------ .../tests/rules/no-base-to-string.test.ts | 39 +++++++++++++++++++ 2 files changed, 41 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 815db1e2b3f8..8feeebd74a90 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -240,18 +240,8 @@ export default createRule({ return; } - for (const part of typeParts) { - const typeArg = checker.getTypeArguments( - part as ts.TypeReference, - )[0]; - - const certainty = getToStringCertainty(node, typeArg); - - if (certainty !== Usefulness.Always) { - checkExpression(memberExpr.object, typeArg, 'baseArrayJoin'); - return; - } - } + const typeArg = type.getNumberIndexType(); + checkExpression(memberExpr.object, typeArg, 'baseArrayJoin'); }, } : undefined), 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 54b8d41f03e4..68b81468298c 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 @@ -455,6 +455,23 @@ String(...objects); ], options: [{ checkArrayJoin: true }], }, + { + code: ` + class Foo {} + const tuple: [string, Foo] = ['string', new Foo()]; + tuple.join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: 'tuple', + }, + messageId: 'baseArrayJoin', + }, + ], + options: [{ checkArrayJoin: true }], + }, { code: ` class Foo {} @@ -472,5 +489,27 @@ String(...objects); ], options: [{ checkArrayJoin: true }], }, + { + code: ` + const array = ['string', {}]; + array.join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: 'array', + }, + messageId: 'baseArrayJoin', + }, + ], + languageOptions: { + parserOptions: { + project: './tsconfig.noUncheckedIndexedAccess.json', + tsconfigRootDir: rootDir, + }, + }, + options: [{ checkArrayJoin: true }], + }, ], }); From c79b0cccd0843c3738c3ab57f538b2ecb12782a7 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Wed, 13 Nov 2024 00:06:56 +0900 Subject: [PATCH 5/7] apply reviews --- .../docs/rules/no-base-to-string.mdx | 14 +-- .../src/rules/no-base-to-string.ts | 54 ++++----- .../no-base-to-string.shot | 16 +-- .../tests/rules/no-base-to-string.test.ts | 112 +++++++++++++----- .../schema-snapshots/no-base-to-string.shot | 6 - 5 files changed, 114 insertions(+), 88 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx index 0ef9c0ee254e..bc16b5ffd14a 100644 --- a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx +++ b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx @@ -34,6 +34,9 @@ value + ''; String({}); ({}).toString(); ({}).toLocaleString(); + +// Stringifying objects or instances in an array with the `Array.prototype.join`. +[{}, class MyClass {}].join(''); ``` @@ -96,17 +99,6 @@ let text = `${value}`; String(/regex/); ``` -### `checkArrayJoin` - -{/* insert option description */} - -```ts option='{ "checkArrayJoin": true }' showPlaygroundButton -[{}, 'string'].join(); - -const array = ['string', {}]; -array.join(''); -``` - ## When Not To Use It If you don't mind a risk of `"[object Object]"` or incorrect type coercions in your values, then you will not need this rule. 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 8feeebd74a90..d4eca8f6505b 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -5,7 +5,12 @@ import { AST_NODE_TYPES } from '@typescript-eslint/utils'; import * as tsutils from 'ts-api-utils'; import * as ts from 'typescript'; -import { createRule, getParserServices, getTypeName } from '../util'; +import { + createRule, + getConstrainedTypeAtLocation, + getParserServices, + getTypeName, +} from '../util'; enum Usefulness { Always = 'always', @@ -15,7 +20,6 @@ enum Usefulness { type Options = [ { - checkArrayJoin?: boolean; ignoredTypeNames?: string[]; }, ]; @@ -33,7 +37,7 @@ export default createRule({ }, messages: { baseArrayJoin: - "Using `join()` for {{name}} use Object's default stringification format ('[object Object]') when stringified.", + "Using `join()` for {{name}} {{certainty}} use Object's default stringification format ('[object Object]') when stringified.", baseToString: "'{{name}}' {{certainty}} use Object's default stringification format ('[object Object]') when stringified.", }, @@ -42,11 +46,6 @@ export default createRule({ type: 'object', additionalProperties: false, properties: { - checkArrayJoin: { - type: 'boolean', - description: - 'Whether to check for arrays that are stringified by `Array.prototype.join`.', - }, ignoredTypeNames: { type: 'array', description: @@ -61,7 +60,6 @@ export default createRule({ }, defaultOptions: [ { - checkArrayJoin: false, ignoredTypeNames: ['Error', 'RegExp', 'URL', 'URLSearchParams'], }, ], @@ -69,7 +67,6 @@ export default createRule({ const services = getParserServices(context); const checker = services.program.getTypeChecker(); const ignoredTypeNames = option.ignoredTypeNames ?? []; - const checkArrayJoin = option.checkArrayJoin ?? false; function getToStringCertainty( node: TSESTree.Node, @@ -211,12 +208,29 @@ export default createRule({ checkExpression(node.arguments[0]); } }, + 'CallExpression > MemberExpression.callee > Identifier[name = "join"].property'( + node: TSESTree.Expression, + ): void { + const memberExpr = node.parent as TSESTree.MemberExpression; + const type = getConstrainedTypeAtLocation(services, memberExpr.object); + const typeParts = tsutils.typeParts(type); + const isArrayOrTuple = typeParts.every( + part => checker.isArrayType(part) || checker.isTupleType(part), + ); + if (!isArrayOrTuple) { + return; + } + + const typeArg = type.getNumberIndexType(); + checkExpression(memberExpr.object, typeArg, 'baseArrayJoin'); + }, 'CallExpression > MemberExpression.callee > Identifier[name = /^(toLocaleString|toString)$/].property'( node: TSESTree.Expression, ): void { const memberExpr = node.parent as TSESTree.MemberExpression; checkExpression(memberExpr.object); }, + TemplateLiteral(node: TSESTree.TemplateLiteral): void { if (node.parent.type === AST_NODE_TYPES.TaggedTemplateExpression) { return; @@ -225,26 +239,6 @@ export default createRule({ checkExpression(expression); } }, - ...(checkArrayJoin - ? { - 'CallExpression > MemberExpression.callee > Identifier[name = "join"].property'( - node: TSESTree.Expression, - ): void { - const memberExpr = node.parent as TSESTree.MemberExpression; - const type = services.getTypeAtLocation(memberExpr.object); - const typeParts = tsutils.typeParts(type); - const isArrayOrTuple = typeParts.every( - part => checker.isArrayType(part) || checker.isTupleType(part), - ); - if (!isArrayOrTuple) { - return; - } - - const typeArg = type.getNumberIndexType(); - checkExpression(memberExpr.object, typeArg, 'baseArrayJoin'); - }, - } - : undefined), }; }, }); diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot index 2d498325e95a..8d61ec3c1c2a 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot @@ -21,6 +21,10 @@ String({}); ~~ '{}' will use Object's default stringification format ('[object Object]') when stringified. ({}).toLocaleString(); ~~ '{}' will use Object's default stringification format ('[object Object]') when stringified. + +// Stringifying objects or instances in an array with the \`Array.prototype.join\`. +[{}, class MyClass {}].join(''); +~~~~~~~~~~~~~~~~~~~~~~ Using \`join()\` for [{}, class MyClass {}] may use Object's default stringification format ('[object Object]') when stringified. " `; @@ -63,15 +67,3 @@ let text = \`\${value}\`; String(/regex/); " `; - -exports[`Validating rule docs no-base-to-string.mdx code examples ESLint output 4`] = ` -"Options: { "checkArrayJoin": true } - -[{}, 'string'].join(); -~~~~~~~~~~~~~~ Using \`join()\` for [{}, 'string'] use Object's default stringification format ('[object Object]') when stringified. - -const array = ['string', {}]; -array.join(''); -~~~~~ Using \`join()\` for array use Object's default stringification format ('[object Object]') when stringified. -" -`; 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 68b81468298c..4b3f1da2e6ce 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 @@ -156,24 +156,33 @@ String(myValue); import { String } from 'foo'; String({}); `, - { - code: "['foo', 'bar'].join('');", - options: [{ checkArrayJoin: true }], - }, - { - code: "([{}, 'bar'] as string[]).join('');", - options: [{ checkArrayJoin: true }], - }, - { - code: ` - class Foo { - join() {} - } - const foo = new Foo(); - foo.join(); - `, - options: [{ checkArrayJoin: true }], - }, + ` +['foo', 'bar'].join(''); + `, + + ` +([{}, 'bar'] as string[]).join(''); + `, + ` +class Foo { + join() {} +} +const foo = new Foo(); +foo.join(); + `, + ` +function foo(array: T[]) { + return array.join(); +} + `, + ` +class Foo { + toString() { + return ''; + } +} +[new Foo()].join(); + `, ], invalid: [ { @@ -385,7 +394,6 @@ String(...objects); messageId: 'baseArrayJoin', }, ], - options: [{ checkArrayJoin: true }], }, { code: ` @@ -401,7 +409,6 @@ String(...objects); messageId: 'baseArrayJoin', }, ], - options: [{ checkArrayJoin: true }], }, { code: ` @@ -417,7 +424,6 @@ String(...objects); messageId: 'baseArrayJoin', }, ], - options: [{ checkArrayJoin: true }], }, { code: ` @@ -435,7 +441,6 @@ String(...objects); messageId: 'baseArrayJoin', }, ], - options: [{ checkArrayJoin: true }], }, { code: ` @@ -453,7 +458,6 @@ String(...objects); messageId: 'baseArrayJoin', }, ], - options: [{ checkArrayJoin: true }], }, { code: ` @@ -464,13 +468,12 @@ String(...objects); errors: [ { data: { - certainty: 'will', + certainty: 'may', name: 'tuple', }, messageId: 'baseArrayJoin', }, ], - options: [{ checkArrayJoin: true }], }, { code: ` @@ -487,17 +490,16 @@ String(...objects); messageId: 'baseArrayJoin', }, ], - options: [{ checkArrayJoin: true }], }, { code: ` - const array = ['string', {}]; + const array = ['string', { foo: 'bar' }]; array.join(''); `, errors: [ { data: { - certainty: 'will', + certainty: 'may', name: 'array', }, messageId: 'baseArrayJoin', @@ -509,7 +511,59 @@ String(...objects); tsconfigRootDir: rootDir, }, }, - options: [{ checkArrayJoin: true }], + }, + { + code: ` + type Bar = Record; + function foo(array: T[]) { + return array.join(); + } + `, + errors: [ + { + data: { + certainty: 'will', + name: 'array', + }, + messageId: 'baseArrayJoin', + }, + ], + }, + { + code: ` + type Bar = Record; + function foo(array: T[]) { + return array; + } + foo([{ foo: 'foo' }]).join(); + `, + errors: [ + { + data: { + certainty: 'will', + name: "foo([{ foo: 'foo' }])", + }, + messageId: 'baseArrayJoin', + }, + ], + }, + { + code: ` + type Bar = Record; + function foo(array: T[]) { + return array; + } + foo([{ foo: 'foo' }, 'bar']).join(); + `, + errors: [ + { + data: { + certainty: 'may', + name: "foo([{ foo: 'foo' }, 'bar'])", + }, + messageId: 'baseArrayJoin', + }, + ], }, ], }); diff --git a/packages/eslint-plugin/tests/schema-snapshots/no-base-to-string.shot b/packages/eslint-plugin/tests/schema-snapshots/no-base-to-string.shot index 61552f5b5288..08776052d51f 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/no-base-to-string.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/no-base-to-string.shot @@ -8,10 +8,6 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos { "additionalProperties": false, "properties": { - "checkArrayJoin": { - "description": "Whether to check for arrays that are stringified by \`Array.prototype.join\`.", - "type": "boolean" - }, "ignoredTypeNames": { "description": "Stringified regular expressions of type names to ignore.", "items": { @@ -29,8 +25,6 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos type Options = [ { - /** Whether to check for arrays that are stringified by \`Array.prototype.join\`. */ - checkArrayJoin?: boolean; /** Stringified regular expressions of type names to ignore. */ ignoredTypeNames?: string[]; }, From b4d8a93a26d687edfd718eee0eca05abc8ff924f Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Sun, 17 Nov 2024 17:21:09 +0900 Subject: [PATCH 6/7] improve certainty --- .../docs/rules/no-base-to-string.mdx | 2 +- .../src/rules/no-base-to-string.ts | 66 ++++++++++++------- .../no-base-to-string.shot | 4 +- .../tests/rules/no-base-to-string.test.ts | 34 +++++++++- 4 files changed, 80 insertions(+), 26 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx index bc16b5ffd14a..8097ca8b7f41 100644 --- a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx +++ b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx @@ -36,7 +36,7 @@ String({}); ({}).toLocaleString(); // Stringifying objects or instances in an array with the `Array.prototype.join`. -[{}, class MyClass {}].join(''); +[{}, new MyClass()].join(''); ``` 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 d4eca8f6505b..ab6e3286854a 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -68,33 +68,40 @@ export default createRule({ const checker = services.program.getTypeChecker(); const ignoredTypeNames = option.ignoredTypeNames ?? []; - function getToStringCertainty( - node: TSESTree.Node, - type?: ts.Type, - ): Usefulness { + function checkExpression(node: TSESTree.Node, type?: ts.Type): void { if (node.type === AST_NODE_TYPES.Literal) { - return Usefulness.Always; + return; } - const certainty = collectToStringCertainty( type ?? services.getTypeAtLocation(node), ); - return certainty; + if (certainty === Usefulness.Always) { + return; + } + + context.report({ + node, + messageId: 'baseToString', + data: { + name: context.sourceCode.getText(node), + certainty, + }, + }); } - function checkExpression( + function checkExpressionForArrayJoin( node: TSESTree.Node, - type?: ts.Type, - messageId: MessageIds = 'baseToString', + type: ts.Type, ): void { - const certainty = getToStringCertainty(node, type); + const certainty = collectJoinCertainty(type); + if (certainty === Usefulness.Always) { return; } context.report({ node, - messageId, + messageId: 'baseArrayJoin', data: { name: context.sourceCode.getText(node), certainty, @@ -102,6 +109,30 @@ export default createRule({ }); } + function collectJoinCertainty(type: ts.Type): Usefulness { + const typeParts = tsutils.typeParts(type); + + const isArrayOrTuple = typeParts.every( + part => checker.isArrayType(part) || checker.isTupleType(part), + ); + if (!isArrayOrTuple) { + return Usefulness.Always; + } + + if (checker.isTupleType(type)) { + const typeArgs = checker.getTypeArguments(type); + const certainties = typeArgs.map(t => collectToStringCertainty(t)); + if (certainties.some(certainty => certainty === Usefulness.Never)) { + return Usefulness.Never; + } + } + const elemType = type.getNumberIndexType(); + if (!elemType) { + return Usefulness.Always; + } + return collectToStringCertainty(elemType); + } + function collectToStringCertainty(type: ts.Type): Usefulness { const toString = checker.getPropertyOfType(type, 'toString') ?? @@ -213,16 +244,7 @@ export default createRule({ ): void { const memberExpr = node.parent as TSESTree.MemberExpression; const type = getConstrainedTypeAtLocation(services, memberExpr.object); - const typeParts = tsutils.typeParts(type); - const isArrayOrTuple = typeParts.every( - part => checker.isArrayType(part) || checker.isTupleType(part), - ); - if (!isArrayOrTuple) { - return; - } - - const typeArg = type.getNumberIndexType(); - checkExpression(memberExpr.object, typeArg, 'baseArrayJoin'); + checkExpressionForArrayJoin(memberExpr.object, type); }, 'CallExpression > MemberExpression.callee > Identifier[name = /^(toLocaleString|toString)$/].property'( node: TSESTree.Expression, diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot index 8d61ec3c1c2a..3e5e4b614498 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-base-to-string.shot @@ -23,8 +23,8 @@ String({}); ~~ '{}' will use Object's default stringification format ('[object Object]') when stringified. // Stringifying objects or instances in an array with the \`Array.prototype.join\`. -[{}, class MyClass {}].join(''); -~~~~~~~~~~~~~~~~~~~~~~ Using \`join()\` for [{}, class MyClass {}] may use Object's default stringification format ('[object Object]') when stringified. +[{}, new MyClass()].join(''); +~~~~~~~~~~~~~~~~~~~ Using \`join()\` for [{}, new MyClass()] will use Object's default stringification format ('[object Object]') when stringified. " `; 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 4b3f1da2e6ce..47b438a382ba 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 @@ -468,7 +468,7 @@ String(...objects); errors: [ { data: { - certainty: 'may', + certainty: 'will', name: 'tuple', }, messageId: 'baseArrayJoin', @@ -491,6 +491,38 @@ String(...objects); }, ], }, + { + code: ` + class Foo {} + const tuple: [Foo | string, string] = [new Foo(), 'string']; + tuple.join(''); + `, + errors: [ + { + data: { + certainty: 'may', + name: 'tuple', + }, + messageId: 'baseArrayJoin', + }, + ], + }, + { + code: ` + class Foo {} + declare const tuple: [string, string] | [Foo, Foo]; + tuple.join(''); + `, + errors: [ + { + data: { + certainty: 'may', + name: 'tuple', + }, + messageId: 'baseArrayJoin', + }, + ], + }, { code: ` const array = ['string', { foo: 'bar' }]; From 6595fd50e56e4ba3f877f66a84bb15f9126ef432 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Tue, 19 Nov 2024 01:31:03 +0900 Subject: [PATCH 7/7] apply reviews --- .../src/rules/no-base-to-string.ts | 98 ++++++----- .../tests/rules/no-base-to-string.test.ts | 166 ++++++++++++++++-- 2 files changed, 201 insertions(+), 63 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 ab6e3286854a..f7c012eeb21f 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 { getConstrainedTypeAtLocation, getParserServices, getTypeName, + nullThrows, } from '../util'; enum Usefulness { @@ -109,28 +110,69 @@ export default createRule({ }); } - function collectJoinCertainty(type: ts.Type): Usefulness { - const typeParts = tsutils.typeParts(type); + function collectUnionTypeCertainty( + type: ts.UnionType, + collectSubTypeCertainty: (type: ts.Type) => Usefulness, + ): Usefulness { + const certainties = type.types.map(t => collectSubTypeCertainty(t)); + if (certainties.every(certainty => certainty === Usefulness.Never)) { + return Usefulness.Never; + } - const isArrayOrTuple = typeParts.every( - part => checker.isArrayType(part) || checker.isTupleType(part), - ); - if (!isArrayOrTuple) { + if (certainties.every(certainty => certainty === Usefulness.Always)) { return Usefulness.Always; } + return Usefulness.Sometimes; + } + + function collectIntersectionTypeCertainty( + type: ts.IntersectionType, + collectSubTypeCertainty: (type: ts.Type) => Usefulness, + ): Usefulness { + for (const subType of type.types) { + const subtypeUsefulness = collectSubTypeCertainty(subType); + + if (subtypeUsefulness === Usefulness.Always) { + return Usefulness.Always; + } + } + + return Usefulness.Never; + } + + function collectJoinCertainty(type: ts.Type): Usefulness { + if (tsutils.isUnionType(type)) { + return collectUnionTypeCertainty(type, collectJoinCertainty); + } + + if (tsutils.isIntersectionType(type)) { + return collectIntersectionTypeCertainty(type, collectJoinCertainty); + } + if (checker.isTupleType(type)) { const typeArgs = checker.getTypeArguments(type); const certainties = typeArgs.map(t => collectToStringCertainty(t)); if (certainties.some(certainty => certainty === Usefulness.Never)) { return Usefulness.Never; } - } - const elemType = type.getNumberIndexType(); - if (!elemType) { + + if (certainties.some(certainty => certainty === Usefulness.Sometimes)) { + return Usefulness.Sometimes; + } + return Usefulness.Always; } - return collectToStringCertainty(elemType); + + if (checker.isArrayType(type)) { + const elemType = nullThrows( + type.getNumberIndexType(), + 'array should have number index type', + ); + return collectToStringCertainty(elemType); + } + + return Usefulness.Always; } function collectToStringCertainty(type: ts.Type): Usefulness { @@ -164,45 +206,13 @@ export default createRule({ } if (type.isIntersection()) { - for (const subType of type.types) { - const subtypeUsefulness = collectToStringCertainty(subType); - - if (subtypeUsefulness === Usefulness.Always) { - return Usefulness.Always; - } - } - - return Usefulness.Never; + return collectIntersectionTypeCertainty(type, collectToStringCertainty); } if (!type.isUnion()) { return Usefulness.Never; } - - let allSubtypesUseful = true; - let someSubtypeUseful = false; - - for (const subType of type.types) { - const subtypeUsefulness = collectToStringCertainty(subType); - - if (subtypeUsefulness !== Usefulness.Always && allSubtypesUseful) { - allSubtypesUseful = false; - } - - if (subtypeUsefulness !== Usefulness.Never && !someSubtypeUseful) { - someSubtypeUseful = true; - } - } - - if (allSubtypesUseful && someSubtypeUseful) { - return Usefulness.Always; - } - - if (someSubtypeUseful) { - return Usefulness.Sometimes; - } - - return Usefulness.Never; + return collectUnionTypeCertainty(type, collectToStringCertainty); } 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 47b438a382ba..df1f3d4a6979 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 @@ -164,13 +164,6 @@ String({}); ([{}, 'bar'] as string[]).join(''); `, ` -class Foo { - join() {} -} -const foo = new Foo(); -foo.join(); - `, - ` function foo(array: T[]) { return array.join(); } @@ -183,6 +176,45 @@ class Foo { } [new Foo()].join(); `, + ` +class Foo { + join() {} +} +const foo = new Foo(); +foo.join(); + `, + ` +declare const array: string[]; +array.join(''); + `, + ` +class Foo {} +declare const array: (string & Foo)[]; +array.join(''); + `, + ` +class Foo {} +class Bar {} +declare const array: (string & Foo)[] | (string & Bar)[]; +array.join(''); + `, + ` +class Foo {} +class Bar {} +declare const array: (string & Foo)[] & (string & Bar)[]; +array.join(''); + `, + ` +class Foo {} +class Bar {} +declare const tuple: [string & Foo, string & Bar]; +tuple.join(''); + `, + ` +class Foo {} +declare const tuple: [string] & [Foo]; +tuple.join(''); + `, ], invalid: [ { @@ -380,7 +412,56 @@ String(...objects); }, ], }, - + { + code: ` +class Foo {} +declare const foo: string | Foo; +\`\${foo}\`; + `, + errors: [ + { + data: { + certainty: 'may', + name: 'foo', + }, + messageId: 'baseToString', + }, + ], + }, + { + code: ` +class Foo {} +class Bar {} +declare const foo: Bar | Foo; +\`\${foo}\`; + `, + errors: [ + { + data: { + certainty: 'will', + name: 'foo', + }, + messageId: 'baseToString', + }, + ], + }, + { + code: ` +class Foo {} +class Bar {} +declare const foo: Bar & Foo; +\`\${foo}\`; + `, + errors: [ + { + data: { + certainty: 'will', + name: 'foo', + }, + messageId: 'baseToString', + }, + ], + }, { code: ` [{}, {}].join(''); @@ -395,16 +476,47 @@ String(...objects); }, ], }, + { + code: ` + const array = [{}, {}]; + array.join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: 'array', + }, + messageId: 'baseArrayJoin', + }, + ], + }, { code: ` class A {} - [{}, 'str'].join(''); + [new A(), 'str'].join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: "[new A(), 'str']", + }, + messageId: 'baseArrayJoin', + }, + ], + }, + { + code: ` + class Foo {} + declare const array: (string | Foo)[]; + array.join(''); `, errors: [ { data: { certainty: 'may', - name: "[{}, 'str']", + name: 'array', }, messageId: 'baseArrayJoin', }, @@ -412,13 +524,14 @@ String(...objects); }, { code: ` - const array = [{}, {}]; + class Foo {} + declare const array: (string & Foo) | (string | Foo)[]; array.join(''); `, errors: [ { data: { - certainty: 'will', + certainty: 'may', name: 'array', }, messageId: 'baseArrayJoin', @@ -429,7 +542,7 @@ String(...objects); code: ` class Foo {} class Bar {} - const array: Foo[] | Bar[] = [new Foo(), new Bar()]; + declare const array: Foo[] & Bar[]; array.join(''); `, errors: [ @@ -445,14 +558,13 @@ String(...objects); { code: ` class Foo {} - class Bar {} - const array: Foo[] & Bar[] = [new Foo(), new Bar()]; + declare const array: string[] | Foo[]; array.join(''); `, errors: [ { data: { - certainty: 'will', + certainty: 'may', name: 'array', }, messageId: 'baseArrayJoin', @@ -462,7 +574,7 @@ String(...objects); { code: ` class Foo {} - const tuple: [string, Foo] = ['string', new Foo()]; + declare const tuple: [string, Foo]; tuple.join(''); `, errors: [ @@ -478,7 +590,7 @@ String(...objects); { code: ` class Foo {} - const tuple: [Foo, Foo] = [new Foo(), new Foo()]; + declare const tuple: [Foo, Foo]; tuple.join(''); `, errors: [ @@ -494,7 +606,7 @@ String(...objects); { code: ` class Foo {} - const tuple: [Foo | string, string] = [new Foo(), 'string']; + declare const tuple: [Foo | string, string]; tuple.join(''); `, errors: [ @@ -523,6 +635,22 @@ String(...objects); }, ], }, + { + code: ` + class Foo {} + declare const tuple: [Foo, string] & [Foo, Foo]; + tuple.join(''); + `, + errors: [ + { + data: { + certainty: 'will', + name: 'tuple', + }, + messageId: 'baseArrayJoin', + }, + ], + }, { code: ` const array = ['string', { foo: 'bar' }];