From 6718906fa83fbceb947b0ea0b7f23428b993b1d3 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 3 Apr 2019 12:48:27 -0400 Subject: [PATCH] feat(eslint-plugin): added new rule unbound-method (#204) --- packages/eslint-plugin/README.md | 1 + packages/eslint-plugin/ROADMAP.md | 3 +- .../docs/rules/unbound-method.md | 92 ++++++ .../eslint-plugin/src/rules/unbound-method.ts | 123 ++++++++ .../tests/rules/unbound-method.test.ts | 273 ++++++++++++++++++ 5 files changed, 491 insertions(+), 1 deletion(-) create mode 100644 packages/eslint-plugin/docs/rules/unbound-method.md create mode 100644 packages/eslint-plugin/src/rules/unbound-method.ts create mode 100644 packages/eslint-plugin/tests/rules/unbound-method.test.ts diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 6d433a5eef5..8057f15843d 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -152,6 +152,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/promise-function-async`](./docs/rules/promise-function-async.md) | Requires any function or method that returns a Promise to be marked async. (`promise-function-async` from TSLint) | | | :thought_balloon: | | [`@typescript-eslint/restrict-plus-operands`](./docs/rules/restrict-plus-operands.md) | When adding two variables, operands must both be of type number or of type string. (`restrict-plus-operands` from TSLint) | | | :thought_balloon: | | [`@typescript-eslint/type-annotation-spacing`](./docs/rules/type-annotation-spacing.md) | Require consistent spacing around type annotations (`typedef-whitespace` from TSLint) | :heavy_check_mark: | :wrench: | | +| [`@typescript-eslint/unbound-method`](./docs/rules/unbound-method.md) | Enforces unbound methods are called with their expected scope. (`no-unbound-method` from TSLint) | :heavy_check_mark: | | :thought_balloon: | | [`@typescript-eslint/unified-signatures`](./docs/rules/unified-signatures.md) | Warns for any two overloads that could be unified into one. (`unified-signatures` from TSLint) | | | | diff --git a/packages/eslint-plugin/ROADMAP.md b/packages/eslint-plugin/ROADMAP.md index 11ccb8ac22b..c0cdcbf0b3e 100644 --- a/packages/eslint-plugin/ROADMAP.md +++ b/packages/eslint-plugin/ROADMAP.md @@ -77,7 +77,7 @@ | [`no-submodule-imports`] | 🌓 | [`import/no-internal-modules`] (slightly different) | | [`no-switch-case-fall-through`] | 🌟 | [`no-fallthrough`][no-fallthrough] | | [`no-this-assignment`] | ✅ | [`@typescript-eslint/no-this-alias`] | -| [`no-unbound-method`] | 🛑 | N/A | +| [`no-unbound-method`] | ✅ | [`@typescript-eslint/unbound-method`] | | [`no-unnecessary-class`] | ✅ | [`@typescript-eslint/no-extraneous-class`] | | [`no-unsafe-any`] | 🛑 | N/A | | [`no-unsafe-finally`] | 🌟 | [`no-unsafe-finally`][no-unsafe-finally] | @@ -586,6 +586,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint- [`@typescript-eslint/no-namespace`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-namespace.md [`@typescript-eslint/no-non-null-assertion`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-non-null-assertion.md [`@typescript-eslint/no-triple-slash-reference`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-triple-slash-reference.md +[`@typescript-eslint/unbound-method`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/unbound-method.md [`@typescript-eslint/no-unnecessary-type-assertion`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unnecessary-type-assertion.md [`@typescript-eslint/no-var-requires`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-var-requires.md [`@typescript-eslint/type-annotation-spacing`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/type-annotation-spacing.md diff --git a/packages/eslint-plugin/docs/rules/unbound-method.md b/packages/eslint-plugin/docs/rules/unbound-method.md new file mode 100644 index 00000000000..d0268dadeec --- /dev/null +++ b/packages/eslint-plugin/docs/rules/unbound-method.md @@ -0,0 +1,92 @@ +# Enforces unbound methods are called with their expected scope (unbound-method) + +Warns when a method is used outside of a method call. + +Class functions don't preserve the class scope when passed as standalone variables. + +## Rule Details + +Examples of **incorrect** code for this rule + +```ts +class MyClass { + public log(): void { + console.log(this); + } +} + +const instance = new MyClass(); + +// This logs the global scope (`window`/`global`), not the class instance +const myLog = instance.log; +myLog(); + +// This log might later be called with an incorrect scope +const { log } = instance; +``` + +Examples of **correct** code for this rule + +```ts +class MyClass { + public logUnbound(): void { + console.log(this); + } + + public logBound = () => console.log(this); +} + +const instance = new MyClass(); + +// logBound will always be bound with the correct scope +const { logBound } = instance; +logBound(); + +// .bind and lambdas will also add a correct scope +const dotBindLog = instance.log.bind(instance); +const innerLog = () => instance.log(); +``` + +## Options + +The rule accepts an options object with the following property: + +- `ignoreStatic` to not check whether `static` methods are correctly bound + +### `ignoreStatic` + +Examples of **correct** code for this rule with `{ ignoreStatic: true }`: + +```ts +class OtherClass { + static log() { + console.log(OtherClass); + } +} + +// With `ignoreStatic`, statics are assumed to not rely on a particular scope +const { log } = OtherClass; + +log(); +``` + +### Example + +```json +{ + "@typescript-eslint/unbound-method": [ + "error", + { + "ignoreStatic": true + } + ] +} +``` + +## When Not To Use It + +If your code intentionally waits to bind methods after use, such as by passing a `scope: this` along with the method, you can disable this rule. + +## Related To + +- TSLint: [no-unbound-method](https://palantir.github.io/tslint/rules/no-unbound-method/) diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts new file mode 100644 index 00000000000..621e95ec7c5 --- /dev/null +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -0,0 +1,123 @@ +import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/typescript-estree'; +import * as tsutils from 'tsutils'; +import * as ts from 'typescript'; + +import * as util from '../util'; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +interface Config { + ignoreStatic: boolean; +} + +type Options = [Config]; + +type MessageIds = 'unbound'; + +export default util.createRule({ + name: 'unbound-method', + meta: { + docs: { + category: 'Best Practices', + description: + 'Enforces unbound methods are called with their expected scope.', + tslintName: 'no-unbound-method', + recommended: 'error', + }, + messages: { + unbound: + 'Avoid referencing unbound methods which may cause unintentional scoping of `this`.', + }, + schema: [ + { + type: 'object', + properties: { + ignoreStatic: { + type: 'boolean', + }, + }, + additionalProperties: false, + }, + ], + type: 'problem', + }, + defaultOptions: [ + { + ignoreStatic: false, + }, + ], + create(context, [{ ignoreStatic }]) { + const parserServices = util.getParserServices(context); + const checker = parserServices.program.getTypeChecker(); + + return { + [AST_NODE_TYPES.MemberExpression](node: TSESTree.MemberExpression) { + if (isSafeUse(node)) { + return; + } + + const originalNode = parserServices.esTreeNodeToTSNodeMap.get(node); + const symbol = checker.getSymbolAtLocation(originalNode); + + if (symbol && isDangerousMethod(symbol, ignoreStatic)) { + context.report({ + messageId: 'unbound', + node, + }); + } + }, + }; + }, +}); + +function isDangerousMethod(symbol: ts.Symbol, ignoreStatic: boolean) { + const { valueDeclaration } = symbol; + + switch (valueDeclaration.kind) { + case ts.SyntaxKind.MethodDeclaration: + case ts.SyntaxKind.MethodSignature: + return !( + ignoreStatic && + tsutils.hasModifier( + valueDeclaration.modifiers, + ts.SyntaxKind.StaticKeyword, + ) + ); + } + + return false; +} + +function isSafeUse(node: TSESTree.Node): boolean { + const parent = node.parent!; + + switch (parent.type) { + case AST_NODE_TYPES.IfStatement: + case AST_NODE_TYPES.ForStatement: + case AST_NODE_TYPES.MemberExpression: + case AST_NODE_TYPES.UpdateExpression: + case AST_NODE_TYPES.WhileStatement: + return true; + + case AST_NODE_TYPES.CallExpression: + return parent.callee === node; + + case AST_NODE_TYPES.ConditionalExpression: + return parent.test === node; + + case AST_NODE_TYPES.LogicalExpression: + return parent.operator !== '||'; + + case AST_NODE_TYPES.TaggedTemplateExpression: + return parent.tag === node; + + case AST_NODE_TYPES.TSNonNullExpression: + case AST_NODE_TYPES.TSAsExpression: + case AST_NODE_TYPES.TSTypeAssertion: + return isSafeUse(parent); + } + + return false; +} diff --git a/packages/eslint-plugin/tests/rules/unbound-method.test.ts b/packages/eslint-plugin/tests/rules/unbound-method.test.ts new file mode 100644 index 00000000000..8b4ed003fd4 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/unbound-method.test.ts @@ -0,0 +1,273 @@ +import path from 'path'; +import rule from '../../src/rules/unbound-method'; +import { RuleTester } from '../RuleTester'; + +const rootPath = path.join(process.cwd(), 'tests/fixtures/'); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootPath, + project: './tsconfig.json', + }, +}); + +ruleTester.run('unbound-method', rule, { + valid: [ + ` +class ContainsMethods { + bound?: () => void; + unbound?(): void; + + static boundStatic?: () => void; + static unboundStatic?(): void; +} + +let instance = new ContainsMethods(); + +instance.bound(); +instance.unbound(); + +ContainsMethods.boundStatic(); +ContainsMethods.unboundStatic(); + +{ + const bound = instance.bound; + const boundStatic = ContainsMethods; +} +{ + const { bound } = instance; + const { boundStatic } = ContainsMethods; +} + +(instance.bound)(); +(instance.unbound)(); + +(ContainsMethods.boundStatic)(); +(ContainsMethods.unboundStatic)(); + +instance.bound\`\`; +instance.unbound\`\`; + +if (instance.bound) { } +if (instance.unbound) { } + +if (ContainsMethods.boundStatic) { } +if (ContainsMethods.unboundStatic) { } + +while (instance.bound) { } +while (instance.unbound) { } + +while (ContainsMethods.boundStatic) { } +while (ContainsMethods.unboundStatic) { } + +instance.bound as any; +ContainsMethods.boundStatic as any; + +instance.bound++; ++instance.bound; +++instance.bound; +instance.bound--; +-instance.bound; +--instance.bound; +instance.bound += 1; +instance.bound -= 1; +instance.bound *= 1; +instance.bound /= 1; + +instance.bound || 0; +instane.bound && 0; + +instance.bound ? 1 : 0; +instance.unbound ? 1 : 0; + +ContainsMethods.boundStatic++; ++ContainsMethods.boundStatic; +++ContainsMethods.boundStatic; +ContainsMethods.boundStatic--; +-ContainsMethods.boundStatic; +--ContainsMethods.boundStatic; +ContainsMethods.boundStatic += 1; +ContainsMethods.boundStatic -= 1; +ContainsMethods.boundStatic *= 1; +ContainsMethods.boundStatic /= 1; + +ContainsMethods.boundStatic || 0; +instane.boundStatic && 0; + +ContainsMethods.boundStatic ? 1 : 0; +ContainsMethods.unboundStatic ? 1 : 0; +`, + ], + invalid: [ + { + code: ` +class ContainsMethods { + bound?: () => void; + unbound?(): void; + static boundStatic?: () => void; + static unboundStatic?(): void; +} + +const instance = new ContainsMethods(); + +{ + const unbound = instance.unbound; + const unboundStatic = ContainsMethods.unboundStatic; +} +{ + const { unbound } = instance.unbound; + const { unboundStatic } = ContainsMethods.unboundStatic; +} + +instance.unbound; +instance.unbound as any; + +ContainsMethods.unboundStatic; +ContainsMethods.unboundStatic as any; + +instance.unbound++; ++instance.unbound; +++instance.unbound; +instance.unbound--; +-instance.unbound; +--instance.unbound; +instance.unbound += 1; +instance.unbound -= 1; +instance.unbound *= 1; +instance.unbound /= 1; + +instance.unbound || 0; +instance.unbound && 0; + +ContainsMethods.unboundStatic++; ++ContainsMethods.unboundStatic; +++ContainsMethods.unboundStatic; +ContainsMethods.unboundStatic--; +-ContainsMethods.unboundStatic; +--ContainsMethods.unboundStatic; +ContainsMethods.unboundStatic += 1; +ContainsMethods.unboundStatic -= 1; +ContainsMethods.unboundStatic *= 1; +ContainsMethods.unboundStatic /= 1; + +ContainsMethods.unboundStatic || 0; +ContainsMethods.unboundStatic && 0; +`, + errors: [ + { + line: 12, + messageId: 'unbound', + }, + { + line: 13, + messageId: 'unbound', + }, + { + line: 16, + messageId: 'unbound', + }, + { + line: 17, + messageId: 'unbound', + }, + { + line: 20, + messageId: 'unbound', + }, + { + line: 21, + messageId: 'unbound', + }, + { + line: 23, + messageId: 'unbound', + }, + { + line: 24, + messageId: 'unbound', + }, + { + line: 27, + messageId: 'unbound', + }, + { + line: 30, + messageId: 'unbound', + }, + { + line: 32, + messageId: 'unbound', + }, + { + line: 33, + messageId: 'unbound', + }, + { + line: 34, + messageId: 'unbound', + }, + { + line: 35, + messageId: 'unbound', + }, + { + line: 37, + messageId: 'unbound', + }, + { + line: 41, + messageId: 'unbound', + }, + { + line: 44, + messageId: 'unbound', + }, + { + line: 46, + messageId: 'unbound', + }, + { + line: 47, + messageId: 'unbound', + }, + { + line: 48, + messageId: 'unbound', + }, + { + line: 49, + messageId: 'unbound', + }, + { + line: 51, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class ContainsMethods { + unbound?(): void; + + static unboundStatic?(): void; +} + +new ContainsMethods().unbound; + +ContainsMethods.unboundStatic; +`, + options: [ + { + ignoreStatic: true, + }, + ], + errors: [ + { + line: 8, + messageId: 'unbound', + }, + ], + }, + ], +});