build: ngOnChanges property access rule not working

tools/tslint-rules/ngOnChangesPropertyAccessRule.ts

@@ -1,6 +1,5 @@
 import * as ts from 'typescript';
 import * as Lint from 'tslint';
-import * as tsutils from 'tsutils';
 
 /**
  * Rule that catches cases where a property of a `SimpleChanges` object is accessed directly,
@@ -24,30 +23,45 @@ class Walker extends Lint.ProgramAwareRuleWalker {
       // Walk through all the nodes and look for property access expressions
       // (e.g. `changes.something`). Note that this is different from element access
       // expressions which look like `changes['something']`.
-      if (tsutils.isPropertyAccessExpression(node)) {
-        const symbol = this.getTypeChecker().getTypeAtLocation(node.expression).symbol;
-
-        // Add a failure if we're trying to access a property on a SimpleChanges object
-        // directly, because it can cause issues with Closure's property renaming.
-        if (symbol && symbol.name === 'SimpleChanges') {
-          const expressionName = node.expression.getText();
-          const propName = node.name.getText();
-
-          this.addFailureAtNode(node, 'Accessing properties of SimpleChanges objects directly ' +
-                                      'is not allowed. Use index access instead (e.g. ' +
-                                      `${expressionName}.${propName} should be ` +
-                                      `${expressionName}['${propName}']).`);
-        }
+      if (ts.isPropertyAccessExpression(node) && this._isSimpleChangesAccess(method, node)) {
+        const expressionName = node.expression.getText();
+        const propName = node.name.getText();
+
+        this.addFailureAtNode(node, 'Accessing properties of SimpleChanges objects directly ' +
+                                    'is not allowed. Use index access instead (e.g. ' +
+                                    `${expressionName}.${propName} should be ` +
+                                    `${expressionName}['${propName}']).`);
       }
 
-      // Don't walk the property accesses inside of call expressions. This prevents us
-      // from flagging cases like `changes.hasOwnProperty('something')` incorrectly.
-      if (!tsutils.isCallExpression(node)) {
+      // Don't walk calls to `hasOwnProperty` since they can be used for null checking.
+      if (!ts.isCallExpression(node) || !ts.isPropertyAccessExpression(node.expression) ||
+        !ts.isIdentifier(node.expression.name) || node.expression.name.text !== 'hasOwnProperty') {
         node.forEachChild(walkChildren);
       }
     };
 
     method.body.forEachChild(walkChildren);
     super.visitMethodDeclaration(method);
   }
+
+  /** Checks whether a property access is operating on a `SimpleChanges` object. */
+  private _isSimpleChangesAccess(method: ts.MethodDeclaration, node: ts.PropertyAccessExpression) {
+    const changesParam = method.parameters[0];
+    const changesName = changesParam && ts.isParameter(changesParam) &&
+        ts.isIdentifier(changesParam.name) ? changesParam.name.text : null;
+    const receiverName = ts.isIdentifier(node.expression) ? node.expression.text : null;
+
+    // Try to resolve based on the name. This should be quicker and more robust since it doesn't
+    // require the type checker to be present and to have been configured correctly. Note that
+    // we filter out property accesses inside of other property accesses since we only want to
+    // look at top-level ones so that we don't flag something like `foo.bar.changes`.
+    if (changesName && receiverName && changesName === receiverName &&
+        !ts.isPropertyAccessExpression(node.parent)) {
+      return true;
+    }
+
+    // Fall back to trying to resolve using the type checker.
+    const symbol = this.getTypeChecker().getTypeAtLocation(node.expression).symbol;
+    return symbol != null && symbol.name === 'SimpleChanges';
+  }
 }