From 2d16326d9a3f45260aa80bcae78745ab2f199138 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 30 Sep 2024 17:07:54 +0100 Subject: [PATCH 01/13] fix[scripts/devtools/publish-release]: parse version list instead of handling 404 (#31087) Discovered yesterday while was publishing a new release. NPM `10.x.x` changed the text for 404 errors, so this check was failing. Instead of handling 404 as a signal, I think its better to just parse the whole list of versions and check if the new one is already there. --- scripts/devtools/publish-release.js | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/scripts/devtools/publish-release.js b/scripts/devtools/publish-release.js index 495fddefc2cd6..6a2ab4f79d2ef 100755 --- a/scripts/devtools/publish-release.js +++ b/scripts/devtools/publish-release.js @@ -82,18 +82,13 @@ async function publishToNPM() { // If so we might be resuming from a previous run. // We could infer this by comparing the build-info.json, // But for now the easiest way is just to ask if this is expected. - const info = await execRead(`npm view ${npmPackage}@${version}`) - // Early versions of npm view gives empty response, but newer versions give 404 error. - // Catch the error to keep it consistent. - .catch(childProcessError => { - if (childProcessError.stderr.startsWith('npm ERR! code E404')) { - return null; - } - - throw childProcessError; - }); + const versionListJSON = await execRead( + `npm view ${npmPackage} versions --json` + ); + const versionList = JSON.parse(versionListJSON); + const versionIsAlreadyPublished = versionList.includes(version); - if (info) { + if (versionIsAlreadyPublished) { console.log(''); console.log( `${npmPackage} version ${chalk.bold( From 943e45e910d1a125f2be431c2b66f22a035ea0c9 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 30 Sep 2024 12:24:19 -0400 Subject: [PATCH 02/13] [compiler][test fixtures] Fork more fixtures for hir-rewrite Followup from #30894 , not sure how these got missed. Note that this PR just copies the fixtures without adding `@enablePropagateDepsInHIR`. #31032 follows and actually enables the HIR-version of propagateScopeDeps to run. I split this out into two PRs to make snapshot differences easier to review, but also happy to merge Fixtures found from locally setting snap test runner to default to `enablePropagateDepsInHIR: 'enabled_baseline'` and forking fixtures files with different output. ghstack-source-id: 7d7cf41aa923d83ad49f89079171b0411923ce6b Pull Request resolved: https://github.com/facebook/react/pull/31030 --- .../conditional-break-labeled.expect.md | 65 ++++++ .../conditional-break-labeled.js | 21 ++ .../conditional-early-return.expect.md | 197 ++++++++++++++++++ .../conditional-early-return.js | 58 ++++++ .../conditional-on-mutable.expect.md | 88 ++++++++ .../conditional-on-mutable.js | 26 +++ ...rly-return-within-reactive-scope.expect.md | 89 ++++++++ ...sted-early-return-within-reactive-scope.js | 21 ++ ...rly-return-within-reactive-scope.expect.md | 112 ++++++++++ .../early-return-within-reactive-scope.js | 33 +++ ...-optional-call-chain-in-optional.expect.md | 34 +++ ...or.todo-optional-call-chain-in-optional.ts | 13 ++ .../iife-return-modified-later-phi.expect.md | 57 +++++ .../iife-return-modified-later-phi.js | 16 ++ ...consequent-alternate-both-return.expect.md | 66 ++++++ ...ted-in-consequent-alternate-both-return.js | 17 ++ ...rly-return-within-reactive-scope.expect.md | 80 +++++++ ...tial-early-return-within-reactive-scope.js | 20 ++ ...ence-array-push-consecutive-phis.expect.md | 110 ++++++++++ ...e-inference-array-push-consecutive-phis.js | 37 ++++ .../phi-type-inference-array-push.expect.md | 83 ++++++++ .../phi-type-inference-array-push.js | 26 +++ ...hi-type-inference-property-store.expect.md | 72 +++++++ .../phi-type-inference-property-store.js | 22 ++ ...properties-inside-optional-chain.expect.md | 31 +++ ...tional-properties-inside-optional-chain.js | 3 + ...ming-unconditional-with-mutation.expect.md | 79 +++++++ ...sa-renaming-unconditional-with-mutation.js | 27 +++ .../packages/snap/src/SproutTodoFilter.ts | 1 + 29 files changed, 1504 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.expect.md new file mode 100644 index 0000000000000..76648c251a101 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +/** + * props.b *does* influence `a` + */ +function Component(props) { + const a = []; + a.push(props.a); + label: { + if (props.b) { + break label; + } + a.push(props.c); + } + a.push(props.d); + return a; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ['TodoAdd'], + isComponent: 'TodoAdd', +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; /** + * props.b *does* influence `a` + */ +function Component(props) { + const $ = _c(2); + let a; + if ($[0] !== props) { + a = []; + a.push(props.a); + bb0: { + if (props.b) { + break bb0; + } + + a.push(props.c); + } + + a.push(props.d); + $[0] = props; + $[1] = a; + } else { + a = $[1]; + } + return a; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ["TodoAdd"], + isComponent: "TodoAdd", +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.js new file mode 100644 index 0000000000000..ccf983189cf71 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.js @@ -0,0 +1,21 @@ +/** + * props.b *does* influence `a` + */ +function Component(props) { + const a = []; + a.push(props.a); + label: { + if (props.b) { + break label; + } + a.push(props.c); + } + a.push(props.d); + return a; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ['TodoAdd'], + isComponent: 'TodoAdd', +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.expect.md new file mode 100644 index 0000000000000..82537902bfa19 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.expect.md @@ -0,0 +1,197 @@ + +## Input + +```javascript +/** + * props.b does *not* influence `a` + */ +function ComponentA(props) { + const a_DEBUG = []; + a_DEBUG.push(props.a); + if (props.b) { + return null; + } + a_DEBUG.push(props.d); + return a_DEBUG; +} + +/** + * props.b *does* influence `a` + */ +function ComponentB(props) { + const a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + } + a.push(props.d); + return a; +} + +/** + * props.b *does* influence `a`, but only in a way that is never observable + */ +function ComponentC(props) { + const a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + return null; + } + a.push(props.d); + return a; +} + +/** + * props.b *does* influence `a` + */ +function ComponentD(props) { + const a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + return a; + } + a.push(props.d); + return a; +} + +export const FIXTURE_ENTRYPOINT = { + fn: ComponentA, + params: [{a: 1, b: false, d: 3}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; /** + * props.b does *not* influence `a` + */ +function ComponentA(props) { + const $ = _c(3); + let a_DEBUG; + let t0; + if ($[0] !== props) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + a_DEBUG = []; + a_DEBUG.push(props.a); + if (props.b) { + t0 = null; + break bb0; + } + + a_DEBUG.push(props.d); + } + $[0] = props; + $[1] = a_DEBUG; + $[2] = t0; + } else { + a_DEBUG = $[1]; + t0 = $[2]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } + return a_DEBUG; +} + +/** + * props.b *does* influence `a` + */ +function ComponentB(props) { + const $ = _c(2); + let a; + if ($[0] !== props) { + a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + } + + a.push(props.d); + $[0] = props; + $[1] = a; + } else { + a = $[1]; + } + return a; +} + +/** + * props.b *does* influence `a`, but only in a way that is never observable + */ +function ComponentC(props) { + const $ = _c(3); + let a; + let t0; + if ($[0] !== props) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + t0 = null; + break bb0; + } + + a.push(props.d); + } + $[0] = props; + $[1] = a; + $[2] = t0; + } else { + a = $[1]; + t0 = $[2]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } + return a; +} + +/** + * props.b *does* influence `a` + */ +function ComponentD(props) { + const $ = _c(3); + let a; + let t0; + if ($[0] !== props) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + t0 = a; + break bb0; + } + + a.push(props.d); + } + $[0] = props; + $[1] = a; + $[2] = t0; + } else { + a = $[1]; + t0 = $[2]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } + return a; +} + +export const FIXTURE_ENTRYPOINT = { + fn: ComponentA, + params: [{ a: 1, b: false, d: 3 }], +}; + +``` + +### Eval output +(kind: ok) [1,3] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.js new file mode 100644 index 0000000000000..e0ed101409252 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.js @@ -0,0 +1,58 @@ +/** + * props.b does *not* influence `a` + */ +function ComponentA(props) { + const a_DEBUG = []; + a_DEBUG.push(props.a); + if (props.b) { + return null; + } + a_DEBUG.push(props.d); + return a_DEBUG; +} + +/** + * props.b *does* influence `a` + */ +function ComponentB(props) { + const a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + } + a.push(props.d); + return a; +} + +/** + * props.b *does* influence `a`, but only in a way that is never observable + */ +function ComponentC(props) { + const a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + return null; + } + a.push(props.d); + return a; +} + +/** + * props.b *does* influence `a` + */ +function ComponentD(props) { + const a = []; + a.push(props.a); + if (props.b) { + a.push(props.c); + return a; + } + a.push(props.d); + return a; +} + +export const FIXTURE_ENTRYPOINT = { + fn: ComponentA, + params: [{a: 1, b: false, d: 3}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.expect.md new file mode 100644 index 0000000000000..24c565b088d2f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.expect.md @@ -0,0 +1,88 @@ + +## Input + +```javascript +function ComponentA(props) { + const a = []; + const b = []; + if (b) { + a.push(props.p0); + } + if (props.p1) { + b.push(props.p2); + } + return ; +} + +function ComponentB(props) { + const a = []; + const b = []; + if (mayMutate(b)) { + a.push(props.p0); + } + if (props.p1) { + b.push(props.p2); + } + return ; +} + +function Foo() {} +function mayMutate() {} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function ComponentA(props) { + const $ = _c(2); + let t0; + if ($[0] !== props) { + const a = []; + const b = []; + if (b) { + a.push(props.p0); + } + if (props.p1) { + b.push(props.p2); + } + + t0 = ; + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +function ComponentB(props) { + const $ = _c(2); + let t0; + if ($[0] !== props) { + const a = []; + const b = []; + if (mayMutate(b)) { + a.push(props.p0); + } + if (props.p1) { + b.push(props.p2); + } + + t0 = ; + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +function Foo() {} +function mayMutate() {} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.js new file mode 100644 index 0000000000000..0378ab655c1ef --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.js @@ -0,0 +1,26 @@ +function ComponentA(props) { + const a = []; + const b = []; + if (b) { + a.push(props.p0); + } + if (props.p1) { + b.push(props.p2); + } + return ; +} + +function ComponentB(props) { + const a = []; + const b = []; + if (mayMutate(b)) { + a.push(props.p0); + } + if (props.p1) { + b.push(props.p2); + } + return ; +} + +function Foo() {} +function mayMutate() {} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.expect.md new file mode 100644 index 0000000000000..2d33981f73fd1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.expect.md @@ -0,0 +1,89 @@ + +## Input + +```javascript +function Component(props) { + let x = []; + if (props.cond) { + x.push(props.a); + if (props.b) { + const y = [props.b]; + x.push(y); + // oops no memo! + return x; + } + // oops no memo! + return x; + } else { + return foo(); + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true, a: 42, b: 3.14}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component(props) { + const $ = _c(5); + let t0; + if ($[0] !== props) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + const x = []; + if (props.cond) { + x.push(props.a); + if (props.b) { + let t1; + if ($[2] !== props.b) { + t1 = [props.b]; + $[2] = props.b; + $[3] = t1; + } else { + t1 = $[3]; + } + const y = t1; + x.push(y); + t0 = x; + break bb0; + } + + t0 = x; + break bb0; + } else { + let t1; + if ($[4] === Symbol.for("react.memo_cache_sentinel")) { + t1 = foo(); + $[4] = t1; + } else { + t1 = $[4]; + } + t0 = t1; + break bb0; + } + } + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true, a: 42, b: 3.14 }], +}; + +``` + +### Eval output +(kind: ok) [42,[3.14]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.js new file mode 100644 index 0000000000000..53eb06bc591e4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.js @@ -0,0 +1,21 @@ +function Component(props) { + let x = []; + if (props.cond) { + x.push(props.a); + if (props.b) { + const y = [props.b]; + x.push(y); + // oops no memo! + return x; + } + // oops no memo! + return x; + } else { + return foo(); + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true, a: 42, b: 3.14}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.expect.md new file mode 100644 index 0000000000000..6c3525e9e77eb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.expect.md @@ -0,0 +1,112 @@ + +## Input + +```javascript +import {makeArray} from 'shared-runtime'; + +function Component(props) { + let x = []; + if (props.cond) { + x.push(props.a); + // oops no memo! + return x; + } else { + return makeArray(props.b); + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + // pattern 1 + {cond: true, a: 42}, + {cond: true, a: 42}, + // pattern 2 + {cond: false, b: 3.14}, + {cond: false, b: 3.14}, + // pattern 1 + {cond: true, a: 42}, + // pattern 2 + {cond: false, b: 3.14}, + // pattern 1 + {cond: true, a: 42}, + // pattern 2 + {cond: false, b: 3.14}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray } from "shared-runtime"; + +function Component(props) { + const $ = _c(4); + let t0; + if ($[0] !== props) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + const x = []; + if (props.cond) { + x.push(props.a); + t0 = x; + break bb0; + } else { + let t1; + if ($[2] !== props.b) { + t1 = makeArray(props.b); + $[2] = props.b; + $[3] = t1; + } else { + t1 = $[3]; + } + t0 = t1; + break bb0; + } + } + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + // pattern 1 + { cond: true, a: 42 }, + { cond: true, a: 42 }, + // pattern 2 + { cond: false, b: 3.14 }, + { cond: false, b: 3.14 }, + // pattern 1 + { cond: true, a: 42 }, + // pattern 2 + { cond: false, b: 3.14 }, + // pattern 1 + { cond: true, a: 42 }, + // pattern 2 + { cond: false, b: 3.14 }, + ], +}; + +``` + +### Eval output +(kind: ok) [42] +[42] +[3.14] +[3.14] +[42] +[3.14] +[42] +[3.14] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.js new file mode 100644 index 0000000000000..7446d76fb320c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.js @@ -0,0 +1,33 @@ +import {makeArray} from 'shared-runtime'; + +function Component(props) { + let x = []; + if (props.cond) { + x.push(props.a); + // oops no memo! + return x; + } else { + return makeArray(props.b); + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + // pattern 1 + {cond: true, a: 42}, + {cond: true, a: 42}, + // pattern 2 + {cond: false, b: 3.14}, + {cond: false, b: 3.14}, + // pattern 1 + {cond: true, a: 42}, + // pattern 2 + {cond: false, b: 3.14}, + // pattern 1 + {cond: true, a: 42}, + // pattern 2 + {cond: false, b: 3.14}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md new file mode 100644 index 0000000000000..75c5d61d407f0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md @@ -0,0 +1,34 @@ + +## Input + +```javascript +function useFoo(props: {value: {x: string; y: string} | null}) { + const value = props.value; + return createArray(value?.x, value?.y)?.join(', '); +} + +function createArray(...args: Array): Array { + return args; +} + +export const FIXTURE_ENTRYPONT = { + fn: useFoo, + props: [{value: null}], +}; + +``` + + +## Error + +``` + 1 | function useFoo(props: {value: {x: string; y: string} | null}) { + 2 | const value = props.value; +> 3 | return createArray(value?.x, value?.y)?.join(', '); + | ^^^^^^^^ Todo: Unexpected terminal kind `optional` for optional test block (3:3) + 4 | } + 5 | + 6 | function createArray(...args: Array): Array { +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.ts new file mode 100644 index 0000000000000..7ee50e0380288 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.ts @@ -0,0 +1,13 @@ +function useFoo(props: {value: {x: string; y: string} | null}) { + const value = props.value; + return createArray(value?.x, value?.y)?.join(', '); +} + +function createArray(...args: Array): Array { + return args; +} + +export const FIXTURE_ENTRYPONT = { + fn: useFoo, + props: [{value: null}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.expect.md new file mode 100644 index 0000000000000..bed1c329f0d10 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.expect.md @@ -0,0 +1,57 @@ + +## Input + +```javascript +function Component(props) { + const items = (() => { + if (props.cond) { + return []; + } else { + return null; + } + })(); + items?.push(props.a); + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{a: {}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component(props) { + const $ = _c(2); + let items; + if ($[0] !== props) { + let t0; + if (props.cond) { + t0 = []; + } else { + t0 = null; + } + items = t0; + + items?.push(props.a); + $[0] = props; + $[1] = items; + } else { + items = $[1]; + } + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ a: {} }], +}; + +``` + +### Eval output +(kind: ok) null \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.js new file mode 100644 index 0000000000000..f4f953d294e6f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.js @@ -0,0 +1,16 @@ +function Component(props) { + const items = (() => { + if (props.cond) { + return []; + } else { + return null; + } + })(); + items?.push(props.a); + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{a: {}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.expect.md new file mode 100644 index 0000000000000..8a20f9186b447 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.expect.md @@ -0,0 +1,66 @@ + +## Input + +```javascript +import {makeObject_Primitives} from 'shared-runtime'; + +function Component(props) { + const object = makeObject_Primitives(); + if (props.cond) { + object.value = 1; + return object; + } else { + object.value = props.value; + return object; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: false, value: [0, 1, 2]}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeObject_Primitives } from "shared-runtime"; + +function Component(props) { + const $ = _c(2); + let t0; + if ($[0] !== props) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + const object = makeObject_Primitives(); + if (props.cond) { + object.value = 1; + t0 = object; + break bb0; + } else { + object.value = props.value; + t0 = object; + break bb0; + } + } + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: false, value: [0, 1, 2] }], +}; + +``` + +### Eval output +(kind: ok) {"a":0,"b":"value1","c":true,"value":[0,1,2]} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.js new file mode 100644 index 0000000000000..94a142d033708 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.js @@ -0,0 +1,17 @@ +import {makeObject_Primitives} from 'shared-runtime'; + +function Component(props) { + const object = makeObject_Primitives(); + if (props.cond) { + object.value = 1; + return object; + } else { + object.value = props.value; + return object; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: false, value: [0, 1, 2]}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.expect.md new file mode 100644 index 0000000000000..398161f0c6a24 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.expect.md @@ -0,0 +1,80 @@ + +## Input + +```javascript +function Component(props) { + let x = []; + let y = null; + if (props.cond) { + x.push(props.a); + // oops no memo! + return x; + } else { + y = foo(); + if (props.b) { + return; + } + } + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true, a: 42}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component(props) { + const $ = _c(4); + let y; + let t0; + if ($[0] !== props) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + const x = []; + if (props.cond) { + x.push(props.a); + t0 = x; + break bb0; + } else { + let t1; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t1 = foo(); + $[3] = t1; + } else { + t1 = $[3]; + } + y = t1; + if (props.b) { + t0 = undefined; + break bb0; + } + } + } + $[0] = props; + $[1] = y; + $[2] = t0; + } else { + y = $[1]; + t0 = $[2]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true, a: 42 }], +}; + +``` + +### Eval output +(kind: ok) [42] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.js new file mode 100644 index 0000000000000..66d68242b8bb6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.js @@ -0,0 +1,20 @@ +function Component(props) { + let x = []; + let y = null; + if (props.cond) { + x.push(props.a); + // oops no memo! + return x; + } else { + y = foo(); + if (props.b) { + return; + } + } + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true, a: 42}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.expect.md new file mode 100644 index 0000000000000..f17bcc92cb7ce --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.expect.md @@ -0,0 +1,110 @@ + +## Input + +```javascript +import {makeArray} from 'shared-runtime'; + +function Component(props) { + const x = {}; + let y; + if (props.cond) { + if (props.cond2) { + y = [props.value]; + } else { + y = [props.value2]; + } + } else { + y = []; + } + // This should be inferred as ` y` s.t. `x` can still + // be independently memoized. *But* this also must properly + // extend the mutable range of the array literals in the + // if/else branches + y.push(x); + + return [x, y]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true, cond2: true, value: 42}], + sequentialRenders: [ + {cond: true, cond2: true, value: 3.14}, + {cond: true, cond2: true, value: 42}, + {cond: true, cond2: true, value: 3.14}, + {cond: true, cond2: false, value2: 3.14}, + {cond: true, cond2: false, value2: 42}, + {cond: true, cond2: false, value2: 3.14}, + {cond: false}, + {cond: false}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray } from "shared-runtime"; + +function Component(props) { + const $ = _c(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = {}; + $[0] = t0; + } else { + t0 = $[0]; + } + const x = t0; + let t1; + if ($[1] !== props) { + let y; + if (props.cond) { + if (props.cond2) { + y = [props.value]; + } else { + y = [props.value2]; + } + } else { + y = []; + } + + y.push(x); + + t1 = [x, y]; + $[1] = props; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true, cond2: true, value: 42 }], + sequentialRenders: [ + { cond: true, cond2: true, value: 3.14 }, + { cond: true, cond2: true, value: 42 }, + { cond: true, cond2: true, value: 3.14 }, + { cond: true, cond2: false, value2: 3.14 }, + { cond: true, cond2: false, value2: 42 }, + { cond: true, cond2: false, value2: 3.14 }, + { cond: false }, + { cond: false }, + ], +}; + +``` + +### Eval output +(kind: ok) [{},[3.14,"[[ cyclic ref *1 ]]"]] +[{},[42,"[[ cyclic ref *1 ]]"]] +[{},[3.14,"[[ cyclic ref *1 ]]"]] +[{},[3.14,"[[ cyclic ref *1 ]]"]] +[{},[42,"[[ cyclic ref *1 ]]"]] +[{},[3.14,"[[ cyclic ref *1 ]]"]] +[{},["[[ cyclic ref *1 ]]"]] +[{},["[[ cyclic ref *1 ]]"]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.js new file mode 100644 index 0000000000000..0a9aa39defea4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.js @@ -0,0 +1,37 @@ +import {makeArray} from 'shared-runtime'; + +function Component(props) { + const x = {}; + let y; + if (props.cond) { + if (props.cond2) { + y = [props.value]; + } else { + y = [props.value2]; + } + } else { + y = []; + } + // This should be inferred as ` y` s.t. `x` can still + // be independently memoized. *But* this also must properly + // extend the mutable range of the array literals in the + // if/else branches + y.push(x); + + return [x, y]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true, cond2: true, value: 42}], + sequentialRenders: [ + {cond: true, cond2: true, value: 3.14}, + {cond: true, cond2: true, value: 42}, + {cond: true, cond2: true, value: 3.14}, + {cond: true, cond2: false, value2: 3.14}, + {cond: true, cond2: false, value2: 42}, + {cond: true, cond2: false, value2: 3.14}, + {cond: false}, + {cond: false}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.expect.md new file mode 100644 index 0000000000000..f58eed10fda5a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.expect.md @@ -0,0 +1,83 @@ + +## Input + +```javascript +function Component(props) { + const x = {}; + let y; + if (props.cond) { + y = [props.value]; + } else { + y = []; + } + // This should be inferred as ` y` s.t. `x` can still + // be independently memoized. *But* this also must properly + // extend the mutable range of the array literals in the + // if/else branches + y.push(x); + + return [x, y]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true, value: 42}], + sequentialRenders: [ + {cond: true, value: 3.14}, + {cond: false, value: 3.14}, + {cond: true, value: 42}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component(props) { + const $ = _c(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = {}; + $[0] = t0; + } else { + t0 = $[0]; + } + const x = t0; + let t1; + if ($[1] !== props) { + let y; + if (props.cond) { + y = [props.value]; + } else { + y = []; + } + + y.push(x); + + t1 = [x, y]; + $[1] = props; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true, value: 42 }], + sequentialRenders: [ + { cond: true, value: 3.14 }, + { cond: false, value: 3.14 }, + { cond: true, value: 42 }, + ], +}; + +``` + +### Eval output +(kind: ok) [{},[3.14,"[[ cyclic ref *1 ]]"]] +[{},["[[ cyclic ref *1 ]]"]] +[{},[42,"[[ cyclic ref *1 ]]"]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.js new file mode 100644 index 0000000000000..732a1524cba2d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.js @@ -0,0 +1,26 @@ +function Component(props) { + const x = {}; + let y; + if (props.cond) { + y = [props.value]; + } else { + y = []; + } + // This should be inferred as ` y` s.t. `x` can still + // be independently memoized. *But* this also must properly + // extend the mutable range of the array literals in the + // if/else branches + y.push(x); + + return [x, y]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true, value: 42}], + sequentialRenders: [ + {cond: true, value: 3.14}, + {cond: false, value: 3.14}, + {cond: true, value: 42}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.expect.md new file mode 100644 index 0000000000000..70551c8e9d7b0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.expect.md @@ -0,0 +1,72 @@ + +## Input + +```javascript +// @debug +function Component(props) { + const x = {}; + let y; + if (props.cond) { + y = {}; + } else { + y = {a: props.a}; + } + // This should be inferred as ` y` s.t. `x` can still + // be independently memoized. *But* this also must properly + // extend the mutable range of the object literals in the + // if/else branches + y.x = x; + + return [x, y]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: false, a: 'a!'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @debug +function Component(props) { + const $ = _c(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = {}; + $[0] = t0; + } else { + t0 = $[0]; + } + const x = t0; + let t1; + if ($[1] !== props) { + let y; + if (props.cond) { + y = {}; + } else { + y = { a: props.a }; + } + + y.x = x; + + t1 = [x, y]; + $[1] = props; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: false, a: "a!" }], +}; + +``` + +### Eval output +(kind: ok) [{},{"a":"a!","x":"[[ cyclic ref *1 ]]"}] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.js new file mode 100644 index 0000000000000..ba7133ff808df --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.js @@ -0,0 +1,22 @@ +// @debug +function Component(props) { + const x = {}; + let y; + if (props.cond) { + y = {}; + } else { + y = {a: props.a}; + } + // This should be inferred as ` y` s.t. `x` can still + // be independently memoized. *But* this also must properly + // extend the mutable range of the object literals in the + // if/else branches + y.x = x; + + return [x, y]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: false, a: 'a!'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md new file mode 100644 index 0000000000000..6574bd1966ed9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md @@ -0,0 +1,31 @@ + +## Input + +```javascript +function Component(props) { + return props.post.feedback.comments?.edges?.map(render); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component(props) { + const $ = _c(2); + let t0; + if ($[0] !== props.post.feedback.comments) { + t0 = props.post.feedback.comments?.edges?.map(render); + $[0] = props.post.feedback.comments; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.js new file mode 100644 index 0000000000000..e8e0da392e9e7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.js @@ -0,0 +1,3 @@ +function Component(props) { + return props.post.feedback.comments?.edges?.map(render); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.expect.md new file mode 100644 index 0000000000000..f4689e5795552 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.expect.md @@ -0,0 +1,79 @@ + +## Input + +```javascript +import {mutate} from 'shared-runtime'; + +function useFoo(props) { + let x = []; + x.push(props.bar); + if (props.cond) { + x = {}; + x = []; + x.push(props.foo); + } else { + x = []; + x = []; + x.push(props.bar); + } + mutate(x); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{bar: 'bar', foo: 'foo', cond: true}], + sequentialRenders: [ + {bar: 'bar', foo: 'foo', cond: true}, + {bar: 'bar', foo: 'foo', cond: true}, + {bar: 'bar', foo: 'foo', cond: false}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { mutate } from "shared-runtime"; + +function useFoo(props) { + const $ = _c(2); + let x; + if ($[0] !== props) { + x = []; + x.push(props.bar); + if (props.cond) { + x = []; + x.push(props.foo); + } else { + x = []; + x.push(props.bar); + } + + mutate(x); + $[0] = props; + $[1] = x; + } else { + x = $[1]; + } + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ bar: "bar", foo: "foo", cond: true }], + sequentialRenders: [ + { bar: "bar", foo: "foo", cond: true }, + { bar: "bar", foo: "foo", cond: true }, + { bar: "bar", foo: "foo", cond: false }, + ], +}; + +``` + +### Eval output +(kind: ok) ["foo","joe"] +["foo","joe"] +["bar","joe"] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.js new file mode 100644 index 0000000000000..3e7078cfc7999 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.js @@ -0,0 +1,27 @@ +import {mutate} from 'shared-runtime'; + +function useFoo(props) { + let x = []; + x.push(props.bar); + if (props.cond) { + x = {}; + x = []; + x.push(props.foo); + } else { + x = []; + x = []; + x.push(props.bar); + } + mutate(x); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{bar: 'bar', foo: 'foo', cond: true}], + sequentialRenders: [ + {bar: 'bar', foo: 'foo', cond: true}, + {bar: 'bar', foo: 'foo', cond: true}, + {bar: 'bar', foo: 'foo', cond: false}, + ], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 7d7476dc74739..7140cad2f74bc 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -50,6 +50,7 @@ const skipFilter = new Set([ 'component', 'cond-deps-conditional-member-expr', 'conditional-break-labeled', + 'propagate-scope-deps-hir-fork/conditional-break-labeled', 'conditional-set-state-in-render', 'constant-computed', 'constant-propagation-phi', From 1a779207a7b85314e16d410b185d427702f22ebc Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 30 Sep 2024 12:24:20 -0400 Subject: [PATCH 03/13] [compiler][test fixtures] Add enablePropagateDepsInHIR to forked tests Annotates fixtures added in #31030 with `@enablePropagateDepsInHIR` to fork behavior (and commit snapshot differences) ghstack-source-id: e423e8c42db62f1bb87562b770761be09fc8ffc6 Pull Request resolved: https://github.com/facebook/react/pull/31031 --- .../conditional-break-labeled.expect.md | 22 +++-- .../conditional-break-labeled.js | 1 + .../conditional-early-return.expect.md | 82 +++++++++++++------ .../conditional-early-return.js | 1 + .../conditional-on-mutable.expect.md | 27 +++--- .../conditional-on-mutable.js | 1 + ...rly-return-within-reactive-scope.expect.md | 29 ++++--- ...sted-early-return-within-reactive-scope.js | 1 + ...rly-return-within-reactive-scope.expect.md | 23 +++--- .../early-return-within-reactive-scope.js | 1 + ...-optional-call-chain-in-optional.expect.md | 15 ++-- ...or.todo-optional-call-chain-in-optional.ts | 1 + .../iife-return-modified-later-phi.expect.md | 14 ++-- .../iife-return-modified-later-phi.js | 1 + ...consequent-alternate-both-return.expect.md | 14 ++-- ...ted-in-consequent-alternate-both-return.js | 1 + ...rly-return-within-reactive-scope.expect.md | 25 +++--- ...tial-early-return-within-reactive-scope.js | 1 + ...ence-array-push-consecutive-phis.expect.md | 21 +++-- ...e-inference-array-push-consecutive-phis.js | 1 + .../phi-type-inference-array-push.expect.md | 14 ++-- .../phi-type-inference-array-push.js | 1 + ...hi-type-inference-property-store.expect.md | 15 ++-- .../phi-type-inference-property-store.js | 2 +- ...properties-inside-optional-chain.expect.md | 3 +- ...tional-properties-inside-optional-chain.js | 1 + ...ming-unconditional-with-mutation.expect.md | 15 ++-- ...sa-renaming-unconditional-with-mutation.js | 1 + 28 files changed, 210 insertions(+), 124 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.expect.md index 76648c251a101..f778c74037d32 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR /** * props.b *does* influence `a` */ @@ -29,13 +30,19 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; /** +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR +/** * props.b *does* influence `a` */ function Component(props) { - const $ = _c(2); + const $ = _c(5); let a; - if ($[0] !== props) { + if ( + $[0] !== props.a || + $[1] !== props.b || + $[2] !== props.c || + $[3] !== props.d + ) { a = []; a.push(props.a); bb0: { @@ -47,10 +54,13 @@ function Component(props) { } a.push(props.d); - $[0] = props; - $[1] = a; + $[0] = props.a; + $[1] = props.b; + $[2] = props.c; + $[3] = props.d; + $[4] = a; } else { - a = $[1]; + a = $[4]; } return a; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.js index ccf983189cf71..770a4794fe075 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-break-labeled.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR /** * props.b *does* influence `a` */ diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.expect.md index 82537902bfa19..602859389e3a1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR /** * props.b does *not* influence `a` */ @@ -66,14 +67,15 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; /** +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR +/** * props.b does *not* influence `a` */ function ComponentA(props) { - const $ = _c(3); + const $ = _c(5); let a_DEBUG; let t0; - if ($[0] !== props) { + if ($[0] !== props.a || $[1] !== props.b || $[2] !== props.d) { t0 = Symbol.for("react.early_return_sentinel"); bb0: { a_DEBUG = []; @@ -85,12 +87,14 @@ function ComponentA(props) { a_DEBUG.push(props.d); } - $[0] = props; - $[1] = a_DEBUG; - $[2] = t0; + $[0] = props.a; + $[1] = props.b; + $[2] = props.d; + $[3] = a_DEBUG; + $[4] = t0; } else { - a_DEBUG = $[1]; - t0 = $[2]; + a_DEBUG = $[3]; + t0 = $[4]; } if (t0 !== Symbol.for("react.early_return_sentinel")) { return t0; @@ -102,9 +106,14 @@ function ComponentA(props) { * props.b *does* influence `a` */ function ComponentB(props) { - const $ = _c(2); + const $ = _c(5); let a; - if ($[0] !== props) { + if ( + $[0] !== props.a || + $[1] !== props.b || + $[2] !== props.c || + $[3] !== props.d + ) { a = []; a.push(props.a); if (props.b) { @@ -112,10 +121,13 @@ function ComponentB(props) { } a.push(props.d); - $[0] = props; - $[1] = a; + $[0] = props.a; + $[1] = props.b; + $[2] = props.c; + $[3] = props.d; + $[4] = a; } else { - a = $[1]; + a = $[4]; } return a; } @@ -124,10 +136,15 @@ function ComponentB(props) { * props.b *does* influence `a`, but only in a way that is never observable */ function ComponentC(props) { - const $ = _c(3); + const $ = _c(6); let a; let t0; - if ($[0] !== props) { + if ( + $[0] !== props.a || + $[1] !== props.b || + $[2] !== props.c || + $[3] !== props.d + ) { t0 = Symbol.for("react.early_return_sentinel"); bb0: { a = []; @@ -140,12 +157,15 @@ function ComponentC(props) { a.push(props.d); } - $[0] = props; - $[1] = a; - $[2] = t0; + $[0] = props.a; + $[1] = props.b; + $[2] = props.c; + $[3] = props.d; + $[4] = a; + $[5] = t0; } else { - a = $[1]; - t0 = $[2]; + a = $[4]; + t0 = $[5]; } if (t0 !== Symbol.for("react.early_return_sentinel")) { return t0; @@ -157,10 +177,15 @@ function ComponentC(props) { * props.b *does* influence `a` */ function ComponentD(props) { - const $ = _c(3); + const $ = _c(6); let a; let t0; - if ($[0] !== props) { + if ( + $[0] !== props.a || + $[1] !== props.b || + $[2] !== props.c || + $[3] !== props.d + ) { t0 = Symbol.for("react.early_return_sentinel"); bb0: { a = []; @@ -173,12 +198,15 @@ function ComponentD(props) { a.push(props.d); } - $[0] = props; - $[1] = a; - $[2] = t0; + $[0] = props.a; + $[1] = props.b; + $[2] = props.c; + $[3] = props.d; + $[4] = a; + $[5] = t0; } else { - a = $[1]; - t0 = $[2]; + a = $[4]; + t0 = $[5]; } if (t0 !== Symbol.for("react.early_return_sentinel")) { return t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.js index e0ed101409252..02edefc77f571 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-early-return.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR /** * props.b does *not* influence `a` */ diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.expect.md index 24c565b088d2f..17ff728051ddb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR function ComponentA(props) { const a = []; const b = []; @@ -34,11 +35,11 @@ function mayMutate() {} ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function ComponentA(props) { - const $ = _c(2); + const $ = _c(4); let t0; - if ($[0] !== props) { + if ($[0] !== props.p0 || $[1] !== props.p1 || $[2] !== props.p2) { const a = []; const b = []; if (b) { @@ -49,18 +50,20 @@ function ComponentA(props) { } t0 = ; - $[0] = props; - $[1] = t0; + $[0] = props.p0; + $[1] = props.p1; + $[2] = props.p2; + $[3] = t0; } else { - t0 = $[1]; + t0 = $[3]; } return t0; } function ComponentB(props) { - const $ = _c(2); + const $ = _c(4); let t0; - if ($[0] !== props) { + if ($[0] !== props.p0 || $[1] !== props.p1 || $[2] !== props.p2) { const a = []; const b = []; if (mayMutate(b)) { @@ -71,10 +74,12 @@ function ComponentB(props) { } t0 = ; - $[0] = props; - $[1] = t0; + $[0] = props.p0; + $[1] = props.p1; + $[2] = props.p2; + $[3] = t0; } else { - t0 = $[1]; + t0 = $[3]; } return t0; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.js index 0378ab655c1ef..d03e36aaab56c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/conditional-on-mutable.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR function ComponentA(props) { const a = []; const b = []; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.expect.md index 2d33981f73fd1..476e1c017c655 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR function Component(props) { let x = []; if (props.cond) { @@ -29,11 +30,11 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { - const $ = _c(5); + const $ = _c(7); let t0; - if ($[0] !== props) { + if ($[0] !== props.cond || $[1] !== props.a || $[2] !== props.b) { t0 = Symbol.for("react.early_return_sentinel"); bb0: { const x = []; @@ -41,12 +42,12 @@ function Component(props) { x.push(props.a); if (props.b) { let t1; - if ($[2] !== props.b) { + if ($[4] !== props.b) { t1 = [props.b]; - $[2] = props.b; - $[3] = t1; + $[4] = props.b; + $[5] = t1; } else { - t1 = $[3]; + t1 = $[5]; } const y = t1; x.push(y); @@ -58,20 +59,22 @@ function Component(props) { break bb0; } else { let t1; - if ($[4] === Symbol.for("react.memo_cache_sentinel")) { + if ($[6] === Symbol.for("react.memo_cache_sentinel")) { t1 = foo(); - $[4] = t1; + $[6] = t1; } else { - t1 = $[4]; + t1 = $[6]; } t0 = t1; break bb0; } } - $[0] = props; - $[1] = t0; + $[0] = props.cond; + $[1] = props.a; + $[2] = props.b; + $[3] = t0; } else { - t0 = $[1]; + t0 = $[3]; } if (t0 !== Symbol.for("react.early_return_sentinel")) { return t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.js index 53eb06bc591e4..c8c24172d79e6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-nested-early-return-within-reactive-scope.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR function Component(props) { let x = []; if (props.cond) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.expect.md index 6c3525e9e77eb..bf54b52b59289 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR import {makeArray} from 'shared-runtime'; function Component(props) { @@ -41,13 +42,13 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR import { makeArray } from "shared-runtime"; function Component(props) { - const $ = _c(4); + const $ = _c(6); let t0; - if ($[0] !== props) { + if ($[0] !== props.cond || $[1] !== props.a || $[2] !== props.b) { t0 = Symbol.for("react.early_return_sentinel"); bb0: { const x = []; @@ -57,21 +58,23 @@ function Component(props) { break bb0; } else { let t1; - if ($[2] !== props.b) { + if ($[4] !== props.b) { t1 = makeArray(props.b); - $[2] = props.b; - $[3] = t1; + $[4] = props.b; + $[5] = t1; } else { - t1 = $[3]; + t1 = $[5]; } t0 = t1; break bb0; } } - $[0] = props; - $[1] = t0; + $[0] = props.cond; + $[1] = props.a; + $[2] = props.b; + $[3] = t0; } else { - t0 = $[1]; + t0 = $[3]; } if (t0 !== Symbol.for("react.early_return_sentinel")) { return t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.js index 7446d76fb320c..256eb46bc9280 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/early-return-within-reactive-scope.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR import {makeArray} from 'shared-runtime'; function Component(props) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md index 75c5d61d407f0..8b52187920cbe 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR function useFoo(props: {value: {x: string; y: string} | null}) { const value = props.value; return createArray(value?.x, value?.y)?.join(', '); @@ -22,13 +23,13 @@ export const FIXTURE_ENTRYPONT = { ## Error ``` - 1 | function useFoo(props: {value: {x: string; y: string} | null}) { - 2 | const value = props.value; -> 3 | return createArray(value?.x, value?.y)?.join(', '); - | ^^^^^^^^ Todo: Unexpected terminal kind `optional` for optional test block (3:3) - 4 | } - 5 | - 6 | function createArray(...args: Array): Array { + 2 | function useFoo(props: {value: {x: string; y: string} | null}) { + 3 | const value = props.value; +> 4 | return createArray(value?.x, value?.y)?.join(', '); + | ^^^^^^^^ Todo: Unexpected terminal kind `optional` for optional test block (4:4) + 5 | } + 6 | + 7 | function createArray(...args: Array): Array { ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.ts index 7ee50e0380288..0031bc770c678 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.ts @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR function useFoo(props: {value: {x: string; y: string} | null}) { const value = props.value; return createArray(value?.x, value?.y)?.join(', '); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.expect.md index bed1c329f0d10..744824d0bd2c3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR function Component(props) { const items = (() => { if (props.cond) { @@ -24,11 +25,11 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { - const $ = _c(2); + const $ = _c(3); let items; - if ($[0] !== props) { + if ($[0] !== props.cond || $[1] !== props.a) { let t0; if (props.cond) { t0 = []; @@ -38,10 +39,11 @@ function Component(props) { items = t0; items?.push(props.a); - $[0] = props; - $[1] = items; + $[0] = props.cond; + $[1] = props.a; + $[2] = items; } else { - items = $[1]; + items = $[2]; } return items; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.js index f4f953d294e6f..4e8eb097de8a7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/iife-return-modified-later-phi.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR function Component(props) { const items = (() => { if (props.cond) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.expect.md index 8a20f9186b447..142fc9cefe64c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR import {makeObject_Primitives} from 'shared-runtime'; function Component(props) { @@ -25,13 +26,13 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR import { makeObject_Primitives } from "shared-runtime"; function Component(props) { - const $ = _c(2); + const $ = _c(3); let t0; - if ($[0] !== props) { + if ($[0] !== props.cond || $[1] !== props.value) { t0 = Symbol.for("react.early_return_sentinel"); bb0: { const object = makeObject_Primitives(); @@ -45,10 +46,11 @@ function Component(props) { break bb0; } } - $[0] = props; - $[1] = t0; + $[0] = props.cond; + $[1] = props.value; + $[2] = t0; } else { - t0 = $[1]; + t0 = $[2]; } if (t0 !== Symbol.for("react.early_return_sentinel")) { return t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.js index 94a142d033708..2386448adfbbc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/object-mutated-in-consequent-alternate-both-return.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR import {makeObject_Primitives} from 'shared-runtime'; function Component(props) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.expect.md index 398161f0c6a24..324eb714fc89b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR function Component(props) { let x = []; let y = null; @@ -28,12 +29,12 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { - const $ = _c(4); + const $ = _c(6); let y; let t0; - if ($[0] !== props) { + if ($[0] !== props.cond || $[1] !== props.a || $[2] !== props.b) { t0 = Symbol.for("react.early_return_sentinel"); bb0: { const x = []; @@ -43,11 +44,11 @@ function Component(props) { break bb0; } else { let t1; - if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + if ($[5] === Symbol.for("react.memo_cache_sentinel")) { t1 = foo(); - $[3] = t1; + $[5] = t1; } else { - t1 = $[3]; + t1 = $[5]; } y = t1; if (props.b) { @@ -56,12 +57,14 @@ function Component(props) { } } } - $[0] = props; - $[1] = y; - $[2] = t0; + $[0] = props.cond; + $[1] = props.a; + $[2] = props.b; + $[3] = y; + $[4] = t0; } else { - y = $[1]; - t0 = $[2]; + y = $[3]; + t0 = $[4]; } if (t0 !== Symbol.for("react.early_return_sentinel")) { return t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.js index 66d68242b8bb6..d54f650c36bbe 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/partial-early-return-within-reactive-scope.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR function Component(props) { let x = []; let y = null; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.expect.md index f17bcc92cb7ce..4ae80006678c6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR import {makeArray} from 'shared-runtime'; function Component(props) { @@ -45,11 +46,11 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR import { makeArray } from "shared-runtime"; function Component(props) { - const $ = _c(3); + const $ = _c(6); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = {}; @@ -59,7 +60,12 @@ function Component(props) { } const x = t0; let t1; - if ($[1] !== props) { + if ( + $[1] !== props.cond || + $[2] !== props.cond2 || + $[3] !== props.value || + $[4] !== props.value2 + ) { let y; if (props.cond) { if (props.cond2) { @@ -74,10 +80,13 @@ function Component(props) { y.push(x); t1 = [x, y]; - $[1] = props; - $[2] = t1; + $[1] = props.cond; + $[2] = props.cond2; + $[3] = props.value; + $[4] = props.value2; + $[5] = t1; } else { - t1 = $[2]; + t1 = $[5]; } return t1; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.js index 0a9aa39defea4..9173848b49b90 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push-consecutive-phis.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR import {makeArray} from 'shared-runtime'; function Component(props) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.expect.md index f58eed10fda5a..0b756a648ec23 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR function Component(props) { const x = {}; let y; @@ -34,9 +35,9 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { - const $ = _c(3); + const $ = _c(4); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = {}; @@ -46,7 +47,7 @@ function Component(props) { } const x = t0; let t1; - if ($[1] !== props) { + if ($[1] !== props.cond || $[2] !== props.value) { let y; if (props.cond) { y = [props.value]; @@ -57,10 +58,11 @@ function Component(props) { y.push(x); t1 = [x, y]; - $[1] = props; - $[2] = t1; + $[1] = props.cond; + $[2] = props.value; + $[3] = t1; } else { - t1 = $[2]; + t1 = $[3]; } return t1; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.js index 732a1524cba2d..0b60f4e44315d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-array-push.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR function Component(props) { const x = {}; let y; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.expect.md index 70551c8e9d7b0..1bfaeb1c84a57 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.expect.md @@ -2,7 +2,7 @@ ## Input ```javascript -// @debug +// @debug @enablePropagateDepsInHIR function Component(props) { const x = {}; let y; @@ -30,9 +30,9 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; // @debug +import { c as _c } from "react/compiler-runtime"; // @debug @enablePropagateDepsInHIR function Component(props) { - const $ = _c(3); + const $ = _c(4); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = {}; @@ -42,7 +42,7 @@ function Component(props) { } const x = t0; let t1; - if ($[1] !== props) { + if ($[1] !== props.cond || $[2] !== props.a) { let y; if (props.cond) { y = {}; @@ -53,10 +53,11 @@ function Component(props) { y.x = x; t1 = [x, y]; - $[1] = props; - $[2] = t1; + $[1] = props.cond; + $[2] = props.a; + $[3] = t1; } else { - t1 = $[2]; + t1 = $[3]; } return t1; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.js index ba7133ff808df..3611da08d2fbd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/phi-type-inference-property-store.js @@ -1,4 +1,4 @@ -// @debug +// @debug @enablePropagateDepsInHIR function Component(props) { const x = {}; let y; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md index 6574bd1966ed9..6db3983d10751 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR function Component(props) { return props.post.feedback.comments?.edges?.map(render); } @@ -11,7 +12,7 @@ function Component(props) { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { const $ = _c(2); let t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.js index e8e0da392e9e7..58ad2a210fab5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR function Component(props) { return props.post.feedback.comments?.edges?.map(render); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.expect.md index f4689e5795552..1cb3cf2474b6a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enablePropagateDepsInHIR import {mutate} from 'shared-runtime'; function useFoo(props) { @@ -35,13 +36,13 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR import { mutate } from "shared-runtime"; function useFoo(props) { - const $ = _c(2); + const $ = _c(4); let x; - if ($[0] !== props) { + if ($[0] !== props.bar || $[1] !== props.cond || $[2] !== props.foo) { x = []; x.push(props.bar); if (props.cond) { @@ -53,10 +54,12 @@ function useFoo(props) { } mutate(x); - $[0] = props; - $[1] = x; + $[0] = props.bar; + $[1] = props.cond; + $[2] = props.foo; + $[3] = x; } else { - x = $[1]; + x = $[3]; } return x; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.js index 3e7078cfc7999..99e28942268bd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/ssa-renaming-unconditional-with-mutation.js @@ -1,3 +1,4 @@ +// @enablePropagateDepsInHIR import {mutate} from 'shared-runtime'; function useFoo(props) { From 8c89fa76430b3d9fbecd6d535c93171d22f0377f Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 30 Sep 2024 12:24:20 -0400 Subject: [PATCH 04/13] [compiler][hir-rewrite] Infer non-null props, destructure source Followup from #30894. This adds a new flagged mode `enablePropagateScopeDepsInHIR: "enabled_with_optimizations"`, under which we infer more hoistable loads: - it's always safe to evaluate loads from `props` (i.e. first parameter of a `component`) - destructuring sources are safe to evaluate loads from (e.g. given `{x} = obj`, we infer that it's safe to evaluate obj.y) - computed load sources are safe to evaluate loads from (e.g. given `arr[0]`, we can infer that it's safe to evaluate arr.length) ghstack-source-id: 32f3bb72e9f85922825579bd785d636f4ccf724d Pull Request resolved: https://github.com/facebook/react/pull/31033 --- .../src/HIR/CollectHoistablePropertyLoads.ts | 139 ++++++++++++------ .../infer-component-props-non-null.expect.md | 60 ++++++++ .../infer-component-props-non-null.tsx | 20 +++ .../infer-non-null-destructure.expect.md | 91 ++++++++++++ .../infer-non-null-destructure.ts | 23 +++ 5 files changed, 289 insertions(+), 44 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-component-props-non-null.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-component-props-non-null.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-non-null-destructure.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-non-null-destructure.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts index 941c60dea9d4f..ff50113b6660c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -8,6 +8,7 @@ import { HIRFunction, Identifier, IdentifierId, + InstructionId, Place, ReactiveScopeDependency, ScopeId, @@ -66,7 +67,7 @@ export function collectHoistablePropertyLoads( fn: HIRFunction, temporaries: ReadonlyMap, ): ReadonlyMap { - const nodes = collectPropertyLoadsInBlocks(fn, temporaries); + const nodes = collectNonNullsInBlocks(fn, temporaries); propagateNonNull(fn, nodes); const nodesKeyedByScopeId = new Map(); @@ -165,7 +166,7 @@ type PropertyLoadNode = class Tree { roots: Map = new Map(); - #getOrCreateRoot(identifier: Identifier): PropertyLoadNode { + getOrCreateRoot(identifier: Identifier): PropertyLoadNode { /** * Reads from a statically scoped variable are always safe in JS, * with the exception of TDZ (not addressed by this pass). @@ -207,17 +208,15 @@ class Tree { } getPropertyLoadNode(n: ReactiveScopeDependency): PropertyLoadNode { - CompilerError.invariant(n.path.length > 0, { - reason: - '[CollectHoistablePropertyLoads] Expected property node, found root node', - loc: GeneratedSource, - }); /** * We add ReactiveScopeDependencies according to instruction ordering, * so all subpaths of a PropertyLoad should already exist * (e.g. a.b is added before a.b.c), */ - let currNode = this.#getOrCreateRoot(n.identifier); + let currNode = this.getOrCreateRoot(n.identifier); + if (n.path.length === 0) { + return currNode; + } for (let i = 0; i < n.path.length - 1; i++) { currNode = assertNonNull(currNode.properties.get(n.path[i].property)); } @@ -226,10 +225,44 @@ class Tree { } } -function collectPropertyLoadsInBlocks( +function pushPropertyLoadNode( + loadSource: Identifier, + loadSourceNode: PropertyLoadNode, + instrId: InstructionId, + knownImmutableIdentifiers: Set, + result: Set, +): void { + /** + * Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges + * are not valid with respect to current instruction id numbering. + * We use attached reactive scope ranges as a proxy for mutable range, but this + * is an overestimate as (1) scope ranges merge and align to form valid program + * blocks and (2) passes like MemoizeFbtAndMacroOperands may assign scopes to + * non-mutable identifiers. + * + * See comment at top of function for why we track known immutable identifiers. + */ + const isMutableAtInstr = + loadSource.mutableRange.end > loadSource.mutableRange.start + 1 && + loadSource.scope != null && + inRange({id: instrId}, loadSource.scope.range); + if ( + !isMutableAtInstr || + knownImmutableIdentifiers.has(loadSourceNode.fullPath.identifier.id) + ) { + let curr: PropertyLoadNode | null = loadSourceNode; + while (curr != null) { + result.add(curr); + curr = curr.parent; + } + } +} + +function collectNonNullsInBlocks( fn: HIRFunction, temporaries: ReadonlyMap, ): ReadonlyMap { + const tree = new Tree(); /** * Due to current limitations of mutable range inference, there are edge cases in * which we infer known-immutable values (e.g. props or hook params) to have a @@ -238,53 +271,70 @@ function collectPropertyLoadsInBlocks( * We track known immutable identifiers to reduce regressions (as PropagateScopeDeps * is being rewritten to HIR). */ - const knownImmutableIdentifiers = new Set(); + const knownImmutableIdentifiers = new Set(); if (fn.fnType === 'Component' || fn.fnType === 'Hook') { for (const p of fn.params) { if (p.kind === 'Identifier') { - knownImmutableIdentifiers.add(p.identifier); + knownImmutableIdentifiers.add(p.identifier.id); } } } - const tree = new Tree(); + /** + * Known non-null objects such as functional component props can be safely + * read from any block. + */ + const knownNonNullIdentifiers = new Set(); + if ( + fn.fnType === 'Component' && + fn.params.length > 0 && + fn.params[0].kind === 'Identifier' + ) { + const identifier = fn.params[0].identifier; + knownNonNullIdentifiers.add(tree.getOrCreateRoot(identifier)); + } const nodes = new Map(); for (const [_, block] of fn.body.blocks) { - const assumedNonNullObjects = new Set(); + const assumedNonNullObjects = new Set( + knownNonNullIdentifiers, + ); for (const instr of block.instructions) { if (instr.value.kind === 'PropertyLoad') { - const property = getProperty( - instr.value.object, - instr.value.property, - temporaries, + const source = temporaries.get(instr.value.object.identifier.id) ?? { + identifier: instr.value.object.identifier, + path: [], + }; + pushPropertyLoadNode( + instr.value.object.identifier, + tree.getPropertyLoadNode(source), + instr.id, + knownImmutableIdentifiers, + assumedNonNullObjects, ); - const propertyNode = tree.getPropertyLoadNode(property); - const object = instr.value.object.identifier; - /** - * Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges - * are not valid with respect to current instruction id numbering. - * We use attached reactive scope ranges as a proxy for mutable range, but this - * is an overestimate as (1) scope ranges merge and align to form valid program - * blocks and (2) passes like MemoizeFbtAndMacroOperands may assign scopes to - * non-mutable identifiers. - * - * See comment at top of function for why we track known immutable identifiers. - */ - const isMutableAtInstr = - object.mutableRange.end > object.mutableRange.start + 1 && - object.scope != null && - inRange(instr, object.scope.range); - if ( - !isMutableAtInstr || - knownImmutableIdentifiers.has(propertyNode.fullPath.identifier) - ) { - let curr = propertyNode.parent; - while (curr != null) { - assumedNonNullObjects.add(curr); - curr = curr.parent; - } + } else if (instr.value.kind === 'Destructure') { + const source = instr.value.value.identifier.id; + const sourceNode = temporaries.get(source); + if (sourceNode != null) { + pushPropertyLoadNode( + instr.value.value.identifier, + tree.getPropertyLoadNode(sourceNode), + instr.id, + knownImmutableIdentifiers, + assumedNonNullObjects, + ); + } + } else if (instr.value.kind === 'ComputedLoad') { + const source = instr.value.object.identifier.id; + const sourceNode = temporaries.get(source); + if (sourceNode != null) { + pushPropertyLoadNode( + instr.value.object.identifier, + tree.getPropertyLoadNode(sourceNode), + instr.id, + knownImmutableIdentifiers, + assumedNonNullObjects, + ); } } - // TODO handle destructuring } nodes.set(block.id, { @@ -449,10 +499,11 @@ function propagateNonNull( ); for (const [id, node] of nodes) { - node.assumedNonNullObjects = Set_union( + const assumedNonNullObjects = Set_union( assertNonNull(fromEntry.get(id)), assertNonNull(fromExit.get(id)), ); + node.assumedNonNullObjects = assumedNonNullObjects; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-component-props-non-null.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-component-props-non-null.expect.md new file mode 100644 index 0000000000000..da9ab95fb6b53 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-component-props-non-null.expect.md @@ -0,0 +1,60 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR +import {identity, Stringify} from 'shared-runtime'; + +function Foo(props) { + /** + * props.value should be inferred as the dependency of this scope + * since we know that props is safe to read from (i.e. non-null) + * as it is arg[0] of a component function + */ + const arr = []; + if (cond) { + arr.push(identity(props.value)); + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{value: 2}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR +import { identity, Stringify } from "shared-runtime"; + +function Foo(props) { + const $ = _c(2); + let t0; + if ($[0] !== props.value) { + const arr = []; + if (cond) { + arr.push(identity(props.value)); + } + + t0 = ; + $[0] = props.value; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ value: 2 }], +}; + +``` + +### Eval output +(kind: exception) cond is not defined \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-component-props-non-null.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-component-props-non-null.tsx new file mode 100644 index 0000000000000..d5aa3534d4dc3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-component-props-non-null.tsx @@ -0,0 +1,20 @@ +// @enablePropagateDepsInHIR +import {identity, Stringify} from 'shared-runtime'; + +function Foo(props) { + /** + * props.value should be inferred as the dependency of this scope + * since we know that props is safe to read from (i.e. non-null) + * as it is arg[0] of a component function + */ + const arr = []; + if (cond) { + arr.push(identity(props.value)); + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{value: 2}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-non-null-destructure.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-non-null-destructure.expect.md new file mode 100644 index 0000000000000..faed26bc8aef4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-non-null-destructure.expect.md @@ -0,0 +1,91 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR +import {identity, useIdentity} from 'shared-runtime'; + +function useFoo({arg, cond}: {arg: number; cond: boolean}) { + const maybeObj = useIdentity({value: arg}); + const {value} = maybeObj; + useIdentity(null); + /** + * maybeObj.value should be inferred as the dependency of this scope + * since we know that maybeObj is safe to read from (i.e. non-null) + * due to the above destructuring instruction + */ + const arr = []; + if (cond) { + arr.push(identity(maybeObj.value)); + } + return {arr, value}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{arg: 2, cond: false}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR +import { identity, useIdentity } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(10); + const { arg, cond } = t0; + let t1; + if ($[0] !== arg) { + t1 = { value: arg }; + $[0] = arg; + $[1] = t1; + } else { + t1 = $[1]; + } + const maybeObj = useIdentity(t1); + const { value } = maybeObj; + useIdentity(null); + let arr; + if ($[2] !== cond || $[3] !== maybeObj.value) { + arr = []; + if (cond) { + let t2; + if ($[5] !== maybeObj.value) { + t2 = identity(maybeObj.value); + $[5] = maybeObj.value; + $[6] = t2; + } else { + t2 = $[6]; + } + arr.push(t2); + } + $[2] = cond; + $[3] = maybeObj.value; + $[4] = arr; + } else { + arr = $[4]; + } + let t2; + if ($[7] !== arr || $[8] !== value) { + t2 = { arr, value }; + $[7] = arr; + $[8] = value; + $[9] = t2; + } else { + t2 = $[9]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ arg: 2, cond: false }], +}; + +``` + +### Eval output +(kind: ok) {"arr":[],"value":2} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-non-null-destructure.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-non-null-destructure.ts new file mode 100644 index 0000000000000..f67991df7254e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-non-null-destructure.ts @@ -0,0 +1,23 @@ +// @enablePropagateDepsInHIR +import {identity, useIdentity} from 'shared-runtime'; + +function useFoo({arg, cond}: {arg: number; cond: boolean}) { + const maybeObj = useIdentity({value: arg}); + const {value} = maybeObj; + useIdentity(null); + /** + * maybeObj.value should be inferred as the dependency of this scope + * since we know that maybeObj is safe to read from (i.e. non-null) + * due to the above destructuring instruction + */ + const arr = []; + if (cond) { + arr.push(identity(maybeObj.value)); + } + return {arr, value}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{arg: 2, cond: false}], +}; From 58a3ca3b47f6a51cea48ea95ded26c9887baca38 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 30 Sep 2024 12:24:21 -0400 Subject: [PATCH 05/13] [compiler][hir-rewrite] Cleanup Identifier -> IdentifierId Since removing ExitSSA, Identifier and IdentifierId should mean the same thing ghstack-source-id: 076cacbe8360e716b0555088043502823f9ee72e Pull Request resolved: https://github.com/facebook/react/pull/31034 --- .../src/HIR/CollectHoistablePropertyLoads.ts | 66 ++----------------- .../src/HIR/PropagateScopeDependenciesHIR.ts | 49 +++++++++++++- 2 files changed, 53 insertions(+), 62 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts index ff50113b6660c..4f642e57a65f3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -9,7 +9,6 @@ import { Identifier, IdentifierId, InstructionId, - Place, ReactiveScopeDependency, ScopeId, } from './HIR'; @@ -88,61 +87,6 @@ export type BlockInfo = { assumedNonNullObjects: ReadonlySet; }; -export function getProperty( - object: Place, - propertyName: string, - temporaries: ReadonlyMap, -): ReactiveScopeDependency { - /* - * (1) Get the base object either from the temporary sidemap (e.g. a LoadLocal) - * or a deep copy of an existing property dependency. - * Example 1: - * $0 = LoadLocal x - * $1 = PropertyLoad $0.y - * getProperty($0, ...) -> resolvedObject = x, resolvedDependency = null - * - * Example 2: - * $0 = LoadLocal x - * $1 = PropertyLoad $0.y - * $2 = PropertyLoad $1.z - * getProperty($1, ...) -> resolvedObject = null, resolvedDependency = x.y - * - * Example 3: - * $0 = Call(...) - * $1 = PropertyLoad $0.y - * getProperty($0, ...) -> resolvedObject = null, resolvedDependency = null - */ - const resolvedDependency = temporaries.get(object.identifier.id); - - /** - * (2) Push the last PropertyLoad - * TODO(mofeiZ): understand optional chaining - */ - let property: ReactiveScopeDependency; - if (resolvedDependency == null) { - property = { - identifier: object.identifier, - path: [{property: propertyName, optional: false}], - }; - } else { - property = { - identifier: resolvedDependency.identifier, - path: [ - ...resolvedDependency.path, - {property: propertyName, optional: false}, - ], - }; - } - return property; -} - -export function resolveTemporary( - place: Place, - temporaries: ReadonlyMap, -): Identifier { - return temporaries.get(place.identifier.id) ?? place.identifier; -} - /** * Tree data structure to dedupe property loads (e.g. a.b.c) * and make computing sets intersections simpler. @@ -152,7 +96,7 @@ type RootNode = { parent: null; // Recorded to make later computations simpler fullPath: ReactiveScopeDependency; - root: Identifier; + root: IdentifierId; }; type PropertyLoadNode = @@ -164,18 +108,18 @@ type PropertyLoadNode = | RootNode; class Tree { - roots: Map = new Map(); + roots: Map = new Map(); getOrCreateRoot(identifier: Identifier): PropertyLoadNode { /** * Reads from a statically scoped variable are always safe in JS, * with the exception of TDZ (not addressed by this pass). */ - let rootNode = this.roots.get(identifier); + let rootNode = this.roots.get(identifier.id); if (rootNode === undefined) { rootNode = { - root: identifier, + root: identifier.id, properties: new Map(), fullPath: { identifier, @@ -183,7 +127,7 @@ class Tree { }, parent: null, }; - this.roots.set(identifier, rootNode); + this.roots.set(identifier.id, rootNode); } return rootNode; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts index 4c7dac004d80a..1fe218c352818 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -20,7 +20,6 @@ import { import { BlockInfo, collectHoistablePropertyLoads, - getProperty, } from './CollectHoistablePropertyLoads'; import { ScopeBlockTraversal, @@ -220,6 +219,54 @@ function collectTemporariesSidemap( return temporaries; } +function getProperty( + object: Place, + propertyName: string, + temporaries: ReadonlyMap, +): ReactiveScopeDependency { + /* + * (1) Get the base object either from the temporary sidemap (e.g. a LoadLocal) + * or a deep copy of an existing property dependency. + * Example 1: + * $0 = LoadLocal x + * $1 = PropertyLoad $0.y + * getProperty($0, ...) -> resolvedObject = x, resolvedDependency = null + * + * Example 2: + * $0 = LoadLocal x + * $1 = PropertyLoad $0.y + * $2 = PropertyLoad $1.z + * getProperty($1, ...) -> resolvedObject = null, resolvedDependency = x.y + * + * Example 3: + * $0 = Call(...) + * $1 = PropertyLoad $0.y + * getProperty($0, ...) -> resolvedObject = null, resolvedDependency = null + */ + const resolvedDependency = temporaries.get(object.identifier.id); + + /** + * (2) Push the last PropertyLoad + * TODO(mofeiZ): understand optional chaining + */ + let property: ReactiveScopeDependency; + if (resolvedDependency == null) { + property = { + identifier: object.identifier, + path: [{property: propertyName, optional: false}], + }; + } else { + property = { + identifier: resolvedDependency.identifier, + path: [ + ...resolvedDependency.path, + {property: propertyName, optional: false}, + ], + }; + } + return property; +} + type Decl = { id: InstructionId; scope: Stack; From 5d12e9e10b9957bc131ec77e013e1a76e4f32eb6 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 30 Sep 2024 12:24:22 -0400 Subject: [PATCH 06/13] [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 0000000000000..9a95e7dc875b4 --- /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 0000000000000..bb361e3c9fefd --- /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 7140cad2f74bc..0ae22a643b326 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', From 2cbea245cca4044f02c4c231a7f86c8062074579 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 30 Sep 2024 12:24:23 -0400 Subject: [PATCH 07/13] [compiler][fixtures] Patch error-handling edge case in snap evaluator Fix edge case in which we incorrectly returned a cached exception instead of trying to rerender with new props. ghstack-source-id: 843fb85df4a2ae7a88f296104fb16b5f9a34c76e Pull Request resolved: https://github.com/facebook/react/pull/31082 --- .../throw-before-scope-starts.expect.md | 2 +- .../packages/snap/src/sprout/evaluator.ts | 45 +++++++++++++------ 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/jump-unpoisoned/throw-before-scope-starts.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/jump-unpoisoned/throw-before-scope-starts.expect.md index 4f04ef7fdc1ba..dd7509c48c4c4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/jump-unpoisoned/throw-before-scope-starts.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/jump-unpoisoned/throw-before-scope-starts.expect.md @@ -75,7 +75,7 @@ export const FIXTURE_ENTRYPOINT = { ### Eval output (kind: ok) [[ (exception in render) Error: throw with error! ]] -[[ (exception in render) Error: throw with error! ]] +[2] [[ (exception in render) Error: throw with error! ]] [[ (exception in render) TypeError: Cannot read properties of undefined (reading 'b') ]] [null] diff --git a/compiler/packages/snap/src/sprout/evaluator.ts b/compiler/packages/snap/src/sprout/evaluator.ts index 27cbe55858f60..77e8044fb5ed5 100644 --- a/compiler/packages/snap/src/sprout/evaluator.ts +++ b/compiler/packages/snap/src/sprout/evaluator.ts @@ -60,6 +60,7 @@ const ExportSchema = z.object({ FIXTURE_ENTRYPOINT: EntrypointSchema, }); +const NO_ERROR_SENTINEL = Symbol(); /** * Wraps WrapperTestComponent in an error boundary to simplify re-rendering * when an exception is thrown. @@ -67,31 +68,47 @@ const ExportSchema = z.object({ */ class WrapperTestComponentWithErrorBoundary extends React.Component< {fn: any; params: Array}, - {hasError: boolean; error: any} + {errorFromLastRender: any} > { - propsErrorMap: MutableRefObject>; + /** + * Limit retries of the child component by caching seen errors. + */ + propsErrorMap: Map; + lastProps: any | null; + // lastProps: object | null; constructor(props: any) { super(props); - this.state = {hasError: false, error: null}; - this.propsErrorMap = React.createRef() as MutableRefObject>; - this.propsErrorMap.current = new Map(); + this.lastProps = null; + this.propsErrorMap = new Map(); + this.state = { + errorFromLastRender: NO_ERROR_SENTINEL, + }; } static getDerivedStateFromError(error: any) { - return {hasError: true, error: error}; + // Reschedule a second render that immediately returns the cached error + return {errorFromLastRender: error}; } override componentDidUpdate() { - if (this.state.hasError) { - this.setState({hasError: false, error: null}); + if (this.state.errorFromLastRender !== NO_ERROR_SENTINEL) { + // Reschedule a third render that immediately returns the cached error + this.setState({errorFromLastRender: NO_ERROR_SENTINEL}); } } override render() { - if (this.state.hasError) { - this.propsErrorMap.current!.set( - this.props, - `[[ (exception in render) ${this.state.error?.toString()} ]]`, - ); + if ( + this.state.errorFromLastRender !== NO_ERROR_SENTINEL && + this.props === this.lastProps + ) { + /** + * The last render errored, cache the error message to avoid running the + * test fixture more than once + */ + const errorMsg = `[[ (exception in render) ${this.state.errorFromLastRender?.toString()} ]]`; + this.propsErrorMap.set(this.lastProps, errorMsg); + return errorMsg; } - const cachedError = this.propsErrorMap.current!.get(this.props); + this.lastProps = this.props; + const cachedError = this.propsErrorMap.get(this.props); if (cachedError != null) { return cachedError; } From c67e241c1656dea4ece22a4ee5c25b6b36d0ca75 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 30 Sep 2024 12:24:24 -0400 Subject: [PATCH 08/13] [compiler] Renames and no-op refactor for next PR Rename for clarity: - `CollectHoistablePropertyLoads:Tree` -> `CollectHoistablePropertyLoads:PropertyPathRegistry` - `getPropertyLoadNode` -> `getOrCreateProperty` - `getOrCreateRoot` -> `getOrCreateIdentifier` - `PropertyLoadNode` -> `PropertyPathNode` Refactor to CFG joining logic for `CollectHoistablePropertyLoads`. We now write to the same set of inferredNonNullObjects when traversing from entry and exit blocks. This is more correct, as non-nulls inferred from a forward traversal should be included when computing the backward traversal (and vice versa). This fix is needed by an edge case in #31036 Added invariant into fixed-point iteration to terminate (instead of infinite looping). ghstack-source-id: 1e8eb2d566b649ede93de9a9c13dad09b96416a5 Pull Request resolved: https://github.com/facebook/react/pull/31036 --- .../src/HIR/CollectHoistablePropertyLoads.ts | 160 ++++++++---------- .../src/HIR/DeriveMinimalDependenciesHIR.ts | 25 ++- .../src/HIR/HIR.ts | 18 +- 3 files changed, 95 insertions(+), 108 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts index 4f642e57a65f3..cb778c329226b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -4,6 +4,7 @@ import {Set_intersect, Set_union, getOrInsertDefault} from '../Utils/utils'; import { BasicBlock, BlockId, + DependencyPathEntry, GeneratedSource, HIRFunction, Identifier, @@ -66,7 +67,9 @@ export function collectHoistablePropertyLoads( fn: HIRFunction, temporaries: ReadonlyMap, ): ReadonlyMap { - const nodes = collectNonNullsInBlocks(fn, temporaries); + const registry = new PropertyPathRegistry(); + + const nodes = collectNonNullsInBlocks(fn, temporaries, registry); propagateNonNull(fn, nodes); const nodesKeyedByScopeId = new Map(); @@ -84,33 +87,33 @@ export function collectHoistablePropertyLoads( export type BlockInfo = { block: BasicBlock; - assumedNonNullObjects: ReadonlySet; + assumedNonNullObjects: ReadonlySet; }; /** - * Tree data structure to dedupe property loads (e.g. a.b.c) + * PropertyLoadRegistry data structure to dedupe property loads (e.g. a.b.c) * and make computing sets intersections simpler. */ type RootNode = { - properties: Map; + properties: Map; parent: null; // Recorded to make later computations simpler fullPath: ReactiveScopeDependency; root: IdentifierId; }; -type PropertyLoadNode = +type PropertyPathNode = | { - properties: Map; - parent: PropertyLoadNode; + properties: Map; + parent: PropertyPathNode; fullPath: ReactiveScopeDependency; } | RootNode; -class Tree { +class PropertyPathRegistry { roots: Map = new Map(); - getOrCreateRoot(identifier: Identifier): PropertyLoadNode { + getOrCreateIdentifier(identifier: Identifier): PropertyPathNode { /** * Reads from a statically scoped variable are always safe in JS, * with the exception of TDZ (not addressed by this pass). @@ -132,49 +135,61 @@ class Tree { return rootNode; } - static #getOrCreateProperty( - node: PropertyLoadNode, - property: string, - ): PropertyLoadNode { - let child = node.properties.get(property); + static getOrCreatePropertyEntry( + parent: PropertyPathNode, + entry: DependencyPathEntry, + ): PropertyPathNode { + if (entry.optional) { + CompilerError.throwTodo({ + reason: 'handle optional nodes', + loc: GeneratedSource, + }); + } + let child = parent.properties.get(entry.property); if (child == null) { child = { properties: new Map(), - parent: node, + parent: parent, fullPath: { - identifier: node.fullPath.identifier, - path: node.fullPath.path.concat([{property, optional: false}]), + identifier: parent.fullPath.identifier, + path: parent.fullPath.path.concat(entry), }, }; - node.properties.set(property, child); + parent.properties.set(entry.property, child); } return child; } - getPropertyLoadNode(n: ReactiveScopeDependency): PropertyLoadNode { + getOrCreateProperty(n: ReactiveScopeDependency): PropertyPathNode { /** * We add ReactiveScopeDependencies according to instruction ordering, * so all subpaths of a PropertyLoad should already exist * (e.g. a.b is added before a.b.c), */ - let currNode = this.getOrCreateRoot(n.identifier); + let currNode = this.getOrCreateIdentifier(n.identifier); if (n.path.length === 0) { return currNode; } for (let i = 0; i < n.path.length - 1; i++) { - currNode = assertNonNull(currNode.properties.get(n.path[i].property)); + currNode = PropertyPathRegistry.getOrCreatePropertyEntry( + currNode, + n.path[i], + ); } - return Tree.#getOrCreateProperty(currNode, n.path.at(-1)!.property); + return PropertyPathRegistry.getOrCreatePropertyEntry( + currNode, + n.path.at(-1)!, + ); } } -function pushPropertyLoadNode( - loadSource: Identifier, - loadSourceNode: PropertyLoadNode, +function addNonNullPropertyPath( + source: Identifier, + sourceNode: PropertyPathNode, instrId: InstructionId, knownImmutableIdentifiers: Set, - result: Set, + result: Set, ): void { /** * Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges @@ -187,26 +202,22 @@ function pushPropertyLoadNode( * See comment at top of function for why we track known immutable identifiers. */ const isMutableAtInstr = - loadSource.mutableRange.end > loadSource.mutableRange.start + 1 && - loadSource.scope != null && - inRange({id: instrId}, loadSource.scope.range); + source.mutableRange.end > source.mutableRange.start + 1 && + source.scope != null && + inRange({id: instrId}, source.scope.range); if ( !isMutableAtInstr || - knownImmutableIdentifiers.has(loadSourceNode.fullPath.identifier.id) + knownImmutableIdentifiers.has(sourceNode.fullPath.identifier.id) ) { - let curr: PropertyLoadNode | null = loadSourceNode; - while (curr != null) { - result.add(curr); - curr = curr.parent; - } + result.add(sourceNode); } } function collectNonNullsInBlocks( fn: HIRFunction, temporaries: ReadonlyMap, + registry: PropertyPathRegistry, ): ReadonlyMap { - const tree = new Tree(); /** * Due to current limitations of mutable range inference, there are edge cases in * which we infer known-immutable values (e.g. props or hook params) to have a @@ -227,18 +238,18 @@ function collectNonNullsInBlocks( * Known non-null objects such as functional component props can be safely * read from any block. */ - const knownNonNullIdentifiers = new Set(); + const knownNonNullIdentifiers = new Set(); if ( fn.fnType === 'Component' && fn.params.length > 0 && fn.params[0].kind === 'Identifier' ) { const identifier = fn.params[0].identifier; - knownNonNullIdentifiers.add(tree.getOrCreateRoot(identifier)); + knownNonNullIdentifiers.add(registry.getOrCreateIdentifier(identifier)); } const nodes = new Map(); for (const [_, block] of fn.body.blocks) { - const assumedNonNullObjects = new Set( + const assumedNonNullObjects = new Set( knownNonNullIdentifiers, ); for (const instr of block.instructions) { @@ -247,9 +258,9 @@ function collectNonNullsInBlocks( identifier: instr.value.object.identifier, path: [], }; - pushPropertyLoadNode( + addNonNullPropertyPath( instr.value.object.identifier, - tree.getPropertyLoadNode(source), + registry.getOrCreateProperty(source), instr.id, knownImmutableIdentifiers, assumedNonNullObjects, @@ -258,9 +269,9 @@ function collectNonNullsInBlocks( const source = instr.value.value.identifier.id; const sourceNode = temporaries.get(source); if (sourceNode != null) { - pushPropertyLoadNode( + addNonNullPropertyPath( instr.value.value.identifier, - tree.getPropertyLoadNode(sourceNode), + registry.getOrCreateProperty(sourceNode), instr.id, knownImmutableIdentifiers, assumedNonNullObjects, @@ -270,9 +281,9 @@ function collectNonNullsInBlocks( const source = instr.value.object.identifier.id; const sourceNode = temporaries.get(source); if (sourceNode != null) { - pushPropertyLoadNode( + addNonNullPropertyPath( instr.value.object.identifier, - tree.getPropertyLoadNode(sourceNode), + registry.getOrCreateProperty(sourceNode), instr.id, knownImmutableIdentifiers, assumedNonNullObjects, @@ -314,7 +325,6 @@ function propagateNonNull( nodeId: BlockId, direction: 'forward' | 'backward', traversalState: Map, - nonNullObjectsByBlock: Map>, ): boolean { /** * Avoid re-visiting computed or currently active nodes, which can @@ -345,7 +355,6 @@ function propagateNonNull( pred, direction, traversalState, - nonNullObjectsByBlock, ); changed ||= neighborChanged; } @@ -374,38 +383,36 @@ function propagateNonNull( const neighborAccesses = Set_intersect( Array.from(neighbors) .filter(n => traversalState.get(n) === 'done') - .map(n => assertNonNull(nonNullObjectsByBlock.get(n))), + .map(n => assertNonNull(nodes.get(n)).assumedNonNullObjects), ); - const prevObjects = assertNonNull(nonNullObjectsByBlock.get(nodeId)); - const newObjects = Set_union(prevObjects, neighborAccesses); + const prevObjects = assertNonNull(nodes.get(nodeId)).assumedNonNullObjects; + const mergedObjects = Set_union(prevObjects, neighborAccesses); - nonNullObjectsByBlock.set(nodeId, newObjects); + assertNonNull(nodes.get(nodeId)).assumedNonNullObjects = mergedObjects; traversalState.set(nodeId, 'done'); - changed ||= prevObjects.size !== newObjects.size; + changed ||= prevObjects.size !== mergedObjects.size; return changed; } - const fromEntry = new Map>(); - const fromExit = new Map>(); - for (const [blockId, blockInfo] of nodes) { - fromEntry.set(blockId, blockInfo.assumedNonNullObjects); - fromExit.set(blockId, blockInfo.assumedNonNullObjects); - } const traversalState = new Map(); const reversedBlocks = [...fn.body.blocks]; reversedBlocks.reverse(); - let i = 0; let changed; + let i = 0; do { - i++; + CompilerError.invariant(i++ < 100, { + reason: + '[CollectHoistablePropertyLoads] fixed point iteration did not terminate after 100 loops', + loc: GeneratedSource, + }); + changed = false; for (const [blockId] of fn.body.blocks) { const forwardChanged = recursivelyPropagateNonNull( blockId, 'forward', traversalState, - fromEntry, ); changed ||= forwardChanged; } @@ -415,43 +422,14 @@ function propagateNonNull( blockId, 'backward', traversalState, - fromExit, ); changed ||= backwardChanged; } traversalState.clear(); } while (changed); - - /** - * TODO: validate against meta internal code, then remove in future PR. - * Currently cannot come up with a case that requires fixed-point iteration. - */ - CompilerError.invariant(i <= 2, { - reason: 'require fixed-point iteration', - description: `#iterations = ${i}`, - loc: GeneratedSource, - }); - - CompilerError.invariant( - fromEntry.size === fromExit.size && fromEntry.size === nodes.size, - { - reason: - 'bad sizes after calculating fromEntry + fromExit ' + - `${fromEntry.size} ${fromExit.size} ${nodes.size}`, - loc: GeneratedSource, - }, - ); - - for (const [id, node] of nodes) { - const assumedNonNullObjects = Set_union( - assertNonNull(fromEntry.get(id)), - assertNonNull(fromExit.get(id)), - ); - node.assumedNonNullObjects = assumedNonNullObjects; - } } -function assertNonNull, U>( +export function assertNonNull, U>( value: T | null | undefined, source?: string, ): T { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts index ecc1844b006aa..f2bb0b31f05c9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts @@ -19,16 +19,17 @@ const ENABLE_DEBUG_INVARIANTS = true; export class ReactiveScopeDependencyTreeHIR { #roots: Map = new Map(); - #getOrCreateRoot(identifier: Identifier, isNonNull: boolean): DependencyNode { + #getOrCreateRoot( + identifier: Identifier, + accessType: PropertyAccessType, + ): DependencyNode { // roots can always be accessed unconditionally in JS let rootNode = this.#roots.get(identifier); if (rootNode === undefined) { rootNode = { properties: new Map(), - accessType: isNonNull - ? PropertyAccessType.NonNullAccess - : PropertyAccessType.Access, + accessType, }; this.#roots.set(identifier, rootNode); } @@ -37,7 +38,7 @@ export class ReactiveScopeDependencyTreeHIR { addDependency(dep: ReactiveScopePropertyDependency): void { const {path} = dep; - let currNode = this.#getOrCreateRoot(dep.identifier, false); + let currNode = this.#getOrCreateRoot(dep.identifier, MIN_ACCESS_TYPE); const accessType = PropertyAccessType.Access; @@ -45,8 +46,11 @@ export class ReactiveScopeDependencyTreeHIR { for (const property of path) { // all properties read 'on the way' to a dependency are marked as 'access' - let currChild = getOrMakeProperty(currNode, property.property); - currChild.accessType = merge(currChild.accessType, accessType); + let currChild = makeOrMergeProperty( + currNode, + property.property, + accessType, + ); currNode = currChild; } @@ -251,17 +255,20 @@ function printSubtree( return results; } -function getOrMakeProperty( +function makeOrMergeProperty( node: DependencyNode, property: string, + accessType: PropertyAccessType, ): DependencyNode { let child = node.properties.get(property); if (child == null) { child = { properties: new Map(), - accessType: MIN_ACCESS_TYPE, + accessType, }; node.properties.set(property, child); + } else { + child.accessType = merge(child.accessType, accessType); } return child; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 30408ab032b35..615ec18feb962 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -874,13 +874,7 @@ export type InstructionValue = }; loc: SourceLocation; } - | { - kind: 'StoreLocal'; - lvalue: LValue; - value: Place; - type: t.FlowType | t.TSType | null; - loc: SourceLocation; - } + | StoreLocal | { kind: 'StoreContext'; lvalue: { @@ -1123,6 +1117,13 @@ export type Primitive = { export type JSXText = {kind: 'JSXText'; value: string; loc: SourceLocation}; +export type StoreLocal = { + kind: 'StoreLocal'; + lvalue: LValue; + value: Place; + type: t.FlowType | t.TSType | null; + loc: SourceLocation; +}; export type PropertyLoad = { kind: 'PropertyLoad'; object: Place; @@ -1496,7 +1497,8 @@ export type ReactiveScopeDeclaration = { scope: ReactiveScope; // the scope in which the variable was originally declared }; -export type DependencyPath = Array<{property: string; optional: boolean}>; +export type DependencyPathEntry = {property: string; optional: boolean}; +export type DependencyPath = Array; export type ReactiveScopeDependency = { identifier: Identifier; path: DependencyPath; From 326832a56d41b4462919f9efe69916712ca87a95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 30 Sep 2024 12:45:13 -0700 Subject: [PATCH 09/13] [Flight] Serialize Error Values (#31104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The idea is that the RSC protocol is a superset of Structured Clone. #25687 One exception that we left out was serializing Error objects as values. We serialize "throws" or "rejections" as Error (regardless of their type) but not Error values. This fixes that by serializing `Error` objects. We don't include digest in this case since we don't call `onError` and it's not really expected that you'd log it on the server with some way to look it up. In general this is not super useful outside throws. Especially since we hide their values in prod. However, there is one case where it is quite useful. When you replay console logs in DEV you might often log an Error object within the scope of a Server Component. E.g. the default RSC error handling just console.error and error object. Before this would just be an empty object due to our lax console log serialization: Screenshot 2024-09-30 at 2 24 03 PM After: Screenshot 2024-09-30 at 2 36 48 PM TODO for a follow up: Flight Reply direction. This direction doesn't actually serialize thrown errors because they always reject the serialization. --- .../react-client/src/ReactFlightClient.js | 74 +++++++++---------- .../src/__tests__/ReactFlight-test.js | 40 ++++++++++ .../react-server/src/ReactFlightServer.js | 36 +++++++++ 3 files changed, 112 insertions(+), 38 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 83c509dab46a8..98a52c4ec4a9a 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -1287,6 +1287,21 @@ function parseModelString( createFormData, ); } + case 'Z': { + // Error + if (__DEV__) { + const ref = value.slice(2); + return getOutlinedModel( + response, + ref, + parentObject, + key, + resolveErrorDev, + ); + } else { + return resolveErrorProd(response); + } + } case 'i': { // Iterator const ref = value.slice(2); @@ -1881,11 +1896,7 @@ function formatV8Stack( } type ErrorWithDigest = Error & {digest?: string}; -function resolveErrorProd( - response: Response, - id: number, - digest: string, -): void { +function resolveErrorProd(response: Response): Error { if (__DEV__) { // These errors should never make it into a build so we don't need to encode them in codes.json // eslint-disable-next-line react-internal/prod-error-codes @@ -1899,25 +1910,17 @@ function resolveErrorProd( ' may provide additional details about the nature of the error.', ); error.stack = 'Error: ' + error.message; - (error: any).digest = digest; - const errorWithDigest: ErrorWithDigest = (error: any); - const chunks = response._chunks; - const chunk = chunks.get(id); - if (!chunk) { - chunks.set(id, createErrorChunk(response, errorWithDigest)); - } else { - triggerErrorOnChunk(chunk, errorWithDigest); - } + return error; } function resolveErrorDev( response: Response, - id: number, - digest: string, - message: string, - stack: ReactStackTrace, - env: string, -): void { + errorInfo: {message: string, stack: ReactStackTrace, env: string, ...}, +): Error { + const message: string = errorInfo.message; + const stack: ReactStackTrace = errorInfo.stack; + const env: string = errorInfo.env; + if (!__DEV__) { // These errors should never make it into a build so we don't need to encode them in codes.json // eslint-disable-next-line react-internal/prod-error-codes @@ -1957,16 +1960,8 @@ function resolveErrorDev( } } - (error: any).digest = digest; (error: any).environmentName = env; - const errorWithDigest: ErrorWithDigest = (error: any); - const chunks = response._chunks; - const chunk = chunks.get(id); - if (!chunk) { - chunks.set(id, createErrorChunk(response, errorWithDigest)); - } else { - triggerErrorOnChunk(chunk, errorWithDigest); - } + return error; } function resolvePostponeProd(response: Response, id: number): void { @@ -2622,17 +2617,20 @@ function processFullStringRow( } case 69 /* "E" */: { const errorInfo = JSON.parse(row); + let error; if (__DEV__) { - resolveErrorDev( - response, - id, - errorInfo.digest, - errorInfo.message, - errorInfo.stack, - errorInfo.env, - ); + error = resolveErrorDev(response, errorInfo); + } else { + error = resolveErrorProd(response); + } + (error: any).digest = errorInfo.digest; + const errorWithDigest: ErrorWithDigest = (error: any); + const chunks = response._chunks; + const chunk = chunks.get(id); + if (!chunk) { + chunks.set(id, createErrorChunk(response, errorWithDigest)); } else { - resolveErrorProd(response, id, errorInfo.digest); + triggerErrorOnChunk(chunk, errorWithDigest); } return; } diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 34986dc623de8..27db069ef324f 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -653,6 +653,46 @@ describe('ReactFlight', () => { `); }); + it('can transport Error objects as values', async () => { + function ComponentClient({prop}) { + return ` + is error: ${prop instanceof Error} + message: ${prop.message} + stack: ${normalizeCodeLocInfo(prop.stack).split('\n').slice(0, 2).join('\n')} + environmentName: ${prop.environmentName} + `; + } + const Component = clientReference(ComponentClient); + + function ServerComponent() { + const error = new Error('hello'); + return ; + } + + const transport = ReactNoopFlightServer.render(); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + if (__DEV__) { + expect(ReactNoop).toMatchRenderedOutput(` + is error: true + message: hello + stack: Error: hello + in ServerComponent (at **) + environmentName: Server + `); + } else { + expect(ReactNoop).toMatchRenderedOutput(` + is error: true + message: An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details. A digest property is included on this error instance which may provide additional details about the nature of the error. + stack: Error: An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details. A digest property is included on this error instance which may provide additional details about the nature of the error. + environmentName: undefined + `); + } + }); + it('can transport cyclic objects', async () => { function ComponentClient({prop}) { expect(prop.obj.obj.obj).toBe(prop.obj.obj); diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 5d79482de0186..19c40c214b918 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -2688,6 +2688,9 @@ function renderModelDestructive( if (typeof FormData === 'function' && value instanceof FormData) { return serializeFormData(request, value); } + if (value instanceof Error) { + return serializeErrorValue(request, value); + } if (enableBinaryFlight) { if (value instanceof ArrayBuffer) { @@ -3114,6 +3117,36 @@ function emitPostponeChunk( request.completedErrorChunks.push(processedChunk); } +function serializeErrorValue(request: Request, error: Error): string { + if (__DEV__) { + let message; + let stack: ReactStackTrace; + let env = (0, request.environmentName)(); + try { + // eslint-disable-next-line react-internal/safe-string-coercion + message = String(error.message); + stack = filterStackTrace(request, error, 0); + const errorEnv = (error: any).environmentName; + if (typeof errorEnv === 'string') { + // This probably came from another FlightClient as a pass through. + // Keep the environment name. + env = errorEnv; + } + } catch (x) { + message = 'An error occurred but serializing the error message failed.'; + stack = []; + } + const errorInfo = {message, stack, env}; + const id = outlineModel(request, errorInfo); + return '$Z' + id.toString(16); + } else { + // In prod we don't emit any information about this Error object to avoid + // unintentional leaks. Since this doesn't actually throw on the server + // we don't go through onError and so don't register any digest neither. + return '$Z'; + } +} + function emitErrorChunk( request: Request, id: number, @@ -3403,6 +3436,9 @@ function renderConsoleValue( if (typeof FormData === 'function' && value instanceof FormData) { return serializeFormData(request, value); } + if (value instanceof Error) { + return serializeErrorValue(request, value); + } if (enableBinaryFlight) { if (value instanceof ArrayBuffer) { From 654e387d7eac113ddbf85f8a9029d1af7117679e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 30 Sep 2024 22:39:20 -0700 Subject: [PATCH 10/13] [Flight] Serialize Server Components Props in DEV (#31105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows us to show props in React DevTools when inspecting a Server Component. I currently drastically limit the object depth that's serialized since this is very implicit and you can have heavy objects on the server. We previously was using the general outlineModel to outline ReactComponentInfo but we weren't consistently using it everywhere which could cause some bugs with the parsing when it got deduped on the client. It also lead to the weird feature detect of `isReactComponent`. It also meant that this serialization was using the plain serialization instead of `renderConsoleValue` which means we couldn't safely serialize arbitrary debug info that isn't serializable there. So the main change here is to call `outlineComponentInfo` and have that always write every "Server Component" instance as outlined and in a way that lets its props be serialized using `renderConsoleValue`. Screenshot 2024-10-01 at 1 25 05 AM --- .../react-client/src/ReactFlightClient.js | 21 +- .../src/__tests__/ReactFlight-test.js | 33 +++ .../src/backend/fiber/renderer.js | 3 +- .../react-devtools-shared/src/hydration.js | 58 ++++- .../__tests__/ReactFlightDOMBrowser-test.js | 8 +- .../react-server/src/ReactFlightServer.js | 218 +++++++++++------- packages/shared/ReactTypes.js | 1 + 7 files changed, 245 insertions(+), 97 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 98a52c4ec4a9a..2f60ccddb4b4d 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -2640,11 +2640,22 @@ function processFullStringRow( } case 68 /* "D" */: { if (__DEV__) { - const debugInfo: ReactComponentInfo | ReactAsyncInfo = parseModel( - response, - row, - ); - resolveDebugInfo(response, id, debugInfo); + const chunk: ResolvedModelChunk = + createResolvedModelChunk(response, row); + initializeModelChunk(chunk); + const initializedChunk: SomeChunk = + chunk; + if (initializedChunk.status === INITIALIZED) { + resolveDebugInfo(response, id, initializedChunk.value); + } else { + // TODO: This is not going to resolve in the right order if there's more than one. + chunk.then( + v => resolveDebugInfo(response, id, v), + e => { + // Ignore debug info errors for now. Unnecessary noise. + }, + ); + } return; } // Fallthrough to share the error with Console entries. diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 27db069ef324f..857ce99868d9b 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -308,6 +308,10 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: { + firstName: 'Seb', + lastName: 'Smith', + }, }, ] : undefined, @@ -347,6 +351,10 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: { + firstName: 'Seb', + lastName: 'Smith', + }, }, ] : undefined, @@ -2665,6 +2673,9 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: { + transport: expect.arrayContaining([]), + }, }, ] : undefined, @@ -2683,6 +2694,7 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: {}, }, ] : undefined, @@ -2698,6 +2710,7 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in myLazy (at **)\n in lazyInitializer (at **)' : undefined, + props: {}, }, ] : undefined, @@ -2713,6 +2726,7 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: {}, }, ] : undefined, @@ -2787,6 +2801,9 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: { + transport: expect.arrayContaining([]), + }, }, ] : undefined, @@ -2804,6 +2821,9 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in ServerComponent (at **)' : undefined, + props: { + children: {}, + }, }, ] : undefined, @@ -2820,6 +2840,7 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: {}, }, ] : undefined, @@ -2978,6 +2999,7 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: {}, }, { env: 'B', @@ -3108,6 +3130,9 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' : undefined, + props: { + firstName: 'Seb', + }, }; expect(getDebugInfo(greeting)).toEqual([ greetInfo, @@ -3119,6 +3144,14 @@ describe('ReactFlight', () => { stack: gate(flag => flag.enableOwnerStacks) ? ' in Greeting (at **)' : undefined, + props: { + children: expect.objectContaining({ + type: 'span', + props: { + children: ['Hello, ', 'Seb'], + }, + }), + }, }, ]); // The owner that created the span was the outer server component. diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 4fac8839ae799..9732bf105ca6c 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -4348,8 +4348,7 @@ export function attach( const componentInfo = virtualInstance.data; const key = typeof componentInfo.key === 'string' ? componentInfo.key : null; - const props = null; // TODO: Track props on ReactComponentInfo; - + const props = componentInfo.props == null ? null : componentInfo.props; const owners: null | Array = getOwnersListFromInstance(virtualInstance); diff --git a/packages/react-devtools-shared/src/hydration.js b/packages/react-devtools-shared/src/hydration.js index c5b78135e74a2..c21efe40a88fa 100644 --- a/packages/react-devtools-shared/src/hydration.js +++ b/packages/react-devtools-shared/src/hydration.js @@ -216,16 +216,19 @@ export function dehydrate( if (level >= LEVEL_THRESHOLD && !isPathAllowedCheck) { return createDehydrated(type, true, data, cleaned, path); } - return data.map((item, i) => - dehydrate( - item, + const arr: Array = []; + for (let i = 0; i < data.length; i++) { + arr[i] = dehydrateKey( + data, + i, cleaned, unserializable, path.concat([i]), isPathAllowed, isPathAllowedCheck ? 1 : level + 1, - ), - ); + ); + } + return arr; case 'html_all_collection': case 'typed_array': @@ -311,8 +314,9 @@ export function dehydrate( } = {}; getAllEnumerableKeys(data).forEach(key => { const name = key.toString(); - object[name] = dehydrate( - data[key], + object[name] = dehydrateKey( + data, + key, cleaned, unserializable, path.concat([name]), @@ -373,6 +377,46 @@ export function dehydrate( } } +function dehydrateKey( + parent: Object, + key: number | string | symbol, + cleaned: Array>, + unserializable: Array>, + path: Array, + isPathAllowed: (path: Array) => boolean, + level: number = 0, +): $PropertyType { + try { + return dehydrate( + parent[key], + cleaned, + unserializable, + path, + isPathAllowed, + level, + ); + } catch (error) { + let preview = ''; + if ( + typeof error === 'object' && + error !== null && + typeof error.stack === 'string' + ) { + preview = error.stack; + } else if (typeof error === 'string') { + preview = error; + } + cleaned.push(path); + return { + inspectable: false, + preview_short: '[Exception]', + preview_long: preview ? '[Exception: ' + preview + ']' : '[Exception]', + name: preview, + type: 'unknown', + }; + } +} + export function fillInPath( object: Object, data: DehydratedData, diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index 0408a63fc512f..882c0bbb01969 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -709,7 +709,7 @@ describe('ReactFlightDOMBrowser', () => { expect(container.innerHTML).toBe(expectedHtml); if (__DEV__) { - const resolvedPath1b = await response.value[0].props.children[1]._payload; + const resolvedPath1b = response.value[0].props.children[1]; expect(resolvedPath1b._owner).toEqual( expect.objectContaining({ @@ -1028,8 +1028,10 @@ describe('ReactFlightDOMBrowser', () => { expect(flightResponse).toContain('(loading everything)'); expect(flightResponse).toContain('(loading sidebar)'); expect(flightResponse).toContain('(loading posts)'); - expect(flightResponse).not.toContain(':friends:'); - expect(flightResponse).not.toContain(':name:'); + if (!__DEV__) { + expect(flightResponse).not.toContain(':friends:'); + expect(flightResponse).not.toContain(':name:'); + } await serverAct(() => { resolveFriends(); diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 19c40c214b918..5db03d628146f 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -1148,14 +1148,20 @@ function renderFunctionComponent( ? null : filterStackTrace(request, task.debugStack, 1); // $FlowFixMe[cannot-write] + componentDebugInfo.props = props; + // $FlowFixMe[cannot-write] componentDebugInfo.debugStack = task.debugStack; // $FlowFixMe[cannot-write] componentDebugInfo.debugTask = task.debugTask; + } else { + // $FlowFixMe[cannot-write] + componentDebugInfo.props = props; } // We outline this model eagerly so that we can refer to by reference as an owner. // If we had a smarter way to dedupe we might not have to do this if there ends up // being no references to this as an owner. - outlineModel(request, componentDebugInfo); + + outlineComponentInfo(request, componentDebugInfo); emitDebugChunk(request, componentDebugID, componentDebugInfo); // We've emitted the latest environment for this task so we track that. @@ -1582,6 +1588,13 @@ function renderClientElement( } else if (keyPath !== null) { key = keyPath + ',' + key; } + if (__DEV__) { + if (task.debugOwner !== null) { + // Ensure we outline this owner if it is the first time we see it. + // So that we can refer to it directly. + outlineComponentInfo(request, task.debugOwner); + } + } const element = __DEV__ ? enableOwnerStacks ? [ @@ -1702,6 +1715,7 @@ function renderElement( task.debugStack === null ? null : filterStackTrace(request, task.debugStack, 1), + props: props, debugStack: task.debugStack, debugTask: task.debugTask, }; @@ -2128,7 +2142,7 @@ function serializeSet(request: Request, set: Set): string { function serializeConsoleMap( request: Request, - counter: {objectCount: number}, + counter: {objectLimit: number}, map: Map, ): string { // Like serializeMap but for renderConsoleValue. @@ -2139,7 +2153,7 @@ function serializeConsoleMap( function serializeConsoleSet( request: Request, - counter: {objectCount: number}, + counter: {objectLimit: number}, set: Set, ): string { // Like serializeMap but for renderConsoleValue. @@ -2263,23 +2277,6 @@ function escapeStringValue(value: string): string { } } -function isReactComponentInfo(value: any): boolean { - // TODO: We don't currently have a brand check on ReactComponentInfo. Reconsider. - return ( - ((typeof value.debugTask === 'object' && - value.debugTask !== null && - // $FlowFixMe[method-unbinding] - typeof value.debugTask.run === 'function') || - value.debugStack instanceof Error) && - (enableOwnerStacks - ? isArray((value: any).stack) || (value: any).stack === null - : typeof (value: any).stack === 'undefined') && - typeof value.name === 'string' && - typeof value.env === 'string' && - value.owner !== undefined - ); -} - let modelRoot: null | ReactClientValue = false; function renderModel( @@ -2795,25 +2792,6 @@ function renderModelDestructive( ); } if (__DEV__) { - if (isReactComponentInfo(value)) { - // This looks like a ReactComponentInfo. We can't serialize the ConsoleTask object so we - // need to omit it before serializing. - const componentDebugInfo: Omit< - ReactComponentInfo, - 'debugTask' | 'debugStack', - > = { - name: (value: any).name, - env: (value: any).env, - key: (value: any).key, - owner: (value: any).owner, - }; - if (enableOwnerStacks) { - // $FlowFixMe[cannot-write] - componentDebugInfo.stack = (value: any).stack; - } - return componentDebugInfo; - } - if (objectName(value) !== 'Object') { callWithDebugContextInDEV(request, task, () => { console.error( @@ -3241,7 +3219,7 @@ function emitDebugChunk( // We use the console encoding so that we can dedupe objects but don't necessarily // use the full serialization that requires a task. - const counter = {objectCount: 0}; + const counter = {objectLimit: 500}; function replacer( this: | {+[key: string | number]: ReactClientValue} @@ -3265,6 +3243,61 @@ function emitDebugChunk( request.completedRegularChunks.push(processedChunk); } +function outlineComponentInfo( + request: Request, + componentInfo: ReactComponentInfo, +): void { + if (!__DEV__) { + // These errors should never make it into a build so we don't need to encode them in codes.json + // eslint-disable-next-line react-internal/prod-error-codes + throw new Error( + 'outlineComponentInfo should never be called in production mode. This is a bug in React.', + ); + } + + if (request.writtenObjects.has(componentInfo)) { + // Already written + return; + } + + if (componentInfo.owner != null) { + // Ensure the owner is already outlined. + outlineComponentInfo(request, componentInfo.owner); + } + + // Limit the number of objects we write to prevent emitting giant props objects. + let objectLimit = 10; + if (componentInfo.stack != null) { + // Ensure we have enough object limit to encode the stack trace. + objectLimit += componentInfo.stack.length; + } + + // We use the console encoding so that we can dedupe objects but don't necessarily + // use the full serialization that requires a task. + const counter = {objectLimit}; + + // We can't serialize the ConsoleTask/Error objects so we need to omit them before serializing. + const componentDebugInfo: Omit< + ReactComponentInfo, + 'debugTask' | 'debugStack', + > = { + name: componentInfo.name, + env: componentInfo.env, + key: componentInfo.key, + owner: componentInfo.owner, + }; + if (enableOwnerStacks) { + // $FlowFixMe[cannot-write] + componentDebugInfo.stack = componentInfo.stack; + } + // Ensure we serialize props after the stack to favor the stack being complete. + // $FlowFixMe[cannot-write] + componentDebugInfo.props = componentInfo.props; + + const id = outlineConsoleValue(request, counter, componentDebugInfo); + request.writtenObjects.set(componentInfo, serializeByValueID(id)); +} + function emitTypedArrayChunk( request: Request, id: number, @@ -3322,7 +3355,7 @@ function serializeEval(source: string): string { // in the depth it can encode. function renderConsoleValue( request: Request, - counter: {objectCount: number}, + counter: {objectLimit: number}, parent: | {+[propertyName: string | number]: ReactClientValue} | $ReadOnlyArray, @@ -3366,23 +3399,64 @@ function renderConsoleValue( } } - if (counter.objectCount > 500) { + const writtenObjects = request.writtenObjects; + const existingReference = writtenObjects.get(value); + if (existingReference !== undefined) { + // We've already emitted this as a real object, so we can + // just refer to that by its existing reference. + return existingReference; + } + + if (counter.objectLimit <= 0) { // We've reached our max number of objects to serialize across the wire so we serialize this // as a marker so that the client can error when this is accessed by the console. return serializeLimitedObject(); } - counter.objectCount++; + counter.objectLimit--; - const writtenObjects = request.writtenObjects; - const existingReference = writtenObjects.get(value); - // $FlowFixMe[method-unbinding] - if (typeof value.then === 'function') { - if (existingReference !== undefined) { - // We've seen this promise before, so we can just refer to the same result. - return existingReference; + switch ((value: any).$$typeof) { + case REACT_ELEMENT_TYPE: { + const element: ReactElement = (value: any); + + if (element._owner != null) { + outlineComponentInfo(request, element._owner); + } + if (enableOwnerStacks) { + let debugStack: null | ReactStackTrace = null; + if (element._debugStack != null) { + // Outline the debug stack so that it doesn't get cut off. + debugStack = filterStackTrace(request, element._debugStack, 1); + const stackId = outlineConsoleValue( + request, + {objectLimit: debugStack.length + 2}, + debugStack, + ); + request.writtenObjects.set(debugStack, serializeByValueID(stackId)); + } + return [ + REACT_ELEMENT_TYPE, + element.type, + element.key, + element.props, + element._owner, + debugStack, + element._store.validated, + ]; + } + + return [ + REACT_ELEMENT_TYPE, + element.type, + element.key, + element.props, + element._owner, + ]; } + } + // $FlowFixMe[method-unbinding] + if (typeof value.then === 'function') { const thenable: Thenable = (value: any); switch (thenable.status) { case 'fulfilled': { @@ -3416,12 +3490,6 @@ function renderConsoleValue( return serializeInfinitePromise(); } - if (existingReference !== undefined) { - // We've already emitted this as a real object, so we can - // just refer to that by its existing reference. - return existingReference; - } - if (isArray(value)) { return value; } @@ -3503,25 +3571,6 @@ function renderConsoleValue( return Array.from((value: any)); } - if (isReactComponentInfo(value)) { - // This looks like a ReactComponentInfo. We can't serialize the ConsoleTask object so we - // need to omit it before serializing. - const componentDebugInfo: Omit< - ReactComponentInfo, - 'debugTask' | 'debugStack', - > = { - name: (value: any).name, - env: (value: any).env, - key: (value: any).key, - owner: (value: any).owner, - }; - if (enableOwnerStacks) { - // $FlowFixMe[cannot-write] - componentDebugInfo.stack = (value: any).stack; - } - return componentDebugInfo; - } - // $FlowFixMe[incompatible-return] return value; } @@ -3602,7 +3651,7 @@ function renderConsoleValue( function outlineConsoleValue( request: Request, - counter: {objectCount: number}, + counter: {objectLimit: number}, model: ReactClientValue, ): number { if (!__DEV__) { @@ -3629,7 +3678,9 @@ function outlineConsoleValue( value, ); } catch (x) { - return 'unknown value'; + return ( + 'Unknown Value: React could not send it from the server.\n' + x.message + ); } } @@ -3660,7 +3711,7 @@ function emitConsoleChunk( ); } - const counter = {objectCount: 0}; + const counter = {objectLimit: 500}; function replacer( this: | {+[key: string | number]: ReactClientValue} @@ -3677,10 +3728,17 @@ function emitConsoleChunk( value, ); } catch (x) { - return 'unknown value'; + return ( + 'Unknown Value: React could not send it from the server.\n' + x.message + ); } } + // Ensure the owner is already outlined. + if (owner != null) { + outlineComponentInfo(request, owner); + } + // TODO: Don't double badge if this log came from another Flight Client. const env = (0, request.environmentName)(); const payload = [methodName, stackTrace, owner, env]; @@ -3704,7 +3762,7 @@ function forwardDebugInfo( // We outline this model eagerly so that we can refer to by reference as an owner. // If we had a smarter way to dedupe we might not have to do this if there ends up // being no references to this as an owner. - outlineModel(request, debugInfo[i]); + outlineComponentInfo(request, (debugInfo[i]: any)); } emitDebugChunk(request, id, debugInfo[i]); } diff --git a/packages/shared/ReactTypes.js b/packages/shared/ReactTypes.js index 7d51fbe4725d3..54eccd5538dd8 100644 --- a/packages/shared/ReactTypes.js +++ b/packages/shared/ReactTypes.js @@ -193,6 +193,7 @@ export type ReactComponentInfo = { +key?: null | string, +owner?: null | ReactComponentInfo, +stack?: null | ReactStackTrace, + +props?: null | {[name: string]: mixed}, // Stashed Data for the Specific Execution Environment. Not part of the transport protocol +debugStack?: null | Error, +debugTask?: null | ConsoleTask, From 40357fe63071950b0bba304657a003755aec4e30 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Tue, 1 Oct 2024 14:03:48 +0100 Subject: [PATCH 11/13] fix[react-devtools]: request hook initialization inside http server response (#31102) Fixes https://github.com/facebook/react/issues/31100. There are 2 things: 1. In https://github.com/facebook/react/pull/30987, we've introduced a breaking change: importing `react-devtools-core` is no longer enough for installing React DevTools global Hook. You need to call `initialize`, in which you may provide initial settings. I am not adding settings here, because it is not implemented, and there are no plans for supporting this. 2. Calling `installHook` is not necessary inside `standalone.js`, because this script is running inside Electron wrapper (which is just a UI, not the app that we are debugging). We will loose the ability to use React DevTools on this React application, but I guess thats fine. --- packages/react-devtools-core/src/standalone.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-devtools-core/src/standalone.js b/packages/react-devtools-core/src/standalone.js index bfd05cf5227ee..d65b41b478fa2 100644 --- a/packages/react-devtools-core/src/standalone.js +++ b/packages/react-devtools-core/src/standalone.js @@ -17,7 +17,6 @@ import {registerDevToolsEventLogger} from 'react-devtools-shared/src/registerDev import {Server} from 'ws'; import {join} from 'path'; import {readFileSync} from 'fs'; -import {installHook} from 'react-devtools-shared/src/hook'; import DevTools from 'react-devtools-shared/src/devtools/views/DevTools'; import {doesFilePathExist, launchEditor} from './editor'; import { @@ -29,8 +28,6 @@ import {localStorageSetItem} from 'react-devtools-shared/src/storage'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type {Source} from 'react-devtools-shared/src/shared/types'; -installHook(window); - export type StatusTypes = 'server-connected' | 'devtools-connected' | 'error'; export type StatusListener = (message: string, status: StatusTypes) => void; export type OnDisconnectedCallback = () => void; @@ -371,9 +368,12 @@ function startServer( '\n;' + backendFile.toString() + '\n;' + + 'ReactDevToolsBackend.initialize();' + + '\n' + `ReactDevToolsBackend.connectToDevTools({port: ${port}, host: '${host}', useHttps: ${ useHttps ? 'true' : 'false' - }});`, + }}); + `, ); }); From 9ea5ffa9cba4869474a1d5d53e7d6c135be6adf7 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Tue, 1 Oct 2024 14:04:05 +0100 Subject: [PATCH 12/13] chore[react-devtools]: add legacy mode error message to the ignore list for tests (#31060) Without this, the console gets spammy whenever we run React DevTools tests against React 18.x, where this deprecation message was added. --- packages/react-devtools-shared/src/__tests__/setupTests.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/__tests__/setupTests.js b/packages/react-devtools-shared/src/__tests__/setupTests.js index d4afd05899a56..0a821345fcbac 100644 --- a/packages/react-devtools-shared/src/__tests__/setupTests.js +++ b/packages/react-devtools-shared/src/__tests__/setupTests.js @@ -150,7 +150,10 @@ function patchConsoleForTestingBeforeHookInstallation() { firstArg.startsWith( 'The current testing environment is not configured to support act', ) || - firstArg.startsWith('You seem to have overlapping act() calls')) + firstArg.startsWith('You seem to have overlapping act() calls') || + firstArg.startsWith( + 'ReactDOM.render is no longer supported in React 18.', + )) ) { // DevTools intentionally wraps updates with acts from both DOM and test-renderer, // since test updates are expected to impact both renderers. From 6e612587ecfca0ea2e331300635d497d54437930 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Tue, 1 Oct 2024 14:26:12 +0100 Subject: [PATCH 13/13] chore[react-devtools]: drop legacy context tests (#31059) We've dropped the support for detecting changes in legacy Contexts in https://github.com/facebook/react/pull/30896. --- .../src/__tests__/profilingCache-test.js | 688 ------------------ 1 file changed, 688 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js index 0d6d8d02a1989..fb1fa6c5f2298 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js @@ -13,7 +13,6 @@ import type Store from 'react-devtools-shared/src/devtools/store'; import {getVersionedRenderImplementation} from './utils'; describe('ProfilingCache', () => { - let PropTypes; let React; let ReactDOM; let ReactDOMClient; @@ -34,7 +33,6 @@ describe('ProfilingCache', () => { store.collapseNodesByDefault = false; store.recordChangeDescriptions = true; - PropTypes = require('prop-types'); React = require('react'); ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); @@ -309,692 +307,6 @@ describe('ProfilingCache', () => { }); }); - // @reactVersion >= 16.9 - // @reactVersion <= 18.2 - it('should record changed props/state/context/hooks for React version [16.9; 18.2] with legacy context', () => { - let instance = null; - - const ModernContext = React.createContext(0); - - class LegacyContextProvider extends React.Component { - static childContextTypes = { - count: PropTypes.number, - }; - state = {count: 0}; - getChildContext() { - return this.state; - } - render() { - instance = this; - return ( - - - - - - - ); - } - } - - const FunctionComponentWithHooks = ({count}) => { - React.useMemo(() => count, [count]); - return null; - }; - - class ModernContextConsumer extends React.Component { - static contextType = ModernContext; - render() { - return ; - } - } - - class LegacyContextConsumer extends React.Component { - static contextTypes = { - count: PropTypes.number, - }; - render() { - return ; - } - } - - utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => render()); - expect(instance).not.toBeNull(); - utils.act(() => (instance: any).setState({count: 1})); - utils.act(() => render()); - utils.act(() => render()); - utils.act(() => render()); - utils.act(() => store.profilerStore.stopProfiling()); - - const rootID = store.roots[0]; - - let changeDescriptions = store.profilerStore - .getDataForRoot(rootID) - .commitData.map(commitData => commitData.changeDescriptions); - expect(changeDescriptions).toHaveLength(5); - expect(changeDescriptions[0]).toMatchInlineSnapshot(` - Map { - 2 => { - "context": null, - "didHooksChange": false, - "isFirstMount": true, - "props": null, - "state": null, - }, - 4 => { - "context": null, - "didHooksChange": false, - "isFirstMount": true, - "props": null, - "state": null, - }, - 5 => { - "context": null, - "didHooksChange": false, - "isFirstMount": true, - "props": null, - "state": null, - }, - 6 => { - "context": null, - "didHooksChange": false, - "isFirstMount": true, - "props": null, - "state": null, - }, - 7 => { - "context": null, - "didHooksChange": false, - "isFirstMount": true, - "props": null, - "state": null, - }, - } - `); - expect(changeDescriptions[1]).toMatchInlineSnapshot(` - Map { - 5 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [ - "count", - ], - "state": null, - }, - 4 => { - "context": true, - "didHooksChange": false, - "hooks": null, - "isFirstMount": false, - "props": [], - "state": null, - }, - 7 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [ - "count", - ], - "state": null, - }, - 6 => { - "context": [ - "count", - ], - "didHooksChange": false, - "hooks": null, - "isFirstMount": false, - "props": [], - "state": null, - }, - 2 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [], - "state": [ - "count", - ], - }, - } - `); - expect(changeDescriptions[2]).toMatchInlineSnapshot(` - Map { - 5 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [], - "state": null, - }, - 4 => { - "context": false, - "didHooksChange": false, - "hooks": null, - "isFirstMount": false, - "props": [], - "state": null, - }, - 7 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [], - "state": null, - }, - 6 => { - "context": [], - "didHooksChange": false, - "hooks": null, - "isFirstMount": false, - "props": [], - "state": null, - }, - 2 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [ - "foo", - ], - "state": [], - }, - } - `); - expect(changeDescriptions[3]).toMatchInlineSnapshot(` - Map { - 5 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [], - "state": null, - }, - 4 => { - "context": false, - "didHooksChange": false, - "hooks": null, - "isFirstMount": false, - "props": [], - "state": null, - }, - 7 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [], - "state": null, - }, - 6 => { - "context": [], - "didHooksChange": false, - "hooks": null, - "isFirstMount": false, - "props": [], - "state": null, - }, - 2 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [ - "foo", - "bar", - ], - "state": [], - }, - } - `); - expect(changeDescriptions[4]).toMatchInlineSnapshot(` - Map { - 5 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [], - "state": null, - }, - 4 => { - "context": false, - "didHooksChange": false, - "hooks": null, - "isFirstMount": false, - "props": [], - "state": null, - }, - 7 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [], - "state": null, - }, - 6 => { - "context": [], - "didHooksChange": false, - "hooks": null, - "isFirstMount": false, - "props": [], - "state": null, - }, - 2 => { - "context": null, - "didHooksChange": false, - "hooks": [], - "isFirstMount": false, - "props": [ - "bar", - ], - "state": [], - }, - } - `); - - utils.exportImportHelper(bridge, store); - - const prevChangeDescriptions = [...changeDescriptions]; - - changeDescriptions = store.profilerStore - .getDataForRoot(rootID) - .commitData.map(commitData => commitData.changeDescriptions); - expect(changeDescriptions).toHaveLength(5); - - for (let commitIndex = 0; commitIndex < 5; commitIndex++) { - expect(changeDescriptions[commitIndex]).toEqual( - prevChangeDescriptions[commitIndex], - ); - } - }); - - // @reactVersion > 18.2 - // @gate !disableLegacyContext - it('should record changed props/state/context/hooks for React version (18.2; ∞) with legacy context enabled', () => { - let instance = null; - - const ModernContext = React.createContext(0); - - class LegacyContextProvider extends React.Component { - static childContextTypes = { - count: PropTypes.number, - }; - state = {count: 0}; - getChildContext() { - return this.state; - } - render() { - instance = this; - return ( - - - - - - - ); - } - } - - const FunctionComponentWithHooks = ({count}) => { - React.useMemo(() => count, [count]); - return null; - }; - - class ModernContextConsumer extends React.Component { - static contextType = ModernContext; - render() { - return ; - } - } - - class LegacyContextConsumer extends React.Component { - static contextTypes = { - count: PropTypes.number, - }; - render() { - return ; - } - } - - utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => render()); - expect(instance).not.toBeNull(); - utils.act(() => (instance: any).setState({count: 1})); - utils.act(() => render()); - utils.act(() => render()); - utils.act(() => render()); - utils.act(() => store.profilerStore.stopProfiling()); - - const rootID = store.roots[0]; - - let changeDescriptions = store.profilerStore - .getDataForRoot(rootID) - .commitData.map(commitData => commitData.changeDescriptions); - expect(changeDescriptions).toHaveLength(5); - expect(changeDescriptions[0]).toEqual( - new Map([ - [ - 2, - { - context: null, - didHooksChange: false, - isFirstMount: true, - props: null, - state: null, - }, - ], - [ - 4, - { - context: null, - didHooksChange: false, - isFirstMount: true, - props: null, - state: null, - }, - ], - [ - 5, - { - context: null, - didHooksChange: false, - isFirstMount: true, - props: null, - state: null, - }, - ], - [ - 6, - { - context: null, - didHooksChange: false, - isFirstMount: true, - props: null, - state: null, - }, - ], - [ - 7, - { - context: null, - didHooksChange: false, - isFirstMount: true, - props: null, - state: null, - }, - ], - ]), - ); - - expect(changeDescriptions[1]).toEqual( - new Map([ - [ - 5, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: ['count'], - state: null, - }, - ], - [ - 4, - { - context: true, - didHooksChange: false, - hooks: null, - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 7, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: ['count'], - state: null, - }, - ], - [ - 6, - { - context: ['count'], - didHooksChange: false, - hooks: null, - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 2, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: [], - state: ['count'], - }, - ], - ]), - ); - - expect(changeDescriptions[2]).toEqual( - new Map([ - [ - 5, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 4, - { - context: false, - didHooksChange: false, - hooks: null, - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 7, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 6, - { - context: [], - didHooksChange: false, - hooks: null, - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 2, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: ['foo'], - state: [], - }, - ], - ]), - ); - - expect(changeDescriptions[3]).toEqual( - new Map([ - [ - 5, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 4, - { - context: false, - didHooksChange: false, - hooks: null, - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 7, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 6, - { - context: [], - didHooksChange: false, - hooks: null, - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 2, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: ['foo', 'bar'], - state: [], - }, - ], - ]), - ); - - expect(changeDescriptions[4]).toEqual( - new Map([ - [ - 5, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 4, - { - context: false, - didHooksChange: false, - hooks: null, - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 7, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 6, - { - context: [], - didHooksChange: false, - hooks: null, - isFirstMount: false, - props: [], - state: null, - }, - ], - [ - 2, - { - context: null, - didHooksChange: false, - hooks: [], - isFirstMount: false, - props: ['bar'], - state: [], - }, - ], - ]), - ); - - utils.exportImportHelper(bridge, store); - - const prevChangeDescriptions = [...changeDescriptions]; - - changeDescriptions = store.profilerStore - .getDataForRoot(rootID) - .commitData.map(commitData => commitData.changeDescriptions); - expect(changeDescriptions).toHaveLength(5); - - for (let commitIndex = 0; commitIndex < 5; commitIndex++) { - expect(changeDescriptions[commitIndex]).toEqual( - prevChangeDescriptions[commitIndex], - ); - } - }); - // @reactVersion >= 18.0 it('should properly detect changed hooks', () => { const Context = React.createContext(0);