Skip to content

Commit 8d3365e

Browse files
devversionalxhub
authored andcommitted
fix(core): static-query usage migration strategy should detect ambiguous query usage (#30215)
Currently we always just set the timing to `false` if we aren't able to analyze a given call expression or new expression. e.g. ```ts ngOnInit() { thirdPartyCallSync(() => this.query.doSomething()) } ``` In that case the `thirdPartyCallSync` function comes from the `node_modules` and is only defined through types while there is no code for the actual function logic that can be analyzed. This makes it impossible to tell whether the given call expression actually causes the specified arrow function to be executed synchronously or not. In order to be able to make this better, we now peek into the passed arrow function and check for a synchronous query usage. If so, we set the query timing to static and mark it as ambiguous. This ensures that the usage strategy is less "magical" and more correct with third-party code. Additionally since functions like `setTimeout` are not analyzable but known to be asynchronous, there is a hard-coded list of known functions which shouldn't be marked as ambiguous. Resolves FW-1214. As planned within https://hackmd.io/hPiLWpPlQ4uynC1luIBdfQ PR Close #30215
1 parent 8cec8f5 commit 8d3365e

4 files changed

Lines changed: 308 additions & 100 deletions

File tree

‎packages/core/schematics/migrations/static-queries/strategies/usage_strategy/declaration_usage_visitor.ts‎

Lines changed: 107 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,16 @@
88

99
import * as ts from 'typescript';
1010
import {isFunctionLikeDeclaration, unwrapExpression} from '../../../../utils/typescript/functions';
11+
import {getPropertyNameText} from '../../../../utils/typescript/property_name';
1112

1213
export type FunctionContext = Map<ts.Node, ts.Node>;
1314

15+
export enum ResolvedUsage {
16+
SYNCHRONOUS,
17+
ASYNCHRONOUS,
18+
AMBIGUOUS,
19+
}
20+
1421
/**
1522
* List of TypeScript syntax tokens that can be used within a binary expression as
1623
* compound assignment. These imply a read and write of the left-side expression.
@@ -26,6 +33,19 @@ const BINARY_COMPOUND_TOKENS = [
2633
ts.SyntaxKind.SlashEqualsToken,
2734
];
2835

36+
/**
37+
* List of known asynchronous external call expressions which aren't analyzable
38+
* but are guaranteed to not execute the passed argument synchronously.
39+
*/
40+
const ASYNC_EXTERNAL_CALLS = [
41+
{parent: ['Promise'], name: 'then'},
42+
{parent: ['Promise'], name: 'catch'},
43+
{parent: [null, 'Window'], name: 'requestAnimationFrame'},
44+
{parent: [null, 'Window'], name: 'setTimeout'},
45+
{parent: [null, 'Window'], name: 'setInterval'},
46+
{parent: ['*'], name: 'addEventListener'},
47+
];
48+
2949
/**
3050
* Class that can be used to determine if a given TypeScript node is used within
3151
* other given TypeScript nodes. This is achieved by walking through all children
@@ -36,9 +56,18 @@ export class DeclarationUsageVisitor {
3656
/** Set of visited symbols that caused a jump in control flow. */
3757
private visitedJumpExprNodes = new Set<ts.Node>();
3858

39-
/** Queue of nodes that need to be checked for declaration usage. */
59+
/**
60+
* Queue of nodes that need to be checked for declaration usage and
61+
* are guaranteed to be executed synchronously.
62+
*/
4063
private nodeQueue: ts.Node[] = [];
4164

65+
/**
66+
* Nodes which need to be checked for declaration usage but aren't
67+
* guaranteed to execute synchronously.
68+
*/
69+
private ambiguousNodeQueue: ts.Node[] = [];
70+
4271
/**
4372
* Function context that holds the TypeScript node values for all parameters
4473
* of the currently analyzed function block.
@@ -68,6 +97,7 @@ export class DeclarationUsageVisitor {
6897
const callExprSymbol = this._getDeclarationSymbolOfNode(node);
6998

7099
if (!callExprSymbol || !callExprSymbol.valueDeclaration) {
100+
this.peekIntoJumpExpression(callExpression);
71101
return;
72102
}
73103

@@ -77,6 +107,7 @@ export class DeclarationUsageVisitor {
77107
// this could cause cycles.
78108
if (!isFunctionLikeDeclaration(expressionDecl) ||
79109
this.visitedJumpExprNodes.has(expressionDecl) || !expressionDecl.body) {
110+
this.peekIntoJumpExpression(callExpression);
80111
return;
81112
}
82113

@@ -95,6 +126,7 @@ export class DeclarationUsageVisitor {
95126
// we should not visit already visited symbols as this could cause cycles.
96127
if (!newExprSymbol || !newExprSymbol.valueDeclaration ||
97128
!ts.isClassDeclaration(newExprSymbol.valueDeclaration)) {
129+
this.peekIntoJumpExpression(node);
98130
return;
99131
}
100132

@@ -111,6 +143,8 @@ export class DeclarationUsageVisitor {
111143

112144
this.visitedJumpExprNodes.add(targetConstructor);
113145
this.nodeQueue.push(targetConstructor.body);
146+
} else {
147+
this.peekIntoJumpExpression(node);
114148
}
115149
}
116150

@@ -160,7 +194,7 @@ export class DeclarationUsageVisitor {
160194
return true;
161195
}
162196

163-
isSynchronouslyUsedInNode(searchNode: ts.Node): boolean {
197+
getResolvedNodeUsage(searchNode: ts.Node): ResolvedUsage {
164198
this.nodeQueue = [searchNode];
165199
this.visitedJumpExprNodes.clear();
166200
this.context.clear();
@@ -171,11 +205,17 @@ export class DeclarationUsageVisitor {
171205
// the derived class.
172206
this.baseContext.forEach((value, key) => this.context.set(key, value));
173207

208+
return this.isSynchronouslyUsedInNode(searchNode);
209+
}
210+
211+
private isSynchronouslyUsedInNode(searchNode: ts.Node): ResolvedUsage {
212+
this.ambiguousNodeQueue = [];
213+
174214
while (this.nodeQueue.length) {
175215
const node = this.nodeQueue.shift() !;
176216

177217
if (ts.isIdentifier(node) && this.isReferringToSymbol(node)) {
178-
return true;
218+
return ResolvedUsage.SYNCHRONOUS;
179219
}
180220

181221
// Handle call expressions within TypeScript nodes that cause a jump in control
@@ -218,7 +258,65 @@ export class DeclarationUsageVisitor {
218258
this.nodeQueue.push(...node.getChildren());
219259
}
220260
}
221-
return false;
261+
262+
if (this.ambiguousNodeQueue.length) {
263+
// Update the node queue to all stored ambiguous nodes. These nodes are not
264+
// guaranteed to be executed and therefore in case of a synchronous usage
265+
// within one of those nodes, the resolved usage is ambiguous.
266+
this.nodeQueue = this.ambiguousNodeQueue;
267+
const usage = this.isSynchronouslyUsedInNode(searchNode);
268+
return usage === ResolvedUsage.SYNCHRONOUS ? ResolvedUsage.AMBIGUOUS : usage;
269+
}
270+
return ResolvedUsage.ASYNCHRONOUS;
271+
}
272+
273+
/**
274+
* Peeks into the given jump expression by adding all function like declarations
275+
* which are referenced in the jump expression arguments to the ambiguous node
276+
* queue. These arguments could technically access the given declaration but it's
277+
* not guaranteed that the jump expression is executed. In that case the resolved
278+
* usage is ambiguous.
279+
*/
280+
private peekIntoJumpExpression(jumpExp: ts.CallExpression|ts.NewExpression) {
281+
if (!jumpExp.arguments) {
282+
return;
283+
}
284+
285+
// For some call expressions we don't want to add the arguments to the
286+
// ambiguous node queue. e.g. "setTimeout" is not analyzable but is
287+
// guaranteed to execute its argument asynchronously. We handle a subset
288+
// of these call expressions by having a hardcoded list of some.
289+
if (ts.isCallExpression(jumpExp)) {
290+
const symbol = this._getDeclarationSymbolOfNode(jumpExp.expression);
291+
if (symbol && symbol.valueDeclaration) {
292+
const parentNode = symbol.valueDeclaration.parent;
293+
if (parentNode && (ts.isInterfaceDeclaration(parentNode) || ts.isSourceFile(parentNode)) &&
294+
(ts.isMethodSignature(symbol.valueDeclaration) ||
295+
ts.isFunctionDeclaration(symbol.valueDeclaration)) &&
296+
symbol.valueDeclaration.name) {
297+
const parentName = ts.isInterfaceDeclaration(parentNode) ? parentNode.name.text : null;
298+
const callName = getPropertyNameText(symbol.valueDeclaration.name);
299+
if (ASYNC_EXTERNAL_CALLS.some(
300+
c =>
301+
(c.name === callName &&
302+
(c.parent.indexOf(parentName) !== -1 || c.parent.indexOf('*') !== -1)))) {
303+
return;
304+
}
305+
}
306+
}
307+
}
308+
309+
jumpExp.arguments !.forEach((node: ts.Node) => {
310+
node = this._resolveDeclarationOfNode(node);
311+
312+
if (ts.isVariableDeclaration(node) && node.initializer) {
313+
node = node.initializer;
314+
}
315+
316+
if (isFunctionLikeDeclaration(node) && !!node.body) {
317+
this.ambiguousNodeQueue.push(node.body);
318+
}
319+
});
222320
}
223321

224322
/**
@@ -252,18 +350,19 @@ export class DeclarationUsageVisitor {
252350
}
253351

254352
if (ts.isIdentifier(argumentNode)) {
255-
this.context.set(parameter, this._resolveIdentifier(argumentNode));
353+
this.context.set(parameter, this._resolveDeclarationOfNode(argumentNode));
256354
} else {
257355
this.context.set(parameter, argumentNode);
258356
}
259357
});
260358
}
261359

262360
/**
263-
* Resolves a TypeScript identifier node. For example an identifier can refer to a
264-
* function parameter which can be resolved through the function context.
361+
* Resolves the declaration of a given TypeScript node. For example an identifier can
362+
* refer to a function parameter. This parameter can then be resolved through the
363+
* function context.
265364
*/
266-
private _resolveIdentifier(node: ts.Identifier): ts.Node {
365+
private _resolveDeclarationOfNode(node: ts.Node): ts.Node {
267366
const symbol = this._getDeclarationSymbolOfNode(node);
268367

269368
if (!symbol || !symbol.valueDeclaration) {

0 commit comments

Comments
 (0)