From 3b3107b6106e87b184023f0a140657403f11a3b8 Mon Sep 17 00:00:00 2001 From: Kanitkorn S Date: Mon, 4 Feb 2019 19:37:28 +0900 Subject: [PATCH 1/3] fix(eslint-plugin): fix false positive from adjacent-overload-signatures `adjacent-overload-signatures` rule won't distinguish between static and instance methods, despite they're different. So when they have the same name, they'll be treated as the same thing, which can cause a false positive result. Fix https://github.com/typescript-eslint/typescript-eslint/issues/169 --- .../lib/rules/adjacent-overload-signatures.js | 12 +++--- .../lib/rules/adjacent-overload-signatures.js | 37 +++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js b/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js index d8eb79509558..8fce10e07da9 100644 --- a/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js +++ b/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js @@ -84,27 +84,29 @@ module.exports = { if (members) { let name; + let nameWithStatic; let index; let lastName; const seen = []; members.forEach(member => { name = getMemberName(member); + nameWithStatic = (member.static ? 'static ' : '') + name; - index = seen.indexOf(name); - if (index > -1 && lastName !== name) { + index = seen.indexOf(nameWithStatic); + if (index > -1 && lastName !== nameWithStatic) { context.report({ node: member, messageId: 'adjacentSignature', data: { - name + name: nameWithStatic } }); } else if (name && index === -1) { - seen.push(name); + seen.push(nameWithStatic); } - lastName = name; + lastName = nameWithStatic; }); } } diff --git a/packages/eslint-plugin/tests/lib/rules/adjacent-overload-signatures.js b/packages/eslint-plugin/tests/lib/rules/adjacent-overload-signatures.js index 161ffd4035e6..fe42efd1015b 100644 --- a/packages/eslint-plugin/tests/lib/rules/adjacent-overload-signatures.js +++ b/packages/eslint-plugin/tests/lib/rules/adjacent-overload-signatures.js @@ -211,6 +211,23 @@ class Foo { foo(sn: string | number): void {} bar(): void {} baz(): void {} +} + `, + ` +class Foo { + name: string; + static foo(s: string): void; + static foo(n: number): void; + static foo(sn: string | number): void {} + bar(): void {} + baz(): void {} +} + `, + ` +class Test { + static test() {} + untest() {} + test() {} } `, // examples from https://github.com/nzakas/eslint-plugin-typescript/issues/138 @@ -789,6 +806,26 @@ class Foo { column: 5 } ] + }, + { + code: ` +class Foo { + static foo(s: string): void; + name: string; + static foo(n: number): void; + static foo(sn: string | number): void {} + bar(): void {} + baz(): void {} +} + `, + errors: [ + { + messageId: 'adjacentSignature', + data: { name: 'static foo' }, + line: 5, + column: 5 + } + ] } ] }); From 2285b6c2565f3f9a4c3429254a64bfed72c171a7 Mon Sep 17 00:00:00 2001 From: Kanitkorn S Date: Tue, 5 Feb 2019 14:34:23 +0900 Subject: [PATCH 2/3] refactor: switch interim variable type to object according to PR comment Comment: https://github.com/typescript-eslint/typescript-eslint/pull/206#issuecomment-460327368 --- .../lib/rules/adjacent-overload-signatures.js | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js b/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js index 8fce10e07da9..555ba163adbd 100644 --- a/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js +++ b/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js @@ -73,6 +73,17 @@ module.exports = { } } + /** + * Determine whether two methods are the same or not + * @param {{ name: string; static: boolean }} method1 a method to compare + * @param {{ name: string; static: boolean }} method2 another method to compare with + * @returns {boolean} true if two methods are the same + * @private + */ + function isSameMethod(method1, method2) { + return method1.name === method2.name && method1.static === method2.static; + } + /** * Check the body for overload methods. * @param {ASTNode} node the body to be inspected. @@ -84,29 +95,34 @@ module.exports = { if (members) { let name; - let nameWithStatic; + let method; let index; - let lastName; - const seen = []; + let lastMethod; + const seenMethods = []; members.forEach(member => { name = getMemberName(member); - nameWithStatic = (member.static ? 'static ' : '') + name; + method = { + name, + static: member.static + }; - index = seen.indexOf(nameWithStatic); - if (index > -1 && lastName !== nameWithStatic) { + index = seenMethods.findIndex(seenMethod => + isSameMethod(method, seenMethod) + ); + if (index > -1 && !isSameMethod(method, lastMethod)) { context.report({ node: member, messageId: 'adjacentSignature', data: { - name: nameWithStatic + name: (method.static ? 'static ' : '') + method.name } }); } else if (name && index === -1) { - seen.push(nameWithStatic); + seenMethods.push(method); } - lastName = nameWithStatic; + lastMethod = method; }); } } From 060c5fe9504e1f4e98410bca2aced8b3eff01569 Mon Sep 17 00:00:00 2001 From: Kanitkorn S Date: Tue, 5 Feb 2019 16:45:36 +0900 Subject: [PATCH 3/3] refactor: move variables those are not being reused into the inner scope According to PR review: https://github.com/typescript-eslint/typescript-eslint/pull/206#discussion_r253755582 --- .../lib/rules/adjacent-overload-signatures.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js b/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js index 555ba163adbd..0c53769c0e8c 100644 --- a/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js +++ b/packages/eslint-plugin/lib/rules/adjacent-overload-signatures.js @@ -94,20 +94,17 @@ module.exports = { const members = node.body || node.members; if (members) { - let name; - let method; - let index; let lastMethod; const seenMethods = []; members.forEach(member => { - name = getMemberName(member); - method = { + const name = getMemberName(member); + const method = { name, static: member.static }; - index = seenMethods.findIndex(seenMethod => + const index = seenMethods.findIndex(seenMethod => isSameMethod(method, seenMethod) ); if (index > -1 && !isSameMethod(method, lastMethod)) {