Allow impure ref initialization in render

[everettbu]

Mirror of facebook/react#35992
Original author: angular-bootstrap

---

Summary

Allows the narrow safe case of initializing ref.current from an impure call during render when it is guarded by the standard null-check initialization pattern.

This updates the compiler to recognize:

if (ref.current == null) {
  ref.current = Date.now();
}

as a valid one-time ref initialization flow, while continuing to reject other impure render-time calls.

Changes

Add shared compiler analysis to detect allowed impure ref initializers.
Use that analysis in ValidateNoImpureFunctionsInRender.
Use the same analysis in InferMutationAliasingRanges so the inference path does not still surface the call as an Impure error.
Add compiler fixture coverage for the allowed case in both fixture trees.

Test Plan

yarn snap -p allow-impure-ref-initialization
yarn snap -p error.invalid-impure-functions-in-render

Both targeted fixture runs reported the expected pass/fail behavior locally. The snap runner in this environment still exits non-zero afterward because of its existing worker shutdown bug (Farm is ended, no more calls can be done to it).

Issue

Closes #35973

[greptile-apps-staging]

Greptile Summary

This PR introduces a new compiler analysis pass (ComputeAllowedImpureRefInitializers) that recognizes the idiomatic one-time ref initialization pattern (if (ref.current == null) { ref.current = Date.now(); }) and exempts the impure call from the two places that would otherwise reject it: ValidateNoImpureFunctionsInRender and InferMutationAliasingRanges. The approach — tracking ref identity through the HIR, detecting a null-guard condition, and legitimizing assignments inside the guarded block — fits well with the existing compiler architecture.

Key points from the review:

• Logic bug in guard detection: The BinaryExpression case in computeRefTracking never checks value.operator, so any comparison of ref.current against null/undefined — including !== null and != null — is classified as a Guard. This allows patterns like if (ref.current !== null) { ref.current = Date.now(); } to slip through, which is a render-side mutation of an already-initialized ref (not a one-time initialization). The operator should be restricted to == / ===.
• Duplicate computation: computeAllowedImpureRefInitializers is invoked independently from both ValidateNoImpureFunctionsInRender and InferMutationAliasingRanges, doubling the cost of the fixed-point analysis for every compiled function.
• Magic iteration limit: The fixed-point loop in computeRefTracking is capped at 10 passes with no explanation; a comments or a principled bound tied to the block count would be safer and more self-documenting.
• Test coverage gap: Fixtures only cover the valid == null case; there are no negative-case fixtures verifying that != null or unguarded impure calls are still rejected.

Confidence Score: 3/5

