From e2e9ffc547a7bb891d7e9a76faf694b1f7513c94 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Mon, 4 Nov 2024 19:22:40 +0200 Subject: [PATCH] feat(eslint-plugin): [no-confusing-void-expression] add an option to ignore void<->void (#10067) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * initial implementation * enable rule * improvements and fixes * use checker.getContextualType over getContextualType * more fixes * improve implementation * add docs * update snapshots * enable rule with option * rename function names to be a bit less confusing * reword game-plan * move rule def to be under its proper title * remove invalid tests * add more tests * add more tests * add more tests * refactor tests * refactor tests * wording * Update packages/eslint-plugin/docs/rules/no-confusing-void-expression.mdx Co-authored-by: Josh Goldberg ✨ * Update packages/eslint-plugin/docs/rules/no-confusing-void-expression.mdx Co-authored-by: Josh Goldberg ✨ * Update packages/eslint-plugin/src/rules/no-confusing-void-expression.ts Co-authored-by: Josh Goldberg ✨ * format * format * add missing test * remove code to handle multiple call signatures * figure out the case of multiple call signatures in a function expression --------- Co-authored-by: Josh Goldberg ✨ --- eslint.config.mjs | 7 +- .../rules/no-confusing-void-expression.mdx | 22 + .../src/rules/no-confusing-void-expression.ts | 81 +- .../src/rules/no-unsafe-return.ts | 27 +- .../src/util/getParentFunctionNode.ts | 28 + .../no-confusing-void-expression.shot | 15 + .../no-confusing-void-expression.test.ts | 714 ++++++++++++++++++ .../no-confusing-void-expression.shot | 6 + .../tests/lib/getParsedConfigFile.test.ts | 2 +- 9 files changed, 869 insertions(+), 33 deletions(-) create mode 100644 packages/eslint-plugin/src/util/getParentFunctionNode.ts diff --git a/eslint.config.mjs b/eslint.config.mjs index 3476a5dbbb4a..8e2b821f54ed 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -105,13 +105,14 @@ export default tseslint.config( }, linterOptions: { reportUnusedDisableDirectives: 'error' }, rules: { - // TODO: https://github.com/typescript-eslint/typescript-eslint/issues/8538 - '@typescript-eslint/no-confusing-void-expression': 'off', - // // our plugin :D // + '@typescript-eslint/no-confusing-void-expression': [ + 'error', + { ignoreVoidReturningFunctions: true }, + ], '@typescript-eslint/ban-ts-comment': [ 'error', { diff --git a/packages/eslint-plugin/docs/rules/no-confusing-void-expression.mdx b/packages/eslint-plugin/docs/rules/no-confusing-void-expression.mdx index 15c2815ac19c..a66f94085403 100644 --- a/packages/eslint-plugin/docs/rules/no-confusing-void-expression.mdx +++ b/packages/eslint-plugin/docs/rules/no-confusing-void-expression.mdx @@ -123,6 +123,28 @@ function doSomething() { console.log(void alert('Hello, world!')); ``` +### `ignoreVoidReturningFunctions` + +Whether to ignore returns from functions with `void` return types when inside a function with a `void` return type. + +Some projects prefer allowing functions that explicitly return `void` to return `void` expressions. Doing so allows more writing more succinct functions. + +:::note +This is technically risky as the `void`-returning function might actually be returning a value not seen by the type system. +::: + +```ts option='{ "ignoreVoidReturningFunctions": true }' showPlaygroundButton +function foo(): void { + return console.log(); +} + +function onError(callback: () => void): void { + callback(); +} + +onError(() => console.log('oops')); +``` + ## When Not To Use It The return type of a function can be inspected by going to its definition or hovering over it in an IDE. diff --git a/packages/eslint-plugin/src/rules/no-confusing-void-expression.ts b/packages/eslint-plugin/src/rules/no-confusing-void-expression.ts index 22b487b89bbf..28de6615d131 100644 --- a/packages/eslint-plugin/src/rules/no-confusing-void-expression.ts +++ b/packages/eslint-plugin/src/rules/no-confusing-void-expression.ts @@ -16,11 +16,13 @@ import { nullThrows, NullThrowsReasons, } from '../util'; +import { getParentFunctionNode } from '../util/getParentFunctionNode'; export type Options = [ { ignoreArrowShorthand?: boolean; ignoreVoidOperator?: boolean; + ignoreVoidReturningFunctions?: boolean; }, ]; @@ -86,6 +88,11 @@ export default createRule({ description: 'Whether to ignore returns that start with the `void` operator.', }, + ignoreVoidReturningFunctions: { + type: 'boolean', + description: + 'Whether to ignore returns from functions with explicit `void` return types and functions with contextual `void` return types.', + }, }, }, ], @@ -93,6 +100,9 @@ export default createRule({ defaultOptions: [{ ignoreArrowShorthand: false, ignoreVoidOperator: false }], create(context, [options]) { + const services = getParserServices(context); + const checker = services.program.getTypeChecker(); + return { 'AwaitExpression, CallExpression, TaggedTemplateExpression'( node: @@ -100,7 +110,6 @@ export default createRule({ | TSESTree.CallExpression | TSESTree.TaggedTemplateExpression, ): void { - const services = getParserServices(context); const type = getConstrainedTypeAtLocation(services, node); if (!tsutils.isTypeFlagSet(type, ts.TypeFlags.VoidLike)) { // not a void expression @@ -122,6 +131,14 @@ export default createRule({ if (invalidAncestor.type === AST_NODE_TYPES.ArrowFunctionExpression) { // handle arrow function shorthand + if (options.ignoreVoidReturningFunctions) { + const returnsVoid = isVoidReturningFunctionNode(invalidAncestor); + + if (returnsVoid) { + return; + } + } + if (options.ignoreVoidOperator) { // handle wrapping with `void` return context.report({ @@ -177,6 +194,18 @@ export default createRule({ if (invalidAncestor.type === AST_NODE_TYPES.ReturnStatement) { // handle return statement + if (options.ignoreVoidReturningFunctions) { + const functionNode = getParentFunctionNode(invalidAncestor); + + if (functionNode) { + const returnsVoid = isVoidReturningFunctionNode(functionNode); + + if (returnsVoid) { + return; + } + } + } + if (options.ignoreVoidOperator) { // handle wrapping with `void` return context.report({ @@ -380,8 +409,6 @@ export default createRule({ function canFix( node: ReturnStatementWithArgument | TSESTree.ArrowFunctionExpression, ): boolean { - const services = getParserServices(context); - const targetNode = node.type === AST_NODE_TYPES.ReturnStatement ? node.argument @@ -390,5 +417,53 @@ export default createRule({ const type = getConstrainedTypeAtLocation(services, targetNode); return tsutils.isTypeFlagSet(type, ts.TypeFlags.VoidLike); } + + function isFunctionReturnTypeIncludesVoid(functionType: ts.Type): boolean { + const callSignatures = tsutils.getCallSignaturesOfType(functionType); + + return callSignatures.some(signature => { + const returnType = signature.getReturnType(); + + return tsutils + .unionTypeParts(returnType) + .some(tsutils.isIntrinsicVoidType); + }); + } + + function isVoidReturningFunctionNode( + functionNode: + | TSESTree.ArrowFunctionExpression + | TSESTree.FunctionDeclaration + | TSESTree.FunctionExpression, + ): boolean { + // Game plan: + // - If the function node has a type annotation, check if it includes `void`. + // - If it does then the function is safe to return `void` expressions in. + // - Otherwise, check if the function is a function-expression or an arrow-function. + // - If it is, get its contextual type and bail if we cannot. + // - Return based on whether the contextual type includes `void` or not + + const functionTSNode = services.esTreeNodeToTSNodeMap.get(functionNode); + + if (functionTSNode.type) { + const returnType = checker.getTypeFromTypeNode(functionTSNode.type); + + return tsutils + .unionTypeParts(returnType) + .some(tsutils.isIntrinsicVoidType); + } + + if (ts.isExpression(functionTSNode)) { + const functionType = checker.getContextualType(functionTSNode); + + if (functionType) { + return tsutils + .unionTypeParts(functionType) + .some(isFunctionReturnTypeIncludesVoid); + } + } + + return false; + } }, }); diff --git a/packages/eslint-plugin/src/rules/no-unsafe-return.ts b/packages/eslint-plugin/src/rules/no-unsafe-return.ts index b5becaa4e633..838f84eff6af 100644 --- a/packages/eslint-plugin/src/rules/no-unsafe-return.ts +++ b/packages/eslint-plugin/src/rules/no-unsafe-return.ts @@ -1,6 +1,5 @@ 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'; @@ -18,6 +17,7 @@ import { isTypeUnknownType, isUnsafeAssignment, } from '../util'; +import { getParentFunctionNode } from '../util/getParentFunctionNode'; export default createRule({ name: 'no-unsafe-return', @@ -49,31 +49,6 @@ export default createRule({ 'noImplicitThis', ); - function getParentFunctionNode( - node: TSESTree.Node, - ): - | TSESTree.ArrowFunctionExpression - | TSESTree.FunctionDeclaration - | TSESTree.FunctionExpression - | null { - let current = node.parent; - while (current) { - if ( - current.type === AST_NODE_TYPES.ArrowFunctionExpression || - current.type === AST_NODE_TYPES.FunctionDeclaration || - current.type === AST_NODE_TYPES.FunctionExpression - ) { - return current; - } - - current = current.parent; - } - - // this shouldn't happen in correct code, but someone may attempt to parse bad code - // the parser won't error, so we shouldn't throw here - /* istanbul ignore next */ return null; - } - function checkReturn( returnNode: TSESTree.Node, reportingNode: TSESTree.Node = returnNode, diff --git a/packages/eslint-plugin/src/util/getParentFunctionNode.ts b/packages/eslint-plugin/src/util/getParentFunctionNode.ts new file mode 100644 index 000000000000..f03024ef400c --- /dev/null +++ b/packages/eslint-plugin/src/util/getParentFunctionNode.ts @@ -0,0 +1,28 @@ +import type { TSESTree } from '@typescript-eslint/utils'; + +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; + +export function getParentFunctionNode( + node: TSESTree.Node, +): + | TSESTree.ArrowFunctionExpression + | TSESTree.FunctionDeclaration + | TSESTree.FunctionExpression + | null { + let current = node.parent; + while (current) { + if ( + current.type === AST_NODE_TYPES.ArrowFunctionExpression || + current.type === AST_NODE_TYPES.FunctionDeclaration || + current.type === AST_NODE_TYPES.FunctionExpression + ) { + return current; + } + + current = current.parent; + } + + // this shouldn't happen in correct code, but someone may attempt to parse bad code + // the parser won't error, so we shouldn't throw here + /* istanbul ignore next */ return null; +} diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-void-expression.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-void-expression.shot index bf20e61945c3..db49175d4373 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-void-expression.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-void-expression.shot @@ -80,3 +80,18 @@ function doSomething() { console.log(void alert('Hello, world!')); " `; + +exports[`Validating rule docs no-confusing-void-expression.mdx code examples ESLint output 5`] = ` +"Options: { "ignoreVoidReturningFunctions": true } + +function foo(): void { + return console.log(); +} + +function onError(callback: () => void): void { + callback(); +} + +onError(() => console.log('oops')); +" +`; diff --git a/packages/eslint-plugin/tests/rules/no-confusing-void-expression.test.ts b/packages/eslint-plugin/tests/rules/no-confusing-void-expression.test.ts index 67c2dc8d845e..4fcb1b8b6865 100644 --- a/packages/eslint-plugin/tests/rules/no-confusing-void-expression.test.ts +++ b/packages/eslint-plugin/tests/rules/no-confusing-void-expression.test.ts @@ -120,6 +120,296 @@ function cool(input: string) { } `, }, + { + code: ` +function test(): void { + return console.log('bar'); +} + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +const test = (): void => { + return console.log('bar'); +}; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +const test = (): void => console.log('bar'); + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +function test(): void { + { + return console.log('foo'); + } +} + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +const obj = { + test(): void { + return console.log('foo'); + }, +}; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +class Foo { + test(): void { + return console.log('foo'); + } +} + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +function test() { + function nestedTest(): void { + return console.log('foo'); + } +} + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +type Foo = () => void; +const test = (() => console.log()) as Foo; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +type Foo = { + foo: () => void; +}; +const test: Foo = { + foo: () => console.log(), +}; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +const test = { + foo: () => console.log(), +} as { + foo: () => void; +}; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +const test: { + foo: () => void; +} = { + foo: () => console.log(), +}; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +type Foo = { + foo: { bar: () => void }; +}; + +const test = { + foo: { bar: () => console.log() }, +} as Foo; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +type Foo = { + foo: { bar: () => void }; +}; + +const test: Foo = { + foo: { bar: () => console.log() }, +}; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +type MethodType = () => void; + +class App { + private method: MethodType = () => console.log(); +} + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +interface Foo { + foo: () => void; +} + +function bar(): Foo { + return { + foo: () => console.log(), + }; +} + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +type Foo = () => () => () => void; +const x: Foo = () => () => () => console.log(); + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +type Foo = { + foo: () => void; +}; + +const test = { + foo: () => console.log(), +} as Foo; + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: ` +type Foo = () => void; +const test: Foo = () => console.log('foo'); + `, + options: [{ ignoreVoidReturningFunctions: true }], + }, + { + code: 'const foo =