Handle overloaded method edge case (#16)

Also, clean up the code

.gitignore

@@ -1,6 +1,6 @@
 node_modules
 rules/*.js
 rules/*.d.ts
-test/fixes/spec.bad.tofix.ts
+test/fixers/spec.bad.tofix.ts
 *.js
 *.d.ts

.travis.yml

@@ -6,7 +6,6 @@ script:
 - npm run build
 - npm run lint
 - npm test
-- "./test/fixes/test.sh"
 deploy:
   provider: npm
   email: [email protected]

package.json

@@ -4,7 +4,7 @@
     "description": "tslint support for an override keyword",
     "main": "tslint-override.json",
     "scripts": {
-        "test": "tslint --test test && node_modules/.bin/ts-node -P test/tsconfig.json test/test.ts && tslint -p test test/test.ts && tslint -p test/exclude-interfaces",
+        "test": "./test.sh",
         "lint": "tslint -p .",
         "tslint": "tslint",
         "build": "npm run clean && tsc -p .",

rules/explicitOverrideRule.ts

@@ -27,6 +27,11 @@ const OPTION_DECORATOR = 'decorator';
 const OPTION_JSDOC_TAG = 'jsdoc';
 const OPTION_EXCLUDE_INTERFACES = 'exclude-interfaces';
 
+const MESSAGE_EXTRA_CONSTRUCTOR = 'Extraneous override keyword: constructors always override the parent';
+const MESSAGE_EXTRA_STATIC = 'Extraneous override keyword: static members cannot override';
+const MESSAGE_EXTRA_NO_OVERRIDE = 'Member with @override keyword does not override any base class member';
+const MESSAGE_MISSING_OVERRIDE = 'Member is overriding a base member. Use the @override keyword if this override is intended';
+
 interface IOptions {
     useJsdocTag: boolean;
     useDecorator: boolean;
@@ -121,16 +126,14 @@ class Walker extends Lint.AbstractWalker<IOptions> {
     private checkNonOverrideableElementDeclaration(node: AllClassElements) {
         const foundKeyword = this.checkNodeForOverrideKeyword(node);
         if (foundKeyword !== undefined) {
-            this.addFailureAtNode(foundKeyword, 'Extraneous override keyword',
-                    Lint.Replacement.deleteText(foundKeyword.getStart(), foundKeyword.getWidth()));
+            this.addFailureAtNode(foundKeyword, 'Extraneous override keyword', fixRemoveOverrideKeyword(foundKeyword));
         }
     }
 
     private checkConstructorDeclaration(node: ts.ConstructorDeclaration) {
         const foundKeyword = this.checkNodeForOverrideKeyword(node);
         if (foundKeyword !== undefined) {
-            this.addFailureAtNode(foundKeyword, 'Extraneous override keyword: constructors always override the parent',
-                    Lint.Replacement.deleteText(foundKeyword.getStart(), foundKeyword.getWidth()));
+            this.addFailureAtNode(foundKeyword, MESSAGE_EXTRA_CONSTRUCTOR, fixRemoveOverrideKeyword(foundKeyword));
         }
     }
 
@@ -139,26 +142,26 @@ class Walker extends Lint.AbstractWalker<IOptions> {
 
         if (isStaticMember(node)) {
             if (foundKeyword !== undefined) {
-                this.addFailureAtNode(foundKeyword, 'Extraneous override keyword: static members cannot override',
-                        Lint.Replacement.deleteText(foundKeyword.getStart(), foundKeyword.getWidth()));
+                this.addFailureAtNode(foundKeyword, MESSAGE_EXTRA_STATIC, fixRemoveOverrideKeyword(foundKeyword));
             }
             return;
         }
 
+        if (!ts.isPropertyDeclaration(node) && node.body === undefined) {
+            // Special case if this is just an overload signature
+            return;
+        }
+
         const parent = node.parent;
         if (parent == null || !isClassType(parent)) {
             return;
         }
         const base = this.checkHeritageChain(parent, node);
 
         if (foundKeyword !== undefined && base === undefined) {
-            this.addFailureAtNode(node.name, 'Member with @override keyword does not override any base class member',
-                    Lint.Replacement.deleteText(foundKeyword.getStart(), foundKeyword.getWidth()));
+            this.addFailureAtNode(node.name, MESSAGE_EXTRA_NO_OVERRIDE, fixRemoveOverrideKeyword(foundKeyword));
         } else if (foundKeyword === undefined && base !== undefined) {
-            const fix = this.fixAddOverrideKeyword(node);
-            this.addFailureAtNode(node.name,
-                    'Member is overriding a base member. Use the @override keyword if this override is intended',
-                    fix);
+            this.addFailureAtNode(node.name, MESSAGE_MISSING_OVERRIDE, this.fixAddOverrideKeyword(node));
         }
     }
 
@@ -328,6 +331,10 @@ class Walker extends Lint.AbstractWalker<IOptions> {
     }
 }
 
+function fixRemoveOverrideKeyword(keyword: OverrideKeyword) {
+    return Lint.Replacement.deleteText(keyword.getStart(), keyword.getWidth());
+}
+
 function isStaticMember(node: ts.Node): boolean {
     return (ts.getCombinedModifierFlags(node) & ts.ModifierFlags.Static) !== 0;
 }

test.sh

@@ -0,0 +1,23 @@
+#!/bin/sh -xe
+
+# Test the lint rule
+
+tslint --test test
+node_modules/.bin/ts-node -P test/tsconfig.json test/test.ts
+tslint -p test test/test.ts
+tslint -p test/exclude-interfaces
+
+# Test the fixers
+
+FIX_TEST_DIR="test/fixers"
+
+cp "${FIX_TEST_DIR}/spec.bad.ts" "${FIX_TEST_DIR}/spec.bad.tofix.ts" # Make a copy to keep the original untouched
+npm run tslint -- --fix -p "${FIX_TEST_DIR}" # Fix
+diff "${FIX_TEST_DIR}/spec.fixed.jsdoc.ts" "${FIX_TEST_DIR}/spec.bad.tofix.ts" # Check the result against the reference
+node_modules/.bin/ts-node -P "${FIX_TEST_DIR}/tsconfig.json" "${FIX_TEST_DIR}/spec.bad.tofix.ts" # Check that the result is valid for tsc
+
+# Repeat for the decorator-based fixer
+cp "${FIX_TEST_DIR}/spec.bad.ts" "${FIX_TEST_DIR}/spec.bad.tofix.ts"
+npm run tslint -- --fix -c ${FIX_TEST_DIR}/tslint.decorator.json -p "${FIX_TEST_DIR}"
+diff "${FIX_TEST_DIR}/spec.fixed.decorator.ts" "${FIX_TEST_DIR}/spec.bad.tofix.ts"
+node_modules/.bin/ts-node -P "${FIX_TEST_DIR}/tsconfig.json" "${FIX_TEST_DIR}/spec.bad.tofix.ts"

test/fixes/spec.bad.ts → test/fixers/spec.bad.ts

@@ -1,3 +1,5 @@
+import '../../register';
+
 abstract class First {
 
 }
@@ -7,11 +9,17 @@ class Foo extends First {
     /** doc comment **/
     public bar(): void { }
 
+    public bar2(): void { }
+
     public overrideMe(arg0: string): number { return 0; }
     public overrideMe2(arg0: string): number { return 0; }
     public overrideMe3(arg0: string): number { return 0; }
 
     public notToOverride(): void { } 
+
+    public overloadedMethod(): void;
+    public overloadedMethod(v: string): void;
+    public overloadedMethod(v?: string): void { }
 }
 
 export class Baz extends Foo {
@@ -60,5 +68,9 @@ export class Baz extends Foo {
     public notToOverride(): void { }
 
     @override
-    @override public bar(): void { }
+    @override public bar2(): void { }
+
+    public overloadedMethod(): void;
+    public overloadedMethod(v: string): void;
+    public overloadedMethod(v?: string): void { }
 }

test/fixes/spec.fixed.decorator.ts → test/fixers/spec.fixed.decorator.ts

@@ -1,3 +1,5 @@
+import '../../register';
+
 abstract class First {
 
 }
@@ -7,11 +9,17 @@ class Foo extends First {
     /** doc comment **/
     public bar(): void { }
 
+    public bar2(): void { }
+
     public overrideMe(arg0: string): number { return 0; }
     public overrideMe2(arg0: string): number { return 0; }
     public overrideMe3(arg0: string): number { return 0; }
 
     public notToOverride(): void { } 
+
+    public overloadedMethod(): void;
+    public overloadedMethod(v: string): void;
+    public overloadedMethod(v?: string): void { }
 }
 
 export class Baz extends Foo {
@@ -59,5 +67,9 @@ export class Baz extends Foo {
 
     @override public notToOverride(): void { }
 
-    @override public bar(): void { }
+    @override public bar2(): void { }
+
+    public overloadedMethod(): void;
+    public overloadedMethod(v: string): void;
+    @override public overloadedMethod(v?: string): void { }
 }

test/fixes/spec.fixed.jsdoc.ts → test/fixers/spec.fixed.jsdoc.ts

@@ -1,3 +1,5 @@
+import '../../register';
+
 abstract class First {
 
 }
@@ -7,11 +9,17 @@ class Foo extends First {
     /** doc comment **/
     public bar(): void { }
 
+    public bar2(): void { }
+
     public overrideMe(arg0: string): number { return 0; }
     public overrideMe2(arg0: string): number { return 0; }
     public overrideMe3(arg0: string): number { return 0; }
 
     public notToOverride(): void { } 
+
+    public overloadedMethod(): void;
+    public overloadedMethod(v: string): void;
+    public overloadedMethod(v?: string): void { }
 }
 
 export class Baz extends Foo {
@@ -62,5 +70,9 @@ export class Baz extends Foo {
 
     /** @override */ public notToOverride(): void { }
 
-    @override public bar(): void { }
+    @override public bar2(): void { }
+
+    public overloadedMethod(): void;
+    public overloadedMethod(v: string): void;
+    /** @override */ public overloadedMethod(v?: string): void { }
 }

test/fixers/tsconfig.json

@@ -0,0 +1,10 @@
+{
+    "compilerOptions": {
+      "target": "es2015",
+      "experimentalDecorators": true,
+      "strict": true
+    },
+    "files": [
+      "spec.bad.tofix.ts"
+    ]
+  }

test/spec.ts.lint

@@ -27,6 +27,9 @@ class Base extends ABase {
     public propertyNotToOverride = 1;
     public methodToOverride(): void { }
     public methodNotToOverride(): void { }
+    public overloadedMethod(a: number): void;
+    public overloadedMethod(a: string): void;
+    public overloadedMethod(a: number | string): void { }
 }
 
 const DynBase = class extends ABase {
@@ -115,6 +118,11 @@ class Example1 extends Base {
     /** @override */ public aMethodToOverride(): void { }
     public aMethodNotToOverride(): void { }
            ~~~~~~~~~~~~~~~~~~~~        [Member is overriding a base member. Use the @override keyword if this override is intended]
+
+    public overloadedMethod(a: number): void;
+    public overloadedMethod(a: string): void;
+    public overloadedMethod(a: number | string): void { }
+           ~~~~~~~~~~~~~~~~            [Member is overriding a base member. Use the @override keyword if this override is intended]
 }
 
 const Example2 = class extends Base {

test/tslint.json

@@ -1,3 +1,4 @@
 {
-    "extends": "../tslint-override.json"
+    "extends": "../tslint-override.json",
+    "one-line": true
 }