diff --git a/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/declaration_usage_visitor.ts b/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/declaration_usage_visitor.ts index 346a745f46..f42d647005 100644 --- a/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/declaration_usage_visitor.ts +++ b/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/declaration_usage_visitor.ts @@ -8,9 +8,16 @@ import * as ts from 'typescript'; import {isFunctionLikeDeclaration, unwrapExpression} from '../../../../utils/typescript/functions'; +import {getPropertyNameText} from '../../../../utils/typescript/property_name'; export type FunctionContext = Map; +export enum ResolvedUsage { + SYNCHRONOUS, + ASYNCHRONOUS, + AMBIGUOUS, +} + /** * 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. @@ -26,6 +33,19 @@ const BINARY_COMPOUND_TOKENS = [ ts.SyntaxKind.SlashEqualsToken, ]; +/** + * List of known asynchronous external call expressions which aren't analyzable + * but are guaranteed to not execute the passed argument synchronously. + */ +const ASYNC_EXTERNAL_CALLS = [ + {parent: ['Promise'], name: 'then'}, + {parent: ['Promise'], name: 'catch'}, + {parent: [null, 'Window'], name: 'requestAnimationFrame'}, + {parent: [null, 'Window'], name: 'setTimeout'}, + {parent: [null, 'Window'], name: 'setInterval'}, + {parent: ['*'], name: 'addEventListener'}, +]; + /** * Class that can be used to determine if a given TypeScript node is used within * other given TypeScript nodes. This is achieved by walking through all children @@ -36,9 +56,18 @@ export class DeclarationUsageVisitor { /** Set of visited symbols that caused a jump in control flow. */ private visitedJumpExprNodes = new Set(); - /** Queue of nodes that need to be checked for declaration usage. */ + /** + * Queue of nodes that need to be checked for declaration usage and + * are guaranteed to be executed synchronously. + */ private nodeQueue: ts.Node[] = []; + /** + * Nodes which need to be checked for declaration usage but aren't + * guaranteed to execute synchronously. + */ + private ambiguousNodeQueue: ts.Node[] = []; + /** * Function context that holds the TypeScript node values for all parameters * of the currently analyzed function block. @@ -68,6 +97,7 @@ export class DeclarationUsageVisitor { const callExprSymbol = this._getDeclarationSymbolOfNode(node); if (!callExprSymbol || !callExprSymbol.valueDeclaration) { + this.peekIntoJumpExpression(callExpression); return; } @@ -77,6 +107,7 @@ export class DeclarationUsageVisitor { // this could cause cycles. if (!isFunctionLikeDeclaration(expressionDecl) || this.visitedJumpExprNodes.has(expressionDecl) || !expressionDecl.body) { + this.peekIntoJumpExpression(callExpression); return; } @@ -95,6 +126,7 @@ export class DeclarationUsageVisitor { // we should not visit already visited symbols as this could cause cycles. if (!newExprSymbol || !newExprSymbol.valueDeclaration || !ts.isClassDeclaration(newExprSymbol.valueDeclaration)) { + this.peekIntoJumpExpression(node); return; } @@ -111,6 +143,8 @@ export class DeclarationUsageVisitor { this.visitedJumpExprNodes.add(targetConstructor); this.nodeQueue.push(targetConstructor.body); + } else { + this.peekIntoJumpExpression(node); } } @@ -160,7 +194,7 @@ export class DeclarationUsageVisitor { return true; } - isSynchronouslyUsedInNode(searchNode: ts.Node): boolean { + getResolvedNodeUsage(searchNode: ts.Node): ResolvedUsage { this.nodeQueue = [searchNode]; this.visitedJumpExprNodes.clear(); this.context.clear(); @@ -171,11 +205,17 @@ export class DeclarationUsageVisitor { // the derived class. this.baseContext.forEach((value, key) => this.context.set(key, value)); + return this.isSynchronouslyUsedInNode(searchNode); + } + + private isSynchronouslyUsedInNode(searchNode: ts.Node): ResolvedUsage { + this.ambiguousNodeQueue = []; + while (this.nodeQueue.length) { const node = this.nodeQueue.shift() !; if (ts.isIdentifier(node) && this.isReferringToSymbol(node)) { - return true; + return ResolvedUsage.SYNCHRONOUS; } // Handle call expressions within TypeScript nodes that cause a jump in control @@ -218,7 +258,65 @@ export class DeclarationUsageVisitor { this.nodeQueue.push(...node.getChildren()); } } - return false; + + if (this.ambiguousNodeQueue.length) { + // Update the node queue to all stored ambiguous nodes. These nodes are not + // guaranteed to be executed and therefore in case of a synchronous usage + // within one of those nodes, the resolved usage is ambiguous. + this.nodeQueue = this.ambiguousNodeQueue; + const usage = this.isSynchronouslyUsedInNode(searchNode); + return usage === ResolvedUsage.SYNCHRONOUS ? ResolvedUsage.AMBIGUOUS : usage; + } + return ResolvedUsage.ASYNCHRONOUS; + } + + /** + * Peeks into the given jump expression by adding all function like declarations + * which are referenced in the jump expression arguments to the ambiguous node + * queue. These arguments could technically access the given declaration but it's + * not guaranteed that the jump expression is executed. In that case the resolved + * usage is ambiguous. + */ + private peekIntoJumpExpression(jumpExp: ts.CallExpression|ts.NewExpression) { + if (!jumpExp.arguments) { + return; + } + + // For some call expressions we don't want to add the arguments to the + // ambiguous node queue. e.g. "setTimeout" is not analyzable but is + // guaranteed to execute its argument asynchronously. We handle a subset + // of these call expressions by having a hardcoded list of some. + if (ts.isCallExpression(jumpExp)) { + const symbol = this._getDeclarationSymbolOfNode(jumpExp.expression); + if (symbol && symbol.valueDeclaration) { + const parentNode = symbol.valueDeclaration.parent; + if (parentNode && (ts.isInterfaceDeclaration(parentNode) || ts.isSourceFile(parentNode)) && + (ts.isMethodSignature(symbol.valueDeclaration) || + ts.isFunctionDeclaration(symbol.valueDeclaration)) && + symbol.valueDeclaration.name) { + const parentName = ts.isInterfaceDeclaration(parentNode) ? parentNode.name.text : null; + const callName = getPropertyNameText(symbol.valueDeclaration.name); + if (ASYNC_EXTERNAL_CALLS.some( + c => + (c.name === callName && + (c.parent.indexOf(parentName) !== -1 || c.parent.indexOf('*') !== -1)))) { + return; + } + } + } + } + + jumpExp.arguments !.forEach((node: ts.Node) => { + node = this._resolveDeclarationOfNode(node); + + if (ts.isVariableDeclaration(node) && node.initializer) { + node = node.initializer; + } + + if (isFunctionLikeDeclaration(node) && !!node.body) { + this.ambiguousNodeQueue.push(node.body); + } + }); } /** @@ -252,7 +350,7 @@ export class DeclarationUsageVisitor { } if (ts.isIdentifier(argumentNode)) { - this.context.set(parameter, this._resolveIdentifier(argumentNode)); + this.context.set(parameter, this._resolveDeclarationOfNode(argumentNode)); } else { this.context.set(parameter, argumentNode); } @@ -260,10 +358,11 @@ export class DeclarationUsageVisitor { } /** - * Resolves a TypeScript identifier node. For example an identifier can refer to a - * function parameter which can be resolved through the function context. + * Resolves the declaration of a given TypeScript node. For example an identifier can + * refer to a function parameter. This parameter can then be resolved through the + * function context. */ - private _resolveIdentifier(node: ts.Identifier): ts.Node { + private _resolveDeclarationOfNode(node: ts.Node): ts.Node { const symbol = this._getDeclarationSymbolOfNode(node); if (!symbol || !symbol.valueDeclaration) { diff --git a/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts b/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts index 4ee07dc6f5..c8b6ac0f3b 100644 --- a/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts +++ b/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts @@ -14,7 +14,7 @@ import {ClassMetadataMap} from '../../angular/ng_query_visitor'; import {NgQueryDefinition, QueryTiming, QueryType} from '../../angular/query-definition'; import {TimingResult, TimingStrategy} from '../timing-strategy'; -import {DeclarationUsageVisitor, FunctionContext} from './declaration_usage_visitor'; +import {DeclarationUsageVisitor, FunctionContext, ResolvedUsage} from './declaration_usage_visitor'; import {updateSuperClassAbstractMembersContext} from './super_class_context'; import {TemplateUsageVisitor} from './template_usage_visitor'; @@ -48,96 +48,112 @@ export class QueryUsageStrategy implements TimingStrategy { return {timing: null, message: 'Queries defined on accessors cannot be analyzed.'}; } - return { - timing: - isQueryUsedStatically(query.container, query, this.classMetadata, this.typeChecker, []) ? - QueryTiming.STATIC : - QueryTiming.DYNAMIC - }; + const usage = this.analyzeQueryUsage(query.container, query, []); + + if (usage === ResolvedUsage.AMBIGUOUS) { + return { + timing: QueryTiming.STATIC, + message: 'Query timing is ambiguous. Please check if the query can be marked as dynamic.' + }; + } else if (usage === ResolvedUsage.SYNCHRONOUS) { + return {timing: QueryTiming.STATIC}; + } else { + return {timing: QueryTiming.DYNAMIC}; + } + } + + /** + * Checks whether a given query is used statically within the given class, its super + * class or derived classes. + */ + private analyzeQueryUsage( + classDecl: ts.ClassDeclaration, query: NgQueryDefinition, knownInputNames: string[], + functionCtx: FunctionContext = new Map(), visitInheritedClasses = true): ResolvedUsage { + const usageVisitor = + new DeclarationUsageVisitor(query.property !, this.typeChecker, functionCtx); + const classMetadata = this.classMetadata.get(classDecl); + let usage: ResolvedUsage = ResolvedUsage.ASYNCHRONOUS; + + // In case there is metadata for the current class, we collect all resolved Angular input + // names and add them to the list of known inputs that need to be checked for usages of + // the current query. e.g. queries used in an @Input() *setter* are always static. + if (classMetadata) { + knownInputNames.push(...classMetadata.ngInputNames); + } + + // Array of TypeScript nodes which can contain usages of the given query in + // order to access it statically. + const possibleStaticQueryNodes = filterQueryClassMemberNodes(classDecl, query, knownInputNames); + + // In case nodes that can possibly access a query statically have been found, check + // if the query declaration is synchronously used within any of these nodes. + if (possibleStaticQueryNodes.length) { + possibleStaticQueryNodes.forEach( + n => usage = combineResolvedUsage(usage, usageVisitor.getResolvedNodeUsage(n))); + } + + if (!classMetadata) { + return usage; + } + + // In case there is a component template for the current class, we check if the + // template statically accesses the current query. In case that's true, the query + // can be marked as static. + if (classMetadata.template && hasPropertyNameText(query.property !.name)) { + const template = classMetadata.template; + const parsedHtml = parseHtmlGracefully(template.content, template.filePath); + const htmlVisitor = new TemplateUsageVisitor(query.property !.name.text); + + if (parsedHtml && htmlVisitor.isQueryUsedStatically(parsedHtml)) { + return ResolvedUsage.SYNCHRONOUS; + } + } + + // In case derived classes should also be analyzed, we determine the classes that derive + // from the current class and check if these have input setters or lifecycle hooks that + // use the query statically. + if (visitInheritedClasses) { + classMetadata.derivedClasses.forEach(derivedClass => { + usage = combineResolvedUsage( + usage, this.analyzeQueryUsage(derivedClass, query, knownInputNames)); + }); + } + + // In case the current class has a super class, we determine declared abstract function-like + // declarations in the super-class that are implemented in the current class. The super class + // will then be analyzed with the abstract declarations mapped to the implemented TypeScript + // nodes. This allows us to handle queries which are used in super classes through derived + // abstract method declarations. + if (classMetadata.superClass) { + const superClassDecl = classMetadata.superClass; + + // Update the function context to map abstract declaration nodes to their implementation + // node in the base class. This ensures that the declaration usage visitor can analyze + // abstract class member declarations. + updateSuperClassAbstractMembersContext(classDecl, functionCtx, this.classMetadata); + + usage = combineResolvedUsage( + usage, this.analyzeQueryUsage(superClassDecl, query, [], functionCtx, false)); + } + + return usage; } } - /** - * Checks whether a given query is used statically within the given class, its super - * class or derived classes. + * Combines two resolved usages based on a fixed priority. "Synchronous" takes + * precedence over "Ambiguous" whereas ambiguous takes precedence over "Asynchronous". */ -function isQueryUsedStatically( - classDecl: ts.ClassDeclaration, query: NgQueryDefinition, classMetadataMap: ClassMetadataMap, - typeChecker: ts.TypeChecker, knownInputNames: string[], - functionCtx: FunctionContext = new Map(), visitInheritedClasses = true): boolean { - const usageVisitor = new DeclarationUsageVisitor(query.property !, typeChecker, functionCtx); - const classMetadata = classMetadataMap.get(classDecl); - - // In case there is metadata for the current class, we collect all resolved Angular input - // names and add them to the list of known inputs that need to be checked for usages of - // the current query. e.g. queries used in an @Input() *setter* are always static. - if (classMetadata) { - knownInputNames.push(...classMetadata.ngInputNames); +function combineResolvedUsage(base: ResolvedUsage, target: ResolvedUsage): ResolvedUsage { + if (base === ResolvedUsage.SYNCHRONOUS) { + return base; + } else if (target !== ResolvedUsage.ASYNCHRONOUS) { + return target; + } else { + return ResolvedUsage.ASYNCHRONOUS; } - - // Array of TypeScript nodes which can contain usages of the given query in - // order to access it statically. - const possibleStaticQueryNodes = filterQueryClassMemberNodes(classDecl, query, knownInputNames); - - // In case nodes that can possibly access a query statically have been found, check - // if the query declaration is synchronously used within any of these nodes. - if (possibleStaticQueryNodes.length && - possibleStaticQueryNodes.some(n => usageVisitor.isSynchronouslyUsedInNode(n))) { - return true; - } - - if (!classMetadata) { - return false; - } - - // In case there is a component template for the current class, we check if the - // template statically accesses the current query. In case that's true, the query - // can be marked as static. - if (classMetadata.template && hasPropertyNameText(query.property !.name)) { - const template = classMetadata.template; - const parsedHtml = parseHtmlGracefully(template.content, template.filePath); - const htmlVisitor = new TemplateUsageVisitor(query.property !.name.text); - - if (parsedHtml && htmlVisitor.isQueryUsedStatically(parsedHtml)) { - return true; - } - } - - // In case derived classes should also be analyzed, we determine the classes that derive - // from the current class and check if these have input setters or lifecycle hooks that - // use the query statically. - if (visitInheritedClasses) { - if (classMetadata.derivedClasses.some( - derivedClass => isQueryUsedStatically( - derivedClass, query, classMetadataMap, typeChecker, knownInputNames))) { - return true; - } - } - - // In case the current class has a super class, we determine declared abstract function-like - // declarations in the super-class that are implemented in the current class. The super class - // will then be analyzed with the abstract declarations mapped to the implemented TypeScript - // nodes. This allows us to handle queries which are used in super classes through derived - // abstract method declarations. - if (classMetadata.superClass) { - const superClassDecl = classMetadata.superClass; - - // Update the function context to map abstract declaration nodes to their implementation - // node in the base class. This ensures that the declaration usage visitor can analyze - // abstract class member declarations. - updateSuperClassAbstractMembersContext(classDecl, functionCtx, classMetadataMap); - - if (isQueryUsedStatically( - superClassDecl, query, classMetadataMap, typeChecker, [], functionCtx, false)) { - return true; - } - } - - return false; } - /** * Filters all class members from the class declaration that can access the * given query statically (e.g. ngOnInit lifecycle hook or @Input setters) diff --git a/packages/core/schematics/migrations/static-queries/transform.ts b/packages/core/schematics/migrations/static-queries/transform.ts index 14aec6ca2e..139df0afa2 100644 --- a/packages/core/schematics/migrations/static-queries/transform.ts +++ b/packages/core/schematics/migrations/static-queries/transform.ts @@ -17,7 +17,8 @@ export type TransformedQueryResult = null | { failureMessage: string|null; }; -const TODO_COMMENT = 'TODO: add static flag'; +const TODO_SPECIFY_COMMENT = 'TODO: add static flag'; +const TODO_CHECK_COMMENT = 'TODO: check static flag'; /** * Transforms the given query decorator by explicitly specifying the timing based on the @@ -37,7 +38,9 @@ export function getTransformedQueryCallExpr( // keep the existing options untouched and just add the new property if possible. if (queryArguments.length === 2) { const existingOptions = queryArguments[1]; - const hasTodoComment = existingOptions.getFullText().includes(TODO_COMMENT); + const existingOptionsText = existingOptions.getFullText(); + const hasTodoComment = existingOptionsText.includes(TODO_SPECIFY_COMMENT) || + existingOptionsText.includes(TODO_CHECK_COMMENT); let newOptionsNode: ts.Expression; let failureMessage: string|null = null; @@ -55,7 +58,7 @@ export function getTransformedQueryCallExpr( // In case we want to add a todo and the options do not have the todo // yet, we add the query timing todo as synthetic multi-line comment. if (createTodo && !hasTodoComment) { - addQueryTimingTodoToNode(newOptionsNode); + addQueryTimingTodoToNode(newOptionsNode, timing === null); } } else { // In case the options query parameter is not an object literal expression, and @@ -63,7 +66,7 @@ export function getTransformedQueryCallExpr( newOptionsNode = existingOptions; // We always want to add a TODO in case the query options cannot be updated. if (!hasTodoComment) { - addQueryTimingTodoToNode(existingOptions); + addQueryTimingTodoToNode(existingOptions, true); } // If there is a new explicit timing that has been determined for the given query, // we create a transformation failure message that shows developers that they need @@ -85,7 +88,7 @@ export function getTransformedQueryCallExpr( const optionsNode = ts.createObjectLiteral(queryPropertyAssignments); if (createTodo) { - addQueryTimingTodoToNode(optionsNode); + addQueryTimingTodoToNode(optionsNode, timing === null); } return { @@ -98,14 +101,15 @@ export function getTransformedQueryCallExpr( /** * Adds a to-do to the given TypeScript node which reminds developers to specify - * an explicit query timing. + * an explicit query timing or to double-check the updated timing. */ -function addQueryTimingTodoToNode(node: ts.Node) { - ts.setSyntheticLeadingComments(node, [{ - pos: -1, - end: -1, - hasTrailingNewLine: false, - kind: ts.SyntaxKind.MultiLineCommentTrivia, - text: ` ${TODO_COMMENT} ` - }]); +function addQueryTimingTodoToNode(node: ts.Node, addSpecifyTimingTodo: boolean) { + ts.setSyntheticLeadingComments( + node, [{ + pos: -1, + end: -1, + hasTrailingNewLine: false, + kind: ts.SyntaxKind.MultiLineCommentTrivia, + text: ` ${addSpecifyTimingTodo ? TODO_SPECIFY_COMMENT : TODO_CHECK_COMMENT} ` + }]); } diff --git a/packages/core/schematics/test/static_queries_migration_usage_spec.ts b/packages/core/schematics/test/static_queries_migration_usage_spec.ts index 0e4d0f5563..fa92fa1e0b 100644 --- a/packages/core/schematics/test/static_queries_migration_usage_spec.ts +++ b/packages/core/schematics/test/static_queries_migration_usage_spec.ts @@ -661,6 +661,19 @@ describe('static-queries migration with usage strategy', () => { }); it('should not mark queries used in promises as static', async() => { + writeFile('/es2015.dom.d.ts', ` + interface PromiseConstructor { + resolve(): Promise; + reject(): Promise; + } + + interface Promise { + then(cb: Function): Promise; + catch(cb: Function): Promise; + } + + declare var Promise: PromiseConstructor; + `); writeFile('/index.ts', ` import {Component, ${queryType}} from '@angular/core'; @@ -791,6 +804,7 @@ describe('static-queries migration with usage strategy', () => { }); it('should not mark queries used in setTimeout as static', async() => { + writeFile('/lib.dom.d.ts', `declare function setTimeout(cb: Function);`); writeFile('/index.ts', ` import {Component, ${queryType}} from '@angular/core'; @@ -826,14 +840,19 @@ describe('static-queries migration with usage strategy', () => { }); it('should not mark queries used in "addEventListener" as static', async() => { + writeFile('/lib.dom.d.ts', ` + interface HTMLElement { + addEventListener(cb: Function); + } + `); writeFile('/index.ts', ` - import {Component, ElementRef, ${queryType}} from '@angular/core'; + import {Component, ${queryType}} from '@angular/core'; @Component({template: ''}) export class MyComp { private @${queryType}('test') query: any; - constructor(private elementRef: ElementRef) {} + constructor(private elementRef: HTMLElement) {} ngOnInit() { this.elementRef.addEventListener(() => { @@ -850,6 +869,7 @@ describe('static-queries migration with usage strategy', () => { }); it('should not mark queries used in "requestAnimationFrame" as static', async() => { + writeFile('/lib.dom.d.ts', `declare function requestAnimationFrame(cb: Function);`); writeFile('/index.ts', ` import {Component, ElementRef, ${queryType}} from '@angular/core'; @@ -1392,6 +1412,75 @@ describe('static-queries migration with usage strategy', () => { .toContain(`@${queryType}('test', { static: true }) query: any;`); }); + it('should mark queries which could be accessed statically within third-party calls as ambiguous', + async() => { + writeFile('/index.ts', ` + import {Component, ${queryType}} from '@angular/core'; + import {thirdPartySync} from 'my-lib'; + + @Component({template: 'Template'}) + export class MyComponent { + @${queryType}('test') query: any; + @${queryType}('test') query2: any; + + ngOnInit() { + const myVarFn = () => this.query2.doSomething(); + + thirdPartySync(() => this.query.doSomething()); + thirdPartySync(myVarFn); + } + } + `); + + writeFile( + '/node_modules/my-lib/index.d.ts', `export function thirdPartySync(fn: Function);`); + + await runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain( + `@${queryType}('test', /* TODO: check static flag */ { static: true }) query: any;`); + expect(tree.readContent('/index.ts')) + .toContain( + `@${queryType}('test', /* TODO: check static flag */ { static: true }) query2: any;`); + expect(warnOutput.length).toBe(2); + expect(warnOutput[0]).toContain('Query timing is ambiguous.'); + expect(warnOutput[1]).toContain('Query timing is ambiguous.'); + }); + + it('should mark queries which could be accessed statically within third-party new expressions as ambiguous', + async() => { + writeFile('/index.ts', ` + import {Component, ${queryType}} from '@angular/core'; + import {ThirdParty} from 'my-lib'; + + @Component({template: 'Template'}) + export class MyComponent { + @${queryType}('test') query: any; + + ngOnInit() { + new ThirdParty(() => this.query.doSomething()); + } + } + `); + + writeFile('/node_modules/my-lib/index.d.ts', ` + export declare class ThirdParty { + constructor(cb: Function); + } + `); + + await runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain( + `@${queryType}('test', /* TODO: check static flag */ { static: true }) query: any;`); + expect(warnOutput.length).toBe(1); + expect(warnOutput[0]) + .toContain( + 'Query timing is ambiguous. Please check if the query can be marked as dynamic'); + }); + it('should properly handle multiple tsconfig files', async() => { writeFile('/src/index.ts', ` import {Component, ${queryType}} from '@angular/core';