Fix/review bug repros, mark fixed
I reviewed the test cases we have marked as bugs ("_bug.*") and realized that several of them are already fixed — woohoo! Then one wasn't fixed _yet_: our type inference loses track of refs if you stash them inside an object/array. But that's why I added the ValidateNoRefAccessInRender pass, which i've updated to detect and reject these invalid cases. There are now only a few bugs left (more fixes coming).
Joe Savona committed
Jun 4, 2023 at 11:08 UTC
2ae3592d29caf9aca2cff36e6089c1dfb92dfa4e
12 files changed
+42
-66
compiler/forget/src/HIR/ValidateNoRefAccesInRender.ts
+10
@@ -40,6 +40,15 @@ export function validateNoRefAccessInRender(fn: HIRFunction): void {
40
for (const [, block] of fn.body.blocks) {
41
for (const instr of block.instructions) {
42
switch (instr.value.kind) {
43
+ case "PropertyLoad":
44
+ case "LoadLocal":
45
+ case "StoreLocal":
46
+ case "Destructure": {
47
+ // These instructions are necessary for storing the results of a useRef into
48
+ // a variable and referencing them in functions. We can propagate type info
49
+ // for these instructions so they ensure we have a complete analysis.
50
+ break;
51
+ }
52
case "FunctionExpression": {
53
// For now we assume *all* function expressions are safe, eventually we can
54
// be more precise and disallow ref access in functions that may be called
@@ -57,6 +66,7 @@ export function validateNoRefAccessInRender(fn: HIRFunction): void {
66
default: {
67
for (const operand of eachInstructionValueOperand(instr.value)) {
68
validateNonRefValue(error, operand);
69
+ validateNonRefObject(error, operand);
70
}
71
}
72
}
compiler/forget/src/__tests__/fixtures/compiler/_bug.use-ref-added-to-dep-without-type-info.expect.md
deleted
-61
@@ -1,61 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function Foo({ a }) {
6
- const ref = useRef();
7
- // type information is lost here as we don't track types of fields
8
- const val = { ref };
9
- // without type info, we don't know that val.ref.current is a ref value so we
10
- // end up depending on val.ref.current
11
- const x = { a, val: val.ref.current };
12
-
13
- return <VideoList videos={x} />;
14
-}
15
-
16
-```
17
-
18
-## Code
19
-
20
-```javascript
21
-import { unstable_useMemoCache as useMemoCache } from "react";
22
-function Foo(t23) {
23
- const $ = useMemoCache(7);
24
- const { a } = t23;
25
- const ref = useRef();
26
- const c_0 = $[0] !== ref;
27
- let t0;
28
- if (c_0) {
29
- t0 = { ref };
30
- $[0] = ref;
31
- $[1] = t0;
32
- } else {
33
- t0 = $[1];
34
- }
35
- const val = t0;
36
- const c_2 = $[2] !== a;
37
- const c_3 = $[3] !== val.ref.current;
38
- let t1;
39
- if (c_2 || c_3) {
40
- t1 = { a, val: val.ref.current };
41
- $[2] = a;
42
- $[3] = val.ref.current;
43
- $[4] = t1;
44
- } else {
45
- t1 = $[4];
46
- }
47
- const x = t1;
48
- const c_5 = $[5] !== x;
49
- let t2;
50
- if (c_5) {
51
- t2 = <VideoList videos={x} />;
52
- $[5] = x;
53
- $[6] = t2;
54
- } else {
55
- t2 = $[6];
56
- }
57
- return t2;
58
-}
59
-
60
-```
61
-
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/capturing-reference-changes-type.expect.md
renamed
compiler/forget/src/__tests__/fixtures/compiler/capturing-reference-changes-type.js
renamed
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md
-4
@@ -15,10 +15,6 @@ function Component(props) {
15
## Error
16
17
```
18
-[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at <unknown> $20:TObject<BuiltInRefValue> (4:4)
19
-
20
-[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at <unknown> value$21:TObject<BuiltInRefValue> (5:5)
21
-
18
[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at <unknown> $23:TObject<BuiltInRefValue> (5:5)
19
```
20
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md
+2
@@ -14,6 +14,8 @@ function Component(props) {
14
## Error
15
16
```
17
+[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at <unknown> $22:TObject<BuiltInUseRefId> (3:3)
18
+
19
[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at <unknown> $25:TObject<BuiltInRefValue> (4:4)
20
```
21
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md
new
+27
@@ -0,0 +1,27 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Foo({ a }) {
6
+ const ref = useRef();
7
+ // type information is lost here as we don't track types of fields
8
+ const val = { ref };
9
+ // without type info, we don't know that val.ref.current is a ref value so we
10
+ // *would* end up depending on val.ref.current
11
+ // however, this is an instance of accessing a ref during render and is disallowed
12
+ // under React's rules, so we reject this input
13
+ const x = { a, val: val.ref.current };
14
+
15
+ return <VideoList videos={x} />;
16
+}
17
+
18
+```
19
+
20
+
21
+## Error
22
+
23
+```
24
+[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at <unknown> $30:TObject<BuiltInUseRefId> (4:4)
25
+```
26
+
27
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.js
renamed
+3
-1
@@ -3,7 +3,9 @@ function Foo({ a }) {
3
// type information is lost here as we don't track types of fields
4
const val = { ref };
5
// without type info, we don't know that val.ref.current is a ref value so we
6
- // end up depending on val.ref.current
6
+ // *would* end up depending on val.ref.current
7
+ // however, this is an instance of accessing a ref during render and is disallowed
8
+ // under React's rules, so we reject this input
9
const x = { a, val: val.ref.current };
10
11
return <VideoList videos={x} />;