Skip to content

Commit 565ef32

Browse files
Brooooooklynclaude
andcommitted
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 <noreply@anthropic.com>
1 parent bf443cb commit 565ef32

5 files changed

Lines changed: 204 additions & 9 deletions

File tree

crates/oxc_angular_compiler/src/ir/expression.rs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2092,12 +2092,13 @@ pub fn transform_expressions_in_create_op<'a, F>(
20922092
transform_expressions_in_expression(&mut op.initializer, transform, flags);
20932093
}
20942094
CreateOp::Listener(op) => {
2095-
// Process handler expression if present
2095+
// Process handler ops and handler_expression as child operations.
2096+
// handler_expression is the return expression of the listener function and
2097+
// must be treated as part of the handler scope (not the parent scope).
2098+
let child_flags = flags.union(VisitorContextFlag::IN_CHILD_OPERATION);
20962099
if let Some(handler_expr) = &mut op.handler_expression {
2097-
transform_expressions_in_expression(handler_expr, transform, flags);
2100+
transform_expressions_in_expression(handler_expr, transform, child_flags);
20982101
}
2099-
// Process handler ops in the listener
2100-
let child_flags = flags.union(VisitorContextFlag::IN_CHILD_OPERATION);
21012102
for handler_op in op.handler_ops.iter_mut() {
21022103
transform_expressions_in_update_op(handler_op, transform, child_flags);
21032104
}
@@ -2279,10 +2280,10 @@ pub fn visit_expressions_in_create_op<'a, F>(
22792280
visit_expressions_in_expression(&op.initializer, visitor, flags);
22802281
}
22812282
CreateOp::Listener(op) => {
2283+
let child_flags = flags.union(VisitorContextFlag::IN_CHILD_OPERATION);
22822284
if let Some(handler_expr) = &op.handler_expression {
2283-
visit_expressions_in_expression(handler_expr, visitor, flags);
2285+
visit_expressions_in_expression(handler_expr, visitor, child_flags);
22842286
}
2285-
let child_flags = flags.union(VisitorContextFlag::IN_CHILD_OPERATION);
22862287
for handler_op in op.handler_ops.iter() {
22872288
visit_expressions_in_update_op(handler_op, visitor, child_flags);
22882289
}

crates/oxc_angular_compiler/src/pipeline/phases/temporary_variables.rs

Lines changed: 145 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ use rustc_hash::{FxHashMap, FxHashSet};
3939

4040
use crate::ir::expression::{
4141
IrExpression, VisitorContextFlag, transform_expressions_in_create_op,
42-
transform_expressions_in_update_op, visit_expressions_in_create_op,
42+
transform_expressions_in_expression, transform_expressions_in_update_op,
43+
visit_expressions_in_create_op, visit_expressions_in_expression,
4344
visit_expressions_in_update_op,
4445
};
4546
use crate::ir::list::{CreateOpList, UpdateOpList};
@@ -132,8 +133,16 @@ fn generate_temporaries_for_create<'a>(
132133
// for nested ops lists and the results are prepended to those lists.
133134
match op {
134135
CreateOp::Listener(listener) => {
135-
let mut handler_stmts =
136-
generate_temporaries_for_handler_ops(&mut listener.handler_ops, allocator);
136+
// Process handler_ops and handler_expression together.
137+
// In Angular TS, the handler_expression is part of handlerOps.
138+
// In our IR, it's a separate field, so we must include it in
139+
// temporary variable processing to generate the correct `let tmp_N_0;`
140+
// declarations inside the listener function.
141+
let mut handler_stmts = generate_temporaries_for_handler_ops_with_expression(
142+
&mut listener.handler_ops,
143+
&mut listener.handler_expression,
144+
allocator,
145+
);
137146
prepend_update_ops(&mut listener.handler_ops, &mut handler_stmts, allocator);
138147
}
139148
CreateOp::AnimationListener(listener) => {
@@ -219,6 +228,139 @@ fn generate_temporaries_for_handler_ops<'a>(
219228
generated_statements
220229
}
221230

231+
/// Generates temporary variable declarations for handler_ops AND handler_expression together.
232+
///
233+
/// In Angular TS, the handler_expression is part of handlerOps (there is no separate field).
234+
/// In our IR, handler_expression is a separate field on ListenerOp. We must include it in
235+
/// temporary variable processing so that `let tmp_N_0;` declarations are generated inside
236+
/// the listener function scope rather than the parent create scope.
237+
///
238+
/// The algorithm mirrors `generate_temporaries_for_handler_ops` but additionally visits/transforms
239+
/// the handler_expression alongside the ops.
240+
fn generate_temporaries_for_handler_ops_with_expression<'a>(
241+
ops: &mut [UpdateOp<'a>],
242+
handler_expression: &mut Option<oxc_allocator::Box<'a, IrExpression<'a>>>,
243+
allocator: &'a Allocator,
244+
) -> Vec<UpdateOp<'a>> {
245+
let mut op_count = 0;
246+
let mut generated_statements = Vec::new();
247+
248+
for op in ops.iter_mut() {
249+
// Pass 1: Count reads per xref to determine final reads
250+
let read_counts = RefCell::new(FxHashMap::<XrefId, usize>::default());
251+
visit_expressions_in_update_op(
252+
op,
253+
&|expr, flags| {
254+
if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) {
255+
return;
256+
}
257+
if let IrExpression::ReadTemporary(read) = expr {
258+
*read_counts.borrow_mut().entry(read.xref).or_insert(0) += 1;
259+
}
260+
},
261+
VisitorContextFlag::NONE,
262+
);
263+
// Also count reads in handler_expression
264+
if let Some(handler_expr) = handler_expression.as_ref() {
265+
visit_expressions_in_expression(
266+
handler_expr,
267+
&|expr, flags| {
268+
if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) {
269+
return;
270+
}
271+
if let IrExpression::ReadTemporary(read) = expr {
272+
*read_counts.borrow_mut().entry(read.xref).or_insert(0) += 1;
273+
}
274+
},
275+
VisitorContextFlag::NONE,
276+
);
277+
}
278+
let total_reads = read_counts.into_inner();
279+
280+
// Pass 2: Assign names with reuse when final read is encountered
281+
let tracker = RefCell::new(TempVarTracker::new(op_count, total_reads));
282+
283+
transform_expressions_in_update_op(
284+
op,
285+
&|expr, flags| {
286+
if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) {
287+
return;
288+
}
289+
assign_temp_names(expr, &tracker, allocator);
290+
},
291+
VisitorContextFlag::NONE,
292+
);
293+
// Also assign names in handler_expression
294+
if let Some(handler_expr) = handler_expression.as_mut() {
295+
transform_expressions_in_expression(
296+
handler_expr,
297+
&|expr, flags| {
298+
if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) {
299+
return;
300+
}
301+
assign_temp_names(expr, &tracker, allocator);
302+
},
303+
VisitorContextFlag::NONE,
304+
);
305+
}
306+
307+
// Collect unique names and create declarations
308+
let defs = tracker.into_inner().defs;
309+
for name in collect_unique_names(&defs) {
310+
let stmt = create_declare_var_statement(allocator, &name);
311+
generated_statements.push(UpdateOp::Statement(StatementOp {
312+
base: UpdateOpBase::default(),
313+
statement: stmt,
314+
}));
315+
}
316+
317+
op_count += 1;
318+
}
319+
320+
// If there are no ops but handler_expression has temporaries, process it standalone
321+
if ops.is_empty() {
322+
if let Some(handler_expr) = handler_expression.as_mut() {
323+
let read_counts = RefCell::new(FxHashMap::<XrefId, usize>::default());
324+
visit_expressions_in_expression(
325+
handler_expr,
326+
&|expr, flags| {
327+
if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) {
328+
return;
329+
}
330+
if let IrExpression::ReadTemporary(read) = expr {
331+
*read_counts.borrow_mut().entry(read.xref).or_insert(0) += 1;
332+
}
333+
},
334+
VisitorContextFlag::NONE,
335+
);
336+
let total_reads = read_counts.into_inner();
337+
338+
let tracker = RefCell::new(TempVarTracker::new(op_count, total_reads));
339+
transform_expressions_in_expression(
340+
handler_expr,
341+
&|expr, flags| {
342+
if flags.contains(VisitorContextFlag::IN_CHILD_OPERATION) {
343+
return;
344+
}
345+
assign_temp_names(expr, &tracker, allocator);
346+
},
347+
VisitorContextFlag::NONE,
348+
);
349+
350+
let defs = tracker.into_inner().defs;
351+
for name in collect_unique_names(&defs) {
352+
let stmt = create_declare_var_statement(allocator, &name);
353+
generated_statements.push(UpdateOp::Statement(StatementOp {
354+
base: UpdateOpBase::default(),
355+
statement: stmt,
356+
}));
357+
}
358+
}
359+
}
360+
361+
generated_statements
362+
}
363+
222364
/// Prepends generated statement ops to a handler_ops Vec.
223365
/// Since handler_ops is a Vec (not OpList), we need to handle prepending manually.
224366
fn prepend_update_ops<'a>(

