Skip to content

fix(angular): fix 3 compiler divergences from Angular reference #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Brooooooklyn merged 1 commit into from
Feb 24, 2026
Merged

fix(angular): fix 3 compiler divergences from Angular reference #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion crates/oxc_angular_compiler/src/pipeline/ingest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });

Expand Down Expand Up @@ -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<OxcDiagnostic>,
) -> IrExpression<'a> {
match value {
"$index" => {
Expand Down Expand Up @@ -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)
}
}
Expand Down
66 changes: 52 additions & 14 deletions crates/oxc_angular_compiler/src/pipeline/phases/resolve_contexts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Vec<OxcDiagnostic>> = RefCell::new(Vec::new());

// Collect view xrefs to avoid borrow issues
let view_xrefs: Vec<_> = job.all_views().map(|v| v.xref).collect();

Expand All @@ -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.
Expand All @@ -63,6 +78,7 @@ fn process_lexical_scope_create<'a>(
view_xref: XrefId,
is_root: bool,
ops: &mut CreateOpList<'a>,
errors: &RefCell<Vec<OxcDiagnostic>>,
) {
// Track how to access each view's context by its XrefId.
let mut scope: FxHashMap<XrefId, ContextAccess> = FxHashMap::default();
Expand Down Expand Up @@ -100,6 +116,7 @@ fn process_lexical_scope_create<'a>(
is_root,
&mut listener.handler_ops,
&mut listener.handler_expression,
errors,
);
}
CreateOp::TwoWayListener(listener) => {
Expand All @@ -110,6 +127,7 @@ fn process_lexical_scope_create<'a>(
is_root,
&mut listener.handler_ops,
&mut None,
errors,
);
}
CreateOp::AnimationListener(listener) => {
Expand All @@ -120,6 +138,7 @@ fn process_lexical_scope_create<'a>(
is_root,
&mut listener.handler_ops,
&mut None,
errors,
);
}
CreateOp::Animation(animation) => {
Expand All @@ -130,6 +149,7 @@ fn process_lexical_scope_create<'a>(
is_root,
&mut animation.handler_ops,
&mut None,
errors,
);
}
CreateOp::RepeaterCreate(repeater) => {
Expand All @@ -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
Expand All @@ -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,
);
Expand All @@ -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,
);
Expand All @@ -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<oxc_allocator::Box<'a, IrExpression<'a>>>,
errors: &RefCell<Vec<OxcDiagnostic>>,
) {
// Build scope from handler_ops - look for Context variables (restoreView/nextContext)
let mut scope: FxHashMap<XrefId, ContextAccess> = FxHashMap::default();
Expand Down Expand Up @@ -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,
);
Expand All @@ -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,
);
Expand All @@ -246,6 +273,7 @@ fn process_lexical_scope_update<'a>(
view_xref: XrefId,
is_root: bool,
ops: &mut UpdateOpList<'a>,
errors: &RefCell<Vec<OxcDiagnostic>>,
) {
// Track how to access each view's context by its XrefId.
let mut scope: FxHashMap<XrefId, ContextAccess> = FxHashMap::default();
Expand Down Expand Up @@ -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,
);
Expand All @@ -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<Vec<OxcDiagnostic>>,
) {
// Track how to access each view's context by its XrefId.
let mut scope: FxHashMap<XrefId, ContextAccess> = FxHashMap::default();
Expand Down Expand Up @@ -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,
);
Expand All @@ -333,6 +362,8 @@ fn transform_context_expr<'a>(
allocator: &'a oxc_allocator::Allocator,
expr: &mut IrExpression<'a>,
scope: &FxHashMap<XrefId, ContextAccess>,
view_xref: XrefId,
errors: &RefCell<Vec<OxcDiagnostic>>,
) {
if let IrExpression::Context(ctx_expr) = expr {
match scope.get(&ctx_expr.view) {
Expand All @@ -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
)));
}
}
}
Expand All @@ -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<Vec<OxcDiagnostic>> = 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());
}
73 changes: 5 additions & 68 deletions crates/oxc_angular_compiler/src/pipeline/phases/track_fn_optimization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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<String> {
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.
Expand Down
41 changes: 41 additions & 0 deletions crates/oxc_angular_compiler/tests/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) { <div>{{item.name}}</div> }",
"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) { <div>{{item.name}}</div> }",
"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(
Expand Down
Loading
Loading