[compiler] repro for dep merging edge case (non-hir)
Found when writing #31037, summary copied from comments: This is an extreme edge case and not code we'd expect any reasonable developer to write. In most cases e.g. `(a?.b != null ? a.b : DEFAULT)`, we do want to take a dependency on `a?.b`. I found this trying to come up with edge cases that break the current dependency + CFG merging logic. I think it makes sense to error on the side of correctness. After all, we still take `a` as a dependency if users write `a != null ? a.b : DEFAULT`, and the same fix (understanding the `<hoistable> != null` test expression) works for both. Can be convinced otherwise though! ghstack-source-id: cc06afda59f7681e228495f5e35a596c20f875f5 Pull Request resolved: https://github.com/facebook/react/pull/31035
Mofei Zhang committed
Sep 30, 2024 at 12:24 UTC
5d12e9e10b9957bc131ec77e013e1a76e4f32eb6
3 files changed
+120
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.expect.md
new
+88
@@ -0,0 +1,88 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import {identity} from 'shared-runtime';
6
+
7
+/**
8
+ * Evaluator failure:
9
+ * Found differences in evaluator results
10
+ * Non-forget (expected):
11
+ * (kind: ok) {}
12
+ * [[ (exception in render) TypeError: Cannot read properties of null (reading 'title_text') ]]
13
+ * Forget:
14
+ * (kind: ok) {}
15
+ * {}
16
+ */
17
+/**
18
+ * Very contrived text fixture showing that it's technically incorrect to merge
19
+ * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an
20
+ * unconditionally evaluated optional chain (`dep?.path`).
21
+ *
22
+ *
23
+ * when screen is non-null, useFoo returns { title: null } or "(not null)"
24
+ * when screen is null, useFoo throws
25
+ */
26
+function useFoo({screen}: {screen: null | undefined | {title_text: null}}) {
27
+ return screen?.title_text != null
28
+ ? '(not null)'
29
+ : identity({title: screen.title_text});
30
+}
31
+export const FIXTURE_ENTRYPOINT = {
32
+ fn: useFoo,
33
+ params: [{screen: null}],
34
+ sequentialRenders: [{screen: {title_bar: undefined}}, {screen: null}],
35
+};
36
+
37
+```
38
+
39
+## Code
40
+
41
+```javascript
42
+import { c as _c } from "react/compiler-runtime";
43
+import { identity } from "shared-runtime";
44
+
45
+/**
46
+ * Evaluator failure:
47
+ * Found differences in evaluator results
48
+ * Non-forget (expected):
49
+ * (kind: ok) {}
50
+ * [[ (exception in render) TypeError: Cannot read properties of null (reading 'title_text') ]]
51
+ * Forget:
52
+ * (kind: ok) {}
53
+ * {}
54
+ */
55
+/**
56
+ * Very contrived text fixture showing that it's technically incorrect to merge
57
+ * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an
58
+ * unconditionally evaluated optional chain (`dep?.path`).
59
+ *
60
+ *
61
+ * when screen is non-null, useFoo returns { title: null } or "(not null)"
62
+ * when screen is null, useFoo throws
63
+ */
64
+function useFoo(t0) {
65
+ const $ = _c(2);
66
+ const { screen } = t0;
67
+ let t1;
68
+ if ($[0] !== screen?.title_text) {
69
+ t1 =
70
+ screen?.title_text != null
71
+ ? "(not null)"
72
+ : identity({ title: screen.title_text });
73
+ $[0] = screen?.title_text;
74
+ $[1] = t1;
75
+ } else {
76
+ t1 = $[1];
77
+ }
78
+ return t1;
79
+}
80
+
81
+export const FIXTURE_ENTRYPOINT = {
82
+ fn: useFoo,
83
+ params: [{ screen: null }],
84
+ sequentialRenders: [{ screen: { title_bar: undefined } }, { screen: null }],
85
+};
86
+
87
+```
88
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.ts
new
+31
@@ -0,0 +1,31 @@
1
+import {identity} from 'shared-runtime';
2
+
3
+/**
4
+ * Evaluator failure:
5
+ * Found differences in evaluator results
6
+ * Non-forget (expected):
7
+ * (kind: ok) {}
8
+ * [[ (exception in render) TypeError: Cannot read properties of null (reading 'title_text') ]]
9
+ * Forget:
10
+ * (kind: ok) {}
11
+ * {}
12
+ */
13
+/**
14
+ * Very contrived text fixture showing that it's technically incorrect to merge
15
+ * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an
16
+ * unconditionally evaluated optional chain (`dep?.path`).
17
+ *
18
+ *
19
+ * when screen is non-null, useFoo returns { title: null } or "(not null)"
20
+ * when screen is null, useFoo throws
21
+ */
22
+function useFoo({screen}: {screen: null | undefined | {title_text: null}}) {
23
+ return screen?.title_text != null
24
+ ? '(not null)'
25
+ : identity({title: screen.title_text});
26
+}
27
+export const FIXTURE_ENTRYPOINT = {
28
+ fn: useFoo,
29
+ params: [{screen: null}],
30
+ sequentialRenders: [{screen: {title_bar: undefined}}, {screen: null}],
31
+};
compiler/packages/snap/src/SproutTodoFilter.ts
+1
@@ -478,6 +478,7 @@ const skipFilter = new Set([
478
'fbt/bug-fbt-plural-multiple-function-calls',
479
'fbt/bug-fbt-plural-multiple-mixed-call-tag',
480
'bug-invalid-hoisting-functionexpr',
481
+ 'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond',
482
'original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block',
483
'original-reactive-scopes-fork/bug-hoisted-declaration-with-scope',
484
'bug-codegen-inline-iife',