From 565ef32391df5e404f58c5ea628a440398b3b50b Mon Sep 17 00:00:00 2001 From: LongYinan Date: Wed, 4 Feb 2026 15:53:53 +0800 Subject: [PATCH 1/2] fix: listener handler_expression should generate tmp variable declarations in handler scope handler_expression was being processed without the IN_CHILD_OPERATION flag, causing temporary variables to be scoped to the parent create block instead of the listener function. This meant `let tmp_N_0;` declarations were missing inside listener functions that use safe navigation on function call receivers (e.g., `getPopover()?.close()`). Co-Authored-By: Claude Opus 4.5 --- .../oxc_angular_compiler/src/ir/expression.rs | 13 +- .../pipeline/phases/temporary_variables.rs | 148 +++++++++++++++++- .../tests/integration_test.rs | 22 +++ ...tegration_test__safe_call_in_listener.snap | 15 ++ ...y_read_with_call_receiver_in_listener.snap | 15 ++ 5 files changed, 204 insertions(+), 9 deletions(-) create mode 100644 crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_call_in_listener.snap create mode 100644 crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_property_read_with_call_receiver_in_listener.snap diff --git a/crates/oxc_angular_compiler/src/ir/expression.rs b/crates/oxc_angular_compiler/src/ir/expression.rs index 5f1b6b10d..a58538add 100644 --- a/crates/oxc_angular_compiler/src/ir/expression.rs +++ b/crates/oxc_angular_compiler/src/ir/expression.rs @@ -2092,12 +2092,13 @@ pub fn transform_expressions_in_create_op<'a, F>( transform_expressions_in_expression(&mut op.initializer, transform, flags); } CreateOp::Listener(op) => { - // Process handler expression if present + // Process handler ops and handler_expression as child operations. + // handler_expression is the return expression of the listener function and + // must be treated as part of the handler scope (not the parent scope). + let child_flags = flags.union(VisitorContextFlag::IN_CHILD_OPERATION); if let Some(handler_expr) = &mut op.handler_expression { - transform_expressions_in_expression(handler_expr, transform, flags); + transform_expressions_in_expression(handler_expr, transform, child_flags); } - // Process handler ops in the listener - let child_flags = flags.union(VisitorContextFlag::IN_CHILD_OPERATION); for handler_op in op.handler_ops.iter_mut() { transform_expressions_in_update_op(handler_op, transform, child_flags); } @@ -2279,10 +2280,10 @@ pub fn visit_expressions_in_create_op<'a, F>( visit_expressions_in_expression(&op.initializer, visitor, flags); } CreateOp::Listener(op) => { + let child_flags = flags.union(VisitorContextFlag::IN_CHILD_OPERATION); if let Some(handler_expr) = &op.handler_expression { - visit_expressions_in_expression(handler_expr, visitor, flags); + visit_expressions_in_expression(handler_expr, visitor, child_flags); } - let child_flags = flags.union(VisitorContextFlag::IN_CHILD_OPERATION); for handler_op in op.handler_ops.iter() { visit_expressions_in_update_op(handler_op, visitor, child_flags); } diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs b/crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs index 0111fe8c2..97f680beb 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs @@ -39,7 +39,8 @@ use rustc_hash::{FxHashMap, FxHashSet}; use crate::ir::expression::{ IrExpression, VisitorContextFlag, transform_expressions_in_create_op, - transform_expressions_in_update_op, visit_expressions_in_create_op, + transform_expressions_in_expression, transform_expressions_in_update_op, + visit_expressions_in_create_op, visit_expressions_in_expression, visit_expressions_in_update_op, }; use crate::ir::list::{CreateOpList, UpdateOpList}; @@ -132,8 +133,16 @@ fn generate_temporaries_for_create<'a>( // for nested ops lists and the results are prepended to those lists. match op { CreateOp::Listener(listener) => { - let mut handler_stmts = - generate_temporaries_for_handler_ops(&mut listener.handler_ops, allocator); + // Process handler_ops and handler_expression together. + // In Angular TS, the handler_expression is part of handlerOps. + // In our IR, it's a separate field, so we must include it in + // temporary variable processing to generate the correct `let tmp_N_0;` + // declarations inside the listener function. + let mut handler_stmts = generate_temporaries_for_handler_ops_with_expression( + &mut listener.handler_ops, + &mut listener.handler_expression, + allocator, + ); prepend_update_ops(&mut listener.handler_ops, &mut handler_stmts, allocator); } CreateOp::AnimationListener(listener) => { @@ -219,6 +228,139 @@ fn generate_temporaries_for_handler_ops<'a>( generated_statements } +/// Generates temporary variable declarations for handler_ops AND handler_expression together. +/// +/// In Angular TS, the handler_expression is part of handlerOps (there is no separate field). +/// In our IR, handler_expression is a separate field on ListenerOp. We must include it in +/// temporary variable processing so that `let tmp_N_0;` declarations are generated inside +/// the listener function scope rather than the parent create scope. +/// +/// The algorithm mirrors `generate_temporaries_for_handler_ops` but additionally visits/transforms +/// the handler_expression alongside the ops. +fn generate_temporaries_for_handler_ops_with_expression<'a>( + ops: &mut [UpdateOp<'a>], + handler_expression: &mut Option>>, + allocator: &'a Allocator, +) -> Vec> { + let mut op_count = 0; + let mut generated_statements = Vec::new(); + + for op in ops.iter_mut() { + // Pass 1: Count reads per xref to determine final reads + let read_counts = RefCell::new(FxHashMap::::default()); + visit_expressions_in_update_op( + op, + &|expr, flags| { + if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { + return; + } + if let IrExpression::ReadTemporary(read) = expr { + *read_counts.borrow_mut().entry(read.xref).or_insert(0) += 1; + } + }, + VisitorContextFlag::NONE, + ); + // Also count reads in handler_expression + if let Some(handler_expr) = handler_expression.as_ref() { + visit_expressions_in_expression( + handler_expr, + &|expr, flags| { + if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { + return; + } + if let IrExpression::ReadTemporary(read) = expr { + *read_counts.borrow_mut().entry(read.xref).or_insert(0) += 1; + } + }, + VisitorContextFlag::NONE, + ); + } + let total_reads = read_counts.into_inner(); + + // Pass 2: Assign names with reuse when final read is encountered + let tracker = RefCell::new(TempVarTracker::new(op_count, total_reads)); + + transform_expressions_in_update_op( + op, + &|expr, flags| { + if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { + return; + } + assign_temp_names(expr, &tracker, allocator); + }, + VisitorContextFlag::NONE, + ); + // Also assign names in handler_expression + if let Some(handler_expr) = handler_expression.as_mut() { + transform_expressions_in_expression( + handler_expr, + &|expr, flags| { + if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { + return; + } + assign_temp_names(expr, &tracker, allocator); + }, + VisitorContextFlag::NONE, + ); + } + + // Collect unique names and create declarations + let defs = tracker.into_inner().defs; + for name in collect_unique_names(&defs) { + let stmt = create_declare_var_statement(allocator, &name); + generated_statements.push(UpdateOp::Statement(StatementOp { + base: UpdateOpBase::default(), + statement: stmt, + })); + } + + op_count += 1; + } + + // If there are no ops but handler_expression has temporaries, process it standalone + if ops.is_empty() { + if let Some(handler_expr) = handler_expression.as_mut() { + let read_counts = RefCell::new(FxHashMap::::default()); + visit_expressions_in_expression( + handler_expr, + &|expr, flags| { + if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { + return; + } + if let IrExpression::ReadTemporary(read) = expr { + *read_counts.borrow_mut().entry(read.xref).or_insert(0) += 1; + } + }, + VisitorContextFlag::NONE, + ); + let total_reads = read_counts.into_inner(); + + let tracker = RefCell::new(TempVarTracker::new(op_count, total_reads)); + transform_expressions_in_expression( + handler_expr, + &|expr, flags| { + if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { + return; + } + assign_temp_names(expr, &tracker, allocator); + }, + VisitorContextFlag::NONE, + ); + + let defs = tracker.into_inner().defs; + for name in collect_unique_names(&defs) { + let stmt = create_declare_var_statement(allocator, &name); + generated_statements.push(UpdateOp::Statement(StatementOp { + base: UpdateOpBase::default(), + statement: stmt, + })); + } + } + } + + generated_statements +} + /// Prepends generated statement ops to a handler_ops Vec. /// Since handler_ops is a Vec (not OpList), we need to handle prepending manually. fn prepend_update_ops<'a>( diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index a1b339e9c..9ad3a768e 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -1040,6 +1040,28 @@ fn test_safe_call() { insta::assert_snapshot!("safe_call", js); } +#[test] +fn test_safe_call_in_listener() { + // When a listener handler contains `fn()?.method()`, Angular generates a temporary variable + // because `fn()` is a function call that shouldn't be evaluated twice. + // Expected output should contain `let tmp_N_0;` declaration inside the listener function. + let js = compile_template_to_js( + r#""#, + "TestComponent", + ); + insta::assert_snapshot!("safe_call_in_listener", js); +} + +#[test] +fn test_safe_property_read_with_call_receiver_in_listener() { + // Pattern: `fn()?.prop` in listener — receiver is a call, needs tmp variable + let js = compile_template_to_js( + r#""#, + "TestComponent", + ); + insta::assert_snapshot!("safe_property_read_with_call_receiver_in_listener", js); +} + // ============================================================================ // Event Modifier Tests // ============================================================================ diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_call_in_listener.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_call_in_listener.snap new file mode 100644 index 000000000..462f7b737 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_call_in_listener.snap @@ -0,0 +1,15 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +function TestComponent_Template(rf,ctx) { + if ((rf & 1)) { + i0.ɵɵelementStart(0,"button",0); + i0.ɵɵlistener("click",function TestComponent_Template_button_click_0_listener() { + let tmp_0_0; + return (((tmp_0_0 = ctx.getPopover()) == null)? null: tmp_0_0.close()); + }); + i0.ɵɵtext(1,"Close"); + i0.ɵɵelementEnd(); + } +} diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_property_read_with_call_receiver_in_listener.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_property_read_with_call_receiver_in_listener.snap new file mode 100644 index 000000000..e5643ec44 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_property_read_with_call_receiver_in_listener.snap @@ -0,0 +1,15 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +function TestComponent_Template(rf,ctx) { + if ((rf & 1)) { + i0.ɵɵelementStart(0,"button",0); + i0.ɵɵlistener("click",function TestComponent_Template_button_click_0_listener() { + let tmp_0_0; + return (((tmp_0_0 = ctx.getDialog()) == null)? null: tmp_0_0.visible); + }); + i0.ɵɵtext(1,"Toggle"); + i0.ɵɵelementEnd(); + } +} From ac7b95c18cf32433c580db90dc1a731504829c7b Mon Sep 17 00:00:00 2001 From: LongYinan Date: Wed, 4 Feb 2026 17:16:20 +0800 Subject: [PATCH 2/2] fix: process handler_expression after handler_ops to avoid duplicate tmp declarations handler_expression was being processed inside the per-op loop, causing it to be visited/transformed once per handler_op. When handler_ops had multiple entries (e.g., restoreView + nextContext), this produced spurious declarations (let tmp_0_0; let tmp_1_0;) and wrong variable names. Now handler_expression is processed once after the loop with op_count set to handler_ops.len(), matching Angular TS where the return expression is the last entry in handlerOps. For a listener with 2 handler_ops, this correctly generates tmp_2_0 instead of tmp_1_0. Co-Authored-By: Claude Opus 4.5 --- .../pipeline/phases/temporary_variables.rs | 97 +++---------------- .../tests/integration_test.rs | 13 +++ ...e_call_in_listener_inside_conditional.snap | 24 +++++ 3 files changed, 53 insertions(+), 81 deletions(-) create mode 100644 crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_call_in_listener_inside_conditional.snap diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs b/crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs index 97f680beb..6c2bced42 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs @@ -235,21 +235,28 @@ fn generate_temporaries_for_handler_ops<'a>( /// temporary variable processing so that `let tmp_N_0;` declarations are generated inside /// the listener function scope rather than the parent create scope. /// -/// The algorithm mirrors `generate_temporaries_for_handler_ops` but additionally visits/transforms -/// the handler_expression alongside the ops. +/// The handler_ops are processed first (same as `generate_temporaries_for_handler_ops`), +/// then handler_expression is processed once AFTER the loop with op_count equal to +/// handler_ops.len(). This matches Angular TS where the return expression is the last +/// entry in handlerOps at index N. fn generate_temporaries_for_handler_ops_with_expression<'a>( ops: &mut [UpdateOp<'a>], handler_expression: &mut Option>>, allocator: &'a Allocator, ) -> Vec> { - let mut op_count = 0; - let mut generated_statements = Vec::new(); + // First, process handler_ops exactly like generate_temporaries_for_handler_ops + let mut generated_statements = generate_temporaries_for_handler_ops(ops, allocator); + + // Then process handler_expression once, with op_count = ops.len(). + // In Angular TS, the return expression is the last entry in handlerOps, + // so its op index equals the number of preceding ops. + if let Some(handler_expr) = handler_expression.as_mut() { + let op_count = ops.len(); - for op in ops.iter_mut() { // Pass 1: Count reads per xref to determine final reads let read_counts = RefCell::new(FxHashMap::::default()); - visit_expressions_in_update_op( - op, + visit_expressions_in_expression( + handler_expr, &|expr, flags| { if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { return; @@ -260,28 +267,12 @@ fn generate_temporaries_for_handler_ops_with_expression<'a>( }, VisitorContextFlag::NONE, ); - // Also count reads in handler_expression - if let Some(handler_expr) = handler_expression.as_ref() { - visit_expressions_in_expression( - handler_expr, - &|expr, flags| { - if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { - return; - } - if let IrExpression::ReadTemporary(read) = expr { - *read_counts.borrow_mut().entry(read.xref).or_insert(0) += 1; - } - }, - VisitorContextFlag::NONE, - ); - } let total_reads = read_counts.into_inner(); // Pass 2: Assign names with reuse when final read is encountered let tracker = RefCell::new(TempVarTracker::new(op_count, total_reads)); - - transform_expressions_in_update_op( - op, + transform_expressions_in_expression( + handler_expr, &|expr, flags| { if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { return; @@ -290,19 +281,6 @@ fn generate_temporaries_for_handler_ops_with_expression<'a>( }, VisitorContextFlag::NONE, ); - // Also assign names in handler_expression - if let Some(handler_expr) = handler_expression.as_mut() { - transform_expressions_in_expression( - handler_expr, - &|expr, flags| { - if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { - return; - } - assign_temp_names(expr, &tracker, allocator); - }, - VisitorContextFlag::NONE, - ); - } // Collect unique names and create declarations let defs = tracker.into_inner().defs; @@ -313,49 +291,6 @@ fn generate_temporaries_for_handler_ops_with_expression<'a>( statement: stmt, })); } - - op_count += 1; - } - - // If there are no ops but handler_expression has temporaries, process it standalone - if ops.is_empty() { - if let Some(handler_expr) = handler_expression.as_mut() { - let read_counts = RefCell::new(FxHashMap::::default()); - visit_expressions_in_expression( - handler_expr, - &|expr, flags| { - if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { - return; - } - if let IrExpression::ReadTemporary(read) = expr { - *read_counts.borrow_mut().entry(read.xref).or_insert(0) += 1; - } - }, - VisitorContextFlag::NONE, - ); - let total_reads = read_counts.into_inner(); - - let tracker = RefCell::new(TempVarTracker::new(op_count, total_reads)); - transform_expressions_in_expression( - handler_expr, - &|expr, flags| { - if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) { - return; - } - assign_temp_names(expr, &tracker, allocator); - }, - VisitorContextFlag::NONE, - ); - - let defs = tracker.into_inner().defs; - for name in collect_unique_names(&defs) { - let stmt = create_declare_var_statement(allocator, &name); - generated_statements.push(UpdateOp::Statement(StatementOp { - base: UpdateOpBase::default(), - statement: stmt, - })); - } - } } generated_statements diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index 9ad3a768e..dbc37245c 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -1062,6 +1062,19 @@ fn test_safe_property_read_with_call_receiver_in_listener() { insta::assert_snapshot!("safe_property_read_with_call_receiver_in_listener", js); } +#[test] +fn test_safe_call_in_listener_inside_conditional() { + // When a listener is inside an embedded view (e.g., @if), handler_ops contains + // restoreView and nextContext statements. The handler_expression (return value) + // must be processed AFTER those ops, not once per op, to get the correct + // tmp variable name (tmp_2_0, matching the op index after restoreView and nextContext). + let js = compile_template_to_js( + r#"@if (show) { }"#, + "TestComponent", + ); + insta::assert_snapshot!("safe_call_in_listener_inside_conditional", js); +} + // ============================================================================ // Event Modifier Tests // ============================================================================ diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_call_in_listener_inside_conditional.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_call_in_listener_inside_conditional.snap new file mode 100644 index 000000000..93c69bf78 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__safe_call_in_listener_inside_conditional.snap @@ -0,0 +1,24 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +function TestComponent_Conditional_0_Template(rf,ctx) { + if ((rf & 1)) { + const _r1 = i0.ɵɵgetCurrentView(); + i0.ɵɵtext(0," "); + i0.ɵɵelementStart(1,"button",0); + i0.ɵɵlistener("click",function TestComponent_Conditional_0_Template_button_click_1_listener() { + let tmp_2_0; + i0.ɵɵrestoreView(_r1); + const ctx_r1 = i0.ɵɵnextContext(); + return i0.ɵɵresetView((((tmp_2_0 = ctx_r1.getPopover()) == null)? null: tmp_2_0.close())); + }); + i0.ɵɵtext(2,"Close"); + i0.ɵɵelementEnd(); + i0.ɵɵtext(3," "); + } +} +function TestComponent_Template(rf,ctx) { + if ((rf & 1)) { i0.ɵɵconditionalCreate(0,TestComponent_Conditional_0_Template,4,0); } + if ((rf & 2)) { i0.ɵɵconditional((ctx.show? 0: -1)); } +}