crates/oxc_angular_compiler/tests/integration_test.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,6 +1040,28 @@ fn test_safe_call() {
10401040
insta::assert_snapshot!("safe_call", js);
10411041
}
10421042

1043+
#[test]
1044+
fn test_safe_call_in_listener() {
1045+
// When a listener handler contains `fn()?.method()`, Angular generates a temporary variable
1046+
// because `fn()` is a function call that shouldn't be evaluated twice.
1047+
// Expected output should contain `let tmp_N_0;` declaration inside the listener function.
1048+
let js = compile_template_to_js(
1049+
r#"<button (click)="getPopover()?.close()">Close</button>"#,
1050+
"TestComponent",
1051+
);
1052+
insta::assert_snapshot!("safe_call_in_listener", js);
1053+
}
1054+
1055+
#[test]
1056+
fn test_safe_property_read_with_call_receiver_in_listener() {
1057+
// Pattern: `fn()?.prop` in listener — receiver is a call, needs tmp variable
1058+
let js = compile_template_to_js(
1059+
r#"<button (click)="getDialog()?.visible">Toggle</button>"#,
1060+
"TestComponent",
1061+
);
1062+
insta::assert_snapshot!("safe_property_read_with_call_receiver_in_listener", js);
1063+
}
1064+
10431065
// ============================================================================
10441066
// Event Modifier Tests
10451067
// ============================================================================
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
source: crates/oxc_angular_compiler/tests/integration_test.rs
3+
expression: js
4+
---
5+
function TestComponent_Template(rf,ctx) {
6+
if ((rf & 1)) {
7+
i0.ɵɵelementStart(0,"button",0);
8+
i0.ɵɵlistener("click",function TestComponent_Template_button_click_0_listener() {
9+
let tmp_0_0;
10+
return (((tmp_0_0 = ctx.getPopover()) == null)? null: tmp_0_0.close());
11+
});
12+
i0.ɵɵtext(1,"Close");
13+
i0.ɵɵelementEnd();
14+
}
15+
}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
source: crates/oxc_angular_compiler/tests/integration_test.rs
3+
expression: js
4+
---
5+
function TestComponent_Template(rf,ctx) {
6+
if ((rf & 1)) {
7+
i0.ɵɵelementStart(0,"button",0);
8+
i0.ɵɵlistener("click",function TestComponent_Template_button_click_0_listener() {
9+
let tmp_0_0;
10+
return (((tmp_0_0 = ctx.getDialog()) == null)? null: tmp_0_0.visible);
11+
});
12+
i0.ɵɵtext(1,"Toggle");
13+
i0.ɵɵelementEnd();
14+
}
15+
}

0 commit comments

Comments
 (0)