Skip to content

Commit

Permalink
fixup! refactor(compiler-cli): effective narrowing of signal reads in…
Browse files Browse the repository at this point in the history 
… templates WIP
  • Loading branch information
@JoostK
JoostK committed Apr 2, 2024
1 parent cd1bb14 commit b081fd0
Show file tree
Hide file tree
Showing 6 changed files with 197 additions and 145 deletions.
4 changes: 3 additions & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,9 @@ export class TypeCheckContextImpl implements TypeCheckContext {
}
result.appendLeft(update.pos, update.text);
}
return result.toString();
const code = result.toString();
console.log(code);
return code;
}

finalize(): Map<AbsoluteFsPath, FileUpdate> {
Expand Down
74 changes: 46 additions & 28 deletions packages/compiler-cli/src/ngtsc/typecheck/src/signal_calls.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,38 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AstVisitor, ASTWithSource, Binary, BindingPipe, Call, Chain, Conditional, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralPrimitive, NonNullAssert, PrefixNot, PropertyRead, PropertyWrite, SafeCall, SafeKeyedRead, SafePropertyRead, ThisReceiver, Unary,} from '@angular/compiler';
import {AST, AstVisitor, ASTWithSource, Binary, BindingPipe, Call, Chain, Conditional, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralPrimitive, NonNullAssert, PrefixNot, PropertyRead, PropertyWrite, SafeCall, SafeKeyedRead, SafePropertyRead, ThisReceiver, Unary,} from '@angular/compiler';

export type SignalCallIdentifier = string&{__brand: 'SignalCallIdentifier'};
export type SignalCallIdentity = string&{__brand: 'SignalCallIdentity'};

