From 1bc105a2c6ae3fde9596f0419fed0de699dc57c7 Mon Sep 17 00:00:00 2001 From: sinyovercosy Date: Sun, 17 May 2020 14:31:47 -0500 Subject: [PATCH] feat(eslint-plugin): [no-invalid-void-type] allow union of void and `allowInGenericTypeArguments` (#1960) --- .../docs/rules/no-invalid-void-type.md | 12 +- .../src/rules/no-invalid-void-type.ts | 105 ++++++++++++++---- .../tests/rules/no-invalid-void-type.test.ts | 66 ++++++++++- 3 files changed, 158 insertions(+), 25 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-invalid-void-type.md b/packages/eslint-plugin/docs/rules/no-invalid-void-type.md index 9cc9808852f..f1607478a0d 100644 --- a/packages/eslint-plugin/docs/rules/no-invalid-void-type.md +++ b/packages/eslint-plugin/docs/rules/no-invalid-void-type.md @@ -1,13 +1,14 @@ # Disallows usage of `void` type outside of generic or return types (`no-invalid-void-type`) Disallows usage of `void` type outside of return types or generic type arguments. -If `void` is used as return type, it shouldn’t be a part of intersection/union type. +If `void` is used as return type, it shouldn’t be a part of intersection/union type with most other types. ## Rationale The `void` type means “nothing” or that a function does not return any value, in contrast with implicit `undefined` type which means that a function returns a value `undefined`. -So “nothing” cannot be mixed with any other types. If you need this - use the `undefined` type instead. +So “nothing” cannot be mixed with any other types, other than `never`, which accepts all types. +If you need this - use the `undefined` type instead. ## Rule Details @@ -44,6 +45,8 @@ function noop(): void {} let trulyUndefined = void 0; async function promiseMeSomething(): Promise {} + +type stillVoid = void | never; ``` ### Options @@ -64,6 +67,8 @@ This option lets you control if `void` can be used as a valid value for generic Alternatively, you can provide an array of strings which whitelist which types may accept `void` as a generic type parameter. +Any types considered valid by this option will be considered valid as part of a union type with `void`. + This option is `true` by default. The following patterns are considered warnings with `{ allowInGenericTypeArguments: false }`: @@ -88,7 +93,8 @@ type NotAllowedVoid3 = Promise; The following patterns are not considered warnings with `{ allowInGenericTypeArguments: ['Ex.Mx.Tx'] }`: ```ts -type AllowedVoid = Ex.MX.Tx; +type AllowedVoid = Ex.Mx.Tx; +type AllowedVoidUnion = void | Ex.Mx.Tx; ``` ## When Not To Use It diff --git a/packages/eslint-plugin/src/rules/no-invalid-void-type.ts b/packages/eslint-plugin/src/rules/no-invalid-void-type.ts index 7d5dea30033..b78107bac65 100644 --- a/packages/eslint-plugin/src/rules/no-invalid-void-type.ts +++ b/packages/eslint-plugin/src/rules/no-invalid-void-type.ts @@ -60,11 +60,81 @@ export default util.createRule<[Options], MessageIds>({ AST_NODE_TYPES.ClassProperty, AST_NODE_TYPES.Identifier, ]; + const validUnionMembers: AST_NODE_TYPES[] = [ + AST_NODE_TYPES.TSVoidKeyword, + AST_NODE_TYPES.TSNeverKeyword, + ]; if (allowInGenericTypeArguments === true) { validParents.push(AST_NODE_TYPES.TSTypeParameterInstantiation); } + /** + * @brief check if the given void keyword is used as a valid generic type + * + * reports if the type parametrized by void is not in the whitelist, or + * allowInGenericTypeArguments is false. + * no-op if the given void keyword is not used as generic type + */ + function checkGenericTypeArgument(node: TSESTree.TSVoidKeyword): void { + // only matches T<..., void, ...> + // extra check for precaution + /* istanbul ignore next */ + if ( + node.parent?.type !== AST_NODE_TYPES.TSTypeParameterInstantiation || + node.parent.parent?.type !== AST_NODE_TYPES.TSTypeReference + ) { + return; + } + + // check whitelist + if (Array.isArray(allowInGenericTypeArguments)) { + const sourceCode = context.getSourceCode(); + const fullyQualifiedName = sourceCode + .getText(node.parent.parent.typeName) + .replace(/ /gu, ''); + + if ( + !allowInGenericTypeArguments + .map(s => s.replace(/ /gu, '')) + .includes(fullyQualifiedName) + ) { + context.report({ + messageId: 'invalidVoidForGeneric', + data: { generic: fullyQualifiedName }, + node, + }); + } + return; + } + + if (!allowInGenericTypeArguments) { + context.report({ + messageId: 'invalidVoidNotReturn', + node, + }); + } + } + + /** + * @brief checks that a union containing void is valid + * @return true if every member of the union is specified as a valid type in + * validUnionMembers, or is a valid generic type parametrized by void + */ + function isValidUnionType(node: TSESTree.TSUnionType): boolean { + return node.types.every( + member => + validUnionMembers.includes(member.type) || + // allows any T<..., void, ...> here, checked by checkGenericTypeArgument + (member.type === AST_NODE_TYPES.TSTypeReference && + member.typeParameters?.type === + AST_NODE_TYPES.TSTypeParameterInstantiation && + member.typeParameters?.params + .map(param => param.type) + .includes(AST_NODE_TYPES.TSVoidKeyword)), + ); + } + return { TSVoidKeyword(node: TSESTree.TSVoidKeyword): void { /* istanbul ignore next */ @@ -72,35 +142,28 @@ export default util.createRule<[Options], MessageIds>({ return; } + // checks T<..., void, ...> against specification of allowInGenericArguments option if ( - validParents.includes(node.parent.type) && - !invalidGrandParents.includes(node.parent.parent.type) + node.parent.type === AST_NODE_TYPES.TSTypeParameterInstantiation && + node.parent.parent.type === AST_NODE_TYPES.TSTypeReference ) { + checkGenericTypeArgument(node); return; } + // union w/ void must contain types from validUnionMembers, or a valid generic void type if ( - node.parent.type === AST_NODE_TYPES.TSTypeParameterInstantiation && - node.parent.parent.type === AST_NODE_TYPES.TSTypeReference && - Array.isArray(allowInGenericTypeArguments) + node.parent.type === AST_NODE_TYPES.TSUnionType && + isValidUnionType(node.parent) ) { - const sourceCode = context.getSourceCode(); - const fullyQualifiedName = sourceCode - .getText(node.parent.parent.typeName) - .replace(/ /gu, ''); - - if ( - !allowInGenericTypeArguments - .map(s => s.replace(/ /gu, '')) - .includes(fullyQualifiedName) - ) { - context.report({ - messageId: 'invalidVoidForGeneric', - data: { generic: fullyQualifiedName }, - node, - }); - } + return; + } + // default cases + if ( + validParents.includes(node.parent.type) && + !invalidGrandParents.includes(node.parent.parent.type) + ) { return; } diff --git a/packages/eslint-plugin/tests/rules/no-invalid-void-type.test.ts b/packages/eslint-plugin/tests/rules/no-invalid-void-type.test.ts index 057da9b7e84..31771c561c0 100644 --- a/packages/eslint-plugin/tests/rules/no-invalid-void-type.test.ts +++ b/packages/eslint-plugin/tests/rules/no-invalid-void-type.test.ts @@ -11,6 +11,23 @@ ruleTester.run('allowInGenericTypeArguments: false', rule, { code: 'type Generic = [T];', options: [{ allowInGenericTypeArguments: false }], }, + { + // https://github.com/typescript-eslint/typescript-eslint/issues/1946 + code: ` +function foo(): void | never { + throw new Error('Test'); +} + `, + options: [{ allowInGenericTypeArguments: false }], + }, + { + code: 'type voidNeverUnion = void | never;', + options: [{ allowInGenericTypeArguments: false }], + }, + { + code: 'type neverVoidUnion = never | void;', + options: [{ allowInGenericTypeArguments: false }], + }, ], invalid: [ { @@ -67,6 +84,17 @@ ruleTester.run('allowInGenericTypeArguments: false', rule, { }, ], }, + { + code: 'type invalidVoidUnion = void | number;', + options: [{ allowInGenericTypeArguments: false }], + errors: [ + { + messageId: 'invalidVoidNotReturn', + line: 1, + column: 25, + }, + ], + }, ], }); @@ -89,6 +117,8 @@ ruleTester.run('allowInGenericTypeArguments: true', rule, { 'type UnionType = string | number;', 'type GenericVoid = Generic;', 'type Generic = [T];', + 'type voidPromiseUnion = void | Promise;', + 'type promiseNeverUnion = Promise | never;', ], invalid: [ { @@ -315,7 +345,6 @@ ruleTester.run('allowInGenericTypeArguments: true', rule, { { code: ` type VoidType = void; - class OtherClassName { private propName: VoidType; } @@ -406,6 +435,16 @@ ruleTester.run('allowInGenericTypeArguments: true', rule, { }, ], }, + { + code: 'type invalidVoidUnion = void | Map;', + errors: [ + { + messageId: 'invalidVoidNotReturnOrGeneric', + line: 1, + column: 25, + }, + ], + }, ], }); @@ -435,6 +474,31 @@ ruleTester.run('allowInGenericTypeArguments: whitelist', rule, { code: 'type AllowedVoid = Ex.Mx.Tx;', options: [{ allowInGenericTypeArguments: ['Ex . Mx . Tx'] }], }, + { + code: 'type voidPromiseUnion = void | Promise;', + options: [{ allowInGenericTypeArguments: ['Promise'] }], + }, + { + code: 'type promiseVoidUnion = Promise | void;', + options: [{ allowInGenericTypeArguments: ['Promise'] }], + }, + { + // https://github.com/typescript-eslint/typescript-eslint/issues/1956 + code: ` +async function foo(bar: () => void | Promise) { + await bar(); +} + `, + options: [{ allowInGenericTypeArguments: ['Promise'] }], + }, + { + code: 'type promiseNeverUnion = Promise | never;', + options: [{ allowInGenericTypeArguments: ['Promise'] }], + }, + { + code: 'type voidPromiseNeverUnion = void | Promise | never;', + options: [{ allowInGenericTypeArguments: ['Promise'] }], + }, ], invalid: [ {