Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
64 changes: 64 additions & 0 deletions crates/oxc_angular_compiler/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
);
}
Loading