From 4e8c2c3422b75b3171830057e4bc0359ca185e3d Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 8 Apr 2019 16:01:25 +0200 Subject: [PATCH] refactor(core): static-query schematic should handle abstract classes (#29688) Queries can not only be accessed within derived classes, but also in the super class through abstract methods. e.g. ``` abstract class BaseClass { abstract getEmbeddedForm(): NgForm {} ngOnInit() { this.getEmbeddedForm().doSomething(); } } class Subclass extends BaseClass { @ViewChild(NgForm) form: NgForm; getEmbeddedForm() { return this.form; } } ``` Same applies for abstract properties which are implemented in the base class through accessors. This case is also now handled by the schematic. Resolves FW-1213 PR Close #29688 --- .../angular/analyze_query_usage.ts | 100 +++++++---- .../angular/declaration_usage_visitor.ts | 52 ++++-- .../angular/ng_query_visitor.ts | 39 +++-- .../static-queries/angular/super_class.ts | 62 +++++++ .../test/static_queries_migration_spec.ts | 157 ++++++++++++++++++ .../core/schematics/utils/typescript/nodes.ts | 14 ++ 6 files changed, 368 insertions(+), 56 deletions(-) create mode 100644 packages/core/schematics/migrations/static-queries/angular/super_class.ts create mode 100644 packages/core/schematics/utils/typescript/nodes.ts 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 f38a0ccf6e..a75f9ef5b1 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 @@ -8,9 +8,11 @@ import * as ts from 'typescript'; import {hasPropertyNameText} from '../../../utils/typescript/property_name'; -import {DeclarationUsageVisitor} from './declaration_usage_visitor'; + +import {DeclarationUsageVisitor, FunctionContext} from './declaration_usage_visitor'; import {ClassMetadataMap} from './ng_query_visitor'; import {NgQueryDefinition, QueryTiming, QueryType} from './query-definition'; +import {updateSuperClassAbstractMembersContext} from './super_class'; /** * Object that maps a given type of query to a list of lifecycle hooks that @@ -34,11 +36,15 @@ export function analyzeNgQueryUsage( QueryTiming.DYNAMIC; } -/** Checks whether a given class or it's derived classes use the specified query statically. */ +/** + * Checks whether a given query is used statically within the given class, its super + * class or derived classes. + */ function isQueryUsedStatically( classDecl: ts.ClassDeclaration, query: NgQueryDefinition, classMetadataMap: ClassMetadataMap, - typeChecker: ts.TypeChecker, knownInputNames: string[]): boolean { - const usageVisitor = new DeclarationUsageVisitor(query.property, typeChecker); + 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 @@ -48,24 +54,9 @@ function isQueryUsedStatically( knownInputNames.push(...classMetadata.ngInputNames); } - // List of TypeScript nodes which can contain usages of the given query in order to - // 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) && 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 !); + // 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. @@ -74,13 +65,66 @@ function isQueryUsedStatically( return true; } - // In case there are classes that derive from the current class, visit each - // derived class as inherited queries could be used statically. - if (classMetadata) { - return classMetadata.derivedClasses.some( - derivedClass => isQueryUsedStatically( - derivedClass, query, classMetadataMap, typeChecker, knownInputNames)); + if (!classMetadata) { + return false; + } + + // 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) + */ +function filterQueryClassMemberNodes( + classDecl: ts.ClassDeclaration, query: NgQueryDefinition, + knownInputNames: string[]): ts.Block[] { + // Returns an array of TypeScript nodes which can contain usages of the given query + // in order to access it statically. e.g. + // (1) queries used in the "ngOnInit" lifecycle hook are static. + // (2) inputs with setters can access queries statically. + return 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 !); +} 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 e028c322e9..cdbcddd0db 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,7 +9,7 @@ import * as ts from 'typescript'; import {isFunctionLikeDeclaration, unwrapExpression} from '../../../utils/typescript/functions'; -type FunctionContext = Map; +export type FunctionContext = Map; /** * List of TypeScript syntax tokens that can be used within a binary expression as @@ -45,7 +45,9 @@ export class DeclarationUsageVisitor { */ private context: FunctionContext = new Map(); - constructor(private declaration: ts.Node, private typeChecker: ts.TypeChecker) {} + constructor( + private declaration: ts.Node, private typeChecker: ts.TypeChecker, + private baseContext: FunctionContext = new Map()) {} private isReferringToSymbol(node: ts.Node): boolean { const symbol = this.typeChecker.getSymbolAtLocation(node); @@ -114,7 +116,7 @@ export class DeclarationUsageVisitor { private visitPropertyAccessors( node: ts.PropertyAccessExpression, checkSetter: boolean, checkGetter: boolean) { - const propertySymbol = this.typeChecker.getSymbolAtLocation(node.name); + const propertySymbol = this._getPropertyAccessSymbol(node); if (!propertySymbol || !propertySymbol.declarations.length || (propertySymbol.getFlags() & ts.SymbolFlags.Accessor) === 0) { @@ -142,13 +144,6 @@ export class DeclarationUsageVisitor { return false; } - const symbol = this.typeChecker.getSymbolAtLocation(leftExpr.name); - - if (!symbol || !symbol.declarations.length || - (symbol.getFlags() & ts.SymbolFlags.Accessor) === 0) { - return false; - } - 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. @@ -166,9 +161,15 @@ export class DeclarationUsageVisitor { } isSynchronouslyUsedInNode(searchNode: ts.Node): boolean { + this.nodeQueue = [searchNode]; this.visitedJumpExprNodes.clear(); this.context.clear(); - this.nodeQueue = [searchNode]; + + // Copy base context values into the current function block context. The + // base context is useful if nodes need to be mapped to other nodes. e.g. + // abstract super class methods are mapped to their implementation node of + // the derived class. + this.baseContext.forEach((value, key) => this.context.set(key, value)); while (this.nodeQueue.length) { const node = this.nodeQueue.shift() !; @@ -225,7 +226,7 @@ export class DeclarationUsageVisitor { * the context, the original node is returned. */ private _resolveNodeFromContext(node: ts.Node): ts.Node { - if (ts.isParameter(node) && this.context.has(node)) { + if (this.context.has(node)) { return this.context.get(node) !; } return node; @@ -279,4 +280,31 @@ export class DeclarationUsageVisitor { return symbol; } + + /** Gets the symbol of the given property access expression. */ + private _getPropertyAccessSymbol(node: ts.PropertyAccessExpression): ts.Symbol|null { + let propertySymbol = this._getDeclarationSymbolOfNode(node.name); + + if (!propertySymbol) { + return null; + } + + if (!this.context.has(propertySymbol.valueDeclaration)) { + return propertySymbol; + } + + // In case the context has the value declaration of the given property access + // name identifier, we need to replace the "propertySymbol" with the symbol + // referring to the resolved symbol based on the context. e.g. abstract properties + // can ultimately resolve into an accessor declaration based on the implementation. + const contextNode = this._resolveNodeFromContext(propertySymbol.valueDeclaration); + + if (!ts.isAccessor(contextNode)) { + return null; + } + + // Resolve the symbol referring to the "accessor" using the name identifier + // of the accessor declaration. + return this._getDeclarationSymbolOfNode(contextNode.name); + } } diff --git a/packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts b/packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts index 7cb2dded82..f6d4130762 100644 --- a/packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts +++ b/packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts @@ -16,6 +16,8 @@ import {NgQueryDefinition, QueryType} from './query-definition'; export interface ClassMetadata { /** List of class declarations that derive from the given class. */ derivedClasses: ts.ClassDeclaration[]; + /** Super class of the given class. */ + superClass: ts.ClassDeclaration|null; /** List of property names that declare an Angular input within the given class. */ ngInputNames: string[]; } @@ -101,29 +103,34 @@ export class NgQueryResolveVisitor { private _recordClassInheritances(node: ts.ClassDeclaration) { const baseTypes = getBaseTypeIdentifiers(node); - if (!baseTypes || !baseTypes.length) { + if (!baseTypes || baseTypes.length !== 1) { return; } - baseTypes.forEach(baseTypeIdentifier => { - // We need to resolve the value declaration through the resolved type as the base - // class could be declared in different source files and the local symbol won't - // contain a value declaration as the value is not declared locally. - const symbol = this.typeChecker.getTypeAtLocation(baseTypeIdentifier).getSymbol(); + const superClass = baseTypes[0]; + const baseClassMetadata = this._getClassMetadata(node); - if (symbol && symbol.valueDeclaration && ts.isClassDeclaration(symbol.valueDeclaration)) { - const extendedClass = symbol.valueDeclaration; - const classMetadata = this._getClassMetadata(extendedClass); + // We need to resolve the value declaration through the resolved type as the base + // class could be declared in different source files and the local symbol won't + // contain a value declaration as the value is not declared locally. + const symbol = this.typeChecker.getTypeAtLocation(superClass).getSymbol(); - // Record all classes that derive from the given class. This makes it easy to - // determine all classes that could potentially use inherited queries statically. - classMetadata.derivedClasses.push(node); - this.classMetadata.set(extendedClass, classMetadata); - } - }); + if (symbol && symbol.valueDeclaration && ts.isClassDeclaration(symbol.valueDeclaration)) { + const extendedClass = symbol.valueDeclaration; + const classMetadataExtended = this._getClassMetadata(extendedClass); + + // Record all classes that derive from the given class. This makes it easy to + // determine all classes that could potentially use inherited queries statically. + classMetadataExtended.derivedClasses.push(node); + this.classMetadata.set(extendedClass, classMetadataExtended); + + // Record the super class of the current class. + baseClassMetadata.superClass = extendedClass; + this.classMetadata.set(node, baseClassMetadata); + } } private _getClassMetadata(node: ts.ClassDeclaration): ClassMetadata { - return this.classMetadata.get(node) || {derivedClasses: [], ngInputNames: []}; + return this.classMetadata.get(node) || {derivedClasses: [], superClass: null, ngInputNames: []}; } } diff --git a/packages/core/schematics/migrations/static-queries/angular/super_class.ts b/packages/core/schematics/migrations/static-queries/angular/super_class.ts new file mode 100644 index 0000000000..b86860a731 --- /dev/null +++ b/packages/core/schematics/migrations/static-queries/angular/super_class.ts @@ -0,0 +1,62 @@ +/** + * @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'; + +import {isFunctionLikeDeclaration} from '../../../utils/typescript/functions'; +import {hasModifier} from '../../../utils/typescript/nodes'; +import {getPropertyNameText} from '../../../utils/typescript/property_name'; + +import {FunctionContext} from './declaration_usage_visitor'; +import {ClassMetadataMap} from './ng_query_visitor'; + + +/** + * Updates the specified function context to map abstract super-class class members + * to their implementation TypeScript nodes. This allows us to run the declaration visitor + * for the super class with the context of the "baseClass" (e.g. with implemented abstract + * class members) + */ +export function updateSuperClassAbstractMembersContext( + baseClass: ts.ClassDeclaration, context: FunctionContext, classMetadataMap: ClassMetadataMap) { + getSuperClassDeclarations(baseClass, classMetadataMap).forEach(superClassDecl => { + superClassDecl.members.forEach(superClassMember => { + if (!superClassMember.name || !hasModifier(superClassMember, ts.SyntaxKind.AbstractKeyword)) { + return; + } + + // Find the matching implementation of the abstract declaration from the super class. + const baseClassImpl = baseClass.members.find( + baseClassMethod => !!baseClassMethod.name && + getPropertyNameText(baseClassMethod.name) === + getPropertyNameText(superClassMember.name !)); + + if (!baseClassImpl || !isFunctionLikeDeclaration(baseClassImpl) || !baseClassImpl.body) { + return; + } + + if (!context.has(superClassMember)) { + context.set(superClassMember, baseClassImpl); + } + }); + }); +} + +/** Gets all super-class TypeScript declarations for the given class. */ +function getSuperClassDeclarations( + classDecl: ts.ClassDeclaration, classMetadataMap: ClassMetadataMap) { + const declarations: ts.ClassDeclaration[] = []; + + let current = classMetadataMap.get(classDecl); + while (current && current.superClass) { + declarations.push(current.superClass); + current = classMetadataMap.get(current.superClass); + } + + return declarations; +} diff --git a/packages/core/schematics/test/static_queries_migration_spec.ts b/packages/core/schematics/test/static_queries_migration_spec.ts index 3a6a0a3a4c..ca54b88864 100644 --- a/packages/core/schematics/test/static_queries_migration_spec.ts +++ b/packages/core/schematics/test/static_queries_migration_spec.ts @@ -1032,6 +1032,163 @@ describe('static-queries migration', () => { .toContain(`@${queryType}('test', { static: true }) query: any;`); }); + it('should check derived abstract class methods', () => { + writeFile('/index.ts', ` + import {Component, ${queryType}} from '@angular/core'; + + export abstract class RootBaseClass { + abstract getQuery(): any; + + ngOnInit() { + this.getQuery().doSomething(); + } + } + + export abstract class BaseClass extends RootBaseClass { + abstract getQuery2(): any; + + getQuery() { + this.getQuery2(); + } + } + + @Component({template: ''}) + export class Subclass extends BaseClass { + @${queryType}('test') query: any; + + getQuery2(): any { + return this.query; + } + } + `); + + runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`@${queryType}('test', { static: true }) query: any;`); + }); + + it('should detect queries accessed through deep abstract class method', () => { + writeFile('/index.ts', ` + import {Component, ${queryType}} from '@angular/core'; + + export abstract class RootBaseClass { + abstract getQuery(): any; + + ngOnInit() { + this.getQuery().doSomething(); + } + } + + export abstract class BaseClass extends RootBaseClass { + /* additional layer of indirection */ + } + + @Component({template: ''}) + export class Subclass extends BaseClass { + @${queryType}('test') query: any; + + getQuery(): any { + return this.query; + } + } + `); + + runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`@${queryType}('test', { static: true }) query: any;`); + }); + + it('should detect queries accessed through abstract property getter', () => { + writeFile('/index.ts', ` + import {Component, ${queryType}} from '@angular/core'; + + export abstract class BaseClass { + abstract myQuery: any; + + ngOnInit() { + this.myQuery.doSomething(); + } + } + + @Component({template: ''}) + export class Subclass extends BaseClass { + @${queryType}('test') query: any; + + get myQuery() { return this.query; } + } + `); + + runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`@${queryType}('test', { static: true }) query: any;`); + }); + + it('should detect queries accessed through abstract property setter', () => { + writeFile('/index.ts', ` + import {Component, ${queryType}} from '@angular/core'; + + export abstract class BaseClass { + abstract myQuery: any; + + ngOnInit() { + this.myQuery = "trigger"; + } + } + + @Component({template: ''}) + export class Subclass extends BaseClass { + @${queryType}('test') query: any; + + set myQuery(val: any) { this.query.doSomething() } + get myQuery() { /* noop */ } + } + `); + + runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`@${queryType}('test', { static: true }) query: any;`); + }); + + it('should detect query usage in abstract class methods accessing inherited query', () => { + writeFile('/index.ts', ` + import {Component, ${queryType}} from '@angular/core'; + + export abstract class RootBaseClass { + abstract getQuery(): any; + + ngOnInit() { + this.getQuery().doSomething(); + } + } + + export abstract class BaseClass extends RootBaseClass { + @${queryType}('test') query: any; + abstract getQuery2(): any; + + getQuery() { + this.getQuery2(); + } + } + + @Component({template: ''}) + export class Subclass extends BaseClass { + + getQuery2(): any { + return this.query; + } + } + `); + + runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`@${queryType}('test', { static: true }) query: any;`); + }); + it('should properly handle multiple tsconfig files', () => { writeFile('/src/index.ts', ` import {Component, ${queryType}} from '@angular/core'; diff --git a/packages/core/schematics/utils/typescript/nodes.ts b/packages/core/schematics/utils/typescript/nodes.ts new file mode 100644 index 0000000000..48ac25d3b4 --- /dev/null +++ b/packages/core/schematics/utils/typescript/nodes.ts @@ -0,0 +1,14 @@ +/** + * @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 the given TypeScript node has the specified modifier set. */ +export function hasModifier(node: ts.Node, modifierKind: ts.SyntaxKind) { + return !!node.modifiers && node.modifiers.some(m => m.kind === modifierKind); +}