refactor(core): static query schematic should handle asynchronous query usages properly (angular#29133)

With 6215799, we introduced a schematic
for the Angular core package that automatically migrates unexplicit
query definitions to the explicit query timing (static <-> dynamic).

As the initial foundation was already big enough, it was planned
to come up with a follow-up that handles asynchronous query
usages properly. e.g. queries could be used in Promises,
`setTimeout`, `setInterval`, `requestAnimationFrame` and more, but
the schematic would incorrectly declare these queries as static.

This commit ensures that we properly handle these micro/macro
tasks and don't incorrectly consider queries as static.

The declaration usage visitor should only check the synchronous
control flow and completely ignore any statements within function
like expressions which aren't explicitly executed in a synchronous
way. e.g. IIFE's still work as the function expression is
synchronously invoked.

PR Close angular#29133

Author: devversion

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

@@ -53,22 +53,25 @@ function isQueryUsedStatically(
   // access it statically. e.g.
   //  (1) queries used in the "ngOnInit" lifecycle hook are static.
   //  (2) inputs with setters can access queries statically.
-  const possibleStaticQueryNodes: ts.Node[] = classDecl.members.filter(m => {
-    if (ts.isMethodDeclaration(m) && hasPropertyNameText(m.name) &&
-        STATIC_QUERY_LIFECYCLE_HOOKS[query.type].indexOf(m.name.text) !== -1) {
-      return true;
-    } else if (
-        knownInputNames && ts.isSetAccessor(m) && hasPropertyNameText(m.name) &&
-        knownInputNames.indexOf(m.name.text) !== -1) {
-      return true;
-    }
-    return false;
-  });
+  const possibleStaticQueryNodes: ts.Node[] =
+      classDecl.members
+          .filter(m => {
+            if (ts.isMethodDeclaration(m) && m.body && hasPropertyNameText(m.name) &&
+                STATIC_QUERY_LIFECYCLE_HOOKS[query.type].indexOf(m.name.text) !== -1) {
+              return true;
+            } else if (
+                knownInputNames && ts.isSetAccessor(m) && m.body && hasPropertyNameText(m.name) &&
+                knownInputNames.indexOf(m.name.text) !== -1) {
+              return true;
+            }
+            return false;
+          })
+          .map((member: ts.SetAccessorDeclaration | ts.MethodDeclaration) => member.body !);
 
   // In case nodes that can possibly access a query statically have been found, check
-  // if the query declaration is used within any of these nodes.
+  // if the query declaration is synchronously used within any of these nodes.
   if (possibleStaticQueryNodes.length &&
-      possibleStaticQueryNodes.some(hookNode => usageVisitor.isUsedInNode(hookNode))) {
+      possibleStaticQueryNodes.some(n => usageVisitor.isSynchronouslyUsedInNode(n))) {
     return true;
   }
 

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

@@ -7,6 +7,7 @@
  */
 
 import * as ts from 'typescript';
+import {isFunctionLikeDeclaration, unwrapExpression} from '../typescript/functions';
 
 /**
  * Class that can be used to determine if a given TypeScript node is used within
@@ -26,19 +27,33 @@ export class DeclarationUsageVisitor {
   }
 
   private addJumpExpressionToQueue(node: ts.Expression, nodeQueue: ts.Node[]) {
+    // 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);
+      return;
+    }
+
     const callExprSymbol = this.typeChecker.getSymbolAtLocation(node);
 
-    // Note that we should not add previously visited symbols to the queue as this
-    // could cause cycles.
-    if (callExprSymbol && callExprSymbol.valueDeclaration &&
-        !this.visitedJumpExprSymbols.has(callExprSymbol)) {
+    if (!callExprSymbol || !callExprSymbol.valueDeclaration ||
+        !isFunctionLikeDeclaration(callExprSymbol.valueDeclaration)) {
+      return;
+    }
+
+    const expressionDecl = 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)) {
       this.visitedJumpExprSymbols.add(callExprSymbol);
-      nodeQueue.push(callExprSymbol.valueDeclaration);
+      nodeQueue.push(expressionDecl.body);
     }
   }
 
   private addNewExpressionToQueue(node: ts.NewExpression, nodeQueue: ts.Node[]) {
-    const newExprSymbol = this.typeChecker.getSymbolAtLocation(node.expression);
+    const newExprSymbol = this.typeChecker.getSymbolAtLocation(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
@@ -50,15 +65,15 @@ export class DeclarationUsageVisitor {
     }
 
     const targetConstructor =
-        newExprSymbol.valueDeclaration.members.find(d => ts.isConstructorDeclaration(d));
+        newExprSymbol.valueDeclaration.members.find(ts.isConstructorDeclaration);
 
-    if (targetConstructor) {
+    if (targetConstructor && targetConstructor.body) {
       this.visitedJumpExprSymbols.add(newExprSymbol);
-      nodeQueue.push(targetConstructor);
+      nodeQueue.push(targetConstructor.body);
     }
   }
 
-  isUsedInNode(searchNode: ts.Node): boolean {
+  isSynchronouslyUsedInNode(searchNode: ts.Node): boolean {
     const nodeQueue: ts.Node[] = [searchNode];
     this.visitedJumpExprSymbols.clear();
 
@@ -72,7 +87,7 @@ 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(node.expression, nodeQueue);
+        this.addJumpExpressionToQueue(unwrapExpression(node.expression), nodeQueue);
       }
 
       // Handle new expressions that cause a jump in control flow. We resolve the
@@ -81,7 +96,12 @@ export class DeclarationUsageVisitor {
         this.addNewExpressionToQueue(node, nodeQueue);
       }
 
-      nodeQueue.push(...node.getChildren());
+      // 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());
+      }
     }
     return false;
   }

packages/core/schematics/migrations/static-queries/typescript/functions.ts

@@ -0,0 +1,29 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import * as ts from 'typescript';
+
+/** Checks whether a given node is a function like declaration. */
+export function isFunctionLikeDeclaration(node: ts.Node): node is ts.FunctionLikeDeclaration {
+  return ts.isFunctionDeclaration(node) || ts.isMethodDeclaration(node) ||
+      ts.isArrowFunction(node) || ts.isFunctionExpression(node) ||
+      ts.isGetAccessorDeclaration(node) || ts.isSetAccessorDeclaration(node);
+}
+
+/**
+ * Unwraps a given expression TypeScript node. Expressions can be wrapped within multiple
+ * parentheses. e.g. "(((({exp}))))()". The function should return the TypeScript node
+ * referring to the inner expression. e.g "exp".
+ */
+export function unwrapExpression(node: ts.Expression | ts.ParenthesizedExpression): ts.Expression {
+  if (ts.isParenthesizedExpression(node)) {
+    return unwrapExpression(node.expression);
+  } else {
+    return node;
+  }
+}

packages/core/schematics/test/static_queries_migration_spec.ts

@@ -287,6 +287,16 @@ describe('static-queries migration', () => {
           
           ngOnInit() {
             new A(this);
+            
+            new class Inline {
+              constructor(private ctx: MyComp) {
+                this.a();
+              }
+              
+              a() {
+                this.ctx.query2.useStatically();
+              }
+            }(this);
           }
         }
         
@@ -302,7 +312,33 @@ describe('static-queries migration', () => {
       expect(tree.readContent('/index.ts'))
           .toContain(`@${queryType}('test', { static: true }) query: any;`);
       expect(tree.readContent('/index.ts'))
-          .toContain(`@${queryType}('test', { static: false }) query2: any;`);
+          .toContain(`@${queryType}('test', { static: true }) query2: any;`);
+    });
+
+    it('should detect queries used in parenthesized new expressions', () => {
+      writeFile('/index.ts', `
+        import {Component, ${queryType}} from '@angular/core';
+        
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          @${queryType}('test') query: any;
+          
+          ngOnInit() {
+            new ((A))(this);
+          }
+        }
+        
+        export class A {
+          constructor(ctx: MyComp) {
+            ctx.query.test();
+          }
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: true }) query: any;`);
     });
 
     it('should detect queries in lifecycle hook with string literal name', () => {
@@ -520,5 +556,159 @@ describe('static-queries migration', () => {
       expect(tree.readContent('/src/index.ts'))
           .toContain(`@${queryType}('test', { static: true }) query: any;`);
     });
+
+    it('should not mark queries used in promises as static', () => {
+      writeFile('/index.ts', `
+        import {Component, ${queryType}} from '@angular/core';
+                        
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          private @${queryType}('test') query: any;
+          private @${queryType}('test') query2: any;
+        
+          ngOnInit() {
+            const a = Promise.resolve();
+          
+            Promise.resolve().then(() => {
+              this.query.doSomething();
+            });
+            
+            Promise.reject().catch(() => {
+              this.query.doSomething();
+            });
+            
+            a.then(() => {}).then(() => {
+              this.query.doSomething();
+            });
+                        
+            Promise.resolve().then(this.createPromiseCb());
+          }
+          
+          createPromiseCb() {
+            this.query2.doSomething();
+            return () => { /* empty callback */}
+          }
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: false }) query: any;`);
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: true }) query2: any;`);
+    });
+
+    it('should not mark queries used in setTimeout as static', () => {
+      writeFile('/index.ts', `
+        import {Component, ${queryType}} from '@angular/core';
+                                
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          private @${queryType}('test') query: any;
+          private @${queryType}('test') query2: any;
+          private @${queryType}('test') query3: any;
+        
+          ngOnInit() {
+            setTimeout(function() {
+              this.query.doSomething();
+            });
+            
+            setTimeout(createCallback(this));
+          }
+        }
+        
+        function createCallback(instance: MyComp) {
+          instance.query2.doSomething();
+          return () => instance.query3.doSomething();
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: false }) query: any;`);
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: true }) query2: any;`);
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: false }) query3: any;`);
+    });
+
+    it('should not mark queries used in "addEventListener" as static', () => {
+      writeFile('/index.ts', `
+        import {Component, ElementRef, ${queryType}} from '@angular/core';
+                        
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          private @${queryType}('test') query: any;
+        
+          constructor(private elementRef: ElementRef) {}
+        
+          ngOnInit() {
+            this.elementRef.addEventListener(() => {
+              this.query.classList.add('test');
+            });
+          }
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: false }) query: any;`);
+    });
+
+    it('should not mark queries used in "requestAnimationFrame" as static', () => {
+      writeFile('/index.ts', `
+        import {Component, ElementRef, ${queryType}} from '@angular/core';
+                        
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          private @${queryType}('test') query: any;
+        
+          constructor(private elementRef: ElementRef) {}
+        
+          ngOnInit() {
+            requestAnimationFrame(() => {
+              this.query.classList.add('test');
+            });
+          }
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: false }) query: any;`);
+    });
+
+    it('should mark queries used in immediately-invoked function expression as static', () => {
+      writeFile('/index.ts', `
+        import {Component, ${queryType}} from '@angular/core';
+                        
+        @Component({template: '<span #test></span>'})
+        export class MyComp {
+          private @${queryType}('test') query: any;
+          private @${queryType}('test') query2: any;
+                
+          ngOnInit() {
+            (() => {
+              this.query.usedStatically();
+            })();
+            
+            (function(ctx) {
+              ctx.query2.useStatically();
+            })(this);
+          }
+        }
+      `);
+
+      runMigration();
+
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: true }) query: any;`);
+      expect(tree.readContent('/index.ts'))
+          .toContain(`@${queryType}('test', { static: true }) query2: any;`);
+    });
   }
 });