From c09402aa2fc4da56f6ecabe5f5a042436b277a57 Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Fri, 15 Nov 2024 13:06:05 -0500 Subject: [PATCH] [compiler] Stop using function `dependencies` in propagateScopeDeps (#31200) Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) - --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31200). * #31202 * #31203 * #31201 * __->__ #31200 * #31521 --- .../src/HIR/Environment.ts | 2 + .../src/HIR/PropagateScopeDependenciesHIR.ts | 67 +++++++++------ .../capturing-func-mutate-2.expect.md | 21 ++--- ...ures-reassigned-context-property.expect.md | 53 ++++++++++++ ...k-captures-reassigned-context-property.tsx | 32 ++++++++ ...less-specific-conditional-access.expect.md | 2 - ...ures-reassigned-context-property.expect.md | 81 ------------------- ...k-captures-reassigned-context-property.tsx | 21 ----- ...back-captures-reassigned-context.expect.md | 16 ++-- ...llback-extended-contextvar-scope.expect.md | 26 +++--- ...unction-uncond-optionals-hoisted.expect.md | 4 +- ...unction-uncond-optionals-hoisted.expect.md | 4 +- 12 files changed, 159 insertions(+), 170 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.tsx delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 1d2e155848802..855bc039abf37 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -231,6 +231,8 @@ const EnvironmentConfigSchema = z.object({ */ enableUseTypeAnnotations: z.boolean().default(false), + enableFunctionDependencyRewrite: z.boolean().default(true), + /** * Enables inlining ReactElement object literals in place of JSX * An alternative to the standard JSX transform which replaces JSX with React's jsxProd() runtime 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 7fd44c29dccf6..8aed17f8ee847 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -663,35 +663,54 @@ function collectDependencies( const scopeTraversal = new ScopeBlockTraversal(); - for (const [blockId, block] of fn.body.blocks) { - scopeTraversal.recordScopes(block); - const scopeBlockInfo = scopeTraversal.blockInfos.get(blockId); - if (scopeBlockInfo?.kind === 'begin') { - context.enterScope(scopeBlockInfo.scope); - } else if (scopeBlockInfo?.kind === 'end') { - context.exitScope(scopeBlockInfo.scope, scopeBlockInfo?.pruned); - } - - // Record referenced optional chains in phis - for (const phi of block.phis) { - for (const operand of phi.operands) { - const maybeOptionalChain = temporaries.get(operand[1].identifier.id); - if (maybeOptionalChain) { - context.visitDependency(maybeOptionalChain); + const handleFunction = (fn: HIRFunction): void => { + for (const [blockId, block] of fn.body.blocks) { + scopeTraversal.recordScopes(block); + const scopeBlockInfo = scopeTraversal.blockInfos.get(blockId); + if (scopeBlockInfo?.kind === 'begin') { + context.enterScope(scopeBlockInfo.scope); + } else if (scopeBlockInfo?.kind === 'end') { + context.exitScope(scopeBlockInfo.scope, scopeBlockInfo.pruned); + } + // Record referenced optional chains in phis + for (const phi of block.phis) { + for (const operand of phi.operands) { + const maybeOptionalChain = temporaries.get(operand[1].identifier.id); + if (maybeOptionalChain) { + context.visitDependency(maybeOptionalChain); + } } } - } - for (const instr of block.instructions) { - if (!processedInstrsInOptional.has(instr)) { - handleInstruction(instr, context); + for (const instr of block.instructions) { + if ( + fn.env.config.enableFunctionDependencyRewrite && + (instr.value.kind === 'FunctionExpression' || + instr.value.kind === 'ObjectMethod') + ) { + context.declare(instr.lvalue.identifier, { + id: instr.id, + scope: context.currentScope, + }); + /** + * Recursively visit the inner function to extract dependencies there + */ + const wasInInnerFn = context.inInnerFn; + context.inInnerFn = true; + handleFunction(instr.value.loweredFunc.func); + context.inInnerFn = wasInInnerFn; + } else if (!processedInstrsInOptional.has(instr)) { + handleInstruction(instr, context); + } } - } - if (!processedInstrsInOptional.has(block.terminal)) { - for (const place of eachTerminalOperand(block.terminal)) { - context.visitOperand(place); + if (!processedInstrsInOptional.has(block.terminal)) { + for (const place of eachTerminalOperand(block.terminal)) { + context.visitOperand(place); + } } } - } + }; + + handleFunction(fn); return context.deps; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md index b31a16da90e3f..c071d5d20ed99 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md @@ -26,29 +26,20 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { c as _c } from "react/compiler-runtime"; function component(a, b) { - const $ = _c(5); - let t0; - if ($[0] !== b) { - t0 = { b }; - $[0] = b; - $[1] = t0; - } else { - t0 = $[1]; - } - const y = t0; + const $ = _c(2); + const y = { b }; let z; - if ($[2] !== a || $[3] !== y) { + if ($[0] !== a) { z = { a }; const x = function () { z.a = 2; }; x(); - $[2] = a; - $[3] = y; - $[4] = z; + $[0] = a; + $[1] = z; } else { - z = $[4]; + z = $[1]; } return z; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.expect.md new file mode 100644 index 0000000000000..ae44f27912293 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.expect.md @@ -0,0 +1,53 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +import {useCallback} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * TODO: we're currently bailing out because `contextVar` is a context variable + * and not recorded into the PropagateScopeDeps LoadLocal / PropertyLoad + * sidemap. Previously, we were able to avoid this as `BuildHIR` hoisted + * `LoadContext` and `PropertyLoad` instructions into the outer function, which + * we took as eligible dependencies. + * + * One solution is to simply record `LoadContext` identifiers into the + * temporaries sidemap when the instruction occurs *after* the context + * variable's mutable range. + */ +function Foo(props) { + let contextVar; + if (props.cond) { + contextVar = {val: 2}; + } else { + contextVar = {}; + } + + const cb = useCallback(() => [contextVar.val], [contextVar.val]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{cond: true}], +}; + +``` + + +## Error + +``` + 22 | } + 23 | +> 24 | const cb = useCallback(() => [contextVar.val], [contextVar.val]); + | ^^^^^^^^^^^^^^^^^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (24:24) + 25 | + 26 | return ; + 27 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.tsx new file mode 100644 index 0000000000000..8447e3960dc51 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.tsx @@ -0,0 +1,32 @@ +// @validatePreserveExistingMemoizationGuarantees +import {useCallback} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * TODO: we're currently bailing out because `contextVar` is a context variable + * and not recorded into the PropagateScopeDeps LoadLocal / PropertyLoad + * sidemap. Previously, we were able to avoid this as `BuildHIR` hoisted + * `LoadContext` and `PropertyLoad` instructions into the outer function, which + * we took as eligible dependencies. + * + * One solution is to simply record `LoadContext` identifiers into the + * temporaries sidemap when the instruction occurs *after* the context + * variable's mutable range. + */ +function Foo(props) { + let contextVar; + if (props.cond) { + contextVar = {val: 2}; + } else { + contextVar = {}; + } + + const cb = useCallback(() => [contextVar.val], [contextVar.val]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{cond: true}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.useCallback-infer-less-specific-conditional-access.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.useCallback-infer-less-specific-conditional-access.expect.md index 955d391f912cf..940b3975c160d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.useCallback-infer-less-specific-conditional-access.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.useCallback-infer-less-specific-conditional-access.expect.md @@ -44,8 +44,6 @@ function Component({propA, propB}) { | ^^^^^^^^^^^^^^^^^ > 14 | }, [propA?.a, propB.x.y]); | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (6:14) - -CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (6:14) 15 | } 16 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.expect.md deleted file mode 100644 index db69bc2821b12..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.expect.md +++ /dev/null @@ -1,81 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees -import {useCallback} from 'react'; -import {Stringify} from 'shared-runtime'; - -function Foo(props) { - let contextVar; - if (props.cond) { - contextVar = {val: 2}; - } else { - contextVar = {}; - } - - const cb = useCallback(() => [contextVar.val], [contextVar.val]); - - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{cond: true}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees -import { useCallback } from "react"; -import { Stringify } from "shared-runtime"; - -function Foo(props) { - const $ = _c(6); - let contextVar; - if ($[0] !== props.cond) { - if (props.cond) { - contextVar = { val: 2 }; - } else { - contextVar = {}; - } - $[0] = props.cond; - $[1] = contextVar; - } else { - contextVar = $[1]; - } - - const t0 = contextVar; - let t1; - if ($[2] !== t0.val) { - t1 = () => [contextVar.val]; - $[2] = t0.val; - $[3] = t1; - } else { - t1 = $[3]; - } - contextVar; - const cb = t1; - let t2; - if ($[4] !== cb) { - t2 = ; - $[4] = cb; - $[5] = t2; - } else { - t2 = $[5]; - } - return t2; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{ cond: true }], -}; - -``` - -### Eval output -(kind: ok)
{"cb":{"kind":"Function","result":[2]},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.tsx deleted file mode 100644 index cb6f65a9f4f52..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.tsx +++ /dev/null @@ -1,21 +0,0 @@ -// @validatePreserveExistingMemoizationGuarantees -import {useCallback} from 'react'; -import {Stringify} from 'shared-runtime'; - -function Foo(props) { - let contextVar; - if (props.cond) { - contextVar = {val: 2}; - } else { - contextVar = {}; - } - - const cb = useCallback(() => [contextVar.val], [contextVar.val]); - - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{cond: true}], -}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context.expect.md index b66661fbcaa08..41994e1e56e44 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context.expect.md @@ -45,18 +45,16 @@ function Foo(props) { } else { x = $[1]; } - - const t0 = x; - let t1; - if ($[2] !== t0) { - t1 = () => [x]; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== x) { + t0 = () => [x]; + $[2] = x; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } x; - const cb = t1; + const cb = t0; return cb; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md index b141c27614d24..9ce4a62e710d4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md @@ -70,28 +70,26 @@ function useBar(t0, cond) { if (cond) { x = b; } - - const t2 = x; - let t3; - if ($[1] !== a || $[2] !== t2) { - t3 = () => [a, x]; + let t2; + if ($[1] !== a || $[2] !== x) { + t2 = () => [a, x]; $[1] = a; - $[2] = t2; - $[3] = t3; + $[2] = x; + $[3] = t2; } else { - t3 = $[3]; + t2 = $[3]; } x; - const cb = t3; - let t4; + const cb = t2; + let t3; if ($[4] !== cb) { - t4 = ; + t3 = ; $[4] = cb; - $[5] = t4; + $[5] = t3; } else { - t4 = $[5]; + t3 = $[5]; } - return t4; + return t3; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md index 02e60eff91d61..ed56ff068113b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md @@ -34,9 +34,9 @@ function useFoo(t0) { const $ = _c(2); const { a } = t0; let t1; - if ($[0] !== a.b) { + if ($[0] !== a.b?.c.d?.e) { t1 = a.b?.c.d?.e} shouldInvokeFns={true} />; - $[0] = a.b; + $[0] = a.b?.c.d?.e; $[1] = t1; } else { t1 = $[1]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md index 157e2de81a1ff..bb99a5d90fe2b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md @@ -31,9 +31,9 @@ function useFoo(t0) { const $ = _c(2); const { a } = t0; let t1; - if ($[0] !== a.b) { + if ($[0] !== a.b?.c.d?.e) { t1 = a.b?.c.d?.e} shouldInvokeFns={true} />; - $[0] = a.b; + $[0] = a.b?.c.d?.e; $[1] = t1; } else { t1 = $[1];