export function computeSignalCallIdentifier(call: Call): SignalCallIdentifier {
const identifier = call.receiver.visit(new SignalCallIdentification());
return identifier as SignalCallIdentifier;
export type PotentialSignalCall = Call|SafeCall;

export function computeSignalCallIdentity(
call: PotentialSignalCall,
recurse: (receiverCall: PotentialSignalCall) => string | null): SignalCallIdentity {
const identifier = call.receiver.visit(new SignalCallIdentification(recurse));
return identifier as SignalCallIdentity;
}

class SignalCallIdentification implements AstVisitor {
constructor(private recurse: (receiverCall: PotentialSignalCall) => string | null) {}

visitUnary(ast: Unary): string {
return `${ast.operator}${ast.expr.visit(this)}`;
return `${ast.operator}${this.forAst(ast.expr)}`;
}

visitBinary(ast: Binary): string {
return `${ast.left.visit(this)}${ast.operation}${ast.right.visit(this)}`;
return `${this.forAst(ast.left)}${ast.operation}${this.forAst(ast.right)}`;
}

visitChain(ast: Chain): string {
return ast.expressions.map(expr => expr.visit(this)).join(',');
return ast.expressions.map(expr => this.forAst(expr)).join(',');
}

visitConditional(ast: Conditional): string{return `(${ast.condition.visit(this)} ? ${
ast.trueExp.visit(this)} : ${ast.falseExp.visit(this)})`}
visitConditional(ast: Conditional): string {
return `(${this.forAst(ast.condition)} ? ${this.forAst(ast.trueExp)} : ${
this.forAst(ast.falseExp)})`;
}

visitThisReceiver(ast: ThisReceiver): string {
return 'this';
Expand All @@ -40,24 +48,24 @@ class SignalCallIdentification implements AstVisitor {
}

visitInterpolation(ast: Interpolation): string {
return ast.expressions.map(expr => expr.visit(this)).join(',');
return ast.expressions.map(expr => this.forAst(expr)).join(',');
}

visitKeyedRead(ast: KeyedRead): string {
return `${ast.receiver.visit(this)}[${ast.key.visit(this)}]`;
return `${this.forAst(ast.receiver)}[${this.forAst(ast.key)}]`;
}

visitKeyedWrite(ast: KeyedWrite): string {
return `${ast.receiver.visit(this)}[${ast.key.visit(this)}] = ${ast.value.visit(this)}`;
return `${this.forAst(ast.receiver)}[${this.forAst(ast.key)}] = ${this.forAst(ast.value)}`;
}

visitLiteralArray(ast: LiteralArray): string {
const values = ast.expressions.map(expr => expr.visit(this)).join(',');
const values = ast.expressions.map(expr => this.forAst(expr)).join(',');
return `[${values}]`;
}

visitLiteralMap(ast: LiteralMap): string {
const values = ast.keys.map((key, i) => `${key}: ${ast.values[i].visit(this)}`).join(',');
const values = ast.keys.map((key, i) => `${key}: ${this.forAst(ast.values[i])}`).join(',');
return `{${values}}`;
}

Expand All @@ -66,45 +74,55 @@ class SignalCallIdentification implements AstVisitor {
}

visitPipe(ast: BindingPipe): string {
const args = ast.args.map(expr => expr.visit(this)).join(' : ');
return `${ast.exp.visit(this)} | ${ast.name}: ${args}`;
const args = ast.args.map(expr => this.forAst(expr)).join(' : ');
return `${this.forAst(ast.exp)} | ${ast.name}: ${args}`;
}

visitPrefixNot(ast: PrefixNot): string {
return `!${ast.expression.visit(this)}`;
return `!${this.forAst(ast.expression)}`;
}

visitNonNullAssert(ast: NonNullAssert): string {
return `${ast.expression.visit(this)}!`;
return `${this.forAst(ast.expression)}!`;
}

visitPropertyRead(ast: PropertyRead): string {
return `${ast.receiver.visit(this)}.${ast.name}`;
return `${this.forAst(ast.receiver)}.${ast.name}`;
}

visitPropertyWrite(ast: PropertyWrite): string {
return `${ast.receiver.visit(this)}.${ast.name} = ${ast.value.visit(this)}`;
return `${this.forAst(ast.receiver)}.${ast.name} = ${this.forAst(ast.value)}`;
}

visitSafePropertyRead(ast: SafePropertyRead): string {
return `${ast.receiver.visit(this)}?.${ast.name}`;
return `${this.forAst(ast.receiver)}?.${ast.name}`;
}

visitSafeKeyedRead(ast: SafeKeyedRead): string {
return `${ast.receiver.visit(this)}?.[${ast.key.visit(this)}]`;
return `${this.forAst(ast.receiver)}?.[${this.forAst(ast.key)}]`;
}

visitCall(ast: Call): string {
const args = ast.args.map(expr => expr.visit(this)).join(',');
return `${ast.receiver.visit(this)}(${args})`;
const args = ast.args.map(expr => this.forAst(expr)).join(',');
return `${this.forAst(ast.receiver)}(${args})`;
}

visitSafeCall(ast: SafeCall): string {
const args = ast.args.map(expr => expr.visit(this)).join(',');
return `${ast.receiver.visit(this)}?.(${args})`;
const args = ast.args.map(expr => this.forAst(expr)).join(',');
return `${this.forAst(ast.receiver)}?.(${args})`;
}

visitASTWithSource(ast: ASTWithSource): string {
return ast.ast.visit(this);
return this.forAst(ast.ast);
}

private forAst(ast: AST): string {
// if (ast instanceof Call || ast instanceof SafeCall) {
// const result = this.recurse(ast);
// if (result !== null) {
// return `ɵ${result}`;
// }
// }
return ast.visit(this);
}
}
12 changes: 8 additions & 4 deletions packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,18 @@ export function tsDeclareVariable(id: ts.Identifier, type: ts.TypeNode): ts.Vari
const initializer: ts.Expression = ts.factory.createAsExpression(
ts.factory.createNonNullExpression(ts.factory.createNull()), type);

return tsCreateVariable(id, initializer);
}

export function tsDeclareLetVariable(id: ts.Identifier, type?: ts.TypeNode): ts.VariableStatement {
const decl = ts.factory.createVariableDeclaration(
/* name */ id,
/* exclamationToken */ undefined,
/* type */ undefined,
/* initializer */ initializer);
/* type */ type,
/* initializer */ undefined);
return ts.factory.createVariableStatement(
/* modifiers */ undefined,
/* declarationList */[decl]);
/* declarationList */ ts.factory.createVariableDeclarationList([decl], ts.NodeFlags.Let));
}

