8000 feat(compiler-cli): enable narrowing of signal reads in templates · angular/angular@9a8d8d0 · GitHub
[go: up one dir, main page]
Skip to content

Commit 9a8d8d0

Browse files
JoostKJoostK
committed
feat(compiler-cli): enable narrowing of signal reads in templates
Signal reads in Angular templates don't currently benefit from TypeScript's narrowing capabilities, since function calls are not eligible for type narrowing. This is unfortunate since Angular templates should not be updating signal values, such that narrowing would be a safe operation. Ideally there would be a way for TypeScript to enable type-narrowing of signal reads, but this is unfortunately not possible today. Various potential designs have been explored to support type-narrowing of signal reads in the compiler's Type-Check Block (TCB): 1. Assign call expressions to a local variable in the TCB, rewriting further usages of the same signal to the local variable instead. This way, TypeScript is able to apply narrowing of this local variable that all subsequent reads are then able to leverage. 2. Augment the `Function` declaration with a getter field corresponding with the function's return type, then emitting call expression as property reads using this augmented getter. Although approach 2 simplifies the TCB emit compared to 1, it could not be made to work in practice as the `Function` declaration itself doesn't have any generic type to extract the return type from. Instead, this commit implements approach 1. The primary known design limitation of this approach arises with optional chaining, where the call expressions end up with different identities to prevent reusing an earlier variable declaration for the call expression. For example, consider the following template: ``` @if (person()?.address()) { <app-address [address]="person().address()"> } ``` Normally in TypeScript this would be accepted (provided that the call expressions are just property reads), but our referencing approach does not consider the signal reads of `address()` to have the same receiver. You may think this limitation can be overcome by always identifying property reads as equivalent to their safe counterpart (i.e. always treating `.` as `?.`) but doing so would introduce false negatives in the negated case: ``` @if (!person()?.address()) { <app-address [address]="person().address()"> } ``` If the `person().address()` was to correspond with `person()?.address()` to make it align with the condition, then no error would be reported that `person()` may be `null`. To avoid such false negatives in template diagnostics this commit is being conservative with optional chains, where narrowing is not achieved as would be the case with property reads. Perhaps there may be ways of improving this in the future. Another side effect of this change is that diagnostics won't be reported within expressions that have been substituted with a reference. Consider: ``` @if (person().nam()) { {{ person().nam() }} } ``` Since the secondary occurrence of `person().nam()` is being substituted for the local variable that is assigned within the `@if`-condition, the typo in `nam()` is not reported for the expression in the block.
1 parent 101edda commit 9a8d8d0

File tree

6 files changed

+644
-31
lines changed

6 files changed

+644
-31
lines changed

‎packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts

Lines changed: 86 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import ts from 'typescript';
3838
import {TypeCheckingConfig} from '../api';
3939

4040
import {addParseSpanInfo, wrapForDiagnostics, wrapForTypeChecker} from './diagnostics';
41+
import {isPotentialSignalCall, PotentialSignalCall} from './signal_calls';
4142
import {tsCastToAny, tsNumericExpression} from './ts_util';
4243

