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 3722d461d..df669ba68 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/resolve_names.rs @@ -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>, +) -> 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(); @@ -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() { @@ -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( @@ -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( @@ -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( @@ -444,7 +333,6 @@ fn process_lexical_scope_create<'a>( VisitorContextFlag::NONE, ); } - // Note: AnimationOp has no handler_expression field } _ => { transform_expressions_in_create_op( diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index 556d2d915..32d794062 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -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; "#, + "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 // ============================================================================ diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_multiple_in_child_view_varoffset.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_multiple_in_child_view_varoffset.snap new file mode 100644 index 000000000..253dd808b --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_multiple_in_child_view_varoffset.snap @@ -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)); + } +} diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_used_in_child_view.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_used_in_child_view.snap new file mode 100644 index 000000000..c811baf42 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_used_in_child_view.snap @@ -0,0 +1,25 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +function TestComponent_Conditional_3_Template(rf,ctx) { + if ((rf & 1)) { i0.ɵɵtext(0); } + if ((rf & 2)) { + i0.ɵɵnextContext(); + const value_r1 = i0.ɵɵreadContextLet(0); + i0.ɵɵtextInterpolate1(" ",value_r1," "); + } +} +function TestComponent_Template(rf,ctx) { + if ((rf & 1)) { + i0.ɵɵdeclareLet(0); + i0.ɵɵpipe(1,"uppercase"); + i0.ɵɵtext(2," "); + i0.ɵɵconditionalCreate(3,TestComponent_Conditional_3_Template,1,1); + } + if ((rf & 2)) { + i0.ɵɵstoreLet(i0.ɵɵpipeBind1(1,1,ctx.name)); + i0.ɵɵadvance(3); + i0.ɵɵconditional((true? 3: -1)); + } +} diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_used_in_listener.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_used_in_listener.snap new file mode 100644 index 000000000..d28f12cc3 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_with_pipe_used_in_listener.snap @@ -0,0 +1,21 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +function TestComponent_Template(rf,ctx) { + if ((rf & 1)) { + const _r1 = i0.ɵɵgetCurrentView(); + i0.ɵɵdeclareLet(0); + i0.ɵɵpipe(1,"uppercase"); + i0.ɵɵtext(2," "); + i0.ɵɵelementStart(3,"button",0); + i0.ɵɵlistener("click",function TestComponent_Template_button_click_3_listener() { + i0.ɵɵrestoreView(_r1); + const value_r2 = i0.ɵɵreadContextLet(0); + return i0.ɵɵresetView(ctx.onClick(value_r2)); + }); + i0.ɵɵtext(4,"Click"); + i0.ɵɵelementEnd(); + } + if ((rf & 2)) { i0.ɵɵstoreLet(i0.ɵɵpipeBind1(1,0,ctx.name)); } +}