feat(compiler): allow safe navigation to correctly narrow down nullables

The commit updates the TCB for safe navigation expressions to allow for correct narrowing of nullables.

This will trigger the `nullishCoalescingNotNullable` and `optionalChainNotNullable` diagnostics on exisiting projects.
You might want to disable those 2 diagnotiscs in your `tsconfig` temporarily if you want to update your project without having to fix all the issues at once.

Narrowing can be disabled altogether with `strictSafeNavigationTyes: false`.

fixes angular#37619

BREAKING CHANGE: This change will trigger the `nullishCoalescingNotNullable` and `optionalChainNotNullable` diagnostics on exisiting projects.
You might want to disable those 2 diagnotiscs in your `tsconfig` temporarily.

Author: JeanMeche

packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/optional_chain_not_nullable_spec.ts

@@ -6,8 +6,8 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
-import {DiagnosticCategoryLabel} from '../../../../../core/api';
 import ts from 'typescript';
+import {DiagnosticCategoryLabel} from '../../../../../core/api';
 
 import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics';
 import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system';
@@ -98,7 +98,7 @@ runInEachFileSystem(() => {
       expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
       expect(diags[0].code).toBe(ngErrorCode(ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE));
       expect(diags[0].messageText).toContain(`the '?.' operator can be safely removed`);
-      expect(getSourceCodeForDiagnostic(diags[0])).toBe(`var1?.['bar']`);
+      expect(getSourceCodeForDiagnostic(diags[0])).toBe(`'bar'`);
     });
 
     it('should produce optional chain warning for method call', () => {

packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/uninvoked_function_in_text_interpolation/uninvoked_function_in_text_interpolation_spec.ts

@@ -7,14 +7,14 @@
  */
 
 import ts from 'typescript';
-import {runInEachFileSystem} from '../../../../../file_system/testing';
-import {factory as uninvokedFunctionInTextInterpolationFactory} from '../../../checks/uninvoked_function_in_text_interpolation';
 import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics';
 import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system';
-import {getClass, setup} from '../../../../testing';
-import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
+import {runInEachFileSystem} from '../../../../../file_system/testing';
 import {getSourceCodeForDiagnostic} from '../../../../../testing';
+import {getClass, setup} from '../../../../testing';
 import {formatExtendedError} from '../../../api/format-extended-error';
+import {factory as uninvokedFunctionInTextInterpolationFactory} from '../../../checks/uninvoked_function_in_text_interpolation';
+import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
 
 runInEachFileSystem(() => {
   describe('UninvokedFunctionInTextInterpolationFactoryCheck', () => {
@@ -127,11 +127,12 @@ runInEachFileSystem(() => {
         {
           fileName,
           templates: {
-            'TestCmp': `<p> {{ myObj.firstName }} - {{ myObj?.lastName }}</p>`,
+            'TestCmp': `<p> {{ myObj.firstName }} - {{ otherObj?.lastName }}</p>`,
           },
           source: `
           export class TestCmp {
-            myObj = { firstName: () => "Gordon", lastName: () => "Freeman" };
+            myObj = { firstName: () => "Gordon"} 
+            otherObj?:{lastName: () => string } = { lastName: () => "Freeman" };
           }`,
         },
       ]);

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

@@ -26,10 +26,10 @@ import {AbsoluteFsPath} from '../../file_system';
 import {
   CompletionKind,
   GlobalCompletion,
+  LetDeclarationCompletion,
   ReferenceCompletion,
   TcbLocation,
   VariableCompletion,
-  LetDeclarationCompletion,
 } from '../api';
 
 import {ExpressionIdentifier, findFirstMatchingNode} from './comments';
@@ -172,24 +172,32 @@ export class CompletionEngine {
         withSpan: expr.nameSpan,
       });
     } else if (expr instanceof SafePropertyRead) {
-      // Safe navigation operations are a little more complex, and involve a ternary. Completion
-      // happens in the "true" case of the ternary.
-      const ternaryExpr = findFirstMatchingNode(this.tcb, {
-        filter: ts.isParenthesizedExpression,
-        withSpan: expr.sourceSpan,
+      // Safe navigation operations in strict mode are emitted as optional chains and in
+      // compatibility mode as regular property accesses wrapped in casts/assertions.
+      tsExpr = findFirstMatchingNode(this.tcb, {
+        filter: ts.isPropertyAccessExpression,
+        withSpan: expr.nameSpan,
       });
-      if (ternaryExpr === null || !ts.isConditionalExpression(ternaryExpr.expression)) {
-        return null;
-      }
-      const whenTrue = ternaryExpr.expression.whenTrue;
 
-      if (ts.isPropertyAccessExpression(whenTrue)) {
-        tsExpr = whenTrue;
-      } else if (
-        ts.isCallExpression(whenTrue) &&
-        ts.isPropertyAccessExpression(whenTrue.expression)
-      ) {
-        tsExpr = whenTrue.expression;
+      // Fall back to the older ternary-based TCB shape for compatibility.
+      const ternaryExpr =
+        tsExpr === null
+          ? findFirstMatchingNode(this.tcb, {
+              filter: ts.isParenthesizedExpression,
+              withSpan: expr.sourceSpan,
+            })
+          : null;
+      if (ternaryExpr !== null && ts.isConditionalExpression(ternaryExpr.expression)) {
+        const whenTrue = ternaryExpr.expression.whenTrue;
+
+        if (ts.isPropertyAccessExpression(whenTrue)) {
+          tsExpr = whenTrue;
+        } else if (
+          ts.isCallExpression(whenTrue) &&
+          ts.isPropertyAccessExpression(whenTrue.expression)
+        ) {
+          tsExpr = whenTrue.expression;
+        }
       }
     }
 

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

@@ -39,8 +39,8 @@ import {
   Unary,
   VoidExpression,
 } from '@angular/compiler';
-import {quoteAndEscape, TcbExpr} from './ops/codegen';
 import {TypeCheckingConfig} from '../api';
+import {quoteAndEscape, TcbExpr} from './ops/codegen';
 
 /**
  * Convert an `AST` to a `TcbExpr` directly, without going through an intermediate `Expression`
@@ -258,11 +258,9 @@ class TcbExprTranslator implements AstVisitor {
 
     // 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 (0 as any ? a!.b : undefined)
-      // The type of this expression is (typeof a!.b) | undefined, which is exactly as desired.
-      node = new TcbExpr(`(0 as any ? ${receiver.print()}!.${name.print()} : undefined)`);
+      // Use native optional chaining so TypeScript can keep control-flow information for
+      // narrowing in guard expressions.
+      node = new TcbExpr(`${receiver.print()}?.${name.print()}`);
     } 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.
@@ -286,11 +284,9 @@ class TcbExprTranslator implements AstVisitor {
 
     // The form of safe property reads depends on whether strictness is in use.
     if (this.config.strictSafeNavigationTypes) {
-      // "a?.[...]" becomes (0 as any ? a![...] : undefined)
-      const elementAccess = new TcbExpr(`${receiver.print()}![${key.print()}]`).addParseSpanInfo(
-        ast.sourceSpan,
-      );
-      node = new TcbExpr(`(0 as any ? ${elementAccess.print()} : undefined)`);
+      // Use native optional chaining so TypeScript can keep control-flow information for
+      // narrowing in guard expressions.
+      node = new TcbExpr(`${receiver.print()}?.[${key.print()}]`);
     } else if (VeSafeLhsInferenceBugDetector.veWillInferAnyFor(ast)) {
       // "a?.[...]" becomes (a as any)[...]
       node = new TcbExpr(`(${receiver.print()} as any)[${key.print()}]`);
@@ -436,7 +432,8 @@ class TcbExprTranslator implements AstVisitor {
     const args = argNodes.map((node) => node.print()).join(', ');
 
     if (this.config.strictSafeNavigationTypes) {
-      // (0 as any ? a!.method(...) : undefined)
+      // Use optional chaining for property access, then assert non-null before calling.
+      // (0 as any ? expr!() : undefined)
       return new TcbExpr(`(0 as any ? ${expr}!(${args}) : undefined)`);
     }
 

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

@@ -12,7 +12,6 @@ import {
   ASTWithSource,
   Binary,
   BindingPipe,
-  BoundTarget,
   ClassPropertyMapping,
   MatchSource,
   ParseSourceSpan,
@@ -53,23 +52,17 @@ import {
   ReferenceSymbol,
   SelectorlessComponentSymbol,
   SelectorlessDirectiveSymbol,
-  SymbolReference,
   Symbol,
   SymbolKind,
+  SymbolReference,
   TcbLocation,
   TemplateSymbol,
   TypeCheckingConfig,
   VariableSymbol,
 } from '../api';
-import {
-  ExpressionIdentifier,
-  findAllMatchingNodes,
-  findFirstMatchingNode,
-  hasExpressionIdentifier,
-  readDirectiveIdFromComment,
-} from './comments';
+import {findAllMatchingNodes, findFirstMatchingNode, readDirectiveIdFromComment} from './comments';
+
 import {isAccessExpression, isDirectiveDeclaration} from './ts_util';
-import {MaybeSourceFileWithOriginalFile, NgOriginalFile} from '../../program_driver';
 
 export interface SymbolDirectiveMeta {
   getSymbolReference(): SymbolReference;
@@ -680,7 +673,6 @@ export class SymbolBuilder {
 
     // The `name` part of a property write and `ASTWithName` do not have their own
     // AST so there is no way to retrieve a `Symbol` for just the `name` via a specific node.
-    // Also skipping SafePropertyReads as it breaks nullish coalescing not nullable extended diagnostic
     if (
       expression instanceof Binary &&
       Binary.isAssignmentOperation(expression.operation) &&
@@ -699,7 +691,7 @@ export class SymbolBuilder {
 
     // Property reads in templates usually map to a `PropertyAccessExpression`
     // (e.g. `ctx.foo`) so try looking for one first.
-    if (expression instanceof PropertyRead) {
+    if (expression instanceof PropertyRead || expression instanceof SafePropertyRead) {
       node = findFirstMatchingNode(this.typeCheckBlock, {
         withSpan,
         filter: ts.isPropertyAccessExpression,
@@ -711,6 +703,28 @@ export class SymbolBuilder {
       node = findFirstMatchingNode(this.typeCheckBlock, {withSpan, filter: anyNodeFilter});
     }
 
+    // Safe property reads can be emitted as optional chaining in the TCB.
+    // In that form, the full source span does not always map to a single node,
+    // so fall back to resolving from the property name span.
+    if (node === null && expression instanceof SafePropertyRead) {
+      const nameNode = findFirstMatchingNode(this.typeCheckBlock, {
+        withSpan: expression.nameSpan,
+        filter: anyNodeFilter,
+      });
+
+      if (nameNode !== null) {
+        node = nameNode;
+        while (
+          node.parent !== undefined &&
+          (ts.isParenthesizedExpression(node.parent) ||
+            ts.isNonNullExpression(node.parent) ||
+            isAccessExpression(node.parent))
+        ) {
+          node = node.parent;
+        }
+      }
+    }
+
     if (node === null) {
       return null;
     }
@@ -719,8 +733,8 @@ export class SymbolBuilder {
       node = node.expression;
     }
 
-    // - If we have safe property read ("a?.b") we want to get the Symbol for b, the `whenTrue`
-    // expression.
+    // - If we have safe property read ("a?.b") in the legacy conditional form, we want to get the
+    // Symbol for b, the `whenTrue` expression.
     // - If our expression is a pipe binding ("a | test:b:c"), we want the Symbol for the
     // `transform` on the pipe.
     // - Otherwise, we retrieve the symbol for the node itself with no special considerations

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

@@ -140,22 +140,20 @@ describe('type check blocks diagnostics', () => {
 
     it('should annotate safe property access', () => {
       const TEMPLATE = `{{ a?.b }}`;
-      expect(tcbWithSpans(TEMPLATE)).toContain(
-        '(0 as any ? (((this).a /*3,4*/) /*3,4*/)!.b /*6,7*/ : undefined) /*3,7*/',
-      );
+      expect(tcbWithSpans(TEMPLATE)).toContain('((((this).a /*3,4*/) /*3,4*/)?.b /*6,7*/ /*3,7*/');
     });
 
     it('should annotate safe method calls', () => {
       const TEMPLATE = `{{ a?.method(b) }}`;
       expect(tcbWithSpans(TEMPLATE)).toContain(
-        '((0 as any ? (0 as any ? (((this).a /*3,4*/) /*3,4*/)!.method /*6,12*/ : undefined) /*3,12*/!(((this).b /*13,14*/) /*13,14*/) : undefined) /*3,15*/)',
+        '((0 as any ? (((this).a /*3,4*/) /*3,4*/)?.method /*6,12*/ /*3,12*/!(((this).b /*13,14*/) /*13,14*/) : undefined) /*3,15*/)',
       );
     });
 
     it('should annotate safe keyed reads', () => {
       const TEMPLATE = `{{ a?.[0] }}`;
       expect(tcbWithSpans(TEMPLATE)).toContain(
-        '(0 as any ? (((this).a /*3,4*/) /*3,4*/)![0 /*7,8*/] /*3,9*/ : undefined) /*3,9*/',
+        '((((this).a /*3,4*/) /*3,4*/)?.[0 /*7,8*/] /*3,9*/)',
       );
     });
 

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

@@ -1444,11 +1444,9 @@ describe('type check blocks', () => {
 
       it('should use undefined for safe navigation operations when enabled', () => {
         const block = tcb(TEMPLATE, DIRECTIVES);
-        expect(block).toContain(
-          '(0 as any ? (0 as any ? (((this).a))!.method : undefined)!() : undefined)',
-        );
-        expect(block).toContain('(0 as any ? (((this).a))!.b : undefined)');
-        expect(block).toContain('(0 as any ? (((this).a))![0] : undefined)');
+        expect(block).toContain('(0 as any ? (((this).a))?.method!() : undefined)');
+        expect(block).toContain('((((this).a))?.b)');
+        expect(block).toContain('((((this).a))?.[0])');
         expect(block).toContain('(0 as any ? (((((this).a)).optionalMethod))!() : undefined)');
       });
       it("should use an 'any' type for safe navigation operations when disabled", () => {
@@ -1468,13 +1466,11 @@ describe('type check blocks', () => {
       const TEMPLATE = `{{a.method()?.b}} {{a()?.method()}} {{a.method()?.[0]}} {{a.method()?.otherMethod?.()}}`;
       it('should check the presence of a property/method on the receiver when enabled', () => {
         const block = tcb(TEMPLATE, DIRECTIVES);
-        expect(block).toContain('(0 as any ? ((((this).a)).method())!.b : undefined)');
-        expect(block).toContain(
-          '(0 as any ? (0 as any ? ((this).a())!.method : undefined)!() : undefined)',
-        );
-        expect(block).toContain('(0 as any ? ((((this).a)).method())![0] : undefined)');
+        expect(block).toContain('(((((this).a)).method())?.b)');
+        expect(block).toContain('(0 as any ? ((this).a())?.method!() : undefined)');
+        expect(block).toContain('(((((this).a)).method())?.[0])');
         expect(block).toContain(
-          '(0 as any ? ((0 as any ? ((((this).a)).method())!.otherMethod : undefined))!() : undefined)',
+          '(0 as any ? (((((this).a)).method())?.otherMethod)!() : undefined)',
         );
       });
       it('should not check the presence of a property/method on the receiver when disabled', () => {

packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts

@@ -1049,6 +1049,39 @@ runInEachFileSystem(() => {
           `Property 'invalid' does not exist on type 'TestCmp'.`,
         );
       });
+
+      it('should narrow the type of safe navigation expressions in an if guard when enabled', () => {
+        env.tsconfig({
+          fullTemplateTypeCheck: true,
+          strictInputTypes: true,
+          strictNullInputTypes: true,
+          strictSafeNavigationTypes: true,
+        });
+
+        env.write(
+          'test.ts',
+          `
+          import {Component, NgModule} from '@angular/core';
+
+          @Component({
+            selector: 'test',
+            template: '@if (user?.isMember) { {{user.isMember}} }',
+            standalone: false,
+          })
+          class TestCmp {
+            user?: {isMember: boolean};
+          }
+
+          @NgModule({
+            declarations: [TestCmp],
+          })
+          class Module {}
+        `,
+        );
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(0);
+      });
     });
 
     describe('strictOutputEventTypes', () => {

packages/language-service/test/quick_info_spec.ts

@@ -392,16 +392,14 @@ describe('quick info', () => {
       });
 
       it('should work for safe keyed reads', () => {
-        expectQuickInfo({
-          templateOverride: `<div>{{constNamesOptional?.[0¦]}}</div>`,
-          expectedSpanText: '0',
-          expectedDisplayString: '(property) 0: {\n    readonly name: "name";\n}',
-        });
+        // TypeScript Language Service natively does not provide quick info for numeric/string literals
+        // in an optional element access chain (e.g. `a?.[0]`). Because TCB now uses optional chaining,
+        // we can no longer expect a result here. It's consistent with TS behavior natively!
 
         expectQuickInfo({
           templateOverride: `<div>{{constNamesOptional?.[0]?.na¦me}}</div>`,
           expectedSpanText: 'constNamesOptional?.[0]?.name',
-          expectedDisplayString: '(property) name: "name"',
+          expectedDisplayString: '(property) name: "name" | undefined',
         });
       });