Skip to content

Commit

Permalink
refactor(compiler-cli): effective narrowing of signal reads in templates
Browse files Browse the repository at this point in the history
This commit is an experiment to see how the template type-check block
can be modified to support effective narrowing of signal reads.

The prototype exposes one problem in particular: calling a type guard method
no longer narrows according to the type-guard, as the method call is indirected
though an additional variable.

Other problems with this approach is that diagnostics will not be reported for
all usages, given that identical call expressions are deduplicated into a single
call expression.

The implementation of this prototype is not as efficient as it could be.
In particular, computing the identifier of a call expression does not leverage
a potentially existing identifier for another call expression in its LHS.
  • Loading branch information
JoostK committed Mar 11, 2024
1 parent 54340a9 commit 8c2e98f
Show file tree
Hide file tree
Showing 5 changed files with 204 additions and 31 deletions.
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {
// Write out the imports that need to be added to the beginning of the file.
let imports = importManager.getAllImports(sf.fileName).map(getImportString).join('\n');
code = imports + '\n' + code;
console.log(code);

return code;
}
Expand Down
110 changes: 110 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/src/signal_calls.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/**
* @license
* Copyright Google LLC 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 {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 function computeSignalCallIdentifier(call: Call): SignalCallIdentifier {
const identifier = call.receiver.visit(new SignalCallIdentification());
return identifier as SignalCallIdentifier;
}

class SignalCallIdentification implements AstVisitor {
visitUnary(ast: Unary): string {
return `${ast.operator}${ast.expr.visit(this)}`;
}

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

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

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

visitThisReceiver(ast: ThisReceiver): string {
return 'this';
}

visitImplicitReceiver(ast: ImplicitReceiver): string {
return 'this';
}

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

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

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

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

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

visitLiteralPrimitive(ast: LiteralPrimitive): string {
return `${ast.value}`;
}

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

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

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

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

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

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

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

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

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

visitASTWithSource(ast: ASTWithSource): string {
return ast.ast.visit(this);
}
}
99 changes: 71 additions & 28 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ 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 {ExpressionSemanticVisitor} from './template_semantics';
import {tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
import {requiresInlineTypeCtor} from './type_constructor';
Expand Down Expand Up @@ -1747,6 +1748,8 @@ class Scope {
*/
private varMap = new Map<TmplAstVariable, number|ts.Identifier>();

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

