From 47fcbc470462192c4f9e273d8dce8b353d5baaa2 Mon Sep 17 00:00:00 2001 From: Matthieu Riegler Date: Tue, 31 Mar 2026 23:54:03 +0200 Subject: [PATCH] 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 #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. --- .../optional_chain_not_nullable_spec.ts | 4 +- ...ked_function_in_text_interpolation_spec.ts | 13 +++--- .../src/ngtsc/typecheck/src/completion.ts | 42 +++++++++++-------- .../src/ngtsc/typecheck/src/expression.ts | 21 ++++------ .../typecheck/src/template_symbol_builder.ts | 42 ++++++++++++------- .../typecheck/test/span_comments_spec.ts | 8 ++-- .../typecheck/test/type_check_block_spec.ts | 18 ++++---- .../test/ngtsc/template_typecheck_spec.ts | 33 +++++++++++++++ .../language-service/test/quick_info_spec.ts | 10 ++--- 9 files changed, 118 insertions(+), 73 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/optional_chain_not_nullable_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/optional_chain_not_nullable_spec.ts index 36b744ac55e..c0466fda304 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/optional_chain_not_nullable_spec.ts +++ b/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', () => { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/uninvoked_function_in_text_interpolation/uninvoked_function_in_text_interpolation_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/uninvoked_function_in_text_interpolation/uninvoked_function_in_text_interpolation_spec.ts index 76ae3ff4a1e..af417b941b8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/uninvoked_function_in_text_interpolation/uninvoked_function_in_text_interpolation_spec.ts +++ b/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': `

{{ myObj.firstName }} - {{ myObj?.lastName }}

`, + 'TestCmp': `

{{ myObj.firstName }} - {{ otherObj?.lastName }}

`, }, source: ` export class TestCmp { - myObj = { firstName: () => "Gordon", lastName: () => "Freeman" }; + myObj = { firstName: () => "Gordon"} + otherObj?:{lastName: () => string } = { lastName: () => "Freeman" }; }`, }, ]); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/completion.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/completion.ts index e6d9b182fac..d7d1cb2c706 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/completion.ts +++ b/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; + } } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts index c6a6b83ebe4..66979ea75f9 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts +++ b/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)`); } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts index 3e02e1d605d..e40aeec9a7a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts +++ b/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 diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts index e211640e554..d4e7c0dd3c3 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts +++ b/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*/)', ); }); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index aee486d0fbe..daeec348901 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/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('(((((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 : undefined)!() : undefined)', - ); - expect(block).toContain('(0 as any ? ((((this).a)).method())![0] : undefined)'); - 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', () => { diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 405f289fd1f..0c1d09b043d 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/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', () => { diff --git a/packages/language-service/test/quick_info_spec.ts b/packages/language-service/test/quick_info_spec.ts index 9bb27b4e25e..7d6cddfdea8 100644 --- a/packages/language-service/test/quick_info_spec.ts +++ b/packages/language-service/test/quick_info_spec.ts @@ -392,16 +392,14 @@ describe('quick info', () => { }); it('should work for safe keyed reads', () => { - expectQuickInfo({ - templateOverride: `
{{constNamesOptional?.[0¦]}}
`, - 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: `
{{constNamesOptional?.[0]?.na¦me}}
`, expectedSpanText: 'constNamesOptional?.[0]?.name', - expectedDisplayString: '(property) name: "name"', + expectedDisplayString: '(property) name: "name" | undefined', }); });