diff --git a/.circleci/config.yml b/.circleci/config.yml index 894e52fab609d..18372084d3685 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 @@ -345,31 +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: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - parameters: - args: - type: string - steps: - - checkout - - setup_node_modules - - run: yarn test <> --ci - yarn_test_build: docker: *docker environment: *environment @@ -382,7 +357,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 @@ -432,42 +407,6 @@ workflows: build_and_test: unless: << pipeline.parameters.prerelease_commit_sha >> jobs: - - check_generated_fizz_runtime: - filters: - 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: 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/runtime_fizz.yml b/.github/workflows/runtime_fizz.yml new file mode 100644 index 0000000000000..43097728e37de --- /dev/null +++ b/.github/workflows/runtime_fizz.yml @@ -0,0 +1,30 @@ +name: (Runtime) 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) 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 81% rename from .github/workflows/fuzz_tests.yml rename to .github/workflows/runtime_fuzz_tests.yml index 492dd7dc4c81d..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 * * * * @@ -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 new file mode 100644 index 0000000000000..118a520ad59c9 --- /dev/null +++ b/.github/workflows/runtime_test.yml @@ -0,0 +1,85 @@ +name: (Runtime) Test + +on: + push: + branches: [main] + pull_request: + 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 ${{ matrix.params }} (Shard ${{ matrix.shard_id }}) + runs-on: ubuntu-latest + needs: build_test_params + strategy: + matrix: + 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 + 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=github --shard=${{ matrix.shard_id }}/${{ env.SHARD_COUNT }} 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/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", 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 82e8903d1b456..b6d4d92fda136 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -96,7 +96,7 @@ import { validatePreservedManualMemoization, validateUseMemo, } from "../Validation"; -import { propagateScopeDependenciesHIR } from "../HIR/PropagateScopeDepsHIR"; +import { propagateScopeDependenciesHIR } from "../HIR/PropagateScopeDependenciesHIR"; export type CompilerPipelineValue = | { kind: "ast"; name: string; value: CodegenFunction } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts new file mode 100644 index 0000000000000..40e272c3d7900 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -0,0 +1,436 @@ +import { CompilerError } from "../CompilerError"; +import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables"; +import { Set_intersect, Set_union, getOrInsertDefault } from "../Utils/utils"; +import { + BlockId, + GeneratedSource, + HIRFunction, + Identifier, + IdentifierId, + InstructionId, + Place, + ReactiveScopeDependency, + ScopeId, +} from "./HIR"; + +type CollectHoistablePropertyLoadsResult = { + nodes: Map; + temporaries: Map; + properties: Map; +}; + +export function collectHoistablePropertyLoads( + fn: HIRFunction, + usedOutsideDeclaringScope: Set +): CollectHoistablePropertyLoadsResult { + const result = new TemporariesSideMap(); + + const functionExprRvals = fn.env.config.enableTreatFunctionDepsAsConditional + ? collectFunctionExpressionRValues(fn) + : new Set(); + const nodes = collectNodes( + fn, + functionExprRvals, + usedOutsideDeclaringScope, + result + ); + deriveNonNull(fn, nodes); + + return { + nodes, + temporaries: result.temporaries, + properties: result.properties, + }; +} + +export type BlockInfo = { + blockId: BlockId; + scope: ScopeId | null; + preds: Set; + assumedNonNullObjects: Set; +}; + +/** + * Tree data structure to dedupe property loads (e.g. a.b.c) + * and make computing sets and intersections simpler. + */ +type RootNode = { + properties: Map; + parent: null; + // Recorded to make later computations simpler + fullPath: ReactiveScopeDependency; + root: Identifier; +}; + +type PropertyLoadNode = + | { + properties: Map; + parent: PropertyLoadNode; + fullPath: ReactiveScopeDependency; + } + | RootNode; + +class Tree { + roots: Map = new Map(); + + #getOrCreateRoot(identifier: Identifier): PropertyLoadNode { + // roots can always be accessed unconditionally in JS + let rootNode = this.roots.get(identifier); + + if (rootNode === undefined) { + rootNode = { + root: identifier, + properties: new Map(), + fullPath: { + identifier, + path: [], + }, + parent: null, + }; + this.roots.set(identifier, rootNode); + } + return rootNode; + } + + static #getOrMakeProperty( + node: PropertyLoadNode, + property: string + ): PropertyLoadNode { + let child = node.properties.get(property); + if (child == null) { + child = { + properties: new Map(), + parent: node, + fullPath: { + identifier: node.fullPath.identifier, + path: node.fullPath.path.concat([property]), + }, + }; + node.properties.set(property, child); + } + return child; + } + + add(n: ReactiveScopeDependency): PropertyLoadNode { + let currNode = this.#getOrCreateRoot(n.identifier); + // We add ReactiveScopeDependencies sequentially (e.g. a.b before a.b.c), + // so subpaths should already exist. + for (let i = 0; i < n.path.length - 1; i++) { + currNode = assertNonNull(currNode.properties.get(n.path[i])); + } + + currNode = Tree.#getOrMakeProperty(currNode, n.path.at(-1)!); + return currNode; + } +} + +/** + * We currently lower function expression dependencies inline before the + * function expression instruction. This causes our HIR to deviate from + * JS specs. + * + * For example, note that instructions 0-2 in the below HIR are incorrectly + * hoisted. + * ```js + * // Input + * function Component(props) { + * const fn = () => cond && read(props.a.b); + * // ... + * } + * + * // HIR: + * [0] $0 = LoadLocal "props" + * [1] $1 = PropertyLoad $0, "a" + * [2] $2 = PropertyLoad $1, "b" + * [3] $3 = FunctionExpression deps=[$2] context=[$0] { + * ... + * } + * + * TODO: rewrite function expression deps + */ +function collectFunctionExpressionRValues(fn: HIRFunction): Set { + const result = new Set(); + const loads = new Map(); + + for (const [_, block] of fn.body.blocks) { + for (const instr of block.instructions) { + if (instr.value.kind === "LoadLocal") { + loads.set(instr.lvalue.identifier.id, instr.value.place.identifier.id); + } else if (instr.value.kind === "PropertyLoad") { + loads.set(instr.lvalue.identifier.id, instr.value.object.identifier.id); + } else if (instr.value.kind === "FunctionExpression") { + for (const dep of instr.value.loweredFunc.dependencies) { + result.add(dep.identifier.id); + } + } + } + } + + // don't iterate newly added objects as optimization + for (const res of result) { + let curr = loads.get(res); + while (curr != null) { + result.add(curr); + curr = loads.get(curr); + } + } + return result; +} + +class TemporariesSideMap { + temporaries: Map = new Map(); + properties: Map = new Map(); + tree: Tree = new Tree(); + + declareTemporary(from: Identifier, to: Identifier): void { + this.temporaries.set(from, to); + } + + declareProperty( + lvalue: Place, + object: Place, + propertyName: string, + shouldDeclare: boolean + ): PropertyLoadNode { + // temporaries contains object if this is a property load chain from a named variable + // Otherwise, there is a non-trivial expression + const resolvedObject = + this.temporaries.get(object.identifier) ?? object.identifier; + + const resolvedDependency = this.properties.get(resolvedObject); + let property: ReactiveScopeDependency; + if (resolvedDependency == null) { + property = { + identifier: resolvedObject, + path: [propertyName], + }; + } else { + property = { + identifier: resolvedDependency.identifier, + path: [...resolvedDependency.path, propertyName], + }; + } + + if (shouldDeclare) { + this.properties.set(lvalue.identifier, property); + } + return this.tree.add(property); + } +} + +function collectNodes( + fn: HIRFunction, + functionExprRvals: Set, + usedOutsideDeclaringScope: Set, + c: TemporariesSideMap +): Map { + const nodes = new Map(); + const scopeStartBlocks = new Map(); + for (const [blockId, block] of fn.body.blocks) { + const assumedNonNullObjects = new Set(); + for (const instr of block.instructions) { + const { value, lvalue } = instr; + const usedOutside = usedOutsideDeclaringScope.has(lvalue.identifier.id); + if (value.kind === "PropertyLoad") { + const propertyNode = c.declareProperty( + lvalue, + value.object, + value.property, + !usedOutside + ); + if ( + !functionExprRvals.has(lvalue.identifier.id) && + !isMutable(instr, value.object) + ) { + let curr = propertyNode.parent; + while (curr != null) { + assumedNonNullObjects.add(curr); + curr = curr.parent; + } + } + } else if (value.kind === "LoadLocal") { + if ( + lvalue.identifier.name == null && + value.place.identifier.name !== null && + !usedOutside + ) { + c.declareTemporary(lvalue.identifier, value.place.identifier); + } + } + /** + * Note that we do not record StoreLocals as this runs after ExitSSA. + * As a result, an expression like `(a ?? b).c` is represented as two + * StoreLocals to the same identifier id. + */ + } + + if ( + block.terminal.kind === "scope" || + block.terminal.kind === "pruned-scope" + ) { + scopeStartBlocks.set(block.terminal.block, block.terminal.scope.id); + } + + nodes.set(blockId, { + blockId, + scope: scopeStartBlocks.get(blockId) ?? null, + preds: block.preds, + assumedNonNullObjects, + }); + } + return nodes; +} + +function deriveNonNull(fn: HIRFunction, nodes: Map): void { + // block -> successors sidemap + const succ = new Map>(); + const terminalPreds = new Set(); + + for (const [blockId, block] of fn.body.blocks) { + for (const pred of block.preds) { + const predVal = getOrInsertDefault(succ, pred, new Set()); + predVal.add(blockId); + } + if (block.terminal.kind === "throw" || block.terminal.kind === "return") { + terminalPreds.add(blockId); + } + } + + function recursivelyDeriveNonNull( + nodeId: BlockId, + kind: "succ" | "pred", + traversalState: Map, + result: Map> + ): boolean { + if (traversalState.has(nodeId)) { + return false; + } + traversalState.set(nodeId, "active"); + + const node = nodes.get(nodeId); + if (node == null) { + CompilerError.invariant(false, { + reason: `Bad node ${nodeId}, kind: ${kind}`, + loc: GeneratedSource, + }); + } + const neighbors = Array.from( + kind === "succ" ? succ.get(nodeId) ?? [] : node.preds + ); + + let changed = false; + for (const pred of neighbors) { + if (!traversalState.has(pred)) { + const neighborChanged = recursivelyDeriveNonNull( + pred, + kind, + traversalState, + result + ); + changed ||= neighborChanged; + } + } + // active neighbors can be filtered out as we're solving for the following + // relation. + // X = Intersect(X_neighbors, X) + // non-active neighbors with no recorded results can occur due to backedges. + // it's not safe to assume they can be filtered out (e.g. not intersected) + const neighborAccesses = Set_intersect([ + ...(Array.from(neighbors) + .filter((n) => traversalState.get(n) !== "active") + .map((n) => result.get(n) ?? new Set()) as Array< + Set + >), + ]); + + const prevSize = result.get(nodeId)?.size; + // const prevPrinted = [...(result.get(nodeId) ?? [])].map( + // printDependencyNode + // ); + result.set(nodeId, Set_union(node.assumedNonNullObjects, neighborAccesses)); + traversalState.set(nodeId, "done"); + + // const newPrinted = [...(result.get(nodeId) ?? [])].map(printDependencyNode); + // llog(" - ", nodeId, prevPrinted, newPrinted); + + changed ||= prevSize !== result.get(nodeId)!.size; + CompilerError.invariant( + prevSize == null || prevSize <= result.get(nodeId)!.size, + { + reason: "[CollectHoistablePropertyLoads] Nodes shrank!", + description: `${nodeId} ${kind} ${prevSize} ${ + result.get(nodeId)!.size + }`, + loc: GeneratedSource, + } + ); + return changed; + } + const fromEntry = new Map>(); + const fromExit = new Map>(); + let changed = true; + const traversalState = new Map(); + const reversedBlocks = [...fn.body.blocks]; + reversedBlocks.reverse(); + let i = 0; + + while (changed) { + i++; + changed = false; + for (const [blockId] of fn.body.blocks) { + const changed_ = recursivelyDeriveNonNull( + blockId, + "pred", + traversalState, + fromEntry + ); + changed ||= changed_; + } + traversalState.clear(); + for (const [blockId] of reversedBlocks) { + const changed_ = recursivelyDeriveNonNull( + blockId, + "succ", + traversalState, + fromExit + ); + changed ||= changed_; + } + traversalState.clear(); + } + + // TODO: I can't come up with a case that requires fixed-point iteration + CompilerError.invariant(i === 2, { + reason: "require fixed-point iteration", + loc: GeneratedSource, + }); + + CompilerError.invariant( + fromEntry.size === fromExit.size && fromEntry.size === nodes.size, + { + reason: + "bad sizes after calculating fromEntry + fromExit " + + `${fromEntry.size} ${fromExit.size} ${nodes.size}`, + loc: GeneratedSource, + } + ); + + for (const [id, node] of nodes) { + node.assumedNonNullObjects = Set_union( + assertNonNull(fromEntry.get(id)), + assertNonNull(fromExit.get(id)) + ); + } +} + +export function assertNonNull, U>( + value: T | null | undefined, + source?: string +): T { + CompilerError.invariant(value != null, { + reason: "Unexpected null", + description: source != null ? `(from ${source})` : null, + loc: GeneratedSource, + }); + return value; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts new file mode 100644 index 0000000000000..b6ede41fac03a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -0,0 +1,578 @@ +import { + IdentifierId, + ScopeId, + HIRFunction, + Place, + Instruction, + ReactiveScopeDependency, + BlockId, + Identifier, + ReactiveScope, + isObjectMethodType, + isRefValueType, + isUseRefType, + makeInstructionId, + InstructionId, + InstructionKind, + GeneratedSource, +} from "./HIR"; +import { + BlockInfo, + collectHoistablePropertyLoads, +} from "./CollectHoistablePropertyLoads"; +import { + NO_OP, + ScopeBlockTraversal, + eachInstructionOperand, + eachInstructionValueOperand, + eachPatternOperand, + eachTerminalOperand, +} from "./visitors"; +import { ReactiveScopeDependencyTreeHIR } from "./DeriveMinimalDependenciesHIR"; +import { Stack, empty } from "../Utils/Stack"; +import { CompilerError } from "../CompilerError"; + +export function llog(..._args: any): void { + console.log(..._args); +} +type TemporariesUsedOutsideDefiningScope = { + /* + * tracks all relevant temporary declarations (currently LoadLocal and PropertyLoad) + * and the scope where they are defined + */ + declarations: Map; + // temporaries used outside of their defining scope + usedOutsideDeclaringScope: Set; +}; + +function findPromotedTemporaries( + fn: HIRFunction, + { + declarations, + usedOutsideDeclaringScope, + }: TemporariesUsedOutsideDefiningScope +): void { + const prunedScopes = new Set(); + const scopeTraversal = new ScopeBlockTraversal( + null, + (scope, pruned) => { + if (pruned) { + prunedScopes.add(scope.id); + } + return null; + }, + NO_OP + ); + + function handlePlace(place: Place): void { + const declaringScope = declarations.get(place.identifier.id); + if ( + declaringScope != null && + scopeTraversal.activeScopes.indexOf(declaringScope) === -1 && + !prunedScopes.has(declaringScope) + ) { + // Declaring scope is not active === used outside declaring scope + usedOutsideDeclaringScope.add(place.identifier.id); + } + } + + function handleInstruction(instr: Instruction): void { + const scope = scopeTraversal.activeScopes.at(-1); + if (scope === undefined || prunedScopes.has(scope)) { + return; + } + switch (instr.value.kind) { + case "LoadLocal": + case "LoadContext": + case "PropertyLoad": { + declarations.set(instr.lvalue.identifier.id, scope); + break; + } + default: { + break; + } + } + } + + for (const [_, block] of fn.body.blocks) { + scopeTraversal.handleBlock(block); + for (const instr of block.instructions) { + for (const place of eachInstructionOperand(instr)) { + handlePlace(place); + } + handleInstruction(instr); + } + + for (const place of eachTerminalOperand(block.terminal)) { + handlePlace(place); + } + } +} + +type Decl = { + id: InstructionId; + scope: Stack; +}; + +class Context { + #temporariesUsedOutsideScope: Set; + #declarations: Map = new Map(); + #reassignments: Map = new Map(); + // Reactive dependencies used in the current reactive scope. + #dependencies: Array = []; + /* + * We keep a sidemap for temporaries created by PropertyLoads, and do + * not store any control flow (i.e. #inConditionalWithinScope) here. + * - a ReactiveScope (A) containing a PropertyLoad may differ from the + * ReactiveScope (B) that uses the produced temporary. + * - codegen will inline these PropertyLoads back into scope (B) + */ + #properties: Map; + #temporaries: Map; + #scopes: Stack = empty(); + deps: Map> = new Map(); + + get properties(): Map { + return this.#properties; + } + constructor( + temporariesUsedOutsideScope: Set, + temporaries: Map, + properties: Map + ) { + this.#temporariesUsedOutsideScope = temporariesUsedOutsideScope; + this.#temporaries = temporaries; + this.#properties = properties; + } + + static enterScope( + scope: ReactiveScope, + _pruned: boolean, + context: Context + ): { previousDeps: Array } { + // Save context of previous scope + const previousDeps = context.#dependencies; + + /* + * Set context for new scope + */ + context.#dependencies = []; + context.#scopes = context.#scopes.push(scope); + return { previousDeps }; + } + + static exitScope( + scope: ReactiveScope, + pruned: boolean, + context: Context, + state: { previousDeps: Array } + ): void { + const scopedDependencies = context.#dependencies; + + // Restore context of previous scope + context.#scopes = context.#scopes.pop(); + context.#dependencies = state.previousDeps; + + /* + * Collect dependencies we recorded for the exiting scope and propagate + * them upward using the same rules as normal dependency collection. + * Child scopes may have dependencies on values created within the outer + * scope, which necessarily cannot be dependencies of the outer scope. + */ + for (const dep of scopedDependencies) { + if (context.#checkValidDependency(dep)) { + context.#dependencies.push(dep); + } + } + + if (!pruned) { + context.deps.set(scope, scopedDependencies); + } + } + + isUsedOutsideDeclaringScope(place: Place): boolean { + return this.#temporariesUsedOutsideScope.has(place.identifier.id); + } + + /* + * Records where a value was declared, and optionally, the scope where the value originated from. + * This is later used to determine if a dependency should be added to a scope; if the current + * scope we are visiting is the same scope where the value originates, it can't be a dependency + * on itself. + */ + declare(identifier: Identifier, decl: Decl): void { + if (!this.#declarations.has(identifier.id)) { + this.#declarations.set(identifier.id, decl); + } + this.#reassignments.set(identifier, decl); + } + + resolveTemporary(place: Place): Identifier { + return this.#temporaries.get(place.identifier) ?? place.identifier; + } + + getProperty(object: Place, property: string): ReactiveScopeDependency { + return this.#getProperty(object, property, false); + } + + #getProperty( + object: Place, + property: string, + _isConditional: boolean + ): ReactiveScopeDependency { + /** + Example 1: + $0 = LoadLocal x + $1 = PropertyLoad $0.y + resolvedObject = x, resolvedDependency = null + + Example 2: + $0 = LoadLocal x + $1 = PropertyLoad $0.y + $2 = PropertyLoad $1.z + resolvedObject = null, resolvedDependency = x.y + + Example 3: + $0 = Call(...) + $1 = PropertyLoad $0.y + resolvedObject = null, resolvedDependency = null + */ + const resolvedObject = this.resolveTemporary(object); + const resolvedDependency = this.#properties.get(resolvedObject); + let objectDependency: ReactiveScopeDependency; + /* + * (1) Create the base property dependency as either a LoadLocal (from a temporary) + * or a deep copy of an existing property dependency. + */ + if (resolvedDependency === undefined) { + objectDependency = { + identifier: resolvedObject, + path: [], + }; + } else { + objectDependency = { + identifier: resolvedDependency.identifier, + path: [...resolvedDependency.path], + }; + } + + objectDependency.path.push(property); + return objectDependency; + } + + // Checks if identifier is a valid dependency in the current scope + #checkValidDependency(maybeDependency: ReactiveScopeDependency): boolean { + // ref.current access is not a valid dep + if ( + isUseRefType(maybeDependency.identifier) && + maybeDependency.path.at(0) === "current" + ) { + return false; + } + + // ref value is not a valid dep + if (isRefValueType(maybeDependency.identifier)) { + return false; + } + + /* + * object methods are not deps because they will be codegen'ed back in to + * the object literal. + */ + if (isObjectMethodType(maybeDependency.identifier)) { + return false; + } + + const identifier = maybeDependency.identifier; + /* + * If this operand is used in a scope, has a dynamic value, and was defined + * before this scope, then its a dependency of the scope. + */ + const currentDeclaration = + this.#reassignments.get(identifier) ?? + this.#declarations.get(identifier.id); + const currentScope = this.currentScope.value; + return ( + currentScope != null && + currentDeclaration !== undefined && + currentDeclaration.id < currentScope.range.start + ); + } + + #isScopeActive(scope: ReactiveScope): boolean { + if (this.#scopes === null) { + return false; + } + return this.#scopes.find((state) => state === scope); + } + + get currentScope(): Stack { + return this.#scopes; + } + + visitOperand(place: Place): void { + const resolved = this.resolveTemporary(place); + /* + * if this operand is a temporary created for a property load, try to resolve it to + * the expanded Place. Fall back to using the operand as-is. + */ + + let dependency: ReactiveScopeDependency | null = null; + if (resolved.name === null) { + const propertyDependency = this.#properties.get(resolved); + if (propertyDependency !== undefined) { + dependency = { ...propertyDependency }; + } + } + // console.log( + // `resolving ${place.identifier.id} -> ${dependency ? printDependency(dependency) : ""}` + // ); + this.visitDependency( + dependency ?? { + identifier: resolved, + path: [], + } + ); + } + + visitProperty(object: Place, property: string): void { + const nextDependency = this.#getProperty(object, property, false); + // if (object.identifier.id === 32) { + // console.log(printDependency(nextDependency)); + // } + this.visitDependency(nextDependency); + } + + visitDependency(maybeDependency: ReactiveScopeDependency): void { + /* + * Any value used after its originally defining scope has concluded must be added as an + * output of its defining scope. Regardless of whether its a const or not, + * some later code needs access to the value. If the current + * scope we are visiting is the same scope where the value originates, it can't be a dependency + * on itself. + */ + + /* + * if originalDeclaration is undefined here, then this is not a local var + * (all decls e.g. `let x;` should be initialized in BuildHIR) + */ + const originalDeclaration = this.#declarations.get( + maybeDependency.identifier.id + ); + if ( + originalDeclaration !== undefined && + originalDeclaration.scope.value !== null + ) { + originalDeclaration.scope.each((scope) => { + if (!this.#isScopeActive(scope)) { + scope.declarations.set(maybeDependency.identifier.id, { + identifier: maybeDependency.identifier, + scope: originalDeclaration.scope.value!, + }); + } + }); + } + + if (this.#checkValidDependency(maybeDependency)) { + this.#dependencies.push(maybeDependency); + } + } + + /* + * Record a variable that is declared in some other scope and that is being reassigned in the + * current one as a {@link ReactiveScope.reassignments} + */ + visitReassignment(place: Place): void { + const currentScope = this.currentScope.value; + if ( + currentScope != null && + !Array.from(currentScope.reassignments).some( + (identifier) => identifier.id === place.identifier.id + ) && + this.#checkValidDependency({ identifier: place.identifier, path: [] }) + ) { + currentScope.reassignments.add(place.identifier); + } + } +} + +function handleInstruction(instr: Instruction, context: Context) { + const { id, value, lvalue } = instr; + if (value.kind === "LoadLocal") { + if ( + value.place.identifier.name === null || + lvalue.identifier.name !== null || + context.isUsedOutsideDeclaringScope(lvalue) + ) { + context.visitOperand(value.place); + } + } else if (value.kind === "PropertyLoad") { + if (context.isUsedOutsideDeclaringScope(lvalue)) { + context.visitProperty(value.object, value.property); + } else { + const nextDependency = context.getProperty(value.object, value.property); + context.properties.set(lvalue.identifier, nextDependency); + } + } else if (value.kind === "StoreLocal") { + context.visitOperand(value.value); + if (value.lvalue.kind === InstructionKind.Reassign) { + context.visitReassignment(value.lvalue.place); + } + context.declare(value.lvalue.place.identifier, { + id, + scope: context.currentScope, + }); + } else if (value.kind === "DeclareLocal" || value.kind === "DeclareContext") { + /* + * Some variables may be declared and never initialized. We need + * to retain (and hoist) these declarations if they are included + * in a reactive scope. One approach is to simply add all `DeclareLocal`s + * as scope declarations. + */ + + /* + * We add context variable declarations here, not at `StoreContext`, since + * context Store / Loads are modeled as reads and mutates to the underlying + * variable reference (instead of through intermediate / inlined temporaries) + */ + context.declare(value.lvalue.place.identifier, { + id, + scope: context.currentScope, + }); + } else if (value.kind === "Destructure") { + context.visitOperand(value.value); + for (const place of eachPatternOperand(value.lvalue.pattern)) { + if (value.lvalue.kind === InstructionKind.Reassign) { + context.visitReassignment(place); + } + context.declare(place.identifier, { + id, + scope: context.currentScope, + }); + } + } else { + for (const operand of eachInstructionValueOperand(value)) { + context.visitOperand(operand); + } + } + + context.declare(lvalue.identifier, { + id, + scope: context.currentScope, + }); +} + +function collectDependencies( + fn: HIRFunction, + usedOutsideDeclaringScope: Set, + temporaries: Map, + properties: Map +) { + const context = new Context( + usedOutsideDeclaringScope, + temporaries, + properties + ); + + for (const param of fn.params) { + if (param.kind === "Identifier") { + context.declare(param.identifier, { + id: makeInstructionId(0), + scope: empty(), + }); + } else { + context.declare(param.place.identifier, { + id: makeInstructionId(0), + scope: empty(), + }); + } + } + + type ScopeTraversalContext = { previousDeps: Array }; + const scopeTraversal = new ScopeBlockTraversal< + Context, + ScopeTraversalContext + >(context, Context.enterScope, Context.exitScope); + + // TODO don't count optional load rvals as dep (e.g. collectOptionalLoadRValues(...)) + for (const [_, block] of fn.body.blocks) { + // Handle scopes that begin or end at this block + scopeTraversal.handleBlock(block); + + for (const instr of block.instructions) { + handleInstruction(instr, context); + } + for (const place of eachTerminalOperand(block.terminal)) { + context.visitOperand(place); + } + } + return context.deps; +} + +/** + * Compute the set of hoistable property reads. + */ +function recordHoistablePropertyReads( + nodes: Map, + scopeId: ScopeId, + tree: ReactiveScopeDependencyTreeHIR +): void { + let nonNullObjects: Array | null = null; + for (const [_blockId, node] of nodes) { + if (node.scope === scopeId) { + nonNullObjects = [...node.assumedNonNullObjects].map((n) => n.fullPath); + break; + } + } + CompilerError.invariant(nonNullObjects != null, { + reason: "[PropagateScopeDependencies] Scope not found in tracked blocks", + loc: GeneratedSource, + }); + + for (const node of nonNullObjects) { + tree.markNodesNonNull({ + ...node, + optionalPath: [], + }); + } +} + +export function propagateScopeDependenciesHIR(fn: HIRFunction): void { + const escapingTemporaries: TemporariesUsedOutsideDefiningScope = { + declarations: new Map(), + usedOutsideDeclaringScope: new Set(), + }; + findPromotedTemporaries(fn, escapingTemporaries); + const { nodes, temporaries, properties } = collectHoistablePropertyLoads( + fn, + escapingTemporaries.usedOutsideDeclaringScope + ); + + const scopeDeps = collectDependencies( + fn, + escapingTemporaries.usedOutsideDeclaringScope, + temporaries, + properties + ); + + /** + * Derive the minimal set of hoistable dependencies for each scope. + */ + for (const [scope, deps] of scopeDeps) { + const tree = new ReactiveScopeDependencyTreeHIR(); + + /** + * Step 1: Add every dependency used by this scope (e.g. `a.b.c`) + */ + for (const dep of deps) { + tree.addDependency({ ...dep, optionalPath: [] }); + } + /** + * Step 2: Mark "hoistable" property reads, i.e. ones that will + * unconditionally run, given the basic block in which the scope + * begins. + */ + recordHoistablePropertyReads(nodes, scope.id, tree); + scope.dependencies = tree.deriveMinimalDependencies(); + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDepsHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDepsHIR.ts deleted file mode 100644 index 5fa38bb4e8d92..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDepsHIR.ts +++ /dev/null @@ -1,1126 +0,0 @@ -import invariant from "invariant"; -import { - BasicBlock, - BlockId, - GeneratedSource, - HIRFunction, - Identifier, - IdentifierId, - Instruction, - InstructionId, - InstructionKind, - Place, - ReactiveScope, - ReactiveScopeDependency, - ScopeId, - isObjectMethodType, - isRefValueType, - isUseRefType, - makeInstructionId, -} from "."; -import { CompilerError } from "../CompilerError"; -import { Set_intersect, Set_union, getOrInsertDefault } from "../Utils/utils"; -import { printIdentifier } from "./PrintHIR"; -import { - eachInstructionOperand, - eachInstructionValueOperand, - eachPatternOperand, - eachTerminalOperand, -} from "./visitors"; -import { Stack, empty } from "../Utils/Stack"; -import { ReactiveScopeDependencyTreeHIR as ReactiveScopeDependencyTree } from "./DeriveMinimalDependenciesHIR"; -import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables"; - -/** - * We currently lower function expression dependencies inline before the - * function expression instruction. This causes our HIR to deviate from - * JS specs. - * - * For example, note that instructions 0-2 in the below HIR are incorrectly - * hoisted. - * ```js - * // Input - * function Component(props) { - * const fn = () => cond && read(props.a.b); - * // ... - * } - * - * // HIR: - * [0] $0 = LoadLocal "props" - * [1] $1 = PropertyLoad $0, "a" - * [2] $2 = PropertyLoad $1, "b" - * [3] $3 = FunctionExpression deps=[$2] context=[$0] { - * ... - * } - * - * TODO: rewrite function expression deps - */ -function collectFunctionExpressionRValues(fn: HIRFunction): Set { - const result = new Set(); - const loads = new Map(); - - for (const [_, block] of fn.body.blocks) { - for (const instr of block.instructions) { - if (instr.value.kind === "LoadLocal") { - loads.set(instr.lvalue.identifier.id, instr.value.place.identifier.id); - } else if (instr.value.kind === "PropertyLoad") { - loads.set(instr.lvalue.identifier.id, instr.value.object.identifier.id); - } else if (instr.value.kind === "FunctionExpression") { - for (const dep of instr.value.loweredFunc.dependencies) { - result.add(dep.identifier.id); - } - } - } - } - - // don't iterate newly added objects as optimization - for (const res of [...result]) { - let curr = loads.get(res); - while (curr != null) { - result.add(curr); - curr = loads.get(curr); - } - } - return result; -} - -type DependencyRoot = { - properties: Map; - parent: null; - root: Identifier; -}; - -export type DependencyNode = - | { - properties: Map; - parent: DependencyNode; - } - | DependencyRoot; - -export function llog(..._args: any): void { - console.log(..._args); -} -class Tree { - roots: Map = new Map(); - - #getOrCreateRoot(identifier: Identifier): DependencyNode { - // roots can always be accessed unconditionally in JS - let rootNode = this.roots.get(identifier); - - if (rootNode === undefined) { - rootNode = { - root: identifier, - properties: new Map(), - parent: null, - }; - this.roots.set(identifier, rootNode); - } - return rootNode; - } - - static getOrMakeProperty( - node: DependencyNode, - property: string - ): DependencyNode { - let child = node.properties.get(property); - if (child == null) { - child = { - properties: new Map(), - parent: node, - }; - node.properties.set(property, child); - } - return child; - } - - add(n: ReactiveScopeDependency): DependencyNode { - let currNode = this.#getOrCreateRoot(n.identifier); - for (const property of n.path) { - currNode = Tree.getOrMakeProperty(currNode, property); - } - return currNode; - } -} - -export type TNode = { - blockId: BlockId; - instrs: Set; - preds: Set; - assumedNonNullObjects: Set; - normalizedNonNullObjects: Set; -}; - -// Assumed non-null loads -export type TResult = { - nodes: Map; - temporaries: Map; - properties: Map; -}; - -class CollectResult { - temporaries: Map = new Map(); - properties: Map = new Map(); - tree: Tree = new Tree(); - - declareTemporary(from: Identifier, to: Identifier): void { - this.temporaries.set(from, to); - } - - declareProperty( - lvalue: Place, - object: Place, - propertyName: string, - shouldDeclare: boolean - ): DependencyNode { - // temporaries contains object if this is a property load chain from a named variable - // Otherwise, there is a non-trivial expression - const resolvedObject = - this.temporaries.get(object.identifier) ?? object.identifier; - - const resolvedDependency = this.properties.get(resolvedObject); - let property: ReactiveScopeDependency; - if (resolvedDependency == null) { - property = { - identifier: resolvedObject, - path: [propertyName], - }; - } else { - property = { - identifier: resolvedDependency.identifier, - path: [...resolvedDependency.path, propertyName], - }; - } - - if (shouldDeclare) { - this.properties.set(lvalue.identifier, property); - } - return this.tree.add(property); - } -} - -export function printDependencyNode(node: DependencyNode): string { - let names: Array = []; - let curr: DependencyNode | null = node; - while (curr != null) { - if (curr.parent == null) { - names.push(printIdentifier(curr.root)); - } else { - let found = false; - for (const [propName, propNode] of curr.parent.properties) { - if (curr === propNode) { - found = true; - names.push(propName); - break; - } - } - CompilerError.invariant(found, { - reason: "Could not find node in parent", - loc: GeneratedSource, - }); - } - curr = curr.parent; - } - names.reverse(); - return names.join("."); -} - -export function printBlockNode(node: TNode): string { - return ( - "[" + - [...node.normalizedNonNullObjects] - .map((n) => { - return printDependencyNode(n); - }) - .join(" ") + - "]" - ); -} - -function collectNodes( - fn: HIRFunction, - functionExprRvals: Set, - usedOutsideDeclaringScope: Set, - c: CollectResult -): Map { - const nodes = new Map(); - for (const [blockId, block] of fn.body.blocks) { - const instrs = new Set(); - const assumedNonNullObjects = new Set(); - for (const instr of block.instructions) { - const { id, value, lvalue } = instr; - instrs.add(id); - const usedOutside = usedOutsideDeclaringScope.has(lvalue.identifier.id); - if (value.kind === "PropertyLoad") { - const propertyNode = c.declareProperty( - lvalue, - value.object, - value.property, - !usedOutside - ); - if ( - !functionExprRvals.has(lvalue.identifier.id) && - !isMutable(instr, value.object) - ) { - let curr = propertyNode.parent; - while (curr != null) { - assumedNonNullObjects.add(curr); - curr = curr.parent; - } - } - } else if (value.kind === "LoadLocal") { - if ( - lvalue.identifier.name == null && - value.place.identifier.name !== null && - !usedOutside - ) { - c.declareTemporary(lvalue.identifier, value.place.identifier); - } - } - } - - instrs.add(block.terminal.id); - - nodes.set(blockId, { - blockId, - instrs, - preds: block.preds, - assumedNonNullObjects, - normalizedNonNullObjects: new Set(), - }); - } - return nodes; -} - -export function assertNonNull, U>( - value: T | null | undefined -): T { - invariant(value != null, "Unexpected null"); - return value; -} - -function deriveNonNull(fn: HIRFunction, nodes: Map): void { - // block -> successors sidemap - const succ = new Map>(); - const terminalPreds = new Set(); - - for (const [blockId, block] of fn.body.blocks) { - for (const pred of block.preds) { - const predVal = getOrInsertDefault(succ, pred, new Set()); - predVal.add(blockId); - } - if (block.terminal.kind === "throw" || block.terminal.kind === "return") { - terminalPreds.add(blockId); - } - } - - function recursivelyDeriveNonNull( - nodeId: BlockId, - kind: "succ" | "pred", - traversalState: Map, - result: Map> - ): boolean { - if (traversalState.has(nodeId)) { - return false; - } - traversalState.set(nodeId, "active"); - - const node = nodes.get(nodeId); - if (node == null) { - CompilerError.invariant(false, { - reason: `Bad node ${nodeId}, kind: ${kind}`, - loc: GeneratedSource, - }); - } - const neighbors = Array.from( - kind === "succ" ? succ.get(nodeId) ?? [] : node.preds - ); - - let changed = false; - for (const pred of neighbors) { - if (!traversalState.has(pred)) { - const neighborChanged = recursivelyDeriveNonNull( - pred, - kind, - traversalState, - result - ); - changed ||= neighborChanged; - } - } - // non-active neighbors with no recorded results can occur due to backedges. - // it's not safe to assume they can be filtered out (e.g. not intersected) - const neighborAccesses = Set_intersect([ - ...(Array.from(neighbors) - .filter((n) => traversalState.get(n) !== "active") - .map((n) => result.get(n) ?? new Set()) as Array>), - ]); - - const prevSize = result.get(nodeId)?.size; - const prevPrinted = [...(result.get(nodeId) ?? [])].map( - printDependencyNode - ); - result.set(nodeId, Set_union(node.assumedNonNullObjects, neighborAccesses)); - traversalState.set(nodeId, "done"); - - const newPrinted = [...(result.get(nodeId) ?? [])].map(printDependencyNode); - // llog(" - ", nodeId, prevPrinted, newPrinted); - - changed ||= prevSize !== result.get(nodeId)!.size; - invariant( - prevSize == null || prevSize <= result.get(nodeId)!.size, - "nodes shrank! " + - `${nodeId} ${kind} ${prevSize} ${ - result.get(nodeId)!.size - } ${prevPrinted}` - ); - return changed; - } - const fromEntry = new Map>(); - const fromExit = new Map>(); - let changed = true; - const traversalState = new Map(); - const reversedBlocks = [...fn.body.blocks]; - reversedBlocks.reverse(); - let i = 0; - - while (changed) { - i++; - changed = false; - for (const [blockId] of fn.body.blocks) { - const changed_ = recursivelyDeriveNonNull( - blockId, - "pred", - traversalState, - fromEntry - ); - changed ||= changed_; - } - traversalState.clear(); - for (const [blockId] of reversedBlocks) { - const changed_ = recursivelyDeriveNonNull( - blockId, - "succ", - traversalState, - fromExit - ); - changed ||= changed_; - } - traversalState.clear(); - } - - // TODO: I can't come up with a case that requires fixed-point iteration - CompilerError.invariant(i === 2, { - reason: "require fixed-point iteration", - loc: GeneratedSource, - }); - - CompilerError.invariant( - fromEntry.size === fromExit.size && fromEntry.size === nodes.size, - { - reason: - "bad sizes after calculating fromEntry + fromExit " + - `${fromEntry.size} ${fromExit.size} ${nodes.size}`, - loc: GeneratedSource, - } - ); - - for (const [id, node] of nodes) { - node.assumedNonNullObjects = Set_union( - assertNonNull(fromEntry.get(id)), - assertNonNull(fromExit.get(id)) - ); - } -} - -function normalizeNonNull(nodes: Map): void { - for (const { - assumedNonNullObjects, - normalizedNonNullObjects, - } of nodes.values()) { - outer: for (const node of assumedNonNullObjects) { - for (const propertyNode of node.properties.values()) { - if (assumedNonNullObjects.has(propertyNode)) { - continue outer; - } - } - normalizedNonNullObjects.add(node); - } - } -} - -type TemporariesUsedOutsideDefiningScope = { - /* - * tracks all relevant temporary declarations (currently LoadLocal and PropertyLoad) - * and the scope where they are defined - */ - declarations: Map; - // temporaries used outside of their defining scope - usedOutsideDeclaringScope: Set; -}; - -function handlePlace( - activeScopes: Array, - prunedScopes: Set, - state: TemporariesUsedOutsideDefiningScope, - place: Place -): void { - const declaringScope = state.declarations.get(place.identifier.id); - if ( - declaringScope != null && - activeScopes.indexOf(declaringScope) === -1 && - !prunedScopes.has(declaringScope) - ) { - // Declaring scope is not active === used outside declaring scope - state.usedOutsideDeclaringScope.add(place.identifier.id); - } -} - -function handleInstruction( - activeScopes: Array, - prunedScopes: Set, - state: TemporariesUsedOutsideDefiningScope, - instr: Instruction -): void { - const scope = activeScopes.at(-1); - if (scope === undefined || prunedScopes.has(scope)) { - return; - } - switch (instr.value.kind) { - case "LoadLocal": - case "LoadContext": - case "PropertyLoad": { - state.declarations.set(instr.lvalue.identifier.id, scope); - break; - } - default: { - break; - } - } -} - -class ScopeBlockTraversal { - #blockMap: Map< - BlockId, - | { - kind: "end"; - scope: ReactiveScope; - pruned: boolean; - state: TState; - } - | { - kind: "begin"; - scope: ReactiveScope; - pruned: boolean; - fallthrough: BlockId; - } - > = new Map(); - #context: TContext; - #enterCallback: ( - scope: ReactiveScope, - pruned: boolean, - context: TContext - ) => TState; - #exitCallback: - | (( - scope: ReactiveScope, - pruned: boolean, - context: TContext, - state: TState - ) => void) - | null; - activeScopes: Array = []; - prunedScopes: Set = new Set(); - - constructor( - context: TContext, - enter: (scope: ReactiveScope, pruned: boolean, context: TContext) => TState, - exit: - | (( - scope: ReactiveScope, - pruned: boolean, - context: TContext, - state: TState - ) => void) - | null - ) { - this.#context = context; - this.#enterCallback = enter; - this.#exitCallback = exit; - } - - handleBlock(block: BasicBlock): void { - const blockInfo = this.#blockMap.get(block.id); - if (blockInfo != null) { - this.#blockMap.delete(block.id); - if (blockInfo.kind === "begin") { - this.activeScopes.push(blockInfo.scope.id); - const state = this.#enterCallback( - blockInfo.scope, - blockInfo.pruned, - this.#context - ); - - this.#blockMap.set(blockInfo.fallthrough, { - kind: "end", - scope: blockInfo.scope, - pruned: blockInfo.pruned, - state, - }); - } else { - const top = this.activeScopes.at(-1); - CompilerError.invariant(blockInfo.scope.id === top, { - reason: "Expected matching scope fallthrough", - loc: block.instructions[0]?.loc ?? block.terminal.id, - }); - this.activeScopes.pop(); - this.#exitCallback?.( - blockInfo.scope, - blockInfo.pruned, - this.#context, - blockInfo.state - ); - } - } - - if ( - block.terminal.kind === "scope" || - block.terminal.kind === "pruned-scope" - ) { - const isPruned = block.terminal.kind === "pruned-scope"; - if (isPruned) { - this.prunedScopes.add(block.terminal.scope.id); - } - this.#blockMap.set(block.terminal.block, { - kind: "begin", - scope: block.terminal.scope, - pruned: isPruned, - fallthrough: block.terminal.fallthrough, - }); - } - } -} - -function findPromotedTemporaries( - fn: HIRFunction, - escapingTemporaries: TemporariesUsedOutsideDefiningScope -): void { - const scopeTraversal = new ScopeBlockTraversal( - undefined, - () => undefined, - null - ); - const activeScopes = scopeTraversal.activeScopes; - const prunedScopes = scopeTraversal.prunedScopes; - - for (const [_, block] of fn.body.blocks) { - // Handle scopes that begin or end at this block - scopeTraversal.handleBlock(block); - for (const instr of block.instructions) { - for (const place of eachInstructionOperand(instr)) { - handlePlace(activeScopes, prunedScopes, escapingTemporaries, place); - } - handleInstruction(activeScopes, prunedScopes, escapingTemporaries, instr); - } - - for (const place of eachTerminalOperand(block.terminal)) { - handlePlace(activeScopes, prunedScopes, escapingTemporaries, place); - } - } -} - -type Decl = { - id: InstructionId; - scope: Stack; -}; - -class Context { - #temporariesUsedOutsideScope: Set; - #declarations: Map = new Map(); - #reassignments: Map = new Map(); - // Reactive dependencies used in the current reactive scope. - #dependencies: Array = []; - /* - * We keep a sidemap for temporaries created by PropertyLoads, and do - * not store any control flow (i.e. #inConditionalWithinScope) here. - * - a ReactiveScope (A) containing a PropertyLoad may differ from the - * ReactiveScope (B) that uses the produced temporary. - * - codegen will inline these PropertyLoads back into scope (B) - */ - #nodes: Map; - #properties: Map; - #temporaries: Map; - #scopes: Stack = empty(); - deps: Map = new Map(); - - get properties(): Map { - return this.#properties; - } - constructor( - temporariesUsedOutsideScope: Set, - nodes: Map, - temporaries: Map, - properties: Map - ) { - this.#temporariesUsedOutsideScope = temporariesUsedOutsideScope; - this.#temporaries = temporaries; - this.#properties = properties; - this.#nodes = nodes; - } - - static enterScope( - scope: ReactiveScope, - pruned: boolean, - context: Context - ): { previousDeps: Array } { - // Save context of previous scope - const previousDeps = context.#dependencies; - - /* - * Set context for new scope - */ - context.#dependencies = []; - context.#scopes = context.#scopes.push(scope); - return { previousDeps }; - } - - static exitScope( - scope: ReactiveScope, - pruned: boolean, - context: Context, - state: { previousDeps: Array } - ): void { - const scopedDependencies = context.#dependencies; - - // Restore context of previous scope - context.#scopes = context.#scopes.pop(); - context.#dependencies = state.previousDeps; - - /* - * propagate dependencies upward using the same rules as normal dependency - * collection. child scopes may have dependencies on values created within - * the outer scope, which necessarily cannot be dependencies of the outer - * scope - */ - const tree = new ReactiveScopeDependencyTree(); - - for (const dep of scopedDependencies) { - if (context.#checkValidDependency(dep)) { - context.#dependencies.push(dep); - } - // TODO remove optionalPath (we're not using this rn right?) - tree.addDependency({ ...dep, optionalPath: [] }); - } - - let safeNonNulls: Set | null = null; - const firstInstrId = scope.range.start; - for (const [_blockId, node] of context.#nodes) { - if (node.instrs.has(firstInstrId)) { - safeNonNulls = node.normalizedNonNullObjects; - break; - } - } - CompilerError.invariant(safeNonNulls != null, { - reason: "Instruction not found", - loc: GeneratedSource, - }); - - for (const node of safeNonNulls) { - let root: Identifier | null = null; - const path: Array = []; - let curr: DependencyNode | null = node; - while (curr != null) { - if (curr.parent === null) { - root = curr.root; - } else { - let found = false; - for (const [propName, propNode] of curr.parent.properties) { - if (curr === propNode) { - found = true; - path.unshift(propName); - } - } - CompilerError.invariant(found, { - reason: "Could not find node in parent", - loc: GeneratedSource, - }); - } - curr = curr.parent; - } - tree.markNodesNonNull({ - identifier: assertNonNull(root), - path, - optionalPath: [], - }); - } - - if (!pruned) { - context.deps.set(scope, tree); - } - } - - isUsedOutsideDeclaringScope(place: Place): boolean { - return this.#temporariesUsedOutsideScope.has(place.identifier.id); - } - - /* - * Records where a value was declared, and optionally, the scope where the value originated from. - * This is later used to determine if a dependency should be added to a scope; if the current - * scope we are visiting is the same scope where the value originates, it can't be a dependency - * on itself. - */ - declare(identifier: Identifier, decl: Decl): void { - if (!this.#declarations.has(identifier.id)) { - this.#declarations.set(identifier.id, decl); - } - this.#reassignments.set(identifier, decl); - } - - resolveTemporary(place: Place): Identifier { - return this.#temporaries.get(place.identifier) ?? place.identifier; - } - - getProperty(object: Place, property: string): ReactiveScopeDependency { - return this.#getProperty(object, property, false); - } - - #getProperty( - object: Place, - property: string, - _isConditional: boolean - ): ReactiveScopeDependency { - /** - Example 1: - $0 = LoadLocal x - $1 = PropertyLoad $0.y - resolvedObject = x, resolvedDependency = null - - Example 2: - $0 = LoadLocal x - $1 = PropertyLoad $0.y - $2 = PropertyLoad $1.z - resolvedObject = null, resolvedDependency = x.y - - Example 3: - $0 = Call(...) - $1 = PropertyLoad $0.y - resolvedObject = null, resolvedDependency = null - */ - const resolvedObject = this.resolveTemporary(object); - const resolvedDependency = this.#properties.get(resolvedObject); - let objectDependency: ReactiveScopeDependency; - /* - * (1) Create the base property dependency as either a LoadLocal (from a temporary) - * or a deep copy of an existing property dependency. - */ - if (resolvedDependency === undefined) { - objectDependency = { - identifier: resolvedObject, - path: [], - }; - } else { - objectDependency = { - identifier: resolvedDependency.identifier, - path: [...resolvedDependency.path], - }; - } - - objectDependency.path.push(property); - return objectDependency; - } - - // Checks if identifier is a valid dependency in the current scope - #checkValidDependency(maybeDependency: ReactiveScopeDependency): boolean { - // ref.current access is not a valid dep - if ( - isUseRefType(maybeDependency.identifier) && - maybeDependency.path.at(0) === "current" - ) { - return false; - } - - // ref value is not a valid dep - if (isRefValueType(maybeDependency.identifier)) { - return false; - } - - /* - * object methods are not deps because they will be codegen'ed back in to - * the object literal. - */ - if (isObjectMethodType(maybeDependency.identifier)) { - return false; - } - - const identifier = maybeDependency.identifier; - /* - * If this operand is used in a scope, has a dynamic value, and was defined - * before this scope, then its a dependency of the scope. - */ - const currentDeclaration = - this.#reassignments.get(identifier) ?? - this.#declarations.get(identifier.id); - const currentScope = this.currentScope.value; - return ( - currentScope != null && - currentDeclaration !== undefined && - currentDeclaration.id < currentScope.range.start - ); - } - - #isScopeActive(scope: ReactiveScope): boolean { - if (this.#scopes === null) { - return false; - } - return this.#scopes.find((state) => state === scope); - } - - get currentScope(): Stack { - return this.#scopes; - } - - visitOperand(place: Place): void { - const resolved = this.resolveTemporary(place); - /* - * if this operand is a temporary created for a property load, try to resolve it to - * the expanded Place. Fall back to using the operand as-is. - */ - - let dependency: ReactiveScopeDependency | null = null; - if (resolved.name === null) { - const propertyDependency = this.#properties.get(resolved); - if (propertyDependency !== undefined) { - dependency = { ...propertyDependency }; - } - } - // console.log( - // `resolving ${place.identifier.id} -> ${dependency ? printDependency(dependency) : ""}` - // ); - this.visitDependency( - dependency ?? { - identifier: resolved, - path: [], - } - ); - } - - visitProperty(object: Place, property: string): void { - const nextDependency = this.#getProperty(object, property, false); - // if (object.identifier.id === 32) { - // console.log(printDependency(nextDependency)); - // } - this.visitDependency(nextDependency); - } - - visitDependency(maybeDependency: ReactiveScopeDependency): void { - /* - * Any value used after its originally defining scope has concluded must be added as an - * output of its defining scope. Regardless of whether its a const or not, - * some later code needs access to the value. If the current - * scope we are visiting is the same scope where the value originates, it can't be a dependency - * on itself. - */ - - /* - * if originalDeclaration is undefined here, then this is a free var - * (all other decls e.g. `let x;` should be initialized in BuildHIR) - */ - const originalDeclaration = this.#declarations.get( - maybeDependency.identifier.id - ); - if ( - originalDeclaration !== undefined && - originalDeclaration.scope.value !== null - ) { - originalDeclaration.scope.each((scope) => { - // console.log( - // `${maybeDependency.identifier.id}: active=${this.#isScopeActive(scope)}` - // ); - if (!this.#isScopeActive(scope)) { - scope.declarations.set(maybeDependency.identifier.id, { - identifier: maybeDependency.identifier, - scope: originalDeclaration.scope.value!, - }); - } - }); - } - - if (this.#checkValidDependency(maybeDependency)) { - /* - * Add info about this dependency to the existing tree - * We do not try to join/reduce dependencies here due to missing info - */ - // console.log("valid dependency", printDependency(maybeDependency)); - this.#dependencies.push(maybeDependency); - } - } - - /* - * Record a variable that is declared in some other scope and that is being reassigned in the - * current one as a {@link ReactiveScope.reassignments} - */ - visitReassignment(place: Place): void { - const currentScope = this.currentScope.value; - if ( - currentScope != null && - !Array.from(currentScope.reassignments).some( - (identifier) => identifier.id === place.identifier.id - ) && - this.#checkValidDependency({ identifier: place.identifier, path: [] }) - ) { - currentScope.reassignments.add(place.identifier); - } - } -} - -export function collectNonNullObjects( - fn: HIRFunction, - usedOutsideDeclaringScope: Set -): TResult { - const c = new CollectResult(); - const functionExprRvals = fn.env.config.enableTreatFunctionDepsAsConditional - ? collectFunctionExpressionRValues(fn) - : new Set(); - const nodes = collectNodes( - fn, - functionExprRvals, - usedOutsideDeclaringScope, - c - ); - const result: TResult = { - nodes, - temporaries: c.temporaries, - properties: c.properties, - }; - - deriveNonNull(fn, nodes); - normalizeNonNull(nodes); - - return result; -} - -function handleInstruction_(instr: Instruction, context: Context) { - const { id, value, lvalue } = instr; - // TODO: here, should we track global loads in temporaries? - if (value.kind === "LoadLocal") { - if ( - value.place.identifier.name === null || - lvalue.identifier.name !== null || - context.isUsedOutsideDeclaringScope(lvalue) - ) { - context.visitOperand(value.place); - } - } else if (value.kind === "PropertyLoad") { - if (context.isUsedOutsideDeclaringScope(lvalue)) { - // console.log("herehere", lvalue.identifier.id); - context.visitProperty(value.object, value.property); - } else { - const nextDependency = context.getProperty(value.object, value.property); - context.properties.set(lvalue.identifier, nextDependency); - } - } else if (value.kind === "StoreLocal") { - context.visitOperand(value.value); - if (value.lvalue.kind === InstructionKind.Reassign) { - context.visitReassignment(value.lvalue.place); - } - context.declare(value.lvalue.place.identifier, { - id, - scope: context.currentScope, - }); - } else if (value.kind === "DeclareLocal" || value.kind === "DeclareContext") { - /* - * Some variables may be declared and never initialized. We need - * to retain (and hoist) these declarations if they are included - * in a reactive scope. One approach is to simply add all `DeclareLocal`s - * as scope declarations. - */ - - /* - * We add context variable declarations here, not at `StoreContext`, since - * context Store / Loads are modeled as reads and mutates to the underlying - * variable reference (instead of through intermediate / inlined temporaries) - */ - context.declare(value.lvalue.place.identifier, { - id, - scope: context.currentScope, - }); - } else if (value.kind === "Destructure") { - context.visitOperand(value.value); - for (const place of eachPatternOperand(value.lvalue.pattern)) { - if (value.lvalue.kind === InstructionKind.Reassign) { - context.visitReassignment(place); - } - context.declare(place.identifier, { - id, - scope: context.currentScope, - }); - } - } else { - for (const operand of eachInstructionValueOperand(value)) { - context.visitOperand(operand); - } - } - - context.declare(lvalue.identifier, { - id, - scope: context.currentScope, - }); -} - -export function propagateScopeDependenciesHIR(fn: HIRFunction): void { - const escapingTemporaries: TemporariesUsedOutsideDefiningScope = { - declarations: new Map(), - usedOutsideDeclaringScope: new Set(), - }; - // visitReactiveFunction(fn, new FindPromotedTemporaries(), escapingTemporaries); - findPromotedTemporaries(fn, escapingTemporaries); - const { nodes, temporaries, properties } = collectNonNullObjects( - fn, - escapingTemporaries.usedOutsideDeclaringScope - ); - - const context = new Context( - escapingTemporaries.usedOutsideDeclaringScope, - nodes, - temporaries, - new Map( - [...properties.entries()].map(([key, val]) => [key, { ...val }]) as Array< - [Identifier, ReactiveScopeDependency] - > - ) - ); - for (const param of fn.params) { - if (param.kind === "Identifier") { - context.declare(param.identifier, { - id: makeInstructionId(0), - scope: empty(), - }); - } else { - context.declare(param.place.identifier, { - id: makeInstructionId(0), - scope: empty(), - }); - } - } - - type ScopeTraversalContext = { previousDeps: Array }; - const scopeTraversal = new ScopeBlockTraversal< - Context, - ScopeTraversalContext - >(context, Context.enterScope, Context.exitScope); - - // TODO don't count optional load rvals as dep (e.g. collectOptionalLoadRValues(...)) - for (const [_, block] of fn.body.blocks) { - // Handle scopes that begin or end at this block - scopeTraversal.handleBlock(block); - - for (const instr of block.instructions) { - handleInstruction_(instr, context); - } - for (const place of eachTerminalOperand(block.terminal)) { - context.visitOperand(place); - } - } - for (const [scope, depTree] of context.deps) { - scope.dependencies = depTree.deriveMinimalDependencies(); - } -} 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 fd1ea1b14b1c3..aef414b2d2b24 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PruneUnusedLabelsHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PruneUnusedLabelsHIR.ts @@ -69,7 +69,7 @@ export function pruneUnusedLabelsHIR(fn: HIRFunction): void { } for (const [_, block] of fn.body.blocks) { - for (const pred of [...block.preds]) { + for (const pred of block.preds) { const rewritten = rewrites.get(pred); if (rewritten != null) { block.preds.delete(pred); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts index beda6e4a2054b..3c4903cbf23e3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts @@ -5,8 +5,10 @@ * LICENSE file in the root directory of this source tree. */ +import { CompilerError } from ".."; import { assertExhaustive } from "../Utils/utils"; import { + BasicBlock, BlockId, Instruction, InstructionValue, @@ -14,7 +16,9 @@ import { Pattern, Place, ReactiveInstruction, + ReactiveScope, ReactiveValue, + ScopeId, SpreadPattern, Terminal, } from "./HIR"; @@ -1147,3 +1151,112 @@ export function* eachTerminalOperand(terminal: Terminal): Iterable { } } } + + +export const NO_OP = () => null; + +/** + * Helper class for traversing scope blocks in HIR-form. + */ +export class ScopeBlockTraversal { + #blockInfos: Map< + BlockId, + | { + kind: "end"; + scope: ReactiveScope; + pruned: boolean; + state: TState; + } + | { + kind: "begin"; + scope: ReactiveScope; + pruned: boolean; + fallthrough: BlockId; + } + > = new Map(); + #context: TContext; + #enterCallback: ( + scope: ReactiveScope, + pruned: boolean, + context: TContext + ) => TState; + #exitCallback: ( + scope: ReactiveScope, + pruned: boolean, + context: TContext, + state: TState + ) => void; + activeScopes: Array = []; + + constructor( + context: TContext, + enter: (scope: ReactiveScope, pruned: boolean, context: TContext) => TState, + exit: ( + scope: ReactiveScope, + pruned: boolean, + context: TContext, + state: TState + ) => void + ) { + this.#context = context; + this.#enterCallback = enter ?? null; + this.#exitCallback = exit ?? null; + } + + // Handle scopes that begin or end at the start of the given block, + // invoking scope enter/exit callbacks as applicable + handleBlock(block: BasicBlock): void { + const blockInfo = this.#blockInfos.get(block.id); + if (blockInfo?.kind === "begin") { + this.#blockInfos.delete(block.id); + this.activeScopes.push(blockInfo.scope.id); + const state = this.#enterCallback( + blockInfo.scope, + blockInfo.pruned, + this.#context + ); + + CompilerError.invariant(!this.#blockInfos.has(blockInfo.fallthrough), { + reason: "Expected unique scope blocks and fallthroughs", + loc: blockInfo.scope.loc, + }); + this.#blockInfos.set(blockInfo.fallthrough, { + kind: "end", + scope: blockInfo.scope, + pruned: blockInfo.pruned, + state, + }); + } else if (blockInfo?.kind === "end") { + this.#blockInfos.delete(block.id); + const top = this.activeScopes.at(-1); + CompilerError.invariant(blockInfo.scope.id === top, { + reason: + "Expected traversed block fallthrough to match top-most active scope", + loc: block.instructions[0]?.loc ?? block.terminal.id, + }); + this.activeScopes.pop(); + this.#exitCallback?.( + blockInfo.scope, + blockInfo.pruned, + this.#context, + blockInfo.state + ); + } + + if ( + block.terminal.kind === "scope" || + block.terminal.kind === "pruned-scope" + ) { + CompilerError.invariant(!this.#blockInfos.has(block.terminal.block), { + reason: "Expected unique scope blocks and fallthroughs", + loc: block.terminal.loc, + }); + this.#blockInfos.set(block.terminal.block, { + kind: "begin", + scope: block.terminal.scope, + pruned: block.terminal.kind === "pruned-scope", + fallthrough: block.terminal.fallthrough, + }); + } + } +} \ No newline at end of file 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/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/ReactiveScopes/DeriveMinimalDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts index 0bff894cea1b5..968317330addd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts @@ -24,6 +24,9 @@ import { assertExhaustive } from "../Utils/utils"; * path: ['b', 'c'], * optionalPath: ['d', 'e', 'f']. * } + * TODO remove optionalPath or rewrite our PropagateScopeDeps logic to understand + * optional load expressions + * */ export type ReactiveScopePropertyDependency = ReactiveScopeDependency & { optionalPath: Array; 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/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 index 02cc230b41149..b8f525d6b98cf 100644 --- 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 @@ -13,6 +13,7 @@ function Foo() { if (CONST_TRUE) { mutate(thing); } + return thing; } catch {} } @@ -26,17 +27,26 @@ export const FIXTURE_ENTRYPOINT = { ## 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 = null; - if (cond) { - thing = makeObject_Primitives(); - } - if (CONST_TRUE) { - mutate(thing); + 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 {} } 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 index cac9802c9b02e..2cc042094463e 100644 --- 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 @@ -9,6 +9,7 @@ function Foo() { if (CONST_TRUE) { mutate(thing); } + return thing; } catch {} } 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 100% 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 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/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 }], +}; 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; } ``` 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 ; +} 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": { 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" diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 584c13c1a7d00..815285fed0a7a 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/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, 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/**", ], 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__) { 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 => { 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-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/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/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..07c50c7a3ecec 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; @@ -33,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 9404b877f532b..66a3723071986 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, @@ -36,6 +35,7 @@ export const { enableNoCloningMemoCache, enableAddPropertiesFastPath, enableFastJSX, + renameElementSymbol, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build. @@ -66,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; 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([ 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. 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]);