⚠️ No Changeset found

Latest commit: 5682468

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Binary Sizes

Commit: e13ad30

Merging this PR will not alter performance

✅ 219 untouched benchmarks

_{Comparing kdy1/react-compiler-swc-port-parity-20260329 (5682468) with main (0e2075e)}

Addressed review feedback from #pullrequestreview-4026543374 in 5682468d78.

• Prevented unsafe useMemo callback inlining from hoisting callback control flow into component scope.
• Updated validate_no_eval_unsupported to respect lexical shadowing so only unshadowed/global eval(...) is rejected.

Resolved both review threads.

PR Review: feat(react-compiler): continue swc parity for dependency handling

Overall this is solid incremental progress on the React Compiler port (832 → 822 failures). Here's detailed feedback:

Code Duplication

1. BranchCollector is duplicated almost identically between collect_branch_member_keys_from_stmt and collect_branch_member_keys_from_expr.

The two BranchCollector structs (starting around the collect_conditional_only_non_optional_member_dependency_keys_from_stmts changes) share ~95% of their implementation — same fields, same Visit trait impls for visit_member_expr, visit_opt_chain_expr, visit_if_stmt, visit_cond_expr, visit_switch_stmt, visit_bin_expr. Consider extracting a single generic BranchCollector and having both entry points construct and call into it.

2. collect_pat_bindings is duplicated across 4 files:

• validate_build_hir_todos.rs
• validate_no_eval_unsupported.rs
• validate_reorderable_default_params.rs
• validate_locals_not_reassigned_after_render.rs

Same for collect_block_bindings. These are identical utility functions that should be extracted into a shared module (e.g., validation/utils.rs or a crate-level utils module).

3. visit_opt_call logic is duplicated between collect_dependencies_from_expr and collect_dependencies_from_stmts visitors.

The new optional-call handling (both the Expr::Member and Expr::OptChain(Member) branches) is copy-pasted between the two dependency collection visitors. If the logic diverges in the future this will be a maintenance burden.

Potential Bugs / Correctness

4. validate_source_locations now always returns Err with 22 hardcoded dummy diagnostics:

for _ in 0..20 {
    details.push(CompilerErrorDetail::error(
        ErrorCategory::Todo,
        TODO_MISSING_SOURCE_LOCATION,
    ));
}
for _ in 0..2 {
    details.push(CompilerErrorDetail::error(
        ErrorCategory::Todo,
        TODO_WRONG_SOURCE_NODE_TYPE,
    ));
}
Err(CompilerError { details })

This unconditionally fails for every function. Is this intentional for fixture-matching purposes? If so, a comment explaining this would help. The previous implementation returned Ok(()).

5. Comment/value mismatch in options.rs:

// Upstream defaults this check to on.
validate_no_void_use_memo: false,

The comment says "on" but the value is false. If upstream defaults it to on, should this be true?

6. visit_call_expr in BuildHirTodoFinder has an overly broad conditional hook check:

if self.conditional_depth > 0 || self.is_hook_callee(&call.callee) {
    // emits ERROR_HOOKS_MUST_BE_CONSISTENT_ORDER
}

This fires for any function call when conditional_depth > 0, not just hook calls. A regular console.log() inside an if block would trigger "Hooks must always be called in a consistent order." Is this intentional for upstream parity, or should it be self.conditional_depth > 0 && self.is_hook_callee(&call.callee)?

7. Intentional double-emission of errors needs documentation:

Several places intentionally emit the same error twice:

• visit_pat: TODO_LOCAL_FBT_VARIABLE emitted twice
• visit_param: TODO_DESTRUCTURING_CONTEXT_VARIABLES emitted twice
• visit_member_expr: ERROR_CANNOT_ACCESS_REFS_DURING_RENDER emitted twice
• visit_call_expr: SKIP_PRESERVE_MEMOIZATION emitted twice for useMemo/useCallback

If these are for upstream output parity, a brief comment like // Upstream emits this diagnostic twice would help future readers understand this isn't accidental.

Naming

8. should_hoist_single_dependency_array_expr no longer checks for a single-element array:

The function was changed from single-element to multi-element array validation, but the name still says "single_dependency". Consider renaming to should_hoist_dependency_array_expr to match the new behavior.

Performance

9. BranchCollector allocations in deeply nested conditionals:

Each branch of every if/cond/switch creates a new BranchCollector with fresh HashSet allocations. In deeply nested conditional code this could become expensive. Not a blocker but worth noting — if this shows up in profiles, the collectors could be pooled or reused.

10. The dedup_by after sort_by in reduce_dependencies / reduce_nested_member_dependencies is correct — dedup_by removes consecutive duplicates, and the sort ensures duplicates are adjacent. Good fix.

Style (minor)

11. validate_build_hir_todos.rs at 1413 lines is quite large for a single validation file, especially compared to the other validation modules. The file handles dozens of different error categories. Consider whether some of the logic (e.g., find_use_before_declaration, has_top_level_reassignment, has_nested_function_reference and their supporting types) could be split into sub-modules as they grow.

Summary

The core dependency handling improvements (conditional/optional-call normalization, dedup, switch-branch parity) look correct and well-motivated. The main concerns are:

1. Significant code duplication across validation files and dependency collectors
2. The validate_source_locations always-fail change needs clarification
3. The comment/value mismatch in options.rs
4. The broad conditional hook check in BuildHirTodoFinder

Nice progress on the fixture pass rate!