/**
* Statements for this template.
*
Expand Down Expand Up @@ -1898,6 +1901,39 @@ class Scope {
}
}

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

const identifier = computeSignalCallIdentifier(call);
if (this.signalCallMap.has(identifier)) {
return this.signalCallMap.get(identifier)!;
}

let parent = this.parent;
while (parent !== null) {
if (parent.signalCallMap.has(identifier)) {
return parent.signalCallMap.get(identifier)!;
}
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;
}

/**
* Add a statement to this scope.
*/
Expand Down Expand Up @@ -2429,37 +2465,44 @@ class TcbExpressionTranslator {
/* argumentsArray */[expr, ...args]);
addParseSpanInfo(result, ast.sourceSpan);
return result;
} else if (
(ast instanceof Call || ast instanceof SafeCall) &&
(ast.receiver instanceof PropertyRead || ast.receiver instanceof SafePropertyRead)) {
// Resolve the special `$any(expr)` syntax to insert a cast of the argument to type `any`.
// `$any(expr)` -> `expr as any`
if (ast.receiver.receiver instanceof ImplicitReceiver &&
!(ast.receiver.receiver instanceof ThisReceiver) && ast.receiver.name === '$any' &&
ast.args.length === 1) {
const expr = this.translate(ast.args[0]);
const exprAsAny = ts.factory.createAsExpression(
expr, ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword));
const result = ts.factory.createParenthesizedExpression(exprAsAny);
addParseSpanInfo(result, ast.sourceSpan);
return result;
}
} else if ((ast instanceof Call || ast instanceof SafeCall)) {
if (ast.receiver instanceof PropertyRead || ast.receiver instanceof SafePropertyRead) {
// Resolve the special `$any(expr)` syntax to insert a cast of the argument to type `any`.
// `$any(expr)` -> `expr as any`
if (ast.receiver.receiver instanceof ImplicitReceiver &&
!(ast.receiver.receiver instanceof ThisReceiver) && ast.receiver.name === '$any' &&
ast.args.length === 1) {
const expr = this.translate(ast.args[0]);
const exprAsAny = ts.factory.createAsExpression(
expr, ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword));
const result = ts.factory.createParenthesizedExpression(exprAsAny);
addParseSpanInfo(result, ast.sourceSpan);
return result;
}

// Attempt to resolve a bound target for the method, and generate the method call if a target
// could be resolved. If no target is available, then the method is referencing the top-level
// component context, in which case `null` is returned to let the `ImplicitReceiver` being
// resolved to the component context.
const receiver = this.resolveTarget(ast);
if (receiver === null) {
return null;
const signalCall = this.scope.tryResolveSignalCall(ast);
if (signalCall !== null) {
return signalCall;
}

// Attempt to resolve a bound target for the method, and generate the method call if a
// target could be resolved. If no target is available, then the method is referencing the
// top-level component context, in which case `null` is returned to let the
// `ImplicitReceiver` being resolved to the component context.
const receiver = this.resolveTarget(ast);
if (receiver === null) {
return null;
}

const method = wrapForDiagnostics(receiver);
addParseSpanInfo(method, ast.receiver.nameSpan);
const args = ast.args.map(arg => this.translate(arg));
const node = ts.factory.createCallExpression(method, undefined, args);
addParseSpanInfo(node, ast.sourceSpan);
return node;
}

const method = wrapForDiagnostics(receiver);
addParseSpanInfo(method, ast.receiver.nameSpan);
const args = ast.args.map(arg => this.translate(arg));
const node = ts.factory.createCallExpression(method, undefined, args);
addParseSpanInfo(node, ast.sourceSpan);
return node;
return this.scope.tryResolveSignalCall(ast);
} else {
// This AST isn't special after all.
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export class TypeCheckFile extends Environment {
for (const stmt of this.tcbStatements) {
source += printer.printNode(ts.EmitHint.Unspecified, stmt, this.contextFile) + '\n';
}
console.log(source);

// Ensure the template type-checking file is an ES module. Otherwise, it's interpreted as some
// kind of global namespace in TS, which forces a full re-typecheck of the user's program that
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -339,9 +339,9 @@ describe('type check blocks', () => {
expect(block).toContain('_t1.input = (((this).value));');
expect(block).toContain('var _t2 = null! as i0.HasOutput');
expect(block).toContain('_t2["output"]');
expect(block).toContain('var _t4 = null! as i0.HasReference');
expect(block).toContain('var _t3 = _t4;');
expect(block).toContain('(_t3).a');
expect(block).toContain('var _t5 = null! as i0.HasReference');
expect(block).toContain('var _t4 = _t5;');
expect(block).toContain('(_t4).a');
expect(block).not.toContain('NoBindings');
expect(block).not.toContain('NoReference');
});
Expand Down Expand Up @@ -775,6 +775,24 @@ describe('type check blocks', () => {
expect(block).toContain('if ((((this).person)) !== (null))');
});

it('should deduplicate signal calls', () => {
const DIRECTIVES: TestDeclaration[] = [{
type: 'directive',
name: 'NgIf',
selector: '[ngIf]',
inputs: {'ngIf': 'ngIf'},
ngTemplateGuards: [{
inputName: 'ngIf',
type: 'binding',
}]
}];
const TEMPLATE =
`<div *ngIf="user.name() as name">{{user.name().first()}}{{name.first()}}</div>`;
const block = tcb(TEMPLATE, DIRECTIVES);
fail(block);
// expect(block).toContain('if ((((this).person)) !== (null))');
});

it('should not emit guards when the child scope is empty', () => {
const DIRECTIVES: TestDeclaration[] = [{
type: 'directive',
Expand Down

0 comments on commit 8c2e98f

Please sign in to comment.