fix: check compound expressions in needs_temporary_in_safe_access for safe navigation (#14)

Author: Brooooooklyn

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

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#"<div [title]="((data$ | async) || defaultVal)?.name"></div>"#,
+        "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#"<div [title]="((data$ | async) || defaultVal)?.nested?.value"></div>"#,
+        "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}"
+    );
+}