Skip to content

Commit 5d12e9e

Browse files
committed
[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: cc06afd Pull Request resolved: #31035
1 parent 58a3ca3 commit 5d12e9e

File tree

3 files changed

+120
-0
lines changed

3 files changed

+120
-0
lines changed
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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+
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,7 @@ const skipFilter = new Set([
478478
'fbt/bug-fbt-plural-multiple-function-calls',
479479
'fbt/bug-fbt-plural-multiple-mixed-call-tag',
480480
'bug-invalid-hoisting-functionexpr',
481+
'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond',
481482
'original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block',
482483
'original-reactive-scopes-fork/bug-hoisted-declaration-with-scope',
483484
'bug-codegen-inline-iife',

0 commit comments

Comments
 (0)