/**
Expand Down Expand Up @@ -127,7 +131,7 @@ export function tsCreateVariable(
/* initializer */ initializer);
return ts.factory.createVariableStatement(
/* modifiers */ undefined,
/* declarationList */[decl]);
/* declarationList */ ts.factory.createVariableDeclarationList([decl], ts.NodeFlags.Const));
}

/**
Expand Down
74 changes: 51 additions & 23 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import {DomSchemaChecker} from './dom';
import {Environment} from './environment';
import {astToTypescript, NULL_AS_ANY} from './expression';
import {OutOfBandDiagnosticRecorder} from './oob';
import {computeSignalCallIdentifier, SignalCallIdentifier} from './signal_calls';
import {tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
import {computeSignalCallIdentity, PotentialSignalCall, SignalCallIdentity} from './signal_calls';
import {tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareLetVariable, tsDeclareVariable,} from './ts_util';
import {requiresInlineTypeCtor} from './type_constructor';
import {TypeParameterEmitter} from './type_parameter_emitter';

Expand Down Expand Up @@ -1631,6 +1631,11 @@ export class Context {
}
}

interface SignalCall {
identifier: ts.Identifier;
initialized: boolean;
}

/**
* Local scope within the type check block for a particular template.
*
Expand Down Expand Up @@ -1689,7 +1694,7 @@ class Scope {
*/
private varMap = new Map<TmplAstVariable, number|ts.Identifier>();

private signalCallMap = new Map<SignalCallIdentifier, ts.Identifier>();
private signalCallMap = new Map<SignalCallIdentity, ts.Identifier>();

/**
* Statements for this template.
Expand Down Expand Up @@ -1843,37 +1848,40 @@ class Scope {
}
}

tryResolveSignalCall(call: Call|SafeCall): ts.Identifier|null {
tryResolveSignalCall(call: PotentialSignalCall): SignalCall|null {
if (call.args.length !== 0) {
return null;
}

const identifier = computeSignalCallIdentifier(call);
if (this.signalCallMap.has(identifier)) {
return this.signalCallMap.get(identifier)!;
const identity = computeSignalCallIdentity(call, receiverCall => {
const result = this.tryResolveSignalCall(receiverCall);
if (result === null) {
return null;
}
// FIXME: if `result.initialized` is false then no initialization would take place, since
// subsequent requests
// for the signal call will have a cache hit, causing it to assume that variable has been
// initialized.
return result.identifier.text;
});
if (this.signalCallMap.has(identity)) {
const identifier = this.signalCallMap.get(identity)!;
return {identifier, initialized: true};
}

let parent = this.parent;
while (parent !== null) {
if (parent.signalCallMap.has(identifier)) {
return parent.signalCallMap.get(identifier)!;
if (parent.signalCallMap.has(identity)) {
const identifier = parent.signalCallMap.get(identity)!;
return {identifier, initialized: true};
}
parent = parent.parent;
}

const id = this.tcb.allocateId();
this.signalCallMap.set(identifier, id);
const receiver = tcbExpression(call.receiver, this.tcb, this);
// FIXME: requires logic from `AstTranslator.convertToSafeCall`
let initializer: ts.Expression;
if (call instanceof SafeCall) {
initializer = ts.factory.createCallChain(
receiver, ts.factory.createToken(ts.SyntaxKind.QuestionDotToken), undefined, undefined);
} else {
initializer = ts.factory.createCallExpression(receiver, undefined, undefined);
}
this.addStatement(tsCreateVariable(id, initializer));
return id;
this.signalCallMap.set(identity, id);
this.addStatement(tsDeclareLetVariable(id));
return {identifier: id, initialized: false};
}

/**
Expand Down Expand Up @@ -2422,7 +2430,7 @@ class TcbExpressionTranslator {
return result;
}

const signalCall = this.scope.tryResolveSignalCall(ast);
const signalCall = this.tryResolveSignalCall(ast);
if (signalCall !== null) {
return signalCall;
}
Expand All @@ -2444,13 +2452,33 @@ class TcbExpressionTranslator {
return node;
}

return this.scope.tryResolveSignalCall(ast);
return this.tryResolveSignalCall(ast);
} else {
// This AST isn't special after all.
return null;
}
}

private tryResolveSignalCall(ast: PotentialSignalCall): ts.Expression|null {
const signalCall = this.scope.tryResolveSignalCall(ast);
if (signalCall === null) {
return null;
}

if (signalCall.initialized) {
return signalCall.identifier;
}

const receiver = this.translate(ast.receiver);
const expr = ts.factory.createCallChain(
receiver,
ast instanceof SafeCall ? ts.factory.createToken(ts.SyntaxKind.QuestionDotToken) :
undefined,
/* typeArguments */ undefined, /* argumentsArray */ undefined);
return ts.factory.createParenthesizedExpression(
ts.factory.createAssignment(signalCall.identifier, expr));
}

