From d156ba73d8dd4e958bd6015a064ad754c5ce69fe Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 26 Jan 2020 11:17:15 +1300 Subject: [PATCH 1/6] fix(eslint-plugin): compare source file to declaration for unbound check --- .../eslint-plugin/src/rules/unbound-method.ts | 21 +++++++++++++-- .../tests/rules/unbound-method.test.ts | 26 +++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts index 32df694637e1..e8058e00677d 100644 --- a/packages/eslint-plugin/src/rules/unbound-method.ts +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -53,6 +53,9 @@ export default util.createRule({ create(context, [{ ignoreStatic }]) { const parserServices = util.getParserServices(context); const checker = parserServices.program.getTypeChecker(); + const currentSourceFile = parserServices.program.getSourceFile( + context.getFilename(), + ); return { 'MemberExpression, OptionalMemberExpression'( @@ -65,7 +68,10 @@ export default util.createRule({ const originalNode = parserServices.esTreeNodeToTSNodeMap.get(node); const symbol = checker.getSymbolAtLocation(originalNode); - if (symbol && isDangerousMethod(symbol, ignoreStatic)) { + if ( + symbol && + isDangerousMethod(symbol, ignoreStatic, currentSourceFile) + ) { context.report({ messageId: 'unbound', node, @@ -76,13 +82,24 @@ export default util.createRule({ }, }); -function isDangerousMethod(symbol: ts.Symbol, ignoreStatic: boolean): boolean { +function isDangerousMethod( + symbol: ts.Symbol, + ignoreStatic: boolean, + currentSourceFile?: ts.SourceFile, +): boolean { const { valueDeclaration } = symbol; if (!valueDeclaration) { // working around https://github.com/microsoft/TypeScript/issues/31294 return false; } + if ( + currentSourceFile && + currentSourceFile !== valueDeclaration.getSourceFile() + ) { + return false; + } + switch (valueDeclaration.kind) { case ts.SyntaxKind.MethodDeclaration: case ts.SyntaxKind.MethodSignature: diff --git a/packages/eslint-plugin/tests/rules/unbound-method.test.ts b/packages/eslint-plugin/tests/rules/unbound-method.test.ts index dcf58b6447e7..0e8e30965fee 100644 --- a/packages/eslint-plugin/tests/rules/unbound-method.test.ts +++ b/packages/eslint-plugin/tests/rules/unbound-method.test.ts @@ -7,6 +7,7 @@ const rootPath = getFixturesRootDir(); const ruleTester = new RuleTester({ parser: '@typescript-eslint/parser', parserOptions: { + sourceType: 'module', tsconfigRootDir: rootPath, project: './tsconfig.json', }, @@ -43,6 +44,12 @@ function addContainsMethodsClassInvalid( ruleTester.run('unbound-method', rule, { valid: [ + 'Promise.resolve().then(console.log)', + '["1", "2", "3"].map(Number.parseInt)', + ` +import console from './consoleshim'; +Promise.resolve().then(console.log); + `, ...[ 'instance.bound();', 'instance.unbound();', @@ -208,6 +215,25 @@ if(myCondition || x.mightBeDefined) { `, ], invalid: [ + { + code: ` +class Console { + log(str) { + process.stdout.write(str); + } +} + +const console = new Console(); + +Promise.resolve().then(console.log); + `, + errors: [ + { + line: 10, + messageId: 'unbound', + }, + ], + }, { code: addContainsMethodsClass(` function foo(arg: ContainsMethods | null) { From 8828c63867f4f3366b4918fe6065f172c4ab90ed Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Fri, 31 Jan 2020 17:07:09 +1300 Subject: [PATCH 2/6] fix(eslint-plugin): use whitelist for `unbound-method` rule --- .../eslint-plugin/src/rules/unbound-method.ts | 85 +++++++++++++++---- .../eslint-plugin/tests/fixtures/class.ts | 3 + .../tests/rules/unbound-method.test.ts | 14 +++ 3 files changed, 86 insertions(+), 16 deletions(-) diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts index e8058e00677d..3275d75bcb87 100644 --- a/packages/eslint-plugin/src/rules/unbound-method.ts +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -18,6 +18,61 @@ export type Options = [Config]; export type MessageIds = 'unbound'; +const nativelyBoundMembers = [ + ...[ + Promise, + // BigInt, // todo: enable when we drop node@8 + Number, + Object, + String, + RegExp, + Symbol, + Array, + Proxy, + Date, + ].map(x => [x.name, x]), + ['Infinity', Infinity], + ['Atomics', Atomics], + ['Reflect', Reflect], + ['console', console], + ['Math', Math], + ['JSON', JSON], + ['Intl', Intl], +] + .map(([namespace, object]) => + Object.getOwnPropertyNames(object) + .filter( + name => + !name.startsWith('_') && + typeof (object as Record)[name] === 'function', + ) + .map(name => `${namespace}.${name}`), + ) + .reduce((arr, names) => arr.concat(names), []); + +const isMemberImported = ( + symbol: ts.Symbol, + currentSourceFile: ts.SourceFile | undefined, +): boolean => { + const { valueDeclaration } = symbol; + if (!valueDeclaration) { + // working around https://github.com/microsoft/TypeScript/issues/31294 + return false; + } + + return ( + !!currentSourceFile && + currentSourceFile !== valueDeclaration.getSourceFile() + ); +}; + +const getNodeName = (node: TSESTree.Node): string | null => + node.type === AST_NODE_TYPES.Identifier ? node.name : null; + +const getMemberFullName = ( + node: TSESTree.MemberExpression | TSESTree.OptionalMemberExpression, +): string => `${getNodeName(node.object)}.${getNodeName(node.property)}`; + export default util.createRule({ name: 'unbound-method', meta: { @@ -65,13 +120,22 @@ export default util.createRule({ return; } - const originalNode = parserServices.esTreeNodeToTSNodeMap.get(node); - const symbol = checker.getSymbolAtLocation(originalNode); + const objectSymbol = checker.getSymbolAtLocation( + parserServices.esTreeNodeToTSNodeMap.get(node.object), + ); if ( - symbol && - isDangerousMethod(symbol, ignoreStatic, currentSourceFile) + objectSymbol && + nativelyBoundMembers.includes(getMemberFullName(node)) && + isMemberImported(objectSymbol, currentSourceFile) ) { + return; + } + + const originalNode = parserServices.esTreeNodeToTSNodeMap.get(node); + const symbol = checker.getSymbolAtLocation(originalNode); + + if (symbol && isDangerousMethod(symbol, ignoreStatic)) { context.report({ messageId: 'unbound', node, @@ -82,24 +146,13 @@ export default util.createRule({ }, }); -function isDangerousMethod( - symbol: ts.Symbol, - ignoreStatic: boolean, - currentSourceFile?: ts.SourceFile, -): boolean { +function isDangerousMethod(symbol: ts.Symbol, ignoreStatic: boolean): boolean { const { valueDeclaration } = symbol; if (!valueDeclaration) { // working around https://github.com/microsoft/TypeScript/issues/31294 return false; } - if ( - currentSourceFile && - currentSourceFile !== valueDeclaration.getSourceFile() - ) { - return false; - } - switch (valueDeclaration.kind) { case ts.SyntaxKind.MethodDeclaration: case ts.SyntaxKind.MethodSignature: diff --git a/packages/eslint-plugin/tests/fixtures/class.ts b/packages/eslint-plugin/tests/fixtures/class.ts index 9b8aac7d7e38..74f222338ff5 100644 --- a/packages/eslint-plugin/tests/fixtures/class.ts +++ b/packages/eslint-plugin/tests/fixtures/class.ts @@ -1,2 +1,5 @@ // used by no-throw-literal test case to validate custom error export class Error {} + +// used by unbound-method test case to test imports +export const console = { log() {} }; diff --git a/packages/eslint-plugin/tests/rules/unbound-method.test.ts b/packages/eslint-plugin/tests/rules/unbound-method.test.ts index 0e8e30965fee..8f710eb61416 100644 --- a/packages/eslint-plugin/tests/rules/unbound-method.test.ts +++ b/packages/eslint-plugin/tests/rules/unbound-method.test.ts @@ -46,6 +46,8 @@ ruleTester.run('unbound-method', rule, { valid: [ 'Promise.resolve().then(console.log)', '["1", "2", "3"].map(Number.parseInt)', + '[5.2, 7.1, 3.6].map(Math.floor);', + 'const x = console.log', ` import console from './consoleshim'; Promise.resolve().then(console.log); @@ -234,6 +236,18 @@ Promise.resolve().then(console.log); }, ], }, + { + code: ` +import { console } from './class'; +const x = console.log; + `, + errors: [ + { + line: 3, + messageId: 'unbound', + }, + ], + }, { code: addContainsMethodsClass(` function foo(arg: ContainsMethods | null) { From 3e7c2c4887db1b70880cdf88361ccd9669180ffe Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Fri, 31 Jan 2020 18:51:01 +1300 Subject: [PATCH 3/6] fix(eslint-plugin): remove methodless globals from unbound-method --- packages/eslint-plugin/src/rules/unbound-method.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts index 3275d75bcb87..e3423039edec 100644 --- a/packages/eslint-plugin/src/rules/unbound-method.ts +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -25,13 +25,11 @@ const nativelyBoundMembers = [ Number, Object, String, - RegExp, Symbol, Array, Proxy, Date, ].map(x => [x.name, x]), - ['Infinity', Infinity], ['Atomics', Atomics], ['Reflect', Reflect], ['console', console], From fe1584d6e9089c8ea69b1c11b3d72647be28dbd6 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Fri, 31 Jan 2020 18:52:08 +1300 Subject: [PATCH 4/6] test(eslint-plugin): remove incorrect test --- packages/eslint-plugin/tests/rules/unbound-method.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/unbound-method.test.ts b/packages/eslint-plugin/tests/rules/unbound-method.test.ts index 8f710eb61416..2bf38e1c83e2 100644 --- a/packages/eslint-plugin/tests/rules/unbound-method.test.ts +++ b/packages/eslint-plugin/tests/rules/unbound-method.test.ts @@ -48,10 +48,6 @@ ruleTester.run('unbound-method', rule, { '["1", "2", "3"].map(Number.parseInt)', '[5.2, 7.1, 3.6].map(Math.floor);', 'const x = console.log', - ` -import console from './consoleshim'; -Promise.resolve().then(console.log); - `, ...[ 'instance.bound();', 'instance.unbound();', From cae3691d445d19c833b69e2a7be243cd342eac05 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Fri, 31 Jan 2020 18:57:17 +1300 Subject: [PATCH 5/6] chore(eslint-plugin): rename `isMemberImported` function --- packages/eslint-plugin/src/rules/unbound-method.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts index e3423039edec..fd0d19c857a9 100644 --- a/packages/eslint-plugin/src/rules/unbound-method.ts +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -48,7 +48,7 @@ const nativelyBoundMembers = [ ) .reduce((arr, names) => arr.concat(names), []); -const isMemberImported = ( +const isMemberNotImported = ( symbol: ts.Symbol, currentSourceFile: ts.SourceFile | undefined, ): boolean => { @@ -125,7 +125,7 @@ export default util.createRule({ if ( objectSymbol && nativelyBoundMembers.includes(getMemberFullName(node)) && - isMemberImported(objectSymbol, currentSourceFile) + isMemberNotImported(objectSymbol, currentSourceFile) ) { return; } From 4cd7515ea1fb85a89f3d57f2a1cca7664f7094a4 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Mon, 3 Feb 2020 09:42:43 -0800 Subject: [PATCH 6/6] fix: augment node types for es2015+ --- .../eslint-plugin/src/rules/unbound-method.ts | 46 +++++++++---------- packages/eslint-plugin/typings/node.d.ts | 8 ++++ 2 files changed, 31 insertions(+), 23 deletions(-) create mode 100644 packages/eslint-plugin/typings/node.d.ts diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts index fd0d19c857a9..bdd740fb4d4c 100644 --- a/packages/eslint-plugin/src/rules/unbound-method.ts +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -18,34 +18,34 @@ export type Options = [Config]; export type MessageIds = 'unbound'; -const nativelyBoundMembers = [ - ...[ - Promise, - // BigInt, // todo: enable when we drop node@8 - Number, - Object, - String, - Symbol, - Array, - Proxy, - Date, - ].map(x => [x.name, x]), - ['Atomics', Atomics], - ['Reflect', Reflect], - ['console', console], - ['Math', Math], - ['JSON', JSON], - ['Intl', Intl], -] - .map(([namespace, object]) => - Object.getOwnPropertyNames(object) +const nativelyBoundMembers = ([ + 'Promise', + 'Number', + 'Object', + 'String', // eslint-disable-line @typescript-eslint/internal/prefer-ast-types-enum + 'RegExp', + 'Symbol', + 'Array', + 'Proxy', + 'Date', + 'Infinity', + 'Atomics', + 'Reflect', + 'console', + 'Math', + 'JSON', + 'Intl', +] as const) + .map(namespace => { + const object = global[namespace]; + return Object.getOwnPropertyNames(object) .filter( name => !name.startsWith('_') && typeof (object as Record)[name] === 'function', ) - .map(name => `${namespace}.${name}`), - ) + .map(name => `${namespace}.${name}`); + }) .reduce((arr, names) => arr.concat(names), []); const isMemberNotImported = ( diff --git a/packages/eslint-plugin/typings/node.d.ts b/packages/eslint-plugin/typings/node.d.ts new file mode 100644 index 000000000000..6caf4f045544 --- /dev/null +++ b/packages/eslint-plugin/typings/node.d.ts @@ -0,0 +1,8 @@ +// augment nodejs global with ES2015+ things +declare namespace NodeJS { + interface Global { + Atomics: typeof Atomics; + Proxy: typeof Proxy; + Reflect: typeof Reflect; + } +}