honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor(core): static-query schematic should handle binary expressions (#29663) 

Currently we only check getters for property access expressions. This is wrong
because property access expressions do not always cause the "getter" to be
triggered. e.g.

```ts
set a() {...}
get a() {...}

ngOnInit() {
  this.a = true;
}
```

In that case the schematic currently incorrectly checks the "getter", while this is a binary
expression and the property access is used as left-side of the binary expression. In that
case we need to check the setter declaration of the property and not the "getter". In order
to fix this, we need to also check `ts.BinaryExpression` nodes and check getters/setters
based on the used operator token. There are three types of binary expressions:

1) Value assignment (using `=`). In that case only the setter is triggered.
2) Compound assignment (e.g. using `+=`). In that case `getter` and `setter` are triggered.
3) Comparison (e.g. using `===`). In that case only the getter is triggered.

PR Close #29663
This commit is contained in:
Paul Gschwendtner 2019-04-02 15:43:09 +02:00 committed by Igor Minar
parent 82c77ce232
commit 00bf636afa
2 changed files with 185 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

94
packages/core/schematics/migrations/static-queries/angular/declaration_usage_visitor.ts
View File

@ -9,6 +9,21 @@
import * as ts from 'typescript';
import {isFunctionLikeDeclaration, unwrapExpression} from '../../../utils/typescript/functions';
/**
* 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.
*/
const BINARY_COMPOUND_TOKENS = [
ts.SyntaxKind.CaretEqualsToken,
ts.SyntaxKind.AsteriskEqualsToken,
ts.SyntaxKind.AmpersandEqualsToken,
ts.SyntaxKind.BarEqualsToken,
ts.SyntaxKind.AsteriskAsteriskEqualsToken,
ts.SyntaxKind.PlusEqualsToken,
ts.SyntaxKind.MinusEqualsToken,
ts.SyntaxKind.SlashEqualsToken,
];
/**
* 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
@ -19,6 +34,9 @@ export class DeclarationUsageVisitor {
/** Set of visited symbols that caused a jump in control flow. */
private visitedJumpExprSymbols = new Set<ts.Symbol>();
/** Set of visited accessor nodes that caused a jump in control flow. */
private visitedAccessorNodes = new Set<ts.AccessorDeclaration>();
constructor(private declaration: ts.Node, private typeChecker: ts.TypeChecker) {}
private isReferringToSymbol(node: ts.Node): boolean {
@ -74,23 +92,58 @@ export class DeclarationUsageVisitor {
}
}
private visitPropertyAccessExpression(node: ts.PropertyAccessExpression, nodeQueue: ts.Node[]) {
private visitPropertyAccessors(
node: ts.PropertyAccessExpression, nodeQueue: ts.Node[], checkSetter: boolean,
checkGetter: boolean) {
const propertySymbol = this.typeChecker.getSymbolAtLocation(node.name);
if (!propertySymbol || !propertySymbol.valueDeclaration ||
this.visitedJumpExprSymbols.has(propertySymbol)) {
if (!propertySymbol || !propertySymbol.declarations.length ||
(propertySymbol.getFlags() & ts.SymbolFlags.Accessor) === 0) {
return;
}
const valueDeclaration = propertySymbol.valueDeclaration;
// Since we checked the symbol flags and the symbol is describing an accessor, the
// declarations are guaranteed to only contain the getters and setters.
const accessors = propertySymbol.declarations as ts.AccessorDeclaration[];
// In case the property access expression refers to a get accessor, we need to visit
// the body of the get accessor declaration as there could be logic that uses the
// given search node synchronously.
if (ts.isGetAccessorDeclaration(valueDeclaration) && valueDeclaration.body) {
this.visitedJumpExprSymbols.add(propertySymbol);
nodeQueue.push(valueDeclaration.body);
accessors
.filter(
d => (checkSetter && ts.isSetAccessor(d) || checkGetter && ts.isGetAccessor(d)) &&
d.body && !this.visitedAccessorNodes.has(d))
.forEach(d => {
this.visitedAccessorNodes.add(d);
nodeQueue.push(d.body !);
});
}
private visitBinaryExpression(node: ts.BinaryExpression, nodeQueue: ts.Node[]): boolean {
const leftExpr = unwrapExpression(node.left);
if (!ts.isPropertyAccessExpression(leftExpr)) {
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.
this.visitPropertyAccessors(leftExpr, nodeQueue, /* 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);
} 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);
}
return true;
}
isSynchronouslyUsedInNode(searchNode: ts.Node): boolean {
@ -116,10 +169,25 @@ export class DeclarationUsageVisitor {
this.addNewExpressionToQueue(node, nodeQueue);
}
// Handle property access expressions. These could resolve to get-accessor declarations
// which can contain synchronous logic that accesses the search node.
// We also need to handle binary expressions where a value can be either assigned to
// the property, or a value is read from a property expression. Depending on the
// binary expression operator, setters or getters need to be analyzed.
if (ts.isBinaryExpression(node)) {
// In case the binary expression contained a property expression on the left side, we
// 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);
continue;
}
}
// Handle property access expressions. Property expressions which are part of binary
// 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.visitPropertyAccessExpression(node, nodeQueue);
this.visitPropertyAccessors(node, nodeQueue, /* setter */ false, /* getter */ true);
}
// Do not visit nodes that declare a block of statements but are not executed

104
packages/core/schematics/test/static_queries_migration_spec.ts
View File

@ -831,6 +831,7 @@ describe('static-queries migration', () => {
export class External {
constructor(private comp: MyComp) {}
set query() { /** noop */ }
get query() { return this.comp.query; }
}
`);
@ -841,6 +842,109 @@ describe('static-queries migration', () => {
.toContain(`@${queryType}('test', { static: true }) query: any;`);
});
it('should not mark queries as static if a value is assigned to accessor property', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({template: '<span #test></span>'})
export class MyComp {
private @${queryType}('test') query: any;
set myProp(value: any) { /* noop */}
get myProp() {
return this.query.myValue;
}
ngOnInit() {
this.myProp = true;
}
}
`);
runMigration();
expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', { static: false }) query: any;`);
});
it('should mark queries as static if non-input setter uses query', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({template: '<span #test></span>'})
export class MyComp {
private @${queryType}('test') query: any;
get myProp() { return null; }
set myProp(value: any) {
this.query.doSomething();
}
ngOnInit() {
this.myProp = 'newValue';
}
}
`);
runMigration();
expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', { static: true }) query: any;`);
});
it('should check setter and getter when using compound assignment', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({template: '<span #test></span>'})
export class MyComp {
private @${queryType}('test') query: any;
private @${queryType}('test') query2: any;
get myProp() { return this.query2 }
set myProp(value: any) {
this.query.doSomething();
}
ngOnInit() {
this.myProp *= 5;
}
}
`);
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;`);
});
it('should check getters when using comparison operator in binary expression', () => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({template: '<span #test></span>'})
export class MyComp {
private @${queryType}('test') query: any;
get myProp() { return this.query }
set myProp(value: any) { /* noop */ }
ngOnInit() {
if (this.myProp === 3) {
// noop
}
}
}
`);
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';