[compiler] Reposition ref-in-render errors to the read location of .current
Summary: Since we want to make ref-in-render errors enabled by default, we should position those errors at the location of the read. Not only will this be a better experience, but it also aligns the behavior of Forget and Flow. This PR also cleans up the resulting error messages to not emit implementation details about place values. ghstack-source-id: 1d1131706867a6fc88efddd631c4d16d2181e592 Pull Request resolved: https://github.com/facebook/react/pull/30723
Mike Vitousek committed
Aug 16, 2024 at 13:27 UTC
1016174af520fc89bafab7189405fce8ff3a9bb5
11 files changed
+79
-31
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts
+59
-14
@@ -15,7 +15,6 @@ import {
15
isRefValueType,
16
isUseRefType,
17
} from '../HIR';
18
-import {printPlace} from '../HIR/PrintHIR';
18
import {
19
eachInstructionValueOperand,
20
eachTerminalOperand,
@@ -53,6 +52,7 @@ function validateNoRefAccessInRenderImpl(
52
refAccessingFunctions: Set<IdentifierId>,
53
): Result<void, CompilerError> {
54
const errors = new CompilerError();
55
+ const lookupLocations: Map<IdentifierId, SourceLocation> = new Map();
56
for (const [, block] of fn.body.blocks) {
57
for (const instr of block.instructions) {
58
switch (instr.value.kind) {
@@ -64,10 +64,12 @@ function validateNoRefAccessInRenderImpl(
64
severity: ErrorSeverity.InvalidReact,
65
reason:
66
'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)',
67
- loc: operand.loc,
68
- description: `Cannot access ref value at ${printPlace(
69
- operand,
70
- )}`,
67
+ loc: lookupLocations.get(operand.identifier.id) ?? operand.loc,
68
+ description:
69
+ operand.identifier.name !== null &&
70
+ operand.identifier.name.kind === 'named'
71
+ ? `Cannot access ref value \`${operand.identifier.name.value}\``
72
+ : null,
73
suggestions: null,
74
});
75
}
@@ -75,12 +77,24 @@ function validateNoRefAccessInRenderImpl(
77
break;
78
}
79
case 'PropertyLoad': {
80
+ if (
81
+ isRefValueType(instr.lvalue.identifier) &&
82
+ instr.value.property === 'current'
83
+ ) {
84
+ lookupLocations.set(instr.lvalue.identifier.id, instr.loc);
85
+ }
86
break;
87
}
88
case 'LoadLocal': {
89
if (refAccessingFunctions.has(instr.value.place.identifier.id)) {
90
refAccessingFunctions.add(instr.lvalue.identifier.id);
91
}
92
+ if (isRefValueType(instr.lvalue.identifier)) {
93
+ const loc = lookupLocations.get(instr.value.place.identifier.id);
94
+ if (loc !== undefined) {
95
+ lookupLocations.set(instr.lvalue.identifier.id, loc);
96
+ }
97
+ }
98
break;
99
}
100
case 'StoreLocal': {
@@ -88,6 +102,13 @@ function validateNoRefAccessInRenderImpl(
102
refAccessingFunctions.add(instr.value.lvalue.place.identifier.id);
103
refAccessingFunctions.add(instr.lvalue.identifier.id);
104
}
105
+ if (isRefValueType(instr.value.lvalue.place.identifier)) {
106
+ const loc = lookupLocations.get(instr.value.value.identifier.id);
107
+ if (loc !== undefined) {
108
+ lookupLocations.set(instr.value.lvalue.place.identifier.id, loc);
109
+ lookupLocations.set(instr.lvalue.identifier.id, loc);
110
+ }
111
+ }
112
break;
113
}
114
case 'ObjectMethod':
@@ -140,7 +161,11 @@ function validateNoRefAccessInRenderImpl(
161
reason:
162
'This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef)',
163
loc: callee.loc,
143
- description: `Function ${printPlace(callee)} accesses a ref`,
164
+ description:
165
+ callee.identifier.name !== null &&
166
+ callee.identifier.name.kind === 'named'
167
+ ? `Function \`${callee.identifier.name.value}\` accesses a ref`
168
+ : null,
169
suggestions: null,
170
});
171
}
@@ -149,7 +174,7 @@ function validateNoRefAccessInRenderImpl(
174
errors,
175
refAccessingFunctions,
176
operand,
152
- operand.loc,
177
+ lookupLocations.get(operand.identifier.id) ?? operand.loc,
178
);
179
}
180
}
@@ -162,7 +187,7 @@ function validateNoRefAccessInRenderImpl(
187
errors,
188
refAccessingFunctions,
189
operand,
165
- operand.loc,
190
+ lookupLocations.get(operand.identifier.id) ?? operand.loc,
191
);
192
}
193
break;
@@ -175,13 +200,18 @@ function validateNoRefAccessInRenderImpl(
200
errors,
201
refAccessingFunctions,
202
instr.value.object,
178
- instr.loc,
203
+ lookupLocations.get(instr.value.object.identifier.id) ?? instr.loc,
204
);
205
for (const operand of eachInstructionValueOperand(instr.value)) {
206
if (operand === instr.value.object) {
207
continue;
208
}
184
- validateNoRefValueAccess(errors, refAccessingFunctions, operand);
209
+ validateNoRefValueAccess(
210
+ errors,
211
+ refAccessingFunctions,
212
+ lookupLocations,
213
+ operand,
214
+ );
215
}
216
break;
217
}
@@ -190,14 +220,24 @@ function validateNoRefAccessInRenderImpl(
220
break;
221
default: {
222
for (const operand of eachInstructionValueOperand(instr.value)) {
193
- validateNoRefValueAccess(errors, refAccessingFunctions, operand);
223
+ validateNoRefValueAccess(
224
+ errors,
225
+ refAccessingFunctions,
226
+ lookupLocations,
227
+ operand,
228
+ );
229
}
230
break;
231
}
232
}
233
}
234
for (const operand of eachTerminalOperand(block.terminal)) {
200
- validateNoRefValueAccess(errors, refAccessingFunctions, operand);
235
+ validateNoRefValueAccess(
236
+ errors,
237
+ refAccessingFunctions,
238
+ lookupLocations,
239
+ operand,
240
+ );
241
}
242
}
243
@@ -211,6 +251,7 @@ function validateNoRefAccessInRenderImpl(
251
function validateNoRefValueAccess(
252
errors: CompilerError,
253
refAccessingFunctions: Set<IdentifierId>,
254
+ lookupLocations: Map<IdentifierId, SourceLocation>,
255
operand: Place,
256
): void {
257
if (
@@ -221,8 +262,12 @@ function validateNoRefValueAccess(
262
severity: ErrorSeverity.InvalidReact,
263
reason:
264
'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)',
224
- loc: operand.loc,
225
- description: `Cannot access ref value at ${printPlace(operand)}`,
265
+ loc: lookupLocations.get(operand.identifier.id) ?? operand.loc,
266
+ description:
267
+ operand.identifier.name !== null &&
268
+ operand.identifier.name.kind === 'named'
269
+ ? `Cannot access ref value \`${operand.identifier.name.value}\``
270
+ : null,
271
suggestions: null,
272
});
273
}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md
+4
-3
@@ -15,10 +15,11 @@ function Component(props) {
15
## Error
16
17
```
18
+ 2 | function Component(props) {
19
3 | const ref = useRef(null);
19
- 4 | const value = ref.current;
20
-> 5 | return value;
21
- | ^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $22:TObject<BuiltInRefValue> (5:5)
20
+> 4 | const value = ref.current;
21
+ | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
22
+ 5 | return value;
23
6 | }
24
7 |
25
```
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-refs-in-render-transitive.expect.md
+1
-1
@@ -24,7 +24,7 @@ function Component() {
24
7 | };
25
8 | const changeRef = setRef;
26
> 9 | changeRef();
27
- | ^^^^^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef). Function mutate? $39[11:13]:TObject<BuiltInFunction> accesses a ref (9:9)
27
+ | ^^^^^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9)
28
29
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9)
30
10 |
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-destructure.expect.md
+4
-3
@@ -14,10 +14,11 @@ function Component({ref}) {
14
## Error
15
16
```
17
+ 1 | // @validateRefAccessDuringRender @compilationMode(infer)
18
2 | function Component({ref}) {
18
- 3 | const value = ref.current;
19
-> 4 | return <div>{value}</div>;
20
- | ^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at read $17:TObject<BuiltInRefValue> (4:4)
19
+> 3 | const value = ref.current;
20
+ | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (3:3)
21
+ 4 | return <div>{value}</div>;
22
5 | }
23
6 |
24
```
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-property-load.expect.md
+4
-3
@@ -14,10 +14,11 @@ function Component(props) {
14
## Error
15
16
```
17
+ 1 | // @validateRefAccessDuringRender @compilationMode(infer)
18
2 | function Component(props) {
18
- 3 | const value = props.ref.current;
19
-> 4 | return <div>{value}</div>;
20
- | ^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at read $15:TObject<BuiltInRefValue> (4:4)
19
+> 3 | const value = props.ref.current;
20
+ | ^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (3:3)
21
+ 4 | return <div>{value}</div>;
22
5 | }
23
6 |
24
```
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-value-as-props.expect.md
+1
-1
@@ -17,7 +17,7 @@ function Component(props) {
17
2 | function Component(props) {
18
3 | const ref = useRef(null);
19
> 4 | return <Foo ref={ref.current} />;
20
- | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $19:TObject<BuiltInRefValue> (4:4)
20
+ | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
21
5 | }
22
6 |
23
```
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md
+1
-1
@@ -20,7 +20,7 @@ function Component(props) {
20
> 4 | ref.current = props.value;
21
| ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
22
23
-InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $24:TObject<BuiltInRefValue> (5:5)
23
+InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
24
5 | return ref.current;
25
6 | }
26
7 |
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-nested-property-during-render.expect.md
+2
-2
@@ -18,9 +18,9 @@ function Component(props) {
18
2 | function Component(props) {
19
3 | const ref = useRef({inner: null});
20
> 4 | ref.current.inner = props.value;
21
- | ^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
21
+ | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
22
23
-InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $30:TObject<BuiltInRefValue> (5:5)
23
+InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
24
5 | return ref.current.inner;
25
6 | }
26
7 |
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.return-ref-callback.expect.md
+1
-1
@@ -28,7 +28,7 @@ export const FIXTURE_ENTRYPOINT = {
28
8 | };
29
9 |
30
> 10 | return s;
31
- | ^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $25:TObject<BuiltInFunction> (10:10)
31
+ | ^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (10:10)
32
11 | }
33
12 |
34
13 | export const FIXTURE_ENTRYPOINT = {
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md
+1
-1
@@ -34,7 +34,7 @@ export const FIXTURE_ENTRYPOINT = {
34
12 |
35
13 | // The ref is modified later, extending its range and preventing memoization of onChange
36
> 14 | ref.current.inner = null;
37
- | ^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (14:14)
37
+ | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (14:14)
38
15 |
39
16 | return <input onChange={onChange} />;
40
17 | }
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md
+1
-1
@@ -37,7 +37,7 @@ export const FIXTURE_ENTRYPOINT = {
37
15 | ref.current.inner = null;
38
16 | };
39
> 17 | reset();
40
- | ^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef). Function mutate? $77[20:22]:TObject<BuiltInFunction> accesses a ref (17:17)
40
+ | ^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef) (17:17)
41
42
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (17:17)
43
18 |