readonly-keyword rule, check function params and return type (#80)

* Fix parameters

* Return type

* Update readme to mention return types

README.md

@@ -398,7 +398,7 @@ For more background see this [blog post](https://hackernoon.com/rethinking-javas
 > If a tree falls in the woods, does it make a sound?
 > If a pure function mutates some local data in order to produce an immutable return value, is that ok?
 
-The quote above is from the [clojure docs](https://clojure.org/reference/transients). In general, it is more important to enforce immutability for state that is passed in and out of functions than for local state used for internal calculations within a function. For example in Redux, the state going in and out of reducers needs to be immutable while the reducer may be allowed to mutate local state in its calculations in order to achieve higher performance. This is what the `ignore-local` option enables. With this option enabled immutability will be enforced everywhere but in local state. Function parameters are not considered local state so they will still be checked.
+The quote above is from the [clojure docs](https://clojure.org/reference/transients). In general, it is more important to enforce immutability for state that is passed in and out of functions than for local state used for internal calculations within a function. For example in Redux, the state going in and out of reducers needs to be immutable while the reducer may be allowed to mutate local state in its calculations in order to achieve higher performance. This is what the `ignore-local` option enables. With this option enabled immutability will be enforced everywhere but in local state. Function parameters and return types are not considered local state so they will still be checked.
 
 Note that using this option can lead to more imperative code in functions so use with care!
 

package.json

@@ -7,10 +7,7 @@
     "type": "git",
     "url": "git+https://github.com/jonaskello/tslint-immutable.git"
   },
-  "keywords": [
-    "tslint",
-    "immutability"
-  ],
+  "keywords": ["tslint", "immutability"],
   "author": "Jonas Kello",
   "license": "MIT",
   "bugs": {
@@ -35,7 +32,8 @@
     "build": "rimraf rules && yarn compile",
     "lint": "tslint './src/**/*.ts{,x}'",
     "test": "tslint --test test/rules/**/*",
-    "test:work": "yarn build && tslint --test test/rules/no-object-mutation/*",
+    "test:work":
+      "yarn build && tslint --test test/rules/readonly-keyword/ignore-local/*",
     "verify": "yarn build && yarn lint && yarn coverage",
     "coverage": "rimraf coverage .nyc_output && nyc yarn test",
     "report-coverage": "cat ./coverage/lcov.info | coveralls",
@@ -45,9 +43,6 @@
   },
   "lint-staged": {
     "*.{ts,tsx}": "tslint",
-    "*.{ts,tsx,json,css}": [
-      "prettier --write",
-      "git add"
-    ]
+    "*.{ts,tsx,json,css}": ["prettier --write", "git add"]
   }
 }

src/shared/ignore.ts

@@ -79,23 +79,41 @@ function checkIgnoreLocalFunctionNode(
 ): ReadonlyArray<CheckNode.InvalidNode> {
   let myInvalidNodes: Array<CheckNode.InvalidNode> = [];
 
+  const cb = (node: ts.Node): void => {
+    // Check the node
+    const { invalidNodes, skipChildren } = checkNode(node, ctx);
+    if (invalidNodes) {
+      myInvalidNodes = myInvalidNodes.concat(...invalidNodes);
+    }
+    if (skipChildren) {
+      return;
+    }
+    // Use return becuase performance hints docs say it optimizes the function using tail-call recursion
+    return ts.forEachChild(node, cb);
+  };
+
   // Check either the parameter's explicit type if it has one, or itself for implict type
   for (const n of functionNode.parameters.map(p => (p.type ? p.type : p))) {
+    // Check the parameter node itself
     const { invalidNodes: invalidCheckNodes } = checkNode(n, ctx);
     if (invalidCheckNodes) {
       myInvalidNodes = myInvalidNodes.concat(...invalidCheckNodes);
     }
+
+    // Check all children for the paramter node
+    ts.forEachChild(n, cb);
   }
 
   // Check the return type
-  if (functionNode.type) {
-    const { invalidNodes: invalidCheckNodes } = checkNode(
-      functionNode.type,
-      ctx
-    );
+  const nt = functionNode.type;
+  if (nt) {
+    // Check the return type node itself
+    const { invalidNodes: invalidCheckNodes } = checkNode(nt, ctx);
     if (invalidCheckNodes) {
       myInvalidNodes = myInvalidNodes.concat(...invalidCheckNodes);
     }
+    // Check all children for the return type node
+    ts.forEachChild(nt, cb);
   }
 
   return myInvalidNodes;

test/rules/readonly-keyword/default/index-signature.ts.fix

@@ -10,6 +10,12 @@ function foo(): { readonly [source: string]: string } {
   return undefined;
 }
 
+// Index signature as parameter to function
+
+function foo(bar: { readonly [source: string]: string }): void {
+  return undefined;
+}
+
 // Index signature with nested object.
 interface Foo {
   readonly [key: string]: { readonly bar: string }

test/rules/readonly-keyword/default/index-signature.ts.lint

@@ -12,6 +12,13 @@ function foo(): { [source: string]: string } {
   return undefined;
 }
 
+// Index signature as parameter to function
+
+function foo(bar: { [source: string]: string }): void {
+                    ~~~~~~~~~~~~~~~~~~~~~~~~ [failure]
+  return undefined;
+}
+
 // Index signature with nested object.
 interface Foo {
   [key: string]: { bar: string }

test/rules/readonly-keyword/ignore-local/function.ts.lint

@@ -0,0 +1,29 @@
+// Index signature as return value from function
+
+function foo(): { [source: string]: string } {
+                  ~~~~~~~~~~~~~~~~~~~~~~~~ [failure]
+  return undefined;
+}
+
+// Index signature as parameter to function
+
+function foo(bar: { [source: string]: string }): void {
+                    ~~~~~~~~~~~~~~~~~~~~~~~~ [failure]
+  return undefined;
+}
+
+// Object as parameter to function
+
+function foo(bar: { zoo: string }): void {
+                    ~~~~~~~~~~~ [failure]
+  return undefined;
+}
+
+// Object as return value from function
+
+function foo(): { zoo: string } {
+                  ~~~~~~~~~~~ [failure]
+  return undefined;
+}
+
+[failure]: A readonly modifier is required.

test/rules/readonly-keyword/ignore-local/interface.ts.lint

@@ -58,6 +58,4 @@ function foo() {
   }
 }
 
-
-
 [failure]: A readonly modifier is required.