Skip to content
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.

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

feat(eslint-plugin): [restrict-template-expressions] add allowArray option #8389

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c035774
WIP
abrahamguo Sep 29, 2023
4a41e8f
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 6, 2024
f51c879
finish logic
abrahamguo Feb 6, 2024
0116b14
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 12, 2024
37cfa52
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 13, 2024
b7217e7
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 15, 2024
592c036
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 19, 2024
fbd2f30
add doc section
abrahamguo Feb 19, 2024
4ee7087
update snapshot
abrahamguo Feb 19, 2024
035b963
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 23, 2024
bf925e5
refactors and renames
abrahamguo Feb 23, 2024
f56d8e9
simplify type flag testers
abrahamguo Feb 23, 2024
8245146
rename
abrahamguo Feb 23, 2024
bf1a087
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 26, 2024
a6e0c7b
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 26, 2024
f4bb1e8
write tests
abrahamguo Feb 26, 2024
0a60320
simplify test
abrahamguo Feb 26, 2024
d77d075
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Feb 26, 2024
2b03d56
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Mar 7, 2024
20994f5
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Mar 8, 2024
4c375ac
suppress lint error
abrahamguo Mar 8, 2024
9a3c36a
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into r…
abrahamguo Mar 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/eslint-plugin/docs/rules/restrict-template-expressions.md
Expand Up @@ -12,10 +12,10 @@ This rule reports on values used in a template literal string that aren't string

:::note

This rule intentionally does not allow objects with a custom `toString()` method to be used in template literals, because the stringification result may not be user-friendly.
The default settings of this rule intentionally do not allow objects with a custom `toString()` method to be used in template literals, because the stringification result may not be user-friendly.