• Needs changes before merging — the guard detection does not validate the comparison operator, which can suppress valid compiler errors for non-initialization patterns.
• The core algorithm (using fallthrough as a scope-expiry marker for safe blocks) is sound for the target pattern, and the integration into both validation and inference passes is minimal and correct. However, the missing operator check in BinaryExpression handling is a meaningful logic gap: negated null-checks are indistinguishable from the intended == null guard, allowing the compiler to silently pass render-time mutations that should be flagged. The duplicate analysis invocation and magic iteration limit are secondary concerns that reduce code quality without affecting correctness for the common case.
• Pay close attention to ComputeAllowedImpureRefInitializers.ts — specifically the BinaryExpression case in computeRefTracking (line 153) and the iteration bound (line 73).

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/Inference/ComputeAllowedImpureRefInitializers.ts	New analysis pass that identifies allowed impure ref initializers; contains a logic bug where BinaryExpression guard detection ignores the comparison operator (allowing negated checks like !== null to be treated as safe guards), and uses a hardcoded fixed-point iteration limit of 10.
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts	Integrates the new allowed-initializers set to skip Impure effects for recognized ref initialization patterns; the integration looks correct but duplicates the analysis computation already done in ValidateNoImpureFunctionsInRender.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureFunctionsInRender.ts	Now calls computeAllowedImpureRefInitializers to skip validation for allowed ref initializers; also fixes a pre-existing == vs === inconsistency. Duplicate computation of the analysis pass is a concern.
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/allow-impure-ref-initialization.js	New fixture covering the happy-path case; only tests the valid == null guard pattern and lacks coverage for invalid/negated patterns (e.g. !== null guard with assignment).
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/new-mutability/allow-impure-ref-initialization.js	Duplicate of the standard fixture for the new-mutability path; identical content and same coverage gap as its counterpart.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["HIRFunction input"] --> B["computeRefTracking (fixed-point, max 10 iters)"]
    B --> C["Map: IdentifierId → RefTracking\n(None | Nullable | Ref | RefValue | Guard)"]
    C --> D["computeAllowedImpureRefInitializers"]

    D --> E["Iterate blocks in order"]
    E --> F{"Block terminal\nkind = 'if'?"}
    F -- No --> G["Invalidate terminal operands"]
    F -- Yes --> H{"test tracking\n= Guard?"}
    H -- No --> G
    H -- Yes --> I["Push safeBlocks entry\n{block: fallthrough, ref}"]
    I --> G
    G --> E

    E -- "per instruction" --> J{"Instruction kind?"}
    J -- "CallExpression / MethodCall\n(impure sig + safeBlocks non-empty)" --> K["Add to pendingRoots + aliases"]
    J -- "PropertyStore ref.current\nvalue in aliases" --> L{"safeBlocks has\nmatching ref?"}
    L -- Yes --> M["legitimizedRoots.add(rootId)\nallowed.add(instrId)"]
    L -- No --> N["invalidatedRoots.add(rootId)"]
    J -- "Other (Load/Store/Binary/etc.)" --> O["Propagate or invalidate aliases"]

    D --> P["Final pass: remove\ninvalidated roots from allowed"]
    P --> Q["Set of allowed InstructionIds"]

    Q --> R["ValidateNoImpureFunctionsInRender\n(skip error if instr.id in allowed)"]
    Q --> S["InferMutationAliasingRanges\n(skip Impure effect if instr.id in allowed)"]

    style D fill:#f9f,stroke:#333
    style B fill:#ffd,stroke:#333
    style M fill:#cfc,stroke:#333
    style N fill:#fcc,stroke:#333

Loading

_{Last reviewed commit: eb8723c}

[greptile-apps-staging]

Guard detection ignores the comparison operator

The BinaryExpression case marks any expression as a Guard whenever one operand is a RefValue and the other is Nullable — but it never checks value.operator. This means a negated null-check like ref.current !== null or ref.current != null is indistinguishable from the intended ref.current == null pattern.

Consider this scenario:

if (ref.current !== null) {  // ref.current IS non-null here
  ref.current = Date.now();  // overwrites on EVERY render — not a safe initialization
}

This would be classified as a Guard, the assignment would be legitimized, and the compiler would silently skip what is actually an unsound render-time mutation. The PR description explicitly targets the "standard null-check initialization pattern" (== null / === null), so the operator should be validated:

// Only treat == null / === null comparisons as guards
const isNullCheck = value.operator === '==' || value.operator === '===';
changed = setRefTracking(
  refs,
  instr.lvalue.identifier.id,
  isNullCheck &&
    refValue != null &&
    (left?.kind === 'Nullable' || right?.kind === 'Nullable')
    ? {kind: 'Guard', refId: refValue.refId}
    : {kind: 'None'},
) || changed;

[greptile-apps-staging]

Hardcoded fixed-point iteration limit

The limit of 10 is a magic number with no comment explaining why it is sufficient. For deeply nested or complex control-flow graphs where ref identities propagate through many levels (e.g., through multiple StoreLocal/LoadLocal hops or across phi-like patterns), 10 passes may not be enough to reach a fixed point, silently producing incomplete results rather than an error.

Consider either:

• Replacing the limit with fn.body.blocks.size (a provably safe upper bound for acyclic dataflow), or
• Adding a comment/assertion documenting why 10 is the chosen bound (e.g., "HIR is in SSA form so convergence is guaranteed within N passes")

