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 cb090f5593..e028c322e9 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 @@ -9,6 +9,8 @@ import * as ts from 'typescript'; import {isFunctionLikeDeclaration, unwrapExpression} from '../../../utils/typescript/functions'; +type FunctionContext = Map; + /** * 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(); + private visitedJumpExprNodes = new Set(); - /** Set of visited accessor nodes that caused a jump in control flow. */ - private visitedAccessorNodes = new Set(); + /** Queue of nodes that need to be checked for declaration usage. */ + 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 = callExprType.getSymbol(); + const callExprSymbol = this._getDeclarationSymbolOfNode(node); - if (!callExprSymbol || !callExprSymbol.valueDeclaration || - !isFunctionLikeDeclaration(callExprSymbol.valueDeclaration)) { + if (!callExprSymbol || !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)) { - this.visitedJumpExprSymbols.add(callExprSymbol); - nodeQueue.push(expressionDecl.body); + if (!isFunctionLikeDeclaration(expressionDecl) || + this.visitedJumpExprNodes.has(expressionDecl) || !expressionDecl.body) { + return; } + + // Update the context for the new jump expression and its specified arguments. + this._updateContext(callExpression.arguments, expressionDecl.parameters); + + this.visitedJumpExprNodes.add(expressionDecl); + this.nodeQueue.push(expressionDecl.body); } - private addNewExpressionToQueue(node: ts.NewExpression, nodeQueue: ts.Node[]) { - const newExprSymbol = this.typeChecker.getSymbolAtLocation(unwrapExpression(node.expression)); + 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) || - this.visitedJumpExprSymbols.has(newExprSymbol)) { + !ts.isClassDeclaration(newExprSymbol.valueDeclaration)) { return; } const targetConstructor = newExprSymbol.valueDeclaration.members.find(ts.isConstructorDeclaration); - if (targetConstructor && targetConstructor.body) { - this.visitedJumpExprSymbols.add(newExprSymbol); - nodeQueue.push(targetConstructor.body); + if (targetConstructor && targetConstructor.body && + !this.visitedJumpExprNodes.has(targetConstructor)) { + // 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, - checkGetter: boolean) { + node: ts.PropertyAccessExpression, checkSetter: 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); - nodeQueue.push(d.body !); + this.visitedJumpExprNodes.add(d); + 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.visitedJumpExprSymbols.clear(); + this.visitedJumpExprNodes.clear(); + this.context.clear(); + this.nodeQueue = [searchNode]; - while (nodeQueue.length) { - const node = nodeQueue.shift() !; + while (this.nodeQueue.length) { + 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)) { - nodeQueue.push(node.right); + if (this.visitBinaryExpression(node)) { + 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, parameters: ts.NodeArray) { + 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; + } } diff --git a/packages/core/schematics/test/static_queries_migration_spec.ts b/packages/core/schematics/test/static_queries_migration_spec.ts index cf8a860b0a..3a6a0a3a4c 100644 --- a/packages/core/schematics/test/static_queries_migration_spec.ts +++ b/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: ''}) + 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: ''}) + 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: ''}) + 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';