Skip to content

Commit 70148cd

Browse files
committed
[compiler] ref guards apply up to fallthrough of the test
Fixes #30782 When developers do an `if (ref.current == null)` guard for lazy ref initialization, the "safe" blocks should extend up to the if's fallthrough. Previously we only allowed writing to the ref in the if consequent, but this meant that you couldn't use a ternary, logical, etc in the if body.
1 parent b370db0 commit 70148cd

File tree

3 files changed

+107
-8
lines changed

3 files changed

+107
-8
lines changed

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
eachTerminalOperand,
2828
} from '../HIR/visitors';
2929
import {Err, Ok, Result} from '../Utils/Result';
30+
import {retainWhere} from '../Utils/utils';
3031

3132
/**
3233
* Validates that a function does not access a ref value during render. This includes a partial check
@@ -279,9 +280,10 @@ function validateNoRefAccessInRenderImpl(
279280
for (let i = 0; (i == 0 || env.hasChanged()) && i < 10; i++) {
280281
env.resetChanged();
281282
returnValues = [];
282-
const safeBlocks = new Map<BlockId, RefId>();
283+
const safeBlocks: Array<{block: BlockId; ref: RefId}> = [];
283284
const errors = new CompilerError();
284285
for (const [, block] of fn.body.blocks) {
286+
retainWhere(safeBlocks, entry => entry.block !== block.id);
285287
for (const phi of block.phis) {
286288
env.set(
287289
phi.place.identifier.id,
@@ -503,15 +505,17 @@ function validateNoRefAccessInRenderImpl(
503505
case 'PropertyStore':
504506
case 'ComputedDelete':
505507
case 'ComputedStore': {
506-
const safe = safeBlocks.get(block.id);
507508
const target = env.get(instr.value.object.identifier.id);
509+
let safe: (typeof safeBlocks)['0'] | null | undefined = null;
508510
if (
509511
instr.value.kind === 'PropertyStore' &&
510-
safe != null &&
511-
target?.kind === 'Ref' &&
512-
target.refId === safe
512+
target != null &&
513+
target.kind === 'Ref'
513514
) {
514-
safeBlocks.delete(block.id);
515+
safe = safeBlocks.find(entry => entry.ref === target.refId);
516+
}
517+
if (safe != null) {
518+
retainWhere(safeBlocks, entry => entry !== safe);
515519
} else {
516520
validateNoRefUpdate(errors, env, instr.value.object, instr.loc);
517521
}
@@ -599,8 +603,11 @@ function validateNoRefAccessInRenderImpl(
599603

600604
if (block.terminal.kind === 'if') {
601605
const test = env.get(block.terminal.test.identifier.id);
602-
if (test?.kind === 'Guard') {
603-
safeBlocks.set(block.terminal.consequent, test.refId);
606+
if (
607+
test?.kind === 'Guard' &&
608+
safeBlocks.find(entry => entry.ref === test.refId) == null
609+
) {
610+
safeBlocks.push({block: block.terminal.fallthrough, ref: test.refId});
604611
}
605612
}
606613

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validateRefAccessDuringRender
6+
7+
import {useRef} from 'react';
8+
9+
function Component(props) {
10+
const ref = useRef(null);
11+
if (ref.current == null) {
12+
// the logical means the ref write is in a different block
13+
// from the if consequent. this tests that the "safe" blocks
14+
// extend up to the if's fallthrough
15+
ref.current = props.unknownKey ?? props.value;
16+
}
17+
return <Child ref={ref} />;
18+
}
19+
20+
function Child({ref}) {
21+
'use no memo';
22+
return ref.current;
23+
}
24+
25+
export const FIXTURE_ENTRYPOINT = {
26+
fn: Component,
27+
params: [{value: 42}],
28+
};
29+
30+
```
31+
32+
## Code
33+
34+
```javascript
35+
import { c as _c } from "react/compiler-runtime"; // @validateRefAccessDuringRender
36+
37+
import { useRef } from "react";
38+
39+
function Component(props) {
40+
const $ = _c(1);
41+
const ref = useRef(null);
42+
if (ref.current == null) {
43+
ref.current = props.unknownKey ?? props.value;
44+
}
45+
let t0;
46+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
47+
t0 = <Child ref={ref} />;
48+
$[0] = t0;
49+
} else {
50+
t0 = $[0];
51+
}
52+
return t0;
53+
}
54+
55+
function Child({ ref }) {
56+
"use no memo";
57+
return ref.current;
58+
}
59+
60+
export const FIXTURE_ENTRYPOINT = {
61+
fn: Component,
62+
params: [{ value: 42 }],
63+
};
64+
65+
```
66+
67+
### Eval output
68+
(kind: ok) 42
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// @validateRefAccessDuringRender
2+
3+
import {useRef} from 'react';
4+
5+
function Component(props) {
6+
const ref = useRef(null);
7+
if (ref.current == null) {
8+
// the logical means the ref write is in a different block
9+
// from the if consequent. this tests that the "safe" blocks
10+
// extend up to the if's fallthrough
11+
ref.current = props.unknownKey ?? props.value;
12+
}
13+
return <Child ref={ref} />;
14+
}
15+
16+
function Child({ref}) {
17+
'use no memo';
18+
return ref.current;
19+
}
20+
21+
export const FIXTURE_ENTRYPOINT = {
22+
fn: Component,
23+
params: [{value: 42}],
24+
};

0 commit comments

Comments
 (0)