diff --git a/packages/core/schematics/migrations/static-queries/angular/analyze_query_usage.ts b/packages/core/schematics/migrations/static-queries/angular/analyze_query_usage.ts index a6debd1c53..0635db7073 100644 --- a/packages/core/schematics/migrations/static-queries/angular/analyze_query_usage.ts +++ b/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; } diff --git a/packages/core/schematics/migrations/static-queries/angular/declaration_usage_visitor.ts b/packages/core/schematics/migrations/static-queries/angular/declaration_usage_visitor.ts index db6a7c7ac5..1a5a3732a6 100644 --- a/packages/core/schematics/migrations/static-queries/angular/declaration_usage_visitor.ts +++ b/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; } diff --git a/packages/core/schematics/migrations/static-queries/typescript/functions.ts b/packages/core/schematics/migrations/static-queries/typescript/functions.ts new file mode 100644 index 0000000000..3e32af4132 --- /dev/null +++ b/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; + } +} diff --git a/packages/core/schematics/test/static_queries_migration_spec.ts b/packages/core/schematics/test/static_queries_migration_spec.ts index 6ed28cbf39..8bd207ceed 100644 --- a/packages/core/schematics/test/static_queries_migration_spec.ts +++ b/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: ''}) + 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: ''}) + 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: ''}) + 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: ''}) + 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: ''}) + 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: ''}) + 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;`); + }); } });