/**
* Attempts to resolve a bound target for a given expression, and translates it into the
* appropriate `ts.Expression` that represents the bound target. If no target is available,
Expand Down
12 changes: 6 additions & 6 deletions packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ describe('type check blocks diagnostics', () => {
pipeName: 'test',
}];
const block = tcbWithSpans(TEMPLATE, PIPES);
expect(block).toContain('var _pipe1 = null! as i0.TestPipe');
expect(block).toContain('const _pipe1 = null! as i0.TestPipe');
expect(block).toContain(
'(_pipe1.transform /*7,11*/(((this).a /*3,4*/) /*3,4*/, ((this).b /*12,13*/) /*12,13*/) /*3,13*/);');
});
Expand Down Expand Up @@ -224,7 +224,7 @@ describe('type check blocks diagnostics', () => {
expect(tcbWithSpans(template, [])).toContain('(this).users /*14,19*/');
// index variable
expect(tcbWithSpans(template, []))
.toContain('var _t2 /*37,38*/ = null! as number /*T:VAE*/ /*37,47*/');
.toContain('const _t2 /*37,38*/ = null! as number /*T:VAE*/ /*37,47*/');
// track expression
expect(tcbWithSpans(template, [])).toContain('_t1 /*27,31*/;');
});
Expand All @@ -233,16 +233,16 @@ describe('type check blocks diagnostics', () => {
const template =
`@for (x of y; track x; let i = $index, odd = $odd,e_v_e_n=$even) { {{i + odd + e_v_e_n}} }`;
expect(tcbWithSpans(template, []))
.toContain('var _t2 /*27,28*/ = null! as number /*T:VAE*/ /*27,37*/');
.toContain('const _t2 /*27,28*/ = null! as number /*T:VAE*/ /*27,37*/');
expect(tcbWithSpans(template, []))
.toContain('var _t3 /*39,42*/ = null! as boolean /*T:VAE*/ /*39,54*/');
.toContain('const _t3 /*39,42*/ = null! as boolean /*T:VAE*/ /*39,54*/');
expect(tcbWithSpans(template, []))
.toContain('var _t4 /*55,62*/ = null! as boolean /*T:VAE*/ /*55,68*/');
.toContain('const _t4 /*55,62*/ = null! as boolean /*T:VAE*/ /*55,68*/');
});

it('@if', () => {
const template = `@if (x; as alias) { {{alias}} }`;
expect(tcbWithSpans(template, [])).toContain('var _t1 /*11,16*/');
expect(tcbWithSpans(template, [])).toContain('const _t1 /*11,16*/');
expect(tcbWithSpans(template, [])).toContain('(this).x /*5,6*/');
});
});
Expand Down

0 comments on commit b081fd0

Please sign in to comment.