build: expand decorators validation rule to cover class members (#17673)

crisbeto · jelbourn · commit bc58e79f0c8b · 2019-11-18T18:52:52.000+01:00
Expands the lint rule that we use to validate class decorators to also apply to class members. We need this in order to support linting things like `descendants` on `ContentChildren`.

These changes switch around the config data structure to make it more flexible, fix some cases where we were using `any` and add some extra properties to the config.
diff --git a/tools/tslint-rules/validateDecoratorsRule.ts b/tools/tslint-rules/validateDecoratorsRule.ts
@@ -11,10 +11,16 @@ import * as minimatch from 'minimatch';
  * ```
  * "validate-decorators": [true, {
  *   "Component": {
- *     "encapsulation": "\\.None$",
- *     "!styles": ".*"
+ *     "argument": 0,
+ *     "properties": {
+ *       "encapsulation": "\\.None$",
+ *       "!styles": ".*"
+ *     }
  *   },
- *   "NgModule": "^(?!\\s*$).+"
+ *   "NgModule": {
+ *      "argument": 0,
+ *      "properties": "^(?!\\s*$).+"
+ *    }
  * }, "src/material"]
  * ```
  */
@@ -24,15 +30,32 @@ export class Rule extends Lint.Rules.AbstractRule {
   }
 }
 
