diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/reify/mod.rs b/crates/oxc_angular_compiler/src/pipeline/phases/reify/mod.rs index 8bbb4d9fe..c712fa3c6 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/reify/mod.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/reify/mod.rs @@ -480,6 +480,8 @@ fn reify_create_op<'a>( &repeater.track, repeater.track_fn_name.as_ref(), repeater.uses_component_instance, + &mut repeater.track_by_ops, + diagnostics, ); Some(create_repeater_create_stmt_with_track_expr( @@ -1171,7 +1173,12 @@ fn reify_host_create_op<'a>( /// the track expression as the body, registers it with the constant pool, /// and returns the function reference. /// +/// When `track_by_ops` is `Some`, the ops are reified into statements and assembled +/// into a function body. This handles cases where the track expression needs additional +/// context variable declarations (e.g., `nextContext()` calls for outer-scope access). +/// /// Ported from Angular's `reifyTrackBy()` in `reify.ts`. +#[allow(clippy::too_many_arguments)] fn reify_track_by<'a>( allocator: &'a oxc_allocator::Allocator, pool: &mut ConstantPool<'a>, @@ -1180,6 +1187,8 @@ fn reify_track_by<'a>( track: &IrExpression<'a>, track_fn_name: Option<&Atom<'a>>, uses_component_instance: bool, + track_by_ops: &mut Option>>, + diagnostics: &mut Vec, ) -> OutputExpression<'a> { // If the tracking function was already set by optimization phase, return a reference to it if let Some(fn_name) = track_fn_name { @@ -1279,42 +1288,99 @@ fn reify_track_by<'a>( )); } - // Convert the track expression to output expression - let track_body = convert_ir_expression(allocator, track, expressions, root_xref); - // Create the track function with params ($index, $item) - // If uses_component_instance is true, use a regular function expression so `this` is bound correctly. - // Otherwise, use an arrow function. - // Both are registered with the constant pool via getSharedFunctionReference, - // which maintains insertion order in pool.statements. let mut params = OxcVec::with_capacity_in(2, allocator); params.push(FnParam { name: Atom::from("$index") }); params.push(FnParam { name: Atom::from("$item") }); - let fn_expr = if uses_component_instance { - // Regular function expression: function($index, $item) { return track_body; } - // Angular uses o.fn(params, [new o.ReturnStatement(op.track)]) - let mut stmts = OxcVec::with_capacity_in(1, allocator); - stmts.push(OutputStatement::Return(Box::new_in( - ReturnStatement { value: track_body, source_span: None }, - allocator, - ))); + let fn_expr = if let Some(track_ops) = track_by_ops { + // Complex case: track_by_ops is present (set by track_fn_optimization phase). + // This happens when the track expression needs additional ops like context + // variable declarations (e.g., `const group_r2 = nextContext().$implicit`). + // + // Ported from Angular's reify.ts lines 884-904: + // reifyUpdateOperations(unit, op.trackByOps); + // const statements = [...]; // from trackByOps + // fn = op.usesComponentInstance || statements.length !== 1 || !(statements[0] instanceof ReturnStatement) + // ? o.fn(params, statements) + // : o.arrowFn(params, statements[0].value); - OutputExpression::Function(Box::new_in( - FunctionExpr { name: None, params, statements: stmts, source_span: None }, - allocator, - )) + // Reify each op in track_by_ops into output statements + let mut statements = OxcVec::new_in(allocator); + for track_op in track_ops.iter() { + if let Some(stmt) = reify_update_op( + allocator, + track_op, + expressions, + root_xref, + TemplateCompilationMode::Full, + diagnostics, + ) { + statements.push(stmt); + } + } + + // Determine whether to use function or arrow: + // Angular uses function when: + // - usesComponentInstance is true, OR + // - there are multiple statements, OR + // - the single statement is not a ReturnStatement + let use_function = uses_component_instance + || statements.len() != 1 + || !matches!(statements.first(), Some(OutputStatement::Return(_))); + + if use_function { + OutputExpression::Function(Box::new_in( + FunctionExpr { name: None, params, statements, source_span: None }, + allocator, + )) + } else { + // Single return statement → extract value for arrow function body + // Clone the return value since we can't move out of the Box + let return_value = if let Some(OutputStatement::Return(ret)) = statements.first() { + ret.value.clone_in(allocator) + } else { + unreachable!("checked above that there's exactly one Return statement"); + }; + + OutputExpression::ArrowFunction(Box::new_in( + ArrowFunctionExpr { + params, + body: ArrowFunctionBody::Expression(Box::new_in(return_value, allocator)), + source_span: None, + }, + allocator, + )) + } } else { - // Arrow function: ($index, $item) => track_body - // Angular uses o.arrowFn(params, op.track) - OutputExpression::ArrowFunction(Box::new_in( - ArrowFunctionExpr { - params, - body: ArrowFunctionBody::Expression(Box::new_in(track_body, allocator)), - source_span: None, - }, - allocator, - )) + // Simple case: no track_by_ops. Wrap the raw track expression. + // Ported from Angular's reify.ts lines 878-883: + // fn = op.usesComponentInstance + // ? o.fn(params, [new o.ReturnStatement(op.track)]) + // : o.arrowFn(params, op.track); + let track_body = convert_ir_expression(allocator, track, expressions, root_xref); + + if uses_component_instance { + let mut stmts = OxcVec::with_capacity_in(1, allocator); + stmts.push(OutputStatement::Return(Box::new_in( + ReturnStatement { value: track_body, source_span: None }, + allocator, + ))); + + OutputExpression::Function(Box::new_in( + FunctionExpr { name: None, params, statements: stmts, source_span: None }, + allocator, + )) + } else { + OutputExpression::ArrowFunction(Box::new_in( + ArrowFunctionExpr { + params, + body: ArrowFunctionBody::Expression(Box::new_in(track_body, allocator)), + source_span: None, + }, + allocator, + )) + } }; // Register with constant pool as a shared function diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_contexts.rs b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_contexts.rs index 2f1aeed3d..5d4d1b07c 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_contexts.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_contexts.rs @@ -42,6 +42,15 @@ pub fn resolve_contexts(job: &mut ComponentCompilationJob<'_>) { for view_xref in view_xrefs { let is_root = view_xref == root_xref; if let Some(view) = job.view_mut(view_xref) { + // Process arrow functions' ops first, matching Angular's resolve_contexts.ts: + // for (const expr of unit.functions) { processLexicalScope(unit, expr.ops); } + for fn_ptr in view.functions.iter() { + // SAFETY: The pointer is valid because it was populated by generate_arrow_functions + // and the allocator keeps the data alive. + let arrow_fn = unsafe { &mut **fn_ptr }; + process_lexical_scope_update_vec(allocator, view_xref, is_root, &mut arrow_fn.ops); + } + process_lexical_scope_create(allocator, view_xref, is_root, &mut view.create); process_lexical_scope_update(allocator, view_xref, is_root, &mut view.update); } @@ -79,7 +88,8 @@ fn process_lexical_scope_create<'a>( } // Second pass: Transform ContextExpr to the appropriate expression - // Also recursively process listener handler_ops (per resolve_contexts.ts lines 45-49) + // Also recursively process listener handler_ops and RepeaterCreate track_by_ops + // (per resolve_contexts.ts lines 40-61) for op in ops.iter_mut() { match op { CreateOp::Listener(listener) => { @@ -122,6 +132,26 @@ fn process_lexical_scope_create<'a>( &mut None, ); } + CreateOp::RepeaterCreate(repeater) => { + // Process track_by_ops with their own scope first, matching Angular's + // resolve_contexts.ts lines 55-58: + // case ir.OpKind.RepeaterCreate: + // if (op.trackByOps !== null) { processLexicalScope(view, op.trackByOps); } + if let Some(ref mut track_by_ops) = repeater.track_by_ops { + process_lexical_scope_update_vec(allocator, view_xref, is_root, track_by_ops); + } + // Also transform expressions in the RepeaterCreate op itself (e.g., rep.track) + // using the parent scope. Note: track_by_ops have already been processed above + // so re-visiting them via transform_expressions_in_create_op is a no-op + // (ContextExpr nodes in track_by_ops are already resolved). + transform_expressions_in_create_op( + op, + &|expr, _flags| { + transform_context_expr(allocator, expr, &scope); + }, + VisitorContextFlag::NONE, + ); + } _ => { transform_expressions_in_create_op( op, @@ -251,6 +281,50 @@ fn process_lexical_scope_update<'a>( } } +/// Process update operations in a Vec (used for arrow function ops and track_by_ops). +/// +/// This is the same logic as `process_lexical_scope_update` but works with `Vec` +/// instead of `UpdateOpList`. Needed for `ArrowFunctionExpr.ops` and `RepeaterCreate.track_by_ops`. +fn process_lexical_scope_update_vec<'a>( + allocator: &'a oxc_allocator::Allocator, + view_xref: XrefId, + is_root: bool, + ops: &mut oxc_allocator::Vec<'a, UpdateOp<'a>>, +) { + // Track how to access each view's context by its XrefId. + let mut scope: FxHashMap = FxHashMap::default(); + + // The current view's context is accessible via the `ctx` parameter. + scope.insert(view_xref, ContextAccess::CtxParameter); + + // First pass: Build scope from Variable operations + for op in ops.iter() { + if let UpdateOp::Variable(var_op) = op { + if var_op.kind == SemanticVariableKind::Context { + if let Some(target_view) = var_op.view { + scope.insert(target_view, ContextAccess::ReadVariable(var_op.xref)); + } + } + } + } + + // If this is the root view, prefer `ctx` over any variables + if is_root { + scope.insert(view_xref, ContextAccess::CtxParameter); + } + + // Second pass: Transform ContextExpr to the appropriate expression + for op in ops.iter_mut() { + transform_expressions_in_update_op( + op, + &|expr, _flags| { + transform_context_expr(allocator, expr, &scope); + }, + VisitorContextFlag::NONE, + ); + } +} + /// Transforms a ContextExpr based on the scope. /// /// - For current view context: Keep as ContextExpr (reify handles ctx) 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 df669ba68..d86be92c6 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs @@ -50,25 +50,19 @@ pub fn resolve_names(job: &mut ComponentCompilationJob<'_>) { // Collect expression store data we need first let expression_store_ptr = &job.expressions as *const ExpressionStore<'_>; - // Process each view's create and update operation lists - // IMPORTANT: We need to build scope from BOTH create and update ops before processing listeners, - // because listener handler expressions need access to variables from both phases. + // Process each view's create and update operation lists. + // Per Angular's TypeScript (resolve_names.ts lines 25-26), create and update ops are + // processed as SEPARATE lexical scopes — each builds its own scope/localDefinitions + // from its own Variable ops. This is important because variables from update ops + // (like @for loop items: $implicit, $index) should NOT be visible when resolving + // expressions in create ops (like RepeaterCreate.track). The generateVariables phase + // already prepends all necessary variables to each op list. for view in job.all_views_mut() { // SAFETY: We're only reading from expression_store, not modifying it let expressions = unsafe { &*expression_store_ptr }; - // Build scope from update ops first (contains context variables like $implicit, $index, etc.) - let update_scope = build_scope_from_update_ops(&view.update); - - // Process create ops with the update scope available for listener handler expressions - process_lexical_scope_create( - root_xref, - &mut view.create, - None, - &update_scope, - allocator, - expressions, - ); + // Process create ops with their own scope (no update scope merged in) + process_lexical_scope_create(root_xref, &mut view.create, None, allocator, expressions); process_lexical_scope_update(root_xref, &mut view.update, None, allocator, expressions); } @@ -139,50 +133,17 @@ where maps } -/// Build scope maps from update operations (for variables like context reads). -fn build_scope_from_update_ops<'a>(ops: &crate::ir::list::UpdateOpList<'a>) -> ScopeMaps<'a> { - let mut maps = ScopeMaps::default(); - for op in ops.iter() { - if let UpdateOp::Variable(var_op) = op { - match var_op.kind { - SemanticVariableKind::Identifier => { - // Check if this is a local variable (@let declaration) - if var_op.local { - if !maps.local_definitions.contains_key(&var_op.name) { - maps.local_definitions.insert(var_op.name.clone(), var_op.xref); - } - } else if !maps.scope.contains_key(&var_op.name) { - maps.scope.insert(var_op.name.clone(), var_op.xref); - } - // Also add to scope for non-local (always) - if !maps.scope.contains_key(&var_op.name) { - maps.scope.insert(var_op.name.clone(), var_op.xref); - } - } - SemanticVariableKind::Alias => { - if !maps.scope.contains_key(&var_op.name) { - maps.scope.insert(var_op.name.clone(), var_op.xref); - } - } - _ => {} - } - } - } - maps -} - /// Process create operations to build scope and resolve names. fn process_lexical_scope_create<'a>( root_xref: XrefId, ops: &mut crate::ir::list::CreateOpList<'a>, saved_view: Option, - update_scope: &ScopeMaps<'a>, allocator: &'a oxc_allocator::Allocator, expressions: &ExpressionStore<'a>, ) { // Maps variable names to their XrefIds - // Start with update_scope to include context variables (like @for loop items) - let mut scope: ScopeMaps<'a> = update_scope.clone(); + // Per Angular's TypeScript, each processLexicalScope call starts with a fresh scope. + let mut scope: ScopeMaps<'a> = ScopeMaps::default(); // Track saved view for RestoreView expressions let mut current_saved_view = saved_view; @@ -335,6 +296,12 @@ fn process_lexical_scope_create<'a>( } } _ => { + // Note: RepeaterCreate falls through here. Angular's resolve_names.ts first + // pass (lines 88-92) processes trackByOps with its own scope, but trackByOps + // is always None at this phase (phase 31) — it's created later by + // track_fn_optimization (phase 34). The second pass processes all ops + // (including RepeaterCreate) via transformExpressionsInOp with the parent + // scope, which is what transform_expressions_in_create_op does here. transform_expressions_in_create_op( op, &|expr, _flags| { @@ -1521,18 +1488,8 @@ pub fn resolve_names_for_host(job: &mut HostBindingCompilationJob<'_>) { // SAFETY: We're only reading from expression_store, not modifying it let expressions = unsafe { &*expression_store_ptr }; - // Build scope from update ops - let update_scope = build_scope_from_update_ops(&job.root.update); - - // Process create ops with the update scope available - process_lexical_scope_create( - root_xref, - &mut job.root.create, - None, - &update_scope, - allocator, - expressions, - ); + // Process create ops with their own scope (no update scope merged in) + process_lexical_scope_create(root_xref, &mut job.root.create, None, allocator, expressions); process_lexical_scope_update(root_xref, &mut job.root.update, None, allocator, expressions); // Verify no LexicalRead expressions remain after resolution. diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index 52a9dbfb0..a924ac26d 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -1042,6 +1042,19 @@ fn test_nested_for_loops() { insta::assert_snapshot!("nested_for_loops", js); } +#[test] +fn test_nested_for_with_outer_scope_track() { + // Reproduces the bug where inner @for track expression captures outer-scope variable. + // The inner @for's `track group.id` references `group` from the outer @for. + // Angular generates `function _forTrack1($index,$item) { return this.group.id; }` with + // usesComponentInstance=true, NOT an arrow function with an out-of-scope identifier. + let js = compile_template_to_js( + r"@for (group of groups; track group.id) { @for (item of group.items; track group.id) { {{item.name}} } }", + "TestComponent", + ); + insta::assert_snapshot!("nested_for_with_outer_scope_track", js); +} + #[test] fn test_if_inside_for() { let js = compile_template_to_js( diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__nested_for_with_outer_scope_track.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__nested_for_with_outer_scope_track.snap new file mode 100644 index 000000000..51dd31472 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__nested_for_with_outer_scope_track.snap @@ -0,0 +1,38 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +const _forTrack0 = ($index,$item) =>$item.id; +function _forTrack1($index,$item) { + return this.group.id; +} +function TestComponent_For_1_For_2_Template(rf,ctx) { + if ((rf & 1)) { + i0.ɵɵtext(0," "); + i0.ɵɵelementStart(1,"span"); + i0.ɵɵtext(2); + i0.ɵɵelementEnd(); + i0.ɵɵtext(3," "); + } + if ((rf & 2)) { + const item_r1 = ctx.$implicit; + i0.ɵɵadvance(2); + i0.ɵɵtextInterpolate(item_r1.name); + } +} +function TestComponent_For_1_Template(rf,ctx) { + if ((rf & 1)) { + i0.ɵɵtext(0," "); + i0.ɵɵrepeaterCreate(1,TestComponent_For_1_For_2_Template,4,1,null,null,_forTrack1, + true); + } + if ((rf & 2)) { + const group_r2 = ctx.$implicit; + i0.ɵɵadvance(); + i0.ɵɵrepeater(group_r2.items); + } +} +function TestComponent_Template(rf,ctx) { + if ((rf & 1)) { i0.ɵɵrepeaterCreate(0,TestComponent_For_1_Template,3,0,null,null,_forTrack0); } + if ((rf & 2)) { i0.ɵɵrepeater(ctx.groups); } +}