diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts index ac4dd6b28de..32df694637e 100644 --- a/packages/eslint-plugin/src/rules/unbound-method.ts +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -14,9 +14,9 @@ interface Config { ignoreStatic: boolean; } -type Options = [Config]; +export type Options = [Config]; -type MessageIds = 'unbound'; +export type MessageIds = 'unbound'; export default util.createRule({ name: 'unbound-method', @@ -99,9 +99,9 @@ function isDangerousMethod(symbol: ts.Symbol, ignoreStatic: boolean): boolean { } function isSafeUse(node: TSESTree.Node): boolean { - const parent = node.parent!; + const parent = node.parent; - switch (parent.type) { + switch (parent?.type) { case AST_NODE_TYPES.IfStatement: case AST_NODE_TYPES.ForStatement: case AST_NODE_TYPES.MemberExpression: @@ -118,9 +118,6 @@ function isSafeUse(node: TSESTree.Node): boolean { 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; @@ -134,6 +131,16 @@ function isSafeUse(node: TSESTree.Node): boolean { case AST_NODE_TYPES.TSAsExpression: case AST_NODE_TYPES.TSTypeAssertion: return isSafeUse(parent); + + case AST_NODE_TYPES.LogicalExpression: + if (parent.operator === '&&' && parent.left === node) { + // this is safe, as && will return the left if and only if it's falsy + return true; + } + + // in all other cases, it's likely the logical expression will return the method ref + // so make sure the parent is a safe usage + 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 index 634977b477f..6b5fd5500d4 100644 --- a/packages/eslint-plugin/tests/rules/unbound-method.test.ts +++ b/packages/eslint-plugin/tests/rules/unbound-method.test.ts @@ -1,5 +1,6 @@ +import { TSESLint } from '@typescript-eslint/experimental-utils'; import path from 'path'; -import rule from '../../src/rules/unbound-method'; +import rule, { MessageIds, Options } from '../../src/rules/unbound-method'; import { RuleTester } from '../RuleTester'; const rootPath = path.join(process.cwd(), 'tests/fixtures/'); @@ -12,9 +13,8 @@ const ruleTester = new RuleTester({ }, }); -ruleTester.run('unbound-method', rule, { - valid: [ - ` +function addContainsMethodsClass(code: string): string { + return ` class ContainsMethods { bound?: () => void; unbound?(): void; @@ -25,97 +25,125 @@ class ContainsMethods { let instance = new ContainsMethods(); -instance.bound(); -instance.unbound(); - -ContainsMethods.boundStatic(); -ContainsMethods.unboundStatic(); - -{ - const bound = instance.bound; - const boundStatic = ContainsMethods; +${code} + `; } -{ - const { bound } = instance; - const { boundStatic } = ContainsMethods; +function addContainsMethodsClassInvalid( + code: string[], +): TSESLint.InvalidTestCase[] { + return code.map(c => ({ + code: addContainsMethodsClass(c), + errors: [ + { + line: 12, + messageId: 'unbound', + }, + ], + })); } -(instance.bound)(); -(instance.unbound)(); +ruleTester.run('unbound-method', rule, { + valid: [ + ...[ + 'instance.bound();', + 'instance.unbound();', -(ContainsMethods.boundStatic)(); -(ContainsMethods.unboundStatic)(); + 'ContainsMethods.boundStatic();', + 'ContainsMethods.unboundStatic();', -instance.bound\`\`; -instance.unbound\`\`; + 'const bound = instance.bound;', + 'const boundStatic = ContainsMethods;', -if (instance.bound) { } -if (instance.unbound) { } + 'const { bound } = instance;', + 'const { boundStatic } = ContainsMethods;', -if (instance.bound !== undefined) { } -if (instance.unbound !== undefined) { } + '(instance.bound)();', + '(instance.unbound)();', -if (ContainsMethods.boundStatic) { } -if (ContainsMethods.unboundStatic) { } + '(ContainsMethods.boundStatic)();', + '(ContainsMethods.unboundStatic)();', -if (ContainsMethods.boundStatic !== undefined) { } -if (ContainsMethods.unboundStatic !== undefined) { } + 'instance.bound``;', + 'instance.unbound``;', -while (instance.bound) { } -while (instance.unbound) { } + 'if (instance.bound) { }', + 'if (instance.unbound) { }', -while (instance.bound !== undefined) { } -while (instance.unbound !== undefined) { } + 'if (instance.bound !== undefined) { }', + 'if (instance.unbound !== undefined) { }', -while (ContainsMethods.boundStatic) { } -while (ContainsMethods.unboundStatic) { } + 'if (ContainsMethods.boundStatic) { }', + 'if (ContainsMethods.unboundStatic) { }', -while (ContainsMethods.boundStatic !== undefined) { } -while (ContainsMethods.unboundStatic !== undefined) { } + 'if (ContainsMethods.boundStatic !== undefined) { }', + 'if (ContainsMethods.unboundStatic !== undefined) { }', -instance.bound as any; -ContainsMethods.boundStatic as any; + 'if (ContainsMethods.boundStatic && instance) { }', + 'if (ContainsMethods.unboundStatic && instance) { }', -instance.bound++; -+instance.bound; -++instance.bound; -instance.bound--; --instance.bound; ---instance.bound; -instance.bound += 1; -instance.bound -= 1; -instance.bound *= 1; -instance.bound /= 1; + 'if (instance.bound || instance) { }', + 'if (instance.unbound || instance) { }', -instance.bound || 0; -instane.bound && 0; + 'ContainsMethods.unboundStatic && 0 || ContainsMethods;', -instance.bound ? 1 : 0; -instance.unbound ? 1 : 0; + '(instance.bound || instance) ? 1 : 0', + '(instance.unbound || instance) ? 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; + 'while (instance.bound) { }', + 'while (instance.unbound) { }', -ContainsMethods.boundStatic || 0; -instane.boundStatic && 0; + 'while (instance.bound !== undefined) { }', + 'while (instance.unbound !== undefined) { }', -ContainsMethods.boundStatic ? 1 : 0; -ContainsMethods.unboundStatic ? 1 : 0; + 'while (ContainsMethods.boundStatic) { }', + 'while (ContainsMethods.unboundStatic) { }', -typeof instance.bound === 'function'; -typeof instance.unbound === 'function'; + 'while (ContainsMethods.boundStatic !== undefined) { }', + 'while (ContainsMethods.unboundStatic !== undefined) { }', -typeof ContainsMethods.boundStatic === 'function'; -typeof ContainsMethods.unboundStatic === 'function'; - `, + '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;', + 'instance.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;', + + "typeof instance.bound === 'function';", + "typeof instance.unbound === 'function';", + + "typeof ContainsMethods.boundStatic === 'function';", + "typeof ContainsMethods.unboundStatic === 'function';", + ].map(addContainsMethodsClass), ` interface RecordA { readonly type: "A" @@ -165,185 +193,63 @@ function foo(instance: ContainsMethods | null) { typeof instance?.bound === 'function'; typeof instance?.unbound === 'function'; +} + `, + // https://github.com/typescript-eslint/typescript-eslint/issues/1425 + ` +interface OptionalMethod { + mightBeDefined?(): void +} + +const x: OptionalMethod = {}; +declare const myCondition: boolean; +if(myCondition || x.mightBeDefined) { + console.log('hello world') } `, ], invalid: [ { - code: ` -class ContainsMethods { - bound?: () => void; - unbound?(): void; - static boundStatic?: () => void; - static unboundStatic?(): void; -} - -function foo(instance: ContainsMethods | null) { - const unbound = instance?.unbound; - instance.unbound += 1; - instance?.unbound as any; + code: addContainsMethodsClass(` +function foo(arg: ContainsMethods | null) { + const unbound = arg?.unbound; + arg.unbound += 1; + arg?.unbound as any; } - `, + `), errors: [ { - line: 10, + line: 14, messageId: 'unbound', }, { - line: 11, + line: 15, messageId: 'unbound', }, { - line: 12, + line: 16, messageId: 'unbound', }, ], }, - { - code: ` -class ContainsMethods { - bound?: () => void; - unbound?(): void; - static boundStatic?: () => void; - static unboundStatic?(): void; -} + ...addContainsMethodsClassInvalid([ + 'const unbound = instance.unbound;', + 'const unboundStatic = ContainsMethods.unboundStatic;', -const instance = new ContainsMethods(); + 'const { unbound } = instance.unbound;', + 'const { unboundStatic } = ContainsMethods.unboundStatic;', -{ - const unbound = instance.unbound; - const unboundStatic = ContainsMethods.unboundStatic; -} -{ - const { unbound } = instance.unbound; - const { unboundStatic } = ContainsMethods.unboundStatic; -} + 'instance.unbound;', + 'instance.unbound as any;', -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', - }, - ], - }, + 'ContainsMethods.unboundStatic;', + 'ContainsMethods.unboundStatic as any;', + + 'instance.unbound || 0;', + 'ContainsMethods.unboundStatic || 0;', + + 'instance.unbound ? instance.unbound : null', + ]), { code: ` class ContainsMethods {