For example, arrays have a custom [`toString()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toString) method, which only calls `join()` internally, which joins the array elements with commas. This means that (1) array elements are not necessarily stringified to useful results (2) the commas don't have spaces after them, making the result not user-friendly. The best way to format arrays is to use [`Intl.ListFormat`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat), which even supports adding the "and" conjunction where necessary.
You must explicitly call `object.toString()` if you want to use this object in a template literal.
You must explicitly call `object.toString()` if you want to use this object in a template literal, or turn on the `allowArray` option to specifically allow arrays.
The [`no-base-to-string`](./no-base-to-string.md) rule can be used to guard this case against producing `"[object Object]"` by accident.

:::
Expand Down Expand Up @@ -111,6 +111,15 @@ const arg = 'something';
const msg1 = typeof arg === 'string' ? arg : `arg = ${arg}`;
```

### `allowArray`

Examples of additional **correct** code for this rule with `{ allowArray: true }`:

```ts option='{ "allowArray": true }' showPlaygroundButton
const arg = ['foo', 'bar'];
const msg1 = `arg = ${arg}`;
```

## When Not To Use It

If you're not worried about incorrectly stringifying non-string values in template literals, then you likely don't need this rule.
Expand Down
168 changes: 65 additions & 103 deletions packages/eslint-plugin/src/rules/restrict-template-expressions.ts
@@ -1,6 +1,7 @@
import type { TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES } from '@typescript-eslint/utils';
import * as ts from 'typescript';
import type { Type, TypeChecker } from 'typescript';
import { TypeFlags } from 'typescript';

import {
createRule,
Expand All @@ -12,15 +13,44 @@ import {
isTypeNeverType,
} from '../util';

type OptionTester = (
type: Type,
checker: TypeChecker,
recursivelyCheckType: (type: Type) => boolean,
) => boolean;

const testTypeFlag =
(flagsToCheck: TypeFlags): OptionTester =>
type =>
isTypeFlagSet(type, flagsToCheck);

const optionTesters = (
[
['Any', isTypeAnyType],
[
'Array',
(type, checker, recursivelyCheckType): boolean =>
checker.isArrayType(type) &&
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
recursivelyCheckType(type.getNumberIndexType()!),
],
// eslint-disable-next-line @typescript-eslint/internal/prefer-ast-types-enum
Copy link
Member

Choose a reason for hiding this comment

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

@JoshuaKGoldberg - Originally this rule's purpose was to prevent code like node.type === 'string'.
Given we now have the no-unsafe-enum-comparison rule - do you think this internal rule still has value? WDYT about deleting it? (we already have nuec turned on via the recommended config)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'd be in favor of that. It wouldn't be the first example of an internal lint rule ending up as a subset of a general good one.

['Boolean', testTypeFlag(TypeFlags.BooleanLike)],
['Nullish', testTypeFlag(TypeFlags.Null | TypeFlags.Undefined)],
['Number', testTypeFlag(TypeFlags.NumberLike | TypeFlags.BigIntLike)],
[
'RegExp',
(type, checker): boolean => getTypeName(checker, type) === 'RegExp',
],
['Never', isTypeNeverType],
] satisfies [string, OptionTester][]
).map(([type, tester]) => ({
type,
option: `allow${type}` as const,
tester,
}));
type Options = [
{
allowAny?: boolean;
allowBoolean?: boolean;
allowNullish?: boolean;
allowNumber?: boolean;
allowRegExp?: boolean;
allowNever?: boolean;
},
{ [Type in (typeof optionTesters)[number]['option']]?: boolean },
];

type MessageId = 'invalidType';
Expand All @@ -42,38 +72,15 @@ export default createRule<Options, MessageId>({
{
type: 'object',
additionalProperties: false,
properties: {
allowAny: {
description:
'Whether to allow `any` typed values in template expressions.',
type: 'boolean',
},
allowBoolean: {
description:
'Whether to allow `boolean` typed values in template expressions.',
type: 'boolean',
},
allowNullish: {
description:
'Whether to allow `nullish` typed values in template expressions.',
type: 'boolean',
},
allowNumber: {
description:
'Whether to allow `number` typed values in template expressions.',
type: 'boolean',
},
allowRegExp: {
description:
'Whether to allow `regexp` typed values in template expressions.',
type: 'boolean',
},
allowNever: {
description:
'Whether to allow `never` typed values in template expressions.',
type: 'boolean',
},
},
properties: Object.fromEntries(
optionTesters.map(({ option, type }) => [
option,
{
description: `Whether to allow \`${type.toLowerCase()}\` typed values in template expressions.`,
type: 'boolean',
},
]),
),
},
],
},
Expand All @@ -89,47 +96,9 @@ export default createRule<Options, MessageId>({
create(context, [options]) {
const services = getParserServices(context);
const checker = services.program.getTypeChecker();

function isUnderlyingTypePrimitive(type: ts.Type): boolean {
if (isTypeFlagSet(type, ts.TypeFlags.StringLike)) {
return true;
}

if (
options.allowNumber &&
isTypeFlagSet(type, ts.TypeFlags.NumberLike | ts.TypeFlags.BigIntLike)
) {
return true;
}

if (
options.allowBoolean &&
isTypeFlagSet(type, ts.TypeFlags.BooleanLike)
) {
return true;
}

if (options.allowAny && isTypeAnyType(type)) {
return true;
}

if (options.allowRegExp && getTypeName(checker, type) === 'RegExp') {
return true;
}

if (
options.allowNullish &&
isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined)
) {
return true;
}

if (options.allowNever && isTypeNeverType(type)) {
return true;
}

return false;
}
const enabledOptionTesters = optionTesters.filter(
({ option }) => options[option],
);

return {
TemplateLiteral(node: TSESTree.TemplateLiteral): void {
Expand All @@ -144,12 +113,7 @@ export default createRule<Options, MessageId>({
expression,
);

if (
!isInnerUnionOrIntersectionConformingTo(
expressionType,
isUnderlyingTypePrimitive,
)
) {
if (!recursivelyCheckType(expressionType)) {
context.report({
node: expression,
messageId: 'invalidType',
Expand All @@ -160,23 +124,21 @@ export default createRule<Options, MessageId>({
},
};

function isInnerUnionOrIntersectionConformingTo(
type: ts.Type,
predicate: (underlyingType: ts.Type) => boolean,
): boolean {
return rec(type);

function rec(innerType: ts.Type): boolean {
if (innerType.isUnion()) {
return innerType.types.every(rec);
}

if (innerType.isIntersection()) {
return innerType.types.some(rec);
}
function recursivelyCheckType(innerType: Type): boolean {
if (innerType.isUnion()) {
return innerType.types.every(recursivelyCheckType);
}

return predicate(innerType);
if (innerType.isIntersection()) {
return innerType.types.some(recursivelyCheckType);
}

return (
isTypeFlagSet(innerType, TypeFlags.StringLike) ||
enabledOptionTesters.some(({ tester }) =>
tester(innerType, checker, recursivelyCheckType),
)
);
}
},
});
Expand Up @@ -131,6 +131,30 @@ ruleTester.run('restrict-template-expressions', rule, {
}
`,
},
// allowArray
{
options: [{ allowArray: true }],
code: `
const arg = [];
const msg = \`arg = \${arg}\`;
`,
},
{
options: [{ allowArray: true }],
code: `
const arg = [];
const msg = \`arg = \${arg || 'default'}\`;
`,
},
{
options: [{ allowArray: true }],
code: `
const arg = [];
function test<T extends string[]>(arg: T) {
return \`arg = \${arg}\`;
}
`,
},
// allowAny
{
options: [{ allowAny: true }],
Expand Down Expand Up @@ -341,6 +365,20 @@ ruleTester.run('restrict-template-expressions', rule, {
],
options: [{ allowNullish: false }],
},
{
code: `
const msg = \`arg = \${[, 2]}\`;
`,
errors: [
{
messageId: 'invalidType',
data: { type: '(number | undefined)[]' },
line: 2,
column: 30,
},
],
options: [{ allowNullish: false, allowArray: true }],
},
{
code: `
declare const arg: number;
Expand Down Expand Up @@ -369,11 +407,7 @@ ruleTester.run('restrict-template-expressions', rule, {
column: 30,
},
],
options: [
{
allowBoolean: false,
},
],
options: [{ allowBoolean: false }],
},
{
options: [{ allowNumber: true, allowBoolean: true, allowNullish: true }],
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.