From 90cf7bba0843884615352cf904091078cf9363f0 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Thu, 13 Mar 2025 11:52:25 -0400 Subject: [PATCH] [compiler][ez] Only fail gating hoisting check for referenced identifiers Reduce false positive bailouts by using the same `isReferencedIdentifier` logic that the compiler also uses for determining context variables and a function's own hoisted declarations. Details: Previously, we counted every babel identifier as a reference. This is problematic because babel counts most string symbols as an identifier. ```js print(x); // x is an identifier as expected obj.x // x is.. also an identifier here {x: 2} // x is also an identifier here ``` This PR adds a check for `isReferencedIdentifier`. Note that only non-lval references pass this check. This should be fine as we don't need to hoist function declarations before writes to the same lvalue (which should error in strict mode anyways) ```js print(x); // isReferencedIdentifier(x) -> true obj.x // isReferencedIdentifier(x) -> false {x: 2} // isReferencedIdentifier(x) -> false x = 2 // isReferencedIdentifier(x) -> false ``` --- .../src/Entrypoint/Program.ts | 2 +- ...nreferenced-identifier-collision.expect.md | 60 +++++++++++++++++++ ...ting-nonreferenced-identifier-collision.js | 16 +++++ 3 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/gating-nonreferenced-identifier-collision.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/gating-nonreferenced-identifier-collision.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index 787d9e7047..d69dee527d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -1143,7 +1143,7 @@ function checkFunctionReferencedBeforeDeclarationAtTopLevel( * A null scope means there's no function scope, which means we're at the * top level scope. */ - if (scope === null) { + if (scope === null && id.isReferencedIdentifier()) { errors.pushErrorDetail( new CompilerErrorDetail({ reason: `Encountered a function used before its declaration, which breaks Forget's gating codegen due to hoisting`, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/gating-nonreferenced-identifier-collision.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/gating-nonreferenced-identifier-collision.expect.md new file mode 100644 index 0000000000..a8f2a8dc58 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/gating-nonreferenced-identifier-collision.expect.md @@ -0,0 +1,60 @@ + +## Input + +```javascript +// @gating +import {identity, useHook as useRenamed} from 'shared-runtime'; +const _ = { + useHook: () => {}, +}; +identity(_.useHook); + +function useHook() { + useRenamed(); + return
hello world!
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{}], +}; + +``` + +## Code + +```javascript +import { isForgetEnabled_Fixtures } from "ReactForgetFeatureFlag"; +import { c as _c } from "react/compiler-runtime"; // @gating +import { identity, useHook as useRenamed } from "shared-runtime"; +const _ = { + useHook: isForgetEnabled_Fixtures() ? () => {} : () => {}, +}; +identity(_.useHook); +const useHook = isForgetEnabled_Fixtures() + ? function useHook() { + const $ = _c(1); + useRenamed(); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 =
hello world!
; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; + } + : function useHook() { + useRenamed(); + return
hello world!
; + }; + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
hello world!
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/gating-nonreferenced-identifier-collision.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/gating-nonreferenced-identifier-collision.js new file mode 100644 index 0000000000..f5d557978b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/gating-nonreferenced-identifier-collision.js @@ -0,0 +1,16 @@ +// @gating +import {identity, useHook as useRenamed} from 'shared-runtime'; +const _ = { + useHook: () => {}, +}; +identity(_.useHook); + +function useHook() { + useRenamed(); + return
hello world!
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{}], +};