fixup! refactor(compiler-cli): effective narrowing of signal reads in…

… templates WIP
JoostK · Apr 14, 2024 · 441e4b9 · 441e4b9
1 parent 14bfaf1
commit 441e4b9
Show file tree

Hide file tree

Showing 9 changed files with 416 additions and 207 deletions.
diff --git a/packages/compiler-cli/src/ngtsc/file_system/testing/src/test_helper.ts b/packages/compiler-cli/src/ngtsc/file_system/testing/src/test_helper.ts
@@ -35,7 +35,7 @@ const FS_NATIVE = 'Native';
 const FS_OS_X = 'OS/X';
 const FS_UNIX = 'Unix';
 const FS_WINDOWS = 'Windows';
-const FS_ALL = [FS_OS_X, FS_WINDOWS, FS_UNIX, FS_NATIVE];
+const FS_ALL = [FS_NATIVE];
 
 function runInEachFileSystemFn(callback: (os: string) => void) {
   FS_ALL.forEach(os => runInFileSystem(os, callback, false));

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts
@@ -47,7 +47,19 @@ const BINARY_OPS = new Map<string, ts.BinaryOperator>([
 
 export interface SignalCall {
   identifier: ts.Identifier;
-  initialized: boolean;
+}
+
+
+export interface SignalCallOutlining {
+  allocateTarget(call: PotentialSignalCall): ts.Identifier|null;
+
+  resolveTarget(call: PotentialSignalCall): ts.Identifier|null;
+}
+
+
+export enum EmitMode {
+  Regular,
+  Narrowing,
 }
 
 /**
@@ -56,17 +68,17 @@ export interface SignalCall {
  */
 export function astToTypescript(
     ast: AST, maybeResolve: (ast: AST) => (ts.Expression | null),
-    identifySignalCall: (ast: PotentialSignalCall) => SignalCall | null,
-    config: TypeCheckingConfig): ts.Expression {
-  const translator = new AstTranslator(maybeResolve, identifySignalCall, config);
+    signalCallOutlining: SignalCallOutlining, config: TypeCheckingConfig,
+    mode: EmitMode): ts.Expression {
+  const translator = new AstTranslator(maybeResolve, signalCallOutlining, config, mode);
   return translator.translate(ast);
 }
 
 class AstTranslator implements AstVisitor {
   constructor(
       private maybeResolve: (ast: AST) => (ts.Expression | null),
-      private identifySignalCall: (ast: PotentialSignalCall) => SignalCall | null,
-      private config: TypeCheckingConfig) {}
+      private signalCallOutlining: SignalCallOutlining, private config: TypeCheckingConfig,
+      private mode: EmitMode) {}
 
   translate(ast: AST): ts.Expression {
     // Skip over an `ASTWithSource` as its `visit` method calls directly into its ast's `visit`,
@@ -334,12 +346,14 @@ class AstTranslator implements AstVisitor {
 
     let signalAssignmentTarget: ts.Identifier|null = null;
     if (args.length === 0) {
-      const signalCall = this.identifySignalCall(ast);
-      if (signalCall !== null) {
-        if (signalCall.initialized) {
-          return signalCall.identifier;
-        }
-        signalAssignmentTarget = signalCall.identifier;
+      const target = this.signalCallOutlining.resolveTarget(ast);
+      if (target !== null) {
+        // TODO: using the outlined result here means that diagnostics won't be repeated
+        return target;
+      }
+
+      if (this.mode === EmitMode.Narrowing) {
+        signalAssignmentTarget = this.signalCallOutlining.allocateTarget(ast);
       }
     }
 
@@ -384,14 +398,13 @@ class AstTranslator implements AstVisitor {
     const args = ast.args.map(expr => this.translate(expr));
 
     let signalAssignmentTarget: ts.Identifier|null = null;
-    if (args.length === 0) {
-      const signalCall = this.identifySignalCall(ast);
-      if (signalCall !== null) {
-        if (signalCall.initialized) {
-          return signalCall.identifier;
-        }
-        signalAssignmentTarget = signalCall.identifier;
+    if (args.length === 0 && this.mode === EmitMode.Narrowing) {
+      const target = this.signalCallOutlining.resolveTarget(ast);
+      if (target !== null) {
+        // TODO: using the outlined result here means that diagnostics won't be repeated
+        return target;
       }
+      signalAssignmentTarget = this.signalCallOutlining.allocateTarget(ast);
     }
 
     const expr = wrapForDiagnostics(this.translate(ast.receiver));

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/signal_calls.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/signal_calls.ts
@@ -14,115 +14,169 @@ export type PotentialSignalCall = Call|SafeCall;
 
 export function computeSignalCallIdentity(
     call: PotentialSignalCall,
-    recurse: (receiverCall: PotentialSignalCall) => string | null): SignalCallIdentity {
-  const identifier = call.receiver.visit(new SignalCallIdentification(recurse));
+    recurse: (receiverCall: PotentialSignalCall) => string | null,
+    identifyImplicitRead: (expr: AST) => string | null,
+    ): SignalCallIdentity {
+  const identifier =
+      call.receiver.visit(new SignalCallIdentification(recurse, identifyImplicitRead));
   return identifier as SignalCallIdentity;
 }
 
 class SignalCallIdentification implements AstVisitor {
-  constructor(private recurse: (receiverCall: PotentialSignalCall) => string | null) {}
+  constructor(
+      private recurse: (receiverCall: PotentialSignalCall) => string | null,
+      private identifyImplicitRead: (expr: AST) => string | null,
+  ) {}
 
-  visitUnary(ast: Unary): string {
-    return `${ast.operator}${this.forAst(ast.expr)}`;
+  visitUnary(ast: Unary): string|null {
+    const expr = this.forAst(ast.expr);
+    if (expr === null) {
+      return null;
+    }
+    return `${ast.operator}${expr}`;
   }
 
-  visitBinary(ast: Binary): string {
-    return `${this.forAst(ast.left)}${ast.operation}${this.forAst(ast.right)}`;
+  visitBinary(ast: Binary): string|null {
+    const left = this.forAst(ast.left);
+    const right = this.forAst(ast.right);
+    if (left === null || right === null) {
+      return null;
+    }
+    return `${left}${ast.operation}${right}`;
   }
 
-  visitChain(ast: Chain): string {
-    return ast.expressions.map(expr => this.forAst(expr)).join(',');
+  visitChain(ast: Chain): string|null {
+    return null;
   }
 
-  visitConditional(ast: Conditional): string {
-    return `(${this.forAst(ast.condition)} ? ${this.forAst(ast.trueExp)} : ${
-        this.forAst(ast.falseExp)})`;
+  visitConditional(ast: Conditional): string|null {
+    return null;
   }
 
-  visitThisReceiver(ast: ThisReceiver): string {
+  visitThisReceiver(ast: ThisReceiver): string|null {
     return 'this';
   }
 
-  visitImplicitReceiver(ast: ImplicitReceiver): string {
+  visitImplicitReceiver(ast: ImplicitReceiver): string|null {
     return 'this';
   }
 
-  visitInterpolation(ast: Interpolation): string {
-    return ast.expressions.map(expr => this.forAst(expr)).join(',');
+  visitInterpolation(ast: Interpolation): string|null {
+    return null;
   }
 
-  visitKeyedRead(ast: KeyedRead): string {
-    return `${this.forAst(ast.receiver)}[${this.forAst(ast.key)}]`;
+  visitKeyedRead(ast: KeyedRead): string|null {
+    const receiver = this.forAst(ast.receiver);
+    const key = this.forAst(ast.key);
+    if (receiver === null || key === null) {
+      return null;
+    }
+    return `${receiver}[${key}]`;
   }
 
-  visitKeyedWrite(ast: KeyedWrite): string {
-    return `${this.forAst(ast.receiver)}[${this.forAst(ast.key)}] = ${this.forAst(ast.value)}`;
+  visitKeyedWrite(ast: KeyedWrite): string|null {
+    return null;
   }
 
-  visitLiteralArray(ast: LiteralArray): string {
-    const values = ast.expressions.map(expr => this.forAst(expr)).join(',');
-    return `[${values}]`;
+  visitLiteralArray(ast: LiteralArray): string|null {
+    return null;
   }
 
-  visitLiteralMap(ast: LiteralMap): string {
-    const values = ast.keys.map((key, i) => `${key}: ${this.forAst(ast.values[i])}`).join(',');
-    return `{${values}}`;
+  visitLiteralMap(ast: LiteralMap): string|null {
+    return null;
   }
 
-  visitLiteralPrimitive(ast: LiteralPrimitive): string {
+  visitLiteralPrimitive(ast: LiteralPrimitive): string|null {
     return `${ast.value}`;
   }
 
-  visitPipe(ast: BindingPipe): string {
-    const args = ast.args.map(expr => this.forAst(expr)).join(' : ');
-    return `${this.forAst(ast.exp)} | ${ast.name}: ${args}`;
+  visitPipe(ast: BindingPipe): string|null {
+    return null;
   }
 
-  visitPrefixNot(ast: PrefixNot): string {
-    return `!${this.forAst(ast.expression)}`;
+  visitPrefixNot(ast: PrefixNot): string|null {
+    const expression = this.forAst(ast.expression);
+    if (expression === null) {
+      return expression;
+    }
+    return `!${expression}`;
   }
 
-  visitNonNullAssert(ast: NonNullAssert): string {
-    return `${this.forAst(ast.expression)}!`;
+  visitNonNullAssert(ast: NonNullAssert): string|null {
+    return this.forAst(ast.expression);
   }
 
-  visitPropertyRead(ast: PropertyRead): string {
-    return `${this.forAst(ast.receiver)}.${ast.name}`;
+  visitPropertyRead(ast: PropertyRead): string|null {
+    const receiver = this.identifyReceiver(ast);
+    if (receiver === null) {
+      return null;
+    }
+    return `${receiver}.${ast.name}`;
   }
 
-  visitPropertyWrite(ast: PropertyWrite): string {
-    return `${this.forAst(ast.receiver)}.${ast.name} = ${this.forAst(ast.value)}`;
+  visitPropertyWrite(ast: PropertyWrite): string|null {
+    return null;
   }
 
-  visitSafePropertyRead(ast: SafePropertyRead): string {
-    return `${this.forAst(ast.receiver)}?.${ast.name}`;
+  visitSafePropertyRead(ast: SafePropertyRead): string|null {
+    const receiver = this.identifyReceiver(ast);
+    if (receiver === null) {
+      return null;
+    }
+    return `${receiver}?.${ast.name}`;
   }
 
-  visitSafeKeyedRead(ast: SafeKeyedRead): string {
-    return `${this.forAst(ast.receiver)}?.[${this.forAst(ast.key)}]`;
+  visitSafeKeyedRead(ast: SafeKeyedRead): string|null {
+    const receiver = this.forAst(ast.receiver)
+    if (receiver === null) {
+      return null;
+    }
+    return `${receiver}?.[${this.forAst(ast.key)}]`;
   }
 
-  visitCall(ast: Call): string {
-    const args = ast.args.map(expr => this.forAst(expr)).join(',');
-    return `${this.forAst(ast.receiver)}(${args})`;
+  visitCall(ast: Call): string|null {
+    if (ast.args.length > 0) {
+      return null;
+    }
+    const receiver = this.forAst(ast.receiver);
+    if (receiver === null) {
+      return null;
+    }
+    return `${receiver}()`;
   }
 
-  visitSafeCall(ast: SafeCall): string {
-    const args = ast.args.map(expr => this.forAst(expr)).join(',');
-    return `${this.forAst(ast.receiver)}?.(${args})`;
+  visitSafeCall(ast: SafeCall): string|null {
+    if (ast.args.length > 0) {
+      return null;
+    }
+    const receiver = this.forAst(ast.receiver);
+    if (receiver === null) {
+      return null;
+    }
+    return `${receiver}?.()`;
   }
 
-  visitASTWithSource(ast: ASTWithSource): string {
+  visitASTWithSource(ast: ASTWithSource): string|null {
     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}`;
-    //   }
-    // }
+  private identifyReceiver(ast: PropertyRead|SafePropertyRead): string|null {
+    if (ast.receiver instanceof ImplicitReceiver && !(ast.receiver instanceof ThisReceiver)) {
+      const implicitIdentity = this.identifyImplicitRead(ast);
+      if (implicitIdentity !== null) {
+        return implicitIdentity;
+      }
+    }
+    return this.forAst(ast.receiver);
+  }
+
+  private forAst(ast: AST): string|null {
+    if (ast instanceof Call || ast instanceof SafeCall) {
+      const result = this.recurse(ast);
+      if (result !== null) {
+        return `ɵ${result}`;
+      }
+    }
     return ast.visit(this);
   }
 }
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts
@@ -131,7 +131,7 @@ export function tsCreateVariable(
       /* initializer */ initializer);
   return ts.factory.createVariableStatement(
       /* modifiers */ undefined,
-      /* declarationList */ ts.factory.createVariableDeclarationList([decl], ts.NodeFlags.Const));
+      /* declarationList */[decl]);
 }
 
 /**