From 5d12e9e10b9957bc131ec77e013e1a76e4f32eb6 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 30 Sep 2024 12:24:22 -0400 Subject: [PATCH] [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 ` != 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 --- ...e-uncond-optional-chain-and-cond.expect.md | 88 +++++++++++++++++++ ...ug-merge-uncond-optional-chain-and-cond.ts | 31 +++++++ .../packages/snap/src/SproutTodoFilter.ts | 1 + 3 files changed, 120 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.expect.md new file mode 100644 index 0000000000..9a95e7dc87 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.expect.md @@ -0,0 +1,88 @@ + +## Input + +```javascript +import {identity} from 'shared-runtime'; + +/** + * Evaluator failure: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) {} + * [[ (exception in render) TypeError: Cannot read properties of null (reading 'title_text') ]] + * Forget: + * (kind: ok) {} + * {} + */ +/** + * Very contrived text fixture showing that it's technically incorrect to merge + * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an + * unconditionally evaluated optional chain (`dep?.path`). + * + * + * when screen is non-null, useFoo returns { title: null } or "(not null)" + * when screen is null, useFoo throws + */ +function useFoo({screen}: {screen: null | undefined | {title_text: null}}) { + return screen?.title_text != null + ? '(not null)' + : identity({title: screen.title_text}); +} +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{screen: null}], + sequentialRenders: [{screen: {title_bar: undefined}}, {screen: null}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity } from "shared-runtime"; + +/** + * Evaluator failure: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) {} + * [[ (exception in render) TypeError: Cannot read properties of null (reading 'title_text') ]] + * Forget: + * (kind: ok) {} + * {} + */ +/** + * Very contrived text fixture showing that it's technically incorrect to merge + * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an + * unconditionally evaluated optional chain (`dep?.path`). + * + * + * when screen is non-null, useFoo returns { title: null } or "(not null)" + * when screen is null, useFoo throws + */ +function useFoo(t0) { + const $ = _c(2); + const { screen } = t0; + let t1; + if ($[0] !== screen?.title_text) { + t1 = + screen?.title_text != null + ? "(not null)" + : identity({ title: screen.title_text }); + $[0] = screen?.title_text; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ screen: null }], + sequentialRenders: [{ screen: { title_bar: undefined } }, { screen: null }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.ts new file mode 100644 index 0000000000..bb361e3c9f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond.ts @@ -0,0 +1,31 @@ +import {identity} from 'shared-runtime'; + +/** + * Evaluator failure: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) {} + * [[ (exception in render) TypeError: Cannot read properties of null (reading 'title_text') ]] + * Forget: + * (kind: ok) {} + * {} + */ +/** + * Very contrived text fixture showing that it's technically incorrect to merge + * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an + * unconditionally evaluated optional chain (`dep?.path`). + * + * + * when screen is non-null, useFoo returns { title: null } or "(not null)" + * when screen is null, useFoo throws + */ +function useFoo({screen}: {screen: null | undefined | {title_text: null}}) { + return screen?.title_text != null + ? '(not null)' + : identity({title: screen.title_text}); +} +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{screen: null}], + sequentialRenders: [{screen: {title_bar: undefined}}, {screen: null}], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 7140cad2f7..0ae22a643b 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -478,6 +478,7 @@ const skipFilter = new Set([ 'fbt/bug-fbt-plural-multiple-function-calls', 'fbt/bug-fbt-plural-multiple-mixed-call-tag', 'bug-invalid-hoisting-functionexpr', + 'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond', 'original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block', 'original-reactive-scopes-fork/bug-hoisted-declaration-with-scope', 'bug-codegen-inline-iife',