refactor(core): static-query schematic should check templates (#29713)

Queries can technically be also accessed within component templates
e.g.

```html
<my-comp [binding]="myQuery"></my-comp>
```

In that case the query with the property "myQuery" is accessed
statically and needs to be marked with `static: true`. There are
other edge cases that need to be handled as the template property
read doesn't necessarily resolve to the actual query property.

For example:

```html
<foo #myQuery></foo>
<my-comp [binding]="myQuery"></my-comp>
```

In this scenario the binding doesn't refer to the actual query
because the template reference variable takes precedence. The
query doesn't need to be marked with "static: true" this time.

This commit ensures that the `static-query` migration schematic
now handles this cases properly. Also template property reads
that access queries from within a `<ng-template>` are ignored
as these can't access the query before the view has been initialized.

Resolves FW-1216

PR Close #29713
This commit is contained in:
Paul Gschwendtner 2019-04-08 16:16:56 +02:00 committed by Igor Minar
parent b507d076be
commit 5b32f55a3a
16 changed files with 385 additions and 32 deletions

View File

@ -10,6 +10,7 @@ ts_library(
"//packages/core/schematics/test:__pkg__",
],
deps = [
"//packages/compiler",
"//packages/core/schematics/utils",
"@npm//@angular-devkit/schematics",
"@npm//@types/node",

View File

@ -7,13 +7,17 @@
*/
import * as ts from 'typescript';
import {parseHtmlGracefully} from '../../../utils/parse_html';
import {hasPropertyNameText} from '../../../utils/typescript/property_name';
import {DeclarationUsageVisitor, FunctionContext} from './declaration_usage_visitor';
import {ClassMetadataMap} from './ng_query_visitor';
import {NgQueryDefinition, QueryTiming, QueryType} from './query-definition';
import {QueryReadHtmlVisitor} from './query_read_html_visitor';
import {updateSuperClassAbstractMembersContext} from './super_class';
/**
* Object that maps a given type of query to a list of lifecycle hooks that
* could be used to access such a query statically.
@ -69,6 +73,19 @@ function isQueryUsedStatically(
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 QueryReadHtmlVisitor(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.

View File

@ -7,11 +7,15 @@
*/
import * as ts from 'typescript';
import {ResolvedTemplate} from '../../../utils/ng_component_template';
import {getAngularDecorators} from '../../../utils/ng_decorators';
import {findParentClassDeclaration, getBaseTypeIdentifiers} from '../../../utils/typescript/class_declaration';
import {getInputNamesOfClass} from './directive_inputs';
import {NgQueryDefinition, QueryType} from './query-definition';
/** Resolved metadata of a given class. */
export interface ClassMetadata {
/** List of class declarations that derive from the given class. */
@ -20,6 +24,8 @@ export interface ClassMetadata {
superClass: ts.ClassDeclaration|null;
/** List of property names that declare an Angular input within the given class. */
ngInputNames: string[];
/** Component template that belongs to that class if present. */
template?: ResolvedTemplate;
}
/** Type that describes a map which can be used to get a class declaration's metadata. */
@ -48,8 +54,6 @@ export class NgQueryResolveVisitor {
this.visitClassDeclaration(node as ts.ClassDeclaration);
break;
}
ts.forEachChild(node, node => this.visitNode(node));
}
private visitPropertyDeclaration(node: ts.PropertyDeclaration) {

View File

@ -0,0 +1,88 @@
/**
* @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 {ImplicitReceiver, ParseSourceSpan, PropertyRead, RecursiveAstVisitor} from '@angular/compiler';
import {BoundAttribute, BoundEvent, BoundText, Element, Node, NullVisitor, Template, visitAll} from '@angular/compiler/src/render3/r3_ast';
/**
* AST visitor that traverses the Render3 HTML AST in order to check if the given
* query property is accessed statically in the template.
*/
export class QueryReadHtmlVisitor extends NullVisitor {
private hasQueryTemplateReference = false;
private expressionAstVisitor = new ExpressionAstVisitor(this.queryPropertyName);
constructor(public queryPropertyName: string) { super(); }
/** Checks whether the given query is statically accessed within the specified HTML nodes. */
isQueryUsedStatically(htmlNodes: Node[]): boolean {
this.hasQueryTemplateReference = false;
this.expressionAstVisitor.hasQueryPropertyRead = false;
// Visit all AST nodes and check if the query property is used statically.
visitAll(this, htmlNodes);
return !this.hasQueryTemplateReference && this.expressionAstVisitor.hasQueryPropertyRead;
}
visitElement(element: Element): void {
// In case there is a template references variable that matches the query property
// name, we can finish this visitor as such a template variable can be used in the
// entire template and the query therefore can't be accessed from the template.
if (element.references.some(r => r.name === this.queryPropertyName)) {
this.hasQueryTemplateReference = true;
return;
}
visitAll(this, element.attributes);
visitAll(this, element.inputs);
visitAll(this, element.outputs);
visitAll(this, element.children);
}
visitTemplate(template: Template): void {
visitAll(this, template.attributes);
visitAll(this, template.inputs);
visitAll(this, template.outputs);
// We don't want to visit any children of the template as these never can't
// access a query statically. The templates can be rendered in the ngAfterViewInit"
// lifecycle hook at the earliest.
}
visitBoundAttribute(attribute: BoundAttribute) {
attribute.value.visit(this.expressionAstVisitor, attribute.sourceSpan);
}
visitBoundText(text: BoundText) { text.value.visit(this.expressionAstVisitor, text.sourceSpan); }
visitBoundEvent(node: BoundEvent) {
node.handler.visit(this.expressionAstVisitor, node.handlerSpan);
}
}
/**
* AST visitor that checks if the given expression contains property reads that
* refer to the specified query property name.
*/
class ExpressionAstVisitor extends RecursiveAstVisitor {
hasQueryPropertyRead = false;
constructor(private queryPropertyName: string) { super(); }
visitPropertyRead(node: PropertyRead, span: ParseSourceSpan): any {
// The receiver of the property read needs to be "implicit" as queries are accessed
// from the component instance and not from other objects.
if (node.receiver instanceof ImplicitReceiver && node.name === this.queryPropertyName) {
this.hasQueryPropertyRead = true;
return;
}
super.visitPropertyRead(node, span);
}
}

View File

@ -7,6 +7,7 @@ ts_library(
visibility = ["//packages/core/schematics/test:__pkg__"],
deps = [
"//packages/core/schematics/migrations/static-queries",
"//packages/core/schematics/utils",
"@npm//tslint",
],
)

View File

@ -8,6 +8,9 @@
import {Replacement, RuleFailure, Rules} from 'tslint';
import * as ts from 'typescript';
import {NgComponentTemplateVisitor} from '../../../utils/ng_component_template';
import {visitAllNodes} from '../../../utils/typescript/visit_nodes';
import {analyzeNgQueryUsage} from '../angular/analyze_query_usage';
import {NgQueryResolveVisitor} from '../angular/ng_query_visitor';
import {QueryTiming} from '../angular/query-definition';
@ -25,14 +28,29 @@ export class Rule extends Rules.TypedRule {
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
const typeChecker = program.getTypeChecker();
const queryVisitor = new NgQueryResolveVisitor(program.getTypeChecker());
const templateVisitor = new NgComponentTemplateVisitor(typeChecker);
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
const printer = ts.createPrinter();
const failures: RuleFailure[] = [];
// Analyze source files by detecting queries and class relations.
rootSourceFiles.forEach(sourceFile => queryVisitor.visitNode(sourceFile));
// Analyze source files by detecting queries, class relations and component templates.
rootSourceFiles.forEach(sourceFile => {
// The visit utility function only traverses the source file once. We don't want to
// traverse through all source files multiple times for each visitor as this could be
// slow.
visitAllNodes(sourceFile, [queryVisitor, templateVisitor]);
});
const {resolvedQueries, classMetadata} = queryVisitor;
// Add all resolved templates to the class metadata so that we can also
// check component templates for static query usage.
templateVisitor.resolvedTemplates.forEach(template => {
if (classMetadata.has(template.container)) {
classMetadata.get(template.container) !.template = template;
}
});
const queries = resolvedQueries.get(sourceFile);
// No queries detected for the given source file.

View File

@ -10,14 +10,17 @@ import {Rule, SchematicsException, Tree} from '@angular-devkit/schematics';
import {dirname, relative} from 'path';
import * as ts from 'typescript';
import {NgComponentTemplateVisitor} from '../../utils/ng_component_template';
import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths';
import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig';
import {visitAllNodes} from '../../utils/typescript/visit_nodes';
import {analyzeNgQueryUsage} from './angular/analyze_query_usage';
import {NgQueryResolveVisitor} from './angular/ng_query_visitor';
import {getTransformedQueryCallExpr} from './transform';
/** Entry point for the V8 static-query migration. */
export default function(): Rule {
return (tree: Tree) => {
@ -58,14 +61,28 @@ function runStaticQueryMigration(tree: Tree, tsconfigPath: string, basePath: str
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const typeChecker = program.getTypeChecker();
const queryVisitor = new NgQueryResolveVisitor(typeChecker);
const templateVisitor = new NgComponentTemplateVisitor(typeChecker);
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
const printer = ts.createPrinter();
// Analyze source files by detecting queries and class relations.
rootSourceFiles.forEach(sourceFile => queryVisitor.visitNode(sourceFile));
// Analyze source files by detecting queries, class relations and component templates.
rootSourceFiles.forEach(sourceFile => {
// The visit utility function only traverses the source file once. We don't want to
// traverse through all source files multiple times for each visitor as this could be
// slow.
visitAllNodes(sourceFile, [queryVisitor, templateVisitor]);
});
const {resolvedQueries, classMetadata} = queryVisitor;
// Add all resolved templates to the class metadata so that we can also
// check component templates for static query usage.
templateVisitor.resolvedTemplates.forEach(template => {
if (classMetadata.has(template.container)) {
classMetadata.get(template.container) !.template = template;
}
});
// Walk through all source files that contain resolved queries and update
// the source files if needed. Note that we need to update multiple queries
// within a source file within the same recorder in order to not throw off

View File

@ -6,10 +6,12 @@
* found in the LICENSE file at https://angular.io/license
*/
import {PropertyWrite, parseTemplate} from '@angular/compiler';
import {PropertyWrite} from '@angular/compiler';
import {Variable, visitAll} from '@angular/compiler/src/render3/r3_ast';
import {ResolvedTemplate} from '../../utils/ng_component_template';
import {parseHtmlGracefully} from '../../utils/parse_html';
import {PropertyAssignment, PropertyWriteHtmlVisitor} from './angular/property_write_html_visitor';
export interface TemplateVariableAssignment {
@ -24,22 +26,20 @@ export interface TemplateVariableAssignment {
*/
export function analyzeResolvedTemplate(
filePath: string, template: ResolvedTemplate): TemplateVariableAssignment[]|null {
try {
const templateNodes = parseTemplate(template.content, filePath).nodes;
const visitor = new PropertyWriteHtmlVisitor();
const templateNodes = parseHtmlGracefully(template.content, filePath);
// Analyze the Angular Render3 HTML AST and collect all property assignments and
// template variables.
visitAll(visitor, templateNodes);
return filterTemplateVariableAssignments(visitor.propertyAssignments, visitor.templateVariables)
.map(({node, start, end}) => ({node, start: start + node.span.start, end}));
} catch {
// Do nothing if the template couldn't be parsed. We don't want to throw any
// exception if a template is syntactically not valid. e.g. template could be
// using preprocessor syntax.
if (!templateNodes) {
return null;
}
const visitor = new PropertyWriteHtmlVisitor();
// Analyze the Angular Render3 HTML AST and collect all property assignments and
// template variables.
visitAll(visitor, templateNodes);
return filterTemplateVariableAssignments(visitor.propertyAssignments, visitor.templateVariables)
.map(({node, start, end}) => ({node, start: start + node.span.start, end}));
}
/**

View File

@ -9,9 +9,10 @@
import {RuleFailure, Rules} from 'tslint';
import * as ts from 'typescript';
import {createHtmlSourceFile} from '../../../utils/tslint/tslint_html_source_file';
import {analyzeResolvedTemplate} from '../analyze_template';
import {NgComponentTemplateVisitor} from '../../../utils/ng_component_template';
import {createHtmlSourceFile} from '../../../utils/tslint/tslint_html_source_file';
import {visitAllNodes} from '../../../utils/typescript/visit_nodes';
import {analyzeResolvedTemplate} from '../analyze_template';
const FAILURE_MESSAGE = 'Found assignment to template variable. This does not work with Ivy and ' +
'needs to be updated.';
@ -26,7 +27,7 @@ export class Rule extends Rules.TypedRule {
const failures: RuleFailure[] = [];
// Analyze the current source files by detecting all referenced HTML templates.
templateVisitor.visitNode(sourceFile);
visitAllNodes(sourceFile, [templateVisitor]);
const {resolvedTemplates} = templateVisitor;

View File

@ -11,11 +11,12 @@ import {Rule, SchematicContext, SchematicsException, Tree} from '@angular-devkit
import {dirname, relative} from 'path';
import * as ts from 'typescript';
import {NgComponentTemplateVisitor} from '../../utils/ng_component_template';
import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths';
import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig';
import {visitAllNodes} from '../../utils/typescript/visit_nodes';
import {analyzeResolvedTemplate} from './analyze_template';
import {NgComponentTemplateVisitor} from '../../utils/ng_component_template';
type Logger = logging.LoggerApi;
@ -63,7 +64,7 @@ function runTemplateVariableAssignmentCheck(
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
// Analyze source files by detecting HTML templates.
rootSourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile));
rootSourceFiles.forEach(sourceFile => visitAllNodes(sourceFile, [templateVisitor]));
const {resolvedTemplates} = templateVisitor;
const collectedFailures: string[] = [];

View File

@ -128,4 +128,24 @@ describe('Google3 explicitQueryTiming TSLint rule', () => {
expect(failures.length).toBe(1);
expect(failures[0].getFailure()).toMatch(/analysis of the query.*"{static: false}"/);
});
it('should detect query usage in component template', () => {
writeFile('index.ts', `
import {Component, ViewChild} from '@angular/core';
@Component({
template: \`
<span #test></span>
<my-comp [binding]="query"></my-comp>
\`
})
export class MyComp {
@ViewChild('test') query: any;
}
`);
runTSLint();
expectFileToContain('index.ts', `@ViewChild('test', { static: true }) query: any;`);
});
});

View File

@ -1189,6 +1189,144 @@ describe('static-queries migration', () => {
.toContain(`@${queryType}('test', { static: true }) query: any;`);
});
it('should detect query usage within component template', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({templateUrl: 'my-template.html'})
export class MyComponent {
@${queryType}('test') query: any;
}
`);
writeFile(`/my-template.html`, `
<foo #test></foo>
<comp [dir]="query"></comp>
`);
runMigration();
expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', { static: true }) query: any;`);
});
it('should detect query usage with nested property read within component template', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({templateUrl: 'my-template.html'})
export class MyComponent {
@${queryType}('test') query: any;
}
`);
writeFile(`/my-template.html`, `
<foo #test></foo>
<comp [dir]="query.someProperty"></comp>
`);
runMigration();
expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', { static: true }) query: any;`);
});
it('should not mark query as static if template has template reference with same name', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({templateUrl: 'my-template.html'})
export class MyComponent {
@${queryType}('test') query: any;
}
`);
writeFile(`/my-template.html`, `
<foo #test></foo>
<same-name #query></same-name>
<!-- In that case the "query" from the component cannot be referenced. -->
<comp [dir]="query"></comp>
`);
runMigration();
expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', { static: false }) query: any;`);
});
it('should not mark query as static if template has property read with query name but different receiver',
() => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({templateUrl: 'my-template.html'})
export class MyComponent {
myObject: {someProp: any};
@${queryType}('test') someProp: any;
}
`);
// This test ensures that we don't accidentally treat template property reads
// which do not refer to the query of the component instance, but have the same
// "render3Ast.PropertyRead" name, as references to the query declaration.
writeFile(`/my-template.html`, `
<foo #test></foo>
<comp [dir]="myObject.someProp"></comp>
`);
runMigration();
expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', { static: false }) someProp: any;`);
});
it('should ignore queries accessed within <ng-template> element', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({templateUrl: 'my-template.html'})
export class MyComponent {
@${queryType}('test') query: any;
}
`);
writeFile(`/my-template.html`, `
<foo #test></foo>
<ng-template>
<my-comp [myInput]="query"></my-comp>
</ng-template>
`);
runMigration();
expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', { static: false }) query: any;`);
});
it('should detect inherited queries used in templates', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
export class ParentClass {
@${queryType}('test') query: any;
}
@Component({templateUrl: 'my-template.html'})
export class MyComponent extends ParentClass {}
`);
writeFile(`/my-template.html`, `
<foo #test></foo>
<my-comp [myInput]="query"></my-comp>
`);
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';

View File

@ -6,6 +6,7 @@ ts_library(
tsconfig = "//packages/core/schematics:tsconfig.json",
visibility = ["//packages/core/schematics:__subpackages__"],
deps = [
"//packages/compiler",
"@npm//@angular-devkit/core",
"@npm//@angular-devkit/schematics",
],

View File

@ -16,12 +16,16 @@ import {unwrapExpression} from './typescript/functions';
import {getPropertyNameText} from './typescript/property_name';
export interface ResolvedTemplate {
/** Class declaration that contains this template. */
container: ts.ClassDeclaration;
/** File content of the given template. */
content: string;
/** Start offset of the template content (e.g. in the inline source file) */
start: number;
/** Whether the given template is inline or not. */
inline: boolean;
/** Path to the file that contains this template. */
filePath: string;
/**
* Gets the character and line of a given position index in the template.
* If the template is declared inline within a TypeScript source file, the line and
@ -40,13 +44,9 @@ export class NgComponentTemplateVisitor {
constructor(public typeChecker: ts.TypeChecker) {}
visitNode(node: ts.Node) {
switch (node.kind) {
case ts.SyntaxKind.ClassDeclaration:
this.visitClassDeclaration(node as ts.ClassDeclaration);
break;
if (node.kind === ts.SyntaxKind.ClassDeclaration) {
this.visitClassDeclaration(node as ts.ClassDeclaration);
}
ts.forEachChild(node, node => this.visitNode(node));
}
private visitClassDeclaration(node: ts.ClassDeclaration) {
@ -94,7 +94,10 @@ export class NgComponentTemplateVisitor {
// Need to add an offset of one to the start because the template quotes are
// not part of the template content.
const templateStartIdx = property.initializer.getStart() + 1;
this.resolvedTemplates.set(resolve(sourceFileName), {
const filePath = resolve(sourceFileName);
this.resolvedTemplates.set(filePath, {
filePath: filePath,
container: node,
content: property.initializer.text,
inline: true,
start: templateStartIdx,
@ -115,6 +118,8 @@ export class NgComponentTemplateVisitor {
const lineStartsMap = computeLineStartsMap(fileContent);
this.resolvedTemplates.set(templatePath, {
filePath: templatePath,
container: node,
content: fileContent,
inline: false,
start: 0,

View File

@ -0,0 +1,25 @@
/**
* @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 {parseTemplate} from '@angular/compiler';
import {Node} from '@angular/compiler/src/render3/r3_ast';
/**
* Parses the given HTML content using the Angular compiler. In case the parsing
* fails, null is being returned.
*/
export function parseHtmlGracefully(htmlContent: string, filePath: string): Node[]|null {
try {
return parseTemplate(htmlContent, filePath).nodes;
} catch {
// Do nothing if the template couldn't be parsed. We don't want to throw any
// exception if a template is syntactically not valid. e.g. template could be
// using preprocessor syntax.
return null;
}
}

View File

@ -0,0 +1,16 @@
/**
* @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';
export interface TypeScriptVisitor { visitNode(node: ts.Node); }
export function visitAllNodes(node: ts.Node, visitors: TypeScriptVisitor[]) {
visitors.forEach(v => v.visitNode(node));
ts.forEachChild(node, node => visitAllNodes(node, visitors));
}