// Use block count as the bound — for SSA-form HIR without back-edges
// dataflow converges in at most O(depth) passes.
for (let i = 0; i < fn.body.blocks.size; i++) {

[greptile-apps-staging]

computeAllowedImpureRefInitializers is called twice per compilation unit

computeAllowedImpureRefInitializers(fn) is now called here AND in inferMutationAliasingRanges (line 80 of InferMutationAliasingRanges.ts). Both calls walk the same HIR function and run the same fixed-point loop, doubling the analysis cost.

A cleaner design would be to compute the set once at the pipeline level and pass it to both passes, or alternatively cache the result on the HIRFunction (similar to how other analyses are memoized). As a minimum, a comment acknowledging the duplication would help a future reader understand this is intentional.

[greptile-apps]

Greptile Summary

This PR introduces a new dataflow analysis (ComputeAllowedImpureRefInitializers) that recognises the idiomatic if (ref.current == null) { ref.current = impureCall(); } one-time initialisation pattern and exempts it from both ValidateNoImpureFunctionsInRender and the Impure effect recorded in InferMutationAliasingRanges. The intended relaxation is narrow and well-motivated, but the implementation contains two correctness bugs that have been identified in the review:

• Operator not validated in BinaryExpression (line 153 of ComputeAllowedImpureRefInitializers.ts): any comparison of a RefValue against Nullable is classified as a Guard, regardless of the operator. This means ref.current != null, ref.current !== null, or even ref.current > null are treated identically to ref.current == null, silently legitimising overwrites that execute on every render.
• Safe context leaks into the else-branch (line 384–391): the safeBlocks entry is keyed on block.terminal.fallthrough and is only removed when that fallthrough block is entered. Because the alternate block is visited before the fallthrough, the entry remains active during the else-branch, so an impure assignment inside } else { ref.current = Date.now(); } is incorrectly allowed.

Additionally:

• computeAllowedImpureRefInitializers is called once in ValidateNoImpureFunctionsInRender and again in InferMutationAliasingRanges, doubling the fixed-point iteration cost for every compiled function.
• The test fixtures only cover the happy path; no negative error fixtures are added to guard against the above regressions or future breakages.

Confidence Score: 1/5

• Not safe to merge — two confirmed correctness bugs in the core analysis pass will silently allow unsound render-time mutations.
• The central analysis has two verifiable logic errors (operator not checked, else-branch contamination) that cause the compiler to whitelist patterns it should reject. These are not theoretical edge cases; they arise from straightforward usage of != or !== guards and non-empty else branches, both of which are common in real codebases.
• ComputeAllowedImpureRefInitializers.ts requires fixes for the BinaryExpression operator check and the safeBlocks fallthrough-vs-alternate key. The fixture files need corresponding negative (error) test cases.

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/Inference/ComputeAllowedImpureRefInitializers.ts	New analysis pass with multiple correctness bugs: BinaryExpression operator is not validated (any comparison against null is treated as a safe guard), and the safe-block context is keyed on the fallthrough block rather than the consequent, causing it to remain active inside the else-branch where ref.current is provably non-null.
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts	Correctly integrates computeAllowedImpureRefInitializers to suppress Impure effects for legitimized ref initializers; the integration itself is straightforward but doubles the cost of the analysis pass (also called from ValidateNoImpureFunctionsInRender).
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureFunctionsInRender.ts	Integration is correct but computeAllowedImpureRefInitializers is now invoked twice per function (also called in InferMutationAliasingRanges), duplicating the fixed-point iteration work.
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/allow-impure-ref-initialization.js	Only covers the happy path; no negative fixtures test that the cases which should remain rejected (e.g. != null guard, else-branch assignment) actually produce errors.
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/new-mutability/allow-impure-ref-initialization.js	Same as the non-mutability variant — only tests the allowed case, missing negative regression coverage.

_{Last reviewed commit: d140b4d}

[greptile-apps]

Guard tracking ignores the comparison operator

The BinaryExpression case creates a Guard tracking for any binary expression that compares a RefValue against Nullable, without checking the actual operator. This means !=, !==, >, <, etc. all produce the same Guard as == or ===.

As a result, code like:

if (ref.current != null) {
  ref.current = Date.now(); // incorrectly allowed — ref.current is NOT null here
}

would be legitimized by this analysis. In this branch ref.current is guaranteed to be non-null, so assigning Date.now() to it overwrites an existing value on every render — exactly the unstable pattern the compiler is supposed to reject.

The BinaryExpression HIR node carries an operator field, so the fix is to restrict Guard creation to equality/nullish operators only:

