From 68b31c76cd96189202de5131ff4d3ed4d3e6b764 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Tue, 17 Sep 2024 17:11:33 +0900 Subject: [PATCH 1/6] feat(eslint-plugin): [no-base-to-string] handle String() --- .../docs/rules/no-base-to-string.mdx | 7 +++- .../src/rules/no-base-to-string.ts | 10 ++++- .../no-base-to-string.shot | 6 ++- .../tests/rules/no-base-to-string.test.ts | 37 +++++++++++++++++++ 4 files changed, 56 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 97767da415b8..bcd486cbbd05 100644 --- a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx +++ b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx @@ -9,7 +9,7 @@ import TabItem from '@theme/TabItem'; > > See **https://typescript-eslint.io/rules/no-base-to-string** for documentation. -JavaScript will call `toString()` on an object when it is converted to a string, such as when `+` adding to a string or in `${}` template literals. +JavaScript will call `toString()` on an object when it is converted to a string, such as when `+` adding to a string or in `${}` template literals or in `String()`. The default Object `.toString()` uses the format `"[object Object]"`, which is often not what was intended. This rule reports on stringified values that aren't primitives and don't define a more useful `.toString()` method. @@ -31,6 +31,7 @@ value + ''; // Interpolation and manual .toString() calls too: `Value: ${value}`; +String({}); ({}).toString(); ``` @@ -38,11 +39,12 @@ value + ''; ```ts -// These types all have useful .toString()s +// These types all have useful .toString()s. 'Text' + true; `Value: ${123}`; `Arrays too: ${[1, 2, 3]}`; (() => {}).toString(); +String(42); // Defining a custom .toString class is considered acceptable class CustomToString { @@ -79,6 +81,7 @@ The following patterns are considered correct with the default options `{ ignore let value = /regex/; value.toString(); let text = `${value}`; +String(/regex/); ``` ## When Not To Use It 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 d4620aa7c031..db748f243937 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -58,7 +58,7 @@ export default createRule({ const checker = services.program.getTypeChecker(); const ignoredTypeNames = option.ignoredTypeNames ?? []; - function checkExpression(node: TSESTree.Expression, type?: ts.Type): void { + function checkExpression(node: TSESTree.Node, type?: ts.Type): void { if (node.type === AST_NODE_TYPES.Literal) { return; } @@ -180,6 +180,14 @@ export default createRule({ checkExpression(expression); } }, + CallExpression(node: TSESTree.CallExpression): void { + if (node.callee.type !== AST_NODE_TYPES.Identifier) { + return; + } + if (node.arguments[0]) { + checkExpression(node.arguments[0]); + } + }, }; }, }); 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 f5bc9c118ff1..282e5901930e 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 @@ -15,6 +15,8 @@ value + ''; // Interpolation and manual .toString() calls too: \`Value: \${value}\`; ~~~~~ 'value' will use Object's default stringification format ('[object Object]') when stringified. +String({}); + ~~ '{}' will use Object's default stringification format ('[object Object]') when stringified. ({}).toString(); ~~ '{}' will use Object's default stringification format ('[object Object]') when stringified. " @@ -23,11 +25,12 @@ value + ''; exports[`Validating rule docs no-base-to-string.mdx code examples ESLint output 2`] = ` "Correct -// These types all have useful .toString()s +// These types all have useful .toString()s. 'Text' + true; \`Value: \${123}\`; \`Arrays too: \${[1, 2, 3]}\`; (() => {}).toString(); +String(42); // Defining a custom .toString class is considered acceptable class CustomToString { @@ -54,5 +57,6 @@ exports[`Validating rule docs no-base-to-string.mdx code examples ESLint output let value = /regex/; value.toString(); let text = \`\${value}\`; +String(/regex/); " `; 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 59def86e4bfb..96d86f2a4f67 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 @@ -60,6 +60,8 @@ ruleTester.run('no-base-to-string', rule, { `, ), + // String() + ...literalList.map(i => `String(${i});`), ` function someFunction() {} someFunction.toString(); @@ -118,6 +120,14 @@ tag\`\${{}}\`; "'' += new Error();", "'' += new URL();", "'' += new URLSearchParams();", + ` +let numbers = [1, 2, 3]; +String(...a); + `, + { + code: 'String(/regex/);', + options: [{ ignoredTypeNames: ['RegExp'] }], + }, ], invalid: [ { @@ -144,6 +154,33 @@ tag\`\${{}}\`; }, ], }, + { + code: 'String({});', + errors: [ + { + data: { + certainty: 'will', + name: '{}', + }, + messageId: 'baseToString', + }, + ], + }, + { + code: ` +let objects = [{}, {}]; +String(...objects); + `, + errors: [ + { + data: { + certainty: 'will', + name: '...objects', + }, + messageId: 'baseToString', + }, + ], + }, { code: "'' + {};", errors: [ From cd22963cc177d1bafa1d5a8fd70ff7fbb1d5bb52 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Wed, 18 Sep 2024 23:43:13 +0900 Subject: [PATCH 2/6] fix --- packages/eslint-plugin/src/rules/no-base-to-string.ts | 10 ++++++---- .../tests/rules/no-base-to-string.test.ts | 3 +++ 2 files changed, 9 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 db748f243937..b3e8a0a7c5f0 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -181,10 +181,12 @@ export default createRule({ } }, CallExpression(node: TSESTree.CallExpression): void { - if (node.callee.type !== AST_NODE_TYPES.Identifier) { - return; - } - if (node.arguments[0]) { + 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] + ) { 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 96d86f2a4f67..8775636fc5cd 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 @@ -123,6 +123,9 @@ tag\`\${{}}\`; ` let numbers = [1, 2, 3]; String(...a); + `, + ` +Number(1); `, { code: 'String(/regex/);', From addf65281b9dcc071471a4d0f4fab58b99f9495d Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Wed, 25 Sep 2024 22:49:14 +0900 Subject: [PATCH 3/6] handle shadowed string --- .../src/rules/no-base-to-string.ts | 21 +++++++++++++------ .../tests/rules/no-base-to-string.test.ts | 11 ++++++++++ 2 files changed, 26 insertions(+), 6 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 b3e8a0a7c5f0..e7013c2a78dd 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/internal/prefer-ast-types-enum */ import type { TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; import * as ts from 'typescript'; @@ -150,6 +151,19 @@ export default createRule({ return Usefulness.Never; } + function isBuiltInStringCall(node: TSESTree.CallExpression): boolean { + if ( + node.callee.type === AST_NODE_TYPES.Identifier && + node.callee.name === 'String' && + node.arguments[0] + ) { + const scope = context.sourceCode.getScope(node); + const variable = scope.set.get('String'); + return !variable?.defs.length; + } + return false; + } + return { 'AssignmentExpression[operator = "+="], BinaryExpression[operator = "+"]'( node: TSESTree.AssignmentExpression | TSESTree.BinaryExpression, @@ -181,12 +195,7 @@ export default createRule({ } }, CallExpression(node: TSESTree.CallExpression): void { - 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] - ) { + if (isBuiltInStringCall(node)) { 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 8775636fc5cd..bd0e82dc886c 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 @@ -131,6 +131,17 @@ Number(1); code: 'String(/regex/);', options: [{ ignoredTypeNames: ['RegExp'] }], }, + ` +function String(value) { + return value; +} +declare const myValue: object; +String(myValue); + `, + ` +import { String } from 'foo'; +String({}); + `, ], invalid: [ { From 711639f4b723c5cc10929c765256577449492f0a Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Wed, 23 Oct 2024 10:09:10 +0900 Subject: [PATCH 4/6] apply review --- .../eslint-plugin/docs/rules/no-base-to-string.mdx | 9 +++++++++ packages/eslint-plugin/src/rules/no-base-to-string.ts | 10 +++++----- 2 files changed, 14 insertions(+), 5 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 4b5bb401e34f..3e6560ddad94 100644 --- a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx +++ b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx @@ -66,6 +66,15 @@ const literalWithToString = { +## Alternatives + +Consider using `JSON.stringify` when you want to convert non-primitive things to string for logging, debugging, etc. + +```typescript +declare const o: object; +const errorMessage = 'Found unexpected value: ' + JSON.stringify(o); +``` + ## Options ### `ignoredTypeNames` 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 e00642c574c7..1ad43df569a5 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,11 @@ export default createRule({ const memberExpr = node.parent as TSESTree.MemberExpression; checkExpression(memberExpr.object); }, + CallExpression(node: TSESTree.CallExpression): void { + if (isBuiltInStringCall(node)) { + checkExpression(node.arguments[0]); + } + }, TemplateLiteral(node: TSESTree.TemplateLiteral): void { if (node.parent.type === AST_NODE_TYPES.TaggedTemplateExpression) { return; @@ -197,11 +202,6 @@ export default createRule({ checkExpression(expression); } }, - CallExpression(node: TSESTree.CallExpression): void { - if (isBuiltInStringCall(node)) { - checkExpression(node.arguments[0]); - } - }, }; }, }); From 80c39a0dbbf569344846f0eb4b5c5ef1f8208be7 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Wed, 23 Oct 2024 23:22:56 +0900 Subject: [PATCH 5/6] fix lint errors --- packages/eslint-plugin/src/rules/no-base-to-string.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 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 1ad43df569a5..7aad839a584a 100644 --- a/packages/eslint-plugin/src/rules/no-base-to-string.ts +++ b/packages/eslint-plugin/src/rules/no-base-to-string.ts @@ -183,17 +183,17 @@ export default createRule({ checkExpression(node.left, leftType); } }, + CallExpression(node: TSESTree.CallExpression): void { + if (isBuiltInStringCall(node)) { + checkExpression(node.arguments[0]); + } + }, 'CallExpression > MemberExpression.callee > Identifier[name = /^(toLocaleString|toString)$/].property'( node: TSESTree.Expression, ): void { const memberExpr = node.parent as TSESTree.MemberExpression; checkExpression(memberExpr.object); }, - CallExpression(node: TSESTree.CallExpression): void { - if (isBuiltInStringCall(node)) { - checkExpression(node.arguments[0]); - } - }, TemplateLiteral(node: TSESTree.TemplateLiteral): void { if (node.parent.type === AST_NODE_TYPES.TaggedTemplateExpression) { return; From 0220cf031402eddb63be42ed16e87110690e3258 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Thu, 24 Oct 2024 23:30:29 +0900 Subject: [PATCH 6/6] Update packages/eslint-plugin/docs/rules/no-base-to-string.mdx Co-authored-by: Kirk Waiblinger --- packages/eslint-plugin/docs/rules/no-base-to-string.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3e6560ddad94..1da882c1ebe4 100644 --- a/packages/eslint-plugin/docs/rules/no-base-to-string.mdx +++ b/packages/eslint-plugin/docs/rules/no-base-to-string.mdx @@ -9,7 +9,7 @@ import TabItem from '@theme/TabItem'; > > See **https://typescript-eslint.io/rules/no-base-to-string** for documentation. -JavaScript will call `toString()` on an object when it is converted to a string, such as when `+` adding to a string or in `${}` template literals or in `String()`. +JavaScript will call `toString()` on an object when it is converted to a string, such as when concatenated with a string (`expr + ''`), when interpolated into template literals (`${expr}`), or when passed as an argument to the String constructor (`String(expr)`). The default Object `.toString()` and `toLocaleString()` use the format `"[object Object]"`, which is often not what was intended. This rule reports on stringified values that aren't primitives and don't define a more useful `.toString()` or `toLocaleString()` method.