4344
export const NULL_AS_ANY = ts.factory.createAsExpression(
@@ -72,23 +73,56 @@ const BINARY_OPS = new Map<string, ts.BinaryOperator>([
7273
['??', ts.SyntaxKind.QuestionQuestionToken],
7374
]);
7475

76+
export interface SignalCallNarrowing {
77+
/**
78+
* Declares a variable in the TCB to assign the result of the provided call expression to and
79+
* returns the `ts.Identifier` of the variable. If the call expression is not suitable for signal
80+
* narrowing then `null` is returned.
81+
*/
82+
allocateDeclaration(call: PotentialSignalCall): ts.Identifier | null;
83+
84+
/**
85+
* Attempts to resolve a local TCB variable that has been initialized with the provided call
86+
* expression. If the call expression hasn't been assigned into a variable then `null` is
87+
* returned.
88+
*/
89+
resolveDeclaration(call: PotentialSignalCall): ts.Identifier | null;
90+
}
91+
92+
export enum EmitMode {
93+
/**
94+
* Regular emit mode.
95+
*/
96+
Regular,
97+
98+
/**
99+
* In this mode, call expression are being assigned into a local variable declaration for each
100+
* uniquely identified call expression.
101+
*/
102+
Narrowing,
103+
}
104+
75105
/**
76106
* Convert an `AST` to TypeScript code directly, without going through an intermediate `Expression`
77107
* AST.
78108
*/
79109
export function astToTypescript(
80110
ast: AST,
81111
maybeResolve: (ast: AST) => ts.Expression | null,
112+
signalCallOutlining: SignalCallNarrowing,
82113
config: TypeCheckingConfig,
114+
mode: EmitMode,
83115
): ts.Expression {
84-
const translator = new AstTranslator(maybeResolve, config);
116+
const translator = new AstTranslator(maybeResolve, signalCallOutlining, config, mode);
85117
return translator.translate(ast);
86118
}
87119

88120
class AstTranslator implements AstVisitor {
89121
constructor(
90122
private maybeResolve: (ast: AST) => ts.Expression | null,
123+
private signalCallNarrowing: SignalCallNarrowing,
91124
private config: TypeCheckingConfig,
125+
private mode: EmitMode,
92126
) {}
93127

94128
translate(ast: AST): ts.Expression {
@@ -373,6 +407,26 @@ class AstTranslator implements AstVisitor {
373407
visitCall(ast: Call): ts.Expression {
374408
const args = ast.args.map((expr) => this.translate(expr));
375409

410+
// If the call may be a signal call, attempt to reuse a reference to an assigned variable in the
411+
// TCB that corresponds with the same signal call in a higher scope.
412+
let signalAssignmentTarget: ts.Identifier | null = null;
413+
if (isPotentialSignalCall(ast)) {
414+
const declaration = this.signalCallNarrowing.resolveDeclaration(ast);
415+
if (declaration !== null) {
416+
// A local declaration that captures the call's type exists, use it to enable narrowing of
417+
// signal calls. Note that doing so means that any diagnostic within this call expression
418+
// will remain unreported, under the assumption that the same diagnostic are being reported
419+
// in the initial assignment to the local variable.
420+
return declaration;
421+
}
422+
423+
// If the expression is being emitted in a narrowing position, allocate a local declaration
424+
// for the call expression and assign its value into it.
425+
if (this.mode === EmitMode.Narrowing) {
426+
signalAssignmentTarget = this.signalCallNarrowing.allocateDeclaration(ast);
427+
}
428+
}
429+
376430
let expr: ts.Expression;
377431
const receiver = ast.receiver;
378432

@@ -402,14 +456,44 @@ class AstTranslator implements AstVisitor {
402456
node = ts.factory.createCallExpression(expr, undefined, args);
403457
}
404458

459+
if (signalAssignmentTarget !== null) {
460+
node = ts.factory.createAssignment(signalAssignmentTarget, node);
461+
}
462+
405463
addParseSpanInfo(node, ast.sourceSpan);
406464
return node;
407465
}
408466

409467
visitSafeCall(ast: SafeCall): ts.Expression {
410468
const args = ast.args.map((expr) => this.translate(expr));
469+
470+
// If the call may be a signal call, attempt to reuse a reference to an assigned variable in the
471+
// TCB that corresponds with the same signal call in a higher scope.
472+
let signalAssignmentTarget: ts.Identifier | null = null;
473+
if (isPotentialSignalCall(ast)) {
474+
const declaration = this.signalCallNarrowing.resolveDeclaration(ast);
475+
if (declaration !== null) {
476+
// A local declaration that captures the call's type exists, use it to enable narrowing of
477+
// signal calls. Note that doing so means that any diagnostic within this call expression
478+
// will remain unreported, under the assumption that the same diagnostic are being reported
479+
// in the initial assignment to the local variable.
480+
return declaration;
481+
}
482+
483+
// If the expression is being emitted in a narrowing position, allocate a local declaration
484+
// for the call expression and assign its value into it.
485+
if (this.mode === EmitMode.Narrowing) {
486+
signalAssignmentTarget = this.signalCallNarrowing.allocateDeclaration(ast);
487+
}
488+
}
489+
411490
const expr = wrapForDiagnostics(this.translate(ast.receiver));
412-
const node = this.convertToSafeCall(ast, expr, args);
491+
let node = this.convertToSafeCall(ast, expr, args);
492+
493+
if (signalAssignmentTarget !== null) {
494+
node = ts.factory.createAssignment(signalAssignmentTarget, node);
495+
}
496+
413497
addParseSpanInfo(node, ast.sourceSpan);
414498
return node;
415499
}

‎packages/compiler-cli/src/ngtsc/typecheck/src/signal_calls.ts

Lines changed: 257 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {
10+
AST,
11+
AstVisitor,
12+
ASTWithSource,
13+
Binary,
14+
BindingPipe,
15+
Call,
16+
Chain,
17+
Conditional,
18+
ImplicitReceiver,
19+
Interpolation,
20+
KeyedRead,
21+
KeyedWrite,
22+
LiteralArray,
23+
LiteralMap,
24+
LiteralPrimitive,
25+
NonNullAssert,
26+
PrefixNot,
27+
PropertyRead,
28+
PropertyWrite,
29+
SafeCall,
30+
SafeKeyedRead,
31+
SafePropertyRead,
32+
ThisReceiver,
33+
Unary,
34+
} from '@angular/compiler';
35+
36+
/**
37+
* The types of nodes potentially corresponding with a signal call in a template. Any call
38+
* expression without arguments is potentially a signal call, as signal calls cannot be
39+
* differentiated from regular method calls syntactically.
40+
*/
41+
export type PotentialSignalCall = (Call | SafeCall) & {args: []};
42+
43+
/**
44+
* Tests whether the given call expression may correspond with a signal call.
45+
*/
46+
export function isPotentialSignalCall(ast: Call | SafeCall): ast is PotentialSignalCall {
47+
return ast.args.length === 0;
48+
}
49+
50+
/**
51+
* Represents a string that identifies a call expression that may represent a signal call.
52+
*/
53+
export type SignalCallIdentity = string & {__brand: 'SignalCallIdentity'};
54+
55+
/**
56+
* Computes the string identity of the provided call expression. Any expression that is witnessed
57+
* when evaluating the call expression is included in the identity. For implicit reads (e.g.
58+
* property accesses with `ImplicitReceiver` as receiver) an external identity is requested, to
59+
* ensure that each local template variable is assigned its own unique identifier. This is important
60+
* for scenarios like
61+
*
62+
* ```
63+
* @Component({
64+
* template: `
65+
* @if (persons[selectedPerson](); as selectedPerson) {
66+
* {{ persons[selectedPerson]() }}
67+
* }`
68+
* })
69+
* export class ShadowCmp {
70+
* persons = {
71+
* admin: signal('admin');
72+
* guest: signal('guest');
73+
* };
74+
* selectedPerson: keyof ShadowCmp['persons'];
75+
* }
76+
* ```
77+
*
78+
* Here, the local alias `selectedPerson` shadows the `ShadowCmp.selectedPerson` field, such that
79+
* the call expression in the @if's condition expression is not equivalent to the syntactically
80+
* identical expression within its block. Consequently, different identities have to be computed for
81+
* both call expressions, which is achieved by letting `identifyImplicitRead` resolve any implicit
82+
* accesses to a unique identifier for local template variables.
83+
*
84+
* @param call The call expression to compute the identity for.
85+
* @param recurse Callback function to compute the identity of nested signal calls.
86+
* @param identifyImplicitRead Callback function to determine the identity of implicit reads.
87+
*/
88+
export function computeSignalCallIdentity(
89+
call: PotentialSignalCall,
90+
recurse: (receiverCall: PotentialSignalCall) => string | null,
91+
identifyImplicitRead: (expr: AST) => string | null,
92+
): SignalCallIdentity | null {
93+
return call.receiver.visit(new SignalCallIdentification(recurse, identifyImplicitRead));
94+
}
95+
96+
class SignalCallIdentification implements AstVisitor {
97+
constructor(
98+
private recurse: (receiverCall: PotentialSignalCall) => string | null,
99+
private identifyImplicitRead: (expr: AST) => string | null,
100+
) {}
101+
102+
visitUnary(ast: Unary): string | null {
103+
const expr = this.forAst(ast.expr);
104+
if (expr === null) {
105+
return null;
106+
}
107+
return `${ast.operator}${expr}`;
108+
}
109+
110+
visitBinary(ast: Binary): string | null {
111+
const left = this.forAst(ast.left);
112+
const right = this.forAst(ast.right);
113+
if (left === null || right === null) {
114+
return null;
115+
}
116+
return `${left}${ast.operation}${right}`;
117+
}
118+
119+
visitChain(ast: Chain): string | null {
120+
return null;
121+
}
122+
123+
visitConditional(ast: Conditional): string | null {
124+
return null;
125+
}
126+
127+
visitThisReceiver(ast: ThisReceiver): string | null {
128+
return 'this';
129+
}
130+
131+
visitImplicitReceiver(ast: ImplicitReceiver): string | null {
132+
return 'this';
133+
}
134+
135+
visitInterpolation(ast: Interpolation): string | null {
136+
return null;
137+
}
138+
139+
visitKeyedRead(ast: KeyedRead): string | null {
140+
const receiver = this.forAst(ast.receiver);
141+
const key = this.forAst(ast.key);
142+
if (receiver === null || key === null) {
143+
return null;
144+
}
145+
return `${receiver}[${key}]`;
146+
}
147+
148+
visitKeyedWrite(ast: KeyedWrite): string | null {
149+
return null;
150+
}
151+
152+
visitLiteralArray(ast: LiteralArray): string | null {
153+
return null;
154+
}
155+
156+
visitLiteralMap(ast: LiteralMap): string | null {
157+
return null;
158+
}
159+
160+
visitLiteralPrimitive(ast: LiteralPrimitive): string | null {
161+
return `${ast.value}`;
162+
}
163+
164+
visitPipe(ast: BindingPipe): string | null {
165+
// Don't enable narrowing when pipes are being evaluated.
166+
return null;
167+
}
168+
169+
visitPrefixNot(ast: PrefixNot): string | null {
170+
const expression = this.forAst(ast.expression);
171+
if (expression === null) {
172+
return expression;
173+
}
174+
return `!${expression}`;
175+
}
176+
177+
visitNonNullAssert(ast: NonNullAssert): string | null {
178+
return this.forAst(ast.expression);
179+
}
180+
181+
visitPropertyRead(ast: PropertyRead): string | null {
182+
const receiver = this.identifyReceiver(ast);
183+
if (receiver === null) {
184+
return null;
185+
}
186+
return `${receiver}.${ast.name}`;
187+
}
188+
189+
visitPropertyWrite(ast: PropertyWrite): string | null {
190+
return null;
191+
}
192+
193+
visitSafePropertyRead(ast: SafePropertyRead): string | null {
194+
const receiver = this.identifyReceiver(ast);
195+
if (receiver === null) {
196+
return null;
197+
}
198+
return `${receiver}?.${ast.name}`;
199+
}
200+
201+
visitSafeKeyedRead(ast: SafeKeyedRead): string | null {
202+
const receiver = this.forAst(ast.receiver);
203+
if (receiver === null) {
204+
return null;
205+
}
206+
return `${receiver}?.[${this.forAst(ast.key)}]`;
207+
}
208+
209+
visitCall(ast: Call): string | null {
210+
if (ast.args.length > 0) {
211+
// Don't enable narrowing for complex calls.
212+
return null;
213+
}
214+
const receiver = this.forAst(ast.receiver);
215+
if (receiver === null) {
216+
return null;
217+
}
218+
return `${receiver}()`;
219+
}
220+
221+
visitSafeCall(ast: SafeCall): string | null {
222+
if (ast.args.length > 0) {
223+
// Don't enable narrowing for complex calls.
224+
return null;
225+
}
226+
const receiver = this.forAst(ast.receiver);
227+
if (receiver === null) {
228+
return null;
229+
}
230+
return `${receiver}?.()`;
231+
}
232+
233+
visitASTWithSource(ast: ASTWithSource): string | null {
234+
return this.forAst(ast.ast);
235+
}
236+
237+
private identifyReceiver(ast: PropertyRead | SafePropertyRead): string | null {
238+
const receiver = ast.receiver;
239+
if (receiver instanceof ImplicitReceiver && !(receiver instanceof ThisReceiver)) {
240+
const implicitIdentity = this.identifyImplicitRead(ast);
241+
if (implicitIdentity !== null) {
242+
return implicitIdentity;
243+
}
244+
}
245+
return this.forAst(receiver);
246+
}
247+
248+
private forAst(ast: AST): string | null {
249+
if ((ast instanceof Call || ast instanceof SafeCall) && isPotentialSignalCall(ast)) {
250+
const result = this.recurse(ast);
251+
if (result !== null) {
252+
return `ɵ${result}`;
253+
}
254+
}
255+
return ast.visit(this);
256+
}
257+
}

0 commit comments

Comments
 (0)
0