diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts index 9b416ea4a57..222e5115d39 100644 --- a/packages/eslint-plugin/src/rules/unbound-method.ts +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -7,7 +7,8 @@ import { createRule, getModifiers, getParserServices, - isIdentifier, + isBuiltinSymbolLike, + isSymbolFromDefaultLibrary, } from '../util'; //------------------------------------------------------------------------------ @@ -83,12 +84,6 @@ const isNotImported = ( ); }; -const getNodeName = (node: TSESTree.Node): string | null => - node.type === AST_NODE_TYPES.Identifier ? node.name : null; - -const getMemberFullName = (node: TSESTree.MemberExpression): string => - `${getNodeName(node.object)}.${getNodeName(node.property)}`; - const BASE_MESSAGE = 'Avoid referencing unbound methods which may cause unintentional scoping of `this`.'; @@ -135,9 +130,9 @@ export default createRule({ function checkIfMethodAndReport( node: TSESTree.Node, symbol: ts.Symbol | undefined, - ): void { + ): boolean { if (!symbol) { - return; + return false; } const { dangerous, firstParamIsThis } = checkIfMethod( @@ -152,69 +147,145 @@ export default createRule({ : 'unbound', node, }); + return true; } + return false; } - return { - MemberExpression(node: TSESTree.MemberExpression): void { - if (isSafeUse(node)) { - return; - } - - const objectSymbol = services.getSymbolAtLocation(node.object); + function isNativelyBound( + object: TSESTree.Node, + property: TSESTree.Node, + ): boolean { + if ( + object.type === AST_NODE_TYPES.Identifier && + property.type === AST_NODE_TYPES.Identifier + ) { + const objectSymbol = services.getSymbolAtLocation(object); + const notImported = + objectSymbol != null && isNotImported(objectSymbol, currentSourceFile); if ( - objectSymbol && - nativelyBoundMembers.has(getMemberFullName(node)) && - isNotImported(objectSymbol, currentSourceFile) + notImported && + nativelyBoundMembers.has(`${object.name}.${property.name}`) ) { + return true; + } + } + + return ( + isBuiltinSymbolLike( + services.program, + services.getTypeAtLocation(object), + [ + 'NumberConstructor', + 'Number', + 'ObjectConstructor', + 'Object', + 'StringConstructor', + 'String', // eslint-disable-line @typescript-eslint/internal/prefer-ast-types-enum + 'SymbolConstructor', + 'ArrayConstructor', + 'Array', + 'ProxyConstructor', + 'DateConstructor', + 'Date', + 'Atomics', + 'Math', + 'JSON', + ], + ) && + isSymbolFromDefaultLibrary( + services.program, + services.getTypeAtLocation(property).getSymbol(), + ) + ); + } + + return { + MemberExpression(node: TSESTree.MemberExpression): void { + if (isSafeUse(node) || isNativelyBound(node.object, node.property)) { return; } checkIfMethodAndReport(node, services.getSymbolAtLocation(node)); }, - 'VariableDeclarator, AssignmentExpression'( - node: TSESTree.AssignmentExpression | TSESTree.VariableDeclarator, - ): void { - const [idNode, initNode] = - node.type === AST_NODE_TYPES.VariableDeclarator - ? [node.id, node.init] - : [node.left, node.right]; - - if (initNode && idNode.type === AST_NODE_TYPES.ObjectPattern) { - const rightSymbol = services.getSymbolAtLocation(initNode); - const initTypes = services.getTypeAtLocation(initNode); - - const notImported = - rightSymbol && isNotImported(rightSymbol, currentSourceFile); - - idNode.properties.forEach(property => { - if ( - property.type === AST_NODE_TYPES.Property && - property.key.type === AST_NODE_TYPES.Identifier - ) { - if ( - notImported && - isIdentifier(initNode) && - nativelyBoundMembers.has( - `${initNode.name}.${property.key.name}`, - ) - ) { - return; - } + ObjectPattern(node): void { + if (isNodeInsideTypeDeclaration(node)) { + return; + } + let initNode: TSESTree.Node | null = null; + if (node.parent.type === AST_NODE_TYPES.VariableDeclarator) { + initNode = node.parent.init; + } else if ( + node.parent.type === AST_NODE_TYPES.AssignmentPattern || + node.parent.type === AST_NODE_TYPES.AssignmentExpression + ) { + initNode = node.parent.right; + } - checkIfMethodAndReport( + for (const property of node.properties) { + if ( + property.type !== AST_NODE_TYPES.Property || + property.key.type !== AST_NODE_TYPES.Identifier + ) { + continue; + } + + if (initNode) { + if (!isNativelyBound(initNode, property.key)) { + const reported = checkIfMethodAndReport( property.key, - initTypes.getProperty(property.key.name), + services + .getTypeAtLocation(initNode) + .getProperty(property.key.name), ); + if (reported) { + continue; + } + // In assignment patterns, we should also check the type of + // Foo's nativelyBound method because initNode might be used as + // default value: + // function ({ nativelyBound }: Foo = NativeObject) {} + } else if (node.parent.type !== AST_NODE_TYPES.AssignmentPattern) { + continue; } - }); + } + + for (const intersectionPart of tsutils + .unionTypeParts(services.getTypeAtLocation(node)) + .flatMap(unionPart => tsutils.intersectionTypeParts(unionPart))) { + const reported = checkIfMethodAndReport( + property.key, + intersectionPart.getProperty(property.key.name), + ); + if (reported) { + break; + } + } } }, }; }, }); +function isNodeInsideTypeDeclaration(node: TSESTree.Node): boolean { + let parent: TSESTree.Node | undefined = node; + while ((parent = parent.parent)) { + if ( + (parent.type === AST_NODE_TYPES.ClassDeclaration && parent.declare) || + parent.type === AST_NODE_TYPES.TSAbstractMethodDefinition || + parent.type === AST_NODE_TYPES.TSDeclareFunction || + parent.type === AST_NODE_TYPES.TSFunctionType || + parent.type === AST_NODE_TYPES.TSInterfaceDeclaration || + parent.type === AST_NODE_TYPES.TSTypeAliasDeclaration || + (parent.type === AST_NODE_TYPES.VariableDeclaration && parent.declare) + ) { + return true; + } + } + return false; +} + interface CheckMethodResult { dangerous: boolean; firstParamIsThis?: boolean; diff --git a/packages/eslint-plugin/tests/rules/unbound-method.test.ts b/packages/eslint-plugin/tests/rules/unbound-method.test.ts index 81f4f126fd3..fc10aa2a58c 100644 --- a/packages/eslint-plugin/tests/rules/unbound-method.test.ts +++ b/packages/eslint-plugin/tests/rules/unbound-method.test.ts @@ -56,8 +56,78 @@ ruleTester.run('unbound-method', rule, { 'Promise.resolve().then(console.log);', "['1', '2', '3'].map(Number.parseInt);", '[5.2, 7.1, 3.6].map(Math.floor);', + ` + const foo = Number; + ['1', '2', '3'].map(foo.parseInt); + `, + ` + const foo = Math; + [5.2, 7.1, 3.6].map(foo.floor); + `, + "['1', '2', '3'].map(Number['floor']);", + ` + class Foo extends Number {} + const x = Foo.parseInt; + `, 'const x = console.log;', 'const x = Object.defineProperty;', + ` + const foo = Object; + const x = foo.defineProperty; + `, + ` + class Foo extends Object {} + const x = Foo.defineProperty; + `, + 'const x = String.fromCharCode;', + ` + const foo = String; + const x = foo.fromCharCode; + `, + ` + class Foo extends String {} + const x = Foo.fromCharCode; + `, + 'const x = RegExp.prototype;', + 'const x = Symbol.keyFor;', + ` + const foo = Symbol; + const x = foo.keyFor; + `, + 'const x = Array.isArray;', + ` + const foo = Array; + const x = foo.isArray; + `, + ` + class Foo extends Array {} + const x = Foo.isArray; + `, + 'const x = Proxy.revocable;', + ` + const foo = Proxy; + const x = foo.revocable; + `, + 'const x = Date.parse;', + ` + const foo = Date; + const x = foo.parse; + `, + ` + class Foo extends Date {} + const x = Foo.parse; + `, + 'const x = Atomics.load;', + ` + const foo = Atomics; + const x = foo.load; + `, + 'const x = Reflect.deleteProperty;', + 'const x = JSON.stringify;', + ` + const foo = JSON; + const x = foo.stringify; + `, ` const o = { f: function (this: void) {}, @@ -284,6 +354,130 @@ class Foo { } const { bound } = new Foo(); `, + ` +class Foo { + bound = () => 'foo'; +} +function foo({ bound } = new Foo()) {} + `, + ` +class Foo { + bound = () => 'foo'; +} +declare const bar: Foo; +function foo({ bound }: Foo) {} + `, + ` +class Foo { + bound = () => 'foo'; +} +class Bar { + bound = () => 'bar'; +} +function foo({ bound }: Foo | Bar) {} + `, + ` +class Foo { + bound = () => 'foo'; +} +type foo = ({ bound }: Foo) => void; + `, + ` +class Foo { + unbound = function () {}; +} +type foo = ({ unbound }: Foo) => void; + `, + ` +class Foo { + bound = () => 'foo'; +} +class Bar { + bound = () => 'bar'; +} +function foo({ bound }: Foo & Bar) {} + `, + ` +class Foo { + unbound = function () {}; +} +declare const { unbound }: Foo; + `, + "declare const { unbound } = '***';", + ` +class Foo { + unbound = function () {}; +} +type foo = (a: (b: (c: ({ unbound }: Foo) => void) => void) => void) => void; + `, + ` +class Foo { + unbound = function () {}; +} +class Bar { + property: ({ unbound }: Foo) => void; +} + `, + ` +class Foo { + unbound = function () {}; +} +function foo void>() {} + `, + ` +class Foo { + unbound = function () {}; +} +abstract class Bar { + abstract foo({ unbound }: Foo); +} + `, + ` +class Foo { + unbound = function () {}; +} +declare class Bar { + foo({ unbound }: Foo); +} + `, + ` +class Foo { + unbound = function () {}; +} +declare function foo({ unbound }: Foo); + `, + ` +class Foo { + unbound = function () {}; +} +interface Bar { + foo: ({ unbound }: Foo) => void; +} + `, + ` +class Foo { + unbound = function () {}; +} +interface Bar { + foo({ unbound }: Foo): void; +} + `, + ` +class Foo { + unbound = function () {}; +} +interface Bar { + new ({ unbound }: Foo): Foo; +} + `, + ` +class Foo { + unbound = function () {}; +} +type foo = new ({ unbound }: Foo) => void; + `, + 'const { unbound } = { unbound: () => {} };', + 'function foo({ unbound }: { unbound: () => void } = { unbound: () => {} }) {}', // https://github.com/typescript-eslint/typescript-eslint/issues/1866 ` class BaseClass { @@ -444,6 +638,21 @@ instance.unbound = x; // THIS SHOULD NOT }, { code: ` +class Foo extends Number { + static parseInt = function (string: string, radix?: number): number {}; +} +const foo = Foo; +['1', '2', '3'].map(foo.parseInt); + `, + errors: [ + { + line: 6, + messageId: 'unbound', + }, + ], + }, + { + code: ` class Foo { unbound = function () {}; } @@ -516,6 +725,291 @@ let unbound; }, { code: ` +class Foo { + unbound = function () {}; +} +function foo({ unbound }: Foo = new Foo()) {} + `, + errors: [ + { + line: 5, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +declare const bar: Foo; +function foo({ unbound }: Foo = bar) {} + `, + errors: [ + { + line: 6, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +declare const bar: Foo; +function foo({ unbound }: Foo = { unbound: () => {} }) {} + `, + errors: [ + { + line: 6, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +declare const bar: Foo; +function foo({ unbound }: Foo = { unbound: function () {} }) {} + `, + errors: [ + { + line: 6, + messageId: 'unboundWithoutThisAnnotation', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +function foo({ unbound }: Foo) {} + `, + errors: [ + { + line: 5, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +function bar(cb: (arg: Foo) => void) {} +bar(({ unbound }) => {}); + `, + errors: [ + { + line: 6, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +function bar(cb: (arg: { unbound: () => void }) => void) {} +bar(({ unbound } = new Foo()) => {}); + `, + errors: [ + { + line: 6, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +for (const { unbound } of [new Foo(), new Foo()]) { +} + `, + errors: [ + { + line: 5, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; + + foo({ unbound }: Foo) {} +} + `, + errors: [ + { + line: 5, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +class Bar { + unbound = function () {}; +} +function foo({ unbound }: Foo | Bar) {} + `, + errors: [ + { + line: 8, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +function foo({ unbound }: { unbound: () => string } | Foo) {} + `, + errors: [ + { + line: 5, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +class Bar { + unbound = () => {}; +} +function foo({ unbound }: Foo | Bar) {} + `, + errors: [ + { + line: 8, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +const foo = ({ unbound }: Foo & { foo: () => 'bar' }) => {}; + `, + errors: [ + { + line: 5, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +class Bar { + unbound = () => {}; +} +const foo = ({ unbound }: (Foo & { foo: () => 'bar' }) | Bar) => {}; + `, + errors: [ + { + line: 8, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; +} +class Bar { + unbound = () => {}; +} +const foo = ({ unbound }: Foo & Bar) => {}; + `, + errors: [ + { + line: 8, + messageId: 'unbound', + }, + ], + }, + { + code: ` +class Foo { + unbound = function () {}; + + other = function () {}; +} +class Bar { + unbound = () => {}; +} +const foo = ({ unbound, ...rest }: Foo & Bar) => {}; + `, + errors: [ + { + line: 10, + messageId: 'unbound', + }, + ], + }, + { + code: 'const { unbound } = { unbound: function () {} };', + errors: [ + { + line: 1, + messageId: 'unboundWithoutThisAnnotation', + }, + ], + }, + { + code: ` +function foo( + { unbound }: { unbound: () => void } = { unbound: function () {} }, +) {} + `, + errors: [ + { + line: 3, + messageId: 'unboundWithoutThisAnnotation', + }, + ], + }, + { + code: ` +class Foo { + floor = function () {}; +} + +const { floor } = Math.random() > 0.5 ? new Foo() : Math; + `, + errors: [ + { + line: 6, + messageId: 'unboundWithoutThisAnnotation', + }, + ], + }, + { + code: ` class CommunicationError { foo() {} } diff --git a/packages/type-utils/src/builtinSymbolLikes.ts b/packages/type-utils/src/builtinSymbolLikes.ts index 3443a0d0382..307d02e3494 100644 --- a/packages/type-utils/src/builtinSymbolLikes.ts +++ b/packages/type-utils/src/builtinSymbolLikes.ts @@ -105,7 +105,7 @@ export function isBuiltinTypeAliasLike( export function isBuiltinSymbolLike( program: ts.Program, type: ts.Type, - symbolName: string, + symbolName: string | string[], ): boolean { return isBuiltinSymbolLikeRecurser(program, type, subType => { const symbol = subType.getSymbol(); @@ -113,8 +113,12 @@ export function isBuiltinSymbolLike( return false; } + const actualSymbolName = symbol.getName(); + if ( - symbol.getName() === symbolName && + (Array.isArray(symbolName) + ? symbolName.some(name => actualSymbolName === name) + : actualSymbolName === symbolName) && isSymbolFromDefaultLibrary(program, symbol) ) { return true;