diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/expand_safe_reads.rs b/crates/oxc_angular_compiler/src/pipeline/phases/expand_safe_reads.rs index 6413e0484..9d103add1 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/expand_safe_reads.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/expand_safe_reads.rs @@ -127,10 +127,10 @@ pub fn expand_safe_reads(job: &mut ComponentCompilationJob<'_>) { /// Checks if an IR expression requires a temporary variable to avoid re-evaluation. /// -/// Returns true for expressions with side effects: -/// - Function calls/invocations -/// - Pipe bindings -/// - Safe function invocations +/// Returns true for expressions with side effects (function calls, pipe bindings), +/// or for compound expressions that may contain such sub-expressions. +/// +/// Ported from Angular's `needsTemporaryInSafeAccess` in `expand_safe_reads.ts` lines 43-73. fn needs_temporary_in_safe_access(expr: &IrExpression<'_>) -> bool { match expr { // Function calls always need temporaries (they may have side effects) @@ -140,6 +140,26 @@ fn needs_temporary_in_safe_access(expr: &IrExpression<'_>) -> bool { // Pipe bindings need temporaries (they may have side effects) IrExpression::PipeBinding(_) => true, IrExpression::PipeBindingVariadic(_) => true, + // Array and object literals need temporaries + IrExpression::LiteralArray(_) | IrExpression::DerivedLiteralArray(_) => true, + IrExpression::LiteralMap(_) | IrExpression::DerivedLiteralMap(_) => true, + // Unary operators need to check their operand + IrExpression::Unary(u) => needs_temporary_in_safe_access(&u.expr), + IrExpression::Not(n) => needs_temporary_in_safe_access(&n.expr), + // Binary operators need to check both operands + IrExpression::Binary(binary) => { + needs_temporary_in_safe_access(&binary.lhs) + || needs_temporary_in_safe_access(&binary.rhs) + } + IrExpression::ResolvedBinary(rb) => { + needs_temporary_in_safe_access(&rb.left) || needs_temporary_in_safe_access(&rb.right) + } + // Conditional expressions need to check all branches + IrExpression::Ternary(t) => { + needs_temporary_in_safe_access(&t.condition) + || needs_temporary_in_safe_access(&t.true_expr) + || needs_temporary_in_safe_access(&t.false_expr) + } // AssignTemporary expressions need to check their inner expression IrExpression::AssignTemporary(assign) => needs_temporary_in_safe_access(&assign.expr), // Safe property reads need to check their receiver diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index 3365bccbe..1953cb876 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -4374,3 +4374,67 @@ export class TestComponent { assert!(code.contains("i18n_0"), "Should have i18n_0 variable. Output:\n{code}"); assert!(code.contains("i18n_1"), "Should have i18n_1 variable. Output:\n{code}"); } + +/// Test that pipe inside binary expression with safe navigation generates a temporary variable. +/// +/// When a pipe result is wrapped in a binary expression (e.g., `(data$ | async) || fallback`) +/// and then used with safe navigation (`?.`), the compiler should generate a temporary variable +/// to avoid calling the pipe twice. +/// +/// This is a port of the Angular TS behavior where `needsTemporaryInSafeAccess` checks through +/// `BinaryOperatorExpr` to find nested pipe expressions that need temporaries. +/// +/// Without the fix, the compiler generates: +/// `(pipeBind1(...) || fallback) == null ? null : (pipeBind1(...) || fallback).prop` +/// (pipe called twice, slot indices doubled) +/// +/// With the fix: +/// `(tmp_0_0 = pipeBind1(...) || fallback) == null ? null : tmp_0_0.prop` +/// (pipe called once, stored in temp variable) +#[test] +fn test_pipe_in_binary_with_safe_nav_uses_temp_variable() { + let js = compile_template_to_js( + r#"
"#, + "TestComponent", + ); + eprintln!("OUTPUT:\n{js}"); + + // Should use a temporary variable to avoid double pipe evaluation + assert!( + js.contains("tmp_0_0"), + "Should generate tmp_0_0 for pipe inside binary with safe nav. Output:\n{js}" + ); + + // The pipe should only appear ONCE in the expression (stored in tmp) + let pipe_count = js.matches("pipeBind1(").count(); + assert_eq!( + pipe_count, 1, + "pipeBind1 should appear exactly once (not duplicated). Found {pipe_count} occurrences. Output:\n{js}" + ); +} + +/// Test pipe in binary with safe navigation and chained property access. +/// +/// More complex case: `((data$ | async) || fallback)?.nested?.value` +/// The entire safe navigation chain should use the temp variable. +#[test] +fn test_pipe_in_binary_with_safe_nav_chain() { + let js = compile_template_to_js( + r#"
"#, + "TestComponent", + ); + eprintln!("OUTPUT:\n{js}"); + + // Should use a temporary variable + assert!( + js.contains("tmp_0_0"), + "Should generate tmp_0_0 for pipe inside binary with safe nav chain. Output:\n{js}" + ); + + // The pipe should only appear ONCE + let pipe_count = js.matches("pipeBind1(").count(); + assert_eq!( + pipe_count, 1, + "pipeBind1 should appear exactly once. Found {pipe_count}. Output:\n{js}" + ); +}