build: ngOnChanges property access rule not working (#21075)

Follow-up to #21074. We have a lint rule that flags property accesses like `changes.foo`
where `changes` is a `SimpleChanges` object, because they'll break under Closure
compiler. The rule depends on the TS type checker to identify the objects, but it
seems like somewhere along the way, tslint changed how they construct the
`Program` which removed the necessary type information.

These changes add a less complicated fallback which looks at whether the name
of the receiver of a property access is the same as the name of the first method
parameter.

Author: crisbeto

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';
+  }
 }