fix(ivy): emulate a View Engine type-checking bug with safe navigation (#35462)

In its default compatibility mode, the Ivy template type-checker attempts to
emulate the View Engine default mode as accurately as is possible. This
commit addresses a gap in this compatibility that stems from a View Engine
type-checking bug.

Consider two template expressions:

```html
{{ obj?.field }}
{{ fn()?.field }}
```

and suppose that the type of `obj` and `fn()` are the same - both return
either `null` or an object with a `field` property.

Under View Engine, these type-check differently. The `obj` case will catch
if the object type (when not null) does not have a `field` property, while
the `fn()` case will not. This is due to how View Engine represents safe
navigations:

```typescript
// for the 'obj' case
(obj == null ? null as any : obj.field)

// for the 'fn()' case
let tmp: any;
((tmp = fn()) == null ? null as any : tmp.field)
```

Because View Engine uses the same code generation backend as it does to
produce the runtime code for this expression, it uses a ternary for safe
navigation, with a temporary variable to avoid invoking 'fn()' twice. The
type of this temporary variable is 'any', however, which causes the
`tmp.field` check to be meaningless.

Previously, the Ivy template type-checker in compatibility mode assumed that
`fn()?.field` would always check for the presence of 'field' on the non-null
result of `fn()`. This commit emulates the View Engine bug in Ivy's
compatibility mode, so an 'any' type will be inferred under the same
conditions.

As part of this fix, a new format for safe navigation operations in template
type-checking code is introduced. This is based on the realization that
ternary based narrowing is unnecessary.

For the `fn()` case in strict mode, Ivy now generates:

```typescript
(null as any ? fn()!.field : undefined)
```

This effectively uses the ternary operator as a type "or" operation. The
resulting type will be a union of the type of `fn()!.field` with
`undefined`.

For the `fn()` case in compatibility mode, Ivy now emulates the bug with:

```typescript
(fn() as any).field
```

The cast expression includes the call to `fn()` and allows it to be checked
while still returning a type of `any` from the expression.

For the `obj` case in compatibility mode, Ivy now generates:

```typescript
(obj!.field as any)
```

This cast expression still returns `any` for its type, but will check for
the existence of `field` on the type of `obj!`.

PR Close #35462

Author: alxhub

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

@@ -219,39 +219,100 @@ class AstTranslator implements AstVisitor {
   visitQuote(ast: Quote): never { throw new Error('Method not implemented.'); }
 
   visitSafeMethodCall(ast: SafeMethodCall): ts.Expression {
-    // See the comment in SafePropertyRead above for an explanation of the need for the non-null
-    // assertion here.
+    // See the comments in SafePropertyRead above for an explanation of the cases here.
+    let node: ts.Expression;
     const receiver = wrapForDiagnostics(this.translate(ast.receiver));
-    const guard = ts.getMutableClone(receiver);
-    ignoreDiagnostics(guard);
-    const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name);
     const args = ast.args.map(expr => this.translate(expr));
-    const expr = ts.createCall(method, undefined, args);
-    const whenNull = this.config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY;
-    const node = safeTernary(guard, expr, whenNull);
+    if (this.config.strictSafeNavigationTypes) {
+      // "a?.method(...)" becomes (null as any ? a!.method(...) : undefined)
+      const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name);
+      const call = ts.createCall(method, undefined, args);
+      node = ts.createParen(ts.createConditional(NULL_AS_ANY, call, UNDEFINED));
+    } else if (VeSafeLhsInferenceBugDetector.veWillInferAnyFor(ast)) {
+      // "a?.method(...)" becomes (a as any).method(...)
+      const method = ts.createPropertyAccess(tsCastToAny(receiver), ast.name);
+      node = ts.createCall(method, undefined, args);
+    } else {
+      // "a?.method(...)" becomes (a!.method(...) as any)
+      const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name);
+      node = tsCastToAny(ts.createCall(method, undefined, args));
+    }
     addParseSpanInfo(node, ast.sourceSpan);
     return node;
   }
 
   visitSafePropertyRead(ast: SafePropertyRead): ts.Expression {
-    // A safe property expression a?.b takes the form `(a != null ? a!.b : whenNull)`, where
-    // whenNull is either of type 'any' or or 'undefined' depending on strictness. The non-null
-    // assertion is necessary because in practice 'a' may be a method call expression, which won't
-    // have a narrowed type when repeated in the ternary true branch.
+    let node: ts.Expression;
     const receiver = wrapForDiagnostics(this.translate(ast.receiver));
-    const guard = ts.getMutableClone(receiver);
-    ignoreDiagnostics(guard);
-    const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name);
-    const whenNull = this.config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY;
-    const node = safeTernary(guard, expr, whenNull);
+    // The form of safe property reads depends on whether strictness is in use.
+    if (this.config.strictSafeNavigationTypes) {
+      // Basically, the return here is either the type of the complete expression with a null-safe
+      // property read, or `undefined`. So a ternary is used to create an "or" type:
+      // "a?.b" becomes (null as any ? a!.b : undefined)
+      // The type of this expression is (typeof a!.b) | undefined, which is exactly as desired.
+      const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name);
+      node = ts.createParen(ts.createConditional(NULL_AS_ANY, expr, UNDEFINED));
+    } else if (VeSafeLhsInferenceBugDetector.veWillInferAnyFor(ast)) {
+      // Emulate a View Engine bug where 'any' is inferred for the left-hand side of the safe
+      // navigation operation. With this bug, the type of the left-hand side is regarded as any.
+      // Therefore, the left-hand side only needs repeating in the output (to validate it), and then
+      // 'any' is used for the rest of the expression. This is done using a comma operator:
+      // "a?.b" becomes (a as any).b, which will of course have type 'any'.
+      node = ts.createPropertyAccess(tsCastToAny(receiver), ast.name);
+    } else {
+      // The View Engine bug isn't active, so check the entire type of the expression, but the final
+      // result is still inferred as `any`.
+      // "a?.b" becomes (a!.b as any)
+      const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name);
+      node = tsCastToAny(expr);
+    }
     addParseSpanInfo(node, ast.sourceSpan);
     return node;
   }
 }
 
