From 9922fe1ead3dfcd87a16d552415b6d947b1d9937 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Tue, 24 Feb 2026 10:55:20 +0800 Subject: [PATCH] fix(angular): fix 3 compiler divergences from Angular reference 1. (High) Remove incorrect bare property track optimization: Angular does NOT optimize `track id` or `track trackByFn` into direct `ctx.id` references. Only `$index`, `$item`, and `fn($index[, $item])` calls are optimized. Bare property reads now correctly generate wrapper functions like `function _forTrack($index,$item) { return this.id; }`. 2. (Medium) Report diagnostic for missing context in resolve_contexts: Angular throws when a ContextExpr references a view with no scope entry. Now reports an OxcDiagnostic error via job.diagnostics instead of silently keeping the unresolved expression. 3. (Low) Report diagnostic for unknown @for loop variables in ingest: Angular throws an assertion for unknown loop variables. Now reports an OxcDiagnostic error instead of silently returning an empty expression. Co-Authored-By: Claude Opus 4.6 --- .../src/pipeline/ingest.rs | 8 +- .../src/pipeline/phases/resolve_contexts.rs | 66 +++++++++++++---- .../pipeline/phases/track_fn_optimization.rs | 73 ++----------------- .../tests/integration_test.rs | 41 +++++++++++ ...st__for_track_bare_component_property.snap | 26 +++++++ ...test__for_track_bare_method_reference.snap | 26 +++++++ 6 files changed, 157 insertions(+), 83 deletions(-) create mode 100644 crates/oxc_angular_compiler/tests/snapshots/integration_test__for_track_bare_component_property.snap create mode 100644 crates/oxc_angular_compiler/tests/snapshots/integration_test__for_track_bare_method_reference.snap diff --git a/crates/oxc_angular_compiler/src/pipeline/ingest.rs b/crates/oxc_angular_compiler/src/pipeline/ingest.rs index 79b816c83..819408ab1 100644 --- a/crates/oxc_angular_compiler/src/pipeline/ingest.rs +++ b/crates/oxc_angular_compiler/src/pipeline/ingest.rs @@ -2570,6 +2570,7 @@ fn ingest_for_block<'a>( var.value.as_str(), &index_name, &count_name, + &mut job.diagnostics, ); aliases.push(AliasVariable { identifier: var.name.clone(), expression }); @@ -2704,6 +2705,7 @@ fn get_computed_for_loop_variable_expression<'a>( value: &str, index_name: &Atom<'a>, count_name: &Atom<'a>, + diagnostics: &mut std::vec::Vec, ) -> IrExpression<'a> { match value { "$index" => { @@ -2765,7 +2767,11 @@ fn get_computed_for_loop_variable_expression<'a>( ) } _ => { - // Unknown variable - return empty expression + // Angular throws: "AssertionError: unknown @for loop variable ${variable.value}" + // This should not happen if the parser correctly validates loop variables. + diagnostics.push(OxcDiagnostic::error(format!( + "AssertionError: unknown @for loop variable {value}" + ))); IrExpression::empty(allocator, None) } } 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 5d4d1b07c..e04140ead 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_contexts.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_contexts.rs @@ -6,6 +6,9 @@ //! //! Ported from Angular's `template/pipeline/src/phases/resolve_contexts.ts`. +use std::cell::RefCell; + +use oxc_diagnostics::OxcDiagnostic; use rustc_hash::FxHashMap; use crate::ir::enums::SemanticVariableKind; @@ -35,6 +38,9 @@ pub fn resolve_contexts(job: &mut ComponentCompilationJob<'_>) { let allocator = job.allocator; let root_xref = job.root.xref; + // Collect errors in a RefCell to allow mutation from within Fn closures + let errors: RefCell> = RefCell::new(Vec::new()); + // Collect view xrefs to avoid borrow issues let view_xrefs: Vec<_> = job.all_views().map(|v| v.xref).collect(); @@ -48,13 +54,22 @@ pub fn resolve_contexts(job: &mut ComponentCompilationJob<'_>) { // 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_update_vec( + allocator, + view_xref, + is_root, + &mut arrow_fn.ops, + &errors, + ); } - process_lexical_scope_create(allocator, view_xref, is_root, &mut view.create); - process_lexical_scope_update(allocator, view_xref, is_root, &mut view.update); + process_lexical_scope_create(allocator, view_xref, is_root, &mut view.create, &errors); + process_lexical_scope_update(allocator, view_xref, is_root, &mut view.update, &errors); } } + + // Add all collected errors to job diagnostics + job.diagnostics.extend(errors.into_inner()); } /// Process create operations to build scope and resolve context expressions. @@ -63,6 +78,7 @@ fn process_lexical_scope_create<'a>( view_xref: XrefId, is_root: bool, ops: &mut CreateOpList<'a>, + errors: &RefCell>, ) { // Track how to access each view's context by its XrefId. let mut scope: FxHashMap = FxHashMap::default(); @@ -100,6 +116,7 @@ fn process_lexical_scope_create<'a>( is_root, &mut listener.handler_ops, &mut listener.handler_expression, + errors, ); } CreateOp::TwoWayListener(listener) => { @@ -110,6 +127,7 @@ fn process_lexical_scope_create<'a>( is_root, &mut listener.handler_ops, &mut None, + errors, ); } CreateOp::AnimationListener(listener) => { @@ -120,6 +138,7 @@ fn process_lexical_scope_create<'a>( is_root, &mut listener.handler_ops, &mut None, + errors, ); } CreateOp::Animation(animation) => { @@ -130,6 +149,7 @@ fn process_lexical_scope_create<'a>( is_root, &mut animation.handler_ops, &mut None, + errors, ); } CreateOp::RepeaterCreate(repeater) => { @@ -138,7 +158,13 @@ fn process_lexical_scope_create<'a>( // 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); + process_lexical_scope_update_vec( + allocator, + view_xref, + is_root, + track_by_ops, + errors, + ); } // 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 @@ -147,7 +173,7 @@ fn process_lexical_scope_create<'a>( transform_expressions_in_create_op( op, &|expr, _flags| { - transform_context_expr(allocator, expr, &scope); + transform_context_expr(allocator, expr, &scope, view_xref, errors); }, VisitorContextFlag::NONE, ); @@ -156,7 +182,7 @@ fn process_lexical_scope_create<'a>( transform_expressions_in_create_op( op, &|expr, _flags| { - transform_context_expr(allocator, expr, &scope); + transform_context_expr(allocator, expr, &scope, view_xref, errors); }, VisitorContextFlag::NONE, ); @@ -181,6 +207,7 @@ fn process_listener_handler_ops<'a>( is_root: bool, handler_ops: &mut oxc_allocator::Vec<'a, UpdateOp<'a>>, handler_expression: &mut Option>>, + errors: &RefCell>, ) { // Build scope from handler_ops - look for Context variables (restoreView/nextContext) let mut scope: FxHashMap = FxHashMap::default(); @@ -220,7 +247,7 @@ fn process_listener_handler_ops<'a>( transform_expressions_in_update_op( op, &|expr, _flags| { - transform_context_expr(allocator, expr, &scope); + transform_context_expr(allocator, expr, &scope, view_xref, errors); }, VisitorContextFlag::NONE, ); @@ -233,7 +260,7 @@ fn process_listener_handler_ops<'a>( transform_expressions_in_expression( expr.as_mut(), &|e, _flags| { - transform_context_expr(allocator, e, &scope); + transform_context_expr(allocator, e, &scope, view_xref, errors); }, VisitorContextFlag::NONE, ); @@ -246,6 +273,7 @@ fn process_lexical_scope_update<'a>( view_xref: XrefId, is_root: bool, ops: &mut UpdateOpList<'a>, + errors: &RefCell>, ) { // Track how to access each view's context by its XrefId. let mut scope: FxHashMap = FxHashMap::default(); @@ -274,7 +302,7 @@ fn process_lexical_scope_update<'a>( transform_expressions_in_update_op( op, &|expr, _flags| { - transform_context_expr(allocator, expr, &scope); + transform_context_expr(allocator, expr, &scope, view_xref, errors); }, VisitorContextFlag::NONE, ); @@ -290,6 +318,7 @@ fn process_lexical_scope_update_vec<'a>( view_xref: XrefId, is_root: bool, ops: &mut oxc_allocator::Vec<'a, UpdateOp<'a>>, + errors: &RefCell>, ) { // Track how to access each view's context by its XrefId. let mut scope: FxHashMap = FxHashMap::default(); @@ -318,7 +347,7 @@ fn process_lexical_scope_update_vec<'a>( transform_expressions_in_update_op( op, &|expr, _flags| { - transform_context_expr(allocator, expr, &scope); + transform_context_expr(allocator, expr, &scope, view_xref, errors); }, VisitorContextFlag::NONE, ); @@ -333,6 +362,8 @@ fn transform_context_expr<'a>( allocator: &'a oxc_allocator::Allocator, expr: &mut IrExpression<'a>, scope: &FxHashMap, + view_xref: XrefId, + errors: &RefCell>, ) { if let IrExpression::Context(ctx_expr) = expr { match scope.get(&ctx_expr.view) { @@ -348,8 +379,12 @@ fn transform_context_expr<'a>( )); } None => { - // No context found - this is an error condition but we'll keep - // the expression as-is for now. The reify phase will handle it. + // Angular throws: "No context found for reference to view X from view Y" + // This indicates a compiler bug (missing context variable generation). + errors.borrow_mut().push(OxcDiagnostic::error(format!( + "No context found for reference to view {:?} from view {:?}", + ctx_expr.view, view_xref + ))); } } } @@ -361,7 +396,10 @@ fn transform_context_expr<'a>( pub fn resolve_contexts_for_host(job: &mut HostBindingCompilationJob<'_>) { let allocator = job.allocator; let view_xref = job.root.xref; + let errors: RefCell> = RefCell::new(Vec::new()); // Host bindings are always at the root level - process_lexical_scope_create(allocator, view_xref, true, &mut job.root.create); - process_lexical_scope_update(allocator, view_xref, true, &mut job.root.update); + process_lexical_scope_create(allocator, view_xref, true, &mut job.root.create, &errors); + process_lexical_scope_update(allocator, view_xref, true, &mut job.root.update, &errors); + // Add all collected errors to job diagnostics + job.diagnostics.extend(errors.into_inner()); } diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/track_fn_optimization.rs b/crates/oxc_angular_compiler/src/pipeline/phases/track_fn_optimization.rs index ca7bc17ff..59f5345ef 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/track_fn_optimization.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/track_fn_optimization.rs @@ -88,23 +88,11 @@ fn optimize_track_expression<'a>( return; } - // Check for simple property read on the component: this.fn (without calling it) - // This pattern is used when the track function is a pre-defined function on the component. - // Example: `track trackByFn` where trackByFn is a property/method on the component. - if let Some(prop_name) = check_component_property_track(&rep.track, expressions) { - rep.uses_component_instance = true; - // In root view, emit as ctx.propName; otherwise use componentInstance() - if view_xref == root_xref { - let fn_name = format!("ctx.{}", prop_name); - let name_str = allocator.alloc_str(&fn_name); - rep.track_fn_name = Some(Atom::from(name_str)); - } else { - let fn_name = format!("{}().{}", Identifiers::COMPONENT_INSTANCE, prop_name); - let name_str = allocator.alloc_str(&fn_name); - rep.track_fn_name = Some(Atom::from(name_str)); - } - return; - } + // Note: Angular does NOT optimize bare property reads like `track trackByFn` into + // direct references (e.g., `ctx.trackByFn`). Only `$index`, `$item`, and + // `fn($index[, $item])` call patterns are optimized. Bare property reads fall through + // to the non-optimizable case below, which creates a wrapper function like: + // `function _forTrack($index,$item) { return this.trackByFn; }` // First check if the expression contains any ContextExpr or AST expressions // that reference the component instance (implicit receiver) @@ -387,57 +375,6 @@ fn check_ast_for_simple_track_variable( None } -/// Check if the track expression is a simple property read on the component (e.g., `trackByFn`). -/// -/// This pattern is used when the track function is a pre-bound function on the component, -/// like `track trackByFn` where `trackByFn` is a method/property on the component. -/// Angular emits `ctx.trackByFn` directly instead of wrapping it. -/// -/// Returns the property name if found. -fn check_component_property_track( - track: &IrExpression<'_>, - expressions: &crate::pipeline::expression_store::ExpressionStore<'_>, -) -> Option { - use crate::ast::expression::AngularExpression; - - // Handle different expression types - match track { - // AST PropertyRead on implicit receiver - IrExpression::Ast(ast) => { - if let AngularExpression::PropertyRead(pr) = ast.as_ref() { - if matches!(&pr.receiver, AngularExpression::ImplicitReceiver(_)) { - // Don't match $index or $item - those are handled by check_simple_track_variable - let name = pr.name.as_str(); - if name != "$index" && name != "$item" { - return Some(name.to_string()); - } - } - } - } - // ExpressionRef - look up the stored expression - IrExpression::ExpressionRef(id) => { - let stored_expr = expressions.get(*id); - if let AngularExpression::PropertyRead(pr) = stored_expr { - if matches!(&pr.receiver, AngularExpression::ImplicitReceiver(_)) { - let name = pr.name.as_str(); - if name != "$index" && name != "$item" { - return Some(name.to_string()); - } - } - } - } - // ResolvedPropertyRead with Context receiver (after resolveNames phase) - IrExpression::ResolvedPropertyRead(rp) => { - if matches!(rp.receiver.as_ref(), IrExpression::Context(_)) { - return Some(rp.name.to_string()); - } - } - _ => {} - } - - None -} - /// Check if the track expression is a function call pattern: `this.fn($index)` or `this.fn($index, $item)`. /// /// These patterns can be optimized by passing the method reference directly to the repeater. diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index a924ac26d..07eec55ef 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -1033,6 +1033,47 @@ export class ExternalComponent { // Nested Control Flow Tests // ============================================================================ +/// Tests that bare component property tracks like `track id` generate a wrapper function, +/// NOT a direct `ctx.id` reference. Angular emits: +/// `function _forTrack($index,$item) { return this.id; }` then `ɵɵrepeaterCreate(..., _forTrack, true)` +/// Previously, oxc incorrectly optimized this to `ctx.id` passed directly, which changes +/// runtime semantics (passes a non-function value instead of a track function). +#[test] +fn test_for_track_bare_component_property() { + let js = compile_template_to_js( + r"@for (item of items; track id) {
{{item.name}}
}", + "TestComponent", + ); + // Angular wraps bare property reads in a function: _forTrack($index,$item){ return this.id; } + // It should NOT be optimized to ctx.id directly + assert!( + js.contains("_forTrack"), + "Bare component property track should generate a wrapper function _forTrack, not a direct ctx.id reference. Output:\n{js}" + ); + assert!( + js.contains("return this.id"), + "Track wrapper function should use 'this.id' (not ctx.id) since usesComponentInstance is true. Output:\n{js}" + ); + insta::assert_snapshot!("for_track_bare_component_property", js); +} + +/// Tests that `track trackByFn` (bare method reference) also generates a wrapper function. +/// Angular does NOT optimize bare property reads like `track trackByFn` into direct references. +/// Only `track trackByFn($index)` or `track trackByFn($index, $item)` get optimized. +#[test] +fn test_for_track_bare_method_reference() { + let js = compile_template_to_js( + r"@for (item of items; track trackByFn) {
{{item.name}}
}", + "TestComponent", + ); + // Angular wraps bare method references in a function + assert!( + js.contains("_forTrack"), + "Bare method reference track should generate a wrapper function _forTrack. Output:\n{js}" + ); + insta::assert_snapshot!("for_track_bare_method_reference", js); +} + #[test] fn test_nested_for_loops() { let js = compile_template_to_js( diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__for_track_bare_component_property.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__for_track_bare_component_property.snap new file mode 100644 index 000000000..65cb552c4 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__for_track_bare_component_property.snap @@ -0,0 +1,26 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +function _forTrack0($index,$item) { + return this.id; +} +function TestComponent_For_1_Template(rf,ctx) { + if ((rf & 1)) { + i0.ɵɵtext(0," "); + i0.ɵɵelementStart(1,"div"); + 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_Template(rf,ctx) { + if ((rf & 1)) { i0.ɵɵrepeaterCreate(0,TestComponent_For_1_Template,4,1,null,null,_forTrack0, + true); } + if ((rf & 2)) { i0.ɵɵrepeater(ctx.items); } +} diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__for_track_bare_method_reference.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__for_track_bare_method_reference.snap new file mode 100644 index 000000000..bc4db7360 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__for_track_bare_method_reference.snap @@ -0,0 +1,26 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +function _forTrack0($index,$item) { + return this.trackByFn; +} +function TestComponent_For_1_Template(rf,ctx) { + if ((rf & 1)) { + i0.ɵɵtext(0," "); + i0.ɵɵelementStart(1,"div"); + 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_Template(rf,ctx) { + if ((rf & 1)) { i0.ɵɵrepeaterCreate(0,TestComponent_For_1_Template,4,1,null,null,_forTrack0, + true); } + if ((rf & 2)) { i0.ɵɵrepeater(ctx.items); } +}