diff --git a/packages/core/schematics/BUILD.bazel b/packages/core/schematics/BUILD.bazel index 6bcce023052..c25680e2089 100644 --- a/packages/core/schematics/BUILD.bazel +++ b/packages/core/schematics/BUILD.bazel @@ -145,6 +145,10 @@ bundle_entrypoints = [ "model-output", "packages/core/schematics/migrations/model-output/index.js", ], + [ + "safe-optional-chaining", + "packages/core/schematics/migrations/safe-optional-chaining/index.js", + ], ] rollup.rollup( @@ -161,6 +165,7 @@ rollup.rollup( "//packages/core/schematics/migrations/http-xhr-backend", "//packages/core/schematics/migrations/incremental-hydration", "//packages/core/schematics/migrations/model-output", + "//packages/core/schematics/migrations/safe-optional-chaining", "//packages/core/schematics/migrations/strict-safe-navigation-narrow", "//packages/core/schematics/migrations/strict-templates-default", "//packages/core/schematics/ng-generate/cleanup-unused-imports", diff --git a/packages/core/schematics/migrations.json b/packages/core/schematics/migrations.json index f841dde9d0b..25eb2be8dc3 100644 --- a/packages/core/schematics/migrations.json +++ b/packages/core/schematics/migrations.json @@ -34,6 +34,11 @@ "version": "22.0.0", "description": "Migrate broken duplicate outputs", "factory": "./bundles/model-output.cjs#migrate" + }, + "safe-optional-chaining": { + "version": "22.0.0", + "description": "Wraps optional chaining expressions in $safeNavigationMigration().", + "factory": "./bundles/safe-optional-chaining.cjs#migrate" } } } diff --git a/packages/core/schematics/migrations/safe-optional-chaining/BUILD.bazel b/packages/core/schematics/migrations/safe-optional-chaining/BUILD.bazel new file mode 100644 index 00000000000..07683e111e0 --- /dev/null +++ b/packages/core/schematics/migrations/safe-optional-chaining/BUILD.bazel @@ -0,0 +1,41 @@ +load("//tools:defaults.bzl", "jasmine_test", "ts_project") + +package( + default_visibility = [ + "//packages/core/schematics:__pkg__", + "//packages/core/schematics/test:__pkg__", + ], +) + +ts_project( + name = "safe-optional-chaining", + srcs = glob( + ["**/*.ts"], + exclude = ["*.spec.ts"], + ), + deps = [ + "//:node_modules/@angular-devkit/schematics", + "//:node_modules/typescript", + "//packages/compiler-cli/private", + "//packages/core/schematics/utils", + "//packages/core/schematics/utils/tsurge", + "//packages/core/schematics/utils/tsurge/helpers/angular_devkit", + ], +) + +ts_project( + name = "test_lib", + testonly = True, + srcs = glob(["*.spec.ts"]), + deps = [ + ":safe-optional-chaining", + "//:node_modules/typescript", + "//packages/compiler-cli", + "//packages/core/schematics/utils/tsurge", + ], +) + +jasmine_test( + name = "test", + data = [":test_lib"], +) diff --git a/packages/core/schematics/migrations/safe-optional-chaining/index.ts b/packages/core/schematics/migrations/safe-optional-chaining/index.ts new file mode 100644 index 00000000000..35ea97e534e --- /dev/null +++ b/packages/core/schematics/migrations/safe-optional-chaining/index.ts @@ -0,0 +1,20 @@ +/** + * @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.dev/license + */ + +import {Rule} from '@angular-devkit/schematics'; +import {runMigrationInDevkit} from '../../utils/tsurge/helpers/angular_devkit'; +import {SafeOptionalChainingMigration} from './migration'; + +export function migrate(): Rule { + return async (tree) => { + await runMigrationInDevkit({ + tree, + getMigration: () => new SafeOptionalChainingMigration(), + }); + }; +} diff --git a/packages/core/schematics/migrations/safe-optional-chaining/migration.ts b/packages/core/schematics/migrations/safe-optional-chaining/migration.ts new file mode 100644 index 00000000000..389bedffce6 --- /dev/null +++ b/packages/core/schematics/migrations/safe-optional-chaining/migration.ts @@ -0,0 +1,752 @@ +/** + * @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 { + AST, + ASTWithSource, + Binary, + BindingPipe, + BindingType, + Call, + Chain, + Conditional, + KeyedRead, + LiteralPrimitive, + NonNullAssert, + parseTemplate, + PrefixNot, + PropertyRead, + RecursiveAstVisitor, + SafeCall, + SafeKeyedRead, + SafePropertyRead, + TmplAstBoundAttribute, + TmplAstBoundDeferredTrigger, + TmplAstBoundEvent, + TmplAstBoundText, + TmplAstDeferredTrigger, + TmplAstElement, + TmplAstForLoopBlock, + TmplAstIfBlockBranch, + TmplAstLetDeclaration, + TmplAstRecursiveVisitor, + TmplAstSwitchBlock, + TmplAstSwitchBlockCase, + TmplAstSwitchBlockCaseGroup, + TmplAstTemplate, + TmplAstTextAttribute, +} from '@angular/compiler'; +import {AbsoluteFsPath} from '@angular/compiler-cli'; +import ts from 'typescript'; +import {NgComponentTemplateVisitor} from '../../utils/ng_component_template'; +import {getAngularDecorators} from '../../utils/ng_decorators'; +import { + confirmAsSerializable, + ProgramInfo, + projectFile, + ProjectFile, + Replacement, + Serializable, + TextUpdate, + TsurgeFunnelMigration, +} from '../../utils/tsurge'; +import {getPropertyNameText} from '../../utils/typescript/property_name'; + +import('@angular/compiler'); + +export interface CompilationUnitData { + replacements: Replacement[]; +} + +export interface MigrationConfig { + /** + * Whether to migrate this component template. + */ + shouldMigrate?: (containingFile: ProjectFile) => boolean; +} + +/** + * This migration wraps optional chaining expressions in Angular templates with a call to the + * `$safeNavigationMigration()` magic function. This function doesn't exist at runtime, but is + * used as a marker for the Angular compiler to transform the expression to keep the legacy + * behavior of returning `null`. + * + * The migration uses a top-down "sink" approach: each expression is visited with a boolean + * `nullSensitive` context that indicates whether the consumer of the expression's value + * distinguishes between `null` and `undefined`. Safe navigation operators (`?.`) wrap themselves + * when their sink is null-sensitive. + */ +export class SafeOptionalChainingMigration extends TsurgeFunnelMigration< + CompilationUnitData, + CompilationUnitData +> { + constructor(private readonly config: MigrationConfig = {}) { + super(); + } + + override async analyze(info: ProgramInfo): Promise> { + const replacements: Replacement[] = []; + + // Template Iteration + const templateVisitor = new NgComponentTemplateVisitor(info.program.getTypeChecker()); + for (const sourceFile of info.sourceFiles) { + templateVisitor.visitNode(sourceFile); + } + + for (const template of templateVisitor.resolvedTemplates) { + const nodes = parseTemplate(template.content, template.filePath.toString(), { + preserveWhitespaces: true, + preserveLineEndings: true, + preserveSignificantWhitespace: true, + leadingTriviaChars: [], + }).nodes; + const file = template.inline + ? projectFile(template.container.getSourceFile(), info) + : projectFile(template.filePath as AbsoluteFsPath, info); + + const exprMigrator = new ExpressionMigrator(file, template.start); + const visitor = new TmplVisitor(exprMigrator); + + for (const node of nodes) { + if (node.visit) { + node.visit(visitor); + } + } + + replacements.push(...exprMigrator.replacements); + } + + for (const sourceFile of info.sourceFiles) { + replacements.push(...migrateHostBindingsInSourceFile(sourceFile, info)); + } + + return confirmAsSerializable({replacements}); + } + + override async combine( + unitA: CompilationUnitData, + unitB: CompilationUnitData, + ): Promise> { + const seen = new Set(); + const deduped: Replacement[] = []; + + for (const r of [...unitA.replacements, ...unitB.replacements]) { + const key = `${r.projectFile.rootRelativePath}:${r.update.data.position}:${r.update.data.end}:${r.update.data.toInsert}`; + if (!seen.has(key)) { + seen.add(key); + deduped.push(r); + } + } + + return confirmAsSerializable({replacements: deduped}); + } + + override async globalMeta(data: CompilationUnitData): Promise> { + return confirmAsSerializable(data); + } + + override async stats(data: CompilationUnitData) { + return confirmAsSerializable({}); + } + + override async migrate(data: CompilationUnitData) { + return {replacements: data.replacements}; + } +} + +function migrateHostBindingsInSourceFile( + sourceFile: ts.SourceFile, + info: ProgramInfo, +): Replacement[] { + const replacements: Replacement[] = []; + const file = projectFile(sourceFile, info); + const typeChecker = info.program.getTypeChecker(); + + const visitNode = (node: ts.Node) => { + if (ts.isClassDeclaration(node)) { + const decorators = ts.getDecorators(node) ?? []; + const ngDecorators = getAngularDecorators(typeChecker, decorators); + + for (const decorator of ngDecorators) { + if (decorator.name !== 'Component' && decorator.name !== 'Directive') { + continue; + } + + const metadata = decorator.node.expression.arguments[0]; + if (!metadata || !ts.isObjectLiteralExpression(metadata)) { + continue; + } + + for (const prop of metadata.properties) { + if (!ts.isPropertyAssignment(prop)) { + continue; + } + + const propName = getPropertyNameText(prop.name); + if (propName !== 'host' || !ts.isObjectLiteralExpression(prop.initializer)) { + continue; + } + + for (const hostProp of prop.initializer.properties) { + if ( + !ts.isPropertyAssignment(hostProp) || + !ts.isStringLiteralLike(hostProp.initializer) + ) { + continue; + } + + const hostKey = getPropertyNameText(hostProp.name); + if (hostKey === null || (!hostKey.startsWith('[') && !hostKey.startsWith('('))) { + continue; + } + + // Preserve raw text between quotes/backticks so source offsets stay aligned. + const hostExpression = hostProp.initializer.getText().slice(1, -1); + const fakeTemplatePrefix = `
`; + const parsedNodes = parseTemplate(fakeTemplate, sourceFile.fileName, { + preserveWhitespaces: true, + }).nodes; + + const hostExpressionStart = hostProp.initializer.getStart() + 1; + const exprMigrator = new ExpressionMigrator( + file, + hostExpressionStart - fakeTemplatePrefix.length, + ); + const visitor = new HostBindingVisitor(exprMigrator); + + for (const parsedNode of parsedNodes) { + if (parsedNode.visit) { + parsedNode.visit(visitor); + } + } + + replacements.push(...exprMigrator.replacements); + } + } + } + } + + ts.forEachChild(node, visitNode); + }; + + visitNode(sourceFile); + return replacements; +} + +/** Returns whether the given attribute is a class, style, or attribute binding. */ +function isClassStyleOrAttrBinding(attribute: TmplAstBoundAttribute): boolean { + return ( + attribute.type === BindingType.Class || + attribute.type === BindingType.Style || + attribute.type === BindingType.Attribute || + (attribute.type === BindingType.Property && + (attribute.name === 'class' || attribute.name === 'className' || attribute.name === 'style')) + ); +} + +class HostBindingVisitor extends TmplAstRecursiveVisitor { + constructor(private hostExprMigrator: ExpressionMigrator) { + super(); + } + + override visitBoundAttribute(attribute: TmplAstBoundAttribute) { + if (isClassStyleOrAttrBinding(attribute)) { + // Class/style/attr bindings use truthiness — not null-sensitive by default. + // Safe navs inside function calls or pipes will still be wrapped by the migrator. + attribute.value.visit(this.hostExprMigrator, false); + } else { + // Regular property bindings are null-sensitive. + attribute.value.visit(this.hostExprMigrator, true); + } + super.visitBoundAttribute(attribute); + } + + override visitBoundEvent(event: TmplAstBoundEvent) { + if (event.handler && event.handler.visit) { + // Event handlers are not null-sensitive; safe navs inside function calls will + // still be wrapped because function arguments are always null-sensitive. + event.handler.visit(this.hostExprMigrator, false); + } + super.visitBoundEvent(event); + } +} + +/** + * Visits template expressions and inserts `$safeNavigationMigration(…)` wrappers around safe + * navigation chains whose result is consumed by a null-sensitive sink. + * + * The `context` parameter at every visit call is a boolean `nullSensitive` flag that answers: + * "does the parent care whether this expression's value is `null` vs. `undefined`?" + * + * Propagation rules: + * - `||`, `&&`, `??`: children are **not** null-sensitive (both normalise null/undefined). + * - `===` / `!==` with a nullish literal on one side: the other operand **is** null-sensitive. + * - All other binary operators: propagate the parent's null-sensitivity. + * - `!` (prefix not): operand is **not** null-sensitive (uses truthiness). + * - Ternary condition: **not** null-sensitive; branches inherit the parent's null-sensitivity. + * - Function call arguments and pipe inputs: **always** null-sensitive. + * - Receivers of property/keyed/call reads: **not** null-sensitive by default. + * For property/keyed/call continuations, when the receiver is a safe node and the parent sink + * is null-sensitive, wrap the full continuation node (e.g. `foo?.bar.baz`, `foo?.save()`) rather + * than the inner safe receiver (`foo?.bar`, `foo?.save`) so runtime behavior is preserved. + * - `NonNullAssert` (`!`): propagates the parent's null-sensitivity (type-only assertion). + */ +class ExpressionMigrator extends RecursiveAstVisitor { + replacements: Replacement[] = []; + + constructor( + private file: ProjectFile, + private templateStart: number, + ) { + super(); + } + + // --------------------------------------------------------------------------- + // Safe navigation nodes — wrap when the sink is null-sensitive + // --------------------------------------------------------------------------- + + override visitSafePropertyRead(ast: SafePropertyRead, nullSensitive: boolean): any { + if (nullSensitive) { + this.addReplacement(ast); + } + // Receiver: not null-sensitive (further access on null/undefined throws either way). + this.visit(ast.receiver, false); + } + + override visitSafeKeyedRead(ast: SafeKeyedRead, nullSensitive: boolean): any { + if (nullSensitive) { + this.addReplacement(ast); + } + this.visit(ast.receiver, false); + this.visit(ast.key, false); + } + + override visitSafeCall(ast: SafeCall, nullSensitive: boolean): any { + if (nullSensitive) { + this.addReplacement(ast); + } + this.visit(ast.receiver, false); + this.visitAll(ast.args, true); + } + + private hasSafeReceiver(receiver: AST): boolean { + if ( + receiver instanceof SafePropertyRead || + receiver instanceof SafeKeyedRead || + receiver instanceof SafeCall + ) { + return true; + } + if (receiver instanceof NonNullAssert) { + return this.hasSafeReceiver(receiver.expression); + } + return false; + } + + // --------------------------------------------------------------------------- + // Non-safe access — receiver is never null-sensitive + // --------------------------------------------------------------------------- + + override visitPropertyRead(ast: PropertyRead, nullSensitive: boolean): any { + if (nullSensitive && this.hasSafeReceiver(ast.receiver)) { + this.addReplacement(ast); + } + this.visit(ast.receiver, false); + } + + override visitKeyedRead(ast: KeyedRead, nullSensitive: boolean): any { + if (nullSensitive && this.hasSafeReceiver(ast.receiver)) { + this.addReplacement(ast); + } + this.visit(ast.receiver, false); + this.visit(ast.key, false); + } + + // --------------------------------------------------------------------------- + // Function calls — arguments are always null-sensitive + // --------------------------------------------------------------------------- + + override visitCall(ast: Call, nullSensitive: boolean): any { + if (nullSensitive && this.hasSafeReceiver(ast.receiver)) { + this.addReplacement(ast); + } + this.visit(ast.receiver, false); + this.visitAll(ast.args, true); + } + + // --------------------------------------------------------------------------- + // Pipes — input and arguments are always null-sensitive + // --------------------------------------------------------------------------- + + override visitPipe(ast: BindingPipe, _nullSensitive: boolean): any { + this.visit(ast.exp, true); + this.visitAll(ast.args, true); + } + + // --------------------------------------------------------------------------- + // Binary operators + // --------------------------------------------------------------------------- + + override visitBinary(ast: Binary, nullSensitive: boolean): any { + if (ast.operation === '||' || ast.operation === '&&' || ast.operation === '??') { + // These operators normalise null and undefined (both produce the same result), + // so the operands are not null-sensitive. + this.visit(ast.left, false); + this.visit(ast.right, false); + } else if (ast.operation === '===' || ast.operation === '!==') { + // A strict comparison with a nullish literal makes the other side null-sensitive + // (null === null is true but undefined === null is false). + const leftIsNullish = isNullishLiteralAST(ast.left); + const rightIsNullish = isNullishLiteralAST(ast.right); + this.visit(ast.left, rightIsNullish); + this.visit(ast.right, leftIsNullish); + } else { + // All other binary operators (<, >, +, -, …): propagate the parent's null-sensitivity + // because null and undefined can produce different numeric results (null → 0, + // undefined → NaN for arithmetic/comparison). + this.visit(ast.left, nullSensitive); + this.visit(ast.right, nullSensitive); + } + } + + // --------------------------------------------------------------------------- + // Unary / logical operators + // --------------------------------------------------------------------------- + + override visitPrefixNot(ast: PrefixNot, _nullSensitive: boolean): any { + // Logical negation uses truthiness — null and undefined are both falsy. + this.visit(ast.expression, false); + } + + // --------------------------------------------------------------------------- + // Ternary + // --------------------------------------------------------------------------- + + override visitConditional(ast: Conditional, nullSensitive: boolean): any { + // The condition is evaluated as a boolean — not null-sensitive. + this.visit(ast.condition, false); + // The result of a branch is consumed by the parent, so it inherits null-sensitivity. + this.visit(ast.trueExp, nullSensitive); + this.visit(ast.falseExp, nullSensitive); + } + + // --------------------------------------------------------------------------- + // NonNullAssert — a compile-time type annotation, no runtime effect + // --------------------------------------------------------------------------- + + override visitNonNullAssert(ast: NonNullAssert, nullSensitive: boolean): any { + this.visit(ast.expression, nullSensitive); + } + + // --------------------------------------------------------------------------- + // Chain (event handler statements) — never null-sensitive + // --------------------------------------------------------------------------- + + override visitChain(ast: Chain, _nullSensitive: boolean): any { + this.visitAll(ast.expressions, false); + } + + // --------------------------------------------------------------------------- + // Interpolation — coerces to string, so null and undefined produce identical + // output; sub-expressions are therefore never null-sensitive. + // --------------------------------------------------------------------------- + + override visitInterpolation(ast: {expressions: AST[]}, _nullSensitive: boolean): any { + this.visitAll(ast.expressions, false); + } + + // --------------------------------------------------------------------------- + // All other nodes (LiteralArray, LiteralMap, Unary, …) use the default + // RecursiveAstVisitor which propagates the current context to every child — + // the correct "inherit parent null-sensitivity" fallback. + // --------------------------------------------------------------------------- + + private addReplacement(ast: AST): void { + const startArg = ast.sourceSpan.start; + const endArg = ast.sourceSpan.end; + + this.replacements.push( + new Replacement( + this.file, + new TextUpdate({ + position: this.templateStart + endArg, + end: this.templateStart + endArg, + toInsert: ')', + }), + ), + new Replacement( + this.file, + new TextUpdate({ + position: this.templateStart + startArg, + end: this.templateStart + startArg, + toInsert: '$safeNavigationMigration(', + }), + ), + ); + } +} + +// --------------------------------------------------------------------------- +// Helper utilities +// --------------------------------------------------------------------------- + +/** Returns true if the AST node is a literal `null` or `undefined`. */ +function isNullishLiteralAST(ast: AST): boolean { + const innerAst = ast instanceof ASTWithSource ? ast.ast : ast; + return ( + innerAst instanceof LiteralPrimitive && + (innerAst.value === null || innerAst.value === undefined) + ); +} + +/** Returns true if the AST node is a non-null, non-undefined primitive literal. */ +function isNonNullishLiteralAST(ast: AST): boolean { + const innerAst = ast instanceof ASTWithSource ? ast.ast : ast; + return ( + innerAst instanceof LiteralPrimitive && innerAst.value !== null && innerAst.value !== undefined + ); +} + +/** Returns true if any expression in `ast` contains a strict null/undefined check. */ +function hasNullCheckInAST(ast: AST): boolean { + const innerAst = ast instanceof ASTWithSource ? ast.ast : ast; + + const visitor = new NullCheckVisitor(); + innerAst.visit(visitor); + return visitor.hasNullCheck; +} + +class NullCheckVisitor extends RecursiveAstVisitor { + public hasNullCheck: boolean = false; + override visitBinary(node: Binary, context: unknown) { + if (node.operation === '===' || node.operation === '!==') { + const isLeftNullish = isNullishLiteralAST(node.left); + const isRightNullish = isNullishLiteralAST(node.right); + if (isLeftNullish || isRightNullish) { + this.hasNullCheck = true; + } + } + super.visitBinary(node, context); + } +} + +// --------------------------------------------------------------------------- +// Template visitor +// --------------------------------------------------------------------------- + +/** + * Returns true if all *ngSwitchCase bindings in the given nodes (and their children) + * are non-null/non-undefined literal expressions — meaning the switch expression + * doesn't need null-sensitivity migration. + */ +function allNgSwitchCasesAreLiterals(nodes: Array): boolean { + for (const node of nodes) { + for (const input of node.inputs) { + if (input.name === 'ngSwitchCase' && input.value) { + if (!isNonNullishLiteralAST(input.value)) { + return false; + } + } + } + + if (node instanceof TmplAstTemplate) { + for (const attr of node.templateAttrs) { + if (attr instanceof TmplAstBoundAttribute && attr.name === 'ngSwitchCase' && attr.value) { + if (!isNonNullishLiteralAST(attr.value)) { + return false; + } + } + } + } + + const childHosts = node.children.filter( + (child): child is TmplAstElement | TmplAstTemplate => + child instanceof TmplAstElement || child instanceof TmplAstTemplate, + ); + + if (!allNgSwitchCasesAreLiterals(childHosts)) { + return false; + } + } + + return true; +} + +class TmplVisitor extends TmplAstRecursiveVisitor { + private migratableSwitchCases = new WeakSet(); + /** + * Stack tracking whether the current ngSwitch context should be migrated. + * False when all *ngSwitchCase expressions are non-null literals. + */ + private ngSwitchShouldMigrateStack: boolean[] = []; + + constructor(private exprMigrator: ExpressionMigrator) { + super(); + } + + private shouldMigrateCurrentNgSwitchContext(): boolean { + return this.ngSwitchShouldMigrateStack[this.ngSwitchShouldMigrateStack.length - 1] ?? true; + } + + private hasNgSwitchBinding(node: TmplAstElement | TmplAstTemplate): boolean { + return ( + node.inputs.some((attr) => attr.name === 'ngSwitch') || + (node instanceof TmplAstTemplate && + node.templateAttrs.some( + (attr: TmplAstBoundAttribute | TmplAstTextAttribute) => attr.name === 'ngSwitch', + )) + ); + } + + override visitElement(element: TmplAstElement) { + const hasNgSwitch = this.hasNgSwitchBinding(element); + if (hasNgSwitch) { + const childHosts = element.children.filter( + (child): child is TmplAstElement | TmplAstTemplate => + child instanceof TmplAstElement || child instanceof TmplAstTemplate, + ); + this.ngSwitchShouldMigrateStack.push(!allNgSwitchCasesAreLiterals(childHosts)); + } + + super.visitElement(element); + + if (hasNgSwitch) { + this.ngSwitchShouldMigrateStack.pop(); + } + } + + override visitBoundAttribute(attribute: TmplAstBoundAttribute) { + if (attribute.name === 'ngForOf') { + // ngFor/@for item expressions are not null-sensitive by default. + // Still visit so inner null-sensitive sinks (e.g. pipes/functions) are migrated. + attribute.value.visit(this.exprMigrator, false); + } else if (attribute.name === 'ngIf') { + // ngIf evaluates as a boolean; null-sensitivity only arises when the expression + // itself contains a strict null comparison (handled inside ExpressionMigrator). + attribute.value.visit(this.exprMigrator, false); + } else if (attribute.name === 'ngSwitch' || attribute.name === 'ngSwitchCase') { + attribute.value.visit(this.exprMigrator, this.shouldMigrateCurrentNgSwitchContext()); + } else if (isClassStyleOrAttrBinding(attribute)) { + // Class/style/attr bindings use truthiness — not null-sensitive by default. + attribute.value.visit(this.exprMigrator, false); + } else { + // Regular property bindings are null-sensitive. + attribute.value.visit(this.exprMigrator, true); + } + super.visitBoundAttribute(attribute); + } + + override visitBoundEvent(event: TmplAstBoundEvent) { + if (event.handler && event.handler.visit) { + // Event handlers are not null-sensitive. Safe navs inside function calls + // (e.g. `compute(user?.save())`) are still migrated because function arguments + // are always null-sensitive in ExpressionMigrator. + event.handler.visit(this.exprMigrator, false); + } + super.visitBoundEvent(event); + } + + override visitBoundText(text: TmplAstBoundText) { + // Interpolation text is not null-sensitive by default; null-sensitivity is + // introduced by pipes, function calls, or strict null comparisons inside + // the expression, all handled by ExpressionMigrator. + text.value.visit(this.exprMigrator, false); + super.visitBoundText(text); + } + + override visitTemplate(template: TmplAstTemplate) { + const hasNgSwitch = this.hasNgSwitchBinding(template); + if (hasNgSwitch) { + const childHosts = template.children.filter( + (child): child is TmplAstElement | TmplAstTemplate => + child instanceof TmplAstElement || child instanceof TmplAstTemplate, + ); + this.ngSwitchShouldMigrateStack.push(!allNgSwitchCasesAreLiterals(childHosts)); + } + + for (const attr of template.templateAttrs) { + if (!(attr instanceof TmplAstBoundAttribute)) { + continue; + } + + if (attr.name === 'ngIf') { + attr.value.visit(this.exprMigrator, false); + } else if (attr.name === 'ngSwitch' || attr.name === 'ngSwitchCase') { + attr.value.visit(this.exprMigrator, this.shouldMigrateCurrentNgSwitchContext()); + } else if (attr.name === 'ngForOf') { + // ngFor microsyntax expressions are not null-sensitive by default. + // Still visit so nested null-sensitive sinks are handled. + attr.value.visit(this.exprMigrator, false); + } else { + attr.value.visit(this.exprMigrator, true); + } + } + + super.visitTemplate(template); + + if (hasNgSwitch) { + this.ngSwitchShouldMigrateStack.pop(); + } + } + + override visitIfBlockBranch(block: TmplAstIfBlockBranch) { + if (block.expression) { + // @if condition: not null-sensitive by default (same logic as ngIf). + block.expression.visit(this.exprMigrator, false); + } + super.visitIfBlockBranch(block); + } + + override visitForLoopBlock(block: TmplAstForLoopBlock) { + block.expression.visit(this.exprMigrator, false); + block.trackBy.visit(this.exprMigrator, false); + super.visitForLoopBlock(block); + } + + override visitLetDeclaration(decl: TmplAstLetDeclaration) { + // @let value is assigned directly — null-sensitive. + decl.value.visit(this.exprMigrator, true); + super.visitLetDeclaration(decl); + } + + override visitSwitchBlock(block: TmplAstSwitchBlock) { + const switchCases = block.groups.flatMap((group: TmplAstSwitchBlockCaseGroup) => group.cases); + + // Don't migrate if every case expression is a non-null/non-undefined literal + // (e.g. strings, numbers, booleans). In that case null-vs-undefined can never + // match a case, so wrapping the switch expression would be pointless. + const shouldMigrate = !switchCases + .filter((switchCase) => switchCase.expression) + .every((switchCase) => isNonNullishLiteralAST(switchCase.expression!)); + + if (shouldMigrate) { + block.expression.visit(this.exprMigrator, true); + for (const switchCase of switchCases) { + this.migratableSwitchCases.add(switchCase); + } + } + + super.visitSwitchBlock(block); + } + + override visitSwitchBlockCase(block: TmplAstSwitchBlockCase) { + if (this.migratableSwitchCases.has(block) && block.expression) { + block.expression.visit(this.exprMigrator, true); + } + super.visitSwitchBlockCase(block); + } + + override visitDeferredTrigger(trigger: TmplAstDeferredTrigger) { + if (trigger instanceof TmplAstBoundDeferredTrigger) { + // @defer (when …): same logic as @if — not null-sensitive by default. + trigger.value.visit(this.exprMigrator, false); + } + super.visitDeferredTrigger(trigger); + } +} diff --git a/packages/core/schematics/migrations/safe-optional-chaining/safe-optional-chaining.spec.ts b/packages/core/schematics/migrations/safe-optional-chaining/safe-optional-chaining.spec.ts new file mode 100644 index 00000000000..31d098e3225 --- /dev/null +++ b/packages/core/schematics/migrations/safe-optional-chaining/safe-optional-chaining.spec.ts @@ -0,0 +1,582 @@ +/** + * @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.dev/license + */ + +import {absoluteFrom, getFileSystem} from '@angular/compiler-cli'; +import {initMockFileSystem} from '@angular/compiler-cli/private/testing'; +import { + applyTextUpdates, + groupReplacementsByFile, + ProjectRootRelativePath, +} from '../../utils/tsurge'; +import {runTsurgeMigration} from '../../utils/tsurge/testing'; +import {SafeOptionalChainingMigration} from './migration'; + +describe('SafeOptionalChainingMigration', () => { + beforeEach(() => { + initMockFileSystem('Native'); + }); + + it('should migrate optional chaining expressions in interpolations when applicable', async () => { + const content = await migrateInlineTemplate(` + {{ compute(foo?.bar) }} + {{ compute(foo.bar.baz?.alpha) }} + {{ compute(foo.bar?.[0]) }} + {{ compute(foo.bar?.['abc']) }} + {{ foo?.bar | json }} + {{ foo.bar.baz?.alpha | json }} + {{ foo?.bar === null ? 'A' : 'B'}} + {{ foo?.bar === undefined ? 'A' : 'B'}} + `); + + expect(content).toContain('{{ compute($safeNavigationMigration(foo?.bar)) }}'); + expect(content).toContain('{{ compute($safeNavigationMigration(foo.bar.baz?.alpha)) }}'); + expect(content).toContain('{{ compute($safeNavigationMigration(foo.bar?.[0])) }}'); + expect(content).toContain("{{ compute($safeNavigationMigration(foo.bar?.['abc'])) }}"); + expect(content).toContain('{{ $safeNavigationMigration(foo?.bar) | json }}'); + expect(content).toContain('{{ $safeNavigationMigration(foo.bar.baz?.alpha) | json }}'); + expect(content).toContain("{{ $safeNavigationMigration(foo?.bar) === null ? 'A' : 'B'}}"); + expect(content).toContain("{{ $safeNavigationMigration(foo?.bar) === undefined ? 'A' : 'B'}}"); + }); + + it('should not migrate optional chaining expressions in interpolations when unnecessary', async () => { + const content = await migrateInlineTemplate(` + {{ ((((((((((foo?.bar))))))))) }} + {{ foo.bar?.baz }} + {{ foo.bar?.[0] }} + {{ foo.bar?.['abc'] }} + {{ foo?.bar ?? 'ok' }} + {{ foo?.bar && 'ok' }} + {{ foo?.bar ?? foo?.bar }} + {{ foo?.bar || foo?.bar }} + {{ foo?.bar && foo?.bar }} + {{ foo.bar?.() }} + {{ !foo?.bar }} + `); + expect(content).toContain('{{ ((((((((((foo?.bar))))))))) }}'); + expect(content).toContain('{{ foo.bar?.baz }}'); + expect(content).toContain('{{ foo.bar?.[0] }}'); + expect(content).toContain("{{ foo.bar?.['abc'] }}"); + expect(content).toContain("{{ foo?.bar ?? 'ok' }}"); + expect(content).toContain("{{ foo?.bar && 'ok' }}"); + expect(content).toContain('{{ foo?.bar ?? foo?.bar }}'); + expect(content).toContain('{{ foo?.bar || foo?.bar }}'); + expect(content).toContain('{{ foo?.bar && foo?.bar }}'); + expect(content).toContain('{{ foo.bar?.() }}'); + expect(content).toContain('{{ !foo?.bar }}'); + }); + + it('should not input/attribute value with interpolation', async () => { + const content = await migrateInlineTemplate(` + +
+ `); + + expect(content).toContain(''); + expect(content).toContain('
'); + }); + + it('should only migrate @if/ngIf conditional if there is a strict null check', async () => { + const actual = await migrateInlineTemplate(` +
+
+
+ + @if(foo?.bar) {} + @if(foo?.bar !== null) {} + `); + expect(actual).toContain('
'); // Not migrated + expect(actual).toContain('
'); + expect(actual).toContain('
'); // Negation should not be migrated + + expect(actual).toContain('@if(foo?.bar) {}'); // Not migrated + expect(actual).toContain('@if($safeNavigationMigration(foo?.bar) !== null) {}'); + }); + + it('should not migrate @if/ngIf conditionals that are checking for null but not with a strict equality check', async () => { + const actual = await migrateInlineTemplate(` +
+
+ + @if(foo?.bar == null) {} + @if(foo?.bar != null) {} + `); + expect(actual).toContain('
'); + expect(actual).toContain('
'); + + expect(actual).toContain('@if(foo?.bar == null) {}'); + expect(actual).toContain('@if(foo?.bar != null) {}'); + }); + + it('should migrate conditionals if there is a strict check against undefined', async () => { + const actual = await migrateInlineTemplate(` +
+ @if(foo?.bar !== undefined) {} + `); + expect(actual).toContain( + '
', + ); + expect(actual).toContain('@if($safeNavigationMigration(foo?.bar) !== undefined) {}'); + }); + + it('should migrate @defer when condition if there is a strict null check', async () => { + const actual = await migrateInlineTemplate(` + @defer (when foo?.bar === null) { +
Deferred content
+ } + `); + + expect(actual).toContain('@defer (when $safeNavigationMigration(foo?.bar) === null) {'); + }); + + it('should skip simple ngFor/@for expressions', async () => { + const actual = await migrateInlineTemplate(` +
+ @for(item of items?.list; track item) {} + `); + expect(actual).toContain('
'); + expect(actual).toContain('@for(item of items?.list; track item) {}'); + // Sanity check + expect(actual).not.toContain('$safeNavigationMigration'); + }); + + it('should migrate ngFor expressions when needed', async () => { + const actual = await migrateInlineTemplate( + `
`, + ); + expect(actual).toContain( + '
', + ); + }); + + it('should migrate @for expressions when needed', async () => { + const actual = await migrateInlineTemplate(` + @for (item of repository?.project | users; track item.id) {} + `); + expect(actual).toContain( + '@for (item of $safeNavigationMigration(repository?.project) | users; track item.id) {}', + ); + }); + + it('should not migrate the @for track function', async () => { + const actual = await migrateInlineTemplate(` + @for (item of items; track item?.id) {} + `); + expect(actual).toContain('@for (item of items; track item?.id) {}'); + }); + + it('should migrate a track function that contains an expression', async () => { + const actual = await migrateInlineTemplate(` + @for (item of items; track compute(item?.id)) {} + `); + expect(actual).toContain( + '@for (item of items; track compute($safeNavigationMigration(item?.id))) {}', + ); + }); + + it('should migrate @let declarations', async () => { + const actual = await migrateInlineTemplate(`@let x = foo?.bar;`); + expect(actual).toContain(`@let x = $safeNavigationMigration(foo?.bar);`); + }); + + it('should migrate inputs and attribute bindings with optional chaining expressions', async () => { + const actual = await migrateInlineTemplate(` +
+ + + + + + + + + + + + + `); + expect(actual).toContain('
'); + expect(actual).toContain(''); + expect(actual).toContain( + '', + ); + expect(actual).toContain(''); + expect(actual).toContain(''); + expect(actual).toContain(''); + expect(actual).toContain(''); + expect(actual).toContain(''); + + // Yes there are different semantics between null & undefined checks and greater/less than comparisons + expect(actual).toContain(''); + expect(actual).toContain(''); + expect(actual).toContain(''); + expect(actual).toContain(''); + }); + + it('should not migrate binding expressions when not necessary', async () => { + const actual = await migrateInlineTemplate(` + + + + + + `); + + expect(actual).toContain(''); + expect(actual).toContain(''); + expect(actual).toContain(''); + expect(actual).toContain(``); + expect(actual).toContain(''); + }); + + it('should skip interpolation with no function and no pipe', async () => { + const actual = await migrateInlineTemplate(` +

{{ foo?.bar }}

+
{{ compute(foo?.bar) }}
+ {{ foo?.bar | json }} + `); + expect(actual).toContain('

{{ foo?.bar }}

'); // skipped + expect(actual).toContain('
{{ compute($safeNavigationMigration(foo?.bar)) }}
'); + expect(actual).toContain('{{ $safeNavigationMigration(foo?.bar) | json }}'); + }); + + it('should migrate optional chaining expressions in pipe arguments', async () => { + const actual = await migrateInlineTemplate(` +

{{ foo | myPipe:foo?.bar }}

+ `); + expect(actual).toContain('

{{ foo | myPipe:$safeNavigationMigration(foo?.bar) }}

'); + }); + + it('should skip direct optional call in event handlers but migrate wrapped handlers', async () => { + const actual = await migrateInlineTemplate(` + + + `); + + expect(actual).toContain(''); + // In `computed(user?.save())`, the optional-call continuation sits in a function + // argument position, which is null-sensitive for consumers that distinguish + // `null` from `undefined`; this call must be wrapped. + expect(actual).toContain( + '', + ); + }); + + it('should skip class, style, and attribute bindings that are just optional chains', async () => { + const actual = await migrateInlineTemplate(` +
+
+
+
+
+ +
+
+
+
+
+
+
+ `); + + expect(actual).toContain('
'); + expect(actual).toContain('
'); + expect(actual).toContain('
'); + expect(actual).toContain('
'); + expect(actual).toContain('
'); + expect(actual).toContain(`
`); + expect(actual).toContain(`
`); + expect(actual).toContain(`
`); + expect(actual).toContain(`
`); + expect(actual).toContain(`
`); + expect(actual).toContain(`
`); + + expect(actual).toContain('
'); + }); + + it('should migrate some cases of class/styles/attr bindings', async () => { + const actual = await migrateInlineTemplate(` +
+
+ `); + + expect(actual).toContain( + '
', + ); + expect(actual).toContain( + '
', + ); + }); + + it('should migrate all expressions in ngSwitch/@switch', async () => { + const actual = await migrateInlineTemplate(` +
+ +
+ + @switch (foo?.bar) { + @case (foo?.baz) {} + @default {} + } + `); + + expect(actual).toContain('
'); + expect(actual).toContain(''); + expect(actual).toContain('@switch ($safeNavigationMigration(foo?.bar)) {'); + expect(actual).toContain('@case ($safeNavigationMigration(foo?.baz)) {}'); + }); + + it('should not migrate ngSwitch/@switch when all cases are non-null literals', async () => { + const actual = await migrateInlineTemplate(` +
+ + +
+ + @switch (foo?.bar) { + @case ('world') {} + @case (0) {} + @default {} + } + `); + + expect(actual).not.toContain('$safeNavigationMigration'); + }); + + it('should migrate ngSwitch/@switch if at least one case checks for null', async () => { + const actual = await migrateInlineTemplate(` +
+ +
+ + @switch (foo?.bar) { + @case (foo?.baz !== null) {} + @default {} + } + + @switch (foo?.baz) { + @case (null) {} + @default {} + } + `); + + expect(actual).toContain('
'); + expect(actual).toContain( + '', + ); + expect(actual).toContain('@switch ($safeNavigationMigration(foo?.bar)) {'); + expect(actual).toContain('@case ($safeNavigationMigration(foo?.baz) !== null) {}'); + + expect(actual).toContain('@switch ($safeNavigationMigration(foo?.baz)) {'); + }); + + it('should migrate optional-chain continuations in null-sensitive sinks', async () => { + const content = await migrateInlineTemplate(` + {{ computed(compute(foo?.bar.baz)?.bar.baz) }} + `); + // `foo?.bar.baz` and `compute(... )?.bar.baz` are optional-chain continuations. + // In null-sensitive sinks (function args), preserving legacy null-vs-undefined behavior + // requires wrapping these chains. + expect(content).toContain( + 'computed($safeNavigationMigration(compute($safeNavigationMigration(foo?.bar.baz))?.bar.baz))', + ); + }); + + it('should migrate host bindings when applicable', async () => { + const {fs} = await runTsurgeMigration(new SafeOptionalChainingMigration(), [ + { + name: absoluteFrom('/app.component.ts'), + isProgramRootFile: true, + contents: ` + import {Component} from '@angular/core'; + @Component({ + selector: 'app-root', + template:'', + host: { + '[attr.data-id2]': 'computed(user?.id)', + '[attr.data-id2]': 'user?.id | json', + + '[class.active]': 'user?.active === null', + '[id]': 'user?.id', + } + }) + export class AppComponent { foo: any; compute(a: any) {} } + `, + }, + ]); + const content = fs.readFile(absoluteFrom('/app.component.ts')); + + expect(content).toContain(`'[attr.data-id2]': 'computed($safeNavigationMigration(user?.id))',`); + expect(content).toContain(`'[attr.data-id2]': '$safeNavigationMigration(user?.id) | json',`); + expect(content).toContain( + `'[class.active]': '$safeNavigationMigration(user?.active) === null',`, + ); + expect(content).toContain(`'[id]': '$safeNavigationMigration(user?.id)',`); + }); + + it('should not migrate host bindings when not necessary', async () => { + const {fs} = await runTsurgeMigration(new SafeOptionalChainingMigration(), [ + { + name: absoluteFrom('/app.component.ts'), + isProgramRootFile: true, + contents: ` + import {Component} from '@angular/core'; + @Component({ + selector: 'app-root', + template:'', + host: { + '[attr.data-id]': 'user?.id', + '[attr.data-id]': 'user?.id ?? foo', + '[attr.data-id]': 'user?.id || foo', + '[attr.data-id]': 'user?.id && foo', + '(click)': 'user?.save()' + } + }) + export class AppComponent { foo: any; compute(a: any) {} } + `, + }, + ]); + const content = fs.readFile(absoluteFrom('/app.component.ts')); + + expect(content).toContain(`'[attr.data-id]': 'user?.id',`); + expect(content).toContain(`'[attr.data-id]': 'user?.id ?? foo',`); + expect(content).toContain(`'[attr.data-id]': 'user?.id || foo',`); + expect(content).toContain(`'[attr.data-id]': 'user?.id && foo',`); + expect(content).toContain(`'(click)': 'user?.save()'`); + }); + + it('should handle a file that is present in multiple projects', async () => { + const mockFs = getFileSystem(); + + const sharedFile = absoluteFrom('/app.component.ts'); + const sharedContent = ` + import {Component} from '@angular/core'; + @Component({ + selector: 'app-root', + template: \`
\` + }) + export class AppComponent { user: any; } + `; + + mockFs.ensureDir(absoluteFrom('/')); + mockFs.writeFile(sharedFile, sharedContent); + + const tsconfig1 = absoluteFrom('/tsconfig.app.json'); + const tsconfig2 = absoluteFrom('/tsconfig.spec.json'); + + mockFs.writeFile( + tsconfig1, + JSON.stringify({compilerOptions: {strict: true, rootDir: '/'}, files: [sharedFile]}), + ); + mockFs.writeFile( + tsconfig2, + JSON.stringify({compilerOptions: {strict: true, rootDir: '/'}, files: [sharedFile]}), + ); + + const migration = new SafeOptionalChainingMigration(); + + const info1 = migration.createProgram(tsconfig1, mockFs); + const info2 = migration.createProgram(tsconfig2, mockFs); + + const unitData1 = await migration.analyze(info1); + const unitData2 = await migration.analyze(info2); + + const combined = await migration.combine(unitData1, unitData2); + const globalMeta = await migration.globalMeta(combined); + const {replacements} = await migration.migrate(globalMeta); + + const updates = groupReplacementsByFile(replacements); + const relPath = sharedFile.substring(1) as string as ProjectRootRelativePath; // strip leading '/' + const changes = updates.get(relPath) ?? []; + const result = applyTextUpdates(sharedContent, changes); + + // The expression should be wrapped exactly once, not twice. + expect(result).toContain('$safeNavigationMigration(user?.id)'); + expect(result).not.toContain('$safeNavigationMigration($safeNavigationMigration'); + }); + + it('should migrate an external template', async () => { + const content = await migrateExternalTemplate(` + {{ compute(foo?.bar) }} +
+ `); + + expect(content).toContain('{{ compute($safeNavigationMigration(foo?.bar)) }}'); + expect(content).toContain('
'); + }); + + it('should be idempotent — running twice does not double-wrap expressions', async () => { + const input = ` + {{ compute(foo?.bar) }} + {{ foo?.bar | json }} +
+
+ `; + + // First pass: migrate fresh code + const firstPass = await migrateInlineTemplate(input); + + // Verify all expressions are wrapped correctly on first pass + expect(firstPass).toContain('{{ compute($safeNavigationMigration(foo?.bar)) }}'); + expect(firstPass).toContain('{{ $safeNavigationMigration(foo?.bar) | json }}'); + expect(firstPass).toContain('
'); + expect(firstPass).toContain('
'); + + // Second pass: run migration again on already-migrated code + const secondPass = await migrateInlineTemplate(firstPass); + + // Verify no double-wrapping occurred + expect(secondPass).not.toContain('$safeNavigationMigration($safeNavigationMigration'); + // The already-wrapped expressions should remain unchanged + expect(secondPass).toContain('{{ compute($safeNavigationMigration(foo?.bar)) }}'); + expect(secondPass).toContain('{{ $safeNavigationMigration(foo?.bar) | json }}'); + expect(secondPass).toContain('
'); + expect(secondPass).toContain('
'); + }); +}); + +async function migrateInlineTemplate(template: string): Promise { + const {fs} = await runTsurgeMigration(new SafeOptionalChainingMigration(), [ + { + name: absoluteFrom('/app.component.ts'), + isProgramRootFile: true, + contents: ` + import {Component} from '@angular/core'; + @Component({ + selector: 'app-root', + template: \` + ${template} + \` + }) + export class AppComponent { foo: any; compute(a: any) {} } + `, + }, + ]); + return fs.readFile(absoluteFrom('/app.component.ts')); +} + +async function migrateExternalTemplate(template: string): Promise { + const {fs} = await runTsurgeMigration(new SafeOptionalChainingMigration(), [ + { + name: absoluteFrom('/app.component.html'), + contents: template, + }, + { + name: absoluteFrom('/app.component.ts'), + isProgramRootFile: true, + contents: ` + import {Component} from '@angular/core'; + @Component({ + selector: 'app-root', + templateUrl: './app.component.html' + }) + export class AppComponent { foo: any; compute(a: any) {} } + `, + }, + ]); + return fs.readFile(absoluteFrom('/app.component.html')); +}