From e2c39153957fb9c12f2ecf7ce1b59cf67f4f1d1d Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Tue, 26 Nov 2024 16:48:08 -0600 Subject: [PATCH 01/20] feat(no-misused-observables): new rule like `no-misused-promises` --- README.md | 1 + docs/rules/no-misused-observables.md | 15 ++++++ src/index.ts | 2 + src/rules/no-misused-observables.ts | 53 ++++++++++++++++++++++ tests/rules/no-misused-observables.test.ts | 48 ++++++++++++++++++++ 5 files changed, 119 insertions(+) create mode 100644 docs/rules/no-misused-observables.md create mode 100644 src/rules/no-misused-observables.ts create mode 100644 tests/rules/no-misused-observables.test.ts diff --git a/README.md b/README.md index 5c9044dc..f4ccea79 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,7 @@ The package includes the following rules. | [no-implicit-any-catch](docs/rules/no-implicit-any-catch.md) | Disallow implicit `any` error parameters in `catchError` operators. | ✅ 🔒 | 🔧 | 💡 | 💭 | | | [no-index](docs/rules/no-index.md) | Disallow importing index modules. | ✅ 🔒 | | | | | | [no-internal](docs/rules/no-internal.md) | Disallow importing internal modules. | ✅ 🔒 | 🔧 | 💡 | | | +| [no-misused-observables](docs/rules/no-misused-observables.md) | Disallow Observables in places not designed to handle them. | | | | 💭 | | | [no-nested-subscribe](docs/rules/no-nested-subscribe.md) | Disallow calling `subscribe` within a `subscribe` callback. | ✅ 🔒 | | | 💭 | | | [no-redundant-notify](docs/rules/no-redundant-notify.md) | Disallow sending redundant notifications from completed or errored observables. | ✅ 🔒 | | | 💭 | | | [no-sharereplay](docs/rules/no-sharereplay.md) | Disallow unsafe `shareReplay` usage. | ✅ 🔒 | | | | | diff --git a/docs/rules/no-misused-observables.md b/docs/rules/no-misused-observables.md new file mode 100644 index 00000000..1631391e --- /dev/null +++ b/docs/rules/no-misused-observables.md @@ -0,0 +1,15 @@ +# Disallow Observables in places not designed to handle them (`rxjs-x/no-misused-observables`) + +💭 This rule requires [type information](https://typescript-eslint.io/linting/typed-linting). + + + +## Options + + + +| Name | Description | Type | Default | +| :-------------- | :-------------------------------------- | :------ | :------ | +| `checksSpreads` | Disallow `...` spreading an Observable. | Boolean | `true` | + + diff --git a/src/index.ts b/src/index.ts index b0afe264..3bab9b8d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -27,6 +27,7 @@ import { noIgnoredTakewhileValueRule } from './rules/no-ignored-takewhile-value' import { noImplicitAnyCatchRule } from './rules/no-implicit-any-catch'; import { noIndexRule } from './rules/no-index'; import { noInternalRule } from './rules/no-internal'; +import { noMisusedObservablesRule } from './rules/no-misused-observables'; import { noNestedSubscribeRule } from './rules/no-nested-subscribe'; import { noRedundantNotifyRule } from './rules/no-redundant-notify'; import { noSharereplayRule } from './rules/no-sharereplay'; @@ -74,6 +75,7 @@ const plugin = { 'no-implicit-any-catch': noImplicitAnyCatchRule, 'no-index': noIndexRule, 'no-internal': noInternalRule, + 'no-misused-observables': noMisusedObservablesRule, 'no-nested-subscribe': noNestedSubscribeRule, 'no-redundant-notify': noRedundantNotifyRule, 'no-sharereplay': noSharereplayRule, diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts new file mode 100644 index 00000000..0a8db205 --- /dev/null +++ b/src/rules/no-misused-observables.ts @@ -0,0 +1,53 @@ +import { TSESLint } from '@typescript-eslint/utils'; +import { getTypeServices } from '../etc'; +import { ruleCreator } from '../utils'; + +// The implementation of this rule is similar to typescript-eslint's no-misused-promises. MIT License. +// https://github.com/typescript-eslint/typescript-eslint/blob/fcd6cf063a774f73ea00af23705117a197f826d4/packages/eslint-plugin/src/rules/no-misused-promises.ts + +const defaultOptions: readonly { + checksSpreads?: boolean; +}[] = []; + +export const noMisusedObservablesRule = ruleCreator({ + defaultOptions, + meta: { + docs: { + description: 'Disallow Observables in places not designed to handle them.', + requiresTypeChecking: true, + }, + messages: { + forbiddenSpread: 'Expected a non-Observable value to be spread into an object.', + }, + schema: [ + { + properties: { + checksSpreads: { type: 'boolean', default: true, description: 'Disallow `...` spreading an Observable.' }, + }, + type: 'object', + }, + ], + type: 'problem', + }, + name: 'no-misused-observables', + create: (context) => { + const { couldBeObservable } = getTypeServices(context); + const [config = {}] = context.options; + const { checksSpreads = true } = config; + + const spreadChecks: TSESLint.RuleListener = { + SpreadElement: (node) => { + if (couldBeObservable(node.argument)) { + context.report({ + messageId: 'forbiddenSpread', + node: node.argument, + }); + } + }, + }; + + return { + ...(checksSpreads ? spreadChecks : {}), + }; + }, +}); diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts new file mode 100644 index 00000000..666ca6a7 --- /dev/null +++ b/tests/rules/no-misused-observables.test.ts @@ -0,0 +1,48 @@ +import { stripIndent } from 'common-tags'; +import { noMisusedObservablesRule } from '../../src/rules/no-misused-observables'; +import { fromFixture } from '../etc'; +import { ruleTester } from '../rule-tester'; + +ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRule, { + valid: [ + { + code: stripIndent` + // spread; explicitly allowed + import { of } from "rxjs"; + + const source = of(42); + const foo = { ...source }; + `, + options: [{ checksSpreads: false }], + }, + stripIndent` + // unrelated + const foo = { bar: 42 }; + const baz = { ...foo }; + `, + ], + invalid: [ + fromFixture( + stripIndent` + // spread variable + import { of } from "rxjs"; + + const source = of(42); + const foo = { ...source }; + ~~~~~~ [forbiddenSpread] + `, + ), + fromFixture( + stripIndent` + // spread call function + import { of } from "rxjs"; + + function source() { + return of(42); + } + const foo = { ...source() }; + ~~~~~~~~ [forbiddenSpread] + `, + ), + ], +}); From 96641d1913ae3874448919b09dc46268d299cea4 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Tue, 26 Nov 2024 16:53:26 -0600 Subject: [PATCH 02/20] chore(no-misused-observables): boilerplate for void return --- docs/rules/no-misused-observables.md | 7 ++++--- src/rules/no-misused-observables.ts | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/docs/rules/no-misused-observables.md b/docs/rules/no-misused-observables.md index 1631391e..9d204f18 100644 --- a/docs/rules/no-misused-observables.md +++ b/docs/rules/no-misused-observables.md @@ -8,8 +8,9 @@ -| Name | Description | Type | Default | -| :-------------- | :-------------------------------------- | :------ | :------ | -| `checksSpreads` | Disallow `...` spreading an Observable. | Boolean | `true` | +| Name | Description | Type | Default | +| :----------------- | :-------------------------------------------------------------------------- | :------ | :------ | +| `checksSpreads` | Disallow `...` spreading an Observable. | Boolean | `true` | +| `checksVoidReturn` | Disallow returning an Observable from a function typed as returning `void`. | Boolean | `true` | diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 0a8db205..f5822c32 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -6,6 +6,7 @@ import { ruleCreator } from '../utils'; // https://github.com/typescript-eslint/typescript-eslint/blob/fcd6cf063a774f73ea00af23705117a197f826d4/packages/eslint-plugin/src/rules/no-misused-promises.ts const defaultOptions: readonly { + checksVoidReturn?: boolean; checksSpreads?: boolean; }[] = []; @@ -17,11 +18,18 @@ export const noMisusedObservablesRule = ruleCreator({ requiresTypeChecking: true, }, messages: { + forbiddenVoidReturnArgument: 'Observable returned in function argument where a void return was expected.', + forbiddenVoidReturnAttribute: 'Observable-returning function provided to attribute where a void return was expected.', + forbiddenVoidReturnInheritedMethod: 'Observable-returning method provided where a void return was expected by extended/implemented type \'{{heritageTypeName}}\'.', + forbiddenVoidReturnProperty: 'Observable-returning function provided to property where a void return was expected.', + forbiddenVoidReturnReturnValue: 'Observable-returning function provided to return value where a void return was expected.', + forbiddenVoidReturnVariable: 'Observable-returning function provided to variable where a void return was expected.', forbiddenSpread: 'Expected a non-Observable value to be spread into an object.', }, schema: [ { properties: { + checksVoidReturn: { type: 'boolean', default: true, description: 'Disallow returning an Observable from a function typed as returning `void`.' }, checksSpreads: { type: 'boolean', default: true, description: 'Disallow `...` spreading an Observable.' }, }, type: 'object', @@ -33,7 +41,11 @@ export const noMisusedObservablesRule = ruleCreator({ create: (context) => { const { couldBeObservable } = getTypeServices(context); const [config = {}] = context.options; - const { checksSpreads = true } = config; + const { checksVoidReturn = true, checksSpreads = true } = config; + + const voidReturnChecks: TSESLint.RuleListener = { + + }; const spreadChecks: TSESLint.RuleListener = { SpreadElement: (node) => { @@ -47,6 +59,7 @@ export const noMisusedObservablesRule = ruleCreator({ }; return { + ...(checksVoidReturn ? voidReturnChecks : {}), ...(checksSpreads ? spreadChecks : {}), }; }, From 2f08572fdd9b9672748d91280587ab62cf1e86e0 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Wed, 27 Nov 2024 12:57:03 -0600 Subject: [PATCH 03/20] feat(no-misused-observables): check void func args --- src/etc/get-type-services.ts | 2 +- src/rules/no-misused-observables.ts | 85 +++++++++++++++++++++- tests/rules/no-misused-observables.test.ts | 59 ++++++++++++++- 3 files changed, 141 insertions(+), 5 deletions(-) diff --git a/src/etc/get-type-services.ts b/src/etc/get-type-services.ts index 71cb1db4..4d05ddbc 100644 --- a/src/etc/get-type-services.ts +++ b/src/etc/get-type-services.ts @@ -38,7 +38,7 @@ export function getTypeServices< || ts.isMethodDeclaration(tsNode) || ts.isFunctionExpression(tsNode) ) { - tsTypeNode = tsNode.type ?? tsNode.body; + tsTypeNode = tsNode.type ?? tsNode.body; // TODO(#57): this doesn't work for Block bodies. } else if ( ts.isCallSignatureDeclaration(tsNode) || ts.isMethodSignature(tsNode) diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index f5822c32..6e4b48aa 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -1,4 +1,6 @@ -import { TSESLint } from '@typescript-eslint/utils'; +import { TSESTree as es, ESLintUtils, TSESLint } from '@typescript-eslint/utils'; +import * as tsutils from 'ts-api-utils'; +import ts from 'typescript'; import { getTypeServices } from '../etc'; import { ruleCreator } from '../utils'; @@ -39,12 +41,23 @@ export const noMisusedObservablesRule = ruleCreator({ }, name: 'no-misused-observables', create: (context) => { - const { couldBeObservable } = getTypeServices(context); + const { program, esTreeNodeToTSNodeMap } = ESLintUtils.getParserServices(context); + const checker = program.getTypeChecker(); + const { couldBeObservable, couldReturnObservable } = getTypeServices(context); const [config = {}] = context.options; const { checksVoidReturn = true, checksSpreads = true } = config; const voidReturnChecks: TSESLint.RuleListener = { - + CallExpression: checkArguments, + // NewExpression: checkArguments, + // JSXAttribute: checkJSXAttribute, + // ClassDeclaration: checkClassLikeOrInterfaceNode, + // ClassExpression: checkClassLikeOrInterfaceNode, + // TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, + // Property: checkProperty, + // ReturnStatement: checkReturnStatement, + // AssignmentExpression: checkAssignment, + // VariableDeclarator: checkVariableDeclarator, }; const spreadChecks: TSESLint.RuleListener = { @@ -58,9 +71,75 @@ export const noMisusedObservablesRule = ruleCreator({ }, }; + function checkArguments(node: es.CallExpression | es.NewExpression): void { + const tsNode = esTreeNodeToTSNodeMap.get(node); + const voidArgs = voidFunctionArguments(checker, tsNode); + if (!voidArgs.size) { + return; + } + + for (const [index, argument] of node.arguments.entries()) { + if (!voidArgs.has(index)) { + continue; + } + + if (couldReturnObservable(argument)) { + context.report({ + messageId: 'forbiddenVoidReturnArgument', + node: argument, + }); + } + } + } + return { ...(checksVoidReturn ? voidReturnChecks : {}), ...(checksSpreads ? spreadChecks : {}), }; }, }); + +function voidFunctionArguments( + checker: ts.TypeChecker, + tsNode: ts.CallExpression | ts.NewExpression, +): Set { + // let b = new Object; + if (!tsNode.arguments) { + return new Set(); + } + + const voidReturnIndices = new Set(); + const type = checker.getTypeAtLocation(tsNode.expression); + + for (const subType of tsutils.unionTypeParts(type)) { + const signatures = ts.isCallExpression(tsNode) + ? subType.getCallSignatures() + : subType.getConstructSignatures(); + for (const signature of signatures) { + for (const [index, parameter] of signature.parameters.entries()) { + const type = checker.getTypeOfSymbolAtLocation(parameter, tsNode.expression); + if (isVoidReturningFunctionType(type)) { + voidReturnIndices.add(index); + } + } + } + } + + return voidReturnIndices; +} + +function isVoidReturningFunctionType( + type: ts.Type, +): boolean { + let hasVoidReturn = false; + + for (const subType of tsutils.unionTypeParts(type)) { + for (const signature of subType.getCallSignatures()) { + const returnType = signature.getReturnType(); + + hasVoidReturn ||= tsutils.isTypeFlagSet(returnType, ts.TypeFlags.Void); + } + } + + return hasVoidReturn; +} diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index 666ca6a7..fffd782c 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -5,6 +5,27 @@ import { ruleTester } from '../rule-tester'; ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRule, { valid: [ + { + code: stripIndent` + // void return argument; explicitly allowed + import { Observable, of } from "rxjs"; + + [1, 2, 3].forEach((i): Observable => { return of(i); }); + [1, 2, 3].forEach(i => of(i)); + `, + options: [{ checksVoidReturn: false }], + }, + stripIndent` + // void return argument; unrelated + [1, 2, 3].forEach(i => i); + [1, 2, 3].forEach(i => { return i; }); + `, + stripIndent` + // couldReturnType is bugged for block body implicit return types (#57) + import { of } from "rxjs"; + + [1, 2, 3].forEach(i => { return of(i); }); + `, { code: stripIndent` // spread; explicitly allowed @@ -16,12 +37,48 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu options: [{ checksSpreads: false }], }, stripIndent` - // unrelated + // spread; unrelated const foo = { bar: 42 }; const baz = { ...foo }; `, ], invalid: [ + fromFixture( + stripIndent` + // void return argument; block body + import { Observable, of } from "rxjs"; + + [1, 2, 3].forEach((i): Observable => { return of(i); }); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnArgument] + `, + ), + fromFixture( + stripIndent` + // void return argument; inline body + import { of } from "rxjs"; + + [1, 2, 3].forEach(i => of(i)); + ~~~~~~~~~~ [forbiddenVoidReturnArgument] + `, + ), + fromFixture( + stripIndent` + // void return argument; block body; union return + import { Observable, of } from "rxjs"; + + [1, 2, 3].forEach((i): Observable | number => { if (i > 1) { return of(i); } else { return i; } }); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnArgument] + `, + ), + fromFixture( + stripIndent` + // void return argument; inline body; union return + import { of } from "rxjs"; + + [1, 2, 3].forEach(i => i > 1 ? of(i) : i); + ~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnArgument] + `, + ), fromFixture( stripIndent` // spread variable From 07b195da330b2c0590598812af7d376da788b5c7 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Wed, 27 Nov 2024 13:37:09 -0600 Subject: [PATCH 04/20] feat(no-misused-observables): check ctors too --- src/rules/no-misused-observables.ts | 2 +- tests/rules/no-misused-observables.test.ts | 23 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 6e4b48aa..e5cae02a 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -49,7 +49,7 @@ export const noMisusedObservablesRule = ruleCreator({ const voidReturnChecks: TSESLint.RuleListener = { CallExpression: checkArguments, - // NewExpression: checkArguments, + NewExpression: checkArguments, // JSXAttribute: checkJSXAttribute, // ClassDeclaration: checkClassLikeOrInterfaceNode, // ClassExpression: checkClassLikeOrInterfaceNode, diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index fffd782c..c98ccdb6 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -12,6 +12,11 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu [1, 2, 3].forEach((i): Observable => { return of(i); }); [1, 2, 3].forEach(i => of(i)); + + class Foo { + constructor(x: () => void) {} + } + new Foo(() => of(42)); `, options: [{ checksVoidReturn: false }], }, @@ -19,6 +24,12 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu // void return argument; unrelated [1, 2, 3].forEach(i => i); [1, 2, 3].forEach(i => { return i; }); + + class Foo { + constructor(x: () => void) {} + } + new Foo(() => 42); + new Foo; `, stripIndent` // couldReturnType is bugged for block body implicit return types (#57) @@ -79,6 +90,18 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu ~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnArgument] `, ), + fromFixture( + stripIndent` + // void return argument; constructor + import { of } from "rxjs"; + + class Foo { + constructor(x: () => void) {} + } + new Foo(() => of(42)); + ~~~~~~~~~~~~ [forbiddenVoidReturnArgument] + `, + ), fromFixture( stripIndent` // spread variable From 1df757118895edab33d1a5377fb20ccdfa593619 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Wed, 27 Nov 2024 13:42:24 -0600 Subject: [PATCH 05/20] test(no-async-subscribe): only enable jsx when needed --- tests/rules/no-async-subscribe.test.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/rules/no-async-subscribe.test.ts b/tests/rules/no-async-subscribe.test.ts index 8a1f7129..15e39997 100644 --- a/tests/rules/no-async-subscribe.test.ts +++ b/tests/rules/no-async-subscribe.test.ts @@ -3,7 +3,7 @@ import { noAsyncSubscribeRule } from '../../src/rules/no-async-subscribe'; import { fromFixture } from '../etc'; import { ruleTester } from '../rule-tester'; -ruleTester({ types: true, jsx: true }).run('no-async-subscribe', noAsyncSubscribeRule, { +ruleTester({ types: true }).run('no-async-subscribe', noAsyncSubscribeRule, { valid: [ stripIndent` // sync arrow function @@ -17,12 +17,15 @@ ruleTester({ types: true, jsx: true }).run('no-async-subscribe', noAsyncSubscrib of("a").subscribe(function() {}); `, - stripIndent` - // https://github.com/cartant/eslint-plugin-rxjs/issues/46 - import React, { FC } from "react"; - const SomeComponent: FC<{}> = () => some component; - const someElement = ; - `, + { + code: stripIndent` + // https://github.com/cartant/eslint-plugin-rxjs/issues/46 + import React, { FC } from "react"; + const SomeComponent: FC<{}> = () => some component; + const someElement = ; + `, + languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, + }, stripIndent` // https://github.com/cartant/eslint-plugin-rxjs/issues/61 const whatever = { From a57f488945d1d72cb937aea72029a33bbf816e7a Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Wed, 27 Nov 2024 14:44:44 -0600 Subject: [PATCH 06/20] feat(no-misused-observables): check jsx attributes --- src/etc/is.ts | 4 ++ src/rules/no-misused-observables.ts | 17 ++++++- tests/rules/no-misused-observables.test.ts | 58 ++++++++++++++++++++++ 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/etc/is.ts b/src/etc/is.ts index 1171d708..3e6cd5fa 100644 --- a/src/etc/is.ts +++ b/src/etc/is.ts @@ -78,6 +78,10 @@ export function isImportSpecifier(node: TSESTree.Node): node is TSESTree.ImportS return node.type === AST_NODE_TYPES.ImportSpecifier; } +export function isJSXExpressionContainer(node: TSESTree.Node): node is TSESTree.JSXExpressionContainer { + return node.type === AST_NODE_TYPES.JSXExpressionContainer; +} + export function isLiteral(node: TSESTree.Node): node is TSESTree.Literal { return node.type === AST_NODE_TYPES.Literal; } diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index e5cae02a..80e9b8b2 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -1,7 +1,7 @@ import { TSESTree as es, ESLintUtils, TSESLint } from '@typescript-eslint/utils'; import * as tsutils from 'ts-api-utils'; import ts from 'typescript'; -import { getTypeServices } from '../etc'; +import { getTypeServices, isJSXExpressionContainer } from '../etc'; import { ruleCreator } from '../utils'; // The implementation of this rule is similar to typescript-eslint's no-misused-promises. MIT License. @@ -50,7 +50,7 @@ export const noMisusedObservablesRule = ruleCreator({ const voidReturnChecks: TSESLint.RuleListener = { CallExpression: checkArguments, NewExpression: checkArguments, - // JSXAttribute: checkJSXAttribute, + JSXAttribute: checkJSXAttribute, // ClassDeclaration: checkClassLikeOrInterfaceNode, // ClassExpression: checkClassLikeOrInterfaceNode, // TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, @@ -92,6 +92,19 @@ export const noMisusedObservablesRule = ruleCreator({ } } + function checkJSXAttribute(node: es.JSXAttribute): void { + if (!node.value || !isJSXExpressionContainer(node.value)) { + return; + } + + if (couldReturnObservable(node.value.expression)) { + context.report({ + messageId: 'forbiddenVoidReturnAttribute', + node: node.value, + }); + } + } + return { ...(checksVoidReturn ? voidReturnChecks : {}), ...(checksSpreads ? spreadChecks : {}), diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index c98ccdb6..5d66b02e 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -37,6 +37,32 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu [1, 2, 3].forEach(i => { return of(i); }); `, + { + code: stripIndent` + // void return attribute; explicitly allowed + import { Observable, of } from "rxjs"; + import React, { FC } from "react"; + + const Component: FC<{ foo: () => void }> = () =>
; + return ( + of(42)} /> + ); + `, + options: [{ checksVoidReturn: false }], + languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, + }, + { + code: stripIndent` + // void return attribute; unrelated + import React, { FC } from "react"; + + const Component: FC<{ foo: () => void }> = () =>
; + return ( + 42} /> + ); + `, + languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, + }, { code: stripIndent` // spread; explicitly allowed @@ -102,6 +128,38 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu ~~~~~~~~~~~~ [forbiddenVoidReturnArgument] `, ), + fromFixture( + stripIndent` + // void return attribute; block body + import { Observable, of } from "rxjs"; + import React, { FC } from "react"; + + const Component: FC<{ foo: () => void }> = () =>
; + return ( + => { return of(42); }} /> + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnAttribute] + ); + `, + { + languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, + }, + ), + fromFixture( + stripIndent` + // void return attribute; inline body + import { Observable, of } from "rxjs"; + import React, { FC } from "react"; + + const Component: FC<{ foo: () => void }> = () =>
; + return ( + of(42)} /> + ~~~~~~~~~~~~~~ [forbiddenVoidReturnAttribute] + ); + `, + { + languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, + }, + ), fromFixture( stripIndent` // spread variable From ba42cfed669dd486f2511cab30536fb43efcd9cf Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Wed, 27 Nov 2024 15:22:27 -0600 Subject: [PATCH 07/20] test(no-misused-observables): regions for scenarios --- tests/rules/no-misused-observables.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index 5d66b02e..c27b9a3a 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -5,6 +5,7 @@ import { ruleTester } from '../rule-tester'; ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRule, { valid: [ + // #region valid; void return argument { code: stripIndent` // void return argument; explicitly allowed @@ -37,6 +38,8 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu [1, 2, 3].forEach(i => { return of(i); }); `, + // #endregion valid; void return argument + // #region valid; void return attribute { code: stripIndent` // void return attribute; explicitly allowed @@ -63,6 +66,8 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu `, languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, }, + // #endregion valid; void return attribute + // #region valid; spread { code: stripIndent` // spread; explicitly allowed @@ -78,8 +83,10 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu const foo = { bar: 42 }; const baz = { ...foo }; `, + // #endregion valid; spread ], invalid: [ + // #region invalid; void return argument fromFixture( stripIndent` // void return argument; block body @@ -128,6 +135,8 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu ~~~~~~~~~~~~ [forbiddenVoidReturnArgument] `, ), + // #endregion invalid; void return argument + // #region invalid; void return attribute fromFixture( stripIndent` // void return attribute; block body @@ -160,6 +169,8 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, }, ), + // #endregion invalid; void return attribute + // #region invalid; spread fromFixture( stripIndent` // spread variable @@ -182,5 +193,6 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu ~~~~~~~~ [forbiddenSpread] `, ), + // #endregion invalid; spread ], }); From 1f23005309b41b0edcaa29702b85e6b64a190910 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Wed, 27 Nov 2024 15:56:15 -0600 Subject: [PATCH 08/20] feat(no-misused-observables): check class declarations --- src/etc/is.ts | 8 + src/rules/no-misused-observables.ts | 72 ++++++++- tests/rules/no-misused-observables.test.ts | 172 +++++++++++++++++++++ 3 files changed, 250 insertions(+), 2 deletions(-) diff --git a/src/etc/is.ts b/src/etc/is.ts index 3e6cd5fa..1bbaca16 100644 --- a/src/etc/is.ts +++ b/src/etc/is.ts @@ -90,6 +90,10 @@ export function isMemberExpression(node: TSESTree.Node): node is TSESTree.Member return node.type === AST_NODE_TYPES.MemberExpression; } +export function isMethodDefinition(node: TSESTree.Node): node is TSESTree.MethodDefinition { + return node.type === AST_NODE_TYPES.MethodDefinition; +} + export function isNewExpression(node: TSESTree.Node): node is TSESTree.NewExpression { return node.type === AST_NODE_TYPES.NewExpression; } @@ -110,6 +114,10 @@ export function isProperty(node: TSESTree.Node): node is TSESTree.Property { return node.type === AST_NODE_TYPES.Property; } +export function isPropertyDefinition(node: TSESTree.Node): node is TSESTree.PropertyDefinition { + return node.type === AST_NODE_TYPES.PropertyDefinition; +} + export function isPrivateIdentifier( node: TSESTree.Node, ): node is TSESTree.PrivateIdentifier { diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 80e9b8b2..6f9185a4 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -1,7 +1,7 @@ import { TSESTree as es, ESLintUtils, TSESLint } from '@typescript-eslint/utils'; import * as tsutils from 'ts-api-utils'; import ts from 'typescript'; -import { getTypeServices, isJSXExpressionContainer } from '../etc'; +import { getTypeServices, isJSXExpressionContainer, isMethodDefinition, isPropertyDefinition } from '../etc'; import { ruleCreator } from '../utils'; // The implementation of this rule is similar to typescript-eslint's no-misused-promises. MIT License. @@ -51,7 +51,7 @@ export const noMisusedObservablesRule = ruleCreator({ CallExpression: checkArguments, NewExpression: checkArguments, JSXAttribute: checkJSXAttribute, - // ClassDeclaration: checkClassLikeOrInterfaceNode, + ClassDeclaration: checkClassLikeOrInterfaceNode, // ClassExpression: checkClassLikeOrInterfaceNode, // TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, // Property: checkProperty, @@ -105,6 +105,51 @@ export const noMisusedObservablesRule = ruleCreator({ } } + function checkClassLikeOrInterfaceNode( + node: es.ClassDeclaration | es.ClassExpression | es.TSInterfaceDeclaration, + ): void { + const tsNode = esTreeNodeToTSNodeMap.get(node); + + const heritageTypes = getHeritageTypes(checker, tsNode); + if (!heritageTypes?.length) { + return; + } + + for (const element of node.body.body) { + const tsElement = esTreeNodeToTSNodeMap.get(element); + const memberName = tsElement?.name?.getText(); + if (memberName === undefined) { + // See comment in typescript-eslint no-misused-promises for why. + continue; + } + + if (!couldReturnObservable(element)) { + continue; + } + + if (isStaticMember(element)) { + continue; + } + + for (const heritageType of heritageTypes) { + const heritageMember = getMemberIfExists(heritageType, memberName); + if (heritageMember === undefined) { + continue; + } + const memberType = checker.getTypeOfSymbolAtLocation(heritageMember, tsElement); + if (!isVoidReturningFunctionType(memberType)) { + continue; + } + + context.report({ + messageId: 'forbiddenVoidReturnInheritedMethod', + node: element, + data: { heritageTypeName: checker.typeToString(heritageType) }, + }); + } + } + } + return { ...(checksVoidReturn ? voidReturnChecks : {}), ...(checksSpreads ? spreadChecks : {}), @@ -156,3 +201,26 @@ function isVoidReturningFunctionType( return hasVoidReturn; } + +function getHeritageTypes( + checker: ts.TypeChecker, + tsNode: ts.ClassDeclaration | ts.ClassExpression | ts.InterfaceDeclaration, +): ts.Type[] | undefined { + return tsNode.heritageClauses + ?.flatMap(clause => clause.types) + .map(typeExpressions => checker.getTypeAtLocation(typeExpressions)); +} + +function getMemberIfExists( + type: ts.Type, + memberName: string, +): ts.Symbol | undefined { + const escapedMemberName = ts.escapeLeadingUnderscores(memberName); + const symbolMemberMatch = type.getSymbol()?.members?.get(escapedMemberName); + return symbolMemberMatch ?? tsutils.getPropertyOfType(type, escapedMemberName); +} + +function isStaticMember(node: es.Node): boolean { + return (isMethodDefinition(node) || isPropertyDefinition(node)) + && node.static; +} diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index c27b9a3a..5dfafa0d 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -67,6 +67,33 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, }, // #endregion valid; void return attribute + // #region valid; void return inherited method + { + code: stripIndent` + // void return inherited method; explicitly allowed + import { Observable, of } from "rxjs"; + + class Foo { + foo(): void {} + } + + class Bar extends Foo { + foo(): Observable { return of(42); } + } + `, + options: [{ checksVoidReturn: false }], + }, + stripIndent` + // void return inherited method; unrelated + class Foo { + foo(): void {} + } + + class Bar extends Foo { + foo(): number { return 42; } + } + `, + // #endregion valid; void return inherited method // #region valid; spread { code: stripIndent` @@ -170,6 +197,151 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu }, ), // #endregion invalid; void return attribute + // #region invalid; void return inherited method + fromFixture( + stripIndent` + // void return inherited method; class declaration; extends + import { Observable, of } from "rxjs"; + + class Foo { + foo(): void {} + } + + class Bar extends Foo { + foo(): Observable { return of(42); } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class declaration; abstract extends + import { Observable } from "rxjs"; + + class Foo { + foo(): void {} + } + + abstract class Bar extends Foo { + abstract foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class declaration; extends abstract + import { Observable, of } from "rxjs"; + + abstract class Foo { + abstract foo(): void; + } + + class Bar extends Foo { + foo(): Observable { return of(42); } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class declaration; abstract extends abstract + import { Observable } from "rxjs"; + + abstract class Foo { + abstract foo(): void; + } + + abstract class Bar extends Foo { + abstract foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class declaration; implements + import { Observable, of } from "rxjs"; + + interface Foo { + foo(): void; + } + + class Bar implements Foo { + foo(): Observable { return of(42); } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class declaration; abstract implements + import { Observable } from "rxjs"; + + interface Foo { + foo(): void; + } + + abstract class Bar implements Foo { + abstract foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class declaration; implements type intersection + import { Observable, of } from "rxjs"; + + type Foo = { foo(): void } & { bar(): void }; + + class Bar implements Foo { + foo(): Observable { return of(42); } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + bar(): void {} + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class declaration; extends and implements + import { Observable, of } from "rxjs"; + + interface Foo { + foo(): Observable; + } + + interface Bar { + foo(): void; + } + + class Baz { + foo(): void {} + } + + class Qux extends Baz implements Foo, Bar { + foo(): Observable { return of(42); } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Baz" }] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Bar" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class declaration; extends class expression + import { Observable, of } from "rxjs"; + + const Foo = class { + foo(): void {} + } + + class Bar extends Foo { + foo(): Observable { return of(42); } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + // #endregion invalid; void return inherited method // #region invalid; spread fromFixture( stripIndent` From 9882bd9a7633915aeb9514caad45a3354495bc74 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Thu, 28 Nov 2024 16:40:42 -0600 Subject: [PATCH 09/20] feat(no-misused-observables): class expressions --- src/rules/no-misused-observables.ts | 8 ++--- tests/rules/no-misused-observables.test.ts | 34 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 6f9185a4..0a5b0703 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -1,4 +1,4 @@ -import { TSESTree as es, ESLintUtils, TSESLint } from '@typescript-eslint/utils'; +import { TSESTree as es, TSESLint as eslint, ESLintUtils } from '@typescript-eslint/utils'; import * as tsutils from 'ts-api-utils'; import ts from 'typescript'; import { getTypeServices, isJSXExpressionContainer, isMethodDefinition, isPropertyDefinition } from '../etc'; @@ -47,12 +47,12 @@ export const noMisusedObservablesRule = ruleCreator({ const [config = {}] = context.options; const { checksVoidReturn = true, checksSpreads = true } = config; - const voidReturnChecks: TSESLint.RuleListener = { + const voidReturnChecks: eslint.RuleListener = { CallExpression: checkArguments, NewExpression: checkArguments, JSXAttribute: checkJSXAttribute, ClassDeclaration: checkClassLikeOrInterfaceNode, - // ClassExpression: checkClassLikeOrInterfaceNode, + ClassExpression: checkClassLikeOrInterfaceNode, // TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, // Property: checkProperty, // ReturnStatement: checkReturnStatement, @@ -60,7 +60,7 @@ export const noMisusedObservablesRule = ruleCreator({ // VariableDeclarator: checkVariableDeclarator, }; - const spreadChecks: TSESLint.RuleListener = { + const spreadChecks: eslint.RuleListener = { SpreadElement: (node) => { if (couldBeObservable(node.argument)) { context.report({ diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index 5dfafa0d..03cd815e 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -80,6 +80,10 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu class Bar extends Foo { foo(): Observable { return of(42); } } + + const Baz = class extends Foo { + foo(): Observable { return of(42); } + } `, options: [{ checksVoidReturn: false }], }, @@ -341,6 +345,36 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu } `, ), + fromFixture( + stripIndent` + // void return inherited method; class expression; extends + import { Observable, of } from "rxjs"; + + const Foo = class { + foo(): void {} + } + + const Bar = class extends Foo { + foo(): Observable { return of(42); } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; class expression; implements + import { Observable, of } from "rxjs"; + + interface Foo { + foo(): void; + } + + const Bar = class implements Foo { + foo(): Observable { return of(42); } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), // #endregion invalid; void return inherited method // #region invalid; spread fromFixture( From 02b8925ff3bb5c9c0793a5641609feaacdf70a03 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Mon, 2 Dec 2024 09:49:52 -0600 Subject: [PATCH 10/20] feat(no-misused-observables): interface declarations --- src/etc/get-type-services.ts | 4 + src/rules/no-misused-observables.ts | 2 +- tests/rules/no-misused-observables.test.ts | 208 +++++++++++++++++++++ 3 files changed, 213 insertions(+), 1 deletion(-) diff --git a/src/etc/get-type-services.ts b/src/etc/get-type-services.ts index 4d05ddbc..5f38ba6c 100644 --- a/src/etc/get-type-services.ts +++ b/src/etc/get-type-services.ts @@ -44,6 +44,10 @@ export function getTypeServices< || ts.isMethodSignature(tsNode) ) { tsTypeNode = tsNode.type; + } else if ( + ts.isPropertySignature(tsNode) + ) { + // TODO(#66): this doesn't work for functions assigned to class properties, variables, params. } return Boolean( tsTypeNode diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 0a5b0703..17c13f7c 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -53,7 +53,7 @@ export const noMisusedObservablesRule = ruleCreator({ JSXAttribute: checkJSXAttribute, ClassDeclaration: checkClassLikeOrInterfaceNode, ClassExpression: checkClassLikeOrInterfaceNode, - // TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, + TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, // Property: checkProperty, // ReturnStatement: checkReturnStatement, // AssignmentExpression: checkAssignment, diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index 03cd815e..df380400 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -84,9 +84,33 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu const Baz = class extends Foo { foo(): Observable { return of(42); } } + + interface Qux extends Foo { + foo(): Observable; + } `, options: [{ checksVoidReturn: false }], }, + stripIndent` + // void return inherited method; not void + import { Observable, of } from "rxjs"; + + class Foo { + foo(): Observable { return of(42); } + } + + class Bar extends Foo { + foo(): Observable { return of(43); } + } + + const Baz = class extends Foo { + foo(): Observable { return of(44); } + } + + interface Qux extends Foo { + foo(): Observable<45>; + } + `, stripIndent` // void return inherited method; unrelated class Foo { @@ -375,6 +399,190 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu } `, ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends class + import { Observable } from "rxjs"; + + class Foo { + foo(): void {} + } + + interface Bar extends Foo { + foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends abstract + import { Observable } from "rxjs"; + + abstract class Foo { + abstract foo(): void; + } + + interface Bar extends Foo { + foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends interface + import { Observable } from "rxjs"; + + interface Foo { + foo(): void; + } + + interface Bar extends Foo { + foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends conditional type + import { Observable } from "rxjs"; + + type Foo = IsRx extends true + ? { foo(): Observable } + : { foo(): void }; + + interface Bar extends Foo { + foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "{ foo(): void; }" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends multiple + import { Observable } from "rxjs"; + + interface Foo { + foo(): void; + } + + interface Bar { + foo(): void; + } + + interface Baz extends Foo, Bar { + foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + ~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Bar" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends multiple classes + import { Observable } from "rxjs"; + + class Foo { + foo(): void {} + } + + class Bar { + foo(): void {} + } + + interface Baz extends Foo, Bar { + foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + ~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Bar" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends typeof class + import { Observable } from "rxjs"; + + const Foo = class { + foo(): void {} + } + + type Bar = typeof Foo; + + interface Baz extends Bar { + foo(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "typeof Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends function, index, constructor + import { Observable } from "rxjs"; + + interface Foo { + (): void; + (arg: string): void; + new (): void; + [key: string]: () => void; + [key: number]: () => void; + myMethod(): void; + } + + interface Bar extends Foo { + (): Observable; + (arg: string): Observable; + new (): Observable; + [key: string]: () => Observable; + [key: number]: () => Observable; + myMethod(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Foo" }] + } + `, + ), + fromFixture( + stripIndent` + // void return inherited method; interface; extends multiple function, index, constructor + import { Observable } from "rxjs"; + + interface Foo { + (): void; + (arg: string): void; + } + + interface Bar { + [key: string]: () => void; + [key: number]: () => void; + } + + interface Baz { + new (): void; + new (arg: string): void; + } + + interface Qux { + doSyncThing(): void; + doOtherSyncThing(): void; + syncMethodProperty: () => void; + } + + interface Quux extends Foo, Bar, Baz, Qux { + (): void; + (arg: string): Observable; + new (): void; + new (arg: string): void; + [key: string]: () => Observable; + [key: number]: () => void; + doSyncThing(): Observable; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Qux" }] + doRxThing(): Observable; + syncMethodProperty: () => Observable; + //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnInheritedMethod { "heritageTypeName": "Qux" }] + // TODO(#66): couldReturnType doesn't work for properties. + } + `, + ), // #endregion invalid; void return inherited method // #region invalid; spread fromFixture( From 0a8af2cea874e8e93a02eeaade9df404cb2d2d91 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Mon, 2 Dec 2024 14:37:43 -0600 Subject: [PATCH 11/20] feat(no-misused-observables): properties --- src/rules/no-misused-observables.ts | 56 +++++++++++++++- tests/rules/no-misused-observables.test.ts | 75 ++++++++++++++++++++++ 2 files changed, 130 insertions(+), 1 deletion(-) diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 17c13f7c..7b58aa61 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -54,7 +54,7 @@ export const noMisusedObservablesRule = ruleCreator({ ClassDeclaration: checkClassLikeOrInterfaceNode, ClassExpression: checkClassLikeOrInterfaceNode, TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, - // Property: checkProperty, + Property: checkProperty, // ReturnStatement: checkReturnStatement, // AssignmentExpression: checkAssignment, // VariableDeclarator: checkVariableDeclarator, @@ -150,6 +150,27 @@ export const noMisusedObservablesRule = ruleCreator({ } } + function checkProperty(node: es.Property): void { + const tsNode = esTreeNodeToTSNodeMap.get(node); + + const contextualType = getPropertyContextualType(checker, tsNode); + if (contextualType === undefined) { + return; + } + + if (!isVoidReturningFunctionType(contextualType)) { + return; + } + if (!couldReturnObservable(node.value)) { + return; + } + + context.report({ + messageId: 'forbiddenVoidReturnProperty', + node: node.value, + }); + } + return { ...(checksVoidReturn ? voidReturnChecks : {}), ...(checksSpreads ? spreadChecks : {}), @@ -224,3 +245,36 @@ function isStaticMember(node: es.Node): boolean { return (isMethodDefinition(node) || isPropertyDefinition(node)) && node.static; } + +function getPropertyContextualType( + checker: ts.TypeChecker, + tsNode: ts.Node, +): ts.Type | undefined { + if (ts.isPropertyAssignment(tsNode)) { + // { a: 1 } + return checker.getContextualType(tsNode.initializer); + } else if (ts.isShorthandPropertyAssignment(tsNode)) { + // { a } + return checker.getContextualType(tsNode.name); + } else if (ts.isMethodDeclaration(tsNode)) { + // { a() {} } + if (ts.isComputedPropertyName(tsNode.name)) { + return; + } + const obj = tsNode.parent; + if (!ts.isObjectLiteralExpression(obj)) { + return; + } + const objType = checker.getContextualType(obj); + if (objType === undefined) { + return; + } + const propertySymbol = checker.getPropertyOfType(objType, tsNode.name.text); + if (propertySymbol === undefined) { + return; + } + return checker.getTypeOfSymbolAtLocation(propertySymbol, tsNode.name); + } else { + return undefined; + } +} diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index df380400..234f2a84 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -122,6 +122,55 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu } `, // #endregion valid; void return inherited method + // #region valid; void return property + { + code: stripIndent` + // void return property; explicitly allowed + import { Observable, of } from "rxjs"; + + type Foo = { a: () => void, b: () => void, c: () => void }; + const b: () => Observable = () => of(42); + const foo: Foo = { + a: (): Observable => of(42), + b, + c(): Observable { return of(42); }, + }; + `, + options: [{ checksVoidReturn: false }], + }, + stripIndent` + // void return property; not void + import { Observable, of } from "rxjs"; + + type Foo = { a: () => Observable, b: () => Observable, c: () => Observable }; + const b: () => Observable = () => of(42); + const foo: Foo = { + a: () => of(42), + b, + c(): Observable { return of(42); }, + }; + `, + stripIndent` + // void return property; unrelated + type Foo = { a: () => void, b: () => void, c: () => void }; + const b: () => number = () => 42; + const foo: Foo = { + a: () => 42, + b, + c(): number { return 42; }, + }; + `, + stripIndent` + // couldReturnType is bugged for variables (#66) + import { Observable, of } from "rxjs"; + + type Foo = { bar: () => void }; + const bar: () => Observable = () => of(42); + const foo: Foo = { + bar, + }; + `, + // #endregion valid; void return property // #region valid; spread { code: stripIndent` @@ -584,6 +633,32 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu `, ), // #endregion invalid; void return inherited method + // #region invalid; void return property + fromFixture( + stripIndent` + // void return property; arrow function + import { of } from "rxjs"; + + type Foo = { bar: () => void }; + const foo: Foo = { + bar: () => of(42), + ~~~~~~~~~~~~ [forbiddenVoidReturnProperty] + }; + `, + ), + fromFixture( + stripIndent` + // void return property; function + import { Observable, of } from "rxjs"; + + type Foo = { bar: () => void }; + const foo: Foo = { + bar(): Observable { return of(42); }, + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnProperty] + }; + `, + ), + // #endregion invalid; void return property // #region invalid; spread fromFixture( stripIndent` From ad628d278210f76ada52d5ca9ad90e74fbbe5aca Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Mon, 2 Dec 2024 15:59:17 -0600 Subject: [PATCH 12/20] feat(no-misused-observables): void return return values Also fixed invalid JSX. --- src/rules/no-misused-observables.ts | 120 ++++++++++++++++++++- tests/rules/no-misused-observables.test.ts | 90 +++++++++++++--- 2 files changed, 193 insertions(+), 17 deletions(-) diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 7b58aa61..10ca3693 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -1,7 +1,15 @@ -import { TSESTree as es, TSESLint as eslint, ESLintUtils } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES, TSESTree as es, TSESLint as eslint, ESLintUtils } from '@typescript-eslint/utils'; import * as tsutils from 'ts-api-utils'; import ts from 'typescript'; -import { getTypeServices, isJSXExpressionContainer, isMethodDefinition, isPropertyDefinition } from '../etc'; +import { + getTypeServices, + isArrowFunctionExpression, + isFunctionDeclaration, + isFunctionExpression, + isJSXExpressionContainer, + isMethodDefinition, + isPropertyDefinition, +} from '../etc'; import { ruleCreator } from '../utils'; // The implementation of this rule is similar to typescript-eslint's no-misused-promises. MIT License. @@ -55,7 +63,7 @@ export const noMisusedObservablesRule = ruleCreator({ ClassExpression: checkClassLikeOrInterfaceNode, TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, Property: checkProperty, - // ReturnStatement: checkReturnStatement, + ReturnStatement: checkReturnStatement, // AssignmentExpression: checkAssignment, // VariableDeclarator: checkVariableDeclarator, }; @@ -171,6 +179,50 @@ export const noMisusedObservablesRule = ruleCreator({ }); } + function checkReturnStatement(node: es.ReturnStatement): void { + const tsNode = esTreeNodeToTSNodeMap.get(node); + if (tsNode.expression === undefined || !node.argument) { + return; + } + + // Optimization to avoid touching type info. + function getFunctionNode() { + let current: es.Node | undefined = node.parent; + while ( + current + && !isArrowFunctionExpression(current) + && !isFunctionExpression(current) + && !isFunctionDeclaration(current) + ) { + current = current.parent; + } + return current; + } + const functionNode = getFunctionNode(); + if ( + functionNode?.returnType + && !isPossiblyFunctionType(functionNode.returnType) + ) { + return; + } + + const contextualType = checker.getContextualType(tsNode.expression); + if (contextualType === undefined) { + return; + } + if (!isVoidReturningFunctionType(contextualType)) { + return; + } + if (!couldReturnObservable(node.argument)) { + return; + } + + context.report({ + node: node.argument, + messageId: 'forbiddenVoidReturnReturnValue', + }); + } + return { ...(checksVoidReturn ? voidReturnChecks : {}), ...(checksSpreads ? spreadChecks : {}), @@ -278,3 +330,65 @@ function getPropertyContextualType( return undefined; } } + +/** + * From no-misused-promises. + */ +function isPossiblyFunctionType(node: es.TSTypeAnnotation): boolean { + switch (node.typeAnnotation.type) { + case AST_NODE_TYPES.TSConditionalType: + case AST_NODE_TYPES.TSConstructorType: + case AST_NODE_TYPES.TSFunctionType: + case AST_NODE_TYPES.TSImportType: + case AST_NODE_TYPES.TSIndexedAccessType: + case AST_NODE_TYPES.TSInferType: + case AST_NODE_TYPES.TSIntersectionType: + case AST_NODE_TYPES.TSQualifiedName: + case AST_NODE_TYPES.TSThisType: + case AST_NODE_TYPES.TSTypeOperator: + case AST_NODE_TYPES.TSTypeQuery: + case AST_NODE_TYPES.TSTypeReference: + case AST_NODE_TYPES.TSUnionType: + return true; + + case AST_NODE_TYPES.TSTypeLiteral: + return node.typeAnnotation.members.some( + member => + member.type === AST_NODE_TYPES.TSCallSignatureDeclaration + || member.type === AST_NODE_TYPES.TSConstructSignatureDeclaration, + ); + + case AST_NODE_TYPES.TSAbstractKeyword: + case AST_NODE_TYPES.TSAnyKeyword: + case AST_NODE_TYPES.TSArrayType: + case AST_NODE_TYPES.TSAsyncKeyword: + case AST_NODE_TYPES.TSBigIntKeyword: + case AST_NODE_TYPES.TSBooleanKeyword: + case AST_NODE_TYPES.TSDeclareKeyword: + case AST_NODE_TYPES.TSExportKeyword: + case AST_NODE_TYPES.TSIntrinsicKeyword: + case AST_NODE_TYPES.TSLiteralType: + case AST_NODE_TYPES.TSMappedType: + case AST_NODE_TYPES.TSNamedTupleMember: + case AST_NODE_TYPES.TSNeverKeyword: + case AST_NODE_TYPES.TSNullKeyword: + case AST_NODE_TYPES.TSNumberKeyword: + case AST_NODE_TYPES.TSObjectKeyword: + case AST_NODE_TYPES.TSOptionalType: + case AST_NODE_TYPES.TSPrivateKeyword: + case AST_NODE_TYPES.TSProtectedKeyword: + case AST_NODE_TYPES.TSPublicKeyword: + case AST_NODE_TYPES.TSReadonlyKeyword: + case AST_NODE_TYPES.TSRestType: + case AST_NODE_TYPES.TSStaticKeyword: + case AST_NODE_TYPES.TSStringKeyword: + case AST_NODE_TYPES.TSSymbolKeyword: + case AST_NODE_TYPES.TSTemplateLiteralType: + case AST_NODE_TYPES.TSTupleType: + case AST_NODE_TYPES.TSTypePredicate: + case AST_NODE_TYPES.TSUndefinedKeyword: + case AST_NODE_TYPES.TSUnknownKeyword: + case AST_NODE_TYPES.TSVoidKeyword: + return false; + } +} diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index 234f2a84..f1f316ea 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -47,9 +47,11 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu import React, { FC } from "react"; const Component: FC<{ foo: () => void }> = () =>
; - return ( - of(42)} /> - ); + const App = () => { + return ( + of(42)} /> + ); + }; `, options: [{ checksVoidReturn: false }], languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, @@ -60,9 +62,11 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu import React, { FC } from "react"; const Component: FC<{ foo: () => void }> = () =>
; - return ( - 42} /> - ); + const App = () => { + return ( + 42} /> + ); + }; `, languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, }, @@ -171,6 +175,36 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu }; `, // #endregion valid; void return property + // #region valid; void return return value + { + code: stripIndent` + // void return return value; explicitly allowed + import { Observable, of } from "rxjs"; + + function foo(): () => void { + return (): Observable => of(42); + } + `, + options: [{ checksVoidReturn: false }], + }, + stripIndent` + // void return return value; not void + import { Observable, of } from "rxjs"; + + function foo(): () => Observable { + return (): Observable => of(42); + } + `, + stripIndent` + // void return return value; unrelated + function foo(): () => number { + return (): number => 42; + } + function bar(): () => void { + return (): number => 42; + } + `, + // #endregion valid; void return return value // #region valid; spread { code: stripIndent` @@ -248,10 +282,12 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu import React, { FC } from "react"; const Component: FC<{ foo: () => void }> = () =>
; - return ( - => { return of(42); }} /> - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnAttribute] - ); + const App = () => { + return ( + => { return of(42); }} /> + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnAttribute] + ); + }; `, { languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, @@ -264,10 +300,12 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu import React, { FC } from "react"; const Component: FC<{ foo: () => void }> = () =>
; - return ( - of(42)} /> - ~~~~~~~~~~~~~~ [forbiddenVoidReturnAttribute] - ); + const App = () => { + return ( + of(42)} /> + ~~~~~~~~~~~~~~ [forbiddenVoidReturnAttribute] + ); + }; `, { languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, @@ -659,6 +697,30 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu `, ), // #endregion invalid; void return property + // #region invalid; void return return value + fromFixture( + stripIndent` + // void return return value; arrow function + import { Observable, of } from "rxjs"; + + function foo(): () => void { + return (): Observable => of(42); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnReturnValue] + } + `, + ), + fromFixture( + stripIndent` + // void return return value; function + import { Observable, of } from "rxjs"; + + function foo(): () => void { + return function(): Observable { return of(42); }; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [forbiddenVoidReturnReturnValue] + } + `, + ), + // #endregion invalid; void return return value // #region invalid; spread fromFixture( stripIndent` From f442dbdc1393b9139e41dce022a82b2029d02917 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Mon, 2 Dec 2024 16:24:30 -0600 Subject: [PATCH 13/20] feat(no-misused-observables): assignments --- src/rules/no-misused-observables.ts | 18 ++++++- tests/rules/no-misused-observables.test.ts | 58 ++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 10ca3693..c52b0769 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -49,7 +49,7 @@ export const noMisusedObservablesRule = ruleCreator({ }, name: 'no-misused-observables', create: (context) => { - const { program, esTreeNodeToTSNodeMap } = ESLintUtils.getParserServices(context); + const { program, esTreeNodeToTSNodeMap, getTypeAtLocation } = ESLintUtils.getParserServices(context); const checker = program.getTypeChecker(); const { couldBeObservable, couldReturnObservable } = getTypeServices(context); const [config = {}] = context.options; @@ -64,7 +64,7 @@ export const noMisusedObservablesRule = ruleCreator({ TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, Property: checkProperty, ReturnStatement: checkReturnStatement, - // AssignmentExpression: checkAssignment, + AssignmentExpression: checkAssignment, // VariableDeclarator: checkVariableDeclarator, }; @@ -223,6 +223,20 @@ export const noMisusedObservablesRule = ruleCreator({ }); } + function checkAssignment(node: es.AssignmentExpression): void { + const varType = getTypeAtLocation(node.left); + if (!isVoidReturningFunctionType(varType)) { + return; + } + + if (couldReturnObservable(node.right)) { + context.report({ + messageId: 'forbiddenVoidReturnVariable', + node: node.right, + }); + } + } + return { ...(checksVoidReturn ? voidReturnChecks : {}), ...(checksSpreads ? spreadChecks : {}), diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index f1f316ea..99375ec6 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -205,6 +205,30 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu } `, // #endregion valid; void return return value + // #region valid; void return variable + { + code: stripIndent` + // void return variable; explicitly allowed + import { Observable, of } from "rxjs"; + + let foo: () => void; + foo = (): Observable => of(42); + `, + options: [{ checksVoidReturn: false }], + }, + stripIndent` + // void return variable; not void + import { Observable, of } from "rxjs"; + + let foo: () => Observable; + foo = () => of(42); + `, + stripIndent` + // void return variable; unrelated + let foo: () => void; + foo = (): number => 42; + `, + // #endregion valid; void return variable // #region valid; spread { code: stripIndent` @@ -721,6 +745,40 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu `, ), // #endregion invalid; void return return value + // #region invalid; void return variable + fromFixture( + stripIndent` + // void return variable; reassign + import { of } from "rxjs"; + + let foo: () => void; + foo = () => of(42); + ~~~~~~~~~~~~ [forbiddenVoidReturnVariable] + `, + ), + fromFixture( + stripIndent` + // void return variable; nested + import { of } from "rxjs"; + + const foo: { + bar?: () => void; + } = {}; + foo.bar = () => of(42); + ~~~~~~~~~~~~ [forbiddenVoidReturnVariable] + `, + ), + fromFixture( + stripIndent` + // void return variable; Record + import { of } from "rxjs"; + + const foo: Record void> = {}; + foo.bar = () => of(42); + ~~~~~~~~~~~~ [forbiddenVoidReturnVariable] + `, + ), + // #endregion invalid; void return variable // #region invalid; spread fromFixture( stripIndent` From 8ffb56a569ecb95926a6858b4934620c54fa6e2b Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Mon, 2 Dec 2024 16:32:08 -0600 Subject: [PATCH 14/20] feat(no-misused-observables): variable declarations --- src/rules/no-misused-observables.ts | 30 +++++++++++++++++++++- tests/rules/no-misused-observables.test.ts | 12 +++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index c52b0769..c28e89e3 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -65,7 +65,7 @@ export const noMisusedObservablesRule = ruleCreator({ Property: checkProperty, ReturnStatement: checkReturnStatement, AssignmentExpression: checkAssignment, - // VariableDeclarator: checkVariableDeclarator, + VariableDeclarator: checkVariableDeclaration, }; const spreadChecks: eslint.RuleListener = { @@ -237,6 +237,34 @@ export const noMisusedObservablesRule = ruleCreator({ } } + function checkVariableDeclaration(node: es.VariableDeclarator): void { + const tsNode = esTreeNodeToTSNodeMap.get(node); + if ( + tsNode.initializer === undefined + || !node.init + || !node.id.typeAnnotation + ) { + return; + } + + // Optimization to avoid touching type info. + if (!isPossiblyFunctionType(node.id.typeAnnotation)) { + return; + } + + const varType = getTypeAtLocation(node.id); + if (!isVoidReturningFunctionType(varType)) { + return; + } + + if (couldReturnObservable(node.init)) { + context.report({ + messageId: 'forbiddenVoidReturnVariable', + node: node.init, + }); + } + } + return { ...(checksVoidReturn ? voidReturnChecks : {}), ...(checksSpreads ? spreadChecks : {}), diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index 99375ec6..49826dcd 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -213,6 +213,7 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu let foo: () => void; foo = (): Observable => of(42); + const bar: () => void = (): Observable => of(42); `, options: [{ checksVoidReturn: false }], }, @@ -756,6 +757,17 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu ~~~~~~~~~~~~ [forbiddenVoidReturnVariable] `, ), + fromFixture( + stripIndent` + // void return variable; const + import { of } from "rxjs"; + + const foo: () => void = () => of(42); + ~~~~~~~~~~~~ [forbiddenVoidReturnVariable] + const bar = () => of(42), baz: () => void = () => of(42); + ~~~~~~~~~~~~ [forbiddenVoidReturnVariable] + `, + ), fromFixture( stripIndent` // void return variable; nested From fa5e846b2b36ac3e326eced3de33550cd1434d6e Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Mon, 2 Dec 2024 17:08:50 -0600 Subject: [PATCH 15/20] feat(no-misused-observables): sub-options for void return --- docs/rules/no-misused-observables.md | 23 +++-- src/rules/no-misused-observables.ts | 105 ++++++++++++++++++--- tests/rules/no-misused-observables.test.ts | 12 +-- 3 files changed, 114 insertions(+), 26 deletions(-) diff --git a/docs/rules/no-misused-observables.md b/docs/rules/no-misused-observables.md index 9d204f18..82247a2b 100644 --- a/docs/rules/no-misused-observables.md +++ b/docs/rules/no-misused-observables.md @@ -6,11 +6,22 @@ ## Options - + -| Name | Description | Type | Default | -| :----------------- | :-------------------------------------------------------------------------- | :------ | :------ | -| `checksSpreads` | Disallow `...` spreading an Observable. | Boolean | `true` | -| `checksVoidReturn` | Disallow returning an Observable from a function typed as returning `void`. | Boolean | `true` | +| Name | Description | Type | Default | +| :----------------- | :--------------------------------------------------------------------------------------------------------------------------------------- | :------ | :------ | +| `checksSpreads` | Disallow `...` spreading an Observable. | Boolean | `true` | +| `checksVoidReturn` | Disallow returning an Observable from a function typed as returning `void`. | Object | `true` | - +### `checksVoidReturn` + +You can disable selective parts of the `checksVoidReturn` option. The following sub-options are supported: + +| Name | Description | Type | Default | +| :----------------- | :--------------------------------------------------------------------------------------------------------------------------------------- | :------ | :------ | +| `arguments` | Disallow passing an Observable-returning function as an argument where the parameter type expects a function that returns `void`. | Boolean | `true` | +| `attributes` | Disallow passing an Observable-returning function as a JSX attribute expected to be a function that returns `void`. | Boolean | `true` | +| `inheritedMethods` | Disallow providing an Observable-returning function where a function that returns `void` is expected by an extended or implemented type. | Boolean | `true` | +| `properties` | Disallow providing an Observable-returning function where a function that returns `void` is expected by a property. | Boolean | `true` | +| `returns` | Disallow returning an Observable-returning function where a function that returns `void` is expected. | Boolean | `true` | +| `variables` | Disallow assigning or declaring an Observable-returning function where a function that returns `void` is expected. | Boolean | `true` | diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index c28e89e3..87f9748d 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -15,8 +15,48 @@ import { ruleCreator } from '../utils'; // The implementation of this rule is similar to typescript-eslint's no-misused-promises. MIT License. // https://github.com/typescript-eslint/typescript-eslint/blob/fcd6cf063a774f73ea00af23705117a197f826d4/packages/eslint-plugin/src/rules/no-misused-promises.ts +// This is only exported for dts build to work. +export interface ChecksVoidReturnOptions { + arguments?: boolean; + attributes?: boolean; + inheritedMethods?: boolean; + properties?: boolean; + returns?: boolean; + variables?: boolean; +} + +function parseChecksVoidReturn( + checksVoidReturn: boolean | ChecksVoidReturnOptions, +): ChecksVoidReturnOptions | false { + switch (checksVoidReturn) { + case false: + return false; + + case true: + case undefined: + return { + arguments: true, + attributes: true, + inheritedMethods: true, + properties: true, + returns: true, + variables: true, + }; + + default: + return { + arguments: checksVoidReturn.arguments ?? true, + attributes: checksVoidReturn.attributes ?? true, + inheritedMethods: checksVoidReturn.inheritedMethods ?? true, + properties: checksVoidReturn.properties ?? true, + returns: checksVoidReturn.returns ?? true, + variables: checksVoidReturn.variables ?? true, + }; + } +} + const defaultOptions: readonly { - checksVoidReturn?: boolean; + checksVoidReturn?: boolean | ChecksVoidReturnOptions; checksSpreads?: boolean; }[] = []; @@ -39,7 +79,30 @@ export const noMisusedObservablesRule = ruleCreator({ schema: [ { properties: { - checksVoidReturn: { type: 'boolean', default: true, description: 'Disallow returning an Observable from a function typed as returning `void`.' }, + checksVoidReturn: { + default: true, + description: 'Disallow returning an Observable from a function typed as returning `void`.', + oneOf: [ + { + default: true, + type: 'boolean', + description: 'Disallow returning an Observable from all types of functions typed as returning `void`.', + }, + { + type: 'object', + additionalProperties: false, + description: 'Which forms of functions may have checking disabled.', + properties: { + arguments: { type: 'boolean', description: 'Disallow passing an Observable-returning function as an argument where the parameter type expects a function that returns `void`.' }, + attributes: { type: 'boolean', description: 'Disallow passing an Observable-returning function as a JSX attribute expected to be a function that returns `void`.' }, + inheritedMethods: { type: 'boolean', description: 'Disallow providing an Observable-returning function where a function that returns `void` is expected by an extended or implemented type.' }, + properties: { type: 'boolean', description: 'Disallow providing an Observable-returning function where a function that returns `void` is expected by a property.' }, + returns: { type: 'boolean', description: 'Disallow returning an Observable-returning function where a function that returns `void` is expected.' }, + variables: { type: 'boolean', description: 'Disallow assigning or declaring an Observable-returning function where a function that returns `void` is expected.' }, + }, + }, + ], + }, checksSpreads: { type: 'boolean', default: true, description: 'Disallow `...` spreading an Observable.' }, }, type: 'object', @@ -55,18 +118,32 @@ export const noMisusedObservablesRule = ruleCreator({ const [config = {}] = context.options; const { checksVoidReturn = true, checksSpreads = true } = config; - const voidReturnChecks: eslint.RuleListener = { - CallExpression: checkArguments, - NewExpression: checkArguments, - JSXAttribute: checkJSXAttribute, - ClassDeclaration: checkClassLikeOrInterfaceNode, - ClassExpression: checkClassLikeOrInterfaceNode, - TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, - Property: checkProperty, - ReturnStatement: checkReturnStatement, - AssignmentExpression: checkAssignment, - VariableDeclarator: checkVariableDeclaration, - }; + const parsedChecksVoidReturn = parseChecksVoidReturn(checksVoidReturn); + + const voidReturnChecks: eslint.RuleListener = parsedChecksVoidReturn ? { + ...(parsedChecksVoidReturn.arguments && { + CallExpression: checkArguments, + NewExpression: checkArguments, + }), + ...(parsedChecksVoidReturn.attributes && { + JSXAttribute: checkJSXAttribute, + }), + ...(parsedChecksVoidReturn.inheritedMethods && { + ClassDeclaration: checkClassLikeOrInterfaceNode, + ClassExpression: checkClassLikeOrInterfaceNode, + TSInterfaceDeclaration: checkClassLikeOrInterfaceNode, + }), + ...(parsedChecksVoidReturn.properties && { + Property: checkProperty, + }), + ...(parsedChecksVoidReturn.returns && { + ReturnStatement: checkReturnStatement, + }), + ...(parsedChecksVoidReturn.variables && { + AssignmentExpression: checkAssignment, + VariableDeclarator: checkVariableDeclaration, + }), + } : {}; const spreadChecks: eslint.RuleListener = { SpreadElement: (node) => { diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index 49826dcd..9e149dfd 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -19,7 +19,7 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu } new Foo(() => of(42)); `, - options: [{ checksVoidReturn: false }], + options: [{ checksVoidReturn: { arguments: false } }], }, stripIndent` // void return argument; unrelated @@ -53,7 +53,7 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu ); }; `, - options: [{ checksVoidReturn: false }], + options: [{ checksVoidReturn: { attributes: false } }], languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, }, { @@ -93,7 +93,7 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu foo(): Observable; } `, - options: [{ checksVoidReturn: false }], + options: [{ checksVoidReturn: { inheritedMethods: false } }], }, stripIndent` // void return inherited method; not void @@ -140,7 +140,7 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu c(): Observable { return of(42); }, }; `, - options: [{ checksVoidReturn: false }], + options: [{ checksVoidReturn: { properties: false } }], }, stripIndent` // void return property; not void @@ -185,7 +185,7 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu return (): Observable => of(42); } `, - options: [{ checksVoidReturn: false }], + options: [{ checksVoidReturn: { returns: false } }], }, stripIndent` // void return return value; not void @@ -215,7 +215,7 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu foo = (): Observable => of(42); const bar: () => void = (): Observable => of(42); `, - options: [{ checksVoidReturn: false }], + options: [{ checksVoidReturn: { variables: false } }], }, stripIndent` // void return variable; not void From bb8fcbd48f051ed7e76206901e141e737bb8e912 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Tue, 3 Dec 2024 08:19:13 -0600 Subject: [PATCH 16/20] feat(strict)!: add no-misused-observables to strict --- README.md | 2 +- docs/rules/no-misused-observables.md | 2 ++ src/configs/strict.ts | 1 + src/rules/no-misused-observables.ts | 1 + 4 files changed, 5 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index f4ccea79..71f80f61 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ The package includes the following rules. | [no-implicit-any-catch](docs/rules/no-implicit-any-catch.md) | Disallow implicit `any` error parameters in `catchError` operators. | ✅ 🔒 | 🔧 | 💡 | 💭 | | | [no-index](docs/rules/no-index.md) | Disallow importing index modules. | ✅ 🔒 | | | | | | [no-internal](docs/rules/no-internal.md) | Disallow importing internal modules. | ✅ 🔒 | 🔧 | 💡 | | | -| [no-misused-observables](docs/rules/no-misused-observables.md) | Disallow Observables in places not designed to handle them. | | | | 💭 | | +| [no-misused-observables](docs/rules/no-misused-observables.md) | Disallow Observables in places not designed to handle them. | 🔒 | | | 💭 | | | [no-nested-subscribe](docs/rules/no-nested-subscribe.md) | Disallow calling `subscribe` within a `subscribe` callback. | ✅ 🔒 | | | 💭 | | | [no-redundant-notify](docs/rules/no-redundant-notify.md) | Disallow sending redundant notifications from completed or errored observables. | ✅ 🔒 | | | 💭 | | | [no-sharereplay](docs/rules/no-sharereplay.md) | Disallow unsafe `shareReplay` usage. | ✅ 🔒 | | | | | diff --git a/docs/rules/no-misused-observables.md b/docs/rules/no-misused-observables.md index 82247a2b..4529a465 100644 --- a/docs/rules/no-misused-observables.md +++ b/docs/rules/no-misused-observables.md @@ -1,5 +1,7 @@ # Disallow Observables in places not designed to handle them (`rxjs-x/no-misused-observables`) +💼 This rule is enabled in the 🔒 `strict` config. + 💭 This rule requires [type information](https://typescript-eslint.io/linting/typed-linting). diff --git a/src/configs/strict.ts b/src/configs/strict.ts index b10efa99..9d685ade 100644 --- a/src/configs/strict.ts +++ b/src/configs/strict.ts @@ -22,6 +22,7 @@ export const createStrictConfig = ( }], 'rxjs-x/no-index': 'error', 'rxjs-x/no-internal': 'error', + 'rxjs-x/no-misused-observables': 'error', 'rxjs-x/no-nested-subscribe': 'error', 'rxjs-x/no-redundant-notify': 'error', 'rxjs-x/no-sharereplay': 'error', diff --git a/src/rules/no-misused-observables.ts b/src/rules/no-misused-observables.ts index 87f9748d..4754ab35 100644 --- a/src/rules/no-misused-observables.ts +++ b/src/rules/no-misused-observables.ts @@ -65,6 +65,7 @@ export const noMisusedObservablesRule = ruleCreator({ meta: { docs: { description: 'Disallow Observables in places not designed to handle them.', + recommended: 'strict', requiresTypeChecking: true, }, messages: { From 01a327a534a9984fd865ed0afebc3064b334d771 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Tue, 3 Dec 2024 08:34:15 -0600 Subject: [PATCH 17/20] docs(no-floating-observables): link to misused rule --- docs/rules/no-floating-observables.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/rules/no-floating-observables.md b/docs/rules/no-floating-observables.md index 078e2afe..3e829ed3 100644 --- a/docs/rules/no-floating-observables.md +++ b/docs/rules/no-floating-observables.md @@ -19,6 +19,7 @@ This rule will report observable-valued statements that are not treated in one o > [!TIP] > `no-floating-observables` only detects apparently unhandled observable _statements_. +> See [`no-misused-observables`](./no-misused-observables.md) for detecting code that provides observables to _logical_ locations ## Rule details From 329d45b6330042bc59a48f4c634a8537758b1d5f Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Tue, 3 Dec 2024 08:34:32 -0600 Subject: [PATCH 18/20] docs(no-misused-observables): examples & description --- docs/rules/no-misused-observables.md | 61 ++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/docs/rules/no-misused-observables.md b/docs/rules/no-misused-observables.md index 4529a465..056dd8b5 100644 --- a/docs/rules/no-misused-observables.md +++ b/docs/rules/no-misused-observables.md @@ -6,14 +6,63 @@ +This rule forbids providing observables to logical locations where the TypeScript compiler allows them but they are not handled properly. +These situations can often arise due to a misunderstanding of the way observables are handled. + +> [!TIP] +> `no-misused-observables` only detects code that provides observables to incorrect _logical_ locations. +> See [`no-floating-observables`](./no-floating-observables.md) for detecting unhandled observable _statements_. + +This rule is like [no-misused-promises](https://typescript-eslint.io/rules/no-misused-promises) but for Observables. + +> [!NOTE] +> Unlike `@typescript-eslint/no-misused-promises`, this rule does not check conditionals like `if` statements. +> Use `@typescript-eslint/no-unnecessary-condition` for linting those situations. + +## Rule details + +Examples of **incorrect** code for this rule: + +```ts +import { of } from "rxjs"; + +[1, 2, 3].forEach(i => of(i)); + +interface MySyncInterface { + foo(): void; +} +class MyRxClass implements MySyncInterface { + foo(): Observable { + return of(42); + } +} +``` + +Examples of **correct** code for this rule: + +```ts +import { of } from "rxjs"; + +[1, 2, 3].map(i => of(i)); + +interface MyRxInterface { + foo(): Observable; +} +class MyRxClass implements MyRxInterface { + foo(): Observable { + return of(42); + } +} +``` + ## Options -| Name | Description | Type | Default | -| :----------------- | :--------------------------------------------------------------------------------------------------------------------------------------- | :------ | :------ | -| `checksSpreads` | Disallow `...` spreading an Observable. | Boolean | `true` | -| `checksVoidReturn` | Disallow returning an Observable from a function typed as returning `void`. | Object | `true` | +| Name | Description | Type | Default | +| :----------------- | :-------------------------------------------------------------------------- | :------ | :------ | +| `checksSpreads` | Disallow `...` spreading an Observable. | Boolean | `true` | +| `checksVoidReturn` | Disallow returning an Observable from a function typed as returning `void`. | Object | `true` | ### `checksVoidReturn` @@ -27,3 +76,7 @@ You can disable selective parts of the `checksVoidReturn` option. The following | `properties` | Disallow providing an Observable-returning function where a function that returns `void` is expected by a property. | Boolean | `true` | | `returns` | Disallow returning an Observable-returning function where a function that returns `void` is expected. | Boolean | `true` | | `variables` | Disallow assigning or declaring an Observable-returning function where a function that returns `void` is expected. | Boolean | `true` | + +## Further reading + +- [TypeScript void function assignability](https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-functions-returning-non-void-assignable-to-function-returning-void) From 65ce7ab5c9eece04643f8ab7201956ef31fcc545 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Tue, 3 Dec 2024 09:33:27 -0600 Subject: [PATCH 19/20] docs(no-misused-observables): add spread example --- docs/rules/no-misused-observables.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/rules/no-misused-observables.md b/docs/rules/no-misused-observables.md index 056dd8b5..64d9ab44 100644 --- a/docs/rules/no-misused-observables.md +++ b/docs/rules/no-misused-observables.md @@ -36,6 +36,9 @@ class MyRxClass implements MySyncInterface { return of(42); } } + +const a = of(42); +const b = { ...b }; ``` Examples of **correct** code for this rule: From a8063fc501954b690a2c7be88dded86de3be79a7 Mon Sep 17 00:00:00 2001 From: Jason Weinzierl Date: Tue, 3 Dec 2024 09:48:32 -0600 Subject: [PATCH 20/20] test(no-misused-observables): test edge cases --- tests/rules/no-misused-observables.test.ts | 42 +++++++++++++++++++--- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/tests/rules/no-misused-observables.test.ts b/tests/rules/no-misused-observables.test.ts index 9e149dfd..32ac0221 100644 --- a/tests/rules/no-misused-observables.test.ts +++ b/tests/rules/no-misused-observables.test.ts @@ -5,6 +5,18 @@ import { ruleTester } from '../rule-tester'; ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRule, { valid: [ + { + code: stripIndent` + // all checks disabled + import { of } from "rxjs"; + + [1, 2, 3].forEach(i => of(i)); + + const source = of(42); + const foo = { ...source }; + `, + options: [{ checksVoidReturn: false, checksSpreads: false }], + }, // #region valid; void return argument { code: stripIndent` @@ -27,9 +39,9 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu [1, 2, 3].forEach(i => { return i; }); class Foo { - constructor(x: () => void) {} + constructor(x: () => void, y: number) {} } - new Foo(() => 42); + new Foo(() => 42, 0); new Foo; `, stripIndent` @@ -61,10 +73,10 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu // void return attribute; unrelated import React, { FC } from "react"; - const Component: FC<{ foo: () => void }> = () =>
; + const Component: FC<{ foo: () => void, bar: boolean }> = () =>
; const App = () => { return ( - 42} /> + 42} bar /> ); }; `, @@ -101,10 +113,12 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu class Foo { foo(): Observable { return of(42); } + s(): void {} } class Bar extends Foo { foo(): Observable { return of(43); } + static s(): Observable { return of(43); } } const Baz = class extends Foo { @@ -152,16 +166,31 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu a: () => of(42), b, c(): Observable { return of(42); }, + d(): Observable { return of(42); }, }; `, stripIndent` // void return property; unrelated + import { Observable, of } from "rxjs"; + type Foo = { a: () => void, b: () => void, c: () => void }; const b: () => number = () => 42; const foo: Foo = { a: () => 42, b, - c(): number { return 42; }, + ['c'](): number { return 42; }, + }; + const bar = { + a(): Observable { return of(42); }, + } + `, + stripIndent` + // computed property name for method declaration is not supported + import { Observable, of } from "rxjs"; + + type Foo = { a: () => void }; + const foo: Foo = { + ['a'](): Observable { return of(42); }, }; `, stripIndent` @@ -203,6 +232,9 @@ ruleTester({ types: true }).run('no-misused-observables', noMisusedObservablesRu function bar(): () => void { return (): number => 42; } + function baz(): () => void { + return (): void => { return; }; + } `, // #endregion valid; void return return value // #region valid; void return variable