From 187dd6a7a234095dfe67d3ee30dabfd5e2ddc531 Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Tue, 6 Aug 2024 11:06:14 -0400 Subject: [PATCH 01/13] Replace NodeJS 21 with 22 in devEngines field (#30598) 21 is EOL and 22 is stable now. Homebrew installs 22 by default as well which is the practical reason for me to update this. --- fixtures/flight/package.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fixtures/flight/package.json b/fixtures/flight/package.json index f07b667dab910..a0505629baacf 100644 --- a/fixtures/flight/package.json +++ b/fixtures/flight/package.json @@ -4,7 +4,7 @@ "version": "0.1.0", "private": true, "devEngines": { - "node": "20.x || 21.x" + "node": "20.x || 22.x" }, "dependencies": { "@babel/core": "^7.16.0", diff --git a/package.json b/package.json index 82a578e1f3fa9..4507b9b8e6c65 100644 --- a/package.json +++ b/package.json @@ -103,7 +103,7 @@ "yargs": "^15.3.1" }, "devEngines": { - "node": "16.x || 18.x || 20.x || 21.x" + "node": "16.x || 18.x || 20.x || 22.x" }, "jest": { "testRegex": "/scripts/jest/dont-run-jest-directly\\.js$" From eb52f3788fe821427863ebe52fb6935ea7ec2474 Mon Sep 17 00:00:00 2001 From: Arthur <82575487+arthur-mountain@users.noreply.github.com> Date: Wed, 7 Aug 2024 02:43:10 +0800 Subject: [PATCH 02/13] [CI] fix the shared_lint cached key (#30609) ## Summary In the shared_lint, for the first Prettier job, use `${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}` as the cache key. For the following jobs, use `${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }}` as the cache key. Some of the jobs do not hit the cache if the hash does not match. ## How did you test this change? I run [act](https://github.com/nektos/act) locally to test it. --- .github/workflows/shared_lint.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/shared_lint.yml b/.github/workflows/shared_lint.yml index 668aa3e6f6468..31834c3fb9688 100644 --- a/.github/workflows/shared_lint.yml +++ b/.github/workflows/shared_lint.yml @@ -43,7 +43,7 @@ jobs: uses: actions/cache@v4 with: path: "**/node_modules" - key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: node ./scripts/tasks/eslint @@ -61,7 +61,7 @@ jobs: uses: actions/cache@v4 with: path: "**/node_modules" - key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: ./scripts/ci/check_license.sh @@ -79,6 +79,6 @@ jobs: uses: actions/cache@v4 with: path: "**/node_modules" - key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: ./scripts/ci/test_print_warnings.sh From ff0d2621f4846414b134800b1639287b66df42cf Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 6 Aug 2024 14:41:24 -0400 Subject: [PATCH 03/13] Fix rust lints ghstack-source-id: baf433a98a70d9105c8088d82e04a3d96783e9e6 Pull Request resolved: https://github.com/facebook/react/pull/30616 --- compiler/crates/react_hir/src/merge_consecutive_blocks.rs | 1 + compiler/crates/react_semantic_analysis/src/scope_view.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/compiler/crates/react_hir/src/merge_consecutive_blocks.rs b/compiler/crates/react_hir/src/merge_consecutive_blocks.rs index d9cd7bded745e..2f14c96a013ac 100644 --- a/compiler/crates/react_hir/src/merge_consecutive_blocks.rs +++ b/compiler/crates/react_hir/src/merge_consecutive_blocks.rs @@ -151,6 +151,7 @@ impl MergedBlocks { #[derive(Debug, Error)] #[error("Expected predecessor {predecessor} to exist")] +#[allow(dead_code)] pub struct ExpectedPredecessorToExist { predecessor: BlockId, } diff --git a/compiler/crates/react_semantic_analysis/src/scope_view.rs b/compiler/crates/react_semantic_analysis/src/scope_view.rs index 1a9c9a7b5e3b6..81f96a665f0e4 100644 --- a/compiler/crates/react_semantic_analysis/src/scope_view.rs +++ b/compiler/crates/react_semantic_analysis/src/scope_view.rs @@ -159,6 +159,7 @@ impl<'m> std::fmt::Debug for ScopeView<'m> { } #[derive(Clone, Copy)] +#[allow(dead_code)] pub struct LabelView<'m> { #[allow(dead_code)] pub(crate) manager: &'m ScopeManager, From e8a2b47eb5d807a2da22915a6d444d3947c9a885 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 6 Aug 2024 14:41:25 -0400 Subject: [PATCH 04/13] [compiler] Remove sleep in manual release script I originally added this prior to the compiler being OSS'd as a "just in case" feature to panic cancel if something went wrong. Now that the compiler is already launched this is unnecessary. ghstack-source-id: dd17dc8a331657ce23c0cbc012ba967cfc3b9542 Pull Request resolved: https://github.com/facebook/react/pull/30613 --- compiler/scripts/release/publish-manual.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/compiler/scripts/release/publish-manual.js b/compiler/scripts/release/publish-manual.js index df43e98836792..7ce26d6e00506 100644 --- a/compiler/scripts/release/publish-manual.js +++ b/compiler/scripts/release/publish-manual.js @@ -11,7 +11,6 @@ const PUBLISHABLE_PACKAGES = [ 'eslint-plugin-react-compiler', 'react-compiler-healthcheck', ]; -const TIME_TO_RECONSIDER = 3_000; function _spawn(command, args, options, cb) { const child = cp.spawn(command, args, options); @@ -33,10 +32,6 @@ function execHelper(command, options, streamStdout = false) { }); } -function sleep(ms) { - return new Promise(resolve => setTimeout(resolve, ms)); -} - async function getDateStringForCommit(commit) { let dateString = await execHelper( `git show -s --no-show-signature --format=%cd --date=format:%Y%m%d ${commit}` @@ -191,15 +186,6 @@ async function main() { ); } - if (debug === false) { - spinner.info( - `šŸšØšŸšØšŸšØ About to publish to npm in ${ - TIME_TO_RECONSIDER / 1000 - } seconds. You still have time to kill this script!` - ); - await sleep(TIME_TO_RECONSIDER); - } - for (const pkgName of pkgNames) { const pkgDir = path.resolve(__dirname, `../../packages/${pkgName}`); console.log(`\n========== ${pkgName} ==========\n`); From c9143b98d0096c909a2cc23290e3044c1390f6e2 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 6 Aug 2024 14:41:26 -0400 Subject: [PATCH 05/13] [compiler] Refactor release script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updates the release script to publish tags as well as take a `--ci` option Test plan: ``` $ yarn npm:publish --debug --frfr yarn run v1.22.22 $ node scripts/release/publish --debug --frfr ā„¹ Preparing to publish (for real) [debug=true] ā„¹ Building packages āœ” Successfully built babel-plugin-react-compiler āœ” Successfully built eslint-plugin-react-compiler āœ” Successfully built react-compiler-healthcheck NPM 2-factor auth code: ****** āœ” Wrote package.json for babel-plugin-react-compiler@0.0.0-experimental-10cf18a-20240806 ========== babel-plugin-react-compiler ========== ā § Publishing babel-plugin-react-compiler@0.0.0-experimental-10cf18a-20240806 to npm + babel-plugin-react-compiler@0.0.0-experimental-10cf18a-20240806 āœ” Successfully published babel-plugin-react-compiler to npm ā„¹ dry-run: npm dist-tag add babel-plugin-react-compiler@0.0.0-experimental-10cf18a-20240806 experimental --otp=****** āœ” Successfully pushed dist-tag experimental for babel-plugin-react-compiler to npm āœ” Wrote package.json for eslint-plugin-react-compiler@0.0.0-experimental-532f76b-20240806 ========== eslint-plugin-react-compiler ========== ā ¹ Publishing eslint-plugin-react-compiler@0.0.0-experimental-532f76b-20240806 to npm + eslint-plugin-react-compiler@0.0.0-experimental-532f76b-20240806 āœ” Successfully published eslint-plugin-react-compiler to npm ā„¹ dry-run: npm dist-tag add eslint-plugin-react-compiler@0.0.0-experimental-532f76b-20240806 experimental --otp=****** āœ” Successfully pushed dist-tag experimental for eslint-plugin-react-compiler to npm āœ” Wrote package.json for react-compiler-healthcheck@0.0.0-experimental-48a8743-20240806 ========== react-compiler-healthcheck ========== ā ™ Publishing react-compiler-healthcheck@0.0.0-experimental-48a8743-20240806 to npm + react-compiler-healthcheck@0.0.0-experimental-48a8743-20240806 āœ” Successfully published react-compiler-healthcheck to npm ā„¹ dry-run: npm dist-tag add react-compiler-healthcheck@0.0.0-experimental-48a8743-20240806 experimental --otp=****** āœ” Successfully pushed dist-tag experimental for react-compiler-healthcheck to npm āœ… All done āœØ Done in 50.64s. ``` ghstack-source-id: 405cc001c2ab2adaad2bfe4f11fdb7fd28d7e2d1 Pull Request resolved: https://github.com/facebook/react/pull/30614 --- compiler/package.json | 6 +- .../release/{publish-manual.js => publish.js} | 178 +++++++++--------- .../scripts/release/shared/build-packages.js | 22 +++ compiler/scripts/release/shared/packages.js | 9 + compiler/scripts/release/shared/utils.js | 41 ++++ compiler/yarn.lock | 23 ++- 6 files changed, 184 insertions(+), 95 deletions(-) rename compiler/scripts/release/{publish-manual.js => publish.js} (60%) create mode 100644 compiler/scripts/release/shared/build-packages.js create mode 100644 compiler/scripts/release/shared/packages.js create mode 100644 compiler/scripts/release/shared/utils.js diff --git a/compiler/package.json b/compiler/package.json index 82207d4333a13..3adb2d279f159 100644 --- a/compiler/package.json +++ b/compiler/package.json @@ -25,9 +25,11 @@ "snap": "yarn workspace babel-plugin-react-compiler run snap", "snap:build": "yarn workspace snap run build", "postinstall": "perl -p -i -e 's/react\\.element/react.transitional.element/' packages/snap/node_modules/fbt/lib/FbtReactUtil.js && perl -p -i -e 's/didWarnAboutUsingAct = false;/didWarnAboutUsingAct = true;/' packages/babel-plugin-react-compiler/node_modules/react-dom/cjs/react-dom-test-utils.development.js", - "npm:publish": "node scripts/release/publish-manual" + "npm:publish": "node scripts/release/publish" + }, + "dependencies": { + "fs-extra": "^4.0.2" }, - "dependencies": {}, "devDependencies": { "@rollup/plugin-commonjs": "^25.0.7", "@rollup/plugin-json": "^6.1.0", diff --git a/compiler/scripts/release/publish-manual.js b/compiler/scripts/release/publish.js similarity index 60% rename from compiler/scripts/release/publish-manual.js rename to compiler/scripts/release/publish.js index 7ce26d6e00506..df1fdefd17031 100644 --- a/compiler/scripts/release/publish-manual.js +++ b/compiler/scripts/release/publish.js @@ -1,49 +1,16 @@ -const cp = require('child_process'); const ora = require('ora'); const path = require('path'); const yargs = require('yargs'); -const util = require('util'); const {hashElement} = require('folder-hash'); const promptForOTP = require('./prompt-for-otp'); - -const PUBLISHABLE_PACKAGES = [ - 'babel-plugin-react-compiler', - 'eslint-plugin-react-compiler', - 'react-compiler-healthcheck', -]; - -function _spawn(command, args, options, cb) { - const child = cp.spawn(command, args, options); - child.on('close', exitCode => { - cb(null, exitCode); - }); - return child; -} -const spawnHelper = util.promisify(_spawn); - -function execHelper(command, options, streamStdout = false) { - return new Promise((resolve, reject) => { - const proc = cp.exec(command, options, (error, stdout) => - error ? reject(error) : resolve(stdout.trim()) - ); - if (streamStdout) { - proc.stdout.pipe(process.stdout); - } - }); -} - -async function getDateStringForCommit(commit) { - let dateString = await execHelper( - `git show -s --no-show-signature --format=%cd --date=format:%Y%m%d ${commit}` - ); - - // On CI environment, this string is wrapped with quotes '...'s - if (dateString.startsWith("'")) { - dateString = dateString.slice(1, 9); - } - - return dateString; -} +const {PUBLISHABLE_PACKAGES} = require('./shared/packages'); +const { + execHelper, + getDateStringForCommit, + spawnHelper, +} = require('./shared/utils'); +const {buildPackages} = require('./shared/build-packages'); +const {readJson, writeJson} = require('fs-extra'); /** * Script for publishing PUBLISHABLE_PACKAGES to npm. By default, this runs in tarball mode, meaning @@ -83,12 +50,21 @@ async function main() { type: 'boolean', default: false, }) + .option('ci', { + description: 'Publish packages via CI', + type: 'boolean', + default: false, + }) + .option('tags', { + description: 'Tags to publish to npm', + type: 'choices', + choices: ['experimental'], + default: ['experimental'], + }) .help('help') .parseSync(); - const {packages, forReal, debug} = argv; - - if (debug === false) { + if (argv.debug === false) { const currBranchName = await execHelper('git rev-parse --abbrev-ref HEAD'); const isPristine = (await execHelper('git status --porcelain')) === ''; if (currBranchName !== 'main' || isPristine === false) { @@ -98,31 +74,19 @@ async function main() { } } - let pkgNames = packages; - if (Array.isArray(packages) === false) { - pkgNames = [packages]; + let pkgNames = argv.packages; + if (Array.isArray(argv.packages) === false) { + pkgNames = [argv.packages]; } const spinner = ora( `Preparing to publish ${ - forReal === true ? '(for real)' : '(dry run)' - } [debug=${debug}]` + argv.forReal === true ? '(for real)' : '(dry run)' + } [debug=${argv.debug}]` ).info(); - spinner.info('Building packages'); - for (const pkgName of pkgNames) { - const command = `yarn workspace ${pkgName} run build`; - spinner.start(`Running: ${command}\n`); - try { - await execHelper(command); - } catch (e) { - spinner.fail(e.toString()); - throw e; - } - spinner.succeed(`Successfully built ${pkgName}`); - } - spinner.stop(); + await buildPackages(pkgNames); - if (forReal === false) { + if (argv.forReal === false) { spinner.info('Dry run: Report tarball contents'); for (const pkgName of pkgNames) { console.log(`\n========== ${pkgName} ==========\n`); @@ -143,8 +107,7 @@ async function main() { ); } - if (forReal === true) { - const otp = await promptForOTP(); + if (argv.forReal === true) { const commit = await execHelper( 'git show -s --no-show-signature --format=%h', { @@ -152,9 +115,14 @@ async function main() { } ); const dateString = await getDateStringForCommit(commit); + const otp = argv.ci === false ? await promptForOTP() : null; for (const pkgName of pkgNames) { const pkgDir = path.resolve(__dirname, `../../packages/${pkgName}`); + const pkgJsonPath = path.resolve( + __dirname, + `../../packages/${pkgName}/package.json` + ); const {hash} = await hashElement(pkgDir, { encoding: 'hex', folders: {exclude: ['node_modules']}, @@ -163,39 +131,36 @@ async function main() { const truncatedHash = hash.slice(0, 7); const newVersion = `0.0.0-experimental-${truncatedHash}-${dateString}`; - spinner.start(`Bumping version: ${pkgName}`); - try { - await execHelper( - `yarn version --new-version ${newVersion} --no-git-tag-version`, - { - cwd: pkgDir, - } - ); - await execHelper( - `git add package.json && git commit -m "Bump version to ${newVersion}"`, - { - cwd: pkgDir, - } - ); - } catch (e) { - spinner.fail(e.toString()); - throw e; - } - spinner.succeed( - `Bumped ${pkgName} to ${newVersion} and added a git commit` + spinner.start(`Writing package.json for ${pkgName}@${newVersion}`); + await writeJson( + pkgJsonPath, + { + ...(await readJson(pkgJsonPath)), + version: newVersion, + }, + {spaces: 2} ); - } + spinner.succeed(`Wrote package.json for ${pkgName}@${newVersion}`); - for (const pkgName of pkgNames) { - const pkgDir = path.resolve(__dirname, `../../packages/${pkgName}`); console.log(`\n========== ${pkgName} ==========\n`); - spinner.start(`Publishing ${pkgName} to npm\n`); + spinner.start(`Publishing ${pkgName}@${newVersion} to npm\n`); - const opts = debug === true ? ['publish', '--dry-run'] : ['publish']; + let opts = []; + if (argv.debug === true) { + opts.push('--dry-run'); + } + if (otp != null) { + opts.push(`--otp=${otp}`); + } try { await spawnHelper( 'npm', - [...opts, '--registry=https://registry.npmjs.org', `--otp=${otp}`], + [ + 'publish', + ...opts, + '--registry=https://registry.npmjs.org', + '--tag=experimental', + ], { cwd: pkgDir, stdio: 'inherit', @@ -207,9 +172,38 @@ async function main() { throw e; } spinner.succeed(`Successfully published ${pkgName} to npm`); + + spinner.start('Pushing tags to npm'); + for (const tag of argv.tags) { + try { + let opts; + if (otp != null) { + opts = [ + 'dist-tag', + 'add', + `${pkgName}@${newVersion}`, + tag, + `--otp=${otp}`, + ]; + } else { + opts = ['dist-tag', 'add', `${pkgName}@${newVersion}`, tag]; + } + if (argv.debug === true) { + spinner.info(`dry-run: npm ${opts.join(' ')}`); + } else { + await spawnHelper('npm', opts); + } + } catch (e) { + spinner.fail(e.toString()); + throw e; + } + spinner.succeed( + `Successfully pushed dist-tag ${tag} for ${pkgName} to npm` + ); + } } - console.log('\n\nāœ… All done, please push version bump commits to GitHub'); + console.log('\n\nāœ… All done'); } } diff --git a/compiler/scripts/release/shared/build-packages.js b/compiler/scripts/release/shared/build-packages.js new file mode 100644 index 0000000000000..9041416b778a8 --- /dev/null +++ b/compiler/scripts/release/shared/build-packages.js @@ -0,0 +1,22 @@ +const ora = require('ora'); +const {execHelper} = require('./utils'); + +async function buildPackages(pkgNames) { + const spinner = ora(`Building packages`).info(); + for (const pkgName of pkgNames) { + const command = `yarn workspace ${pkgName} run build`; + spinner.start(`Running: ${command}\n`); + try { + await execHelper(command); + } catch (e) { + spinner.fail(e.toString()); + throw e; + } + spinner.succeed(`Successfully built ${pkgName}`); + } + spinner.stop(); +} + +module.exports = { + buildPackages, +}; diff --git a/compiler/scripts/release/shared/packages.js b/compiler/scripts/release/shared/packages.js new file mode 100644 index 0000000000000..c5513d1470132 --- /dev/null +++ b/compiler/scripts/release/shared/packages.js @@ -0,0 +1,9 @@ +const PUBLISHABLE_PACKAGES = [ + 'babel-plugin-react-compiler', + 'eslint-plugin-react-compiler', + 'react-compiler-healthcheck', +]; + +module.exports = { + PUBLISHABLE_PACKAGES, +}; diff --git a/compiler/scripts/release/shared/utils.js b/compiler/scripts/release/shared/utils.js new file mode 100644 index 0000000000000..8406e563303ba --- /dev/null +++ b/compiler/scripts/release/shared/utils.js @@ -0,0 +1,41 @@ +const cp = require('child_process'); +const util = require('util'); + +function execHelper(command, options, streamStdout = false) { + return new Promise((resolve, reject) => { + const proc = cp.exec(command, options, (error, stdout) => + error ? reject(error) : resolve(stdout.trim()) + ); + if (streamStdout) { + proc.stdout.pipe(process.stdout); + } + }); +} + +function _spawn(command, args, options, cb) { + const child = cp.spawn(command, args, options); + child.on('close', exitCode => { + cb(null, exitCode); + }); + return child; +} +const spawnHelper = util.promisify(_spawn); + +async function getDateStringForCommit(commit) { + let dateString = await execHelper( + `git show -s --no-show-signature --format=%cd --date=format:%Y%m%d ${commit}` + ); + + // On CI environment, this string is wrapped with quotes '...'s + if (dateString.startsWith("'")) { + dateString = dateString.slice(1, 9); + } + + return dateString; +} + +module.exports = { + execHelper, + spawnHelper, + getDateStringForCommit, +}; diff --git a/compiler/yarn.lock b/compiler/yarn.lock index bcf7a047a0ee6..002e43a408bfb 100644 --- a/compiler/yarn.lock +++ b/compiler/yarn.lock @@ -5434,6 +5434,15 @@ fraction.js@^4.2.0: resolved "https://registry.yarnpkg.com/fraction.js/-/fraction.js-4.2.0.tgz#448e5109a313a3527f5a3ab2119ec4cf0e0e2950" integrity sha512-MhLuK+2gUcnZe8ZHlaaINnQLl0xRIGRfcGk2yl8xoQAfHrSsL3rYu6FCmBdkdbhc9EPlwyGHewaRsvwRMJtAlA== +fs-extra@^4.0.2: + version "4.0.3" + resolved "https://registry.yarnpkg.com/fs-extra/-/fs-extra-4.0.3.tgz#0d852122e5bc5beb453fb028e9c0c9bf36340c94" + integrity sha512-q6rbdDd1o2mAnQreO7YADIxf/Whx4AHBiRf6d+/cVT8h44ss+lHgxf1FemcqDnQt9X3ct4McHr+JMGlYSsK7Cg== + dependencies: + graceful-fs "^4.1.2" + jsonfile "^4.0.0" + universalify "^0.1.0" + fs.realpath@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/fs.realpath/-/fs.realpath-1.0.0.tgz#1504ad2523158caa40db4a2787cb01411994ea4f" @@ -5686,7 +5695,7 @@ gopd@^1.0.1: dependencies: get-intrinsic "^1.1.3" -graceful-fs@^4.1.2, graceful-fs@^4.2.4: +graceful-fs@^4.1.2, graceful-fs@^4.1.6, graceful-fs@^4.2.4: version "4.2.11" resolved "https://registry.yarnpkg.com/graceful-fs/-/graceful-fs-4.2.11.tgz#4183e4e8bf08bb6e05bbb2f7d2e0c8f712ca40e3" integrity sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ== @@ -7591,6 +7600,13 @@ json5@^2.1.2, json5@^2.2.3: resolved "https://registry.yarnpkg.com/json5/-/json5-2.2.3.tgz#78cd6f1a19bdc12b73db5ad0c61efd66c1e29283" integrity sha512-XmOWe7eyHYH14cLdVPoyg+GOH3rYX++KpzrylJwSW98t3Nk+U8XOl8FWKOgwtzdb8lXGf6zYwDUzeHMWfxasyg== +jsonfile@^4.0.0: + version "4.0.0" + resolved "https://registry.yarnpkg.com/jsonfile/-/jsonfile-4.0.0.tgz#8771aae0799b64076b76640fca058f9c10e33ecb" + integrity sha512-m6F1R3z8jjlf2imQHS2Qez5sjKWQzbuuhuJ/FKYFRZvPE3PuHcSMVZzfsLhGVOkfd20obL5SWEBew5ShlquNxg== + optionalDependencies: + graceful-fs "^4.1.6" + "jsx-ast-utils@^2.4.1 || ^3.0.0", jsx-ast-utils@^3.3.3: version "3.3.3" resolved "https://registry.yarnpkg.com/jsx-ast-utils/-/jsx-ast-utils-3.3.3.tgz#76b3e6e6cece5c69d49a5792c3d01bd1a0cdc7ea" @@ -9718,6 +9734,11 @@ unicode-property-aliases-ecmascript@^2.0.0: resolved "https://registry.yarnpkg.com/unicode-property-aliases-ecmascript/-/unicode-property-aliases-ecmascript-2.1.0.tgz#43d41e3be698bd493ef911077c9b131f827e8ccd" integrity sha512-6t3foTQI9qne+OZoVQB/8x8rk2k1eVy1gRXhV3oFQ5T6R1dqQ1xtin3XqSlx3+ATBkliTaR/hHyJBm+LVPNM8w== +universalify@^0.1.0: + version "0.1.2" + resolved "https://registry.yarnpkg.com/universalify/-/universalify-0.1.2.tgz#b646f69be3942dabcecc9d6639c80dc105efaa66" + integrity sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg== + universalify@^0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/universalify/-/universalify-0.2.0.tgz#6451760566fa857534745ab1dde952d1b1761be0" From 47d85282bcd95f215ad4a496ae1f951904a9ffe4 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 6 Aug 2024 14:41:28 -0400 Subject: [PATCH 06/13] [ci] Add prerelease nightly and manual script for compiler Publishes the compiler packages on the same schedule as the React ones. For now the manual script can only build from `main` but in the future we can add support for building specific commits ghstack-source-id: 66676c578b795b90bf3c5715be8900438868b6ee Pull Request resolved: https://github.com/facebook/react/pull/30615 --- .github/workflows/compiler_prereleases.yml | 51 +++++++++++++++++++ .../workflows/compiler_prereleases_manual.yml | 21 ++++++++ .../compiler_prereleases_nightly.yml | 20 ++++++++ compiler/scripts/release/ci-npmrc | 1 + 4 files changed, 93 insertions(+) create mode 100644 .github/workflows/compiler_prereleases.yml create mode 100644 .github/workflows/compiler_prereleases_manual.yml create mode 100644 .github/workflows/compiler_prereleases_nightly.yml create mode 100644 compiler/scripts/release/ci-npmrc diff --git a/.github/workflows/compiler_prereleases.yml b/.github/workflows/compiler_prereleases.yml new file mode 100644 index 0000000000000..a0cdea4112eae --- /dev/null +++ b/.github/workflows/compiler_prereleases.yml @@ -0,0 +1,51 @@ +name: (Compiler) Publish Prereleases + +on: + workflow_call: + inputs: + commit_sha: + required: true + default: '' + type: string + release_channel: + required: true + type: string + dist_tag: + required: true + type: string + secrets: + NPM_TOKEN: + required: true + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + # https://github.com/actions/cache/blob/main/tips-and-workarounds.md#cache-segment-restore-timeout + SEGMENT_DOWNLOAD_TIMEOUT_MINS: 1 + GH_TOKEN: ${{ github.token }} + NPM_TOKEN: ${{ secrets.NPM_TOKEN }} + +defaults: + run: + working-directory: compiler + +jobs: + publish_prerelease: + name: Publish prelease (${{ inputs.release_channel }}) ${{ inputs.commit_sha }} @${{ inputs.dist_tag }} + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version-file: '.nvmrc' + cache: yarn + cache-dependency-path: compiler/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('compiler/**/yarn.lock') }} + - run: yarn install --frozen-lockfile + - run: | + cp ./scripts/release/ci-npmrc ~/.npmrc + scripts/release/publish.js --ci --tags ${{ inputs.dist_tag }} diff --git a/.github/workflows/compiler_prereleases_manual.yml b/.github/workflows/compiler_prereleases_manual.yml new file mode 100644 index 0000000000000..87de411ddcb6e --- /dev/null +++ b/.github/workflows/compiler_prereleases_manual.yml @@ -0,0 +1,21 @@ +name: (Compiler) Publish Prereleases Manual + +on: + workflow_dispatch: + inputs: + prerelease_commit_sha: + required: false + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + +jobs: + publish_prerelease_experimental: + name: Publish to Experimental channel + uses: facebook/react/.github/workflows/compiler_prereleases.yml@main + with: + commit_sha: ${{ inputs.prerelease_commit_sha || github.sha }} + release_channel: experimental + dist_tag: experimental + secrets: + NPM_TOKEN: ${{ secrets.NPM_TOKEN }} diff --git a/.github/workflows/compiler_prereleases_nightly.yml b/.github/workflows/compiler_prereleases_nightly.yml new file mode 100644 index 0000000000000..90aafac4f631f --- /dev/null +++ b/.github/workflows/compiler_prereleases_nightly.yml @@ -0,0 +1,20 @@ +name: (Compiler) Publish Prereleases Nightly + +on: + schedule: + # At 10 minutes past 16:00 on Mon, Tue, Wed, Thu, and Fri + - cron: 10 16 * * 1,2,3,4,5 + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + +jobs: + publish_prerelease_experimental: + name: Publish to Experimental channel + uses: facebook/react/.github/workflows/compiler_prereleases.yml@main + with: + commit_sha: ${{ github.sha }} + release_channel: experimental + dist_tag: experimental + secrets: + NPM_TOKEN: ${{ secrets.NPM_TOKEN }} diff --git a/compiler/scripts/release/ci-npmrc b/compiler/scripts/release/ci-npmrc new file mode 100644 index 0000000000000..ae643592e765c --- /dev/null +++ b/compiler/scripts/release/ci-npmrc @@ -0,0 +1 @@ +//registry.npmjs.org/:_authToken=${NPM_TOKEN} From 02217c9e1245503d2ba3f7c14244bc11dfd7a97b Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 6 Aug 2024 14:53:00 -0400 Subject: [PATCH 07/13] [ci] Make compiler publish script runnable ghstack-source-id: 309bda2bc31bf21d70d3ad1b6ea0e7834e634a16 Pull Request resolved: https://github.com/facebook/react/pull/30617 --- compiler/scripts/release/publish.js | 4 ++++ 1 file changed, 4 insertions(+) mode change 100644 => 100755 compiler/scripts/release/publish.js diff --git a/compiler/scripts/release/publish.js b/compiler/scripts/release/publish.js old mode 100644 new mode 100755 index df1fdefd17031..4d33474e0521e --- a/compiler/scripts/release/publish.js +++ b/compiler/scripts/release/publish.js @@ -1,3 +1,7 @@ +#!/usr/bin/env node + +'use strict'; + const ora = require('ora'); const path = require('path'); const yargs = require('yargs'); From 22360089b5e40ccfa5df26f1ec491b5c46e7ad61 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 6 Aug 2024 11:24:50 -0700 Subject: [PATCH 08/13] [compiler] Add Identifier.declarationId Adds `Identifier.declarationId` and the new `DeclarationId` (simulated) opaque type. DeclarationId allows uniquely identifying a variable in the original source, ie regardless of reassignments. This allows us to stay in SSA form throughout compilation (see next diff) while still being able to distinguish SSA versions (via IdentifierId) and non-SSA versions (DeclarationId). ghstack-source-id: f2547a58aa7b30cea29fcfe23d5cb45583858a4e Pull Request resolved: https://github.com/facebook/react/pull/30569 --- .../src/HIR/HIR.ts | 32 ++++++++++++-- .../src/HIR/HIRBuilder.ts | 2 + ...neImmediatelyInvokedFunctionExpressions.ts | 44 +++---------------- .../src/SSA/EnterSSA.ts | 1 + 4 files changed, 39 insertions(+), 40 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index fa7b462263368..1dc228f4f397c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -1174,11 +1174,19 @@ export type NonLocalBinding = // Represents a user-defined variable (has a name) or a temporary variable (no name). export type Identifier = { - /* - * unique value to distinguish a variable, since name is not guaranteed to - * exist or be unique + /** + * After EnterSSA, `id` uniquely identifies an SSA instance of a variable. + * Before EnterSSA, `id` matches `declarationId`. */ id: IdentifierId; + + /** + * Uniquely identifies a given variable in the original program. If a value is + * reassigned in the original program each reassigned value will have a distinct + * `id` (after EnterSSA), but they will still have the same `declarationId`. + */ + declarationId: DeclarationId; + // null for temporaries. name is primarily used for debugging. name: IdentifierName | null; // The range for which this variable is mutable @@ -1212,6 +1220,7 @@ export function makeTemporaryIdentifier( return { id, name: null, + declarationId: makeDeclarationId(id), mutableRange: {start: makeInstructionId(0), end: makeInstructionId(0)}, scope: null, type: makeType(), @@ -1508,6 +1517,23 @@ export function makeIdentifierId(id: number): IdentifierId { return id as IdentifierId; } +/* + * Simulated opaque type for IdentifierId to prevent using normal numbers as ids + * accidentally. + */ +const opageDeclarationId = Symbol(); +export type DeclarationId = number & {[opageDeclarationId]: 'DeclarationId'}; + +export function makeDeclarationId(id: number): DeclarationId { + CompilerError.invariant(id >= 0 && Number.isInteger(id), { + reason: 'Expected declaration id to be a non-negative integer', + description: null, + loc: null, + suggestions: null, + }); + return id as DeclarationId; +} + /* * Simulated opaque type for InstructionId to prevent using normal numbers as ids * accidentally. diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts index 6badff5b29798..a0247e093bddd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts @@ -25,6 +25,7 @@ import { Terminal, VariableBinding, makeBlockId, + makeDeclarationId, makeIdentifierName, makeInstructionId, makeTemporaryIdentifier, @@ -320,6 +321,7 @@ export default class HIRBuilder { const id = this.nextIdentifierId; const identifier: Identifier = { id, + declarationId: makeDeclarationId(id), name: makeIdentifierName(name), mutableRange: { start: makeInstructionId(0), diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts index a5f81a433d4b6..c6c6f2f54fbef 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts @@ -8,7 +8,6 @@ import { BasicBlock, BlockId, - Effect, Environment, FunctionExpression, GeneratedSource, @@ -19,11 +18,14 @@ import { LabelTerminal, Place, makeInstructionId, - makeType, promoteTemporary, reversePostorderBlocks, } from '../HIR'; -import {markInstructionIds, markPredecessors} from '../HIR/HIRBuilder'; +import { + createTemporaryPlace, + markInstructionIds, + markPredecessors, +} from '../HIR/HIRBuilder'; import {eachInstructionValueOperand} from '../HIR/visitors'; import {retainWhere} from '../Utils/utils'; @@ -225,23 +227,7 @@ function rewriteBlock( block.instructions.push({ id: makeInstructionId(0), loc: terminal.loc, - lvalue: { - effect: Effect.Unknown, - identifier: { - id: env.nextIdentifierId, - mutableRange: { - start: makeInstructionId(0), - end: makeInstructionId(0), - }, - name: null, - scope: null, - type: makeType(), - loc: terminal.loc, - }, - kind: 'Identifier', - reactive: false, - loc: terminal.loc, - }, + lvalue: createTemporaryPlace(env, terminal.loc), value: { kind: 'StoreLocal', lvalue: {kind: InstructionKind.Reassign, place: {...returnValue}}, @@ -267,23 +253,7 @@ function declareTemporary( block.instructions.push({ id: makeInstructionId(0), loc: GeneratedSource, - lvalue: { - effect: Effect.Unknown, - identifier: { - id: env.nextIdentifierId, - mutableRange: { - start: makeInstructionId(0), - end: makeInstructionId(0), - }, - name: null, - scope: null, - type: makeType(), - loc: result.loc, - }, - kind: 'Identifier', - reactive: false, - loc: GeneratedSource, - }, + lvalue: createTemporaryPlace(env, result.loc), value: { kind: 'DeclareLocal', lvalue: { diff --git a/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts b/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts index 7c1073622ed17..7d665514f4f6e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts @@ -79,6 +79,7 @@ class SSABuilder { makeId(oldId: Identifier): Identifier { return { id: this.nextSsaId, + declarationId: oldId.declarationId, name: oldId.name, mutableRange: { start: makeInstructionId(0), From 3d61b9b4cd4135084d1e8e3b05813b915c38764d Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 6 Aug 2024 11:24:51 -0700 Subject: [PATCH 09/13] [compiler] Stay in SSA form through entire pipeline This PR updates to use SSA form through the entire compilation pipeline. This means that in both HIR form and ReactiveFunction form, `Identifier` instances map 1:1 to `IdentifierId` values. If two identifiers have the same IdentifierId, they are the same instance. What this means is that all our passes can use this more precise information to determine if two particular identifiers are not just the same variable, but the same SSA "version" of that variable. However, some parts of our analysis really care about program variables as opposed to SSA versions, and were relying on LeaveSSA to reset identifiers such that all Identifier instances for a particular program variable would have the same IdentifierId (though not necessarily the same Identifier instance). With LeaveSSA removed, those analysis passes can now use DeclarationId instead to uniquely identify a program variable. Note that this PR surfaces some opportunties to improve edge-cases around reassigned values being declared/reassigned/depended-upon across multiple scopes. Several passes could/should use IdentifierId to more precisely identify exactly which values are accessed - for example, a scope that reassigns `x` but doesn't use `x` prior to reassignment doesn't have to take a dependency on `x`. But today we take a dependnecy. My approach for these cases was to add a "TODO LeaveSSA" comment with notes and the name of the fixture demonstrating the difference, but to intentionally preserve the existing behavior (generally, switching to use DeclarationId when IdentifierId would have been more precise). Beyond updating passes to use DeclarationId instead of Identifier/IdentifierId, the other change here is to extract out the remaining necessary bits of LeaveSSA into a new pass that rewrites InstructionKind (const/let/reassign/etc) based on whether a value is actually const or has reassignments and should be let. ghstack-source-id: 69afdaee5fadf3fdc98ce97549da805f288218b4 Pull Request resolved: https://github.com/facebook/react/pull/30573 --- .../src/Entrypoint/Pipeline.ts | 22 +- .../src/HIR/HIR.ts | 10 +- .../src/HIR/HIRBuilder.ts | 13 + .../src/Inference/AnalyseFunctions.ts | 4 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 27 +- ...tractScopeDeclarationsFromDestructuring.ts | 22 +- .../InferReactiveScopeVariables.ts | 27 +- .../MemoizeFbtAndMacroOperandsInSameScope.ts | 33 +- ...rgeReactiveScopesThatInvalidateTogether.ts | 53 +- .../ReactiveScopes/PromoteUsedTemporaries.ts | 93 +++- .../PropagateScopeDependencies.ts | 88 ++- .../ReactiveScopes/PruneHoistedContexts.ts | 8 +- .../ReactiveScopes/PruneNonEscapingScopes.ts | 71 ++- .../ReactiveScopes/PruneTemporaryLValues.ts | 20 +- .../src/ReactiveScopes/RenameVariables.ts | 14 +- .../src/ReactiveScopes/index.ts | 2 +- .../src/SSA/LeaveSSA.ts | 515 ------------------ ...riteInstructionKindsBasedOnReassignment.ts | 174 ++++++ .../src/SSA/index.ts | 2 +- .../src/TypeInference/PropagatePhiTypes.ts | 108 ++++ .../src/Utils/utils.ts | 12 + .../ValidatePreservedManualMemoization.ts | 22 +- ...-mutable-range-destructured-prop.expect.md | 90 +++ ...invalid-mutable-range-destructured-prop.js | 23 + ...ro-propagate-type-of-ternary-jsx.expect.md | 56 ++ .../repro-propagate-type-of-ternary-jsx.js | 14 + ...propagate-type-of-ternary-nested.expect.md | 72 +++ .../repro-propagate-type-of-ternary-nested.js | 23 + compiler/scripts/anonymize.js | 250 +++++++++ 29 files changed, 1194 insertions(+), 674 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/SSA/LeaveSSA.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/SSA/RewriteInstructionKindsBasedOnReassignment.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/TypeInference/PropagatePhiTypes.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-repro-invalid-mutable-range-destructured-prop.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-repro-invalid-mutable-range-destructured-prop.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-jsx.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-jsx.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.js create mode 100644 compiler/scripts/anonymize.js 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 acee95955502f..cb475c6be8563 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -77,7 +77,11 @@ import {flattenScopesWithHooksOrUseHIR} from '../ReactiveScopes/FlattenScopesWit import {pruneAlwaysInvalidatingScopes} from '../ReactiveScopes/PruneAlwaysInvalidatingScopes'; import pruneInitializationDependencies from '../ReactiveScopes/PruneInitializationDependencies'; import {stabilizeBlockIds} from '../ReactiveScopes/StabilizeBlockIds'; -import {eliminateRedundantPhi, enterSSA, leaveSSA} from '../SSA'; +import { + eliminateRedundantPhi, + enterSSA, + rewriteInstructionKindsBasedOnReassignment, +} from '../SSA'; import {inferTypes} from '../TypeInference'; import { logCodegenFunction, @@ -98,6 +102,7 @@ import { } from '../Validation'; import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLocalsNotReassignedAfterRender'; import {outlineFunctions} from '../Optimization/OutlineFunctions'; +import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -237,8 +242,19 @@ function* runWithEnvironment( inferReactivePlaces(hir); yield log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); - leaveSSA(hir); - yield log({kind: 'hir', name: 'LeaveSSA', value: hir}); + rewriteInstructionKindsBasedOnReassignment(hir); + yield log({ + kind: 'hir', + name: 'RewriteInstructionKindsBasedOnReassignment', + value: hir, + }); + + propagatePhiTypes(hir); + yield log({ + kind: 'hir', + name: 'PropagatePhiTypes', + value: hir, + }); inferReactiveScopeVariables(hir); yield log({kind: 'hir', name: 'InferReactiveScopeVariables', value: hir}); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 1dc228f4f397c..f249329e0c9f9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -1248,6 +1248,9 @@ export function makeIdentifierName(name: string): ValidatedIdentifier { /** * Given an unnamed identifier, promote it to a named identifier. + * + * Note: this uses the identifier's DeclarationId to ensure that all + * instances of the same declaration will have the same name. */ export function promoteTemporary(identifier: Identifier): void { CompilerError.invariant(identifier.name === null, { @@ -1258,7 +1261,7 @@ export function promoteTemporary(identifier: Identifier): void { }); identifier.name = { kind: 'promoted', - value: `#t${identifier.id}`, + value: `#t${identifier.declarationId}`, }; } @@ -1269,6 +1272,9 @@ export function isPromotedTemporary(name: string): boolean { /** * Given an unnamed identifier, promote it to a named identifier, distinguishing * it as a value that needs to be capitalized since it appears in JSX element tag position + * + * Note: this uses the identifier's DeclarationId to ensure that all + * instances of the same declaration will have the same name. */ export function promoteTemporaryJsxTag(identifier: Identifier): void { CompilerError.invariant(identifier.name === null, { @@ -1279,7 +1285,7 @@ export function promoteTemporaryJsxTag(identifier: Identifier): void { }); identifier.name = { kind: 'promoted', - value: `#T${identifier.id}`, + value: `#T${identifier.declarationId}`, }; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts index a0247e093bddd..c694cf310fb39 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts @@ -899,3 +899,16 @@ export function createTemporaryPlace( loc: GeneratedSource, }; } + +/** + * Clones an existing Place, returning a new temporary Place that shares the + * same metadata properties as the original place (effect, reactive flag, type) + * but has a new, temporary Identifier. + */ +export function clonePlaceToTemporary(env: Environment, place: Place): Place { + const temp = createTemporaryPlace(env, place.loc); + temp.effect = place.effect; + temp.identifier.type = place.identifier.type; + temp.reactive = place.reactive; + return temp; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts index 47263ebda2474..b18e19606ce0d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -20,7 +20,7 @@ import { } from '../HIR'; import {deadCodeElimination} from '../Optimization'; import {inferReactiveScopeVariables} from '../ReactiveScopes'; -import {leaveSSA} from '../SSA'; +import {rewriteInstructionKindsBasedOnReassignment} from '../SSA'; import {logHIRFunction} from '../Utils/logger'; import {inferMutableContextVariables} from './InferMutableContextVariables'; import {inferMutableRanges} from './InferMutableRanges'; @@ -108,7 +108,7 @@ function lower(func: HIRFunction): void { inferReferenceEffects(func, {isFunctionExpression: true}); deadCodeElimination(func); inferMutableRanges(func); - leaveSSA(func); + rewriteInstructionKindsBasedOnReassignment(func); inferReactiveScopeVariables(func); inferMutableContextVariables(func); logHIRFunction('AnalyseFunction (inner)', func); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index 980e055ec6624..6fd94dac09bce 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -18,6 +18,7 @@ import {Environment, EnvironmentConfig, ExternalFunction} from '../HIR'; import { ArrayPattern, BlockId, + DeclarationId, GeneratedSource, Identifier, IdentifierId, @@ -309,9 +310,9 @@ function codegenReactiveFunction( ): Result { for (const param of fn.params) { if (param.kind === 'Identifier') { - cx.temp.set(param.identifier.id, null); + cx.temp.set(param.identifier.declarationId, null); } else { - cx.temp.set(param.place.identifier.id, null); + cx.temp.set(param.place.identifier.declarationId, null); } } @@ -392,7 +393,11 @@ class Context { env: Environment; fnName: string; #nextCacheIndex: number = 0; - #declarations: Set = new Set(); + /** + * Tracks which named variables have been declared to dedupe declarations, + * so this uses DeclarationId instead of IdentifierId + */ + #declarations: Set = new Set(); temp: Temporaries; errors: CompilerError = new CompilerError(); objectMethods: Map = new Map(); @@ -418,11 +423,11 @@ class Context { } declare(identifier: Identifier): void { - this.#declarations.add(identifier.id); + this.#declarations.add(identifier.declarationId); } hasDeclared(identifier: Identifier): boolean { - return this.#declarations.has(identifier.id); + return this.#declarations.has(identifier.declarationId); } synthesizeName(name: string): ValidIdentifierName { @@ -1147,7 +1152,7 @@ function codegenTerminal( let catchParam = null; if (terminal.handlerBinding !== null) { catchParam = convertIdentifier(terminal.handlerBinding.identifier); - cx.temp.set(terminal.handlerBinding.identifier.id, null); + cx.temp.set(terminal.handlerBinding.identifier.declarationId, null); } return t.tryStatement( codegenBlock(cx, terminal.block), @@ -1205,7 +1210,7 @@ function codegenInstructionNullable( kind !== InstructionKind.Reassign && place.identifier.name === null ) { - cx.temp.set(place.identifier.id, null); + cx.temp.set(place.identifier.declarationId, null); } const isDeclared = cx.hasDeclared(place.identifier); hasReasign ||= isDeclared; @@ -1261,7 +1266,7 @@ function codegenInstructionNullable( ); if (instr.lvalue !== null) { if (instr.value.kind !== 'StoreContext') { - cx.temp.set(instr.lvalue.identifier.id, expr); + cx.temp.set(instr.lvalue.identifier.declarationId, expr); return null; } else { // Handle chained reassignments for context variables @@ -1530,7 +1535,7 @@ function createCallExpression( } } -type Temporaries = Map; +type Temporaries = Map; function codegenLabel(id: BlockId): string { return `bb${id}`; @@ -1549,7 +1554,7 @@ function codegenInstruction( } if (instr.lvalue.identifier.name === null) { // temporary - cx.temp.set(instr.lvalue.identifier.id, value); + cx.temp.set(instr.lvalue.identifier.declarationId, value); return t.emptyStatement(); } else { const expressionValue = convertValueToExpression(value); @@ -2498,7 +2503,7 @@ function codegenPlaceToExpression(cx: Context, place: Place): t.Expression { } function codegenPlace(cx: Context, place: Place): t.Expression | t.JSXText { - let tmp = cx.temp.get(place.identifier.id); + let tmp = cx.temp.get(place.identifier.declarationId); if (tmp != null) { return tmp; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts index 0fe6a322fe1b9..eb2caa424e417 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts @@ -6,6 +6,7 @@ */ import { + DeclarationId, Destructure, Environment, IdentifierId, @@ -17,6 +18,7 @@ import { ReactiveStatement, promoteTemporary, } from '../HIR'; +import {clonePlaceToTemporary} from '../HIR/HIRBuilder'; import {eachPatternOperand, mapPatternOperands} from '../HIR/visitors'; import { ReactiveFunctionTransform, @@ -82,7 +84,11 @@ export function extractScopeDeclarationsFromDestructuring( class State { env: Environment; - declared: Set = new Set(); + /** + * We need to track which program variables are already declared to convert + * declarations into reassignments, so we use DeclarationId + */ + declared: Set = new Set(); constructor(env: Environment) { this.env = env; @@ -92,7 +98,7 @@ class State { class Visitor extends ReactiveFunctionTransform { override visitScope(scope: ReactiveScopeBlock, state: State): void { for (const [, declaration] of scope.scope.declarations) { - state.declared.add(declaration.identifier.id); + state.declared.add(declaration.identifier.declarationId); } this.traverseScope(scope, state); } @@ -131,7 +137,7 @@ function transformDestructuring( let reassigned: Set = new Set(); let hasDeclaration = false; for (const place of eachPatternOperand(destructure.lvalue.pattern)) { - const isDeclared = state.declared.has(place.identifier.id); + const isDeclared = state.declared.has(place.identifier.declarationId); if (isDeclared) { reassigned.add(place.identifier.id); } @@ -150,15 +156,7 @@ function transformDestructuring( if (!reassigned.has(place.identifier.id)) { return place; } - const tempId = state.env.nextIdentifierId; - const temporary = { - ...place, - identifier: { - ...place.identifier, - id: tempId, - name: null, // overwritten below - }, - }; + const temporary = clonePlaceToTemporary(state.env, place); promoteTemporary(temporary.identifier); renamed.set(place, temporary); return temporary; 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 4e4d00a04b70b..27aba91af2b1c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -8,6 +8,7 @@ import {CompilerError, SourceLocation} from '..'; import {Environment} from '../HIR'; import { + DeclarationId, GeneratedSource, HIRFunction, Identifier, @@ -257,6 +258,14 @@ export function findDisjointMutableValues( fn: HIRFunction, ): DisjointSet { const scopeIdentifiers = new DisjointSet(); + + const declarations = new Map(); + function declareIdentifier(lvalue: Place): void { + if (!declarations.has(lvalue.identifier.declarationId)) { + declarations.set(lvalue.identifier.declarationId, lvalue.identifier); + } + } + for (const [_, block] of fn.body.blocks) { /* * If a phi is mutated after creation, then we need to alias all of its operands such that they @@ -264,14 +273,19 @@ export function findDisjointMutableValues( */ for (const phi of block.phis) { if ( - // The phi was reset because it was not mutated after creation phi.id.mutableRange.start + 1 !== phi.id.mutableRange.end && phi.id.mutableRange.end > (block.instructions.at(0)?.id ?? block.terminal.id) ) { - for (const [, phiId] of phi.operands) { - scopeIdentifiers.union([phi.id, phiId]); + const operands = [phi.id]; + const declaration = declarations.get(phi.id.declarationId); + if (declaration !== undefined) { + operands.push(declaration); + } + for (const [_, phiId] of phi.operands) { + operands.push(phiId); } + scopeIdentifiers.union(operands); } else if (fn.env.config.enableForest) { for (const [, phiId] of phi.operands) { scopeIdentifiers.union([phi.id, phiId]); @@ -286,9 +300,15 @@ export function findDisjointMutableValues( operands.push(instr.lvalue!.identifier); } if ( + instr.value.kind === 'DeclareLocal' || + instr.value.kind === 'DeclareContext' + ) { + declareIdentifier(instr.value.lvalue.place); + } else if ( instr.value.kind === 'StoreLocal' || instr.value.kind === 'StoreContext' ) { + declareIdentifier(instr.value.lvalue.place); if ( instr.value.lvalue.place.identifier.mutableRange.end > instr.value.lvalue.place.identifier.mutableRange.start + 1 @@ -303,6 +323,7 @@ export function findDisjointMutableValues( } } else if (instr.value.kind === 'Destructure') { for (const place of eachPatternOperand(instr.value.lvalue.pattern)) { + declareIdentifier(place); if ( place.identifier.mutableRange.end > place.identifier.mutableRange.start + 1 diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts index cddfc128fc88c..1f3f8b1271814 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts @@ -9,6 +9,7 @@ import { HIRFunction, IdentifierId, makeInstructionId, + MutableRange, Place, ReactiveValue, } from '../HIR'; @@ -110,12 +111,7 @@ function visit( operand.identifier.scope = fbtScope; // Expand the jsx element's range to account for its operands - fbtScope.range.start = makeInstructionId( - Math.min( - fbtScope.range.start, - operand.identifier.mutableRange.start, - ), - ); + expandFbtScopeRange(fbtScope.range, operand.identifier.mutableRange); fbtValues.add(operand.identifier.id); } } else if ( @@ -136,12 +132,7 @@ function visit( operand.identifier.scope = fbtScope; // Expand the jsx element's range to account for its operands - fbtScope.range.start = makeInstructionId( - Math.min( - fbtScope.range.start, - operand.identifier.mutableRange.start, - ), - ); + expandFbtScopeRange(fbtScope.range, operand.identifier.mutableRange); /* * NOTE: we add the operands as fbt values so that they are also @@ -169,12 +160,7 @@ function visit( operand.identifier.scope = fbtScope; // Expand the jsx element's range to account for its operands - fbtScope.range.start = makeInstructionId( - Math.min( - fbtScope.range.start, - operand.identifier.mutableRange.start, - ), - ); + expandFbtScopeRange(fbtScope.range, operand.identifier.mutableRange); } } } @@ -214,3 +200,14 @@ function isFbtJsxChild( fbtValues.has(lvalue.identifier.id) ); } + +function expandFbtScopeRange( + fbtRange: MutableRange, + extendWith: MutableRange, +): void { + if (extendWith.start !== 0) { + fbtRange.start = makeInstructionId( + Math.min(fbtRange.start, extendWith.start), + ); + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts index e2ad0df893ec0..2c9004e6ad9a6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts @@ -7,7 +7,7 @@ import {CompilerError} from '..'; import { - IdentifierId, + DeclarationId, InstructionId, InstructionKind, Place, @@ -28,7 +28,7 @@ import { BuiltInObjectId, } from '../HIR/ObjectShape'; import {eachInstructionLValue} from '../HIR/visitors'; -import {assertExhaustive} from '../Utils/utils'; +import {assertExhaustive, Iterable_some} from '../Utils/utils'; import {printReactiveScopeSummary} from './PrintReactiveFunction'; import { ReactiveFunctionTransform, @@ -97,22 +97,29 @@ function log(msg: string): void { } class FindLastUsageVisitor extends ReactiveFunctionVisitor { - lastUsage: Map = new Map(); + /* + * TODO LeaveSSA: use IdentifierId for more precise tracking + * Using DeclarationId is necessary for compatible output but produces suboptimal results + * in cases where a scope defines a variable, but that version is never read and always + * overwritten later. + * see reassignment-separate-scopes.js for example + */ + lastUsage: Map = new Map(); override visitPlace(id: InstructionId, place: Place, _state: void): void { - const previousUsage = this.lastUsage.get(place.identifier.id); + const previousUsage = this.lastUsage.get(place.identifier.declarationId); const lastUsage = previousUsage !== undefined ? makeInstructionId(Math.max(previousUsage, id)) : id; - this.lastUsage.set(place.identifier.id, lastUsage); + this.lastUsage.set(place.identifier.declarationId, lastUsage); } } class Transform extends ReactiveFunctionTransform { - lastUsage: Map; + lastUsage: Map; - constructor(lastUsage: Map) { + constructor(lastUsage: Map) { super(); this.lastUsage = lastUsage; } @@ -144,7 +151,7 @@ class Transform extends ReactiveFunctionTransform; + lvalues: Set; }; let current: MergedScope | null = null; const merged: Array = []; @@ -204,7 +211,9 @@ class Transform extends ReactiveFunctionTransform, + lastUsage: Map, ): void { - for (const [key] of scope.declarations) { - const lastUsedAt = lastUsage.get(key)!; + for (const [id, decl] of scope.declarations) { + const lastUsedAt = lastUsage.get(decl.identifier.declarationId)!; if (lastUsedAt < scope.range.end) { - scope.declarations.delete(key); + scope.declarations.delete(id); } } } @@ -400,8 +409,8 @@ function updateScopeDeclarations( */ function areLValuesLastUsedByScope( scope: ReactiveScope, - lvalues: Set, - lastUsage: Map, + lvalues: Set, + lastUsage: Map, ): boolean { for (const lvalue of lvalues) { const lastUsedAt = lastUsage.get(lvalue)!; @@ -454,8 +463,12 @@ function canMergeScopes( (next.scope.dependencies.size !== 0 && [...next.scope.dependencies].every( dep => - current.scope.declarations.has(dep.identifier.id) && - isAlwaysInvalidatingType(dep.identifier.type), + isAlwaysInvalidatingType(dep.identifier.type) && + Iterable_some( + current.scope.declarations.values(), + decl => + decl.identifier.declarationId === dep.identifier.declarationId, + ), )) ) { log(` outputs of prev are input to current`); @@ -492,7 +505,7 @@ function areEqualDependencies( let found = false; for (const bValue of b) { if ( - aValue.identifier === bValue.identifier && + aValue.identifier.declarationId === bValue.identifier.declarationId && areEqualPaths(aValue.path, bValue.path) ) { found = true; @@ -506,7 +519,7 @@ function areEqualDependencies( return true; } -function areEqualPaths(a: Array, b: Array): boolean { +export function areEqualPaths(a: Array, b: Array): boolean { return a.length === b.length && a.every((item, ix) => item === b[ix]); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts index 65b27fe639f23..ac68050ddbf19 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts @@ -8,12 +8,13 @@ import {CompilerError} from '../CompilerError'; import {GeneratedSource} from '../HIR'; import { + DeclarationId, Identifier, - IdentifierId, InstructionId, Place, PrunedReactiveScopeBlock, ReactiveFunction, + ReactiveScope, ReactiveScopeBlock, ReactiveValue, ScopeId, @@ -24,7 +25,6 @@ import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors'; class Visitor extends ReactiveFunctionVisitor { override visitScope(scopeBlock: ReactiveScopeBlock, state: State): void { - this.traverseScope(scopeBlock, state); for (const dep of scopeBlock.scope.dependencies) { const {identifier} = dep; if (identifier.name == null) { @@ -43,21 +43,23 @@ class Visitor extends ReactiveFunctionVisitor { promoteIdentifier(declaration.identifier, state); } } + this.traverseScope(scopeBlock, state); } override visitPrunedScope( scopeBlock: PrunedReactiveScopeBlock, state: State, ): void { - this.traversePrunedScope(scopeBlock, state); for (const [, declaration] of scopeBlock.scope.declarations) { if ( declaration.identifier.name == null && - state.pruned.get(declaration.identifier.id)?.usedOutsideScope === true + state.pruned.get(declaration.identifier.declarationId) + ?.usedOutsideScope === true ) { promoteIdentifier(declaration.identifier, state); } } + this.traversePrunedScope(scopeBlock, state); } override visitParam(place: Place, state: State): void { @@ -93,11 +95,75 @@ class Visitor extends ReactiveFunctionVisitor { } } -type JsxExpressionTags = Set; +class Visitor2 extends ReactiveFunctionVisitor { + override visitPlace(_id: InstructionId, place: Place, state: State): void { + if ( + place.identifier.name === null && + state.promoted.has(place.identifier.declarationId) + ) { + promoteIdentifier(place.identifier, state); + } + } + override visitLValue( + _id: InstructionId, + _lvalue: Place, + _state: State, + ): void { + this.visitPlace(_id, _lvalue, _state); + } + traverseScopeIdentifiers(scope: ReactiveScope, state: State): void { + for (const [, decl] of scope.declarations) { + if ( + decl.identifier.name === null && + state.promoted.has(decl.identifier.declarationId) + ) { + promoteIdentifier(decl.identifier, state); + } + } + for (const dep of scope.dependencies) { + if ( + dep.identifier.name === null && + state.promoted.has(dep.identifier.declarationId) + ) { + promoteIdentifier(dep.identifier, state); + } + } + for (const reassignment of scope.reassignments) { + if ( + reassignment.name === null && + state.promoted.has(reassignment.declarationId) + ) { + promoteIdentifier(reassignment, state); + } + } + } + override visitScope(scope: ReactiveScopeBlock, state: State): void { + this.traverseScope(scope, state); + this.traverseScopeIdentifiers(scope.scope, state); + } + override visitPrunedScope( + scopeBlock: PrunedReactiveScopeBlock, + state: State, + ): void { + this.traversePrunedScope(scopeBlock, state); + this.traverseScopeIdentifiers(scopeBlock.scope, state); + } + override visitReactiveFunctionValue( + _id: InstructionId, + _dependencies: Array, + fn: ReactiveFunction, + state: State, + ): void { + visitReactiveFunction(fn, this, state); + } +} + +type JsxExpressionTags = Set; type State = { tags: JsxExpressionTags; + promoted: Set; pruned: Map< - IdentifierId, + DeclarationId, {activeScopes: Array; usedOutsideScope: boolean} >; // true if referenced within another scope, false if only accessed outside of scopes }; @@ -108,9 +174,9 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor { override visitPlace(_id: InstructionId, place: Place, state: State): void { if ( this.activeScopes.length !== 0 && - state.pruned.has(place.identifier.id) + state.pruned.has(place.identifier.declarationId) ) { - const prunedPlace = state.pruned.get(place.identifier.id)!; + const prunedPlace = state.pruned.get(place.identifier.declarationId)!; if (prunedPlace.activeScopes.indexOf(this.activeScopes.at(-1)!) === -1) { prunedPlace.usedOutsideScope = true; } @@ -124,7 +190,7 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor { ): void { this.traverseValue(id, value, state); if (value.kind === 'JsxExpression' && value.tag.kind === 'Identifier') { - state.tags.add(value.tag.identifier.id); + state.tags.add(value.tag.identifier.declarationId); } } @@ -132,8 +198,8 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor { scopeBlock: PrunedReactiveScopeBlock, state: State, ): void { - for (const [id] of scopeBlock.scope.declarations) { - state.pruned.set(id, { + for (const [_id, decl] of scopeBlock.scope.declarations) { + state.pruned.set(decl.identifier.declarationId, { activeScopes: [...this.activeScopes], usedOutsideScope: false, }); @@ -151,6 +217,7 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor { export function promoteUsedTemporaries(fn: ReactiveFunction): void { const state: State = { tags: new Set(), + promoted: new Set(), pruned: new Map(), }; visitReactiveFunction(fn, new CollectPromotableTemporaries(), state); @@ -161,6 +228,7 @@ export function promoteUsedTemporaries(fn: ReactiveFunction): void { } } visitReactiveFunction(fn, new Visitor(), state); + visitReactiveFunction(fn, new Visitor2(), state); } function promoteIdentifier(identifier: Identifier, state: State): void { @@ -171,9 +239,10 @@ function promoteIdentifier(identifier: Identifier, state: State): void { loc: GeneratedSource, suggestions: null, }); - if (state.tags.has(identifier.id)) { + if (state.tags.has(identifier.declarationId)) { promoteTemporaryJsxTag(identifier); } else { promoteTemporary(identifier); } + state.promoted.add(identifier.declarationId); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts index 086ce017de533..690bdb758839d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -8,9 +8,9 @@ import {CompilerError} from '../CompilerError'; import { BlockId, + DeclarationId, GeneratedSource, Identifier, - IdentifierId, InstructionId, InstructionKind, isObjectMethodType, @@ -30,11 +30,12 @@ import { } from '../HIR/HIR'; import {eachInstructionValueOperand, eachPatternOperand} from '../HIR/visitors'; import {empty, Stack} from '../Utils/Stack'; -import {assertExhaustive} from '../Utils/utils'; +import {assertExhaustive, Iterable_some} from '../Utils/utils'; import { ReactiveScopeDependencyTree, ReactiveScopePropertyDependency, } from './DeriveMinimalDependencies'; +import {areEqualPaths} from './MergeReactiveScopesThatInvalidateTogether'; import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors'; /* @@ -76,9 +77,9 @@ type TemporariesUsedOutsideDefiningScope = { * tracks all relevant temporary declarations (currently LoadLocal and PropertyLoad) * and the scope where they are defined */ - declarations: Map; + declarations: Map; // temporaries used outside of their defining scope - usedOutsideDeclaringScope: Set; + usedOutsideDeclaringScope: Set; }; class FindPromotedTemporaries extends ReactiveFunctionVisitor { scopes: Array = []; @@ -107,7 +108,10 @@ class FindPromotedTemporaries extends ReactiveFunctionVisitor; +type DeclMap = Map; type Decl = { id: InstructionId; scope: Stack; @@ -280,7 +286,7 @@ class PoisonState { } class Context { - #temporariesUsedOutsideScope: Set; + #temporariesUsedOutsideScope: Set; #declarations: DeclMap = new Map(); #reassignments: Map = new Map(); // Reactive dependencies used in the current reactive scope. @@ -307,7 +313,7 @@ class Context { #scopes: Stack = empty(); poisonState: PoisonState = new PoisonState(new Set(), new Set(), false); - constructor(temporariesUsedOutsideScope: Set) { + constructor(temporariesUsedOutsideScope: Set) { this.#temporariesUsedOutsideScope = temporariesUsedOutsideScope; } @@ -377,7 +383,9 @@ class Context { } isUsedOutsideDeclaringScope(place: Place): boolean { - return this.#temporariesUsedOutsideScope.has(place.identifier.id); + return this.#temporariesUsedOutsideScope.has( + place.identifier.declarationId, + ); } /* @@ -440,8 +448,8 @@ class Context { * on itself. */ declare(identifier: Identifier, decl: Decl): void { - if (!this.#declarations.has(identifier.id)) { - this.#declarations.set(identifier.id, decl); + if (!this.#declarations.has(identifier.declarationId)) { + this.#declarations.set(identifier.declarationId, decl); } this.#reassignments.set(identifier, decl); } @@ -533,7 +541,7 @@ class Context { */ const currentDeclaration = this.#reassignments.get(identifier) ?? - this.#declarations.get(identifier.id); + this.#declarations.get(identifier.declarationId); const currentScope = this.currentScope.value?.value; return ( currentScope != null && @@ -599,14 +607,23 @@ class Context { * (all other decls e.g. `let x;` should be initialized in BuildHIR) */ const originalDeclaration = this.#declarations.get( - maybeDependency.identifier.id, + maybeDependency.identifier.declarationId, ); if ( originalDeclaration !== undefined && originalDeclaration.scope.value !== null ) { originalDeclaration.scope.each(scope => { - if (!this.#isScopeActive(scope.value)) { + if ( + !this.#isScopeActive(scope.value) && + // TODO LeaveSSA: key scope.declarations by DeclarationId + !Iterable_some( + scope.value.declarations.values(), + decl => + decl.identifier.declarationId === + maybeDependency.identifier.declarationId, + ) + ) { scope.value.declarations.set(maybeDependency.identifier.id, { identifier: maybeDependency.identifier, scope: originalDeclaration.scope.value!.value, @@ -637,11 +654,14 @@ class Context { const currentScope = this.currentScope.value?.value; if ( currentScope != null && - !Array.from(currentScope.reassignments).some( - identifier => identifier.id === place.identifier.id, + !Iterable_some( + currentScope.reassignments, + identifier => + identifier.declarationId === place.identifier.declarationId, ) && this.#checkValidDependency({identifier: place.identifier, path: []}) ) { + // TODO LeaveSSA: scope.reassignments should be keyed by declarationid currentScope.reassignments.add(place.identifier); } } @@ -680,7 +700,37 @@ class PropagationVisitor extends ReactiveFunctionVisitor { const scopeDependencies = context.enter(scope.scope, () => { this.visitBlock(scope.instructions, context); }); - scope.scope.dependencies = scopeDependencies; + for (const candidateDep of scopeDependencies) { + if ( + !Iterable_some( + scope.scope.dependencies, + existingDep => + existingDep.identifier.declarationId === + candidateDep.identifier.declarationId && + areEqualPaths(existingDep.path, candidateDep.path), + ) + ) { + scope.scope.dependencies.add(candidateDep); + } + } + /* + * TODO LeaveSSA: fix existing bug with duplicate deps and reassignments + * see fixture ssa-cascading-eliminated-phis, note that we cache `x` + * twice because its both a dep and a reassignment. + * + * for (const reassignment of scope.scope.reassignments) { + * if ( + * Iterable_some( + * scope.scope.dependencies.values(), + * dep => + * dep.identifier.declarationId === reassignment.declarationId && + * dep.path.length === 0, + * ) + * ) { + * scope.scope.reassignments.delete(reassignment); + * } + * } + */ } override visitPrunedScope( diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts index 94efd27a694e0..8608b32298503 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts @@ -6,7 +6,7 @@ */ import { - Identifier, + DeclarationId, InstructionKind, ReactiveFunction, ReactiveInstruction, @@ -27,7 +27,7 @@ export function pruneHoistedContexts(fn: ReactiveFunction): void { visitReactiveFunction(fn, new Visitor(), hoistedIdentifiers); } -type HoistedIdentifiers = Set; +type HoistedIdentifiers = Set; class Visitor extends ReactiveFunctionTransform { override transformInstruction( @@ -39,13 +39,13 @@ class Visitor extends ReactiveFunctionTransform { instruction.value.kind === 'DeclareContext' && instruction.value.lvalue.kind === 'HoistedConst' ) { - state.add(instruction.value.lvalue.place.identifier); + state.add(instruction.value.lvalue.place.identifier.declarationId); return {kind: 'remove'}; } if ( instruction.value.kind === 'StoreContext' && - state.has(instruction.value.lvalue.place.identifier) + state.has(instruction.value.lvalue.place.identifier.declarationId) ) { return { kind: 'replace', diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index aa9baec851f75..2951d50485300 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -7,8 +7,8 @@ import {CompilerError} from '../CompilerError'; import { + DeclarationId, Environment, - IdentifierId, InstructionId, Pattern, Place, @@ -115,9 +115,9 @@ export function pruneNonEscapingScopes(fn: ReactiveFunction): void { const state = new State(fn.env); for (const param of fn.params) { if (param.kind === 'Identifier') { - state.declare(param.identifier.id); + state.declare(param.identifier.declarationId); } else { - state.declare(param.place.identifier.id); + state.declare(param.place.identifier.declarationId); } } visitReactiveFunction(fn, new CollectDependenciesVisitor(fn.env), state); @@ -193,14 +193,14 @@ function joinAliases( type IdentifierNode = { level: MemoizationLevel; memoized: boolean; - dependencies: Set; + dependencies: Set; scopes: Set; seen: boolean; }; // A scope node describing its dependencies type ScopeNode = { - dependencies: Array; + dependencies: Array; seen: boolean; }; @@ -209,20 +209,30 @@ class State { env: Environment; /* * Maps lvalues for LoadLocal to the identifier being loaded, to resolve indirections - * in subsequent lvalues/rvalues + * in subsequent lvalues/rvalues. + * + * NOTE: this pass uses DeclarationId rather than IdentifierId because the pass is not + * aware of control-flow, only data flow via mutation. Instead of precisely modeling + * control flow, we analyze all values that may flow into a particular program variable, + * and then whether that program variable may escape (if so, the values flowing in may + * escape too). Thus we use DeclarationId to captures all values that may flow into + * a particular program variable, regardless of control flow paths. + * + * In the future when we convert to HIR everywhere this pass can account for control + * flow and use SSA ids. */ - definitions: Map = new Map(); + definitions: Map = new Map(); - identifiers: Map = new Map(); + identifiers: Map = new Map(); scopes: Map = new Map(); - escapingValues: Set = new Set(); + escapingValues: Set = new Set(); constructor(env: Environment) { this.env = env; } // Declare a new identifier, used for function id and params - declare(id: IdentifierId): void { + declare(id: DeclarationId): void { this.identifiers.set(id, { level: MemoizationLevel.Never, memoized: false, @@ -240,14 +250,16 @@ class State { visitOperand( id: InstructionId, place: Place, - identifier: IdentifierId, + identifier: DeclarationId, ): void { const scope = getPlaceScope(id, place); if (scope !== null) { let node = this.scopes.get(scope.id); if (node === undefined) { node = { - dependencies: [...scope.dependencies].map(dep => dep.identifier.id), + dependencies: [...scope.dependencies].map( + dep => dep.identifier.declarationId, + ), seen: false, }; this.scopes.set(scope.id, node); @@ -269,11 +281,11 @@ class State { * to determine which other values should be memoized. Returns a set of all identifiers * that should be memoized. */ -function computeMemoizedIdentifiers(state: State): Set { - const memoized = new Set(); +function computeMemoizedIdentifiers(state: State): Set { + const memoized = new Set(); // Visit an identifier, optionally forcing it to be memoized - function visit(id: IdentifierId, forceMemoize: boolean = false): boolean { + function visit(id: DeclarationId, forceMemoize: boolean = false): boolean { const node = state.identifiers.get(id); CompilerError.invariant(node !== undefined, { reason: `Expected a node for all identifiers, none found for \`${id}\``, @@ -832,14 +844,16 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { // Associate all the rvalues with the instruction's scope if it has one for (const operand of aliasing.rvalues) { const operandId = - state.definitions.get(operand.identifier.id) ?? operand.identifier.id; + state.definitions.get(operand.identifier.declarationId) ?? + operand.identifier.declarationId; state.visitOperand(instruction.id, operand, operandId); } // Add the operands as dependencies of all lvalues. for (const {place: lvalue, level} of aliasing.lvalues) { const lvalueId = - state.definitions.get(lvalue.identifier.id) ?? lvalue.identifier.id; + state.definitions.get(lvalue.identifier.declarationId) ?? + lvalue.identifier.declarationId; let node = state.identifiers.get(lvalueId); if (node === undefined) { node = { @@ -858,7 +872,8 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { */ for (const operand of aliasing.rvalues) { const operandId = - state.definitions.get(operand.identifier.id) ?? operand.identifier.id; + state.definitions.get(operand.identifier.declarationId) ?? + operand.identifier.declarationId; if (operandId === lvalueId) { continue; } @@ -870,8 +885,8 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { if (instruction.value.kind === 'LoadLocal' && instruction.lvalue !== null) { state.definitions.set( - instruction.lvalue.identifier.id, - instruction.value.place.identifier.id, + instruction.lvalue.identifier.declarationId, + instruction.value.place.identifier.declarationId, ); } else if ( instruction.value.kind === 'CallExpression' || @@ -897,7 +912,7 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { } for (const operand of instruction.value.args) { const place = operand.kind === 'Spread' ? operand.place : operand; - state.escapingValues.add(place.identifier.id); + state.escapingValues.add(place.identifier.declarationId); } } } @@ -910,20 +925,20 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { this.traverseTerminal(stmt, state); if (stmt.terminal.kind === 'return') { - state.escapingValues.add(stmt.terminal.value.identifier.id); + state.escapingValues.add(stmt.terminal.value.identifier.declarationId); } } } // Prune reactive scopes that do not have any memoized outputs class PruneScopesTransform extends ReactiveFunctionTransform< - Set + Set > { prunedScopes: Set = new Set(); override transformScope( scopeBlock: ReactiveScopeBlock, - state: Set, + state: Set, ): Transformed { this.visitScope(scopeBlock, state); @@ -945,11 +960,11 @@ class PruneScopesTransform extends ReactiveFunctionTransform< } const hasMemoizedOutput = - Array.from(scopeBlock.scope.declarations.keys()).some(id => - state.has(id), + Array.from(scopeBlock.scope.declarations.values()).some(decl => + state.has(decl.identifier.declarationId), ) || Array.from(scopeBlock.scope.reassignments).some(identifier => - state.has(identifier.id), + state.has(identifier.declarationId), ); if (hasMemoizedOutput) { return {kind: 'keep'}; @@ -964,7 +979,7 @@ class PruneScopesTransform extends ReactiveFunctionTransform< override transformInstruction( instruction: ReactiveInstruction, - state: Set, + state: Set, ): Transformed { this.traverseInstruction(instruction, state); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts index 535a4e5e911d4..76f48adacab4a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts @@ -6,7 +6,7 @@ */ import { - Identifier, + DeclarationId, InstructionId, Place, ReactiveFunction, @@ -18,19 +18,27 @@ import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors'; * Nulls out lvalues for temporary variables that are never accessed later. This only * nulls out the lvalue itself, it does not remove the corresponding instructions. */ -export function pruneTemporaryLValues(fn: ReactiveFunction): void { - const lvalues = new Map(); +export function pruneUnusedLValues(fn: ReactiveFunction): void { + const lvalues = new Map(); visitReactiveFunction(fn, new Visitor(), lvalues); for (const [, instr] of lvalues) { instr.lvalue = null; } } -type LValues = Map; +/** + * This pass uses DeclarationIds because the lvalue IdentifierId of a compound expression + * (ternary, logical, optional) in ReactiveFunction may not be the same as the IdentifierId + * of the phi, and which is referenced later. Keying by DeclarationId ensures we don't + * delete lvalues for identifiers that are used. + * + * TODO LeaveSSA: once we use HIR everywhere, this can likely move back to using IdentifierId + */ +type LValues = Map; class Visitor extends ReactiveFunctionVisitor { override visitPlace(id: InstructionId, place: Place, state: LValues): void { - state.delete(place.identifier); + state.delete(place.identifier.declarationId); } override visitInstruction( instruction: ReactiveInstruction, @@ -41,7 +49,7 @@ class Visitor extends ReactiveFunctionVisitor { instruction.lvalue !== null && instruction.lvalue.identifier.name === null ) { - state.set(instruction.lvalue.identifier, instruction); + state.set(instruction.lvalue.identifier.declarationId, instruction); } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/RenameVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/RenameVariables.ts index a68bdfa5bfda9..f84965b92e6ee 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/RenameVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/RenameVariables.ts @@ -7,8 +7,8 @@ import {CompilerError} from '../CompilerError'; import { + DeclarationId, Identifier, - IdentifierId, IdentifierName, InstructionId, Place, @@ -121,8 +121,8 @@ class Visitor extends ReactiveFunctionVisitor { } class Scopes { - #seen: Map = new Map(); - #stack: Array> = [new Map()]; + #seen: Map = new Map(); + #stack: Array> = [new Map()]; #globals: Set; names: Set = new Set(); @@ -135,7 +135,7 @@ class Scopes { if (originalName === null) { return; } - const mappedName = this.#seen.get(identifier.id); + const mappedName = this.#seen.get(identifier.declarationId); if (mappedName !== undefined) { identifier.name = mappedName; return; @@ -158,12 +158,12 @@ class Scopes { } const identifierName = makeIdentifierName(name); identifier.name = identifierName; - this.#seen.set(identifier.id, identifierName); - this.#stack.at(-1)!.set(identifierName.value, identifier.id); + this.#seen.set(identifier.declarationId, identifierName); + this.#stack.at(-1)!.set(identifierName.value, identifier.declarationId); this.names.add(identifierName.value); } - #lookup(name: string): IdentifierId | null { + #lookup(name: string): DeclarationId | null { for (let i = this.#stack.length - 1; i >= 0; i--) { const scope = this.#stack[i]!; const entry = scope.get(name); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/index.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/index.ts index 8f6cad8d11fba..55f67fc2f7d23 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/index.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/index.ts @@ -27,7 +27,7 @@ export {pruneAllReactiveScopes} from './PruneAllReactiveScopes'; export {pruneHoistedContexts} from './PruneHoistedContexts'; export {pruneNonEscapingScopes} from './PruneNonEscapingScopes'; export {pruneNonReactiveDependencies} from './PruneNonReactiveDependencies'; -export {pruneTemporaryLValues as pruneUnusedLValues} from './PruneTemporaryLValues'; +export {pruneUnusedLValues} from './PruneTemporaryLValues'; export {pruneUnusedLabels} from './PruneUnusedLabels'; export {pruneUnusedScopes} from './PruneUnusedScopes'; export {renameVariables} from './RenameVariables'; diff --git a/compiler/packages/babel-plugin-react-compiler/src/SSA/LeaveSSA.ts b/compiler/packages/babel-plugin-react-compiler/src/SSA/LeaveSSA.ts deleted file mode 100644 index 8cf569a490e8b..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/SSA/LeaveSSA.ts +++ /dev/null @@ -1,515 +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. - */ - -import {CompilerError} from '../CompilerError'; -import { - BasicBlock, - BlockId, - HIRFunction, - Identifier, - InstructionKind, - LValue, - LValuePattern, - Phi, - Place, -} from '../HIR/HIR'; -import {printIdentifier, printPlace} from '../HIR/PrintHIR'; -import { - eachInstructionLValue, - eachInstructionValueOperand, - eachPatternOperand, - eachTerminalOperand, - eachTerminalSuccessor, - terminalFallthrough, -} from '../HIR/visitors'; - -/* - * Removes SSA form by converting all phis into explicit bindings and assignments. There are two main categories - * of phis: - * - * ## Reassignments (operands are independently memoizable) - * - * These are phis that occur after some high-level control flow such as an if, switch, or loop. These phis are rewritten - * to add a new `let` binding for the phi id prior to the control flow node (ie prior to the if/switch), - * and to add a reassignment to that let binding in each of the phi's predecessors. - * - * Example: - * - * ```javascript - * // Input - * let x1 = null; - * if (a) { - * x2 = b; - * } else { - * x3 = c; - * } - * x4 = phi(x2, x3); - * return x4; - * - * // Output - * const x1 = null; - * let x4; // synthesized binding for the phi identifier - * if (a) { - * x2 = b; - * x4 = x2;; // sythesized assignment to the phi identifier - * } else { - * x3 = c; - * x4 = x3; // synthesized assignment - * } - * // phi removed - * return x4; - * ``` - * - * ## Rewrites (operands are not independently memoizable) - * - * Phis that occur inside loop constructs cannot use the reassignment strategy, because there isn't an appropriate place - * to add the new let binding. Instead, we select a single "canonical" id for these phis which is the operand that is - * defined first. Then, all assignments and references for any of the phi ir and operands are rewritten to reference - * the canonical id instead. - * - * Example: - * - * ```javascript - * // Input - * for ( - * let i1 = 0; - * { i2 = phi(i1, i2); i2 < 10 }; // note the phi in the test block - * i2 += 1 - * ) { ... } - * - * // Output - * for ( - * let i1 = 0; // i1 is defined first, so it becomes the canonical id - * i1 < 10; // rewritten to canonical id - * i1 += 1 // rewritten to canonical id - * ) - * ``` - */ -export function leaveSSA(fn: HIRFunction): void { - // Maps identifier names to their original declaration. - const declarations: Map< - string, - {lvalue: LValue | LValuePattern; place: Place} - > = new Map(); - - for (const param of fn.params) { - let place: Place = param.kind === 'Identifier' ? param : param.place; - if (place.identifier.name !== null) { - declarations.set(place.identifier.name.value, { - lvalue: { - kind: InstructionKind.Let, - place, - }, - place, - }); - } - } - - /* - * For non-memoizable phis, this maps original identifiers to the identifier they should be - * *rewritten* to. The keys are the original identifiers, and the value will be _either_ the - * phi id or, more typically, the operand that was defined prior to the phi. - */ - const rewrites: Map = new Map(); - - type PhiState = { - phi: Phi; - block: BasicBlock; - }; - - const seen = new Set(); - const backEdgePhis = new Set(); - for (const [, block] of fn.body.blocks) { - for (const phi of block.phis) { - for (const [pred] of phi.operands) { - if (!seen.has(pred)) { - backEdgePhis.add(phi); - break; - } - } - } - seen.add(block.id); - } - - for (const [, block] of fn.body.blocks) { - for (const instr of block.instructions) { - /* - * Iterate the instructions and perform any rewrites as well as promoting SSA variables to - * `let` or `reassign` where possible. - */ - const {lvalue, value} = instr; - if (value.kind === 'DeclareLocal') { - const name = value.lvalue.place.identifier.name; - if (name !== null) { - CompilerError.invariant(!declarations.has(name.value), { - reason: `Unexpected duplicate declaration`, - description: `Found duplicate declaration for \`${name.value}\``, - loc: value.lvalue.place.loc, - suggestions: null, - }); - declarations.set(name.value, { - lvalue: value.lvalue, - place: value.lvalue.place, - }); - } - } else if ( - value.kind === 'PrefixUpdate' || - value.kind === 'PostfixUpdate' - ) { - CompilerError.invariant(value.lvalue.identifier.name !== null, { - reason: `Expected update expression to be applied to a named variable`, - description: null, - loc: value.lvalue.loc, - suggestions: null, - }); - const originalLVal = declarations.get( - value.lvalue.identifier.name.value, - ); - CompilerError.invariant(originalLVal !== undefined, { - reason: `Expected update expression to be applied to a previously defined variable`, - description: null, - loc: value.lvalue.loc, - suggestions: null, - }); - originalLVal.lvalue.kind = InstructionKind.Let; - } else if (value.kind === 'StoreLocal') { - if (value.lvalue.place.identifier.name != null) { - const originalLVal = declarations.get( - value.lvalue.place.identifier.name.value, - ); - if ( - originalLVal === undefined || - originalLVal.lvalue === value.lvalue // in case this was pre-declared for the `for` initializer - ) { - CompilerError.invariant( - originalLVal !== undefined || - block.kind === 'block' || - block.kind === 'catch', - { - reason: `TODO: Handle reassignment in a value block where the original declaration was removed by dead code elimination (DCE)`, - description: null, - loc: value.lvalue.place.loc, - suggestions: null, - }, - ); - declarations.set(value.lvalue.place.identifier.name.value, { - lvalue: value.lvalue, - place: value.lvalue.place, - }); - value.lvalue.kind = InstructionKind.Const; - } else { - /* - * This is an instance of the original id, so we need to promote the original declaration - * to a `let` and the current lval to a `reassign` - */ - originalLVal.lvalue.kind = InstructionKind.Let; - value.lvalue.kind = InstructionKind.Reassign; - } - } else if (rewrites.has(value.lvalue.place.identifier)) { - value.lvalue.kind = InstructionKind.Const; - } - } else if (value.kind === 'Destructure') { - let kind: InstructionKind | null = null; - for (const place of eachPatternOperand(value.lvalue.pattern)) { - if (place.identifier.name == null) { - CompilerError.invariant( - kind === null || kind === InstructionKind.Const, - { - reason: `Expected consistent kind for destructuring`, - description: `other places were \`${kind}\` but '${printPlace( - place, - )}' is const`, - loc: place.loc, - suggestions: null, - }, - ); - kind = InstructionKind.Const; - } else { - const originalLVal = declarations.get(place.identifier.name.value); - if ( - originalLVal === undefined || - originalLVal.lvalue === value.lvalue - ) { - CompilerError.invariant( - originalLVal !== undefined || block.kind !== 'value', - { - reason: `TODO: Handle reassignment in a value block where the original declaration was removed by dead code elimination (DCE)`, - description: null, - loc: place.loc, - suggestions: null, - }, - ); - declarations.set(place.identifier.name.value, { - lvalue: value.lvalue, - place, - }); - CompilerError.invariant( - kind === null || kind === InstructionKind.Const, - { - reason: `Expected consistent kind for destructuring`, - description: `Other places were \`${kind}\` but '${printPlace( - place, - )}' is const`, - loc: place.loc, - suggestions: null, - }, - ); - kind = InstructionKind.Const; - } else { - CompilerError.invariant( - kind === null || kind === InstructionKind.Reassign, - { - reason: `Expected consistent kind for destructuring`, - description: `Other places were \`${kind}\` but '${printPlace( - place, - )}' is reassigned`, - loc: place.loc, - suggestions: null, - }, - ); - kind = InstructionKind.Reassign; - originalLVal.lvalue.kind = InstructionKind.Let; - } - } - } - CompilerError.invariant(kind !== null, { - reason: 'Expected at least one operand', - description: null, - loc: null, - suggestions: null, - }); - value.lvalue.kind = kind; - } - rewritePlace(lvalue, rewrites, declarations); - for (const operand of eachInstructionLValue(instr)) { - rewritePlace(operand, rewrites, declarations); - } - for (const operand of eachInstructionValueOperand(instr.value)) { - rewritePlace(operand, rewrites, declarations); - } - } - - const terminal = block.terminal; - for (const operand of eachTerminalOperand(terminal)) { - rewritePlace(operand, rewrites, declarations); - } - - /* - * Find any phi nodes which need a variable declaration in the current block - * This includes phis in fallthrough nodes, or blocks that form part of control flow - * such as for or while (and later if/switch). - */ - const reassignmentPhis: Array = []; - const rewritePhis: Array = []; - function pushPhis(phiBlock: BasicBlock): void { - for (const phi of phiBlock.phis) { - if (phi.id.name === null) { - rewritePhis.push({phi, block: phiBlock}); - } else { - reassignmentPhis.push({phi, block: phiBlock}); - } - } - } - const fallthroughId = terminalFallthrough(terminal); - if (fallthroughId !== null) { - const fallthrough = fn.body.blocks.get(fallthroughId)!; - pushPhis(fallthrough); - } - if (terminal.kind === 'while' || terminal.kind === 'for') { - const test = fn.body.blocks.get(terminal.test)!; - pushPhis(test); - - const loop = fn.body.blocks.get(terminal.loop)!; - pushPhis(loop); - } - if ( - terminal.kind === 'for' || - terminal.kind === 'for-of' || - terminal.kind === 'for-in' - ) { - let init = fn.body.blocks.get(terminal.init)!; - pushPhis(init); - - // The first block after the end of the init - let initContinuation = - terminal.kind === 'for' ? terminal.test : terminal.loop; - /* - * To avoid generating a let binding for the initializer prior to the loop, - * check to see if the for declares an iterator variable. - */ - const queue: Array = [init.id]; - while (queue.length !== 0) { - const blockId = queue.shift()!; - if (blockId === initContinuation) { - break; - } - const block = fn.body.blocks.get(blockId)!; - for (const instr of block.instructions) { - if ( - instr.value.kind === 'StoreLocal' && - instr.value.lvalue.kind !== InstructionKind.Reassign - ) { - const value = instr.value; - if (value.lvalue.place.identifier.name !== null) { - const originalLVal = declarations.get( - value.lvalue.place.identifier.name.value, - ); - if (originalLVal === undefined) { - declarations.set(value.lvalue.place.identifier.name.value, { - lvalue: value.lvalue, - place: value.lvalue.place, - }); - value.lvalue.kind = InstructionKind.Const; - } - } - } - } - - switch (block.terminal.kind) { - case 'maybe-throw': { - queue.push(block.terminal.continuation); - break; - } - case 'goto': { - queue.push(block.terminal.block); - break; - } - case 'branch': - case 'logical': - case 'optional': - case 'ternary': - case 'label': { - for (const successor of eachTerminalSuccessor(block.terminal)) { - queue.push(successor); - } - break; - } - default: { - break; - } - } - } - - if (terminal.kind === 'for' && terminal.update !== null) { - const update = fn.body.blocks.get(terminal.update)!; - pushPhis(update); - } - } - - for (const {phi, block: phiBlock} of reassignmentPhis) { - /* - * In some cases one of the phi operands can be defined *before* the let binding - * we will generate. For example, a variable that is only rebound in one branch of - * an if but not another. In this case we populate the let binding with this initial - * value rather than generate an extra assignment. - */ - let initOperand: Identifier | null = null; - for (const [, operand] of phi.operands) { - if (operand.mutableRange.start < terminal.id) { - if (initOperand == null) { - initOperand = operand; - } - } - } - - /* - * If the phi is mutated after its creation, then any values which flow into the phi - * must also have their ranges extended accordingly. - */ - const isPhiMutatedAfterCreation: boolean = - phi.id.mutableRange.end > - (phiBlock.instructions.at(0)?.id ?? phiBlock.terminal.id); - - /* - * If we never saw a declaration for this phi, it may have been pruned by DCE, so synthesize - * a new Let binding - */ - CompilerError.invariant(phi.id.name != null, { - reason: 'Expected reassignment phis to have a name', - description: null, - loc: null, - suggestions: null, - }); - const declaration = declarations.get(phi.id.name.value); - CompilerError.invariant(declaration != null, { - loc: null, - reason: 'Expected a declaration for all variables', - description: `${printIdentifier(phi.id)} in block bb${phiBlock.id}`, - suggestions: null, - }); - if (isPhiMutatedAfterCreation) { - /* - * The declaration is not guaranteed to flow into the phi, for example in the case of a variable - * that is reassigned in all control flow paths to a given phi. The original declaration's range - * has to be extended in this case (if the phi is later mutated) since we are reusing the original - * declaration instead of creating a new declaration. - * - * NOTE: this can *only* happen if the original declaration involves an instruction that DCE does - * not prune. Otherwise, the declaration would have been pruned and we'd synthesize a new one. - */ - declaration.place.identifier.mutableRange.end = phi.id.mutableRange.end; - } - rewrites.set(phi.id, declaration.place.identifier); - } - - /* - * Similar logic for rewrite phis that occur in loops, except that instead of a new let binding - * we pick one of the operands as the canonical id, and rewrite all references to the other - * operands and the phi to reference this canonical id. - */ - for (const {phi} of rewritePhis) { - let canonicalId = rewrites.get(phi.id); - if (canonicalId === undefined) { - canonicalId = phi.id; - for (const [, operand] of phi.operands) { - let canonicalOperand = rewrites.get(operand) ?? operand; - if (canonicalOperand.id < canonicalId.id) { - canonicalId = canonicalOperand; - } - } - rewrites.set(phi.id, canonicalId); - - if (canonicalId.name !== null) { - const declaration = declarations.get(canonicalId.name.value); - if (declaration !== undefined) { - declaration.lvalue.kind = InstructionKind.Let; - } - } - } - - // all versions of the variable need to be remapped to the canonical id - for (const [, operand] of phi.operands) { - rewrites.set(operand, canonicalId); - } - } - } -} - -/* - * Rewrite @param place's identifier based on the given rewrite mapping, if the identifier - * is present. Also expands the mutable range of the target identifier to include the - * place's range. - */ -function rewritePlace( - place: Place, - rewrites: Map, - declarations: Map, -): void { - const prevIdentifier = place.identifier; - const nextIdentifier = rewrites.get(prevIdentifier); - - if (nextIdentifier !== undefined) { - if (nextIdentifier === prevIdentifier) return; - place.identifier = nextIdentifier; - } else if (prevIdentifier.name != null) { - const declaration = declarations.get(prevIdentifier.name.value); - // Only rewrite identifiers that were declared within the function - if (declaration === undefined) return; - const originalIdentifier = declaration.place.identifier; - prevIdentifier.id = originalIdentifier.id; - } -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/SSA/RewriteInstructionKindsBasedOnReassignment.ts b/compiler/packages/babel-plugin-react-compiler/src/SSA/RewriteInstructionKindsBasedOnReassignment.ts new file mode 100644 index 0000000000000..0af28e0d12596 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/SSA/RewriteInstructionKindsBasedOnReassignment.ts @@ -0,0 +1,174 @@ +/** + * 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. + */ + +import {CompilerError} from '../CompilerError'; +import { + DeclarationId, + HIRFunction, + InstructionKind, + LValue, + LValuePattern, + Place, +} from '../HIR/HIR'; +import {printPlace} from '../HIR/PrintHIR'; +import {eachPatternOperand} from '../HIR/visitors'; + +/** + * This pass rewrites the InstructionKind of instructions which declare/assign variables, + * converting the first declaration to a Const/Let depending on whether it is subsequently + * reassigned, and ensuring that subsequent reassignments are marked as a Reassign. Note + * that declarations which were const in the original program cannot become `let`, but the + * inverse is not true: a `let` which was reassigned in the source may be converted to a + * `const` if the reassignment is not used and was removed by dead code elimination. + * + * NOTE: this is a subset of the operations previously performed by the LeaveSSA pass. + */ +export function rewriteInstructionKindsBasedOnReassignment( + fn: HIRFunction, +): void { + const declarations = new Map(); + for (const param of fn.params) { + let place: Place = param.kind === 'Identifier' ? param : param.place; + if (place.identifier.name !== null) { + declarations.set(place.identifier.declarationId, { + kind: InstructionKind.Let, + place, + }); + } + } + for (const place of fn.context) { + if (place.identifier.name !== null) { + declarations.set(place.identifier.declarationId, { + kind: InstructionKind.Let, + place, + }); + } + } + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + const {value} = instr; + switch (value.kind) { + case 'DeclareLocal': { + const lvalue = value.lvalue; + CompilerError.invariant( + !declarations.has(lvalue.place.identifier.declarationId), + { + reason: `Expected variable not to be defined prior to declaration`, + description: `${printPlace(lvalue.place)} was already defined`, + loc: lvalue.place.loc, + }, + ); + declarations.set(lvalue.place.identifier.declarationId, lvalue); + break; + } + case 'StoreLocal': { + const lvalue = value.lvalue; + if (lvalue.place.identifier.name !== null) { + const declaration = declarations.get( + lvalue.place.identifier.declarationId, + ); + if (declaration === undefined) { + CompilerError.invariant( + !declarations.has(lvalue.place.identifier.declarationId), + { + reason: `Expected variable not to be defined prior to declaration`, + description: `${printPlace(lvalue.place)} was already defined`, + loc: lvalue.place.loc, + }, + ); + declarations.set(lvalue.place.identifier.declarationId, lvalue); + lvalue.kind = InstructionKind.Const; + } else { + declaration.kind = InstructionKind.Let; + lvalue.kind = InstructionKind.Reassign; + } + } + break; + } + case 'Destructure': { + const lvalue = value.lvalue; + let kind: InstructionKind | null = null; + for (const place of eachPatternOperand(lvalue.pattern)) { + if (place.identifier.name === null) { + CompilerError.invariant( + kind === null || kind === InstructionKind.Const, + { + reason: `Expected consistent kind for destructuring`, + description: `other places were \`${kind}\` but '${printPlace( + place, + )}' is const`, + loc: place.loc, + suggestions: null, + }, + ); + kind = InstructionKind.Const; + } else { + const declaration = declarations.get( + place.identifier.declarationId, + ); + if (declaration === undefined) { + CompilerError.invariant(block.kind !== 'value', { + reason: `TODO: Handle reassignment in a value block where the original declaration was removed by dead code elimination (DCE)`, + description: null, + loc: place.loc, + suggestions: null, + }); + declarations.set(place.identifier.declarationId, lvalue); + CompilerError.invariant( + kind === null || kind === InstructionKind.Const, + { + reason: `Expected consistent kind for destructuring`, + description: `Other places were \`${kind}\` but '${printPlace( + place, + )}' is const`, + loc: place.loc, + suggestions: null, + }, + ); + kind = InstructionKind.Const; + } else { + CompilerError.invariant( + kind === null || kind === InstructionKind.Reassign, + { + reason: `Expected consistent kind for destructuring`, + description: `Other places were \`${kind}\` but '${printPlace( + place, + )}' is reassigned`, + loc: place.loc, + suggestions: null, + }, + ); + kind = InstructionKind.Reassign; + declaration.kind = InstructionKind.Let; + } + } + } + CompilerError.invariant(kind !== null, { + reason: 'Expected at least one operand', + description: null, + loc: null, + suggestions: null, + }); + lvalue.kind = kind; + break; + } + case 'PostfixUpdate': + case 'PrefixUpdate': { + const lvalue = value.lvalue; + const declaration = declarations.get(lvalue.identifier.declarationId); + CompilerError.invariant(declaration !== undefined, { + reason: `Expected variable to have been defined`, + description: `No declaration for ${printPlace(lvalue)}`, + loc: lvalue.loc, + }); + declaration.kind = InstructionKind.Let; + break; + } + } + } + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/SSA/index.ts b/compiler/packages/babel-plugin-react-compiler/src/SSA/index.ts index 0e9212e78decd..9ce9f5b21d779 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/SSA/index.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/SSA/index.ts @@ -7,4 +7,4 @@ export {eliminateRedundantPhi} from './EliminateRedundantPhi'; export {default as enterSSA} from './EnterSSA'; -export {leaveSSA} from './LeaveSSA'; +export {rewriteInstructionKindsBasedOnReassignment} from './RewriteInstructionKindsBasedOnReassignment'; diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/PropagatePhiTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/PropagatePhiTypes.ts new file mode 100644 index 0000000000000..81335c175a31b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/PropagatePhiTypes.ts @@ -0,0 +1,108 @@ +/** + * 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. + */ + +import {HIRFunction, IdentifierId, Type, typeEquals} from '../HIR'; + +/** + * Temporary workaround for InferTypes not propagating the types of phis. + * Previously, LeaveSSA would replace all the identifiers for each phi (operands and + * the phi itself) with a single "canonical" identifier, generally chosen as the first + * operand to flow into the phi. In case of a phi whose operand was a phi, this could + * sometimes be an operand from the earlier phi. + * + * As a result, even though InferTypes did not propagate types for phis, LeaveSSA + * could end up replacing the phi Identifier with another identifer from an operand, + * which _did_ have a type inferred. + * + * This didn't affect the initial construction of mutable ranges because InferMutableRanges + * runs before LeaveSSA - thus, the types propagated by LeaveSSA only affected later optimizations, + * notably MergeScopesThatInvalidateTogether which uses type to determine if a scope's output + * will always invalidate with its input. + * + * The long-term correct approach is to update InferTypes to infer the types of phis, + * but this is complicated because InferMutableRanges inadvertently depends on phis + * never having a known type, such that a Store effect cannot occur on a phi value. + * Once we fix InferTypes to infer phi types, then we'll also have to update InferMutableRanges + * to handle this case. + * + * As a temporary workaround, this pass propagates the type of phis and can be called + * safely *after* InferMutableRanges. Unlike LeaveSSA, this pass only propagates the + * type if all operands have the same type, it's its more correct. + */ +export function propagatePhiTypes(fn: HIRFunction): void { + /** + * We track which SSA ids have had their types propagated to handle nested ternaries, + * see the StoreLocal handling below + */ + const propagated = new Set(); + for (const [, block] of fn.body.blocks) { + for (const phi of block.phis) { + /* + * We replicate the previous LeaveSSA behavior and only propagate types for + * unnamed variables. LeaveSSA would have chosen one of the operands as the + * canonical id and taken its type as the type of all identifiers. We're + * more conservative and only propagate if the types are the same and the + * phi didn't have a type inferred. + * + * Note that this can change output slightly in cases such as + * `cond ?
: null`. + * + * Previously the first operand's type (BuiltInJsx) would have been propagated, + * and this expression may have been merged with subsequent reactive scopes + * since it appears (based on that type) to always invalidate. + * + * But the correct type is `BuiltInJsx | null`, which we can't express and + * so leave as a generic `Type`, which does not always invalidate and therefore + * does not merge with subsequent scopes. + * + * We also don't propagate scopes for named variables, to preserve compatibility + * with previous LeaveSSA behavior. + */ + if (phi.id.type.kind !== 'Type' || phi.id.name !== null) { + continue; + } + let type: Type | null = null; + for (const [, operand] of phi.operands) { + if (type === null) { + type = operand.type; + } else if (!typeEquals(type, operand.type)) { + type = null; + break; + } + } + if (type !== null) { + phi.id.type = type; + phi.type = type; + propagated.add(phi.id.id); + } + } + for (const instr of block.instructions) { + const {value} = instr; + switch (value.kind) { + case 'StoreLocal': { + /** + * Nested ternaries can lower to a form with an intermediate StoreLocal where + * the value.lvalue is the temporary of the outer ternary, and the value.value + * is the result of the inner ternary. + * + * This is a common pattern in practice and easy enough to support. Again, the + * long-term approach is to update InferTypes and InferMutableRanges. + */ + const lvalue = value.lvalue.place; + if ( + propagated.has(value.value.identifier.id) && + lvalue.identifier.type.kind === 'Type' && + lvalue.identifier.name === null + ) { + lvalue.identifier.type = value.value.identifier.type; + propagated.add(lvalue.identifier.id); + } + } + } + } + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts b/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts index f46382a039a1f..6e07e14a8d0e6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts @@ -93,6 +93,18 @@ export function Set_union(a: Set, b: Set): Set { return union; } +export function Iterable_some( + iter: Iterable, + pred: (item: T) => boolean, +): boolean { + for (const item of iter) { + if (pred(item)) { + return true; + } + } + return false; +} + export function nonNull, U>( value: T | null | undefined, ): value is T { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 3d2b01725c6f5..f0009601f1d62 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -7,6 +7,7 @@ import {CompilerError, Effect, ErrorSeverity} from '..'; import { + DeclarationId, GeneratedSource, Identifier, IdentifierId, @@ -82,7 +83,7 @@ type ManualMemoBlockState = { * } else { ... } * ``` */ - decls: Set; + decls: Set; /* * normalized depslist from useMemo/useCallback @@ -204,7 +205,7 @@ function compareDeps( function validateInferredDep( dep: ReactiveScopeDependency, temporaries: Map, - declsWithinMemoBlock: Set, + declsWithinMemoBlock: Set, validDepsInMemoBlock: Array, errorState: CompilerError, memoLocation: SourceLocation, @@ -240,7 +241,7 @@ function validateInferredDep( for (const decl of declsWithinMemoBlock) { if ( normalizedDep.root.kind === 'NamedLocal' && - decl === normalizedDep.root.value.identifier.id + decl === normalizedDep.root.value.identifier.declarationId ) { return; } @@ -323,7 +324,9 @@ class Visitor extends ReactiveFunctionVisitor { const dep = collectMaybeMemoDependencies(value, this.temporaries); if (value.kind === 'StoreLocal' || value.kind === 'StoreContext') { const storeTarget = value.lvalue.place; - state.manualMemoState?.decls.add(storeTarget.identifier.id); + state.manualMemoState?.decls.add( + storeTarget.identifier.declarationId, + ); if (storeTarget.identifier.name?.kind === 'named' && dep == null) { const dep: ManualMemoDependency = { root: { @@ -343,15 +346,14 @@ class Visitor extends ReactiveFunctionVisitor { recordTemporaries(instr: ReactiveInstruction, state: VisitorState): void { const temporaries = this.temporaries; - const {value} = instr; - const lvalId = instr.lvalue?.identifier.id; + const {lvalue, value} = instr; + const lvalId = lvalue?.identifier.id; if (lvalId != null && temporaries.has(lvalId)) { return; } - const isNamedLocal = - lvalId != null && instr.lvalue?.identifier.name?.kind === 'named'; - if (isNamedLocal && state.manualMemoState != null) { - state.manualMemoState.decls.add(lvalId); + const isNamedLocal = lvalue?.identifier.name?.kind === 'named'; + if (lvalue !== null && isNamedLocal && state.manualMemoState != null) { + state.manualMemoState.decls.add(lvalue.identifier.declarationId); } const maybeDep = this.recordDepsInValue(value, state); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-repro-invalid-mutable-range-destructured-prop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-repro-invalid-mutable-range-destructured-prop.expect.md new file mode 100644 index 0000000000000..d56f7a98ad1ad --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-repro-invalid-mutable-range-destructured-prop.expect.md @@ -0,0 +1,90 @@ + +## Input + +```javascript +import {fbt} from 'fbt'; +import {useMemo} from 'react'; +import {ValidateMemoization} from 'shared-runtime'; + +function Component({data}) { + const el = useMemo( + () => ( + + {data.name ?? ''} + + ), + [data.name] + ); + return ; +} + +const props1 = {data: {name: 'Mike'}}; +const props2 = {data: {name: 'Mofei'}}; +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [props1], + sequentialRenders: [props1, props2, props2, props1, {...props1}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { fbt } from "fbt"; +import { useMemo } from "react"; +import { ValidateMemoization } from "shared-runtime"; + +function Component(t0) { + const $ = _c(7); + const { data } = t0; + let t1; + let t2; + if ($[0] !== data.name) { + t2 = fbt._("{name}", [fbt._param("name", data.name ?? "")], { + hk: "csQUH", + }); + $[0] = data.name; + $[1] = t2; + } else { + t2 = $[1]; + } + t1 = t2; + const el = t1; + let t3; + if ($[2] !== data.name) { + t3 = [data.name]; + $[2] = data.name; + $[3] = t3; + } else { + t3 = $[3]; + } + let t4; + if ($[4] !== t3 || $[5] !== el) { + t4 = ; + $[4] = t3; + $[5] = el; + $[6] = t4; + } else { + t4 = $[6]; + } + return t4; +} + +const props1 = { data: { name: "Mike" } }; +const props2 = { data: { name: "Mofei" } }; +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [props1], + sequentialRenders: [props1, props2, props2, props1, { ...props1 }], +}; + +``` + +### Eval output +(kind: ok)
{"inputs":["Mike"],"output":"Mike"}
+
{"inputs":["Mofei"],"output":"Mofei"}
+
{"inputs":["Mofei"],"output":"Mofei"}
+
{"inputs":["Mike"],"output":"Mike"}
+
{"inputs":["Mike"],"output":"Mike"}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-repro-invalid-mutable-range-destructured-prop.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-repro-invalid-mutable-range-destructured-prop.js new file mode 100644 index 0000000000000..a948755bb6701 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-repro-invalid-mutable-range-destructured-prop.js @@ -0,0 +1,23 @@ +import {fbt} from 'fbt'; +import {useMemo} from 'react'; +import {ValidateMemoization} from 'shared-runtime'; + +function Component({data}) { + const el = useMemo( + () => ( + + {data.name ?? ''} + + ), + [data.name] + ); + return ; +} + +const props1 = {data: {name: 'Mike'}}; +const props2 = {data: {name: 'Mofei'}}; +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [props1], + sequentialRenders: [props1, props2, props2, props1, {...props1}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-jsx.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-jsx.expect.md new file mode 100644 index 0000000000000..1d4a4b5d671ed --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-jsx.expect.md @@ -0,0 +1,56 @@ + +## Input + +```javascript +function V0({v1, v2}: V3<{v1: any, v2: V4}>): V12.V11 { + const v5 = v1.v6?.v7; + return ( + + {v5 != null ? ( + + + + ) : ( + + )} + + ); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function V0(t0) { + const $ = _c(4); + const { v1, v2 } = t0; + const v5 = v1.v6?.v7; + let t1; + if ($[0] !== v5 || $[1] !== v1 || $[2] !== v2) { + t1 = ( + + {v5 != null ? ( + + + + ) : ( + + )} + + ); + $[0] = v5; + $[1] = v1; + $[2] = v2; + $[3] = t1; + } else { + t1 = $[3]; + } + return t1; +} + +``` + +### 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/repro-propagate-type-of-ternary-jsx.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-jsx.js new file mode 100644 index 0000000000000..476ec9e35ac78 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-jsx.js @@ -0,0 +1,14 @@ +function V0({v1, v2}: V3<{v1: any, v2: V4}>): V12.V11 { + const v5 = v1.v6?.v7; + return ( + + {v5 != null ? ( + + + + ) : ( + + )} + + ); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.expect.md new file mode 100644 index 0000000000000..609231ca8dab4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.expect.md @@ -0,0 +1,72 @@ + +## Input + +```javascript +function V0({v1}: V2<{v1?: V3}>): V2b.V2a { + const v4 = v5(V6.v7({v8: V9.va})); + const vb = ( + + gmhubcw + {v1 === V3.V13 ? ( + + iawyneijcgamsfgrrjyvhjrrqvzexxwenxqoknnilmfloafyvnvkqbssqnxnexqvtcpvjysaiovjxyqrorqskfph + + ) : v16.v17('pyorztRC]EJzVuP^e') ? ( + + goprinbjmmjhfserfuqyluxcewpyjihektogc + + ) : ( + + yejarlvudihqdrdgpvahovggdnmgnueedxpbwbkdvvkdhqwrtoiual + + )} + hflmn + + ); + return vb; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function V0(t0) { + const $ = _c(2); + const { v1 } = t0; + v5(V6.v7({ v8: V9.va })); + let t1; + if ($[0] !== v1) { + t1 = ( + + gmhubcw + {v1 === V3.V13 ? ( + + iawyneijcgamsfgrrjyvhjrrqvzexxwenxqoknnilmfloafyvnvkqbssqnxnexqvtcpvjysaiovjxyqrorqskfph + + ) : v16.v17("pyorztRC]EJzVuP^e") ? ( + + goprinbjmmjhfserfuqyluxcewpyjihektogc + + ) : ( + + yejarlvudihqdrdgpvahovggdnmgnueedxpbwbkdvvkdhqwrtoiual + + )} + hflmn + + ); + $[0] = v1; + $[1] = t1; + } else { + t1 = $[1]; + } + const vb = t1; + return vb; +} + +``` + +### 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/repro-propagate-type-of-ternary-nested.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.js new file mode 100644 index 0000000000000..74822160ae1a1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.js @@ -0,0 +1,23 @@ +function V0({v1}: V2<{v1?: V3}>): V2b.V2a { + const v4 = v5(V6.v7({v8: V9.va})); + const vb = ( + + gmhubcw + {v1 === V3.V13 ? ( + + iawyneijcgamsfgrrjyvhjrrqvzexxwenxqoknnilmfloafyvnvkqbssqnxnexqvtcpvjysaiovjxyqrorqskfph + + ) : v16.v17('pyorztRC]EJzVuP^e') ? ( + + goprinbjmmjhfserfuqyluxcewpyjihektogc + + ) : ( + + yejarlvudihqdrdgpvahovggdnmgnueedxpbwbkdvvkdhqwrtoiual + + )} + hflmn + + ); + return vb; +} diff --git a/compiler/scripts/anonymize.js b/compiler/scripts/anonymize.js new file mode 100644 index 0000000000000..278e582c101da --- /dev/null +++ b/compiler/scripts/anonymize.js @@ -0,0 +1,250 @@ +/** + * 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'; + +const fs = require('fs'); +const HermesParser = require('hermes-parser'); +const BabelParser = require('@babel/parser'); +const BabelCore = require('@babel/core'); +const invariant = require('invariant'); +const {argv, stdin} = require('process'); +const prettier = require('prettier'); +const {JSXText} = require('hermes-parser/dist/generated/ESTreeVisitorKeys'); + +function runPlugin(text, file, language) { + let ast; + if (language === 'flow') { + ast = HermesParser.parse(text, { + babel: true, + flow: 'all', + sourceFilename: file, + sourceType: 'module', + enableExperimentalComponentSyntax: true, + }); + } else { + ast = BabelParser.parse(text, { + sourceFilename: file, + plugins: ['typescript', 'jsx'], + sourceType: 'module', + }); + } + const result = BabelCore.transformFromAstSync(ast, text, { + ast: false, + filename: file, + highlightCode: false, + retainLines: true, + plugins: [[AnonymizePlugin]], + sourceType: 'module', + configFile: false, + babelrc: false, + }); + invariant( + result?.code != null, + `Expected BabelPluginReactForget to codegen successfully, got: ${result}` + ); + return result.code; +} + +async function format(code, language) { + return await prettier.format(code, { + semi: true, + parser: language === 'typescript' ? 'babel-ts' : 'flow', + }); +} + +const TAG_NAMES = new Set([ + 'a', + 'body', + 'button', + 'div', + 'form', + 'head', + 'html', + 'input', + 'label', + 'select', + 'span', + 'textarea', + + // property/attribute names + 'value', + 'checked', + 'onClick', + 'onSubmit', + 'name', +]); + +const BUILTIN_HOOKS = new Set([ + 'useContext', + 'useEffect', + 'useInsertionEffect', + 'useLayoutEffect', + 'useReducer', + 'useState', +]); + +const GLOBALS = new Set([ + 'String', + 'Object', + 'Function', + 'Number', + 'RegExp', + 'Date', + 'Error', + 'Function', + 'TypeError', + 'RangeError', + 'ReferenceError', + 'SyntaxError', + 'URIError', + 'EvalError', + 'Boolean', + 'DataView', + 'Float32Array', + 'Float64Array', + 'Int8Array', + 'Int16Array', + 'Int32Array', + 'Map', + 'Set', + 'WeakMap', + 'Uint8Array', + 'Uint8ClampedArray', + 'Uint16Array', + 'Uint32Array', + 'ArrayBuffer', + 'JSON', + 'parseFloat', + 'parseInt', + 'console', + 'isNaN', + 'eval', + 'isFinite', + 'encodeURI', + 'decodeURI', + 'encodeURIComponent', + 'decodeURIComponent', + + // common method/property names of globals + 'map', + 'push', + 'at', + 'filter', + 'slice', + 'splice', + 'add', + 'get', + 'set', + 'has', + 'size', + 'length', + 'toString', +]); + +function AnonymizePlugin(_babel) { + let index = 0; + const identifiers = new Map(); + const literals = new Map(); + return { + name: 'anonymize', + visitor: { + JSXNamespacedName(path) { + throw error('TODO: handle JSXNamedspacedName'); + }, + JSXIdentifier(path) { + const name = path.node.name; + if (TAG_NAMES.has(name)) { + return; + } + let nextName = identifiers.get(name); + if (nextName == null) { + const isCapitalized = + name.slice(0, 1).toUpperCase() === name.slice(0, 1); + nextName = isCapitalized + ? `Component${(index++).toString(16).toUpperCase()}` + : `c${(index++).toString(16)}`; + identifiers.set(name, nextName); + } + path.node.name = nextName; + }, + Identifier(path) { + const name = path.node.name; + if (BUILTIN_HOOKS.has(name) || GLOBALS.has(name)) { + return; + } + let nextName = identifiers.get(name); + if (nextName == null) { + const isCapitalized = + name.slice(0, 1).toUpperCase() === name.slice(0, 1); + const prefix = isCapitalized ? 'V' : 'v'; + nextName = `${prefix}${(index++).toString(16)}`; + if (name.startsWith('use')) { + nextName = + 'use' + nextName.slice(0, 1).toUpperCase() + nextName.slice(1); + } + identifiers.set(name, nextName); + } + path.node.name = nextName; + }, + JSXText(path) { + const value = path.node.value; + let nextValue = literals.get(value); + if (nextValue == null) { + let string = ''; + while (string.length < value.length) { + string += String.fromCharCode(Math.round(Math.random() * 25) + 97); + } + nextValue = string; + literals.set(value, nextValue); + } + path.node.value = nextValue; + }, + StringLiteral(path) { + const value = path.node.value; + let nextValue = literals.get(value); + if (nextValue == null) { + let string = ''; + while (string.length < value.length) { + string += String.fromCharCode(Math.round(Math.random() * 58) + 65); + } + nextValue = string; + literals.set(value, nextValue); + } + path.node.value = nextValue; + }, + NumericLiteral(path) { + const value = path.node.value; + let nextValue = literals.get(value); + if (nextValue == null) { + nextValue = Number.isInteger(value) + ? Math.round(Math.random() * Number.MAX_SAFE_INTEGER) + : Math.random() * Number.MAX_VALUE; + literals.set(value, nextValue); + } + path.node.value = nextValue; + }, + }, + }; +} + +let file; +let text; +if (argv.length >= 3) { + file = argv[2]; + text = fs.readFileSync(file, 'utf8'); +} else { + // read from stdin + file = 'stdin.js'; + text = fs.readFileSync(stdin.fd, 'utf8'); +} +const language = + file.endsWith('.ts') || file.endsWith('.tsx') ? 'typescript' : 'flow'; +const result = runPlugin(text, file, language); +format(result, language).then(formatted => { + console.log(formatted); +}); From 9f0508fcf47026cdc58e49a15e5865b66a24480f Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 6 Aug 2024 15:04:30 -0400 Subject: [PATCH 10/13] [final] Actually publish packages to npm ghstack-source-id: 78a358a81726f1e9898be7f46de23e80005b5bfd Pull Request resolved: https://github.com/facebook/react/pull/30618 --- .github/workflows/compiler_prereleases.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/compiler_prereleases.yml b/.github/workflows/compiler_prereleases.yml index a0cdea4112eae..25ad6adde8861 100644 --- a/.github/workflows/compiler_prereleases.yml +++ b/.github/workflows/compiler_prereleases.yml @@ -46,6 +46,7 @@ jobs: path: "**/node_modules" key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - - run: | + - name: Publish packages to npm + run: | cp ./scripts/release/ci-npmrc ~/.npmrc - scripts/release/publish.js --ci --tags ${{ inputs.dist_tag }} + scripts/release/publish.js --frfr --ci --tags ${{ inputs.dist_tag }} From 030d83bab402723cbcb28d48889b9bd83ee3914a Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 6 Aug 2024 15:30:47 -0400 Subject: [PATCH 11/13] [ci] Fix dist-tag command ghstack-source-id: bdbcc12b2815d00d790397c6a6702c7f6e1564e0 Pull Request resolved: https://github.com/facebook/react/pull/30619 --- compiler/scripts/release/publish.js | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/compiler/scripts/release/publish.js b/compiler/scripts/release/publish.js index 4d33474e0521e..1b0b7377b2ccf 100755 --- a/compiler/scripts/release/publish.js +++ b/compiler/scripts/release/publish.js @@ -180,22 +180,17 @@ async function main() { spinner.start('Pushing tags to npm'); for (const tag of argv.tags) { try { - let opts; + let opts = ['dist-tag', 'add', `${pkgName}@${newVersion}`, tag]; if (otp != null) { - opts = [ - 'dist-tag', - 'add', - `${pkgName}@${newVersion}`, - tag, - `--otp=${otp}`, - ]; - } else { - opts = ['dist-tag', 'add', `${pkgName}@${newVersion}`, tag]; + opts.push(`--otp=${otp}`); } if (argv.debug === true) { spinner.info(`dry-run: npm ${opts.join(' ')}`); } else { - await spawnHelper('npm', opts); + await spawnHelper('npm', opts, { + cwd: pkgDir, + stdio: 'inherit', + }); } } catch (e) { spinner.fail(e.toString()); From 838da52d67a42295e05aad8d03f962129d9d88ff Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 6 Aug 2024 17:15:56 -0400 Subject: [PATCH 12/13] [ci] Fix incorrect tags being pushed for compiler releases ghstack-source-id: 812e49333ce19c3d13adb6cc87154fb83d7639b0 Pull Request resolved: https://github.com/facebook/react/pull/30620 --- compiler/scripts/release/publish.js | 45 +++++++++++++++-------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/compiler/scripts/release/publish.js b/compiler/scripts/release/publish.js index 1b0b7377b2ccf..3262cbabd1509 100755 --- a/compiler/scripts/release/publish.js +++ b/compiler/scripts/release/publish.js @@ -61,9 +61,8 @@ async function main() { }) .option('tags', { description: 'Tags to publish to npm', - type: 'choices', - choices: ['experimental'], - default: ['experimental'], + type: 'string', + default: 'experimental', }) .help('help') .parseSync(); @@ -178,27 +177,29 @@ async function main() { spinner.succeed(`Successfully published ${pkgName} to npm`); spinner.start('Pushing tags to npm'); - for (const tag of argv.tags) { - try { - let opts = ['dist-tag', 'add', `${pkgName}@${newVersion}`, tag]; - if (otp != null) { - opts.push(`--otp=${otp}`); + if (typeof argv.tags === 'string') { + for (const tag of argv.tags.split(',')) { + try { + let opts = ['dist-tag', 'add', `${pkgName}@${newVersion}`, tag]; + if (otp != null) { + opts.push(`--otp=${otp}`); + } + if (argv.debug === true) { + spinner.info(`dry-run: npm ${opts.join(' ')}`); + } else { + await spawnHelper('npm', opts, { + cwd: pkgDir, + stdio: 'inherit', + }); + } + } catch (e) { + spinner.fail(e.toString()); + throw e; } - if (argv.debug === true) { - spinner.info(`dry-run: npm ${opts.join(' ')}`); - } else { - await spawnHelper('npm', opts, { - cwd: pkgDir, - stdio: 'inherit', - }); - } - } catch (e) { - spinner.fail(e.toString()); - throw e; + spinner.succeed( + `Successfully pushed dist-tag ${tag} for ${pkgName} to npm` + ); } - spinner.succeed( - `Successfully pushed dist-tag ${tag} for ${pkgName} to npm` - ); } } From 9eb288e6579333612ed736c4e088669daf90a076 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Tue, 6 Aug 2024 23:47:09 -0700 Subject: [PATCH 13/13] [compiler][ez] Default to using flow suppressions ghstack-source-id: b6592650e08a1b07b6ccaad025bc4b81dd61d442 Pull Request resolved: https://github.com/facebook/react/pull/30622 --- .../babel-plugin-react-compiler/src/Entrypoint/Options.ts | 2 +- .../eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts index 01aa4deb8859a..722c62461d813 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts @@ -194,7 +194,7 @@ export const defaultOptions: PluginOptions = { noEmit: false, runtimeModule: null, eslintSuppressionRules: null, - flowSuppressions: false, + flowSuppressions: true, ignoreUseNoForget: false, sources: filename => { return filename.indexOf('node_modules') === -1; diff --git a/compiler/packages/eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts b/compiler/packages/eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts index 359db08cd7085..2fe34a0b7ff2b 100644 --- a/compiler/packages/eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts +++ b/compiler/packages/eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts @@ -101,6 +101,8 @@ function makeSuggestions( const COMPILER_OPTIONS: Partial = { noEmit: true, panicThreshold: 'none', + // Don't emit errors on Flow suppressions--Flow already gave a signal + flowSuppressions: false, }; const rule: Rule.RuleModule = {