-function safeTernary(
-    lhs: ts.Expression, whenNotNull: ts.Expression, whenNull: ts.Expression): ts.Expression {
-  const notNullComp = ts.createBinary(lhs, ts.SyntaxKind.ExclamationEqualsToken, ts.createNull());
-  const ternary = ts.createConditional(notNullComp, whenNotNull, whenNull);
-  return ts.createParen(ternary);
+/**
+ * Checks whether View Engine will infer a type of 'any' for the left-hand side of a safe navigation
+ * operation.
+ *
+ * In View Engine's template type-checker, certain receivers of safe navigation operations will
+ * cause a temporary variable to be allocated as part of the checking expression, to save the value
+ * of the receiver and use it more than once in the expression. This temporary variable has type
+ * 'any'. In practice, this means certain receivers cause View Engine to not check the full
+ * expression, and other receivers will receive more complete checking.
+ *
+ * For compatibility, this logic is adapted from View Engine's expression_converter.ts so that the
+ * Ivy checker can emulate this bug when needed.
+ */
+class VeSafeLhsInferenceBugDetector implements AstVisitor {
+  private static SINGLETON = new VeSafeLhsInferenceBugDetector();
+
+  static veWillInferAnyFor(ast: SafeMethodCall|SafePropertyRead) {
+    return ast.receiver.visit(VeSafeLhsInferenceBugDetector.SINGLETON);
+  }
+
+  visitBinary(ast: Binary): boolean { return ast.left.visit(this) || ast.right.visit(this); }
+  visitChain(ast: Chain): boolean { return false; }
+  visitConditional(ast: Conditional): boolean {
+    return ast.condition.visit(this) || ast.trueExp.visit(this) || ast.falseExp.visit(this);
+  }
+  visitFunctionCall(ast: FunctionCall): boolean { return true; }
+  visitImplicitReceiver(ast: ImplicitReceiver): boolean { return false; }
+  visitInterpolation(ast: Interpolation): boolean {
+    return ast.expressions.some(exp => exp.visit(this));
+  }
+  visitKeyedRead(ast: KeyedRead): boolean { return false; }
+  visitKeyedWrite(ast: KeyedWrite): boolean { return false; }
+  visitLiteralArray(ast: LiteralArray): boolean { return true; }
+  visitLiteralMap(ast: LiteralMap): boolean { return true; }
+  visitLiteralPrimitive(ast: LiteralPrimitive): boolean { return false; }
+  visitMethodCall(ast: MethodCall): boolean { return true; }
+  visitPipe(ast: BindingPipe): boolean { return true; }
+  visitPrefixNot(ast: PrefixNot): boolean { return ast.expression.visit(this); }
+  visitNonNullAssert(ast: PrefixNot): boolean { return ast.expression.visit(this); }
+  visitPropertyRead(ast: PropertyRead): boolean { return false; }
+  visitPropertyWrite(ast: PropertyWrite): boolean { return false; }
+  visitQuote(ast: Quote): boolean { return false; }
+  visitSafeMethodCall(ast: SafeMethodCall): boolean { return true; }
+  visitSafePropertyRead(ast: SafePropertyRead): boolean { return false; }
 }

packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts

@@ -97,15 +97,14 @@ describe('type check blocks diagnostics', () => {
     it('should annotate safe property access', () => {
       const TEMPLATE = `{{ a?.b }}`;
       expect(tcbWithSpans(TEMPLATE))
-          .toContain(
-              '(((ctx).a /*3,4*/) /*ignore*/ != null ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/;');
+          .toContain('((null as any) ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/');
     });
 
     it('should annotate safe method calls', () => {
       const TEMPLATE = `{{ a?.method(b) }}`;
       expect(tcbWithSpans(TEMPLATE))
           .toContain(
-              '(((ctx).a /*3,4*/) /*ignore*/ != null ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/;');
+              '((null as any) ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/');
     });
 
     it('should annotate $any casts', () => {

packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts

@@ -561,15 +561,31 @@ describe('type check blocks', () => {
 
       it('should use undefined for safe navigation operations when enabled', () => {
         const block = tcb(TEMPLATE, DIRECTIVES);
-        expect(block).toContain('(((ctx).a) != null ? ((ctx).a)!.method() : undefined)');
-        expect(block).toContain('(((ctx).a) != null ? ((ctx).a)!.b : undefined)');
+        expect(block).toContain('((null as any) ? ((ctx).a)!.method() : undefined)');
+        expect(block).toContain('((null as any) ? ((ctx).a)!.b : undefined)');
       });
       it('should use an \'any\' type for safe navigation operations when disabled', () => {
         const DISABLED_CONFIG:
             TypeCheckingConfig = {...BASE_CONFIG, strictSafeNavigationTypes: false};
         const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
-        expect(block).toContain('(((ctx).a) != null ? ((ctx).a)!.method() : null as any)');
-        expect(block).toContain('(((ctx).a) != null ? ((ctx).a)!.b : null as any)');
+        expect(block).toContain('(((ctx).a)!.method() as any)');
+        expect(block).toContain('(((ctx).a)!.b as any)');
+      });
+    });
+
+    describe('config.strictSafeNavigationTypes (View Engine bug emulation)', () => {
+      const TEMPLATE = `{{a.method()?.b}} {{a()?.method()}}`;
+      it('should check the presence of a property/method on the receiver when enabled', () => {
+        const block = tcb(TEMPLATE, DIRECTIVES);
+        expect(block).toContain('((null as any) ? (((ctx).a).method())!.b : undefined)');
+        expect(block).toContain('((null as any) ? ((ctx).a())!.method() : undefined)');
+      });
+      it('should not check the presence of a property/method on the receiver when disabled', () => {
+        const DISABLED_CONFIG:
+            TypeCheckingConfig = {...BASE_CONFIG, strictSafeNavigationTypes: false};
+        const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
+        expect(block).toContain('((((ctx).a).method()) as any).b');
+        expect(block).toContain('(((ctx).a()) as any).method()');
       });
     });