[enhancement] member-access rule on parameter properties (#3325)

palantir · Oct 18, 2017 · d197ee3 · d197ee3
1 parent 74d84be
commit d197ee3
Show file tree

Hide file tree

Showing 9 changed files with 103 additions and 21 deletions.
diff --git a/docs/_data/formatters.json b/docs/_data/formatters.json
@@ -55,7 +55,7 @@
   {
     "formatterName": "stylish",
     "description": "Human-readable formatter which creates stylish messages.",
-    "descriptionDetails": "\nThe output matches that produced by eslint's stylish formatter. Its readability\nenhanced through spacing and colouring",
+    "descriptionDetails": "\nThe output matches what is produced by ESLint's stylish formatter.\nIts readability is enhanced through spacing and colouring.",
     "sample": "\nmyFile.ts\n1:14  semicolon  Missing semicolon",
     "consumer": "human"
   },

diff --git a/docs/formatters/stylish/index.html b/docs/formatters/stylish/index.html
@@ -3,8 +3,8 @@
 description: Human-readable formatter which creates stylish messages.
 descriptionDetails: |-
 
-  The output matches that produced by eslint's stylish formatter. Its readability
-  enhanced through spacing and colouring
+  The output matches what is produced by ESLint's stylish formatter.
+  Its readability is enhanced through spacing and colouring.
 sample: |-
 
   myFile.ts

diff --git a/src/configs/all.ts b/src/configs/all.ts
@@ -36,7 +36,7 @@ export const rules = {
             ["Symbol", "Avoid using the `Symbol` type. Did you mean `symbol`?"],
         ],
     },