case 'BinaryExpression': {
  const left = refs.get(value.left.identifier.id);
  const right = refs.get(value.right.identifier.id);
  const isNullCheck =
    value.operator === '==' ||
    value.operator === '===' ||
    value.operator === '!=' ||  // needed only for the 'then = null' direction
    value.operator === '!==';
  const refValue =
    left?.kind === 'RefValue'
      ? left
      : right?.kind === 'RefValue'
        ? right
        : null;
  const isNullSide = left?.kind === 'Nullable' || right?.kind === 'Nullable';
  // Only create a Guard for `ref.current == null` / `ref.current === null`
  const isEqualNull =
    isNullCheck &&
    (value.operator === '==' || value.operator === '===');
  changed =
    setRefTracking(
      refs,
      instr.lvalue.identifier.id,
      refValue != null && isNullSide && isEqualNull
        ? {kind: 'Guard', refId: refValue.refId}
        : {kind: 'None'},
    ) || changed;
  break;
}

(Alternatively, only the ==/=== operators with a Nullable operand should qualify, since that is the only form whose then-branch guarantees ref.current is null.)

Prompt To Fix With AI

This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Inference/ComputeAllowedImpureRefInitializers.ts
Line: 153-172

Comment:
**Guard tracking ignores the comparison operator**

The `BinaryExpression` case creates a `Guard` tracking for any binary expression that compares a `RefValue` against `Nullable`, without checking the actual operator. This means `!=`, `!==`, `>`, `<`, etc. all produce the same `Guard` as `==` or `===`.

As a result, code like:
```js
if (ref.current != null) {
  ref.current = Date.now(); // incorrectly allowed — ref.current is NOT null here
}
```
would be legitimized by this analysis. In this branch `ref.current` is guaranteed to be non-null, so assigning `Date.now()` to it overwrites an existing value on every render — exactly the unstable pattern the compiler is supposed to reject.

The `BinaryExpression` HIR node carries an `operator` field, so the fix is to restrict Guard creation to equality/nullish operators only:
```ts
case 'BinaryExpression': {
  const left = refs.get(value.left.identifier.id);
  const right = refs.get(value.right.identifier.id);
  const isNullCheck =
    value.operator === '==' ||
    value.operator === '===' ||
    value.operator === '!=' ||  // needed only for the 'then = null' direction
    value.operator === '!==';
  const refValue =
    left?.kind === 'RefValue'
      ? left
      : right?.kind === 'RefValue'
        ? right
        : null;
  const isNullSide = left?.kind === 'Nullable' || right?.kind === 'Nullable';
  // Only create a Guard for `ref.current == null` / `ref.current === null`
  const isEqualNull =
    isNullCheck &&
    (value.operator === '==' || value.operator === '===');
  changed =
    setRefTracking(
      refs,
      instr.lvalue.identifier.id,
      refValue != null && isNullSide && isEqualNull
        ? {kind: 'Guard', refId: refValue.refId}
        : {kind: 'None'},
    ) || changed;
  break;
}
```
(Alternatively, only the `==`/`===` operators with a `Nullable` operand should qualify, since that is the only form whose then-branch guarantees `ref.current` is null.)

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Safe context leaks into the else-branch

The safeBlocks entry is keyed on block.terminal.fallthrough — the merge block that both the then-branch and else-branch flow into. The retainWhere at line 222 only removes the entry when the fallthrough block is entered, meaning the entry remains active during both the consequent and the alternate block.

This allows an impure call in the else-branch to be legitimized:

if (ref.current == null) {
  // nothing here
} else {
  ref.current = Date.now(); // incorrectly legitimized — ref.current is NOT null here
}

Because block.terminal.alternate comes before block.terminal.fallthrough in the block iteration order, the safe entry is still present when processing the else-block. The PropertyStore check then finds a matching entry and adds the Date.now() call to allowed.

The safe context should expire when the alternate block is entered (since that branch is reached only when ref.current was already non-null):

if (
  test?.kind === 'Guard' &&
  safeBlocks.find(entry => entry.ref === test.refId) == null
) {
  // Expire the safe context at the start of the alternate (else) block,
  // not the fallthrough, so the else-branch is never treated as safe.
  safeBlocks.push({block: block.terminal.alternate, ref: test.refId});
}

This ensures the safe context only covers the consequent block (where ref.current is null), not the alternate.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Inference/ComputeAllowedImpureRefInitializers.ts
Line: 384-392

