refactor(core): static-query schematic should handle function callbacks (#29663)

Currently the static-query schematic is not able to properly handle
call expressions that pass function declarations that access a given
query. e.g.

```ts
ngOnInit() {
  this._callFunction(() => this.myQuery.doSomething());
}

_callFunction(cb: any) { cb(); }
```

In that case the passed function is executed synchronously in
the "ngOnInit" lifecycle and therefore the query needs to be
detected as "static".

We can fix this by keeping track of the current function context
and using it to resolve identifiers to the passed arguments.

PR Close #29663

packages/core/schematics/migrations/static-queries/angular/declaration_usage_visitor.ts

@@ -9,6 +9,8 @@
 import * as ts from 'typescript';
 import {isFunctionLikeDeclaration, unwrapExpression} from '../../../utils/typescript/functions';
 
+type FunctionContext = Map<ts.ParameterDeclaration, ts.Node>;
+
 /**
  * List of TypeScript syntax tokens that can be used within a binary expression as
  * compound assignment. These imply a read and write of the left-side expression.
@@ -32,10 +34,16 @@ const BINARY_COMPOUND_TOKENS = [
  */
 export class DeclarationUsageVisitor {
   /** Set of visited symbols that caused a jump in control flow. */
-  private visitedJumpExprSymbols = new Set<ts.Symbol>();
+  private visitedJumpExprNodes = new Set<ts.Node>();
 
-  /** Set of visited accessor nodes that caused a jump in control flow. */
+  /** Queue of nodes that need to be checked for declaration usage. */
-  private visitedAccessorNodes = new Set<ts.AccessorDeclaration>();
+  private nodeQueue: ts.Node[] = [];
+
+  /**
+   * Function context that holds the TypeScript node values for all parameters
+   * of the currently analyzed function block.
+   */
+  private context: FunctionContext = new Map();
 
   constructor(private declaration: ts.Node, private typeChecker: ts.TypeChecker) {}
 
@@ -44,57 +52,68 @@ export class DeclarationUsageVisitor {
     return !!symbol && symbol.valueDeclaration === this.declaration;
   }
 
-  private addJumpExpressionToQueue(node: ts.Expression, nodeQueue: ts.Node[]) {
+  private addJumpExpressionToQueue(callExpression: ts.CallExpression) {
+    const node = unwrapExpression(callExpression.expression);
+
     // In case the given expression is already referring to a function-like declaration,
     // we don't need to resolve the symbol of the expression as the jump expression is
     // defined inline and we can just add the given node to the queue.
     if (isFunctionLikeDeclaration(node) && node.body) {
-      nodeQueue.push(node.body);
+      this.nodeQueue.push(node.body);
       return;
     }
 
-    const callExprType = this.typeChecker.getTypeAtLocation(node);
+    const callExprSymbol = this._getDeclarationSymbolOfNode(node);
-    const callExprSymbol = callExprType.getSymbol();
 
-    if (!callExprSymbol || !callExprSymbol.valueDeclaration ||
+    if (!callExprSymbol || !callExprSymbol.valueDeclaration) {
-        !isFunctionLikeDeclaration(callExprSymbol.valueDeclaration)) {
       return;
     }
 
-    const expressionDecl = callExprSymbol.valueDeclaration;
+    const expressionDecl = this._resolveNodeFromContext(callExprSymbol.valueDeclaration);
 
     // Note that we should not add previously visited symbols to the queue as
     // this could cause cycles.
-    if (expressionDecl.body && !this.visitedJumpExprSymbols.has(callExprSymbol)) {
+    if (!isFunctionLikeDeclaration(expressionDecl) ||
-      this.visitedJumpExprSymbols.add(callExprSymbol);
+        this.visitedJumpExprNodes.has(expressionDecl) || !expressionDecl.body) {
-      nodeQueue.push(expressionDecl.body);
+      return;
-    }
     }
 
-  private addNewExpressionToQueue(node: ts.NewExpression, nodeQueue: ts.Node[]) {
+    // Update the context for the new jump expression and its specified arguments.
-    const newExprSymbol = this.typeChecker.getSymbolAtLocation(unwrapExpression(node.expression));
+    this._updateContext(callExpression.arguments, expressionDecl.parameters);
+
+    this.visitedJumpExprNodes.add(expressionDecl);
+    this.nodeQueue.push(expressionDecl.body);
+  }
+
+  private addNewExpressionToQueue(node: ts.NewExpression) {
+    const newExprSymbol = this._getDeclarationSymbolOfNode(unwrapExpression(node.expression));
 
     // Only handle new expressions which resolve to classes. Technically "new" could
     // also call void functions or objects with a constructor signature. Also note that
     // we should not visit already visited symbols as this could cause cycles.
     if (!newExprSymbol || !newExprSymbol.valueDeclaration ||
-        !ts.isClassDeclaration(newExprSymbol.valueDeclaration) ||
+        !ts.isClassDeclaration(newExprSymbol.valueDeclaration)) {
-        this.visitedJumpExprSymbols.has(newExprSymbol)) {
       return;
     }
 
     const targetConstructor =
         newExprSymbol.valueDeclaration.members.find(ts.isConstructorDeclaration);
 
-    if (targetConstructor && targetConstructor.body) {
+    if (targetConstructor && targetConstructor.body &&
-      this.visitedJumpExprSymbols.add(newExprSymbol);
+        !this.visitedJumpExprNodes.has(targetConstructor)) {
-      nodeQueue.push(targetConstructor.body);
+      // Update the context for the new expression and its specified constructor
+      // parameters if arguments are passed to the class constructor.
+      if (node.arguments) {
+        this._updateContext(node.arguments, targetConstructor.parameters);
+      }
+
+      this.visitedJumpExprNodes.add(targetConstructor);
+      this.nodeQueue.push(targetConstructor.body);
     }
   }
 
   private visitPropertyAccessors(
-      node: ts.PropertyAccessExpression, nodeQueue: ts.Node[], checkSetter: boolean,
+      node: ts.PropertyAccessExpression, checkSetter: boolean, checkGetter: boolean) {
-      checkGetter: boolean) {
     const propertySymbol = this.typeChecker.getSymbolAtLocation(node.name);
 
     if (!propertySymbol || !propertySymbol.declarations.length ||
@@ -109,14 +128,14 @@ export class DeclarationUsageVisitor {
     accessors
         .filter(
             d => (checkSetter && ts.isSetAccessor(d) || checkGetter && ts.isGetAccessor(d)) &&
-                d.body && !this.visitedAccessorNodes.has(d))
+                d.body && !this.visitedJumpExprNodes.has(d))
         .forEach(d => {
-          this.visitedAccessorNodes.add(d);
+          this.visitedJumpExprNodes.add(d);
-          nodeQueue.push(d.body !);
+          this.nodeQueue.push(d.body !);
         });
   }
 
-  private visitBinaryExpression(node: ts.BinaryExpression, nodeQueue: ts.Node[]): boolean {
+  private visitBinaryExpression(node: ts.BinaryExpression): boolean {
     const leftExpr = unwrapExpression(node.left);
 
     if (!ts.isPropertyAccessExpression(leftExpr)) {
@@ -133,25 +152,26 @@ export class DeclarationUsageVisitor {
     if (BINARY_COMPOUND_TOKENS.indexOf(node.operatorToken.kind) !== -1) {
       // Compound assignments always cause the getter and setter to be called.
       // Therefore we need to check the setter and getter of the property access.
-      this.visitPropertyAccessors(leftExpr, nodeQueue, /* setter */ true, /* getter */ true);
+      this.visitPropertyAccessors(leftExpr, /* setter */ true, /* getter */ true);
     } else if (node.operatorToken.kind === ts.SyntaxKind.EqualsToken) {
       // Value assignments using the equals token only cause the "setter" to be called.
       // Therefore we need to analyze the setter declaration of the property access.
-      this.visitPropertyAccessors(leftExpr, nodeQueue, /* setter */ true, /* getter */ false);
+      this.visitPropertyAccessors(leftExpr, /* setter */ true, /* getter */ false);
     } else {
       // If the binary expression is not an assignment, it's a simple property read and
       // we need to check the getter declaration if present.
-      this.visitPropertyAccessors(leftExpr, nodeQueue, /* setter */ false, /* getter */ true);
+      this.visitPropertyAccessors(leftExpr, /* setter */ false, /* getter */ true);
     }
     return true;
   }
 
   isSynchronouslyUsedInNode(searchNode: ts.Node): boolean {
-    const nodeQueue: ts.Node[] = [searchNode];
+    this.visitedJumpExprNodes.clear();
-    this.visitedJumpExprSymbols.clear();
+    this.context.clear();
+    this.nodeQueue = [searchNode];
 
-    while (nodeQueue.length) {
+    while (this.nodeQueue.length) {
-      const node = nodeQueue.shift() !;
+      const node = this.nodeQueue.shift() !;
 
       if (ts.isIdentifier(node) && this.isReferringToSymbol(node)) {
         return true;
@@ -160,13 +180,13 @@ export class DeclarationUsageVisitor {
       // Handle call expressions within TypeScript nodes that cause a jump in control
       // flow. We resolve the call expression value declaration and add it to the node queue.
       if (ts.isCallExpression(node)) {
-        this.addJumpExpressionToQueue(unwrapExpression(node.expression), nodeQueue);
+        this.addJumpExpressionToQueue(node);
       }
 
       // Handle new expressions that cause a jump in control flow. We resolve the
       // constructor declaration of the target class and add it to the node queue.
       if (ts.isNewExpression(node)) {
-        this.addNewExpressionToQueue(node, nodeQueue);
+        this.addNewExpressionToQueue(node);
       }
 
       // We also need to handle binary expressions where a value can be either assigned to
@@ -177,8 +197,8 @@ export class DeclarationUsageVisitor {
         // don't want to continue visiting this property expression on its own. This is necessary
         // because visiting the expression on its own causes a loss of context. e.g. property
         // access expressions *do not* always cause a value read (e.g. property assignments)
-        if (this.visitBinaryExpression(node, nodeQueue)) {
+        if (this.visitBinaryExpression(node)) {
-          nodeQueue.push(node.right);
+          this.nodeQueue.push(node.right);
           continue;
         }
       }
@@ -187,16 +207,76 @@ export class DeclarationUsageVisitor {
       // expressions won't be added to the node queue, so these access expressions are
       // guaranteed to be "read" accesses and we need to check the "getter" declaration.
       if (ts.isPropertyAccessExpression(node)) {
-        this.visitPropertyAccessors(node, nodeQueue, /* setter */ false, /* getter */ true);
+        this.visitPropertyAccessors(node, /* setter */ false, /* getter */ true);
       }
 
       // Do not visit nodes that declare a block of statements but are not executed
       // synchronously (e.g. function declarations). We only want to check TypeScript
       // nodes which are synchronously executed in the control flow.
       if (!isFunctionLikeDeclaration(node)) {
-        nodeQueue.push(...node.getChildren());
+        this.nodeQueue.push(...node.getChildren());
       }
     }
     return false;
   }
+
+  /**
+   * Resolves a given node from the context. In case the node is not mapped in
+   * the context, the original node is returned.
+   */
+  private _resolveNodeFromContext(node: ts.Node): ts.Node {
+    if (ts.isParameter(node) && this.context.has(node)) {
+      return this.context.get(node) !;
+    }
+    return node;
+  }
+
+  /**
+   * Updates the context to reflect the newly set parameter values. This allows future
+   * references to function parameters to be resolved to the actual node through the context.
+   */
+  private _updateContext(
+      callArgs: ts.NodeArray<ts.Expression>, parameters: ts.NodeArray<ts.ParameterDeclaration>) {
+    parameters.forEach((parameter, index) => {
+      let argumentNode: ts.Node = callArgs[index];
+      if (ts.isIdentifier(argumentNode)) {
+        this.context.set(parameter, this._resolveIdentifier(argumentNode));
+      } else {
+        this.context.set(parameter, argumentNode);
+      }
+    });
+  }
+
+  /**
+   * Resolves a TypeScript identifier node. For example an identifier can refer to a
+   * function parameter which can be resolved through the function context.
+   */
+  private _resolveIdentifier(node: ts.Identifier): ts.Node {
+    const symbol = this._getDeclarationSymbolOfNode(node);
+
+    if (!symbol || !symbol.valueDeclaration) {
+      return node;
+    }
+
+    return this._resolveNodeFromContext(symbol.valueDeclaration);
+  }
+
+  /**
+   * Gets the declaration symbol of a given TypeScript node. Resolves aliased
+   * symbols to the symbol containing the value declaration.
+   */
+  private _getDeclarationSymbolOfNode(node: ts.Node): ts.Symbol|null {
+    let symbol = this.typeChecker.getSymbolAtLocation(node);
+
+    if (!symbol) {
+      return null;
+    }
+
+    // Resolve the symbol to it's original declaration symbol.
+    while (symbol.flags & ts.SymbolFlags.Alias) {
+      symbol = this.typeChecker.getAliasedSymbol(symbol);
+    }
+
+    return symbol;
+  }
 }

packages/core/schematics/test/static_queries_migration_spec.ts

@@ -639,6 +639,93 @@ describe('static-queries migration', () => {
           .toContain(`@${queryType}('test', { static: true }) query2: any;`);
     });
 
+    it('should handle function callbacks which statically access queries', () => {
+      writeFile('/index.ts', `
+        import {Component, ${queryType}} from '@angular/core';
+                        
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          private @${queryType}('test') query: any;
+        
+          ngOnInit() {        
+            this.callSync(() => this.query.doSomething());
+          }
+          
+          callSync(cb: Function) {
+            this.callSync2(cb);
+          }
+          
+          callSync2(cb: Function) {
+            cb();
+          }
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: true }) query: any;`);
+    });
+
+    it('should handle class instantiations with specified callbacks that access queries', () => {
+      writeFile('/index.ts', `
+        import {Component, ${queryType}} from '@angular/core';
+        import {External} from './external';
+                        
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          private @${queryType}('test') query: any;
+        
+          ngOnInit() {        
+            new External(() => this.query.doSomething());
+          }
+        }
+      `);
+
+      writeFile('/external.ts', `
+        export class External {
+          constructor(cb: () => void) {
+            // Add extra parentheses to ensure that expression is unwrapped. 
+            ((cb))();
+          }
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: true }) query: any;`);
+    });
+
+    it('should handle nested functions with arguments from parent closure', () => {
+      writeFile('/index.ts', `
+        import {Component, ${queryType}} from '@angular/core';
+                        
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          private @${queryType}('test') query: any;
+        
+          ngOnInit() {        
+            this.callSync(() => this.query.doSomething());
+          }
+          
+          callSync(cb: Function) {
+            function callSyncNested() {
+              // The "cb" identifier comes from the "callSync" function.
+              cb();
+            }
+            
+            callSyncNested();
+          }
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: true }) query: any;`);
+    });
+
     it('should not mark queries used in setTimeout as static', () => {
       writeFile('/index.ts', `
         import {Component, ${queryType}} from '@angular/core';