[compiler] Prevent overriding a derivationEntry on effect mutation - CONTAINS BUG

[ccalvinhhuang]

Summary:
With this we are now comparing a snapshot of the derivationCache with the new changes every time we are done recording the derivations happening in the HIR.

We have to do this after recording everything since we still do some mutations on the cache when recording mutations.

Test Plan:
Test the following in playground:

function Component({ value }) {
  const [checked, setChecked] = useState('');

  useEffect(() => {
    setChecked(value === '' ? [] : value.split(','));
  }, [value]);

  return (
    <div>{checked}</div>
  )
}

This no longer causes an infinite loop.

Added a test case in the next PR in the stack

[ccalvinhhuang]

@greptileai

[greptile-apps]

Greptile Summary

This PR introduces a critical ordering bug in the validation pass that checks for derived computations in effects. The key changes include:

• Added snapshot/change detection mechanism (takeSnapshot(), checkForChanges()) to track cache modifications across iterations
• Modified effect mutation handling to update existing cache entries instead of overriding them
• Critical Bug: Moved recordPhiDerivations to be called AFTER processing instructions (line 229), when it must be called BEFORE

Issues Found

• Logic error (line 225-229): The order of operations is incorrect. In SSA form, phi nodes define values at the beginning of a basic block. Calling recordPhiDerivations after the instruction loop means instructions that reference phi values won't find their derivation metadata in the cache during processing, leading to incomplete validation results.

Confidence Score: 0/5

• This PR contains a critical logic bug that will cause incorrect validation behavior
• Score of 0 reflects a critical ordering bug where recordPhiDerivations is called after processing instructions instead of before. This violates SSA semantics where phi nodes must define values before instructions in a block can use them, resulting in missing cache entries and incorrect validation results
• The file ValidateNoDerivedComputationsInEffects_exp.ts requires immediate attention - the instruction processing order must be fixed

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts	Critical ordering bug: phi derivations are recorded after instructions instead of before, causing missing cache entries

Sequence Diagram

sequenceDiagram
    participant Main as validateNoDerivedComputationsInEffects_exp
    participant Cache as DerivationCache
    participant Block as Block Processing Loop
    participant RecordPhi as recordPhiDerivations
    participant RecordInstr as recordInstructionDerivations

    Main->>Cache: Initialize cache with props/params
    Main->>Main: Start do-while loop
    Main->>Cache: takeSnapshot()
    Note over Cache: Store previous cache state
    
    Main->>Block: Iterate over blocks
    
    rect rgb(255, 200, 200)
        Note over Block,RecordInstr: BUG: Wrong Order!
        Block->>RecordInstr: Process instructions FIRST
        Note over RecordInstr: Looks up phi values in cache
        Note over RecordInstr: Missing entries! Phi derivations not yet recorded
        Block->>RecordPhi: recordPhiDerivations() AFTER
        Note over RecordPhi: NOW adds phi metadata to cache (too late!)
    end
    
    Block->>Main: Continue to next block
    Main->>Cache: checkForChanges()
    Note over Cache: Compare current vs snapshot
    Main->>Cache: snapshot() returns hasChanges
    
    alt hasChanges is true
        Main->>Main: Continue loop
    else hasChanges is false
        Main->>Main: Exit loop, validate effects
    end

Loading

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

logic: Critical ordering bug: recordPhiDerivations must be called BEFORE the instruction loop, not after. Phi nodes define values at the beginning of a block in SSA form, so their derivation metadata must be in the cache before processing instructions that may reference them.

Suggested change

	for (const block of fn.body.blocks.values()) {
	recordPhiDerivations(block, context);
	for (const instr of block.instructions) {
	recordInstructionDerivations(instr, context, isFirstPass);
	}
	recordPhiDerivations(block, context);
	}
	for (const block of fn.body.blocks.values()) {
	recordPhiDerivations(block, context);
	for (const instr of block.instructions) {
	recordInstructionDerivations(instr, context, isFirstPass);
	}
	}

Prompt To Fix With AI

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

Comment:
**logic:** Critical ordering bug: `recordPhiDerivations` must be called BEFORE the instruction loop, not after. Phi nodes define values at the beginning of a block in SSA form, so their derivation metadata must be in the cache before processing instructions that may reference them.

```suggestion
    for (const block of fn.body.blocks.values()) {
      recordPhiDerivations(block, context);
      for (const instr of block.instructions) {
        recordInstructionDerivations(instr, context, isFirstPass);
      }
    }
```

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