+/**
+ * Token used to indicate that all properties of an object
+ * should be linted against a single pattern.
+ */
+const ALL_PROPS_TOKEN = '*';
+
+/** Object that can be used to configured the rule. */
+interface RuleConfig {
+  [key: string]: {
+    argument: number,
+    required?: boolean,
+    properties: {[key: string]: string}
+  };
+}
+
 /** Represents a set of required and forbidden decorator properties. */
 type DecoratorRuleSet = {
-  required:  {[key: string]: RegExp},
-  forbidden:  {[key: string]: RegExp},
+  argument: number,
+  required: boolean,
+  requiredProps: {[key: string]: RegExp},
+  forbiddenProps: {[key: string]: RegExp},
 };
 
 /** Represents a map between decorator names and rule sets. */
 type DecoratorRules = {
-  [decorator: string]: DecoratorRuleSet | RegExp
+  [decorator: string]: DecoratorRuleSet
 };
 
 class Walker extends Lint.RuleWalker {
@@ -57,8 +80,16 @@ class Walker extends Lint.RuleWalker {
   }
 
   visitClassDeclaration(node: ts.ClassDeclaration) {
-    if (this._enabled && node.decorators) {
-      node.decorators.forEach(decorator => this._validatedDecorator(decorator.expression));
+    if (this._enabled) {
+      if (node.decorators) {
+        node.decorators.forEach(decorator => this._validateDecorator(decorator));
+      }
+
+      node.members.forEach(member => {
+        if (member.decorators) {
+          member.decorators.forEach(decorator => this._validateDecorator(decorator));
+        }
+      });
     }
 
     super.visitClassDeclaration(node);
@@ -68,63 +99,76 @@ class Walker extends Lint.RuleWalker {
    * Validates that a decorator matches all of the defined rules.
    * @param decorator Decorator to be checked.
    */
-  private _validatedDecorator(decorator: any) {
+  private _validateDecorator(decorator: ts.Decorator) {
+    const expression = decorator.expression;
+
+    if (!expression || !ts.isCallExpression(expression)) {
+      return;
+    }
+
     // Get the rules that are relevant for the current decorator.
-    const rules = this._rules[decorator.expression.getText()];
+    const rules = this._rules[expression.expression.getText()];
+    const args = expression.arguments;
 
     // Don't do anything if there are no rules.
     if (!rules) {
       return;
     }
 
-    // If the rule is a regex, extract the arguments as a string and run it against them.
-    if (rules instanceof RegExp) {
-      const decoratorText: string = decorator.getText();
-      const openParenthesisIndex = decoratorText.indexOf('(');
-      const closeParenthesisIndex = decoratorText.lastIndexOf(')');
-      const argumentsText = openParenthesisIndex > -1 ? decoratorText.substring(
-        openParenthesisIndex + 1,
-        closeParenthesisIndex > -1 ? closeParenthesisIndex : decoratorText.length) : '';
-
-      if (!rules.test(argumentsText)) {
-        this.addFailureAtNode(decorator.parent, `Expected decorator arguments to match "${rules}"`);
-      }
+    const allPropsRequirement = rules.requiredProps[ALL_PROPS_TOKEN];
 
+    // If we have a rule that applies to all properties, we just run it through once and we exit.
+    if (allPropsRequirement) {
+      const argumentText = args[rules.argument] ? args[rules.argument].getText() : '';
+      if (!allPropsRequirement.test(argumentText)) {
+        this.addFailureAtNode(expression.parent, `Expected decorator argument ${rules.argument} ` +
+                                                 `to match "${allPropsRequirement}"`);
+      }
       return;
     }
 
-    const args = decorator.arguments;
+    if (!args[rules.argument]) {
+      if (rules.required) {
+        this.addFailureAtNode(expression.parent,
+                             `Missing required argument at index ${rules.argument}`);
+      }
+      return;
+    }
 
-    if (!args || !args.length || !args[0].properties) {
+    if (!ts.isObjectLiteralExpression(args[rules.argument])) {
       return;
     }
 
     // Extract the property names and values.
-    const props = args[0].properties
-      .filter((node: ts.PropertyAssignment) => node.name && node.initializer)
-      .map((node: ts.PropertyAssignment) => ({
-        name: node.name.getText(),
-        value: node.initializer.getText(),
-        node
-      }));
+    const props: {name: string, value: string, node: ts.PropertyAssignment}[] = [];
+
+    (args[rules.argument] as ts.ObjectLiteralExpression).properties.forEach(prop => {
+      if (ts.isPropertyAssignment(prop) && prop.name && prop.initializer) {
+        props.push({
+          name: prop.name.getText(),
+          value: prop.initializer.getText(),
+          node: prop
+        });
+      }
+    });
 
     // Find all of the required rule properties that are missing from the decorator.
-    const missing = Object.keys(rules.required)
-        .filter(key => !props.find((prop: any) => prop.name === key));
+    const missing = Object.keys(rules.requiredProps)
+        .filter(key => !props.find(prop => prop.name === key));
 
     if (missing.length) {
       // Exit early if any of the properties are missing.
-      this.addFailureAtNode(decorator.expression,
+      this.addFailureAtNode(expression.expression,
           'Missing required properties: ' + missing.join(', '));
     } else {
       // If all the necessary properties are defined, ensure that
       // they match the pattern and aren't in the forbidden list.
       props
-        .filter((prop: any) => rules.required[prop.name] || rules.forbidden[prop.name])
-        .forEach((prop: any) => {
+        .filter(prop => rules.requiredProps[prop.name] || rules.forbiddenProps[prop.name])
+        .forEach(prop => {
           const {name, value, node} = prop;
-          const requiredPattern = rules.required[name];
-          const forbiddenPattern = rules.forbidden[name];
+          const requiredPattern = rules.requiredProps[name];
+          const forbiddenPattern = rules.forbiddenProps[name];
 
           if (requiredPattern && !requiredPattern.test(value)) {
             this.addFailureAtNode(node, `Invalid value for property. ` +
@@ -143,33 +187,45 @@ class Walker extends Lint.RuleWalker {
    * @param config Config object passed in via the tslint.json.
    * @returns Sanitized rules.
    */
-  private _generateRules(config: {[key: string]: string|{[key: string]: string}}): DecoratorRules {
+  private _generateRules(config: RuleConfig|null): DecoratorRules {
     const output: DecoratorRules = {};
 
     if (config) {
-      Object.keys(config)
-        .filter(decoratorName => Object.keys(config[decoratorName]).length > 0)
-        .forEach(decoratorName => {
-          const decoratorConfig = config[decoratorName];
-
-          if (typeof decoratorConfig === 'string') {
-            output[decoratorName] = new RegExp(decoratorConfig);
-          } else {
-            output[decoratorName] = Object.keys(decoratorConfig).reduce((rules, prop) => {
-              const isForbidden = prop.startsWith('!');
-              const cleanName = isForbidden ? prop.slice(1) : prop;
-              const pattern = new RegExp(decoratorConfig[prop]);
-
-              if (isForbidden) {
-                rules.forbidden[cleanName] = pattern;
-              } else {
-                rules.required[cleanName] = pattern;
-              }
-
-              return rules;
-            }, {required: {}, forbidden: {}} as DecoratorRuleSet);
-          }
-        });
+      Object.keys(config).forEach(decoratorName => {
+        const decoratorConfig = config[decoratorName];
+        const {argument, properties, required} = decoratorConfig;
+
+        // * is a special token which means to run the pattern across the entire object.
+        const allProperties = properties[ALL_PROPS_TOKEN];
+
+        if (allProperties) {
+          output[decoratorName] = {
+            argument,
+            required: !!required,
+            requiredProps: {[ALL_PROPS_TOKEN]: new RegExp(allProperties)},
+            forbiddenProps: {}
+          };
+        } else {
+          output[decoratorName] = Object.keys(decoratorConfig.properties).reduce((rules, prop) => {
+            const isForbidden = prop.startsWith('!');
+            const cleanName = isForbidden ? prop.slice(1) : prop;
+            const pattern = new RegExp(properties[prop]);
+
+            if (isForbidden) {
+              rules.forbiddenProps[cleanName] = pattern;
+            } else {
+              rules.requiredProps[cleanName] = pattern;
+            }
+
+            return rules;
+          }, {
+            argument,
+            required: !!required,
+            requiredProps: {} as {[key: string]: RegExp},
+            forbiddenProps: {} as {[key: string]: RegExp}
+          });
+        }
+      });
     }
 
     return output;
diff --git a/tslint.json b/tslint.json
@@ -133,17 +133,28 @@
     }],
     "validate-decorators": [true, {
       "Component": {
-        "!host": "\\[class\\]",
-        "!preserveWhitespaces": ".*",
-        "!styles": ".*",
-        "!moduleId": ".*",
-        "changeDetection": "\\.OnPush$",
-        "encapsulation": "\\.None$"
+        "argument": 0,
+        "properties": {
+          "!host": "\\[class\\]",
+          "!preserveWhitespaces": ".*",
+          "!styles": ".*",
+          "!moduleId": ".*",
+          "changeDetection": "\\.OnPush$",
+          "encapsulation": "\\.None$"
+        }
       },
       "Directive": {
-        "!host": "\\[class\\]"
+        "argument": 0,
+        "properties": {
+          "!host": "\\[class\\]"
+        }
       },
-      "NgModule": "^(?!\\s*$).+"
+      "NgModule": {
+        "argument": 0,
+        "properties": {
+          "*": "^(?!\\s*$).+"
+        }
+      }
     }, "src/!(a11y-demo|e2e-app|components-examples|universal-app|dev-app)/**/!(*.spec).ts"],
     "require-license-banner": [
       true,
