Skip to content

fix: listener handler scope should not merge parent local_definitions… #4

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 4, 2026
Merged

fix: listener handler scope should not merge parent local_definitions… #4

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
220 changes: 54 additions & 166 deletions crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,47 @@ impl<'a> ScopeMaps<'a> {
}
}

/// Build scope maps from listener handler_ops.
///
/// Per Angular's resolve_names.ts (line 86), handler ops are processed as a
/// completely separate lexical scope via recursive `processLexicalScope(unit, op.handlerOps, savedView)`.
/// The generateVariables phase already prepends all necessary variables to handler_ops,
/// so no merging from the parent view's scope is needed. This matches Angular's
/// behavior where each `processLexicalScope` call starts with fresh scope/localDefinitions maps.
fn build_scope_from_handler_ops<'a, 'b>(
ops: impl Iterator<Item = &'b UpdateOp<'a>>,
) -> ScopeMaps<'a>
where
'a: 'b,
{
let mut maps = ScopeMaps::default();
for op in ops {
if let UpdateOp::Variable(var_op) = op {
match var_op.kind {
SemanticVariableKind::Identifier => {
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);
}
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
}

/// 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();
Expand Down Expand Up @@ -193,58 +234,13 @@ fn process_lexical_scope_create<'a>(
for op in ops.iter_mut() {
match op {
CreateOp::Listener(listener) => {
// Build handler-specific scope that includes Variables from handler_ops.
// This is critical for @for loops where listeners need access to loop variables
// that were prepended to handler_ops by generate_variables phase.
//
// We use "first wins" semantics: the first Variable with a given name takes
// precedence. This matches Angular TypeScript compiler behavior in
// resolve_names.ts (lines 55-58).
//
// handler_ops Variables override update_scope Variables, but among handler_ops
// Variables themselves, the first one wins. This is achieved by:
// 1. First building a scope from handler_ops only (first wins)
// 2. Then merging in update_scope for names not in handler_ops
let mut handler_scope = ScopeMaps::default();
// First pass: add handler_ops Variables (first wins)
for handler_op in listener.handler_ops.iter() {
if let UpdateOp::Variable(var_op) = handler_op {
match var_op.kind {
SemanticVariableKind::Identifier => {
// Handler-local variables take precedence
if var_op.local
&& !handler_scope.local_definitions.contains_key(&var_op.name)
{
handler_scope
.local_definitions
.insert(var_op.name.clone(), var_op.xref);
}
// Only add if not already present (first wins)
if !handler_scope.scope.contains_key(&var_op.name) {
handler_scope.scope.insert(var_op.name.clone(), var_op.xref);
}
}
SemanticVariableKind::Alias => {
// Only add if not already present (first wins)
if !handler_scope.scope.contains_key(&var_op.name) {
handler_scope.scope.insert(var_op.name.clone(), var_op.xref);
}
}
_ => {}
}
}
}
// Second pass: merge in update_scope for names not in handler_ops
for (name, xref) in &scope.scope {
if !handler_scope.scope.contains_key(name) {
handler_scope.scope.insert(name.clone(), *xref);
}
}
for (name, xref) in &scope.local_definitions {
if !handler_scope.local_definitions.contains_key(name) {
handler_scope.local_definitions.insert(name.clone(), *xref);
}
}
// Per Angular's resolve_names.ts (line 86):
// processLexicalScope(unit, op.handlerOps, savedView);
// Handler ops are processed as a SEPARATE lexical scope — a fresh
// scope/localDefinitions is built from handler_ops variables only,
// with NO merging from the parent view's scope. The generateVariables
// phase already prepends all necessary variables to handler_ops.
let handler_scope = build_scope_from_handler_ops(listener.handler_ops.iter());

// Process listener handler_ops with the handler scope
for handler_op in listener.handler_ops.iter_mut() {
Expand Down Expand Up @@ -276,43 +272,8 @@ fn process_lexical_scope_create<'a>(
}
}
CreateOp::TwoWayListener(listener) => {
// Build handler-specific scope for TwoWayListener
// Uses same "first wins" semantics as Listener (see above)
let mut handler_scope = ScopeMaps::default();
for handler_op in listener.handler_ops.iter() {
if let UpdateOp::Variable(var_op) = handler_op {
match var_op.kind {
SemanticVariableKind::Identifier => {
if var_op.local
&& !handler_scope.local_definitions.contains_key(&var_op.name)
{
handler_scope
.local_definitions
.insert(var_op.name.clone(), var_op.xref);
}
if !handler_scope.scope.contains_key(&var_op.name) {
handler_scope.scope.insert(var_op.name.clone(), var_op.xref);
}
}
SemanticVariableKind::Alias => {
if !handler_scope.scope.contains_key(&var_op.name) {
handler_scope.scope.insert(var_op.name.clone(), var_op.xref);
}
}
_ => {}
}
}
}
for (name, xref) in &scope.scope {
if !handler_scope.scope.contains_key(name) {
handler_scope.scope.insert(name.clone(), *xref);
}
}
for (name, xref) in &scope.local_definitions {
if !handler_scope.local_definitions.contains_key(name) {
handler_scope.local_definitions.insert(name.clone(), *xref);
}
}
// Per Angular's resolve_names.ts: handler ops get their own scope
let handler_scope = build_scope_from_handler_ops(listener.handler_ops.iter());

for handler_op in listener.handler_ops.iter_mut() {
transform_expressions_in_update_op(
Expand All @@ -330,46 +291,10 @@ fn process_lexical_scope_create<'a>(
VisitorContextFlag::NONE,
);
}
// Note: TwoWayListenerOp has no handler_expression field
}
CreateOp::AnimationListener(listener) => {
// Build handler-specific scope for AnimationListener
// Uses same "first wins" semantics as Listener (see above)
let mut handler_scope = ScopeMaps::default();
for handler_op in listener.handler_ops.iter() {
if let UpdateOp::Variable(var_op) = handler_op {
match var_op.kind {
SemanticVariableKind::Identifier => {
if var_op.local
&& !handler_scope.local_definitions.contains_key(&var_op.name)
{
handler_scope
.local_definitions
.insert(var_op.name.clone(), var_op.xref);
}
if !handler_scope.scope.contains_key(&var_op.name) {
handler_scope.scope.insert(var_op.name.clone(), var_op.xref);
}
}
SemanticVariableKind::Alias => {
if !handler_scope.scope.contains_key(&var_op.name) {
handler_scope.scope.insert(var_op.name.clone(), var_op.xref);
}
}
_ => {}
}
}
}
for (name, xref) in &scope.scope {
if !handler_scope.scope.contains_key(name) {
handler_scope.scope.insert(name.clone(), *xref);
}
}
for (name, xref) in &scope.local_definitions {
if !handler_scope.local_definitions.contains_key(name) {
handler_scope.local_definitions.insert(name.clone(), *xref);
}
}
// Per Angular's resolve_names.ts: handler ops get their own scope
let handler_scope = build_scope_from_handler_ops(listener.handler_ops.iter());

for handler_op in listener.handler_ops.iter_mut() {
transform_expressions_in_update_op(
Expand All @@ -387,46 +312,10 @@ fn process_lexical_scope_create<'a>(
VisitorContextFlag::NONE,
);
}
// Note: AnimationListenerOp has no handler_expression field
}
CreateOp::Animation(animation) => {
// Build handler-specific scope for Animation
// Uses same "first wins" semantics as Listener (see above)
let mut handler_scope = ScopeMaps::default();
for handler_op in animation.handler_ops.iter() {
if let UpdateOp::Variable(var_op) = handler_op {
match var_op.kind {
SemanticVariableKind::Identifier => {
if var_op.local
&& !handler_scope.local_definitions.contains_key(&var_op.name)
{
handler_scope
.local_definitions
.insert(var_op.name.clone(), var_op.xref);
}
if !handler_scope.scope.contains_key(&var_op.name) {
handler_scope.scope.insert(var_op.name.clone(), var_op.xref);
}
}
SemanticVariableKind::Alias => {
if !handler_scope.scope.contains_key(&var_op.name) {
handler_scope.scope.insert(var_op.name.clone(), var_op.xref);
}
}
_ => {}
}
}
}
for (name, xref) in &scope.scope {
if !handler_scope.scope.contains_key(name) {
handler_scope.scope.insert(name.clone(), *xref);
}
}
for (name, xref) in &scope.local_definitions {
if !handler_scope.local_definitions.contains_key(name) {
handler_scope.local_definitions.insert(name.clone(), *xref);
}
}
// Per Angular's resolve_names.ts: handler ops get their own scope
let handler_scope = build_scope_from_handler_ops(animation.handler_ops.iter());

for handler_op in animation.handler_ops.iter_mut() {
transform_expressions_in_update_op(
Expand All @@ -444,7 +333,6 @@ fn process_lexical_scope_create<'a>(
VisitorContextFlag::NONE,
);
}
// Note: AnimationOp has no handler_expression field
}
_ => {
transform_expressions_in_create_op(
Expand Down
82 changes: 82 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 @@ -576,6 +576,88 @@ fn test_let_inside_for_if_with_component_method_call() {
insta::assert_snapshot!("let_inside_for_if_with_component_method_call", js);
}

// ============================================================================
// @let with pipe in child view Tests
// ============================================================================

#[test]
fn test_let_with_pipe_used_in_child_view() {
// @let with pipe used in a child view (@if block) should keep BOTH declareLet and storeLet.
//
// When a @let wraps a pipe and is referenced from a child view:
// - declareLet is needed because pipes use DI which requires the TNode
// - storeLet is needed because the @let is accessed from another view via readContextLet
//
// Without storeLet, the pipe's varOffset would be wrong because storeLet contributes
// 1 var to the var counting, and removing it shifts all subsequent varOffsets.
//
// Expected Angular output:
// i0.ɵɵstoreLet(i0.ɵɵpipeBind1(1, varOffset, ctx.name));
//
// Bug output (missing storeLet):
// i0.ɵɵpipeBind1(1, varOffset, ctx.name);
let js = compile_template_to_js(
r"@let value = name | uppercase; @if (true) { {{value}} }",
"TestComponent",
);
// storeLet must wrap pipeBind because @let is used externally (in child @if view)
assert!(
js.contains("ɵɵstoreLet(i0.ɵɵpipeBind1("),
"storeLet should wrap pipeBind1 when @let with pipe is used in child view. Output:\n{js}"
);
// declareLet must be present (pipes need TNode for DI)
assert!(
js.contains("ɵɵdeclareLet("),
"declareLet should be present when @let contains a pipe. Output:\n{js}"
);
// readContextLet must be present in the child view
assert!(
js.contains("ɵɵreadContextLet("),
"readContextLet should be present in child view. Output:\n{js}"
);
insta::assert_snapshot!("let_with_pipe_used_in_child_view", js);
}

#[test]
fn test_let_with_pipe_used_in_listener() {
// @let with pipe used in an event listener in the same view should keep storeLet.
//
// Event listeners are callbacks (isCallback=true), so @let declarations
// in the same view generate ContextLetReferenceExpr in the listener's handler ops.
// This means the @let is "used externally" and storeLet must be preserved.
let js = compile_template_to_js(
r#"@let value = name | uppercase; <button (click)="onClick(value)">Click</button>"#,
"TestComponent",
);
// storeLet must wrap pipeBind because @let is used externally (in listener callback)
assert!(
js.contains("ɵɵstoreLet(i0.ɵɵpipeBind1("),
"storeLet should wrap pipeBind1 when @let with pipe is used in listener. Output:\n{js}"
);
insta::assert_snapshot!("let_with_pipe_used_in_listener", js);
}

#[test]
fn test_let_with_pipe_multiple_in_child_view_varoffset() {
// Multiple @let declarations with pipes used in a child view.
// Each storeLet contributes 1 var, so removing them would cause cumulative varOffset drift.
//
// This reproduces the ClickUp AdvancedTabComponent pattern where multiple @let
// declarations with pipes have their storeLet wrappers incorrectly removed,
// causing the second pipe's varOffset to drift by +1 for each missing storeLet.
let js = compile_template_to_js(
r"@let a = x | uppercase; @let b = y | lowercase; @if (true) { {{a}} {{b}} }",
"TestComponent",
);
// Both @let values should have storeLet wrappers
let store_let_count = js.matches("ɵɵstoreLet(").count();
assert!(
store_let_count >= 2,
"Expected at least 2 storeLet calls for 2 @let declarations with pipes used in child view, got {store_let_count}. Output:\n{js}"
);
insta::assert_snapshot!("let_with_pipe_multiple_in_child_view_varoffset", js);
}

// ============================================================================
// ng-content Tests
// ============================================================================
Expand Down
31 changes: 31 additions & 0 deletions ...ler/tests/snapshots/integration_test__let_with_pipe_multiple_in_child_view_varoffset.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
---
source: crates/oxc_angular_compiler/tests/integration_test.rs
expression: js
---
function TestComponent_Conditional_6_Template(rf,ctx) {
if ((rf & 1)) { i0.ɵɵtext(0); }
if ((rf & 2)) {
i0.ɵɵnextContext();
const a_r1 = i0.ɵɵreadContextLet(0);
const b_r2 = i0.ɵɵreadContextLet(3);
i0.ɵɵtextInterpolate2(" ",a_r1," ",b_r2," ");
}
}
function TestComponent_Template(rf,ctx) {
if ((rf & 1)) {
i0.ɵɵdeclareLet(0);
i0.ɵɵpipe(1,"uppercase");
i0.ɵɵtext(2," ");
i0.ɵɵdeclareLet(3);
i0.ɵɵpipe(4,"lowercase");
i0.ɵɵtext(5," ");
i0.ɵɵconditionalCreate(6,TestComponent_Conditional_6_Template,1,2);
}
if ((rf & 2)) {
i0.ɵɵstoreLet(i0.ɵɵpipeBind1(1,1,ctx.x));
i0.ɵɵadvance(3);
i0.ɵɵstoreLet(i0.ɵɵpipeBind1(4,4,ctx.y));
i0.ɵɵadvance(3);
i0.ɵɵconditional((true? 6: -1));
}
}
Loading
Loading