Comment:
**Safe context leaks into the else-branch**

The `safeBlocks` entry is keyed on `block.terminal.fallthrough` — the merge block that both the then-branch and else-branch flow into. The `retainWhere` at line 222 only removes the entry when the **fallthrough** block is entered, meaning the entry remains active during both the consequent **and** the alternate block.

This allows an impure call in the else-branch to be legitimized:
```js
if (ref.current == null) {
  // nothing here
} else {
  ref.current = Date.now(); // incorrectly legitimized — ref.current is NOT null here
}
```

Because `block.terminal.alternate` comes before `block.terminal.fallthrough` in the block iteration order, the safe entry is still present when processing the else-block. The `PropertyStore` check then finds a matching entry and adds the `Date.now()` call to `allowed`.

The safe context should expire when the **alternate** block is entered (since that branch is reached only when `ref.current` was already non-null):
```ts
if (
  test?.kind === 'Guard' &&
  safeBlocks.find(entry => entry.ref === test.refId) == null
) {
  // Expire the safe context at the start of the alternate (else) block,
  // not the fallthrough, so the else-branch is never treated as safe.
  safeBlocks.push({block: block.terminal.alternate, ref: test.refId});
}
```
This ensures the safe context only covers the `consequent` block (where `ref.current` is null), not the `alternate`.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Hardcoded iteration limit

The fixed upper bound of 10 for the dataflow fixpoint loop has no documented rationale. If a component has deeply nested control flow with ref aliases, the analysis might not converge within 10 passes and would silently produce an incomplete/over-conservative result (some valid initializations might not be recognised). Consider either:

• Adding a comment justifying why 10 is sufficient (e.g. "each identifier's tracking is monotone and settles in at most one reassignment cycle"), or
• Using a proper worklist / convergence check without the artificial ceiling, similar to other dataflow passes in the compiler.

// Each identifier's tracking only transitions once (monotone lattice),
// so N iterations is an upper bound where N = max chain depth of StoreLocal/LoadLocal.
// In practice, components with O(N) ref alias chains are pathological; we cap at 10
// to protect compile times.
for (let i = 0; i < 10; i++) {

Prompt To Fix With AI

This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Inference/ComputeAllowedImpureRefInitializers.ts
Line: 73

Comment:
**Hardcoded iteration limit**

The fixed upper bound of `10` for the dataflow fixpoint loop has no documented rationale. If a component has deeply nested control flow with ref aliases, the analysis might not converge within 10 passes and would silently produce an incomplete/over-conservative result (some valid initializations might not be recognised). Consider either:
- Adding a comment justifying why 10 is sufficient (e.g. "each identifier's tracking is monotone and settles in at most one reassignment cycle"), or
- Using a proper worklist / convergence check without the artificial ceiling, similar to other dataflow passes in the compiler.

```ts
// Each identifier's tracking only transitions once (monotone lattice),
// so N iterations is an upper bound where N = max chain depth of StoreLocal/LoadLocal.
// In practice, components with O(N) ref alias chains are pathological; we cap at 10
// to protect compile times.
for (let i = 0; i < 10; i++) {
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

computeAllowedImpureRefInitializers is called twice per function

computeAllowedImpureRefInitializers(fn) performs a full dataflow analysis over the HIR (two nested loops + up to 10 fixpoint iterations). It is called here in validateNoImpureFunctionsInRender and again inside inferMutationAliasingRanges for the same function. For large components this doubles the cost of this pass.

Consider caching the result on the HIRFunction (e.g. as a property populated once before either pass runs), or computing it once and threading it through both callers.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureFunctionsInRender.ts
Line: 24

Comment:
**`computeAllowedImpureRefInitializers` is called twice per function**

`computeAllowedImpureRefInitializers(fn)` performs a full dataflow analysis over the HIR (two nested loops + up to 10 fixpoint iterations). It is called here in `validateNoImpureFunctionsInRender` and again inside `inferMutationAliasingRanges` for the same function. For large components this doubles the cost of this pass.

Consider caching the result on the `HIRFunction` (e.g. as a property populated once before either pass runs), or computing it once and threading it through both callers.

How can I resolve this? If you propose a fix, please make it concise.