@samitouri / QOS-React / commits / 7771d3a797
Fork 0

The library for web and native user interfaces.

[compiler] Track refs through object expressions and property lookups

Summary: This addresses the issue of the compiler being overly restrictive about refs escaping into object expressions. Rather than erroring whenever a ref flows into an object, we will now treat the object itself as a ref, and apply the same escape rules to it. Whenever we look up a property from a ref value, we now don't know whether that value is itself a ref or a ref value, so we assume it's both. The same logic applies to ref-accessing functions--if such a function is stored in an object, we'll propagate that property to the object itself and any properties looked up from it. ghstack-source-id: 5c6fcb895d4a1658ce9dddec286aad3a57a4c9f1 Pull Request resolved: https://github.com/facebook/react/pull/30821

Mike Vitousek committed Aug 27, 2024 at 10:11 UTC 7771d3a7972cc2483c45fde51b7ec2d926cba097
5 files changed +273 -128
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts
+177 -76
@@ -11,12 +11,12 @@ import {
11 IdentifierId,
12 Place,
13 SourceLocation,
14 - isRefOrRefValue,
14 isRefValueType,
15 isUseRefType,
16 } from '../HIR';
17 import {
18 eachInstructionValueOperand,
19 + eachPatternOperand,
20 eachTerminalOperand,
21 } from '../HIR/visitors';
22 import {Err, Ok, Result} from '../Utils/Result';
@@ -42,58 +42,165 @@ import {isEffectHook} from './ValidateMemoizedEffectDependencies';
42 * In the future we may reject more cases, based on either object names (`fooRef.current` is likely a ref)
43 * or based on property name alone (`foo.current` might be a ref).
44 */
45 +type State = {
46 + refs: Set<IdentifierId>;
47 + refValues: Map<IdentifierId, SourceLocation | null>;
48 + refAccessingFunctions: Set<IdentifierId>;
49 +};
50 +
51 export function validateNoRefAccessInRender(fn: HIRFunction): void {
46 - const refAccessingFunctions: Set<IdentifierId> = new Set();
47 - validateNoRefAccessInRenderImpl(fn, refAccessingFunctions).unwrap();
52 + const state = {
53 + refs: new Set<IdentifierId>(),
54 + refValues: new Map<IdentifierId, SourceLocation | null>(),
55 + refAccessingFunctions: new Set<IdentifierId>(),
56 + };
57 + validateNoRefAccessInRenderImpl(fn, state).unwrap();
58 }
59
60 function validateNoRefAccessInRenderImpl(
61 fn: HIRFunction,
52 - refAccessingFunctions: Set<IdentifierId>,
62 + state: State,
63 ): Result<void, CompilerError> {
64 + let place;
65 + for (const param of fn.params) {
66 + if (param.kind === 'Identifier') {
67 + place = param;
68 + } else {
69 + place = param.place;
70 + }
71 +
72 + if (isRefValueType(place.identifier)) {
73 + state.refValues.set(place.identifier.id, null);
74 + }
75 + if (isUseRefType(place.identifier)) {
76 + state.refs.add(place.identifier.id);
77 + }
78 + }
79 const errors = new CompilerError();
55 - const lookupLocations: Map<IdentifierId, SourceLocation> = new Map();
80 for (const [, block] of fn.body.blocks) {
81 + for (const phi of block.phis) {
82 + phi.operands.forEach(operand => {
83 + if (state.refs.has(operand.id) || isUseRefType(phi.id)) {
84 + state.refs.add(phi.id.id);
85 + }
86 + const refValue = state.refValues.get(operand.id);
87 + if (refValue !== undefined || isRefValueType(operand)) {
88 + state.refValues.set(
89 + phi.id.id,
90 + refValue ?? state.refValues.get(phi.id.id) ?? null,
91 + );
92 + }
93 + if (state.refAccessingFunctions.has(operand.id)) {
94 + state.refAccessingFunctions.add(phi.id.id);
95 + }
96 + });
97 + }
98 +
99 for (const instr of block.instructions) {
100 + for (const operand of eachInstructionValueOperand(instr.value)) {
101 + if (isRefValueType(operand.identifier)) {
102 + CompilerError.invariant(state.refValues.has(operand.identifier.id), {
103 + reason: 'Expected ref value to be in state',
104 + loc: operand.loc,
105 + });
106 + }
107 + if (isUseRefType(operand.identifier)) {
108 + CompilerError.invariant(state.refs.has(operand.identifier.id), {
109 + reason: 'Expected ref to be in state',
110 + loc: operand.loc,
111 + });
112 + }
113 + }
114 +
115 switch (instr.value.kind) {
116 case 'JsxExpression':
117 case 'JsxFragment': {
118 for (const operand of eachInstructionValueOperand(instr.value)) {
62 - validateNoDirectRefValueAccess(errors, operand, lookupLocations);
119 + validateNoDirectRefValueAccess(errors, operand, state);
120 }
121 break;
122 }
123 + case 'ComputedLoad':
124 case 'PropertyLoad': {
125 + if (typeof instr.value.property !== 'string') {
126 + validateNoRefValueAccess(errors, state, instr.value.property);
127 + }
128 if (
68 - isRefValueType(instr.lvalue.identifier) &&
69 - instr.value.property === 'current'
129 + state.refAccessingFunctions.has(instr.value.object.identifier.id)
130 ) {
71 - lookupLocations.set(instr.lvalue.identifier.id, instr.loc);
131 + state.refAccessingFunctions.add(instr.lvalue.identifier.id);
132 + }
133 + if (state.refs.has(instr.value.object.identifier.id)) {
134 + /*
135 + * Once an object contains a ref at any level, we treat it as a ref.
136 + * If we look something up from it, that value may either be a ref
137 + * or the ref value (or neither), so we conservatively assume it's both.
138 + */
139 + state.refs.add(instr.lvalue.identifier.id);
140 + state.refValues.set(instr.lvalue.identifier.id, instr.loc);
141 }
142 break;
143 }
144 + case 'LoadContext':
145 case 'LoadLocal': {
76 - if (refAccessingFunctions.has(instr.value.place.identifier.id)) {
77 - refAccessingFunctions.add(instr.lvalue.identifier.id);
146 + if (
147 + state.refAccessingFunctions.has(instr.value.place.identifier.id)
148 + ) {
149 + state.refAccessingFunctions.add(instr.lvalue.identifier.id);
150 }
79 - if (isRefValueType(instr.lvalue.identifier)) {
80 - const loc = lookupLocations.get(instr.value.place.identifier.id);
81 - if (loc !== undefined) {
82 - lookupLocations.set(instr.lvalue.identifier.id, loc);
83 - }
151 + const refValue = state.refValues.get(instr.value.place.identifier.id);
152 + if (refValue !== undefined) {
153 + state.refValues.set(instr.lvalue.identifier.id, refValue);
154 + }
155 + if (state.refs.has(instr.value.place.identifier.id)) {
156 + state.refs.add(instr.lvalue.identifier.id);
157 }
158 break;
159 }
160 + case 'StoreContext':
161 case 'StoreLocal': {
88 - if (refAccessingFunctions.has(instr.value.value.identifier.id)) {
89 - refAccessingFunctions.add(instr.value.lvalue.place.identifier.id);
90 - refAccessingFunctions.add(instr.lvalue.identifier.id);
162 + if (
163 + state.refAccessingFunctions.has(instr.value.value.identifier.id)
164 + ) {
165 + state.refAccessingFunctions.add(
166 + instr.value.lvalue.place.identifier.id,
167 + );
168 + state.refAccessingFunctions.add(instr.lvalue.identifier.id);
169 + }
170 + const refValue = state.refValues.get(instr.value.value.identifier.id);
171 + if (
172 + refValue !== undefined ||
173 + isRefValueType(instr.value.lvalue.place.identifier)
174 + ) {
175 + state.refValues.set(
176 + instr.value.lvalue.place.identifier.id,
177 + refValue ?? null,
178 + );
179 + state.refValues.set(instr.lvalue.identifier.id, refValue ?? null);
180 + }
181 + if (state.refs.has(instr.value.value.identifier.id)) {
182 + state.refs.add(instr.value.lvalue.place.identifier.id);
183 + state.refs.add(instr.lvalue.identifier.id);
184 }
92 - if (isRefValueType(instr.value.lvalue.place.identifier)) {
93 - const loc = lookupLocations.get(instr.value.value.identifier.id);
94 - if (loc !== undefined) {
95 - lookupLocations.set(instr.value.lvalue.place.identifier.id, loc);
96 - lookupLocations.set(instr.lvalue.identifier.id, loc);
185 + break;
186 + }
187 + case 'Destructure': {
188 + const destructuredFunction = state.refAccessingFunctions.has(
189 + instr.value.value.identifier.id,
190 + );
191 + const destructuredRef = state.refs.has(
192 + instr.value.value.identifier.id,
193 + );
194 + for (const lval of eachPatternOperand(instr.value.lvalue.pattern)) {
195 + if (isUseRefType(lval.identifier)) {
196 + state.refs.add(lval.identifier.id);
197 + }
198 + if (destructuredRef || isRefValueType(lval.identifier)) {
199 + state.refs.add(lval.identifier.id);
200 + state.refValues.set(lval.identifier.id, null);
201 + }
202 + if (destructuredFunction) {
203 + state.refAccessingFunctions.add(lval.identifier.id);
204 }
205 }
206 break;
@@ -107,32 +214,27 @@ function validateNoRefAccessInRenderImpl(
214 */
215 [...eachInstructionValueOperand(instr.value)].some(
216 operand =>
110 - isRefValueType(operand.identifier) ||
111 - refAccessingFunctions.has(operand.identifier.id),
217 + state.refValues.has(operand.identifier.id) ||
218 + state.refAccessingFunctions.has(operand.identifier.id),
219 ) ||
220 // check for cases where .current is accessed through an aliased ref
221 ([...eachInstructionValueOperand(instr.value)].some(operand =>
115 - isUseRefType(operand.identifier),
222 + state.refs.has(operand.identifier.id),
223 ) &&
224 validateNoRefAccessInRenderImpl(
225 instr.value.loweredFunc.func,
119 - refAccessingFunctions,
226 + state,
227 ).isErr())
228 ) {
229 // This function expression unconditionally accesses a ref
123 - refAccessingFunctions.add(instr.lvalue.identifier.id);
230 + state.refAccessingFunctions.add(instr.lvalue.identifier.id);
231 }
232 break;
233 }
234 case 'MethodCall': {
235 if (!isEffectHook(instr.value.property.identifier)) {
236 for (const operand of eachInstructionValueOperand(instr.value)) {
130 - validateNoRefAccess(
131 - errors,
132 - refAccessingFunctions,
133 - operand,
134 - operand.loc,
135 - );
237 + validateNoRefAccess(errors, state, operand, operand.loc);
238 }
239 }
240 break;
@@ -142,7 +244,7 @@ function validateNoRefAccessInRenderImpl(
244 const isUseEffect = isEffectHook(callee.identifier);
245 if (!isUseEffect) {
246 // Report a more precise error when calling a local function that accesses a ref
145 - if (refAccessingFunctions.has(callee.identifier.id)) {
247 + if (state.refAccessingFunctions.has(callee.identifier.id)) {
248 errors.push({
249 severity: ErrorSeverity.InvalidReact,
250 reason:
@@ -159,9 +261,9 @@ function validateNoRefAccessInRenderImpl(
261 for (const operand of eachInstructionValueOperand(instr.value)) {
262 validateNoRefAccess(
263 errors,
162 - refAccessingFunctions,
264 + state,
265 operand,
164 - lookupLocations.get(operand.identifier.id) ?? operand.loc,
266 + state.refValues.get(operand.identifier.id) ?? operand.loc,
267 );
268 }
269 }
@@ -170,12 +272,17 @@ function validateNoRefAccessInRenderImpl(
272 case 'ObjectExpression':
273 case 'ArrayExpression': {
274 for (const operand of eachInstructionValueOperand(instr.value)) {
173 - validateNoRefAccess(
174 - errors,
175 - refAccessingFunctions,
176 - operand,
177 - lookupLocations.get(operand.identifier.id) ?? operand.loc,
178 - );
275 + validateNoDirectRefValueAccess(errors, operand, state);
276 + if (state.refAccessingFunctions.has(operand.identifier.id)) {
277 + state.refAccessingFunctions.add(instr.lvalue.identifier.id);
278 + }
279 + if (state.refs.has(operand.identifier.id)) {
280 + state.refs.add(instr.lvalue.identifier.id);
281 + }
282 + const refValue = state.refValues.get(operand.identifier.id);
283 + if (refValue !== undefined) {
284 + state.refValues.set(instr.lvalue.identifier.id, refValue);
285 + }
286 }
287 break;
288 }
@@ -185,20 +292,15 @@ function validateNoRefAccessInRenderImpl(
292 case 'ComputedStore': {
293 validateNoRefAccess(
294 errors,
188 - refAccessingFunctions,
295 + state,
296 instr.value.object,
190 - lookupLocations.get(instr.value.object.identifier.id) ?? instr.loc,
297 + state.refValues.get(instr.value.object.identifier.id) ?? instr.loc,
298 );
299 for (const operand of eachInstructionValueOperand(instr.value)) {
300 if (operand === instr.value.object) {
301 continue;
302 }
196 - validateNoRefValueAccess(
197 - errors,
198 - refAccessingFunctions,
199 - lookupLocations,
200 - operand,
201 - );
303 + validateNoRefValueAccess(errors, state, operand);
304 }
305 break;
306 }
@@ -207,28 +309,27 @@ function validateNoRefAccessInRenderImpl(
309 break;
310 default: {
311 for (const operand of eachInstructionValueOperand(instr.value)) {
210 - validateNoRefValueAccess(
211 - errors,
212 - refAccessingFunctions,
213 - lookupLocations,
214 - operand,
215 - );
312 + validateNoRefValueAccess(errors, state, operand);
313 }
314 break;
315 }
316 }
317 + if (isUseRefType(instr.lvalue.identifier)) {
318 + state.refs.add(instr.lvalue.identifier.id);
319 + }
320 + if (
321 + isRefValueType(instr.lvalue.identifier) &&
322 + !state.refValues.has(instr.lvalue.identifier.id)
323 + ) {
324 + state.refValues.set(instr.lvalue.identifier.id, instr.loc);
325 + }
326 }
327 for (const operand of eachTerminalOperand(block.terminal)) {
328 if (block.terminal.kind !== 'return') {
223 - validateNoRefValueAccess(
224 - errors,
225 - refAccessingFunctions,
226 - lookupLocations,
227 - operand,
228 - );
329 + validateNoRefValueAccess(errors, state, operand);
330 } else {
331 // Allow functions containing refs to be returned, but not direct ref values
231 - validateNoDirectRefValueAccess(errors, operand, lookupLocations);
332 + validateNoDirectRefValueAccess(errors, operand, state);
333 }
334 }
335 }
@@ -242,19 +343,18 @@ function validateNoRefAccessInRenderImpl(
343
344 function validateNoRefValueAccess(
345 errors: CompilerError,
245 - refAccessingFunctions: Set<IdentifierId>,
246 - lookupLocations: Map<IdentifierId, SourceLocation>,
346 + state: State,
347 operand: Place,
348 ): void {
349 if (
250 - isRefValueType(operand.identifier) ||
251 - refAccessingFunctions.has(operand.identifier.id)
350 + state.refValues.has(operand.identifier.id) ||
351 + state.refAccessingFunctions.has(operand.identifier.id)
352 ) {
353 errors.push({
354 severity: ErrorSeverity.InvalidReact,
355 reason:
356 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)',
257 - loc: lookupLocations.get(operand.identifier.id) ?? operand.loc,
357 + loc: state.refValues.get(operand.identifier.id) ?? operand.loc,
358 description:
359 operand.identifier.name !== null &&
360 operand.identifier.name.kind === 'named'
@@ -267,13 +367,14 @@ function validateNoRefValueAccess(
367
368 function validateNoRefAccess(
369 errors: CompilerError,
270 - refAccessingFunctions: Set<IdentifierId>,
370 + state: State,
371 operand: Place,
372 loc: SourceLocation,
373 ): void {
374 if (
275 - isRefOrRefValue(operand.identifier) ||
276 - refAccessingFunctions.has(operand.identifier.id)
375 + state.refs.has(operand.identifier.id) ||
376 + state.refValues.has(operand.identifier.id) ||
377 + state.refAccessingFunctions.has(operand.identifier.id)
378 ) {
379 errors.push({
380 severity: ErrorSeverity.InvalidReact,
@@ -293,14 +394,14 @@ function validateNoRefAccess(
394 function validateNoDirectRefValueAccess(
395 errors: CompilerError,
396 operand: Place,
296 - lookupLocations: Map<IdentifierId, SourceLocation>,
397 + state: State,
398 ): void {
298 - if (isRefValueType(operand.identifier)) {
399 + if (state.refValues.has(operand.identifier.id)) {
400 errors.push({
401 severity: ErrorSeverity.InvalidReact,
402 reason:
403 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)',
303 - loc: lookupLocations.get(operand.identifier.id) ?? operand.loc,
404 + loc: state.refValues.get(operand.identifier.id) ?? operand.loc,
405 description:
406 operand.identifier.name !== null &&
407 operand.identifier.name.kind === 'named'
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md
+9 -7
@@ -22,13 +22,15 @@ function Foo({a}) {
22 ## Error
23
24 ```
25 - 3 | const ref = useRef();
26 - 4 | // type information is lost here as we don't track types of fields
27 -> 5 | const val = {ref};
28 - | ^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
29 - 6 | // without type info, we don't know that val.ref.current is a ref value so we
30 - 7 | // *would* end up depending on val.ref.current
31 - 8 | // however, this is an instance of accessing a ref during render and is disallowed
25 + 8 | // however, this is an instance of accessing a ref during render and is disallowed
26 + 9 | // under React's rules, so we reject this input
27 +> 10 | const x = {a, val: val.ref.current};
28 + | ^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (10:10)
29 +
30 +InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (10:10)
31 + 11 |
32 + 12 | return <VideoList videos={x} />;
33 + 13 | }
34 ```
35
36
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.return-ref-callback-structure.expect.md deleted
-45
@@ -1,45 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -// @flow @validateRefAccessDuringRender @validatePreserveExistingMemoizationGuarantees
6 -
7 -import {useRef} from 'react';
8 -
9 -component Foo(cond: boolean, cond2: boolean) {
10 - const ref = useRef();
11 -
12 - const s = () => {
13 - return ref.current;
14 - };
15 -
16 - if (cond) return [s];
17 - else if (cond2) return {s};
18 - else return {s: [s]};
19 -}
20 -
21 -export const FIXTURE_ENTRYPOINT = {
22 - fn: Foo,
23 - params: [{cond: false, cond2: false}],
24 -};
25 -
26 -```
27 -
28 -
29 -## Error
30 -
31 -```
32 - 10 | };
33 - 11 |
34 -> 12 | if (cond) return [s];
35 - | ^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (12:12)
36 -
37 -InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (13:13)
38 -
39 -InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (14:14)
40 - 13 | else if (cond2) return {s};
41 - 14 | else return {s: [s]};
42 - 15 | }
43 -```
44 -
45 -
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/return-ref-callback-structure.expect.md new
+87
@@ -0,0 +1,87 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @flow @validateRefAccessDuringRender @validatePreserveExistingMemoizationGuarantees
6 +
7 +import {useRef} from 'react';
8 +
9 +component Foo(cond: boolean, cond2: boolean) {
10 + const ref = useRef();
11 +
12 + const s = () => {
13 + return ref.current;
14 + };
15 +
16 + if (cond) return [s];
17 + else if (cond2) return {s};
18 + else return {s: [s]};
19 +}
20 +
21 +export const FIXTURE_ENTRYPOINT = {
22 + fn: Foo,
23 + params: [{cond: false, cond2: false}],
24 +};
25 +
26 +```
27 +
28 +## Code
29 +
30 +```javascript
31 +import { c as _c } from "react/compiler-runtime";
32 +
33 +import { useRef } from "react";
34 +
35 +function Foo(t0) {
36 + const $ = _c(4);
37 + const { cond, cond2 } = t0;
38 + const ref = useRef();
39 + let t1;
40 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
41 + t1 = () => ref.current;
42 + $[0] = t1;
43 + } else {
44 + t1 = $[0];
45 + }
46 + const s = t1;
47 + if (cond) {
48 + let t2;
49 + if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
50 + t2 = [s];
51 + $[1] = t2;
52 + } else {
53 + t2 = $[1];
54 + }
55 + return t2;
56 + } else {
57 + if (cond2) {
58 + let t2;
59 + if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
60 + t2 = { s };
61 + $[2] = t2;
62 + } else {
63 + t2 = $[2];
64 + }
65 + return t2;
66 + } else {
67 + let t2;
68 + if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
69 + t2 = { s: [s] };
70 + $[3] = t2;
71 + } else {
72 + t2 = $[3];
73 + }
74 + return t2;
75 + }
76 + }
77 +}
78 +
79 +export const FIXTURE_ENTRYPOINT = {
80 + fn: Foo,
81 + params: [{ cond: false, cond2: false }],
82 +};
83 +
84 +```
85 +
86 +### Eval output
87 +(kind: ok) {"s":["[[ function params=0 ]]"]}
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/return-ref-callback-structure.js renamed