-    "member-access": [true, "check-accessor", "check-constructor"],
+    "member-access": [true, "check-accessor", "check-constructor", "check-parameter-property"],
     "member-ordering": [true, {
         "order": "statics-first",
         "alphabetize": true,

diff --git a/src/rules/memberAccessRule.ts b/src/rules/memberAccessRule.ts
@@ -15,7 +15,15 @@
  * limitations under the License.
  */
 
-import { getChildOfKind, getModifier, getNextToken, getTokenAtPosition, isClassLikeDeclaration } from "tsutils";
+import {
+    getChildOfKind,
+    getModifier,
+    getNextToken,
+    getTokenAtPosition,
+    isClassLikeDeclaration,
+    isConstructorDeclaration,
+    isParameterProperty,
+} from "tsutils";
 import * as ts from "typescript";
 
 import { showWarningOnce } from "../error";
@@ -24,11 +32,13 @@ import * as Lint from "../index";
 const OPTION_NO_PUBLIC = "no-public";
 const OPTION_CHECK_ACCESSOR = "check-accessor";
 const OPTION_CHECK_CONSTRUCTOR = "check-constructor";
+const OPTION_CHECK_PARAMETER_PROPERTY = "check-parameter-property";
 
 interface Options {
     noPublic: boolean;
     checkAccessor: boolean;
     checkConstructor: boolean;
+    checkParameterProperty: boolean;
 }
 
 export class Rule extends Lint.Rules.AbstractRule {
@@ -42,15 +52,16 @@ export class Rule extends Lint.Rules.AbstractRule {
 
             * \`"no-public"\` forbids public accessibility to be specified, because this is the default.
             * \`"check-accessor"\` enforces explicit visibility on get/set accessors
-            * \`"check-constructor"\`  enforces explicit visibility on constructors`,
+            * \`"check-constructor"\`  enforces explicit visibility on constructors
+            * \`"check-parameter-property"\`  enforces explicit visibility on parameter properties`,
         options: {
             type: "array",
             items: {
                 type: "string",
-                enum: [OPTION_NO_PUBLIC, OPTION_CHECK_ACCESSOR, OPTION_CHECK_CONSTRUCTOR],
+                enum: [OPTION_NO_PUBLIC, OPTION_CHECK_ACCESSOR, OPTION_CHECK_CONSTRUCTOR, OPTION_CHECK_PARAMETER_PROPERTY],
             },
             minLength: 0,
-            maxLength: 3,
+            maxLength: 4,
         },
         optionExamples: [true, [true, OPTION_NO_PUBLIC], [true, OPTION_CHECK_ACCESSOR]],
         type: "typescript",
@@ -71,29 +82,38 @@ export class Rule extends Lint.Rules.AbstractRule {
         const noPublic = options.indexOf(OPTION_NO_PUBLIC) !== -1;
         let checkAccessor = options.indexOf(OPTION_CHECK_ACCESSOR) !== -1;
         let checkConstructor = options.indexOf(OPTION_CHECK_CONSTRUCTOR) !== -1;
+        let checkParameterProperty = options.indexOf(OPTION_CHECK_PARAMETER_PROPERTY) !== -1;
         if (noPublic) {
-            if (checkAccessor || checkConstructor) {
+            if (checkAccessor || checkConstructor || checkParameterProperty) {
                 showWarningOnce(`Warning: ${this.ruleName} - If 'no-public' is present, it should be the only option.`);
                 return [];
             }
-            checkAccessor = checkConstructor = true;
+            checkAccessor = checkConstructor = checkParameterProperty = true;
         }
         return this.applyWithFunction(sourceFile, walk, {
             checkAccessor,
             checkConstructor,
+            checkParameterProperty,
             noPublic,
         });
     }
 }
 
 function walk(ctx: Lint.WalkContext<Options>) {
-    const {noPublic, checkAccessor, checkConstructor} = ctx.options;
+    const { noPublic, checkAccessor, checkConstructor, checkParameterProperty } = ctx.options;
     return ts.forEachChild(ctx.sourceFile, function recur(node: ts.Node): void {
         if (isClassLikeDeclaration(node)) {
             for (const child of node.members) {
                 if (shouldCheck(child)) {
                     check(child);
                 }
+                if (checkParameterProperty && isConstructorDeclaration(child) && child.body !== undefined) {
+                    for (const param of child.parameters) {
+                        if (isParameterProperty(param)) {
+                            check(param);
+                        }
+                    }
+                }
             }
         }
         return ts.forEachChild(node, recur);
@@ -114,19 +134,24 @@ function walk(ctx: Lint.WalkContext<Options>) {
         }
     }
 
-    function check(node: ts.ClassElement): void {
+    function check(node: ts.ClassElement | ts.ParameterDeclaration): void {
         if (Lint.hasModifier(node.modifiers, ts.SyntaxKind.ProtectedKeyword, ts.SyntaxKind.PrivateKeyword)) {
             return;
         }
         const publicKeyword = getModifier(node, ts.SyntaxKind.PublicKeyword);
         if (noPublic && publicKeyword !== undefined) {
-            const start = publicKeyword.end - "public".length;
-            ctx.addFailure(
-                start,
-                publicKeyword.end,
-                Rule.FAILURE_STRING_NO_PUBLIC,
-                Lint.Replacement.deleteFromTo(start, getNextToken(publicKeyword, ctx.sourceFile)!.getStart(ctx.sourceFile)),
-            );
+            const readonlyKeyword = getModifier(node, ts.SyntaxKind.ReadonlyKeyword);
+            // public is not optional for parameter property without the readonly modifier
+            const isPublicOptional = node.kind !== ts.SyntaxKind.Parameter || readonlyKeyword !== undefined;
+            if (isPublicOptional) {
+                const start = publicKeyword.end - "public".length;
+                ctx.addFailure(
+                    start,
+                    publicKeyword.end,
+                    Rule.FAILURE_STRING_NO_PUBLIC,
+                    Lint.Replacement.deleteFromTo(start, getNextToken(publicKeyword, ctx.sourceFile)!.getStart(ctx.sourceFile)),
+                );
+            }
         }
         if (!noPublic && publicKeyword === undefined) {
             const nameNode = node.kind === ts.SyntaxKind.Constructor
@@ -142,12 +167,12 @@ function walk(ctx: Lint.WalkContext<Options>) {
     }
 }
 
-function getInsertionPosition(member: ts.ClassElement, sourceFile: ts.SourceFile): number {
+function getInsertionPosition(member: ts.ClassElement | ts.ParameterDeclaration, sourceFile: ts.SourceFile): number {
     const node = member.decorators === undefined ? member : getTokenAtPosition(member, member.decorators.end, sourceFile)!;
     return node.getStart(sourceFile);
 }
 
-function typeToString(node: ts.ClassElement): string {
+function typeToString(node: ts.ClassElement | ts.ParameterDeclaration): string {
     switch (node.kind) {
         case ts.SyntaxKind.MethodDeclaration:
             return "class method";
@@ -159,6 +184,8 @@ function typeToString(node: ts.ClassElement): string {
             return "get property accessor";
         case ts.SyntaxKind.SetAccessor:
             return "set property accessor";
+        case ts.SyntaxKind.Parameter:
+            return "parameter property";
         default:
             throw new Error(`unhandled node type ${ts.SyntaxKind[node.kind]}`);
     }

diff --git a/test/rules/member-access/no-public/test.ts.fix b/test/rules/member-access/no-public/test.ts.fix
@@ -3,5 +3,10 @@ class {
     constructor() {}
     get y() {}
     public() {}
+    constructor(readonly param : number) {}
+    constructor(readonly param : number) {}
+    constructor(private readonly paramPrivate : number, protected readonly paramProtected : number, readonly paramReadonly : number) {}
+
+    constructor(public necessaryPublic : number) {}
 }
 
diff --git a/test/rules/member-access/no-public/test.ts.lint b/test/rules/member-access/no-public/test.ts.lint
@@ -6,6 +6,14 @@ class {
     public/*some comment*/ get y() {}
     ~~~~~~ [0]
     public() {}
+    constructor(public readonly param : number) {}
+                ~~~~~~ [0]
+    public constructor(public readonly param : number) {}
+    ~~~~~~ [0]         
+                       ~~~~~~ [0]
+    constructor(private readonly paramPrivate : number, protected readonly paramProtected : number, readonly paramReadonly : number) {}
+
+    constructor(public necessaryPublic : number) {}
 }
 
 [0]: 'public' is implicit.
diff --git a/test/rules/member-access/parameter-property/test.ts.fix b/test/rules/member-access/parameter-property/test.ts.fix
@@ -0,0 +1,17 @@
+class Members {
+    constructor(
+        public readonly paramReadonly : number,
+        public paramPublic : number,
+        protected paramProtected : number,
+        private paramPrivate : number,
+        public readonly paramPublicReadonly : number,
+        protected readonly paramProtectedReadonly : number,
+        private readonly paramPrivateReadonly : number
+    ){
+
+    }
+
+    constructor(public readonly a : number, @decorated public readonly b : number) {
+
+    }
+}
diff --git a/test/rules/member-access/parameter-property/test.ts.lint b/test/rules/member-access/parameter-property/test.ts.lint
@@ -0,0 +1,20 @@
+class Members {
+    constructor(
+        readonly paramReadonly : number,
+                 ~~~~~~~~~~~~~ [The parameter property 'paramReadonly' must be marked either 'private', 'public', or 'protected']
+        public paramPublic : number,
+        protected paramProtected : number,
+        private paramPrivate : number,
+        public readonly paramPublicReadonly : number,
+        protected readonly paramProtectedReadonly : number,
+        private readonly paramPrivateReadonly : number
+    ){
+
+    }
+
+    constructor(readonly a : number, @decorated readonly b : number) {
+                         ~ [The parameter property 'a' must be marked either 'private', 'public', or 'protected']
+                                                         ~ [The parameter property 'b' must be marked either 'private', 'public', or 'protected']
+
+    }
+}
diff --git a/test/rules/member-access/parameter-property/tslint.json b/test/rules/member-access/parameter-property/tslint.json
@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "member-access": [true, "check-parameter-property"]
+  }
+}