From 2caaa05c08c345f1edddc952cef3fa53c177d612 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 21 Jun 2024 16:48:00 -0700 Subject: [PATCH 01/25] [compiler] Optimize instruction reordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks. In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures. ghstack-source-id: ad3e516fa474235ced8c5d56f4541d2a7c413608 Pull Request resolved: https://github.com/facebook/react/pull/29882 --- .../src/Optimization/InstructionReordering.ts | 278 ++++++++++++++---- .../InferReactiveScopeVariables.ts | 5 +- ...ge-consecutive-scopes-reordering.expect.md | 54 ++-- .../compiler/merge-scopes-callback.expect.md | 32 +- 4 files changed, 247 insertions(+), 122 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts index 7b90740417ab7..6708758babe21 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts @@ -13,16 +13,19 @@ import { HIRFunction, IdentifierId, Instruction, + InstructionId, + Place, isExpressionBlockKind, + makeInstructionId, markInstructionIds, } from "../HIR"; import { printInstruction } from "../HIR/PrintHIR"; import { + eachInstructionLValue, eachInstructionValueLValue, eachInstructionValueOperand, eachTerminalOperand, } from "../HIR/visitors"; -import { mayAllocate } from "../ReactiveScopes/InferReactiveScopeVariables"; import { getOrInsertWith } from "../Utils/utils"; /** @@ -69,8 +72,9 @@ import { getOrInsertWith } from "../Utils/utils"; export function instructionReordering(fn: HIRFunction): void { // Shared nodes are emitted when they are first used const shared: Nodes = new Map(); + const references = findReferencedRangeOfTemporaries(fn); for (const [, block] of fn.body.blocks) { - reorderBlock(fn.env, block, shared); + reorderBlock(fn.env, block, shared, references); } CompilerError.invariant(shared.size === 0, { reason: `InstructionReordering: expected all reorderable nodes to have been emitted`, @@ -88,13 +92,79 @@ type Nodes = Map; type Node = { instruction: Instruction | null; dependencies: Set; + reorderability: Reorderability; depth: number | null; }; +// Inclusive start and end +type References = { + singleUseIdentifiers: SingleUseIdentifiers; + lastAssignments: LastAssignments; +}; +type LastAssignments = Map; +type SingleUseIdentifiers = Set; +enum ReferenceKind { + Read, + Write, +} +function findReferencedRangeOfTemporaries(fn: HIRFunction): References { + const singleUseIdentifiers = new Map(); + const lastAssignments: LastAssignments = new Map(); + function reference( + instr: InstructionId, + place: Place, + kind: ReferenceKind + ): void { + if ( + place.identifier.name !== null && + place.identifier.name.kind === "named" + ) { + if (kind === ReferenceKind.Write) { + const name = place.identifier.name.value; + const previous = lastAssignments.get(name); + if (previous === undefined) { + lastAssignments.set(name, instr); + } else { + lastAssignments.set( + name, + makeInstructionId(Math.max(previous, instr)) + ); + } + } + return; + } else if (kind === ReferenceKind.Read) { + const previousCount = singleUseIdentifiers.get(place.identifier.id) ?? 0; + singleUseIdentifiers.set(place.identifier.id, previousCount + 1); + } + } + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + for (const operand of eachInstructionValueLValue(instr.value)) { + reference(instr.id, operand, ReferenceKind.Read); + } + for (const lvalue of eachInstructionLValue(instr)) { + reference(instr.id, lvalue, ReferenceKind.Write); + } + } + for (const operand of eachTerminalOperand(block.terminal)) { + reference(block.terminal.id, operand, ReferenceKind.Read); + } + } + return { + singleUseIdentifiers: new Set( + [...singleUseIdentifiers] + .filter(([, count]) => count === 1) + .map(([id]) => id) + ), + lastAssignments, + }; +} + function reorderBlock( env: Environment, block: BasicBlock, - shared: Nodes + shared: Nodes, + references: References ): void { const locals: Nodes = new Map(); const named: Map = new Map(); @@ -102,6 +172,7 @@ function reorderBlock( for (const instr of block.instructions) { const { lvalue, value } = instr; // Get or create a node for this lvalue + const reorderability = getReorderability(instr, references); const node = getOrInsertWith( locals, lvalue.identifier.id, @@ -109,6 +180,7 @@ function reorderBlock( ({ instruction: instr, dependencies: new Set(), + reorderability, depth: null, }) as Node ); @@ -116,7 +188,7 @@ function reorderBlock( * Ensure non-reoderable instructions have their order retained by * adding explicit dependencies to the previous such instruction. */ - if (getReoderability(instr) === Reorderability.Nonreorderable) { + if (reorderability === Reorderability.Nonreorderable) { if (previous !== null) { node.dependencies.add(previous); } @@ -172,66 +244,125 @@ function reorderBlock( DEBUG && console.log(`bb${block.id}`); - // First emit everything that can't be reordered - if (previous !== null) { - DEBUG && console.log(`(last non-reorderable instruction)`); - DEBUG && print(env, locals, shared, seen, previous); - emit(env, locals, shared, nextInstructions, previous); - } - /* - * For "value" blocks the final instruction represents its value, so we have to be - * careful to not change the ordering. Emit the last instruction explicitly. - * Any non-reorderable instructions will get emitted first, and any unused - * reorderable instructions can be deferred to the shared node list. + /** + * The ideal order for emitting instructions may change the final instruction, + * but value blocks have special semantics for the final instruction of a block - + * that's the expression's value!. So we choose between a less optimal strategy + * for value blocks which preserves the final instruction order OR a more optimal + * ordering for statement-y blocks. */ - if (isExpressionBlockKind(block.kind) && block.instructions.length !== 0) { - DEBUG && console.log(`(block value)`); - DEBUG && - print( + if (isExpressionBlockKind(block.kind)) { + // First emit everything that can't be reordered + if (previous !== null) { + DEBUG && console.log(`(last non-reorderable instruction)`); + DEBUG && print(env, locals, shared, seen, previous); + emit(env, locals, shared, nextInstructions, previous); + } + /* + * For "value" blocks the final instruction represents its value, so we have to be + * careful to not change the ordering. Emit the last instruction explicitly. + * Any non-reorderable instructions will get emitted first, and any unused + * reorderable instructions can be deferred to the shared node list. + */ + if (block.instructions.length !== 0) { + DEBUG && console.log(`(block value)`); + DEBUG && + print( + env, + locals, + shared, + seen, + block.instructions.at(-1)!.lvalue.identifier.id + ); + emit( env, locals, shared, - seen, + nextInstructions, block.instructions.at(-1)!.lvalue.identifier.id ); - emit( - env, - locals, - shared, - nextInstructions, - block.instructions.at(-1)!.lvalue.identifier.id - ); - } - /* - * Then emit the dependencies of the terminal operand. In many cases they will have - * already been emitted in the previous step and this is a no-op. - * TODO: sort the dependencies based on weight, like we do for other nodes. Not a big - * deal though since most terminals have a single operand - */ - for (const operand of eachTerminalOperand(block.terminal)) { - DEBUG && console.log(`(terminal operand)`); - DEBUG && print(env, locals, shared, seen, operand.identifier.id); - emit(env, locals, shared, nextInstructions, operand.identifier.id); - } - // Anything not emitted yet is globally reorderable - for (const [id, node] of locals) { - if (node.instruction == null) { - continue; } - CompilerError.invariant( - node.instruction != null && - getReoderability(node.instruction) === Reorderability.Reorderable, - { - reason: `Expected all remaining instructions to be reorderable`, - loc: node.instruction?.loc ?? block.terminal.loc, - description: - node.instruction != null - ? `Instruction [${node.instruction.id}] was not emitted yet but is not reorderable` - : `Lvalue $${id} was not emitted yet but is not reorderable`, + /* + * Then emit the dependencies of the terminal operand. In many cases they will have + * already been emitted in the previous step and this is a no-op. + * TODO: sort the dependencies based on weight, like we do for other nodes. Not a big + * deal though since most terminals have a single operand + */ + for (const operand of eachTerminalOperand(block.terminal)) { + DEBUG && console.log(`(terminal operand)`); + DEBUG && print(env, locals, shared, seen, operand.identifier.id); + emit(env, locals, shared, nextInstructions, operand.identifier.id); + } + // Anything not emitted yet is globally reorderable + for (const [id, node] of locals) { + if (node.instruction == null) { + continue; } - ); - DEBUG && console.log(`save shared: $${id}`); - shared.set(id, node); + CompilerError.invariant( + node.reorderability === Reorderability.Reorderable, + { + reason: `Expected all remaining instructions to be reorderable`, + loc: node.instruction?.loc ?? block.terminal.loc, + description: + node.instruction != null + ? `Instruction [${node.instruction.id}] was not emitted yet but is not reorderable` + : `Lvalue $${id} was not emitted yet but is not reorderable`, + } + ); + + DEBUG && console.log(`save shared: $${id}`); + shared.set(id, node); + } + } else { + /** + * If this is not a value block, then the order within the block doesn't matter + * and we can optimize more. The observation is that blocks often have instructions + * such as: + * + * ``` + * t$0 = nonreorderable + * t$1 = nonreorderable <-- this gets in the way of merging t$0 and t$2 + * t$2 = reorderable deps[ t$0 ] + * return t$2 + * ``` + * + * Ie where there is some pair of nonreorderable+reorderable values, with some intervening + * also non-reorderable instruction. If we emit all non-reorderable instructions first, + * then we'll keep the original order. But reordering instructions doesn't just mean moving + * them later: we can also move them _earlier_. By starting from terminal operands we + * end up emitting: + * + * ``` + * t$0 = nonreorderable // dep of t$2 + * t$2 = reorderable deps[ t$0 ] + * t$1 = nonreorderable <-- not in the way of merging anymore! + * return t$2 + * ``` + * + * Ie all nonreorderable transitive deps of the terminal operands will get emitted first, + * but we'll be able to intersperse the depending reorderable instructions in between + * them in a way that works better with scope merging. + */ + for (const operand of eachTerminalOperand(block.terminal)) { + DEBUG && console.log(`(terminal operand)`); + DEBUG && print(env, locals, shared, seen, operand.identifier.id); + emit(env, locals, shared, nextInstructions, operand.identifier.id); + } + // Anything not emitted yet is globally reorderable + for (const id of Array.from(locals.keys()).reverse()) { + const node = locals.get(id); + if (node === undefined) { + continue; + } + if (node.reorderability === Reorderability.Reorderable) { + DEBUG && console.log(`save shared: $${id}`); + shared.set(id, node); + } else { + DEBUG && console.log("leftover"); + DEBUG && print(env, locals, shared, seen, id); + emit(env, locals, shared, nextInstructions, id); + } + } } block.instructions = nextInstructions; @@ -247,8 +378,7 @@ function getDepth(env: Environment, nodes: Nodes, id: IdentifierId): number { return node.depth; } node.depth = 0; // in case of cycles - let depth = - node.instruction != null && mayAllocate(env, node.instruction) ? 1 : 0; + let depth = node.reorderability === Reorderability.Reorderable ? 1 : 10; for (const dep of node.dependencies) { depth += getDepth(env, nodes, dep); } @@ -265,7 +395,7 @@ function print( depth: number = 0 ): void { if (seen.has(id)) { - console.log(`${"| ".repeat(depth)}$${id} `); + DEBUG && console.log(`${"| ".repeat(depth)}$${id} `); return; } seen.add(id); @@ -282,11 +412,12 @@ function print( for (const dep of deps) { print(env, locals, shared, seen, dep, depth + 1); } - console.log( - `${"| ".repeat(depth)}$${id} ${printNode(node)} deps=[${deps - .map((x) => `$${x}`) - .join(", ")}]` - ); + DEBUG && + console.log( + `${"| ".repeat(depth)}$${id} ${printNode(node)} deps=[${deps + .map((x) => `$${x}`) + .join(", ")}] depth=${node.depth}` + ); } function printNode(node: Node): string { @@ -336,7 +467,10 @@ enum Reorderability { Reorderable, Nonreorderable, } -function getReoderability(instr: Instruction): Reorderability { +function getReorderability( + instr: Instruction, + references: References +): Reorderability { switch (instr.value.kind) { case "JsxExpression": case "JsxFragment": @@ -348,6 +482,20 @@ function getReoderability(instr: Instruction): Reorderability { case "UnaryExpression": { return Reorderability.Reorderable; } + case "LoadLocal": { + const name = instr.value.place.identifier.name; + if (name !== null && name.kind === "named") { + const lastAssignment = references.lastAssignments.get(name.value); + if ( + lastAssignment !== undefined && + lastAssignment < instr.id && + references.singleUseIdentifiers.has(instr.lvalue.identifier.id) + ) { + return Reorderability.Reorderable; + } + } + return Reorderability.Nonreorderable; + } default: { return Reorderability.Nonreorderable; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts index 23a0a839ea4b1..2c9e67646b155 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -186,10 +186,7 @@ export function isMutable({ id }: Instruction, place: Place): boolean { return id >= range.start && id < range.end; } -export function mayAllocate( - env: Environment, - instruction: Instruction -): boolean { +function mayAllocate(env: Environment, instruction: Instruction): boolean { const { value } = instruction; switch (value.kind) { case "Destructure": { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-consecutive-scopes-reordering.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-consecutive-scopes-reordering.expect.md index dc49af4401613..44eaa5b8b731c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-consecutive-scopes-reordering.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-consecutive-scopes-reordering.expect.md @@ -34,59 +34,47 @@ import { useState } from "react"; import { Stringify } from "shared-runtime"; function Component() { - const $ = _c(10); + const $ = _c(7); const [state, setState] = useState(0); let t0; + let t1; if ($[0] !== state) { - t0 = () => setState(state + 1); + t0 = ( + + ); + t1 = {state}; $[0] = state; $[1] = t0; - } else { - t0 = $[1]; - } - let t1; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t1 = ; $[2] = t1; } else { + t0 = $[1]; t1 = $[2]; } let t2; - if ($[3] !== state) { - t2 = {state}; - $[3] = state; - $[4] = t2; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t2 = ; + $[3] = t2; } else { - t2 = $[4]; + t2 = $[3]; } let t3; - if ($[5] !== t0) { + if ($[4] !== t1 || $[5] !== t0) { t3 = ( - - ); - $[5] = t0; - $[6] = t3; - } else { - t3 = $[6]; - } - let t4; - if ($[7] !== t2 || $[8] !== t3) { - t4 = (
- {t1} {t2} - {t3} + {t1} + {t0}
); - $[7] = t2; - $[8] = t3; - $[9] = t4; + $[4] = t1; + $[5] = t0; + $[6] = t3; } else { - t4 = $[9]; + t3 = $[6]; } - return t4; + return t3; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-scopes-callback.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-scopes-callback.expect.md index a2e79ad67f095..49558eb0e4353 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-scopes-callback.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-scopes-callback.expect.md @@ -27,7 +27,7 @@ import { c as _c } from "react/compiler-runtime"; // @enableInstructionReorderin import { useState } from "react"; function Component() { - const $ = _c(6); + const $ = _c(4); const [state, setState] = useState(0); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { @@ -40,34 +40,26 @@ function Component() { } const onClick = t0; let t1; - if ($[1] !== state) { - t1 = Count: {state}; - $[1] = state; - $[2] = t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = ; + $[1] = t1; } else { - t1 = $[2]; + t1 = $[1]; } let t2; - if ($[3] === Symbol.for("react.memo_cache_sentinel")) { - t2 = ; - $[3] = t2; - } else { - t2 = $[3]; - } - let t3; - if ($[4] !== t1) { - t3 = ( + if ($[2] !== state) { + t2 = ( <> + Count: {state} {t1} - {t2} ); - $[4] = t1; - $[5] = t3; + $[2] = state; + $[3] = t2; } else { - t3 = $[5]; + t2 = $[3]; } - return t3; + return t2; } ``` From 454fb35065053d5572689af1a7c125a41849604a Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 21 Jun 2024 16:48:00 -0700 Subject: [PATCH 02/25] [compiler] add fixture for optimization across scopes Adds a fixture based on internal case where our current output is quite a bit more verbose than the original memoization. See the comment in the fixture for more about the heuristic we can apply. ghstack-source-id: e637a3814077559a1a56724eafe2567efed0b8d7 Pull Request resolved: https://github.com/facebook/react/pull/29998 --- .../reordering-across-blocks.expect.md | 109 ++++++++++++++++++ .../compiler/reordering-across-blocks.js | 44 +++++++ 2 files changed, 153 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reordering-across-blocks.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reordering-across-blocks.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reordering-across-blocks.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reordering-across-blocks.expect.md new file mode 100644 index 0000000000000..af32ba709dd87 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reordering-across-blocks.expect.md @@ -0,0 +1,109 @@ + +## Input + +```javascript +import { Stringify } from "shared-runtime"; + +function Component({ config }) { + /** + * The original memoization is optimal in the sense that it has + * one output (the object) and one dependency (`config`). Both + * the `a` and `b` functions will have to be recreated whenever + * `config` changes, cascading to update the object. + * + * However, we currently only consider consecutive scopes for + * merging, so we first see the `a` scope, then the `b` scope, + * and see that the output of the `a` scope is used later - + * so we don't merge these scopes, and so on. + * + * The more optimal thing would be to build a dependency graph + * of scopes so that we can see the data flow is along the lines + * of: + * + * config + * / \ + * [a] [b] + * \ / + * [object] + * + * All the scopes (shown in []) are transitively dependent on + * `config`, so they can be merged. + */ + const object = useMemo(() => { + const a = (event) => { + config?.onA?.(event); + }; + + const b = (event) => { + config?.onB?.(event); + }; + + return { + b, + a, + }; + }, [config]); + + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { Stringify } from "shared-runtime"; + +function Component(t0) { + const $ = _c(9); + const { config } = t0; + let t1; + let t2; + if ($[0] !== config) { + t2 = (event) => { + config?.onA?.(event); + }; + $[0] = config; + $[1] = t2; + } else { + t2 = $[1]; + } + const a = t2; + let t3; + if ($[2] !== config) { + t3 = (event_0) => { + config?.onB?.(event_0); + }; + $[2] = config; + $[3] = t3; + } else { + t3 = $[3]; + } + const b = t3; + let t4; + if ($[4] !== b || $[5] !== a) { + t4 = { b, a }; + $[4] = b; + $[5] = a; + $[6] = t4; + } else { + t4 = $[6]; + } + t1 = t4; + const object = t1; + let t5; + if ($[7] !== object) { + t5 = ; + $[7] = object; + $[8] = t5; + } else { + t5 = $[8]; + } + return t5; +} + +``` + +### 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/reordering-across-blocks.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reordering-across-blocks.js new file mode 100644 index 0000000000000..741eb71d526fb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reordering-across-blocks.js @@ -0,0 +1,44 @@ +import { Stringify } from "shared-runtime"; + +function Component({ config }) { + /** + * The original memoization is optimal in the sense that it has + * one output (the object) and one dependency (`config`). Both + * the `a` and `b` functions will have to be recreated whenever + * `config` changes, cascading to update the object. + * + * However, we currently only consider consecutive scopes for + * merging, so we first see the `a` scope, then the `b` scope, + * and see that the output of the `a` scope is used later - + * so we don't merge these scopes, and so on. + * + * The more optimal thing would be to build a dependency graph + * of scopes so that we can see the data flow is along the lines + * of: + * + * config + * / \ + * [a] [b] + * \ / + * [object] + * + * All the scopes (shown in []) are transitively dependent on + * `config`, so they can be merged. + */ + const object = useMemo(() => { + const a = (event) => { + config?.onA?.(event); + }; + + const b = (event) => { + config?.onB?.(event); + }; + + return { + b, + a, + }; + }, [config]); + + return ; +} From 28c1336c9105c134cba04ed937efcbe391b5979b Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Sat, 22 Jun 2024 12:33:37 -0400 Subject: [PATCH 03/25] [ci] Add new yarn test GitHub action Copies the existing circleci workflow for yarn test into GitHub actions. I didn't remove the circleci job for now just to check for parity. Opted to keep the same hardcoded list of params rather than use GitHub's matrix permutations since this was intentional in the circleci config. ghstack-source-id: b77a091254f402364c0b9de871889683fa3ee7c8 Pull Request resolved: https://github.com/facebook/react/pull/30032 --- .github/workflows/runtime_test.yml | 55 ++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 .github/workflows/runtime_test.yml diff --git a/.github/workflows/runtime_test.yml b/.github/workflows/runtime_test.yml new file mode 100644 index 0000000000000..1acce045217ee --- /dev/null +++ b/.github/workflows/runtime_test.yml @@ -0,0 +1,55 @@ +name: React Runtime (Test) + +on: + push: + branches: [main] + pull_request: + paths-ignore: + - 'compiler/**' + +jobs: + test: + name: yarn test + runs-on: ubuntu-latest + continue-on-error: true + strategy: + matrix: + # Intentionally passing these as strings instead of creating a + # separate parameter per CLI argument, since it's easier to + # control/see which combinations we want to run. + params: [ + "-r=stable --env=development", + "-r=stable --env=production", + "-r=experimental --env=development", + "-r=experimental --env=production", + "-r=www-classic --env=development --variant=false", + "-r=www-classic --env=production --variant=false", + "-r=www-classic --env=development --variant=true", + "-r=www-classic --env=production --variant=true", + "-r=www-modern --env=development --variant=false", + "-r=www-modern --env=production --variant=false", + "-r=www-modern --env=development --variant=true", + "-r=www-modern --env=production --variant=true", + "-r=xplat --env=development --variant=false", + "-r=xplat --env=development --variant=true", + "-r=xplat --env=production --variant=false", + "-r=xplat --env=production --variant=true", + # TODO: Test more persistent configurations? + "-r=stable --env=development --persistent", + "-r=experimental --env=development --persistent" + ] + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.x + cache: "yarn" + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - run: yarn test ${{ matrix.params }} --ci From 85215413cbfd5f28a4448b0d649b9198458e00e8 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Sat, 22 Jun 2024 12:33:37 -0400 Subject: [PATCH 04/25] [ci] Improve parallelism of yarn test This PR adds parallelism similar to our existing circleci setup for running yarn tests with the various test params. It does this by sharding tests into `$SHARD_COUNT` number of groups, then spawning a job for each of them and using jest's built in `--shard` option. Effectively this means that the job will spawn an additional (where `n` is the number of test params) `n * $SHARD_COUNT` number of jobs to run tests in parallel for a total of `n + (n * $SHARD_COUNT)` jobs. This does mean the GitHub UI at the bottom of each PR gets longer and unfortunately it's not sorted in any way as far as I can tell. But if something goes wrong it should still be easy to find out what the problem is. The PR also changes the `ci` argument for jest-cli to be an enum instead so the tests use all available workers in GitHub actions. This will have to live around for a bit until we can fully migrate off of circleci. ghstack-source-id: 08f2d16353aeca7cd15655d5ce2b7963a12507a7 Pull Request resolved: https://github.com/facebook/react/pull/30033 --- .circleci/config.yml | 6 +- .github/workflows/fuzz_tests.yml | 4 +- .github/workflows/runtime_test.yml | 84 +++++++++++++------ .../src/__tests__/ReactMultiChildText-test.js | 2 +- scripts/jest/jest-cli.js | 30 ++++--- 5 files changed, 81 insertions(+), 45 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 894e52fab609d..7ce9c199d4c7f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -281,7 +281,7 @@ jobs: at: . - setup_node_modules - run: ./scripts/circleci/download_devtools_regression_build.js << parameters.version >> --replaceBuild - - run: node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion << parameters.version >> --ci + - run: node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion << parameters.version >> --ci=circleci run_devtools_e2e_tests_for_versions: docker: *docker @@ -368,7 +368,7 @@ jobs: steps: - checkout - setup_node_modules - - run: yarn test <> --ci + - run: yarn test <> --ci=circleci yarn_test_build: docker: *docker @@ -382,7 +382,7 @@ jobs: - attach_workspace: at: . - setup_node_modules - - run: yarn test --build <> --ci + - run: yarn test --build <> --ci=circleci RELEASE_CHANNEL_stable_yarn_test_dom_fixtures: docker: *docker diff --git a/.github/workflows/fuzz_tests.yml b/.github/workflows/fuzz_tests.yml index 492dd7dc4c81d..4c4373be373ea 100644 --- a/.github/workflows/fuzz_tests.yml +++ b/.github/workflows/fuzz_tests.yml @@ -28,5 +28,5 @@ jobs: shell: bash - name: Run fuzz tests run: |- - FUZZ_TEST_SEED=$RANDOM yarn test fuzz --ci - FUZZ_TEST_SEED=$RANDOM yarn test --prod fuzz --ci + FUZZ_TEST_SEED=$RANDOM yarn test fuzz --ci=github + FUZZ_TEST_SEED=$RANDOM yarn test --prod fuzz --ci=github diff --git a/.github/workflows/runtime_test.yml b/.github/workflows/runtime_test.yml index 1acce045217ee..a9ec4dbc7acd1 100644 --- a/.github/workflows/runtime_test.yml +++ b/.github/workflows/runtime_test.yml @@ -7,37 +7,67 @@ on: paths-ignore: - 'compiler/**' +env: + # Number of workers (one per shard) to spawn + SHARD_COUNT: 5 + jobs: + # Define the various test parameters and parallelism for this workflow + build_test_params: + name: Build test params + runs-on: ubuntu-latest + outputs: + params: ${{ steps.define-params.outputs.result }} + shard_id: ${{ steps.define-shards.outputs.result }} + steps: + - uses: actions/github-script@v7 + id: define-shards + with: + script: | + function range(from, to) { + const arr = []; + for (let n = from; n <= to; n++) { + arr.push(n); + } + return arr; + } + return range(1, process.env.SHARD_COUNT); + - uses: actions/github-script@v7 + id: define-params + with: + script: | + return [ + "-r=stable --env=development", + "-r=stable --env=production", + "-r=experimental --env=development", + "-r=experimental --env=production", + "-r=www-classic --env=development --variant=false", + "-r=www-classic --env=production --variant=false", + "-r=www-classic --env=development --variant=true", + "-r=www-classic --env=production --variant=true", + "-r=www-modern --env=development --variant=false", + "-r=www-modern --env=production --variant=false", + "-r=www-modern --env=development --variant=true", + "-r=www-modern --env=production --variant=true", + "-r=xplat --env=development --variant=false", + "-r=xplat --env=development --variant=true", + "-r=xplat --env=production --variant=false", + "-r=xplat --env=production --variant=true", + // TODO: Test more persistent configurations? + "-r=stable --env=development --persistent", + "-r=experimental --env=development --persistent" + ]; + + # Spawn a job for each shard for a given set of test params test: - name: yarn test + name: yarn test ${{ matrix.params }} (Shard ${{ matrix.shard_id }}) runs-on: ubuntu-latest - continue-on-error: true + needs: build_test_params strategy: matrix: - # Intentionally passing these as strings instead of creating a - # separate parameter per CLI argument, since it's easier to - # control/see which combinations we want to run. - params: [ - "-r=stable --env=development", - "-r=stable --env=production", - "-r=experimental --env=development", - "-r=experimental --env=production", - "-r=www-classic --env=development --variant=false", - "-r=www-classic --env=production --variant=false", - "-r=www-classic --env=development --variant=true", - "-r=www-classic --env=production --variant=true", - "-r=www-modern --env=development --variant=false", - "-r=www-modern --env=production --variant=false", - "-r=www-modern --env=development --variant=true", - "-r=www-modern --env=production --variant=true", - "-r=xplat --env=development --variant=false", - "-r=xplat --env=development --variant=true", - "-r=xplat --env=production --variant=false", - "-r=xplat --env=production --variant=true", - # TODO: Test more persistent configurations? - "-r=stable --env=development --persistent", - "-r=experimental --env=development --persistent" - ] + params: ${{ fromJSON(needs.build_test_params.outputs.params) }} + shard_id: ${{ fromJSON(needs.build_test_params.outputs.shard_id) }} + continue-on-error: true steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 @@ -52,4 +82,4 @@ jobs: path: "**/node_modules" key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - - run: yarn test ${{ matrix.params }} --ci + - run: yarn test ${{ matrix.params }} --ci=github --shard=${{ matrix.shard_id }}/${{ env.SHARD_COUNT }} diff --git a/packages/react-dom/src/__tests__/ReactMultiChildText-test.js b/packages/react-dom/src/__tests__/ReactMultiChildText-test.js index c32d13d3b0151..5aeb321308090 100644 --- a/packages/react-dom/src/__tests__/ReactMultiChildText-test.js +++ b/packages/react-dom/src/__tests__/ReactMultiChildText-test.js @@ -77,7 +77,7 @@ const expectChildren = function (container, children) { * faster to render and update. */ describe('ReactMultiChildText', () => { - jest.setTimeout(20000); + jest.setTimeout(30000); it('should correctly handle all possible children for render and update', async () => { await expect(async () => { diff --git a/scripts/jest/jest-cli.js b/scripts/jest/jest-cli.js index 9c3be220fb645..70d4d04b2b456 100644 --- a/scripts/jest/jest-cli.js +++ b/scripts/jest/jest-cli.js @@ -91,8 +91,8 @@ const argv = yargs ci: { describe: 'Run tests in CI', requiresArg: false, - type: 'boolean', - default: false, + type: 'choices', + choices: ['circleci', 'github'], }, compactConsole: { alias: 'c', @@ -309,10 +309,14 @@ function getCommandArgs() { } // CI Environments have limited workers. - if (argv.ci) { + if (argv.ci === 'circleci') { args.push('--maxWorkers=2'); } + if (argv.ci === 'github') { + args.push('--maxConcurrency=10'); + } + // Push the remaining args onto the command. // This will send args like `--watch` to Jest. args.push(...argv._); @@ -364,16 +368,18 @@ function main() { const envars = getEnvars(); const env = Object.entries(envars).map(([k, v]) => `${k}=${v}`); - // Print the full command we're actually running. - const command = `$ ${env.join(' ')} node ${args.join(' ')}`; - console.log(chalk.dim(command)); + if (argv.ci !== 'github') { + // Print the full command we're actually running. + const command = `$ ${env.join(' ')} node ${args.join(' ')}`; + console.log(chalk.dim(command)); - // Print the release channel and project we're running for quick confirmation. - console.log( - chalk.blue( - `\nRunning tests for ${argv.project} (${argv.releaseChannel})...` - ) - ); + // Print the release channel and project we're running for quick confirmation. + console.log( + chalk.blue( + `\nRunning tests for ${argv.project} (${argv.releaseChannel})...` + ) + ); + } // Print a message that the debugger is starting just // for some extra feedback when running the debugger. From 917585d037af5476df9271c24721eb5e992929c0 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Sat, 22 Jun 2024 12:33:38 -0400 Subject: [PATCH 05/25] [ci] Remove circleci yarn_test job This was migrated to GitHub actions ghstack-source-id: adf9b2bdc7b2a59e658176b2202fe65e84bcb4af Pull Request resolved: https://github.com/facebook/react/pull/30034 --- .circleci/config.yml | 43 ------------------------------------------- 1 file changed, 43 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 7ce9c199d4c7f..872759e3edc50 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -358,18 +358,6 @@ jobs: yarn generate-inline-fizz-runtime git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false) - yarn_test: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - parameters: - args: - type: string - steps: - - checkout - - setup_node_modules - - run: yarn test <> --ci=circleci - yarn_test_build: docker: *docker environment: *environment @@ -437,37 +425,6 @@ workflows: branches: ignore: - builds/facebook-www - - yarn_test: - filters: - branches: - ignore: - - builds/facebook-www - matrix: - parameters: - args: - # Intentionally passing these as strings instead of creating a - # separate parameter per CLI argument, since it's easier to - # control/see which combinations we want to run. - - "-r=stable --env=development" - - "-r=stable --env=production" - - "-r=experimental --env=development" - - "-r=experimental --env=production" - - "-r=www-classic --env=development --variant=false" - - "-r=www-classic --env=production --variant=false" - - "-r=www-classic --env=development --variant=true" - - "-r=www-classic --env=production --variant=true" - - "-r=www-modern --env=development --variant=false" - - "-r=www-modern --env=production --variant=false" - - "-r=www-modern --env=development --variant=true" - - "-r=www-modern --env=production --variant=true" - - "-r=xplat --env=development --variant=false" - - "-r=xplat --env=development --variant=true" - - "-r=xplat --env=production --variant=false" - - "-r=xplat --env=production --variant=true" - - # TODO: Test more persistent configurations? - - '-r=stable --env=development --persistent' - - '-r=experimental --env=development --persistent' - yarn_build: filters: branches: From a7c5c07ed4f63348d8809fe5183819721856aa8f Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Sat, 22 Jun 2024 12:33:38 -0400 Subject: [PATCH 06/25] [ci] Add new fizz GitHub action Copies the existing circleci workflow for checking the inlined Fizz runtime into GitHub actions. I didn't remove the circleci job for now just to check for parity. ghstack-source-id: 09480b1a20fbdd374a6075e6ec02a07b6a516c56 Pull Request resolved: https://github.com/facebook/react/pull/30035 --- .github/workflows/fizz.yml | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 .github/workflows/fizz.yml diff --git a/.github/workflows/fizz.yml b/.github/workflows/fizz.yml new file mode 100644 index 0000000000000..71b7bb843dd5f --- /dev/null +++ b/.github/workflows/fizz.yml @@ -0,0 +1,30 @@ +name: Fizz + +on: + push: + branches: [main] + pull_request: + paths-ignore: + - 'compiler/**' + +jobs: + check_generated_fizz_runtime: + name: Confirm generated inline Fizz runtime is up to date + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.x + cache: "yarn" + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - run: | + yarn generate-inline-fizz-runtime + git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false) From 0bff0e437792e94a3757c4afd0ca1ca9d6cedba6 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Sat, 22 Jun 2024 12:33:38 -0400 Subject: [PATCH 07/25] [ci] Remove circleci check_generated_fizz_runtime job This was migrated to GitHub actions ghstack-source-id: 3b307a31101e83f4abefe8be063cfeade57bc53c Pull Request resolved: https://github.com/facebook/react/pull/30036 --- .circleci/config.yml | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 872759e3edc50..18372084d3685 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -345,19 +345,6 @@ jobs: yarn extract-errors git diff --quiet || (echo "Found unminified errors. Either update the error codes map or disable error minification for the affected build, if appropriate." && false) - check_generated_fizz_runtime: - docker: *docker - environment: *environment - steps: - - checkout - - attach_workspace: *attach_workspace - - setup_node_modules - - run: - name: Confirm generated inline Fizz runtime is up to date - command: | - yarn generate-inline-fizz-runtime - git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false) - yarn_test_build: docker: *docker environment: *environment @@ -420,11 +407,6 @@ workflows: build_and_test: unless: << pipeline.parameters.prerelease_commit_sha >> jobs: - - check_generated_fizz_runtime: - filters: - branches: - ignore: - - builds/facebook-www - yarn_build: filters: branches: From 27e9476f0aae99adc67b10ee2b78e8ee7dc61421 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Sat, 22 Jun 2024 12:33:39 -0400 Subject: [PATCH 08/25] [ci] Cleanup - Made each workflow's name consistent - Rename each workflow file with consistent naming scheme - Promote flow-ci-ghaction to flow-ci ghstack-source-id: 490b643dcdbb5f6d853815064a1287109a17d259 Pull Request resolved: https://github.com/facebook/react/pull/30037 --- ...playground.yml => compiler_playground.yml} | 2 +- .../{compiler-rust.yml => compiler_rust.yml} | 2 +- ...typescript.yml => compiler_typescript.yml} | 2 +- .github/workflows/devtools_check_repro.yml | 2 +- ...facts.yml => runtime_commit_artifacts.yml} | 2 +- .../workflows/{fizz.yml => runtime_fizz.yml} | 2 +- .../{flags.yml => runtime_flags.yml} | 2 +- .../workflows/{flow.yml => runtime_flow.yml} | 4 +-- ...{fuzz_tests.yml => runtime_fuzz_tests.yml} | 2 +- .github/workflows/runtime_test.yml | 2 +- .../workflows/{lint.yml => shared_lint.yml} | 2 +- .../workflows/{stale.yml => shared_stale.yml} | 2 +- scripts/tasks/flow-ci-ghaction.js | 30 ------------------ scripts/tasks/flow-ci.js | 31 ++++++++----------- 14 files changed, 26 insertions(+), 61 deletions(-) rename .github/workflows/{compiler-playground.yml => compiler_playground.yml} (96%) rename .github/workflows/{compiler-rust.yml => compiler_rust.yml} (98%) rename .github/workflows/{compiler-typescript.yml => compiler_typescript.yml} (98%) rename .github/workflows/{commit_artifacts.yml => runtime_commit_artifacts.yml} (99%) rename .github/workflows/{fizz.yml => runtime_fizz.yml} (97%) rename .github/workflows/{flags.yml => runtime_flags.yml} (96%) rename .github/workflows/{flow.yml => runtime_flow.yml} (92%) rename .github/workflows/{fuzz_tests.yml => runtime_fuzz_tests.yml} (96%) rename .github/workflows/{lint.yml => shared_lint.yml} (99%) rename .github/workflows/{stale.yml => shared_stale.yml} (98%) delete mode 100644 scripts/tasks/flow-ci-ghaction.js diff --git a/.github/workflows/compiler-playground.yml b/.github/workflows/compiler_playground.yml similarity index 96% rename from .github/workflows/compiler-playground.yml rename to .github/workflows/compiler_playground.yml index 5d97a2bff4cef..7f2a75d324d77 100644 --- a/.github/workflows/compiler-playground.yml +++ b/.github/workflows/compiler_playground.yml @@ -1,4 +1,4 @@ -name: Compiler Playground +name: (Compiler) Playground on: push: diff --git a/.github/workflows/compiler-rust.yml b/.github/workflows/compiler_rust.yml similarity index 98% rename from .github/workflows/compiler-rust.yml rename to .github/workflows/compiler_rust.yml index e492de11e0e33..cf911b9d476b3 100644 --- a/.github/workflows/compiler-rust.yml +++ b/.github/workflows/compiler_rust.yml @@ -1,4 +1,4 @@ -name: React Compiler (Rust) +name: (Compiler) Rust on: push: diff --git a/.github/workflows/compiler-typescript.yml b/.github/workflows/compiler_typescript.yml similarity index 98% rename from .github/workflows/compiler-typescript.yml rename to .github/workflows/compiler_typescript.yml index 71cfe426c171a..233e963bfad74 100644 --- a/.github/workflows/compiler-typescript.yml +++ b/.github/workflows/compiler_typescript.yml @@ -1,4 +1,4 @@ -name: React Compiler (TypeScript) +name: (Compiler) TypeScript on: push: diff --git a/.github/workflows/devtools_check_repro.yml b/.github/workflows/devtools_check_repro.yml index e0335e905939e..adaef6ac32bd6 100644 --- a/.github/workflows/devtools_check_repro.yml +++ b/.github/workflows/devtools_check_repro.yml @@ -1,4 +1,4 @@ -name: DevTools Check for bug repro +name: (DevTools) Check for bug repro on: issues: types: [opened, edited] diff --git a/.github/workflows/commit_artifacts.yml b/.github/workflows/runtime_commit_artifacts.yml similarity index 99% rename from .github/workflows/commit_artifacts.yml rename to .github/workflows/runtime_commit_artifacts.yml index 09d14b30baf4d..2c32281634c7c 100644 --- a/.github/workflows/commit_artifacts.yml +++ b/.github/workflows/runtime_commit_artifacts.yml @@ -1,4 +1,4 @@ -name: Commit Artifacts for Meta WWW and fbsource +name: (Runtime) Commit Artifacts for Meta WWW and fbsource on: push: diff --git a/.github/workflows/fizz.yml b/.github/workflows/runtime_fizz.yml similarity index 97% rename from .github/workflows/fizz.yml rename to .github/workflows/runtime_fizz.yml index 71b7bb843dd5f..43097728e37de 100644 --- a/.github/workflows/fizz.yml +++ b/.github/workflows/runtime_fizz.yml @@ -1,4 +1,4 @@ -name: Fizz +name: (Runtime) Fizz on: push: diff --git a/.github/workflows/flags.yml b/.github/workflows/runtime_flags.yml similarity index 96% rename from .github/workflows/flags.yml rename to .github/workflows/runtime_flags.yml index 66e6c5c211153..baf9a48242c7c 100644 --- a/.github/workflows/flags.yml +++ b/.github/workflows/runtime_flags.yml @@ -1,4 +1,4 @@ -name: Flags +name: (Runtime) Flags on: push: diff --git a/.github/workflows/flow.yml b/.github/workflows/runtime_flow.yml similarity index 92% rename from .github/workflows/flow.yml rename to .github/workflows/runtime_flow.yml index 11eb1f55e4cb7..6b8eb4b774e35 100644 --- a/.github/workflows/flow.yml +++ b/.github/workflows/runtime_flow.yml @@ -1,4 +1,4 @@ -name: Flow +name: (Runtime) Flow on: push: @@ -44,4 +44,4 @@ jobs: path: "**/node_modules" key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - - run: node ./scripts/tasks/flow-ci-ghaction ${{ matrix.flow_inline_config_shortname }} + - run: node ./scripts/tasks/flow-ci ${{ matrix.flow_inline_config_shortname }} diff --git a/.github/workflows/fuzz_tests.yml b/.github/workflows/runtime_fuzz_tests.yml similarity index 96% rename from .github/workflows/fuzz_tests.yml rename to .github/workflows/runtime_fuzz_tests.yml index 4c4373be373ea..5b31d115dcd23 100644 --- a/.github/workflows/fuzz_tests.yml +++ b/.github/workflows/runtime_fuzz_tests.yml @@ -1,4 +1,4 @@ -name: facebook/react/fuzz_tests +name: (Runtime) Fuzz tests on: schedule: - cron: 0 * * * * diff --git a/.github/workflows/runtime_test.yml b/.github/workflows/runtime_test.yml index a9ec4dbc7acd1..118a520ad59c9 100644 --- a/.github/workflows/runtime_test.yml +++ b/.github/workflows/runtime_test.yml @@ -1,4 +1,4 @@ -name: React Runtime (Test) +name: (Runtime) Test on: push: diff --git a/.github/workflows/lint.yml b/.github/workflows/shared_lint.yml similarity index 99% rename from .github/workflows/lint.yml rename to .github/workflows/shared_lint.yml index 4313712727913..ca851ff2324bf 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/shared_lint.yml @@ -1,4 +1,4 @@ -name: Lint +name: (Shared) Lint on: push: diff --git a/.github/workflows/stale.yml b/.github/workflows/shared_stale.yml similarity index 98% rename from .github/workflows/stale.yml rename to .github/workflows/shared_stale.yml index 86928739a4ea4..df9854c270a28 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/shared_stale.yml @@ -1,5 +1,5 @@ # Configuration for stale action workflow - https://github.com/actions/stale -name: 'Manage stale issues and PRs' +name: (Shared) Manage stale issues and PRs on: schedule: # Run hourly diff --git a/scripts/tasks/flow-ci-ghaction.js b/scripts/tasks/flow-ci-ghaction.js deleted file mode 100644 index f420c80c37562..0000000000000 --- a/scripts/tasks/flow-ci-ghaction.js +++ /dev/null @@ -1,30 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -'use strict'; - -process.on('unhandledRejection', err => { - throw err; -}); - -const runFlow = require('../flow/runFlow'); -const inlinedHostConfigs = require('../shared/inlinedHostConfigs'); - -async function check(shortName) { - if (shortName == null) { - throw new Error('Expected an inlinedHostConfig shortName'); - } - const rendererInfo = inlinedHostConfigs.find( - config => config.shortName === shortName - ); - if (rendererInfo.isFlowTyped) { - await runFlow(rendererInfo.shortName, ['check']); - console.log(); - } -} - -check(process.argv[2]); diff --git a/scripts/tasks/flow-ci.js b/scripts/tasks/flow-ci.js index c30e1b974bf9b..0f39d2f6ffffa 100644 --- a/scripts/tasks/flow-ci.js +++ b/scripts/tasks/flow-ci.js @@ -14,24 +14,19 @@ process.on('unhandledRejection', err => { const runFlow = require('../flow/runFlow'); const inlinedHostConfigs = require('../shared/inlinedHostConfigs'); -// Parallelize tests across multiple tasks. -const nodeTotal = process.env.CIRCLE_NODE_TOTAL - ? parseInt(process.env.CIRCLE_NODE_TOTAL, 10) - : 1; -const nodeIndex = process.env.CIRCLE_NODE_INDEX - ? parseInt(process.env.CIRCLE_NODE_INDEX, 10) - : 0; - -async function checkAll() { - for (let i = 0; i < inlinedHostConfigs.length; i++) { - if (i % nodeTotal === nodeIndex) { - const rendererInfo = inlinedHostConfigs[i]; - if (rendererInfo.isFlowTyped) { - await runFlow(rendererInfo.shortName, ['check']); - console.log(); - } - } +async function check(shortName) { + if (shortName == null) { + throw new Error('Expected an inlinedHostConfig shortName'); + } + const rendererInfo = inlinedHostConfigs.find( + config => config.shortName === shortName + ); + if (rendererInfo == null) { + throw new Error(`Could not find inlinedHostConfig for ${shortName}`); + } + if (rendererInfo.isFlowTyped) { + await runFlow(rendererInfo.shortName, ['check']); } } -checkAll(); +check(process.argv[2]); From 7608516479fb85bc40aa73d8a31a0c93397ee6ff Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 24 Jun 2024 13:28:17 +0100 Subject: [PATCH 09/25] refactor[react-devtools/extensions]: dont debounce cleanup logic on navigation (#30027) ## Summary There is a race condition in the way we poll if React is on the page and when we actually clear this polling instance. When user navigates to a different page, we will debounce a callback for 500ms, which will: 1. Cleanup previous React polling instance 2. Start a new React polling instance Since the cleanup logic is debounced, there is a small chance that by the time we are going to clean up this polling instance, it will be `eval`-ed on the page, that is using React. For example, when user is navigating from the page which doesn't have React running, to a page that has React running. Next, we incorrectly will try to mount React DevTools panels twice, which will result into conflicts in the Store, and the error will be shown to the user ## How did you test this change? Since this is a race condition, it is hard to reproduce consistently, but you can try this flow: 1. Open a page that is using React, open browser DevTools and React DevTools components panel 2. Open a page that is NOT using React, like google.com, wait ~5 seconds until you see `"Looks like this page doesn't have React, or it hasn't been loaded yet"` message in RDT panel 3. Open a page that is using React, observe the error `"Uncaught Error: Cannot add node "1" because a node with that id is already in the Store."` Couldn't been able to reproduce this with these changes. --- .../react-devtools-extensions/src/main/index.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/react-devtools-extensions/src/main/index.js b/packages/react-devtools-extensions/src/main/index.js index 5422567db79fc..8ed84dd8fb5e0 100644 --- a/packages/react-devtools-extensions/src/main/index.js +++ b/packages/react-devtools-extensions/src/main/index.js @@ -412,13 +412,19 @@ chrome.devtools.network.onNavigated.addListener(syncSavedPreferences); // into subscribing to the same events from Bridge and window multiple times // In this case, we will handle `operations` event twice or more and user will see // `Cannot add node "1" because a node with that id is already in the Store.` -const debouncedOnNavigatedListener = debounce(() => { +const debouncedMountReactDevToolsCallback = debounce( + mountReactDevToolsWhenReactHasLoaded, + 500, +); + +// Clean up everything, but start mounting React DevTools panels if user stays at this page +function onNavigatedToOtherPage() { performInTabNavigationCleanup(); - mountReactDevToolsWhenReactHasLoaded(); -}, 500); + debouncedMountReactDevToolsCallback(); +} // Cleanup previous page state and remount everything -chrome.devtools.network.onNavigated.addListener(debouncedOnNavigatedListener); +chrome.devtools.network.onNavigated.addListener(onNavigatedToOtherPage); // Should be emitted when browser DevTools are closed if (__IS_FIREFOX__) { From c21bcd627b6a8f31548edfc149dd3b879fea6558 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Mon, 24 Jun 2024 11:18:22 -0400 Subject: [PATCH 10/25] Clean up enableUnifiedSyncLane flag (#30062) `enableUnifiedSyncLane` now passes everywhere. Let's clean it up Implemented with https://github.com/facebook/react/pull/27646 Flag enabled with https://github.com/facebook/react/pull/27646, https://github.com/facebook/react/pull/28269, https://github.com/facebook/react/pull/29052 --- .../src/__tests__/ReactDOMFiberAsync-test.js | 15 +- .../react-reconciler/src/ReactFiberLane.js | 16 +- .../src/__tests__/Activity-test.js | 9 +- .../__tests__/ReactBatching-test.internal.js | 14 +- .../ReactClassSetStateCallback-test.js | 8 +- .../src/__tests__/ReactFlushSync-test.js | 11 +- .../src/__tests__/ReactHooks-test.internal.js | 9 +- .../ReactHooksWithNoopRenderer-test.js | 17 +- .../__tests__/ReactIncrementalUpdates-test.js | 189 +++--------------- .../ReactSuspenseWithNoopRenderer-test.js | 4 +- .../src/__tests__/ReactTransition-test.js | 29 +-- packages/shared/ReactFeatureFlags.js | 2 - .../forks/ReactFeatureFlags.native-fb.js | 1 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 1 - ...actFeatureFlags.test-renderer.native-fb.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../forks/ReactFeatureFlags.www-dynamic.js | 1 - .../shared/forks/ReactFeatureFlags.www.js | 1 - .../src/__tests__/useSubscription-test.js | 8 +- 20 files changed, 62 insertions(+), 276 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index cf47e867da7f7..e161eb85aa194 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js @@ -313,21 +313,12 @@ describe('ReactDOMFiberAsync', () => { assertLog([]); }); // Only the active updates have flushed - if (gate(flags => flags.enableUnifiedSyncLane)) { - expect(container.textContent).toEqual('ABC'); - assertLog(['ABC']); - } else { - expect(container.textContent).toEqual('BC'); - assertLog(['BC']); - } + expect(container.textContent).toEqual('ABC'); + assertLog(['ABC']); await act(() => { instance.push('D'); - if (gate(flags => flags.enableUnifiedSyncLane)) { - expect(container.textContent).toEqual('ABC'); - } else { - expect(container.textContent).toEqual('BC'); - } + expect(container.textContent).toEqual('ABC'); assertLog([]); }); assertLog(['ABCD']); diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js index cde23ef9a6249..7b81c406cf5e8 100644 --- a/packages/react-reconciler/src/ReactFiberLane.js +++ b/packages/react-reconciler/src/ReactFiberLane.js @@ -23,7 +23,6 @@ import { enableRetryLaneExpiration, enableSchedulingProfiler, enableTransitionTracing, - enableUnifiedSyncLane, enableUpdaterTracking, syncLaneExpirationMs, transitionLaneExpirationMs, @@ -51,9 +50,8 @@ export const InputContinuousLane: Lane = /* */ 0b0000000000000000000 export const DefaultHydrationLane: Lane = /* */ 0b0000000000000000000000000010000; export const DefaultLane: Lane = /* */ 0b0000000000000000000000000100000; -export const SyncUpdateLanes: Lane = enableUnifiedSyncLane - ? SyncLane | InputContinuousLane | DefaultLane - : SyncLane; +export const SyncUpdateLanes: Lane = + SyncLane | InputContinuousLane | DefaultLane; const TransitionHydrationLane: Lane = /* */ 0b0000000000000000000000001000000; const TransitionLanes: Lanes = /* */ 0b0000000001111111111111110000000; @@ -151,11 +149,9 @@ let nextTransitionLane: Lane = TransitionLane1; let nextRetryLane: Lane = RetryLane1; function getHighestPriorityLanes(lanes: Lanes | Lane): Lanes { - if (enableUnifiedSyncLane) { - const pendingSyncLanes = lanes & SyncUpdateLanes; - if (pendingSyncLanes !== 0) { - return pendingSyncLanes; - } + const pendingSyncLanes = lanes & SyncUpdateLanes; + if (pendingSyncLanes !== 0) { + return pendingSyncLanes; } switch (getHighestPriorityLane(lanes)) { case SyncHydrationLane: @@ -826,7 +822,7 @@ export function getBumpedLaneForHydration( const renderLane = getHighestPriorityLane(renderLanes); let lane; - if (enableUnifiedSyncLane && (renderLane & SyncUpdateLanes) !== NoLane) { + if ((renderLane & SyncUpdateLanes) !== NoLane) { lane = SyncHydrationLane; } else { switch (renderLane) { diff --git a/packages/react-reconciler/src/__tests__/Activity-test.js b/packages/react-reconciler/src/__tests__/Activity-test.js index c12d49a85ce76..e174c7c06ca76 100644 --- a/packages/react-reconciler/src/__tests__/Activity-test.js +++ b/packages/react-reconciler/src/__tests__/Activity-test.js @@ -698,15 +698,10 @@ describe('Activity', () => { ); // Before the inner update can finish, we receive another pair of updates. - if (gate(flags => flags.enableUnifiedSyncLane)) { - React.startTransition(() => { - setOuter(2); - setInner(2); - }); - } else { + React.startTransition(() => { setOuter(2); setInner(2); - } + }); // Also, before either of these new updates are processed, the hidden // tree is revealed at high priority. diff --git a/packages/react-reconciler/src/__tests__/ReactBatching-test.internal.js b/packages/react-reconciler/src/__tests__/ReactBatching-test.internal.js index 4d2e8f516c01e..7f68dcc1b6158 100644 --- a/packages/react-reconciler/src/__tests__/ReactBatching-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactBatching-test.internal.js @@ -159,17 +159,7 @@ describe('ReactBlockingMode', () => { ); // Now flush the first update - if (gate(flags => flags.enableUnifiedSyncLane)) { - assertLog(['A1', 'B1']); - expect(root).toMatchRenderedOutput('A1B1'); - } else { - // Only the second update should have flushed synchronously - assertLog(['B1']); - expect(root).toMatchRenderedOutput('A0B1'); - - // Now flush the first update - await waitForAll(['A1']); - expect(root).toMatchRenderedOutput('A1B1'); - } + assertLog(['A1', 'B1']); + expect(root).toMatchRenderedOutput('A1B1'); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactClassSetStateCallback-test.js b/packages/react-reconciler/src/__tests__/ReactClassSetStateCallback-test.js index c9e0a36d0cb49..e0533df2bcdae 100644 --- a/packages/react-reconciler/src/__tests__/ReactClassSetStateCallback-test.js +++ b/packages/react-reconciler/src/__tests__/ReactClassSetStateCallback-test.js @@ -39,13 +39,9 @@ describe('ReactClassSetStateCallback', () => { assertLog([0]); await act(() => { - if (gate(flags => flags.enableUnifiedSyncLane)) { - React.startTransition(() => { - app.setState({step: 1}, () => Scheduler.log('Callback 1')); - }); - } else { + React.startTransition(() => { app.setState({step: 1}, () => Scheduler.log('Callback 1')); - } + }); ReactNoop.flushSync(() => { app.setState({step: 2}, () => Scheduler.log('Callback 2')); }); diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js index 3f8c62a42d1c9..c96c381a617a0 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js @@ -102,20 +102,13 @@ describe('ReactFlushSync', () => { // The passive effect will schedule a sync update and a normal update. // They should commit in two separate batches. First the sync one. - await waitForPaint( - gate(flags => flags.enableUnifiedSyncLane) ? ['1, 1'] : ['1, 0'], - ); + await waitForPaint(['1, 1']); // The remaining update is not sync ReactDOM.flushSync(); assertLog([]); - if (gate(flags => flags.enableUnifiedSyncLane)) { - await waitForPaint([]); - } else { - // Now flush it. - await waitForPaint(['1, 1']); - } + await waitForPaint([]); }); expect(getVisibleChildren(container)).toEqual('1, 1'); diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 1aa10459ecbfb..9b714458c74d5 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -541,13 +541,8 @@ describe('ReactHooks', () => { }); }; - if (gate(flags => flags.enableUnifiedSyncLane)) { - // Update at transition priority - React.startTransition(() => update(n => n * 100)); - } else { - // Update at normal priority - ReactTestRenderer.unstable_batchedUpdates(() => update(n => n * 100)); - } + // Update at transition priority + React.startTransition(() => update(n => n * 100)); // The new state is eagerly computed. assertLog(['Compute state (1 -> 100)']); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 04437d7047979..1b67266b04af5 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -899,15 +899,8 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.flushSync(() => { counter.current.dispatch(INCREMENT); }); - if (gate(flags => flags.enableUnifiedSyncLane)) { - assertLog(['Count: 4']); - expect(ReactNoop).toMatchRenderedOutput(); - } else { - assertLog(['Count: 1']); - expect(ReactNoop).toMatchRenderedOutput(); - await waitForAll(['Count: 4']); - expect(ReactNoop).toMatchRenderedOutput(); - } + assertLog(['Count: 4']); + expect(ReactNoop).toMatchRenderedOutput(); }); }); @@ -1613,11 +1606,7 @@ describe('ReactHooksWithNoopRenderer', () => { // As a result we, somewhat surprisingly, commit them in the opposite order. // This should be fine because any non-discrete set of work doesn't guarantee order // and easily could've happened slightly later too. - if (gate(flags => flags.enableUnifiedSyncLane)) { - assertLog(['Will set count to 1', 'Count: 1']); - } else { - assertLog(['Will set count to 1', 'Count: 2', 'Count: 1']); - } + assertLog(['Will set count to 1', 'Count: 1']); expect(ReactNoop).toMatchRenderedOutput(); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js index f3d5cbc675c52..4d4ab4f7c929e 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js @@ -156,23 +156,11 @@ describe('ReactIncrementalUpdates', () => { } // Schedule some async updates - if ( - gate( - flags => - !flags.forceConcurrentByDefaultForTesting || - flags.enableUnifiedSyncLane, - ) - ) { - React.startTransition(() => { - instance.setState(createUpdate('a')); - instance.setState(createUpdate('b')); - instance.setState(createUpdate('c')); - }); - } else { + React.startTransition(() => { instance.setState(createUpdate('a')); instance.setState(createUpdate('b')); instance.setState(createUpdate('c')); - } + }); // Begin the updates but don't flush them yet await waitFor(['a', 'b', 'c']); @@ -189,58 +177,22 @@ describe('ReactIncrementalUpdates', () => { }); // The sync updates should have flushed, but not the async ones. - if ( - gate( - flags => - !flags.forceConcurrentByDefaultForTesting && - flags.enableUnifiedSyncLane, - ) - ) { - assertLog(['d', 'e', 'f']); - expect(ReactNoop).toMatchRenderedOutput(); - } else { - // Update d was dropped and replaced by e. - assertLog(['e', 'f']); - expect(ReactNoop).toMatchRenderedOutput(); - } + assertLog(['d', 'e', 'f']); + expect(ReactNoop).toMatchRenderedOutput(); // Now flush the remaining work. Even though e and f were already processed, // they should be processed again, to ensure that the terminal state // is deterministic. - if ( - gate( - flags => - !flags.forceConcurrentByDefaultForTesting && - !flags.enableUnifiedSyncLane, - ) - ) { - await waitForAll([ - // Since 'g' is in a transition, we'll process 'd' separately first. - // That causes us to process 'd' with 'e' and 'f' rebased. - 'd', - 'e', - 'f', - // Then we'll re-process everything for 'g'. - 'a', - 'b', - 'c', - 'd', - 'e', - 'f', - 'g', - ]); - } else { - await waitForAll([ - // Then we'll re-process everything for 'g'. - 'a', - 'b', - 'c', - 'd', - 'e', - 'f', - 'g', - ]); - } + await waitForAll([ + // Then we'll re-process everything for 'g'. + 'a', + 'b', + 'c', + 'd', + 'e', + 'f', + 'g', + ]); expect(ReactNoop).toMatchRenderedOutput(); }); @@ -267,23 +219,11 @@ describe('ReactIncrementalUpdates', () => { } // Schedule some async updates - if ( - gate( - flags => - !flags.forceConcurrentByDefaultForTesting || - flags.enableUnifiedSyncLane, - ) - ) { - React.startTransition(() => { - instance.setState(createUpdate('a')); - instance.setState(createUpdate('b')); - instance.setState(createUpdate('c')); - }); - } else { + React.startTransition(() => { instance.setState(createUpdate('a')); instance.setState(createUpdate('b')); instance.setState(createUpdate('c')); - } + }); // Begin the updates but don't flush them yet await waitFor(['a', 'b', 'c']); @@ -303,57 +243,22 @@ describe('ReactIncrementalUpdates', () => { }); // The sync updates should have flushed, but not the async ones. - if ( - gate( - flags => - !flags.forceConcurrentByDefaultForTesting && - flags.enableUnifiedSyncLane, - ) - ) { - assertLog(['d', 'e', 'f']); - } else { - // Update d was dropped and replaced by e. - assertLog(['e', 'f']); - } + assertLog(['d', 'e', 'f']); expect(ReactNoop).toMatchRenderedOutput(); // Now flush the remaining work. Even though e and f were already processed, // they should be processed again, to ensure that the terminal state // is deterministic. - if ( - gate( - flags => - !flags.forceConcurrentByDefaultForTesting && - !flags.enableUnifiedSyncLane, - ) - ) { - await waitForAll([ - // Since 'g' is in a transition, we'll process 'd' separately first. - // That causes us to process 'd' with 'e' and 'f' rebased. - 'd', - 'e', - 'f', - // Then we'll re-process everything for 'g'. - 'a', - 'b', - 'c', - 'd', - 'e', - 'f', - 'g', - ]); - } else { - await waitForAll([ - // Then we'll re-process everything for 'g'. - 'a', - 'b', - 'c', - 'd', - 'e', - 'f', - 'g', - ]); - } + await waitForAll([ + // Then we'll re-process everything for 'g'. + 'a', + 'b', + 'c', + 'd', + 'e', + 'f', + 'g', + ]); expect(ReactNoop).toMatchRenderedOutput(); }); @@ -684,25 +589,7 @@ describe('ReactIncrementalUpdates', () => { pushToLog('B'), ); }); - if (gate(flags => flags.enableUnifiedSyncLane)) { - assertLog(['Committed: B', 'Committed: BCD', 'Committed: ABCD']); - } else { - assertLog([ - // A and B are pending. B is higher priority, so we'll render that first. - 'Committed: B', - // Because A comes first in the queue, we're now in rebase mode. B must - // be rebased on top of A. Also, in a layout effect, we received two new - // updates: C and D. C is user-blocking and D is synchronous. - // - // First render the synchronous update. What we're testing here is that - // B *is not dropped* even though it has lower than sync priority. That's - // because we already committed it. However, this render should not - // include C, because that update wasn't already committed. - 'Committed: BD', - 'Committed: BCD', - 'Committed: ABCD', - ]); - } + assertLog(['Committed: B', 'Committed: BCD', 'Committed: ABCD']); expect(root).toMatchRenderedOutput('ABCD'); }); @@ -744,25 +631,7 @@ describe('ReactIncrementalUpdates', () => { pushToLog('B'), ); }); - if (gate(flags => flags.enableUnifiedSyncLane)) { - assertLog(['Committed: B', 'Committed: BCD', 'Committed: ABCD']); - } else { - assertLog([ - // A and B are pending. B is higher priority, so we'll render that first. - 'Committed: B', - // Because A comes first in the queue, we're now in rebase mode. B must - // be rebased on top of A. Also, in a layout effect, we received two new - // updates: C and D. C is user-blocking and D is synchronous. - // - // First render the synchronous update. What we're testing here is that - // B *is not dropped* even though it has lower than sync priority. That's - // because we already committed it. However, this render should not - // include C, because that update wasn't already committed. - 'Committed: BD', - 'Committed: BCD', - 'Committed: ABCD', - ]); - } + assertLog(['Committed: B', 'Committed: BCD', 'Committed: ABCD']); expect(root).toMatchRenderedOutput('ABCD'); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index fd37cdac69934..3249a42368f6a 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -3506,7 +3506,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // @gate enableLegacyCache - // @gate forceConcurrentByDefaultForTesting it('regression: ping at high priority causes update to be dropped', async () => { const {useState, useTransition} = React; @@ -3573,10 +3572,9 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); await waitFor([ - 'B', 'Suspend! [A1]', 'Loading...', - + 'B', 'Suspend! [A2]', 'Loading...', 'Suspend! [B2]', diff --git a/packages/react-reconciler/src/__tests__/ReactTransition-test.js b/packages/react-reconciler/src/__tests__/ReactTransition-test.js index c466fb615c54c..9dcb36c148b9c 100644 --- a/packages/react-reconciler/src/__tests__/ReactTransition-test.js +++ b/packages/react-reconciler/src/__tests__/ReactTransition-test.js @@ -925,28 +925,15 @@ describe('ReactTransition', () => { updateNormalPri(); }); - if (gate(flags => flags.enableUnifiedSyncLane)) { - assertLog([ - 'Normal pri: 0', - 'Commit', - - // Normal pri update. - 'Transition pri: 1', - 'Normal pri: 1', - 'Commit', - ]); - } else { - assertLog([ - // Finish transition update. - 'Normal pri: 0', - 'Commit', + assertLog([ + 'Normal pri: 0', + 'Commit', - // Normal pri update. - 'Transition pri: 1', - 'Normal pri: 1', - 'Commit', - ]); - } + // Normal pri update. + 'Transition pri: 1', + 'Normal pri: 1', + 'Commit', + ]); expect(root).toMatchRenderedOutput('Transition pri: 1, Normal pri: 1'); }); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 8b2d0800cb933..b161ac7cb1141 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -210,8 +210,6 @@ export const enableUseDeferredValueInitialArg = true; // Enables time slicing for updates that aren't wrapped in startTransition. export const forceConcurrentByDefaultForTesting = false; -export const enableUnifiedSyncLane = true; - // Adds an opt-in to time slicing for updates that aren't wrapped in startTransition. export const allowConcurrentByDefault = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index a9e2b146e1bc3..de6086c33f257 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -79,7 +79,6 @@ export const enableSuspenseCallback = false; export const enableTaint = true; export const enableTransitionTracing = false; export const enableTrustedTypesIntegration = false; -export const enableUnifiedSyncLane = true; export const enableUpdaterTracking = __PROFILE__; export const enableUseDeferredValueInitialArg = true; export const enableUseEffectEventHook = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 88e49d2bc94a3..e47a79ef7abb8 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -71,7 +71,6 @@ export const enableSuspenseCallback = false; export const enableTaint = true; export const enableTransitionTracing = false; export const enableTrustedTypesIntegration = false; -export const enableUnifiedSyncLane = true; export const enableUseDeferredValueInitialArg = true; export const enableUseEffectEventHook = false; export const enableUseMemoCacheHook = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index e40351ae1fcf4..717e16b88060c 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -54,7 +54,6 @@ export const disableSchedulerTimeoutInWorkLoop = false; export const enableLazyContextPropagation = false; export const enableLegacyHidden = false; export const forceConcurrentByDefaultForTesting = false; -export const enableUnifiedSyncLane = __EXPERIMENTAL__; export const allowConcurrentByDefault = false; export const consoleManagedByDevToolsDuringStrictMode = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index ec4a96f9c868f..63fa1a451459a 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -66,7 +66,6 @@ export const enableSuspenseCallback = false; export const enableTaint = true; export const enableTransitionTracing = false; export const enableTrustedTypesIntegration = false; -export const enableUnifiedSyncLane = true; export const enableUpdaterTracking = false; export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; export const enableUseEffectEventHook = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 2bd4e0798911c..71f533f8d6d2a 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -56,7 +56,6 @@ export const disableSchedulerTimeoutInWorkLoop = false; export const enableLazyContextPropagation = false; export const enableLegacyHidden = false; export const forceConcurrentByDefaultForTesting = false; -export const enableUnifiedSyncLane = true; export const allowConcurrentByDefault = true; export const consoleManagedByDevToolsDuringStrictMode = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 4ec863c7bbb84..08f8041f2560d 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -16,7 +16,6 @@ export const disableSchedulerTimeoutInWorkLoop = __VARIANT__; export const enableLazyContextPropagation = __VARIANT__; export const forceConcurrentByDefaultForTesting = __VARIANT__; -export const enableUnifiedSyncLane = __VARIANT__; export const enableTransitionTracing = __VARIANT__; export const enableDeferRootSchedulingToMicrotask = __VARIANT__; export const alwaysThrottleRetries = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 9404b877f532b..04ab3405e3cd3 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -18,7 +18,6 @@ export const { enableTrustedTypesIntegration, enableDebugTracing, enableLazyContextPropagation, - enableUnifiedSyncLane, enableRetryLaneExpiration, enableTransitionTracing, enableDeferRootSchedulingToMicrotask, diff --git a/packages/use-subscription/src/__tests__/useSubscription-test.js b/packages/use-subscription/src/__tests__/useSubscription-test.js index d54d806354b68..4ecc405789f91 100644 --- a/packages/use-subscription/src/__tests__/useSubscription-test.js +++ b/packages/use-subscription/src/__tests__/useSubscription-test.js @@ -434,13 +434,9 @@ describe('useSubscription', () => { observableA.next('a-2'); // Update again - if (gate(flags => flags.enableUnifiedSyncLane)) { - React.startTransition(() => { - root.render(); - }); - } else { + React.startTransition(() => { root.render(); - } + }); // Flush everything and ensure that the correct subscribable is used await waitForAll([ From 89580f209ce68ae9e266e309dfeb1625b434fb58 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Mon, 24 Jun 2024 12:28:39 -0400 Subject: [PATCH 11/25] Set renameElementSymbol to dynamic value (#30066) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prepare to roll this out with dynamic flag `yarn flags --diff www canary` Screenshot 2024-06-24 at 11 33 55 AM --- packages/react-dom/src/__tests__/ReactDOMOption-test.js | 2 +- packages/shared/forks/ReactFeatureFlags.www-dynamic.js | 1 + packages/shared/forks/ReactFeatureFlags.www.js | 3 +-- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMOption-test.js b/packages/react-dom/src/__tests__/ReactDOMOption-test.js index ee54bac0c3915..dab7f69b27e22 100644 --- a/packages/react-dom/src/__tests__/ReactDOMOption-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMOption-test.js @@ -134,7 +134,7 @@ describe('ReactDOMOption', () => { }).rejects.toThrow('Objects are not valid as a React child'); }); - // @gate www + // @gate www && !renameElementSymbol it('should support element-ish child', async () => { // This is similar to . // We don't toString it because you must instead provide a value prop. diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 08f8041f2560d..07c50c7a3ecec 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -32,6 +32,7 @@ export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; export const enableAddPropertiesFastPath = __VARIANT__; export const disableLegacyMode = __VARIANT__; +export const renameElementSymbol = __VARIANT__; // Enable this flag to help with concurrent mode debugging. // It logs information to the console about React scheduling, rendering, and commit phases. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 04ab3405e3cd3..66a3723071986 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -35,6 +35,7 @@ export const { enableNoCloningMemoCache, enableAddPropertiesFastPath, enableFastJSX, + renameElementSymbol, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build. @@ -65,8 +66,6 @@ export const enableSchedulingProfiler: boolean = export const disableLegacyContext = __EXPERIMENTAL__; export const enableGetInspectorDataForInstanceInProduction = false; -export const renameElementSymbol = false; - export const enableCache = true; export const enableLegacyCache = true; From f5d2feb4f069a36140d5e605f5eebc52badcc214 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 24 Jun 2024 10:11:14 -0700 Subject: [PATCH 12/25] [compiler] Fix assignment within for update expression When converting value blocks from HIR to ReactiveFunction, we have to drop StoreLocal assignments that represent the assignment of the phi, since ReactiveFunction supports compound expressions. These StoreLocals are only present to represent the conditional assignment of the value itself - but it's also possible for the expression to have contained an assignment expression. Before, in trying to strip the first category of StoreLocal we also accidentally stripped the second category. Now we check that the assignment is for a temporary, and don't strip otherwise. ghstack-source-id: e7759c963bbc1bbff2d3230534b049199e3262ad Pull Request resolved: https://github.com/facebook/react/pull/30067 --- .../ReactiveScopes/BuildReactiveFunction.ts | 44 +++++++++++++---- .../for-with-assignment-as-update.expect.md | 49 +++++++++++++++++++ .../compiler/for-with-assignment-as-update.js | 12 +++++ 3 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts index ebc1d63c6d881..4a5176c7d6ed9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts @@ -921,14 +921,26 @@ class Driver { }); } else if (defaultBlock.instructions.length === 1) { const instr = defaultBlock.instructions[0]!; - let place: Place = instr.lvalue!; + let place: Place = instr.lvalue; let value: ReactiveValue = instr.value; - if (instr.value.kind === "StoreLocal") { - place = instr.value.lvalue.place; + if ( + /* + * Value blocks generally end in a StoreLocal to assign the value of the + * expression for this branch. These StoreLocal instructions can be pruned, + * since we represent the value blocks as a compund value in ReactiveFunction + * (no phis). However, it's also possible to have a value block that ends in + * an AssignmentExpression, which we need to keep. So we only prune + * StoreLocal for temporaries — any named/promoted values must be used + * elsewhere and aren't safe to prune. + */ + value.kind === "StoreLocal" && + value.lvalue.place.identifier.name === null + ) { + place = value.lvalue.place; value = { kind: "LoadLocal", - place: instr.value.value, - loc: instr.value.value.loc, + place: value.value, + loc: value.value.loc, }; } return { @@ -939,14 +951,26 @@ class Driver { }; } else { const instr = defaultBlock.instructions.at(-1)!; - let place: Place = instr.lvalue!; + let place: Place = instr.lvalue; let value: ReactiveValue = instr.value; - if (instr.value.kind === "StoreLocal") { - place = instr.value.lvalue.place; + if ( + /* + * Value blocks generally end in a StoreLocal to assign the value of the + * expression for this branch. These StoreLocal instructions can be pruned, + * since we represent the value blocks as a compund value in ReactiveFunction + * (no phis). However, it's also possible to have a value block that ends in + * an AssignmentExpression, which we need to keep. So we only prune + * StoreLocal for temporaries — any named/promoted values must be used + * elsewhere and aren't safe to prune. + */ + value.kind === "StoreLocal" && + value.lvalue.place.identifier.name === null + ) { + place = value.lvalue.place; value = { kind: "LoadLocal", - place: instr.value.value, - loc: instr.value.value.loc, + place: value.value, + loc: value.value.loc, }; } const sequence: ReactiveSequenceValue = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md new file mode 100644 index 0000000000000..6d44cb418701d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md @@ -0,0 +1,49 @@ + +## Input + +```javascript +function Component(props) { + let x = props.init; + for (let i = 0; i < 100; i = i + 1) { + x += i; + } + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ init: 0 }], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component(props) { + const $ = _c(2); + let x = props.init; + for (let i = 0; i < 100; i = i + 1) { + x = x + i; + } + let t0; + if ($[0] !== x) { + t0 = [x]; + $[0] = x; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ init: 0 }], +}; + +``` + +### Eval output +(kind: ok) [4950] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js new file mode 100644 index 0000000000000..99a7e75d110fe --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js @@ -0,0 +1,12 @@ +function Component(props) { + let x = props.init; + for (let i = 0; i < 100; i = i + 1) { + x += i; + } + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ init: 0 }], +}; From 8971381549a80d476b07639833e6b38eaaa75a40 Mon Sep 17 00:00:00 2001 From: Sathya Gunasekaran Date: Tue, 25 Jun 2024 17:01:43 +0100 Subject: [PATCH 13/25] [compiler] Enable sourceMaps in tsconfig (#30064) With this, we can set a `debugger` breakpoint and we'll break into the source code when running tests with snap. Without this, we'd break into the transpiled js code. --- compiler/packages/snap/src/runner-watch.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/packages/snap/src/runner-watch.ts b/compiler/packages/snap/src/runner-watch.ts index 414d99084c52e..d6c7ae0601c53 100644 --- a/compiler/packages/snap/src/runner-watch.ts +++ b/compiler/packages/snap/src/runner-watch.ts @@ -27,7 +27,7 @@ export function watchSrc( const host = ts.createWatchCompilerHost( configPath, ts.convertCompilerOptionsFromJson( - { module: "commonjs", outDir: "dist" }, + { module: "commonjs", outDir: "dist", sourceMap: true }, "." ).options, ts.sys, From d17f024681ba9f1ebed65969b34cbee815c6d771 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 25 Jun 2024 12:42:06 -0400 Subject: [PATCH 14/25] Bump version to 0.0.0-experimental-696af53-20240625 --- compiler/packages/babel-plugin-react-compiler/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/packages/babel-plugin-react-compiler/package.json b/compiler/packages/babel-plugin-react-compiler/package.json index e1f474149004e..d152768b710d7 100644 --- a/compiler/packages/babel-plugin-react-compiler/package.json +++ b/compiler/packages/babel-plugin-react-compiler/package.json @@ -1,6 +1,6 @@ { "name": "babel-plugin-react-compiler", - "version": "0.0.0-experimental-179941d-20240614", + "version": "0.0.0-experimental-696af53-20240625", "description": "Babel plugin for React Compiler.", "main": "dist/index.js", "license": "MIT", From bdb355f349852e529a7fde06ab12418913ac4ef3 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 25 Jun 2024 12:42:06 -0400 Subject: [PATCH 15/25] Bump version to 0.0.0-experimental-0998c1e-20240625 --- compiler/packages/eslint-plugin-react-compiler/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/packages/eslint-plugin-react-compiler/package.json b/compiler/packages/eslint-plugin-react-compiler/package.json index 2a3151133491b..dcf78c77513a9 100644 --- a/compiler/packages/eslint-plugin-react-compiler/package.json +++ b/compiler/packages/eslint-plugin-react-compiler/package.json @@ -1,6 +1,6 @@ { "name": "eslint-plugin-react-compiler", - "version": "0.0.0-experimental-8ae29d2-20240614", + "version": "0.0.0-experimental-0998c1e-20240625", "description": "ESLint plugin to display errors found by the React compiler.", "main": "dist/index.js", "scripts": { From f6b546737ec58ae3a5ed591e3ed9bae73581e1dd Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 25 Jun 2024 12:42:06 -0400 Subject: [PATCH 16/25] Bump version to 0.0.0-experimental-b130d5f-20240625 --- compiler/packages/react-compiler-healthcheck/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/packages/react-compiler-healthcheck/package.json b/compiler/packages/react-compiler-healthcheck/package.json index 790d17a641a0b..eff9cb2ef1117 100644 --- a/compiler/packages/react-compiler-healthcheck/package.json +++ b/compiler/packages/react-compiler-healthcheck/package.json @@ -1,6 +1,6 @@ { "name": "react-compiler-healthcheck", - "version": "0.0.0-experimental-c20572a-20240614", + "version": "0.0.0-experimental-b130d5f-20240625", "description": "Health check script to test violations of the rules of react.", "bin": { "react-compiler-healthcheck": "dist/index.js" From 708d8f8c495e2456b91de96f3d20248693ee9ce7 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 25 Jun 2024 13:57:58 -0400 Subject: [PATCH 17/25] [ez] Remove outdated files These are no longer in use ghstack-source-id: 075e4fd0ab6140d49857a2086700b1ca9569b626 Pull Request resolved: https://github.com/facebook/react/pull/30092 --- .../packages/snap/src/SproutTodoFilter.ts | 1 - .../scripts/build-packages-forget-feedback.sh | 39 ------------------- compiler/scripts/copyright.js | 3 -- 3 files changed, 43 deletions(-) delete mode 100755 compiler/scripts/build-packages-forget-feedback.sh diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index b36e8064a7876..e193fcb753b98 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -444,7 +444,6 @@ const skipFilter = new Set([ "loop-unused-let", "reanimated-no-memo-arg", - // Tested e2e in forget-feedback repo "userspace-use-memo-cache", "transitive-freeze-function-expressions", diff --git a/compiler/scripts/build-packages-forget-feedback.sh b/compiler/scripts/build-packages-forget-feedback.sh deleted file mode 100755 index 8458d7d47ae77..0000000000000 --- a/compiler/scripts/build-packages-forget-feedback.sh +++ /dev/null @@ -1,39 +0,0 @@ -#!/bin/sh -# Copyright (c) Meta Platforms, Inc. and affiliates. -# -# This source code is licensed under the MIT license found in the -# LICENSE file in the root directory of this source tree. - -# Script to build packages for Forget Feedback (eg when you need to add a new package to the -# testapp) - -set -eo pipefail - -cwd=`basename $(pwd)` - -if [ $cwd != "react-forget" ]; then - echo "Error: This script must be run from the top level react-forget directory. Exiting" - exit 1 -fi - -# ----------------------- Build packages -yarn install --silent -rm -rf forget-feedback/dist -mkdir forget-feedback/dist - -packages=("babel-plugin-react-compiler" "eslint-plugin-react-compiler" "react-forget-runtime") -for package in ${packages[@]}; do - echo "Building" $package - yarn workspace $package run build -done - -echo "Copying artifacts to local forget-feedback directory..." -for package in ${packages[@]}; do - for dir in packages/$package/; do - if [ -d $dir/dist ]; then - package_name=$(basename $dir) - cp -R $dir/dist/. ./forget-feedback/dist/$package_name - cp $dir/package.json ./forget-feedback/dist/$package_name - fi - done -done diff --git a/compiler/scripts/copyright.js b/compiler/scripts/copyright.js index 101d25c453432..c8f12cbdb57c4 100644 --- a/compiler/scripts/copyright.js +++ b/compiler/scripts/copyright.js @@ -22,9 +22,6 @@ const files = glob.sync("**/*.{js,ts,tsx,jsx,rs}", { ignore: [ "**/dist/**", "**/node_modules/**", - "react/**", - "forget-feedback/**", - "packages/js-fuzzer/**", "**/tests/fixtures/**", "**/__tests__/fixtures/**", ], From 133ada72549b9aa01a0bc3df2b1c9bb341e861fd Mon Sep 17 00:00:00 2001 From: LoganDark Date: Tue, 25 Jun 2024 11:30:28 -0700 Subject: [PATCH 18/25] Read constructor name more carefully (#29954) ## Summary Sometimes `constructor` happens to be the name of an unrelated property, or we may be dealing with a `Proxy` that intercepts every read. Verify the constructor is a function before using its name, and reset the name anyway if it turns out not to be serializable. Fixes some cases of the devtools crashing and becoming inoperable upon attempting to inspect components whose props are Hookstate `State`s. ## How did you test this change? Installed a patched version of the extension and confirmed that it solves the problem. --------- Co-authored-by: Ruslan Lesiutin --- packages/react-devtools-shared/src/hydration.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/react-devtools-shared/src/hydration.js b/packages/react-devtools-shared/src/hydration.js index 6522076e7ddae..c5b78135e74a2 100644 --- a/packages/react-devtools-shared/src/hydration.js +++ b/packages/react-devtools-shared/src/hydration.js @@ -84,7 +84,9 @@ function createDehydrated( preview_long: formatDataForPreview(data, true), preview_short: formatDataForPreview(data, false), name: - !data.constructor || data.constructor.name === 'Object' + typeof data.constructor !== 'function' || + typeof data.constructor.name !== 'string' || + data.constructor.name === 'Object' ? '' : data.constructor.name, }; @@ -240,7 +242,9 @@ export function dehydrate( preview_short: formatDataForPreview(data, false), preview_long: formatDataForPreview(data, true), name: - !data.constructor || data.constructor.name === 'Object' + typeof data.constructor !== 'function' || + typeof data.constructor.name !== 'string' || + data.constructor.name === 'Object' ? '' : data.constructor.name, }; @@ -332,7 +336,11 @@ export function dehydrate( readonly: true, preview_short: formatDataForPreview(data, false), preview_long: formatDataForPreview(data, true), - name: data.constructor.name, + name: + typeof data.constructor !== 'function' || + typeof data.constructor.name !== 'string' + ? '' + : data.constructor.name, }; getAllEnumerableKeys(data).forEach(key => { From 4d11e1e88d6be1c244cb8ae76c377eaf195167ec Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 25 Jun 2024 16:03:57 -0400 Subject: [PATCH 19/25] [compiler][fixtures] test repros: codegen, alignScope, phis ghstack-source-id: 04b1526c8567f8b7b59d198f022d10cf837e4c5b Pull Request resolved: https://github.com/facebook/react/pull/29878 The AlignReactiveScope bug should be simplest to fix, but it's also caught by an invariant assertion. I think a fix could be either keeping track of "active" block-fallthrough pairs (`retainWhere(pair => pair.range.end > current.instr[0].id)`) or following the approach in `assertValidBlockNesting`. I'm tempted to pull the value-block aligning logic out into its own pass (using the current `node` tree traversal), then align to non-value blocks with the `assertValidBlockNesting` approach. Happy to hear feedback on this though! The other two are likely bigger issues, as they're not caught by static invariants. Update: - removed bug-phi-reference-effect as it's been patched by @josephsavona - added bug-array-concat-should-capture --- .../bug-array-concat-should-capture.expect.md | 72 +++++++++++++++ .../bug-array-concat-should-capture.ts | 21 +++++ .../bug-codegen-inline-iife.expect.md | 92 +++++++++++++++++++ .../compiler/bug-codegen-inline-iife.ts | 36 ++++++++ ...o-align-scope-starts-within-cond.expect.md | 29 ++++++ ...ror.todo-align-scope-starts-within-cond.ts | 14 +++ ...gn-scopes-nested-block-structure.expect.md | 68 ++++++++++++++ ...odo-align-scopes-nested-block-structure.ts | 53 +++++++++++ .../packages/snap/src/SproutTodoFilter.ts | 2 + .../snap/src/sprout/shared-runtime.ts | 44 +++++---- 10 files changed, 412 insertions(+), 19 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md new file mode 100644 index 0000000000000..41271ffaf0939 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md @@ -0,0 +1,72 @@ + +## Input + +```javascript +import { mutate } from "shared-runtime"; + +/** + * Fixture showing why `concat` needs to capture both the callee and rest args. + * Here, observe that arr1's values are captured into arr2. + * - Later mutations of arr2 may write to values within arr1. + * - Observe that it's technically valid to separately memoize the array arr1 + * itself. + */ +function Foo({ inputNum }) { + const arr1: Array = [{ a: 1 }, {}]; + const arr2 = arr1.concat([1, inputNum]); + mutate(arr2[0]); + return arr2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ inputNum: 2 }], + sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { mutate } from "shared-runtime"; + +/** + * Fixture showing why `concat` needs to capture both the callee and rest args. + * Here, observe that arr1's values are captured into arr2. + * - Later mutations of arr2 may write to values within arr1. + * - Observe that it's technically valid to separately memoize the array arr1 + * itself. + */ +function Foo(t0) { + const $ = _c(3); + const { inputNum } = t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = [{ a: 1 }, {}]; + $[0] = t1; + } else { + t1 = $[0]; + } + const arr1 = t1; + let arr2; + if ($[1] !== inputNum) { + arr2 = arr1.concat([1, inputNum]); + mutate(arr2[0]); + $[1] = inputNum; + $[2] = arr2; + } else { + arr2 = $[2]; + } + return arr2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ inputNum: 2 }], + sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts new file mode 100644 index 0000000000000..f9b5ed619c23e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts @@ -0,0 +1,21 @@ +import { mutate } from "shared-runtime"; + +/** + * Fixture showing why `concat` needs to capture both the callee and rest args. + * Here, observe that arr1's values are captured into arr2. + * - Later mutations of arr2 may write to values within arr1. + * - Observe that it's technically valid to separately memoize the array arr1 + * itself. + */ +function Foo({ inputNum }) { + const arr1: Array = [{ a: 1 }, {}]; + const arr2 = arr1.concat([1, inputNum]); + mutate(arr2[0]); + return arr2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ inputNum: 2 }], + sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md new file mode 100644 index 0000000000000..535018bf76b33 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md @@ -0,0 +1,92 @@ + +## Input + +```javascript +import { makeArray, print } from "shared-runtime"; + +/** + * Exposes bug involving iife inlining + codegen. + * We currently inline iifes to labeled blocks (not value-blocks). + * + * Here, print(1) and the evaluation of makeArray(...) get the same scope + * as the compiler infers that the makeArray call may mutate its arguments. + * Since print(1) does not get its own scope (and is thus not a declaration + * or dependency), it does not get promoted. + * As a result, print(1) gets reordered across the labeled-block instructions + * to be inlined at the makeArray callsite. + * + * Current evaluator results: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) [null,2] + * logs: [1,2] + * Forget: + * (kind: ok) [null,2] + * logs: [2,1] + */ +function useTest() { + return makeArray( + print(1), + (function foo() { + print(2); + return 2; + })() + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray, print } from "shared-runtime"; + +/** + * Exposes bug involving iife inlining + codegen. + * We currently inline iifes to labeled blocks (not value-blocks). + * + * Here, print(1) and the evaluation of makeArray(...) get the same scope + * as the compiler infers that the makeArray call may mutate its arguments. + * Since print(1) does not get its own scope (and is thus not a declaration + * or dependency), it does not get promoted. + * As a result, print(1) gets reordered across the labeled-block instructions + * to be inlined at the makeArray callsite. + * + * Current evaluator results: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) [null,2] + * logs: [1,2] + * Forget: + * (kind: ok) [null,2] + * logs: [2,1] + */ +function useTest() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + let t1; + + print(2); + t1 = 2; + t0 = makeArray(print(1), t1); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts new file mode 100644 index 0000000000000..185bd89cb44f2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts @@ -0,0 +1,36 @@ +import { makeArray, print } from "shared-runtime"; + +/** + * Exposes bug involving iife inlining + codegen. + * We currently inline iifes to labeled blocks (not value-blocks). + * + * Here, print(1) and the evaluation of makeArray(...) get the same scope + * as the compiler infers that the makeArray call may mutate its arguments. + * Since print(1) does not get its own scope (and is thus not a declaration + * or dependency), it does not get promoted. + * As a result, print(1) gets reordered across the labeled-block instructions + * to be inlined at the makeArray callsite. + * + * Current evaluator results: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) [null,2] + * logs: [1,2] + * Forget: + * (kind: ok) [null,2] + * logs: [2,1] + */ +function useTest() { + return makeArray( + print(1), + (function foo() { + print(2); + return 2; + })() + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md new file mode 100644 index 0000000000000..a3b498b1c373a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md @@ -0,0 +1,29 @@ + +## Input + +```javascript +/** + * Similar fixture to `error.todo-align-scopes-nested-block-structure`, but + * a simpler case. + */ +function useFoo(cond) { + let s = null; + if (cond) { + s = {}; + } else { + return null; + } + mutate(s); + return s; +} + +``` + + +## Error + +``` +Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:13) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts new file mode 100644 index 0000000000000..555bb713898a8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts @@ -0,0 +1,14 @@ +/** + * Similar fixture to `error.todo-align-scopes-nested-block-structure`, but + * a simpler case. + */ +function useFoo(cond) { + let s = null; + if (cond) { + s = {}; + } else { + return null; + } + mutate(s); + return s; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md new file mode 100644 index 0000000000000..9d19f7fa21986 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md @@ -0,0 +1,68 @@ + +## Input + +```javascript +/** + * Fixture showing that it's not sufficient to only align direct scoped + * accesses of a block-fallthrough pair. + * Below is a simplified view of HIR blocks in this fixture. + * Note that here, s is mutated in both bb1 and bb4. However, neither + * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves. + * + * This means that we need to recursively visit all scopes accessed between + * a block and its fallthrough and extend the range of those scopes which overlap + * with an active block/fallthrough pair, + * + * bb0 + * ┌──────────────┐ + * │let s = null │ + * │test cond1 │ + * │ │ + * └┬─────────────┘ + * │ bb1 + * ├─►┌───────┐ + * │ │s = {} ├────┐ + * │ └───────┘ │ + * │ bb2 │ + * └─►┌───────┐ │ + * │return;│ │ + * └───────┘ │ + * bb3 │ + * ┌──────────────┐◄┘ + * │test cond2 │ + * │ │ + * └┬─────────────┘ + * │ bb4 + * ├─►┌─────────┐ + * │ │mutate(s)├─┐ + * ▼ └─────────┘ │ + * bb5 │ + * ┌───────────┐ │ + * │return s; │◄──┘ + * └───────────┘ + */ +function useFoo(cond1, cond2) { + let s = null; + if (cond1) { + s = {}; + } else { + return null; + } + + if (cond2) { + mutate(s); + } + + return s; +} + +``` + + +## Error + +``` +Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:15) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts new file mode 100644 index 0000000000000..8e99f0435b562 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts @@ -0,0 +1,53 @@ +/** + * Fixture showing that it's not sufficient to only align direct scoped + * accesses of a block-fallthrough pair. + * Below is a simplified view of HIR blocks in this fixture. + * Note that here, s is mutated in both bb1 and bb4. However, neither + * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves. + * + * This means that we need to recursively visit all scopes accessed between + * a block and its fallthrough and extend the range of those scopes which overlap + * with an active block/fallthrough pair, + * + * bb0 + * ┌──────────────┐ + * │let s = null │ + * │test cond1 │ + * │ │ + * └┬─────────────┘ + * │ bb1 + * ├─►┌───────┐ + * │ │s = {} ├────┐ + * │ └───────┘ │ + * │ bb2 │ + * └─►┌───────┐ │ + * │return;│ │ + * └───────┘ │ + * bb3 │ + * ┌──────────────┐◄┘ + * │test cond2 │ + * │ │ + * └┬─────────────┘ + * │ bb4 + * ├─►┌─────────┐ + * │ │mutate(s)├─┐ + * ▼ └─────────┘ │ + * bb5 │ + * ┌───────────┐ │ + * │return s; │◄──┘ + * └───────────┘ + */ +function useFoo(cond1, cond2) { + let s = null; + if (cond1) { + s = {}; + } else { + return null; + } + + if (cond2) { + mutate(s); + } + + return s; +} diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index e193fcb753b98..52834a2d1a146 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -487,6 +487,8 @@ const skipFilter = new Set([ "bug-invalid-hoisting-functionexpr", "original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block", "original-reactive-scopes-fork/bug-hoisted-declaration-with-scope", + "bug-codegen-inline-iife", + "bug-array-concat-should-capture", // 'react-compiler-runtime' not yet supported "flag-enable-emit-hook-guards", diff --git a/compiler/packages/snap/src/sprout/shared-runtime.ts b/compiler/packages/snap/src/sprout/shared-runtime.ts index 94fba22c04974..7d4687218d0e2 100644 --- a/compiler/packages/snap/src/sprout/shared-runtime.ts +++ b/compiler/packages/snap/src/sprout/shared-runtime.ts @@ -36,7 +36,7 @@ export const CONST_NUMBER2 = 2; export const CONST_TRUE = true; export const CONST_FALSE = false; -export function initFbt() { +export function initFbt(): void { const viewerContext: IntlViewerContext = { GENDER: IntlVariations.GENDER_UNKNOWN, locale: "en_US", @@ -52,7 +52,7 @@ export function initFbt() { export function mutate(arg: any): void { // don't mutate primitive - if (typeof arg === null || typeof arg !== "object") { + if (arg == null || typeof arg !== "object") { return; } @@ -80,7 +80,7 @@ export function mutateAndReturnNewValue(arg: T): string { export function setProperty(arg: any, property: any): void { // don't mutate primitive - if (typeof arg === null || typeof arg !== "object") { + if (arg == null || typeof arg !== "object") { return arg; } @@ -123,7 +123,7 @@ export function calculateExpensiveNumber(x: number): number { /** * Functions that do not mutate their parameters */ -export function shallowCopy(obj: Object): object { +export function shallowCopy(obj: object): object { return Object.assign({}, obj); } @@ -139,9 +139,11 @@ export function addOne(value: number): number { return value + 1; } -// Alias console.log, as it is defined as a global and may have -// different compiler handling than unknown functions -export function print(...args: Array) { +/* + * Alias console.log, as it is defined as a global and may have + * different compiler handling than unknown functions + */ +export function print(...args: Array): void { console.log(...args); } @@ -153,7 +155,7 @@ export function throwErrorWithMessage(message: string): never { throw new Error(message); } -export function throwInput(x: Object): never { +export function throwInput(x: object): never { throw x; } @@ -167,12 +169,12 @@ export function logValue(value: T): void { console.log(value); } -export function useHook(): Object { +export function useHook(): object { return makeObject_Primitives(); } const noAliasObject = Object.freeze({}); -export function useNoAlias(...args: Array): object { +export function useNoAlias(..._args: Array): object { return noAliasObject; } @@ -183,7 +185,7 @@ export function useIdentity(arg: T): T { export function invoke, ReturnType>( fn: (...input: T) => ReturnType, ...params: T -) { +): ReturnType { return fn(...params); } @@ -191,7 +193,7 @@ export function conditionalInvoke, ReturnType>( shouldInvoke: boolean, fn: (...input: T) => ReturnType, ...params: T -) { +): ReturnType | null { if (shouldInvoke) { return fn(...params); } else { @@ -205,21 +207,25 @@ export function conditionalInvoke, ReturnType>( export function Text(props: { value: string; children?: Array; -}) { +}): React.ReactElement { return React.createElement("div", null, props.value, props.children); } -export function StaticText1(props: { children?: Array }) { +export function StaticText1(props: { + children?: Array; +}): React.ReactElement { return React.createElement("div", null, "StaticText1", props.children); } -export function StaticText2(props: { children?: Array }) { +export function StaticText2(props: { + children?: Array; +}): React.ReactElement { return React.createElement("div", null, "StaticText2", props.children); } export function RenderPropAsChild(props: { items: Array<() => React.ReactNode>; -}) { +}): React.ReactElement { return React.createElement( "div", null, @@ -242,7 +248,7 @@ export function ValidateMemoization({ }: { inputs: Array; output: any; -}) { +}): React.ReactElement { "use no forget"; const [previousInputs, setPreviousInputs] = React.useState(inputs); const [previousOutput, setPreviousOutput] = React.useState(output); @@ -273,7 +279,7 @@ export function createHookWrapper( } // helper functions -export function toJSON(value: any, invokeFns: boolean = false) { +export function toJSON(value: any, invokeFns: boolean = false): string { const seen = new Map(); return JSON.stringify(value, (_key: string, val: any) => { @@ -319,7 +325,7 @@ export const ObjectWithHooks = { }, }; -export function useFragment(...args: Array): Object { +export function useFragment(..._args: Array): object { return { a: [1, 2, 3], b: { c: { d: 4 } }, From 86d1a6f54aebb2854aab0033bb57865add9f440d Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 25 Jun 2024 16:03:58 -0400 Subject: [PATCH 20/25] [compiler][rewrite] Patch logic for aligning scopes to non-value blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. ghstack-source-id: 327dec5019483666f81c8156ac0c666ccad511b3 Pull Request resolved: https://github.com/facebook/react/pull/29891 --- .../AlignReactiveScopesToBlockScopesHIR.ts | 162 +++++++++-------- .../src/Utils/utils.ts | 11 ++ .../align-scope-starts-within-cond.expect.md | 76 ++++++++ .../align-scope-starts-within-cond.ts | 21 +++ ...fe-return-modified-later-logical.expect.md | 55 ++++++ ...pes-iife-return-modified-later-logical.ts} | 0 ...gn-scopes-nested-block-structure.expect.md | 169 ++++++++++++++++++ ...=> align-scopes-nested-block-structure.ts} | 28 ++- ...copes-reactive-scope-overlaps-if.expect.md | 84 +++++++++ ...lign-scopes-reactive-scope-overlaps-if.ts} | 0 ...es-reactive-scope-overlaps-label.expect.md | 79 ++++++++ ...n-scopes-reactive-scope-overlaps-label.ts} | 0 ...opes-reactive-scope-overlaps-try.expect.md | 65 +++++++ ...ign-scopes-reactive-scope-overlaps-try.ts} | 2 +- ...rycatch-nested-overlapping-range.expect.md | 61 +++++++ ...copes-trycatch-nested-overlapping-range.ts | 19 ++ ...rycatch-nested-overlapping-range.expect.md | 26 --- ...repro-trycatch-nested-overlapping-range.js | 11 -- ...rror.repro-bug-ref-mutable-range.expect.md | 27 --- .../error.repro-bug-ref-mutable-range.js | 12 -- ...o-align-scope-starts-within-cond.expect.md | 29 --- ...ror.todo-align-scope-starts-within-cond.ts | 14 -- ...odo-align-scopes-nested-block-structure.ts | 53 ------ ...fe-return-modified-later-logical.expect.md | 29 --- ....todo-reactive-scope-overlaps-if.expect.md | 41 ----- ...do-reactive-scope-overlaps-label.expect.md | 40 ----- ...todo-reactive-scope-overlaps-try.expect.md | 36 ---- .../repro-ref-mutable-range.expect.md | 83 +++++++++ .../compiler/repro-ref-mutable-range.tsx | 19 ++ 29 files changed, 841 insertions(+), 411 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scope-starts-within-cond.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scope-starts-within-cond.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-iife-return-modified-later-logical.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-iife-return-modified-later-logical.js => align-scopes-iife-return-modified-later-logical.ts} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-nested-block-structure.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-align-scopes-nested-block-structure.expect.md => align-scopes-nested-block-structure.ts} (81%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-reactive-scope-overlaps-if.ts => align-scopes-reactive-scope-overlaps-if.ts} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-label.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-reactive-scope-overlaps-label.ts => align-scopes-reactive-scope-overlaps-label.ts} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-try.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-reactive-scope-overlaps-try.ts => align-scopes-reactive-scope-overlaps-try.ts} (89%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.ts delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-repro-trycatch-nested-overlapping-range.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-repro-trycatch-nested-overlapping-range.js delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-bug-ref-mutable-range.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-bug-ref-mutable-range.js delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-iife-return-modified-later-logical.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-if.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-label.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-try.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-ref-mutable-range.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-ref-mutable-range.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts index 3e451c23c7807..3d78d99cb10ed 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts @@ -22,8 +22,10 @@ import { mapTerminalSuccessors, terminalFallthrough, } from "../HIR/visitors"; +import { retainWhere_Set } from "../Utils/utils"; import { getPlaceScope } from "./BuildReactiveBlocks"; +type InstructionRange = MutableRange; /* * Note: this is the 2nd of 4 passes that determine how to break a function into discrete * reactive scopes (independently memoizeable units of code): @@ -66,18 +68,20 @@ import { getPlaceScope } from "./BuildReactiveBlocks"; * will be the updated end for that scope). */ export function alignReactiveScopesToBlockScopesHIR(fn: HIRFunction): void { - const blockNodes = new Map(); - const rootNode: BlockNode = { - kind: "node", - valueRange: null, - children: [], - id: makeInstructionId(0), - }; - blockNodes.set(fn.body.entry, rootNode); + const activeBlockFallthroughRanges: Array<{ + range: InstructionRange; + fallthrough: BlockId; + }> = []; + const activeScopes = new Set(); const seen = new Set(); + const valueBlockNodes = new Map(); const placeScopes = new Map(); - function recordPlace(id: InstructionId, place: Place, node: BlockNode): void { + function recordPlace( + id: InstructionId, + place: Place, + node: ValueBlockNode | null + ): void { if (place.identifier.scope !== null) { placeScopes.set(place, place.identifier.scope); } @@ -86,13 +90,14 @@ export function alignReactiveScopesToBlockScopesHIR(fn: HIRFunction): void { if (scope == null) { return; } - node.children.push({ kind: "scope", scope, id }); + activeScopes.add(scope); + node?.children.push({ kind: "scope", scope, id }); if (seen.has(scope)) { return; } seen.add(scope); - if (node.valueRange !== null) { + if (node != null && node.valueRange !== null) { scope.range.start = makeInstructionId( Math.min(node.valueRange.start, scope.range.start) ); @@ -103,16 +108,25 @@ export function alignReactiveScopesToBlockScopesHIR(fn: HIRFunction): void { } for (const [, block] of fn.body.blocks) { - const { instructions, terminal } = block; - const node = blockNodes.get(block.id); - if (node === undefined) { - CompilerError.invariant(false, { - reason: `Expected a node to be initialized for block`, - loc: instructions[0]?.loc ?? terminal.loc, - description: `No node for block bb${block.id} (${block.kind})`, - }); + const startingId = block.instructions[0]?.id ?? block.terminal.id; + retainWhere_Set(activeScopes, (scope) => scope.range.end > startingId); + const top = activeBlockFallthroughRanges.at(-1); + if (top?.fallthrough === block.id) { + activeBlockFallthroughRanges.pop(); + /* + * All active scopes must have either started before or within the last + * block-fallthrough range. In either case, they overlap this block- + * fallthrough range and can have their ranges extended. + */ + for (const scope of activeScopes) { + scope.range.start = makeInstructionId( + Math.min(scope.range.start, top.range.start) + ); + } } + const { instructions, terminal } = block; + const node = valueBlockNodes.get(block.id) ?? null; for (const instr of instructions) { for (const lvalue of eachInstructionLValue(instr)) { recordPlace(instr.id, lvalue, node); @@ -125,36 +139,42 @@ export function alignReactiveScopesToBlockScopesHIR(fn: HIRFunction): void { recordPlace(terminal.id, operand, node); } - // Save the current node for the fallback block, where this block scope continues const fallthrough = terminalFallthrough(terminal); - if (fallthrough !== null && !blockNodes.has(fallthrough)) { + if (fallthrough !== null) { /* - * Any scopes that carried over across a terminal->fallback need their range extended - * to at least the first instruction of the fallback - * - * Note that it's possible for a terminal such as an if or switch to have a null fallback, - * indicating that all control-flow paths diverge instead of reaching the fallthrough. - * In this case there isn't an instruction id in the program that we can point to for the - * updated range. Since the output is correct in this case we leave it, but it would be - * more correct to find the maximum instuction id in the whole program and set the range.end - * to one greater. Alternatively, we could leave in an unreachable fallthrough (with a new - * "unreachable" terminal variant, perhaps) and use that instruction id. + * Any currently active scopes that overlaps the block-fallthrough range + * need their range extended to at least the first instruction of the + * fallthrough */ const fallthroughBlock = fn.body.blocks.get(fallthrough)!; const nextId = fallthroughBlock.instructions[0]?.id ?? fallthroughBlock.terminal.id; - for (const child of node.children) { - if (child.kind !== "scope") { - continue; - } - const scope = child.scope; + for (const scope of activeScopes) { if (scope.range.end > terminal.id) { scope.range.end = makeInstructionId( Math.max(scope.range.end, nextId) ); } } - blockNodes.set(fallthrough, node); + /** + * We also record the block-fallthrough range for future scopes that begin + * within the range (and overlap with the range end). + */ + activeBlockFallthroughRanges.push({ + fallthrough, + range: { + start: terminal.id, + end: nextId, + }, + }); + + CompilerError.invariant(!valueBlockNodes.has(fallthrough), { + reason: "Expect hir blocks to have unique fallthroughs", + loc: terminal.loc, + }); + if (node != null) { + valueBlockNodes.set(fallthrough, node); + } } /* @@ -166,48 +186,35 @@ export function alignReactiveScopesToBlockScopesHIR(fn: HIRFunction): void { * just those that are direct successors for normal control-flow ordering. */ mapTerminalSuccessors(terminal, (successor) => { - if (blockNodes.has(successor)) { + if (valueBlockNodes.has(successor)) { return successor; } const successorBlock = fn.body.blocks.get(successor)!; - /* - * we need the block kind check here because the do..while terminal's successor - * is a block, and try's successor is a catch block - */ if (successorBlock.kind === "block" || successorBlock.kind === "catch") { - const childNode: BlockNode = { - kind: "node", - id: terminal.id, - children: [], - valueRange: null, - }; - node.children.push(childNode); - blockNodes.set(successor, childNode); + /* + * we need the block kind check here because the do..while terminal's + * successor is a block, and try's successor is a catch block + */ } else if ( - node.valueRange === null || + node == null || terminal.kind === "ternary" || terminal.kind === "logical" || terminal.kind === "optional" ) { /** - * Create a new scope node whenever we transition from block scope -> value scope. + * Create a new node whenever we transition from non-value -> value block. * * For compatibility with the previous ReactiveFunction-based scope merging logic, * we also create new scope nodes for ternary, logical, and optional terminals. - * However, inside value blocks we always store a range (valueRange) that is the + * Inside value blocks we always store a range (valueRange) that is the * start/end instruction ids at the nearest parent block scope level, so that * scopes inside the value blocks can be extended to align with block scope * instructions. */ - const childNode = { - kind: "node", - id: terminal.id, - children: [], - valueRange: null, - } as BlockNode; - if (node.valueRange === null) { - // Transition from block->value scope, derive the outer block scope range + let valueRange: MutableRange; + if (node == null) { + // Transition from block->value block, derive the outer block range CompilerError.invariant(fallthrough !== null, { reason: `Expected a fallthrough for value block`, loc: terminal.loc, @@ -216,32 +223,36 @@ export function alignReactiveScopesToBlockScopesHIR(fn: HIRFunction): void { const nextId = fallthroughBlock.instructions[0]?.id ?? fallthroughBlock.terminal.id; - childNode.valueRange = { + valueRange = { start: terminal.id, end: nextId, }; } else { // else value->value transition, reuse the range - childNode.valueRange = node.valueRange; + valueRange = node.valueRange; } - node.children.push(childNode); - blockNodes.set(successor, childNode); + const childNode: ValueBlockNode = { + kind: "node", + id: terminal.id, + children: [], + valueRange, + }; + node?.children.push(childNode); + valueBlockNodes.set(successor, childNode); } else { // this is a value -> value block transition, reuse the node - blockNodes.set(successor, node); + valueBlockNodes.set(successor, node); } return successor; }); } - - // console.log(_debug(rootNode)); } -type BlockNode = { +type ValueBlockNode = { kind: "node"; id: InstructionId; - valueRange: MutableRange | null; - children: Array; + valueRange: MutableRange; + children: Array; }; type ReactiveScopeNode = { kind: "scope"; @@ -249,13 +260,13 @@ type ReactiveScopeNode = { scope: ReactiveScope; }; -function _debug(node: BlockNode): string { +function _debug(node: ValueBlockNode): string { const buf: Array = []; _printNode(node, buf, 0); return buf.join("\n"); } function _printNode( - node: BlockNode | ReactiveScopeNode, + node: ValueBlockNode | ReactiveScopeNode, out: Array, depth: number = 0 ): void { @@ -265,10 +276,7 @@ function _printNode( `${prefix}[${node.id}] @${node.scope.id} [${node.scope.range.start}:${node.scope.range.end}]` ); } else { - let range = - node.valueRange !== null - ? ` [${node.valueRange.start}:${node.valueRange.end}]` - : ""; + let range = ` (range=[${node.valueRange.start}:${node.valueRange.end}])`; out.push(`${prefix}[${node.id}] node${range} [`); for (const child of node.children) { _printNode(child, out, depth + 1); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts b/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts index c02e813255676..921a6bf2097d0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts @@ -45,6 +45,17 @@ export function retainWhere( array.length = writeIndex; } +export function retainWhere_Set( + items: Set, + predicate: (item: T) => boolean +): void { + for (const item of items) { + if (!predicate(item)) { + items.delete(item); + } + } +} + export function getOrInsertWith( m: Map, key: U, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scope-starts-within-cond.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scope-starts-within-cond.expect.md new file mode 100644 index 0000000000000..d386efbc310a6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scope-starts-within-cond.expect.md @@ -0,0 +1,76 @@ + +## Input + +```javascript +import { mutate } from "shared-runtime"; + +/** + * Similar fixture to `align-scopes-nested-block-structure`, but + * a simpler case. + */ +function useFoo(cond) { + let s = null; + if (cond) { + s = {}; + } else { + return null; + } + mutate(s); + return s; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { mutate } from "shared-runtime"; + +/** + * Similar fixture to `align-scopes-nested-block-structure`, but + * a simpler case. + */ +function useFoo(cond) { + const $ = _c(3); + let s; + let t0; + if ($[0] !== cond) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + if (cond) { + s = {}; + } else { + t0 = null; + break bb0; + } + + mutate(s); + } + $[0] = cond; + $[1] = t0; + $[2] = s; + } else { + t0 = $[1]; + s = $[2]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } + return s; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; + +``` + +### Eval output +(kind: ok) {"wat0":"joe"} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scope-starts-within-cond.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scope-starts-within-cond.ts new file mode 100644 index 0000000000000..03b73fa179192 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scope-starts-within-cond.ts @@ -0,0 +1,21 @@ +import { mutate } from "shared-runtime"; + +/** + * Similar fixture to `align-scopes-nested-block-structure`, but + * a simpler case. + */ +function useFoo(cond) { + let s = null; + if (cond) { + s = {}; + } else { + return null; + } + mutate(s); + return s; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-iife-return-modified-later-logical.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-iife-return-modified-later-logical.expect.md new file mode 100644 index 0000000000000..449beb18fd81c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-iife-return-modified-later-logical.expect.md @@ -0,0 +1,55 @@ + +## Input + +```javascript +import { getNull } from "shared-runtime"; + +function Component(props) { + const items = (() => { + return getNull() ?? []; + })(); + items.push(props.a); + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ a: {} }], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { getNull } from "shared-runtime"; + +function Component(props) { + const $ = _c(3); + let t0; + let items; + if ($[0] !== props.a) { + t0 = getNull() ?? []; + items = t0; + + items.push(props.a); + $[0] = props.a; + $[1] = items; + $[2] = t0; + } else { + items = $[1]; + t0 = $[2]; + } + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ a: {} }], +}; + +``` + +### Eval output +(kind: ok) [{}] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-iife-return-modified-later-logical.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-iife-return-modified-later-logical.ts similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-iife-return-modified-later-logical.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-iife-return-modified-later-logical.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-nested-block-structure.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-nested-block-structure.expect.md new file mode 100644 index 0000000000000..b06f495cd7272 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-nested-block-structure.expect.md @@ -0,0 +1,169 @@ + +## Input + +```javascript +import { mutate } from "shared-runtime"; +/** + * Fixture showing that it's not sufficient to only align direct scoped + * accesses of a block-fallthrough pair. + * Below is a simplified view of HIR blocks in this fixture. + * Note that here, s is mutated in both bb1 and bb4. However, neither + * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves. + * + * This means that we need to recursively visit all scopes accessed between + * a block and its fallthrough and extend the range of those scopes which overlap + * with an active block/fallthrough pair, + * + * bb0 + * ┌──────────────┐ + * │let s = null │ + * │test cond1 │ + * │ │ + * └┬─────────────┘ + * │ bb1 + * ├─►┌───────┐ + * │ │s = {} ├────┐ + * │ └───────┘ │ + * │ bb2 │ + * └─►┌───────┐ │ + * │return;│ │ + * └───────┘ │ + * bb3 │ + * ┌──────────────┐◄┘ + * │test cond2 │ + * │ │ + * └┬─────────────┘ + * │ bb4 + * ├─►┌─────────┐ + * │ │mutate(s)├─┐ + * ▼ └─────────┘ │ + * bb5 │ + * ┌───────────┐ │ + * │return s; │◄──┘ + * └───────────┘ + */ +function useFoo({ cond1, cond2 }) { + let s = null; + if (cond1) { + s = {}; + } else { + return null; + } + + if (cond2) { + mutate(s); + } + + return s; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ cond1: true, cond2: false }], + sequentialRenders: [ + { cond1: true, cond2: false }, + { cond1: true, cond2: false }, + { cond1: true, cond2: true }, + { cond1: true, cond2: true }, + { cond1: false, cond2: true }, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { mutate } from "shared-runtime"; +/** + * Fixture showing that it's not sufficient to only align direct scoped + * accesses of a block-fallthrough pair. + * Below is a simplified view of HIR blocks in this fixture. + * Note that here, s is mutated in both bb1 and bb4. However, neither + * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves. + * + * This means that we need to recursively visit all scopes accessed between + * a block and its fallthrough and extend the range of those scopes which overlap + * with an active block/fallthrough pair, + * + * bb0 + * ┌──────────────┐ + * │let s = null │ + * │test cond1 │ + * │ │ + * └┬─────────────┘ + * │ bb1 + * ├─►┌───────┐ + * │ │s = {} ├────┐ + * │ └───────┘ │ + * │ bb2 │ + * └─►┌───────┐ │ + * │return;│ │ + * └───────┘ │ + * bb3 │ + * ┌──────────────┐◄┘ + * │test cond2 │ + * │ │ + * └┬─────────────┘ + * │ bb4 + * ├─►┌─────────┐ + * │ │mutate(s)├─┐ + * ▼ └─────────┘ │ + * bb5 │ + * ┌───────────┐ │ + * │return s; │◄──┘ + * └───────────┘ + */ +function useFoo(t0) { + const $ = _c(4); + const { cond1, cond2 } = t0; + let s; + let t1; + if ($[0] !== cond1 || $[1] !== cond2) { + t1 = Symbol.for("react.early_return_sentinel"); + bb0: { + if (cond1) { + s = {}; + } else { + t1 = null; + break bb0; + } + if (cond2) { + mutate(s); + } + } + $[0] = cond1; + $[1] = cond2; + $[2] = t1; + $[3] = s; + } else { + t1 = $[2]; + s = $[3]; + } + if (t1 !== Symbol.for("react.early_return_sentinel")) { + return t1; + } + return s; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ cond1: true, cond2: false }], + sequentialRenders: [ + { cond1: true, cond2: false }, + { cond1: true, cond2: false }, + { cond1: true, cond2: true }, + { cond1: true, cond2: true }, + { cond1: false, cond2: true }, + ], +}; + +``` + +### Eval output +(kind: ok) {} +{} +{"wat0":"joe"} +{"wat0":"joe"} +null \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-nested-block-structure.ts similarity index 81% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-nested-block-structure.ts index 9d19f7fa21986..3087f041a553e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-nested-block-structure.ts @@ -1,7 +1,4 @@ - -## Input - -```javascript +import { mutate } from "shared-runtime"; /** * Fixture showing that it's not sufficient to only align direct scoped * accesses of a block-fallthrough pair. @@ -41,7 +38,7 @@ * │return s; │◄──┘ * └───────────┘ */ -function useFoo(cond1, cond2) { +function useFoo({ cond1, cond2 }) { let s = null; if (cond1) { s = {}; @@ -56,13 +53,14 @@ function useFoo(cond1, cond2) { return s; } -``` - - -## Error - -``` -Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:15) -``` - - \ No newline at end of file +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ cond1: true, cond2: false }], + sequentialRenders: [ + { cond1: true, cond2: false }, + { cond1: true, cond2: false }, + { cond1: true, cond2: true }, + { cond1: true, cond2: true }, + { cond1: false, cond2: true }, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md new file mode 100644 index 0000000000000..906c15092e076 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md @@ -0,0 +1,84 @@ + +## Input + +```javascript +function useFoo({ cond }) { + let items: any = {}; + b0: { + if (cond) { + // Mutable range of `items` begins here, but its reactive scope block + // should be aligned to above the if-branch + items = []; + } else { + break b0; + } + items.push(2); + } + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ cond: true }], + sequentialRenders: [ + { cond: true }, + { cond: true }, + { cond: false }, + { cond: false }, + { cond: true }, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function useFoo(t0) { + const $ = _c(3); + const { cond } = t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = {}; + $[0] = t1; + } else { + t1 = $[0]; + } + let items = t1; + bb0: if ($[1] !== cond) { + if (cond) { + items = []; + } else { + break bb0; + } + + items.push(2); + $[1] = cond; + $[2] = items; + } else { + items = $[2]; + } + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ cond: true }], + sequentialRenders: [ + { cond: true }, + { cond: true }, + { cond: false }, + { cond: false }, + { cond: true }, + ], +}; + +``` + +### Eval output +(kind: ok) [2] +[2] +{} +{} +[2] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-if.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.ts similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-if.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-label.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-label.expect.md new file mode 100644 index 0000000000000..dd5a9a1926d8f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-label.expect.md @@ -0,0 +1,79 @@ + +## Input + +```javascript +import { arrayPush } from "shared-runtime"; + +function useFoo({ cond, value }) { + let items; + label: { + items = []; + // Mutable range of `items` begins here, but its reactive scope block + // should be aligned to above the label-block + if (cond) break label; + arrayPush(items, value); + } + arrayPush(items, value); + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ cond: true, value: 2 }], + sequentialRenders: [ + { cond: true, value: 2 }, + { cond: true, value: 2 }, + { cond: true, value: 3 }, + { cond: false, value: 3 }, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { arrayPush } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(3); + const { cond, value } = t0; + let items; + if ($[0] !== cond || $[1] !== value) { + bb0: { + items = []; + if (cond) { + break bb0; + } + arrayPush(items, value); + } + + arrayPush(items, value); + $[0] = cond; + $[1] = value; + $[2] = items; + } else { + items = $[2]; + } + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ cond: true, value: 2 }], + sequentialRenders: [ + { cond: true, value: 2 }, + { cond: true, value: 2 }, + { cond: true, value: 3 }, + { cond: false, value: 3 }, + ], +}; + +``` + +### Eval output +(kind: ok) [2] +[2] +[3] +[3,3] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-label.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-label.ts similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-label.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-label.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-try.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-try.expect.md new file mode 100644 index 0000000000000..80a0349b3e8c0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-try.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +import { arrayPush, mutate } from "shared-runtime"; + +function useFoo({ value }) { + let items = null; + try { + // Mutable range of `items` begins here, but its reactive scope block + // should be aligned to above the try-block + items = []; + arrayPush(items, value); + } catch { + // ignore + } + mutate(items); + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ value: 2 }], + sequentialRenders: [{ value: 2 }, { value: 2 }, { value: 3 }], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { arrayPush, mutate } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(2); + const { value } = t0; + let items; + if ($[0] !== value) { + try { + items = []; + arrayPush(items, value); + } catch {} + + mutate(items); + $[0] = value; + $[1] = items; + } else { + items = $[1]; + } + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ value: 2 }], + sequentialRenders: [{ value: 2 }, { value: 2 }, { value: 3 }], +}; + +``` + +### Eval output +(kind: ok) [2] +[2] +[3] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-try.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-try.ts similarity index 89% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-try.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-try.ts index 0a4a7eab6c73d..378cdff83a3ee 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-try.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-try.ts @@ -1,4 +1,4 @@ -import { arrayPush } from "shared-runtime"; +import { arrayPush, mutate } from "shared-runtime"; function useFoo({ value }) { let items = null; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.expect.md new file mode 100644 index 0000000000000..b8f525d6b98cf --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.expect.md @@ -0,0 +1,61 @@ + +## Input + +```javascript +import { CONST_TRUE, makeObject_Primitives } from "shared-runtime"; + +function Foo() { + try { + let thing = null; + if (cond) { + thing = makeObject_Primitives(); + } + if (CONST_TRUE) { + mutate(thing); + } + return thing; + } catch {} +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { CONST_TRUE, makeObject_Primitives } from "shared-runtime"; + +function Foo() { + const $ = _c(1); + try { + let thing; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + thing = null; + if (cond) { + thing = makeObject_Primitives(); + } + if (CONST_TRUE) { + mutate(thing); + } + $[0] = thing; + } else { + thing = $[0]; + } + return thing; + } catch {} +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.ts new file mode 100644 index 0000000000000..2cc042094463e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.ts @@ -0,0 +1,19 @@ +import { CONST_TRUE, makeObject_Primitives } from "shared-runtime"; + +function Foo() { + try { + let thing = null; + if (cond) { + thing = makeObject_Primitives(); + } + if (CONST_TRUE) { + mutate(thing); + } + return thing; + } catch {} +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-repro-trycatch-nested-overlapping-range.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-repro-trycatch-nested-overlapping-range.expect.md deleted file mode 100644 index ca77829e2f7f4..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-repro-trycatch-nested-overlapping-range.expect.md +++ /dev/null @@ -1,26 +0,0 @@ - -## Input - -```javascript -function Foo() { - try { - let thing = null; - if (cond) { - thing = makeObject(); - } - if (otherCond) { - mutate(thing); - } - } catch {} -} - -``` - - -## Error - -``` -Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 2:24(18:26) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-repro-trycatch-nested-overlapping-range.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-repro-trycatch-nested-overlapping-range.js deleted file mode 100644 index 37be9363a5acf..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-repro-trycatch-nested-overlapping-range.js +++ /dev/null @@ -1,11 +0,0 @@ -function Foo() { - try { - let thing = null; - if (cond) { - thing = makeObject(); - } - if (otherCond) { - mutate(thing); - } - } catch {} -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-bug-ref-mutable-range.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-bug-ref-mutable-range.expect.md deleted file mode 100644 index 7cd2acc9affab..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-bug-ref-mutable-range.expect.md +++ /dev/null @@ -1,27 +0,0 @@ - -## Input - -```javascript -function Foo(props, ref) { - const value = {}; - if (cond1) { - mutate(value); - return ; - } - mutate(value); - if (cond2) { - return ; - } - return value; -} - -``` - - -## Error - -``` -Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 1:21(16:23) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-bug-ref-mutable-range.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-bug-ref-mutable-range.js deleted file mode 100644 index 8837c348eeba7..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-bug-ref-mutable-range.js +++ /dev/null @@ -1,12 +0,0 @@ -function Foo(props, ref) { - const value = {}; - if (cond1) { - mutate(value); - return ; - } - mutate(value); - if (cond2) { - return ; - } - return value; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md deleted file mode 100644 index a3b498b1c373a..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md +++ /dev/null @@ -1,29 +0,0 @@ - -## Input - -```javascript -/** - * Similar fixture to `error.todo-align-scopes-nested-block-structure`, but - * a simpler case. - */ -function useFoo(cond) { - let s = null; - if (cond) { - s = {}; - } else { - return null; - } - mutate(s); - return s; -} - -``` - - -## Error - -``` -Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:13) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts deleted file mode 100644 index 555bb713898a8..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts +++ /dev/null @@ -1,14 +0,0 @@ -/** - * Similar fixture to `error.todo-align-scopes-nested-block-structure`, but - * a simpler case. - */ -function useFoo(cond) { - let s = null; - if (cond) { - s = {}; - } else { - return null; - } - mutate(s); - return s; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts deleted file mode 100644 index 8e99f0435b562..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts +++ /dev/null @@ -1,53 +0,0 @@ -/** - * Fixture showing that it's not sufficient to only align direct scoped - * accesses of a block-fallthrough pair. - * Below is a simplified view of HIR blocks in this fixture. - * Note that here, s is mutated in both bb1 and bb4. However, neither - * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves. - * - * This means that we need to recursively visit all scopes accessed between - * a block and its fallthrough and extend the range of those scopes which overlap - * with an active block/fallthrough pair, - * - * bb0 - * ┌──────────────┐ - * │let s = null │ - * │test cond1 │ - * │ │ - * └┬─────────────┘ - * │ bb1 - * ├─►┌───────┐ - * │ │s = {} ├────┐ - * │ └───────┘ │ - * │ bb2 │ - * └─►┌───────┐ │ - * │return;│ │ - * └───────┘ │ - * bb3 │ - * ┌──────────────┐◄┘ - * │test cond2 │ - * │ │ - * └┬─────────────┘ - * │ bb4 - * ├─►┌─────────┐ - * │ │mutate(s)├─┐ - * ▼ └─────────┘ │ - * bb5 │ - * ┌───────────┐ │ - * │return s; │◄──┘ - * └───────────┘ - */ -function useFoo(cond1, cond2) { - let s = null; - if (cond1) { - s = {}; - } else { - return null; - } - - if (cond2) { - mutate(s); - } - - return s; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-iife-return-modified-later-logical.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-iife-return-modified-later-logical.expect.md deleted file mode 100644 index c14ae737e544a..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-iife-return-modified-later-logical.expect.md +++ /dev/null @@ -1,29 +0,0 @@ - -## Input - -```javascript -import { getNull } from "shared-runtime"; - -function Component(props) { - const items = (() => { - return getNull() ?? []; - })(); - items.push(props.a); - return items; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ a: {} }], -}; - -``` - - -## Error - -``` -Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 2:15(3:21) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-if.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-if.expect.md deleted file mode 100644 index df0192db2a006..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-if.expect.md +++ /dev/null @@ -1,41 +0,0 @@ - -## Input - -```javascript -function useFoo({ cond }) { - let items: any = {}; - b0: { - if (cond) { - // Mutable range of `items` begins here, but its reactive scope block - // should be aligned to above the if-branch - items = []; - } else { - break b0; - } - items.push(2); - } - return items; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [{ cond: true }], - sequentialRenders: [ - { cond: true }, - { cond: true }, - { cond: false }, - { cond: false }, - { cond: true }, - ], -}; - -``` - - -## Error - -``` -Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 6:11(7:15) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-label.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-label.expect.md deleted file mode 100644 index 62295b9a0ca0a..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-label.expect.md +++ /dev/null @@ -1,40 +0,0 @@ - -## Input - -```javascript -import { arrayPush } from "shared-runtime"; - -function useFoo({ cond, value }) { - let items; - label: { - items = []; - // Mutable range of `items` begins here, but its reactive scope block - // should be aligned to above the label-block - if (cond) break label; - arrayPush(items, value); - } - arrayPush(items, value); - return items; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [{ cond: true, value: 2 }], - sequentialRenders: [ - { cond: true, value: 2 }, - { cond: true, value: 2 }, - { cond: true, value: 3 }, - { cond: false, value: 3 }, - ], -}; - -``` - - -## Error - -``` -Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 3:14(4:18) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-try.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-try.expect.md deleted file mode 100644 index 95cdbe5aeea76..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-reactive-scope-overlaps-try.expect.md +++ /dev/null @@ -1,36 +0,0 @@ - -## Input - -```javascript -import { arrayPush } from "shared-runtime"; - -function useFoo({ value }) { - let items = null; - try { - // Mutable range of `items` begins here, but its reactive scope block - // should be aligned to above the try-block - items = []; - arrayPush(items, value); - } catch { - // ignore - } - mutate(items); - return items; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [{ value: 2 }], - sequentialRenders: [{ value: 2 }, { value: 2 }, { value: 3 }], -}; - -``` - - -## Error - -``` -Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:19(5:22) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-ref-mutable-range.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-ref-mutable-range.expect.md new file mode 100644 index 0000000000000..69ae9aa3d6c2a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-ref-mutable-range.expect.md @@ -0,0 +1,83 @@ + +## Input + +```javascript +import { Stringify, identity, mutate, CONST_TRUE } from "shared-runtime"; + +function Foo(props, ref) { + const value = {}; + if (CONST_TRUE) { + mutate(value); + return ; + } + mutate(value); + if (CONST_TRUE) { + return ; + } + return value; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}, { current: "fake-ref-object" }], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { Stringify, identity, mutate, CONST_TRUE } from "shared-runtime"; + +function Foo(props, ref) { + const $ = _c(5); + let value; + let t0; + if ($[0] !== ref) { + t0 = Symbol.for("react.early_return_sentinel"); + bb0: { + value = {}; + if (CONST_TRUE) { + mutate(value); + t0 = ; + break bb0; + } + + mutate(value); + if (CONST_TRUE) { + const t1 = identity(ref); + let t2; + if ($[3] !== t1) { + t2 = ; + $[3] = t1; + $[4] = t2; + } else { + t2 = $[4]; + } + t0 = t2; + break bb0; + } + } + $[0] = ref; + $[1] = value; + $[2] = t0; + } else { + value = $[1]; + t0 = $[2]; + } + if (t0 !== Symbol.for("react.early_return_sentinel")) { + return t0; + } + return value; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}, { current: "fake-ref-object" }], +}; + +``` + +### Eval output +(kind: ok)
{"ref":{"current":"fake-ref-object"}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-ref-mutable-range.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-ref-mutable-range.tsx new file mode 100644 index 0000000000000..106ebd772e611 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-ref-mutable-range.tsx @@ -0,0 +1,19 @@ +import { Stringify, identity, mutate, CONST_TRUE } from "shared-runtime"; + +function Foo(props, ref) { + const value = {}; + if (CONST_TRUE) { + mutate(value); + return ; + } + mutate(value); + if (CONST_TRUE) { + return ; + } + return value; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}, { current: "fake-ref-object" }], +}; From 9c2c7c670cfc51a63a9e1353c55d1d3beecb29d1 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 25 Jun 2024 16:03:58 -0400 Subject: [PATCH 21/25] [compiler][ez] Patch Array.concat object shape to capture callee ghstack-source-id: 503fbf8f76e1cd10095b44cf180863221710774f Pull Request resolved: https://github.com/facebook/react/pull/30074 --- .../src/HIR/ObjectShape.ts | 2 +- ... => array-concat-should-capture.expect.md} | 24 +++---- ...ture.ts => array-concat-should-capture.ts} | 0 ...k-reordering-deplist-controlflow.expect.md | 61 ++++++++---------- ...k-reordering-depslist-assignment.expect.md | 39 +++++------- ...o-reordering-depslist-assignment.expect.md | 43 ++++++------- ...-reordering-depslist-controlflow.expect.md | 63 +++++++++---------- .../packages/snap/src/SproutTodoFilter.ts | 1 - 8 files changed, 100 insertions(+), 133 deletions(-) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-array-concat-should-capture.expect.md => array-concat-should-capture.expect.md} (84%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-array-concat-should-capture.ts => array-concat-should-capture.ts} (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index 0a3451023cd4c..be2fca54f9287 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -237,7 +237,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [ kind: "Object", shapeId: BuiltInArrayId, }, - calleeEffect: Effect.Read, + calleeEffect: Effect.Capture, returnValueKind: ValueKind.Mutable, }), ], diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-concat-should-capture.expect.md similarity index 84% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-concat-should-capture.expect.md index 41271ffaf0939..6d34414bcd3ac 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-concat-should-capture.expect.md @@ -40,24 +40,17 @@ import { mutate } from "shared-runtime"; * itself. */ function Foo(t0) { - const $ = _c(3); + const $ = _c(2); const { inputNum } = t0; - let t1; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t1 = [{ a: 1 }, {}]; - $[0] = t1; - } else { - t1 = $[0]; - } - const arr1 = t1; let arr2; - if ($[1] !== inputNum) { + if ($[0] !== inputNum) { + const arr1 = [{ a: 1 }, {}]; arr2 = arr1.concat([1, inputNum]); mutate(arr2[0]); - $[1] = inputNum; - $[2] = arr2; + $[0] = inputNum; + $[1] = arr2; } else { - arr2 = $[2]; + arr2 = $[1]; } return arr2; } @@ -69,4 +62,7 @@ export const FIXTURE_ENTRYPOINT = { }; ``` - \ No newline at end of file + +### Eval output +(kind: ok) [{"a":1,"wat0":"joe"},{},1,2] +[{"a":1,"wat0":"joe"},{},1,3] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-concat-should-capture.ts similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-concat-should-capture.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-reordering-deplist-controlflow.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-reordering-deplist-controlflow.expect.md index f47b6089ebcf7..c96844716eaa4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-reordering-deplist-controlflow.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-reordering-deplist-controlflow.expect.md @@ -40,53 +40,46 @@ import { useCallback } from "react"; import { Stringify } from "shared-runtime"; function Foo(t0) { - const $ = _c(11); + const $ = _c(9); const { arr1, arr2, foo } = t0; let t1; - if ($[0] !== arr1) { - t1 = [arr1]; - $[0] = arr1; - $[1] = t1; - } else { - t1 = $[1]; - } - const x = t1; - let t2; let getVal1; - if ($[2] !== foo || $[3] !== x || $[4] !== arr2) { + if ($[0] !== arr1 || $[1] !== foo || $[2] !== arr2) { + const x = [arr1]; + let y; y = []; - let t3; - if ($[7] === Symbol.for("react.memo_cache_sentinel")) { - t3 = () => ({ x: 2 }); - $[7] = t3; + let t2; + if ($[5] === Symbol.for("react.memo_cache_sentinel")) { + t2 = () => ({ x: 2 }); + $[5] = t2; } else { - t3 = $[7]; + t2 = $[5]; } - getVal1 = t3; + getVal1 = t2; - t2 = () => [y]; + t1 = () => [y]; foo ? (y = x.concat(arr2)) : y; - $[2] = foo; - $[3] = x; - $[4] = arr2; - $[5] = t2; - $[6] = getVal1; + $[0] = arr1; + $[1] = foo; + $[2] = arr2; + $[3] = t1; + $[4] = getVal1; } else { - t2 = $[5]; - getVal1 = $[6]; + t1 = $[3]; + getVal1 = $[4]; } - const getVal2 = t2; - let t3; - if ($[8] !== getVal1 || $[9] !== getVal2) { - t3 = ; - $[8] = getVal1; - $[9] = getVal2; - $[10] = t3; + const getVal2 = t1; + let t2; + if ($[6] !== getVal1 || $[7] !== getVal2) { + t2 = ; + $[6] = getVal1; + $[7] = getVal2; + $[8] = t2; } else { - t3 = $[10]; + t2 = $[8]; } - return t3; + return t2; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-reordering-depslist-assignment.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-reordering-depslist-assignment.expect.md index a11b5f709e280..2401298c69ad5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-reordering-depslist-assignment.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-reordering-depslist-assignment.expect.md @@ -36,38 +36,31 @@ import { Stringify } from "shared-runtime"; // We currently produce invalid output (incorrect scoping for `y` declaration) function useFoo(arr1, arr2) { - const $ = _c(7); + const $ = _c(5); let t0; - if ($[0] !== arr1) { - t0 = [arr1]; + if ($[0] !== arr1 || $[1] !== arr2) { + const x = [arr1]; + + let y; + t0 = () => ({ y }); + + (y = x.concat(arr2)), y; $[0] = arr1; - $[1] = t0; + $[1] = arr2; + $[2] = t0; } else { - t0 = $[1]; + t0 = $[2]; } - const x = t0; + const getVal = t0; let t1; - if ($[2] !== x || $[3] !== arr2) { - let y; - t1 = () => ({ y }); - - (y = x.concat(arr2)), y; - $[2] = x; - $[3] = arr2; + if ($[3] !== getVal) { + t1 = ; + $[3] = getVal; $[4] = t1; } else { t1 = $[4]; } - const getVal = t1; - let t2; - if ($[5] !== getVal) { - t2 = ; - $[5] = getVal; - $[6] = t2; - } else { - t2 = $[6]; - } - return t2; + return t1; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md index c7fd376912f5c..e485b6fc7ad65 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md @@ -30,38 +30,31 @@ import { c as _c } from "react/compiler-runtime"; import { useMemo } from "react"; function useFoo(arr1, arr2) { - const $ = _c(7); - let t0; - if ($[0] !== arr1) { - t0 = [arr1]; - $[0] = arr1; - $[1] = t0; - } else { - t0 = $[1]; - } - const x = t0; + const $ = _c(5); let y; - if ($[2] !== x || $[3] !== arr2) { + if ($[0] !== arr1 || $[1] !== arr2) { + const x = [arr1]; + y; (y = x.concat(arr2)), y; - $[2] = x; - $[3] = arr2; - $[4] = y; + $[0] = arr1; + $[1] = arr2; + $[2] = y; } else { - y = $[4]; + y = $[2]; } - let t1; - const t2 = y; - let t3; - if ($[5] !== t2) { - t3 = { y: t2 }; - $[5] = t2; - $[6] = t3; + let t0; + const t1 = y; + let t2; + if ($[3] !== t1) { + t2 = { y: t1 }; + $[3] = t1; + $[4] = t2; } else { - t3 = $[6]; + t2 = $[4]; } - t1 = t3; - return t1; + t0 = t2; + return t0; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-controlflow.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-controlflow.expect.md index 577db4ae91d57..787ff42def026 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-controlflow.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-controlflow.expect.md @@ -40,55 +40,48 @@ import { useMemo } from "react"; import { Stringify } from "shared-runtime"; function Foo(t0) { - const $ = _c(11); + const $ = _c(9); const { arr1, arr2, foo } = t0; let t1; - if ($[0] !== arr1) { - t1 = [arr1]; - $[0] = arr1; - $[1] = t1; - } else { - t1 = $[1]; - } - const x = t1; - let t2; let val1; - if ($[2] !== foo || $[3] !== x || $[4] !== arr2) { + if ($[0] !== arr1 || $[1] !== foo || $[2] !== arr2) { + const x = [arr1]; + let y; y = []; + let t2; let t3; - let t4; - if ($[7] === Symbol.for("react.memo_cache_sentinel")) { - t4 = { x: 2 }; - $[7] = t4; + if ($[5] === Symbol.for("react.memo_cache_sentinel")) { + t3 = { x: 2 }; + $[5] = t3; } else { - t4 = $[7]; + t3 = $[5]; } - t3 = t4; - val1 = t3; + t2 = t3; + val1 = t2; foo ? (y = x.concat(arr2)) : y; - t2 = (() => [y])(); - $[2] = foo; - $[3] = x; - $[4] = arr2; - $[5] = t2; - $[6] = val1; + t1 = (() => [y])(); + $[0] = arr1; + $[1] = foo; + $[2] = arr2; + $[3] = t1; + $[4] = val1; } else { - t2 = $[5]; - val1 = $[6]; + t1 = $[3]; + val1 = $[4]; } - const val2 = t2; - let t3; - if ($[8] !== val1 || $[9] !== val2) { - t3 = ; - $[8] = val1; - $[9] = val2; - $[10] = t3; + const val2 = t1; + let t2; + if ($[6] !== val1 || $[7] !== val2) { + t2 = ; + $[6] = val1; + $[7] = val2; + $[8] = t2; } else { - t3 = $[10]; + t2 = $[8]; } - return t3; + return t2; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 52834a2d1a146..815285fed0a7a 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -488,7 +488,6 @@ const skipFilter = new Set([ "original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block", "original-reactive-scopes-fork/bug-hoisted-declaration-with-scope", "bug-codegen-inline-iife", - "bug-array-concat-should-capture", // 'react-compiler-runtime' not yet supported "flag-enable-emit-hook-guards", From 9262761f1c25bfe52a4585419fad16dd01a07424 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 25 Jun 2024 16:03:58 -0400 Subject: [PATCH 22/25] [compiler][ez] Add more Array.prototype methods Adds Array.prototype methods that return primitives or other arrays -- naive type inference can be really helpful in reducing mutable ranges -> achieving higher quality memoization. Also copies Array.prototype methods to our mixed read-only JSON-like object shape. (Inspired after going through some suboptimal internal compilation outputs.) ghstack-source-id: 0bfad11180992fc5db61a1c5f23954f48acf07b8 Pull Request resolved: https://github.com/facebook/react/pull/30075 --- .../src/HIR/ObjectShape.ts | 129 +++++++++++++++++- 1 file changed, 128 insertions(+), 1 deletion(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index be2fca54f9287..5aa19ed09b763 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -218,6 +218,36 @@ addObject(BUILTIN_SHAPES, BuiltInPropsId, [ /* Built-in array shape */ addObject(BUILTIN_SHAPES, BuiltInArrayId, [ + [ + "indexOf", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.Read, + returnType: { kind: "Primitive" }, + calleeEffect: Effect.Read, + returnValueKind: ValueKind.Primitive, + }), + ], + [ + "includes", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.Read, + returnType: { kind: "Primitive" }, + calleeEffect: Effect.Read, + returnValueKind: ValueKind.Primitive, + }), + ], + [ + "pop", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: null, + returnType: { kind: "Poly" }, + calleeEffect: Effect.Store, + returnValueKind: ValueKind.Mutable, + }), + ], [ "at", addFunction(BUILTIN_SHAPES, [], { @@ -252,6 +282,19 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [ returnValueKind: ValueKind.Primitive, }), ], + [ + "slice", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.Read, + returnType: { + kind: "Object", + shapeId: BuiltInArrayId, + }, + calleeEffect: Effect.Capture, + returnValueKind: ValueKind.Mutable, + }), + ], [ "map", addFunction(BUILTIN_SHAPES, [], { @@ -353,7 +396,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [ "join", addFunction(BUILTIN_SHAPES, [], { positionalParams: [], - restParam: Effect.ConditionallyMutate, + restParam: Effect.Read, returnType: PRIMITIVE_TYPE, calleeEffect: Effect.Read, returnValueKind: ValueKind.Primitive, @@ -478,6 +521,90 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [ noAlias: true, }), ], + [ + "concat", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.Capture, + returnType: { + kind: "Object", + shapeId: BuiltInArrayId, + }, + calleeEffect: Effect.Capture, + returnValueKind: ValueKind.Mutable, + }), + ], + [ + "slice", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.Read, + returnType: { + kind: "Object", + shapeId: BuiltInArrayId, + }, + calleeEffect: Effect.Capture, + returnValueKind: ValueKind.Mutable, + }), + ], + [ + "every", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.ConditionallyMutate, + returnType: { kind: "Primitive" }, + calleeEffect: Effect.ConditionallyMutate, + returnValueKind: ValueKind.Primitive, + noAlias: true, + mutableOnlyIfOperandsAreMutable: true, + }), + ], + [ + "some", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.ConditionallyMutate, + returnType: { kind: "Primitive" }, + calleeEffect: Effect.ConditionallyMutate, + returnValueKind: ValueKind.Primitive, + noAlias: true, + mutableOnlyIfOperandsAreMutable: true, + }), + ], + [ + "find", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.ConditionallyMutate, + returnType: { kind: "Poly" }, + calleeEffect: Effect.ConditionallyMutate, + returnValueKind: ValueKind.Mutable, + noAlias: true, + mutableOnlyIfOperandsAreMutable: true, + }), + ], + [ + "findIndex", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.ConditionallyMutate, + returnType: { kind: "Primitive" }, + calleeEffect: Effect.ConditionallyMutate, + returnValueKind: ValueKind.Primitive, + noAlias: true, + mutableOnlyIfOperandsAreMutable: true, + }), + ], + [ + "join", + addFunction(BUILTIN_SHAPES, [], { + positionalParams: [], + restParam: Effect.Read, + returnType: PRIMITIVE_TYPE, + calleeEffect: Effect.Read, + returnValueKind: ValueKind.Primitive, + }), + ], ["*", { kind: "Object", shapeId: BuiltInMixedReadonlyId }], ]); From 7d9861e70642719b120a5236ade5124912e42a92 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 25 Jun 2024 16:06:21 -0400 Subject: [PATCH 23/25] [compiler][hir] Correctly remove non-existent terminal preds when pruning labels Missed this initially in `pruneUnusedLabelsHIR`. It wasn't an active bug as `preds` wasn't referenced by later passes, until #30079 ghstack-source-id: 3e151b74c31554299e870f001c0ac3f72706318c Pull Request resolved: https://github.com/facebook/react/pull/30076 --- .../src/Entrypoint/Pipeline.ts | 3 +++ ...sExist.ts => AssertTerminalBlocksExist.ts} | 22 ++++++++++++++++++- .../src/HIR/PruneUnusedLabelsHIR.ts | 10 +++++++++ .../src/HIR/index.ts | 5 ++++- 4 files changed, 38 insertions(+), 2 deletions(-) rename compiler/packages/babel-plugin-react-compiler/src/HIR/{AssertTerminalSuccessorsExist.ts => AssertTerminalBlocksExist.ts} (54%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 863eca5dcf351..2e7613f0a2adf 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -13,6 +13,7 @@ import { HIRFunction, ReactiveFunction, assertConsistentIdentifiers, + assertTerminalPredsExist, assertTerminalSuccessorsExist, assertValidBlockNesting, assertValidMutableRanges, @@ -303,6 +304,8 @@ function* runWithEnvironment( name: "FlattenScopesWithHooksOrUseHIR", value: hir, }); + assertTerminalSuccessorsExist(hir); + assertTerminalPredsExist(hir); } const reactiveFunction = buildReactiveFunction(hir); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalSuccessorsExist.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalBlocksExist.ts similarity index 54% rename from compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalSuccessorsExist.ts rename to compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalBlocksExist.ts index 493ff54c03fec..e5dbb1b8dbfcc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalSuccessorsExist.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalBlocksExist.ts @@ -8,7 +8,7 @@ import { CompilerError } from "../CompilerError"; import { GeneratedSource, HIRFunction } from "./HIR"; import { printTerminal } from "./PrintHIR"; -import { mapTerminalSuccessors } from "./visitors"; +import { eachTerminalSuccessor, mapTerminalSuccessors } from "./visitors"; export function assertTerminalSuccessorsExist(fn: HIRFunction): void { for (const [, block] of fn.body.blocks) { @@ -25,3 +25,23 @@ export function assertTerminalSuccessorsExist(fn: HIRFunction): void { }); } } + +export function assertTerminalPredsExist(fn: HIRFunction): void { + for (const [, block] of fn.body.blocks) { + for (const pred of block.preds) { + const predBlock = fn.body.blocks.get(pred); + CompilerError.invariant(predBlock != null, { + reason: "Expected predecessor block to exist", + description: `Block ${block.id} references non-existent ${pred}`, + loc: GeneratedSource, + }); + CompilerError.invariant( + [...eachTerminalSuccessor(predBlock.terminal)].includes(block.id), + { + reason: "Terminal successor does not reference correct predecessor", + loc: GeneratedSource, + } + ); + } + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PruneUnusedLabelsHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PruneUnusedLabelsHIR.ts index 10714d5d87971..aef414b2d2b24 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PruneUnusedLabelsHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PruneUnusedLabelsHIR.ts @@ -67,4 +67,14 @@ export function pruneUnusedLabelsHIR(fn: HIRFunction): void { fn.body.blocks.delete(fallthroughId); rewrites.set(fallthroughId, labelId); } + + for (const [_, block] of fn.body.blocks) { + for (const pred of block.preds) { + const rewritten = rewrites.get(pred); + if (rewritten != null) { + block.preds.delete(pred); + block.preds.add(rewritten); + } + } + } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/index.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/index.ts index b17d3a09d08b4..bef4f1cb95d28 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/index.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/index.ts @@ -6,7 +6,10 @@ */ export { assertConsistentIdentifiers } from "./AssertConsistentIdentifiers"; -export { assertTerminalSuccessorsExist } from "./AssertTerminalSuccessorsExist"; +export { + assertTerminalSuccessorsExist, + assertTerminalPredsExist, +} from "./AssertTerminalBlocksExist"; export { assertValidBlockNesting } from "./AssertValidBlockNesting"; export { assertValidMutableRanges } from "./AssertValidMutableRanges"; export { lower } from "./BuildHIR"; From 4bfab0783204810cb51b9dda24464bb57777eb97 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 25 Jun 2024 16:06:21 -0400 Subject: [PATCH 24/25] [compiler][patch] Patch O(n^2) traversal in validatePreserveMemo Double checked by syncing internally and verifying the # of `visitInstruction` calls with unique `InstructionId`s. This is a bit of an awkward pattern though. A cleaner alternative might be to override `visitValue` and store its results in a sidemap (instead of returning) ghstack-source-id: f6797d765224fb49c7d26cd377319662830d7348 Pull Request resolved: https://github.com/facebook/react/pull/30077 --- .../ValidatePreservedManualMemoization.ts | 37 +++++++--- ...epro-slow-validate-preserve-memo.expect.md | 68 +++++++++++++++++++ .../repro-slow-validate-preserve-memo.ts | 21 ++++++ .../snap/src/sprout/shared-runtime.ts | 16 +++++ 4 files changed, 134 insertions(+), 8 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-slow-validate-preserve-memo.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-slow-validate-preserve-memo.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index b15ef94d92e58..365e3aa88d4af 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -280,7 +280,13 @@ class Visitor extends ReactiveFunctionVisitor { scopeMapping = new Map(); temporaries: Map = new Map(); - collectMaybeMemoDependencies( + /** + * Recursively visit values and instructions to collect declarations + * and property loads. + * @returns a @{ManualMemoDependency} representing the variable + + * property reads represented by @value + */ + recordDepsInValue( value: ReactiveValue, state: VisitorState ): ManualMemoDependency | null { @@ -289,16 +295,28 @@ class Visitor extends ReactiveFunctionVisitor { for (const instr of value.instructions) { this.visitInstruction(instr, state); } - const result = this.collectMaybeMemoDependencies(value.value, state); - + const result = this.recordDepsInValue(value.value, state); return result; } case "OptionalExpression": { - return this.collectMaybeMemoDependencies(value.value, state); + return this.recordDepsInValue(value.value, state); + } + case "ReactiveFunctionValue": { + CompilerError.throwTodo({ + reason: + "Handle ReactiveFunctionValue in ValidatePreserveManualMemoization", + loc: value.loc, + }); + } + case "ConditionalExpression": { + this.recordDepsInValue(value.test, state); + this.recordDepsInValue(value.consequent, state); + this.recordDepsInValue(value.alternate, state); + return null; } - case "ReactiveFunctionValue": - case "ConditionalExpression": case "LogicalExpression": { + this.recordDepsInValue(value.left, state); + this.recordDepsInValue(value.right, state); return null; } default: { @@ -336,7 +354,7 @@ class Visitor extends ReactiveFunctionVisitor { state.manualMemoState.decls.add(lvalId); } - const maybeDep = this.collectMaybeMemoDependencies(value, state); + const maybeDep = this.recordDepsInValue(value, state); if (lvalId != null) { if (maybeDep != null) { temporaries.set(lvalId, maybeDep); @@ -400,7 +418,10 @@ class Visitor extends ReactiveFunctionVisitor { instruction: ReactiveInstruction, state: VisitorState ): void { - this.traverseInstruction(instruction, state); + /** + * We don't invoke traverseInstructions because `recordDepsInValue` + * recursively visits ReactiveValues and instructions + */ this.recordTemporaries(instruction, state); if (instruction.value.kind === "StartMemoize") { let depsFromSource: Array | null = null; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-slow-validate-preserve-memo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-slow-validate-preserve-memo.expect.md new file mode 100644 index 0000000000000..4cfd4cd247819 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-slow-validate-preserve-memo.expect.md @@ -0,0 +1,68 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import { Builder } from "shared-runtime"; +function useTest({ isNull, data }: { isNull: boolean; data: string }) { + const result = Builder.makeBuilder(isNull, "hello world") + ?.push("1", 2) + ?.push(3, { + a: 4, + b: 5, + c: data, + }) + ?.push(6, data) + ?.push(7, "8") + ?.push("8", Builder.makeBuilder(!isNull)?.push(9).vals)?.vals; + return result; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [{ isNull: false, data: "param" }], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees + +import { Builder } from "shared-runtime"; +function useTest(t0) { + const $ = _c(3); + const { isNull, data } = t0; + let t1; + if ($[0] !== isNull || $[1] !== data) { + t1 = Builder.makeBuilder(isNull, "hello world") + ?.push("1", 2) + ?.push(3, { a: 4, b: 5, c: data }) + ?.push( + 6, + + data, + ) + ?.push(7, "8") + ?.push("8", Builder.makeBuilder(!isNull)?.push(9).vals)?.vals; + $[0] = isNull; + $[1] = data; + $[2] = t1; + } else { + t1 = $[2]; + } + const result = t1; + return result; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [{ isNull: false, data: "param" }], +}; + +``` + +### Eval output +(kind: ok) ["hello world","1",2,3,{"a":4,"b":5,"c":"param"},6,"param",7,"8","8",null] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-slow-validate-preserve-memo.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-slow-validate-preserve-memo.ts new file mode 100644 index 0000000000000..0fd75e4fc361e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-slow-validate-preserve-memo.ts @@ -0,0 +1,21 @@ +// @validatePreserveExistingMemoizationGuarantees + +import { Builder } from "shared-runtime"; +function useTest({ isNull, data }: { isNull: boolean; data: string }) { + const result = Builder.makeBuilder(isNull, "hello world") + ?.push("1", 2) + ?.push(3, { + a: 4, + b: 5, + c: data, + }) + ?.push(6, data) + ?.push(7, "8") + ?.push("8", Builder.makeBuilder(!isNull)?.push(9).vals)?.vals; + return result; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [{ isNull: false, data: "param" }], +}; diff --git a/compiler/packages/snap/src/sprout/shared-runtime.ts b/compiler/packages/snap/src/sprout/shared-runtime.ts index 7d4687218d0e2..a67ab298ada0c 100644 --- a/compiler/packages/snap/src/sprout/shared-runtime.ts +++ b/compiler/packages/snap/src/sprout/shared-runtime.ts @@ -312,6 +312,22 @@ export function toJSON(value: any, invokeFns: boolean = false): string { return val; }); } +export class Builder { + vals: Array = []; + static makeBuilder(isNull: boolean, ...args: Array): Builder | null { + if (isNull) { + return null; + } else { + const builder = new Builder(); + builder.push(...args); + return builder; + } + } + push(...args: Array): Builder { + this.vals.push(...args); + return this; + } +} export const ObjectWithHooks = { useFoo(): number { From d878489431e2c6cf69982b51403f8e1f68f4e73d Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 25 Jun 2024 16:06:21 -0400 Subject: [PATCH 25/25] [compiler][ez] PrintHIR prints optional flag for debugging Adding the equivalent of [PrintReactiveFunction:OptionalExpression](https://github.com/facebook/react/blob/f5d2feb4f069a36140d5e605f5eebc52badcc214/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts#L218) to `PrintHIR`. ghstack-source-id: 5b175f6f62f72a867a00c52c4f825fd396d450a9 Pull Request resolved: https://github.com/facebook/react/pull/30078 --- .../packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 443a2522ee18b..df7d2698f4b29 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -194,7 +194,7 @@ export function printTerminal(terminal: Terminal): Array | string { break; } case "optional": { - value = `[${terminal.id}] Optional test:bb${terminal.test} fallthrough=bb${terminal.fallthrough}`; + value = `[${terminal.id}] Optional (optional=${terminal.optional}) test:bb${terminal.test} fallthrough=bb${terminal.fallthrough}`; break; } case "throw": {