From 25b261a8fdebc17d965b4273e3dc3d779533fcde Mon Sep 17 00:00:00 2001 From: Edouard Bozon Date: Mon, 19 Oct 2026 15:41:27 +0200 Subject: [PATCH 1/3] fix(compiler): resolve template locals inside `$safeNavigationMigration` in listeners A listener's `handler_expression` is resolved by calling `resolve_expression` directly rather than through the generic expression visitor, and its explicit match had no arm for the `SafeNavigationMigration` wrapper. Template locals in the wrapped subtree (`@for` items, `@if ... as` aliases) were left unresolved and later read from the listener context (`ctx.option`), so `(click)="pick($safeNavigationMigration(option?.value))"` always passed `null`. Fixes #585 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/pipeline/phases/resolve_names.rs | 14 ++++++++ .../tests/integration_test.rs | 34 +++++++++++++++++++ .../safe-navigation-chain.fixture.ts | 30 ++++++++++++++++ 3 files changed, 78 insertions(+) diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs index ddd9be122..88eafb7f8 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs @@ -877,6 +877,20 @@ fn resolve_expression<'a>( ); } + // `$safeNavigationMigration(...)` wrapper - resolve the wrapped expression. A + // listener's `handler_expression` is resolved through this function directly + // rather than the generic visitor, so the wrapper must be descended here. + IrExpression::SafeNavigationMigration(migration) => { + resolve_expression( + migration.expr.as_mut(), + scope, + root_xref, + saved_view, + allocator, + expressions, + ); + } + // ResolvedTemplateLiteral (created by ingest for template literals with inner expressions) // - resolve each inner expression so LexicalRead refs to @let vars and pipe args are resolved IrExpression::ResolvedTemplateLiteral(tl) => { diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index 2c2283ca6..597bb8fb6 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -2339,6 +2339,40 @@ fn test_safe_navigation_migration_ignores_qualified_call() { ); } +#[test] +fn test_safe_navigation_migration_in_listener_reads_template_locals() { + // Inside a listener, the wrapped expression must resolve template locals + // (`@for` items, `@if ... as` aliases) to the restored-view variables, like + // any other listener expression, instead of falling back to `ctx.`. + let js = compile_template_to_js_with_version( + r#"@for (option of options; track $index) { + +} +@if (user; as u) { + +}"#, + "TestComponent", + Some(AngularVersion::new(22, 0, 0)), + ); + assert!( + !js.contains("ctx.option ==") && !js.contains("ctx.option.value"), + "`option` must not be read from the context, got:\n{js}" + ); + assert!( + !js.contains("ctx.u ==") && !js.contains("ctx.u.id"), + "`u` must not be read from the context, got:\n{js}" + ); + assert!( + js.contains(".pick(((option_r") && js.contains("== null)? null: option_r"), + "`option` must be read from the restored view with legacy null semantics, got:\n{js}" + ); + assert!( + js.contains(".save(((u_r") && js.contains("== null)? null: u_r"), + "`u` must be read from the restored view with legacy null semantics, got:\n{js}" + ); + assert!(!js.contains("$safeNavigationMigration"), "the wrapper must be stripped, got:\n{js}"); +} + // ============================================================================ // Event Modifier Tests // ============================================================================ diff --git a/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-chain.fixture.ts b/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-chain.fixture.ts index 89d953a76..0437d721d 100644 --- a/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-chain.fixture.ts +++ b/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-chain.fixture.ts @@ -125,4 +125,34 @@ export class SafeNavCombinedComponent { `.trim(), expectedFeatures: ['ɵɵtextInterpolate1'], }, + { + name: 'safe-nav-migration-in-listener', + category: 'regressions', + description: '$safeNavigationMigration in listeners reading template locals', + className: 'SafeNavMigrationInListenerComponent', + type: 'full-transform', + sourceCode: ` +import { Component } from '@angular/core'; + +@Component({ + selector: 'app-safe-nav-migration-in-listener', + standalone: true, + template: \` + @for (option of options; track $index) { + + } + @if (user; as u) { + + } + \`, +}) +export class SafeNavMigrationInListenerComponent { + options: Array<{ value: string; label: string } | null> = []; + user: { flags?: { verified: boolean } } | null = null; + pick(value: string | null) {} + save(event: Event, verified: boolean | null) {} +} + `.trim(), + expectedFeatures: ['ɵɵlistener', 'ɵɵrestoreView'], + }, ] From 424a09e8342d29da865be8d2937c46cca696f5a0 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Tue, 20 Oct 2026 15:45:09 +0800 Subject: [PATCH 2/3] test(compiler): cover arrow functions and more template locals for `$safeNavigationMigration` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - resolve_names: the wrapper arm comment now names both direct roots (a listener's handler_expression and hoisted arrow-function bodies). - Listener test: add ng-template `let-`, root-view `#ref` and `@let` cases; assert the exact declaration and read for each local. - New test for arrow functions: the hoisted factory restores the local from its own view and returns through `ɵɵresetView`. - Move the compare fixture into safe-navigation-migration.fixture.ts and add more listener locals plus an arrow-function entry; both match Angular 22.2.2. Co-Authored-By: Claude Opus 5.5 --- .../src/pipeline/phases/resolve_names.rs | 6 +- .../tests/integration_test.rs | 120 ++++++++++++++---- .../safe-navigation-chain.fixture.ts | 30 ----- .../safe-navigation-migration.fixture.ts | 79 ++++++++++++ 4 files changed, 176 insertions(+), 59 deletions(-) create mode 100644 napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-migration.fixture.ts diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs index 88eafb7f8..b51d0605f 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs @@ -877,9 +877,9 @@ fn resolve_expression<'a>( ); } - // `$safeNavigationMigration(...)` wrapper - resolve the wrapped expression. A - // listener's `handler_expression` is resolved through this function directly - // rather than the generic visitor, so the wrapper must be descended here. + // `$safeNavigationMigration(...)` wrapper - resolve the wrapped expression. A listener's + // `handler_expression` and hoisted arrow-function bodies reach this function directly, + // not through the generic visitor, so the wrapper must be descended here. IrExpression::SafeNavigationMigration(migration) => { resolve_expression( migration.expr.as_mut(), diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index 597bb8fb6..2cd164f85 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -2342,35 +2342,103 @@ fn test_safe_navigation_migration_ignores_qualified_call() { #[test] fn test_safe_navigation_migration_in_listener_reads_template_locals() { // Inside a listener, the wrapped expression must resolve template locals - // (`@for` items, `@if ... as` aliases) to the restored-view variables, like - // any other listener expression, instead of falling back to `ctx.`. - let js = compile_template_to_js_with_version( - r#"@for (option of options; track $index) { + // (`@for` items, `@if ... as` aliases, `let-` variables, `#ref`s, `@let`s) to + // the restored-view variables, like any other listener expression, instead of + // falling back to `ctx.`. + // (template, declaration of the local, read inside the wrapper, old context read) + let cases = [ + ( + r#"@for (option of options; track $index) { -} -@if (user; as u) { +}"#, + "const option_r2 = i0.ɵɵrestoreView(_r1).$implicit;", + "ctx_r2.pick(((option_r2 == null)? null: option_r2.value))", + "ctx.option.value", + ), + ( + r#"@if (user; as u) { }"#, - "TestComponent", - Some(AngularVersion::new(22, 0, 0)), - ); - assert!( - !js.contains("ctx.option ==") && !js.contains("ctx.option.value"), - "`option` must not be read from the context, got:\n{js}" - ); - assert!( - !js.contains("ctx.u ==") && !js.contains("ctx.u.id"), - "`u` must not be read from the context, got:\n{js}" - ); - assert!( - js.contains(".pick(((option_r") && js.contains("== null)? null: option_r"), - "`option` must be read from the restored view with legacy null semantics, got:\n{js}" - ); - assert!( - js.contains(".save(((u_r") && js.contains("== null)? null: u_r"), - "`u` must be read from the restored view with legacy null semantics, got:\n{js}" - ); - assert!(!js.contains("$safeNavigationMigration"), "the wrapper must be stripped, got:\n{js}"); + "const u_r2 = i0.ɵɵrestoreView(_r1);", + "ctx_r2.save(((u_r2 == null)? null: u_r2.id))", + "ctx.u.id", + ), + ( + r#" + +"#, + "const item_r2 = i0.ɵɵrestoreView(_r1).$implicit;", + "ctx_r2.pick(((item_r2 == null)? null: item_r2.value))", + "ctx.item.value", + ), + ( + r#" +"#, + "const box_r2 = i0.ɵɵreference(1);", + "ctx.pick(((box_r2 == null)? null: box_r2.value))", + "ctx.box.value", + ), + ( + r#"@let v = user?.profile; +"#, + "const v_r2 = i0.ɵɵreadContextLet(0);", + "ctx.pick(((v_r2 == null)? null: v_r2.name))", + "ctx.v.name", + ), + ]; + for (template, declaration, read, old_read) in cases { + let js = compile_template_to_js_with_version( + template, + "TestComponent", + Some(AngularVersion::new(22, 0, 0)), + ); + assert!(js.contains(declaration), "expected `{declaration}`, got:\n{js}"); + assert!(js.contains(read), "expected `{read}`, got:\n{js}"); + assert!(!js.contains(old_read), "must not read `{old_read}`, got:\n{js}"); + assert!( + !js.contains("$safeNavigationMigration"), + "the wrapper must be stripped, got:\n{js}" + ); + } +} + +#[test] +fn test_safe_navigation_migration_in_arrow_function_reads_template_locals() { + // A template arrow function is hoisted into a module-level factory that gets + // the view as a parameter. The wrapped expression in its body must restore + // the local from that view (and return through `ɵɵresetView`) instead of + // reading a free variable that only exists inside the template function. + // (template, declaration of the local, return of the factory) + let cases = [ + ( + r#"@for (option of options; track $index) { +
+}"#, + "const option_r1 = i0.ɵɵrestoreView(view).$implicit;", + "return i0.ɵɵresetView(((option_r1 == null)? null: option_r1.value));", + ), + ( + r#"@if (user; as u) { +
+}"#, + "const u_r1 = i0.ɵɵrestoreView(view);", + "return i0.ɵɵresetView(((u_r1 == null)? null: u_r1.id));", + ), + ]; + for (template, declaration, ret) in cases { + let js = compile_template_to_js_with_version( + template, + "TestComponent", + Some(AngularVersion::new(22, 0, 0)), + ); + let factory = + format!("const arrowFn0 = (ctx,view) =>() =>{{\n {declaration}\n {ret}\n}};"); + assert!(js.contains(&factory), "expected the arrow factory\n{factory}\ngot:\n{js}"); + assert!( + !js.contains("$safeNavigationMigration"), + "the wrapper must be stripped, got:\n{js}" + ); + } } // ============================================================================ diff --git a/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-chain.fixture.ts b/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-chain.fixture.ts index 0437d721d..89d953a76 100644 --- a/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-chain.fixture.ts +++ b/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-chain.fixture.ts @@ -125,34 +125,4 @@ export class SafeNavCombinedComponent { `.trim(), expectedFeatures: ['ɵɵtextInterpolate1'], }, - { - name: 'safe-nav-migration-in-listener', - category: 'regressions', - description: '$safeNavigationMigration in listeners reading template locals', - className: 'SafeNavMigrationInListenerComponent', - type: 'full-transform', - sourceCode: ` -import { Component } from '@angular/core'; - -@Component({ - selector: 'app-safe-nav-migration-in-listener', - standalone: true, - template: \` - @for (option of options; track $index) { - - } - @if (user; as u) { - - } - \`, -}) -export class SafeNavMigrationInListenerComponent { - options: Array<{ value: string; label: string } | null> = []; - user: { flags?: { verified: boolean } } | null = null; - pick(value: string | null) {} - save(event: Event, verified: boolean | null) {} -} - `.trim(), - expectedFeatures: ['ɵɵlistener', 'ɵɵrestoreView'], - }, ] diff --git a/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-migration.fixture.ts b/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-migration.fixture.ts new file mode 100644 index 000000000..7bec14418 --- /dev/null +++ b/napi/angular-compiler/e2e/compare/fixtures/regressions/safe-navigation-migration.fixture.ts @@ -0,0 +1,79 @@ +/** + * Regression: Template locals inside $safeNavigationMigration. + * + * Issue: Template locals read inside $safeNavigationMigration(...) in + * listeners and arrow functions must resolve to the restored-view + * variables instead of the component context (#585). + */ +import type { Fixture } from '../types.js' + +export const fixtures: Fixture[] = [ + { + name: 'safe-nav-migration-in-listener', + category: 'regressions', + description: '$safeNavigationMigration in listeners reading template locals', + className: 'SafeNavMigrationInListenerComponent', + type: 'full-transform', + sourceCode: ` +import { Component } from '@angular/core'; + +@Component({ + selector: 'app-safe-nav-migration-in-listener', + standalone: true, + template: \` + @for (option of options; track $index) { + + + } + @if (user; as u) { + + } + + + + + + @let firstOption = options[0]; + + \`, +}) +export class SafeNavMigrationInListenerComponent { + options: Array<{ value: string; label: string } | null> = []; + selected: { value: string } | null = null; + user: { flags?: { verified: boolean } } | null = null; + pick(value: string | null) {} + save(event: Event, verified: boolean | null) {} +} + `.trim(), + expectedFeatures: ['ɵɵlistener', 'ɵɵreference', 'ɵɵreadContextLet'], + }, + { + name: 'safe-nav-migration-in-arrow-function', + category: 'regressions', + description: '$safeNavigationMigration in arrow functions reading template locals', + className: 'SafeNavMigrationInArrowFunctionComponent', + type: 'full-transform', + sourceCode: ` +import { Component } from '@angular/core'; + +@Component({ + selector: 'app-safe-nav-migration-in-arrow-function', + standalone: true, + template: \` + @for (option of options; track $index) { +
+ {{ fmt(() => $safeNavigationMigration(option?.label)) }} +
+ } + \`, +}) +export class SafeNavMigrationInArrowFunctionComponent { + options: Array<{ value: string; label: string } | null> = []; + fmt(fn: () => string | null) { + return fn() ?? ''; + } +} + `.trim(), + expectedFeatures: ['ɵɵarrowFunction', 'ɵɵrestoreView', 'ɵɵresetView'], + }, +] From afe989331d8483a14ffbccf37cb9b58d6fc581ed Mon Sep 17 00:00:00 2001 From: LongYinan Date: Tue, 20 Oct 2026 15:58:16 +0800 Subject: [PATCH 3/3] test(compiler): assert whole listener bodies in the `$safeNavigationMigration` test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Assert every statement of each listener body as one block, as the arrow-function test does. The old checks still passed when the root-view `#ref`/`@let` listeners had no `ɵɵrestoreView`/`ɵɵresetView`, which is the pre-fix shape. Co-Authored-By: Claude Opus 5.5 --- .../tests/integration_test.rs | 43 +++++++++++++------ 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index 2cd164f85..c52f33f6b 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -2345,55 +2345,70 @@ fn test_safe_navigation_migration_in_listener_reads_template_locals() { // (`@for` items, `@if ... as` aliases, `let-` variables, `#ref`s, `@let`s) to // the restored-view variables, like any other listener expression, instead of // falling back to `ctx.`. - // (template, declaration of the local, read inside the wrapper, old context read) + // (template, statements of the listener body, old context read) let cases = [ ( r#"@for (option of options; track $index) { }"#, - "const option_r2 = i0.ɵɵrestoreView(_r1).$implicit;", - "ctx_r2.pick(((option_r2 == null)? null: option_r2.value))", + [ + "const option_r2 = i0.ɵɵrestoreView(_r1).$implicit;", + "const ctx_r2 = i0.ɵɵnextContext();", + "return i0.ɵɵresetView(ctx_r2.pick(((option_r2 == null)? null: option_r2.value)));", + ], "ctx.option.value", ), ( r#"@if (user; as u) { }"#, - "const u_r2 = i0.ɵɵrestoreView(_r1);", - "ctx_r2.save(((u_r2 == null)? null: u_r2.id))", + [ + "const u_r2 = i0.ɵɵrestoreView(_r1);", + "const ctx_r2 = i0.ɵɵnextContext();", + "return i0.ɵɵresetView(ctx_r2.save(((u_r2 == null)? null: u_r2.id)));", + ], "ctx.u.id", ), ( r#" "#, - "const item_r2 = i0.ɵɵrestoreView(_r1).$implicit;", - "ctx_r2.pick(((item_r2 == null)? null: item_r2.value))", + [ + "const item_r2 = i0.ɵɵrestoreView(_r1).$implicit;", + "const ctx_r2 = i0.ɵɵnextContext();", + "return i0.ɵɵresetView(ctx_r2.pick(((item_r2 == null)? null: item_r2.value)));", + ], "ctx.item.value", ), ( r#" "#, - "const box_r2 = i0.ɵɵreference(1);", - "ctx.pick(((box_r2 == null)? null: box_r2.value))", + [ + "i0.ɵɵrestoreView(_r1);", + "const box_r2 = i0.ɵɵreference(1);", + "return i0.ɵɵresetView(ctx.pick(((box_r2 == null)? null: box_r2.value)));", + ], "ctx.box.value", ), ( r#"@let v = user?.profile; "#, - "const v_r2 = i0.ɵɵreadContextLet(0);", - "ctx.pick(((v_r2 == null)? null: v_r2.name))", + [ + "i0.ɵɵrestoreView(_r1);", + "const v_r2 = i0.ɵɵreadContextLet(0);", + "return i0.ɵɵresetView(ctx.pick(((v_r2 == null)? null: v_r2.name)));", + ], "ctx.v.name", ), ]; - for (template, declaration, read, old_read) in cases { + for (template, statements, old_read) in cases { let js = compile_template_to_js_with_version( template, "TestComponent", Some(AngularVersion::new(22, 0, 0)), ); - assert!(js.contains(declaration), "expected `{declaration}`, got:\n{js}"); - assert!(js.contains(read), "expected `{read}`, got:\n{js}"); + let body = format!("_listener() {{\n {}\n }});", statements.join("\n ")); + assert!(js.contains(&body), "expected the listener body\n{body}\ngot:\n{js}"); assert!(!js.contains(old_read), "must not read `{old_read}`, got:\n{js}"); assert!( !js.contains("$safeNavigationMigration"),