From c93d4b34b2e048fcf3fba8be7c0560e84ae72c07 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 21 May 2024 11:24:10 +0100 Subject: [PATCH] compiler: only resolve globals and react imports Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. ghstack-source-id: 0c26e76d41512db0976de16bd33475723d6b99f6 Pull Request resolved: https://github.com/facebook/react/pull/29190 --- .../src/HIR/Environment.ts | 46 ++++-- .../src/HIR/PrintHIR.ts | 33 +++- ...and-local-variables-with-default.expect.md | 89 +++++----- ...bals-dont-resolve-local-useState.expect.md | 152 ++++++++++++++++++ .../globals-dont-resolve-local-useState.js | 44 +++++ .../compiler/useEffect-snap-test.expect.md | 65 ++++++++ .../fixtures/compiler/useEffect-snap-test.js | 15 ++ compiler/packages/snap/src/compiler.ts | 1 + 8 files changed, 392 insertions(+), 53 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index c75b0a15b037b..cda0aeded666b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -11,7 +11,6 @@ import { fromZodError } from "zod-validation-error"; import { CompilerError } from "../CompilerError"; import { Logger } from "../Entrypoint"; import { Err, Ok, Result } from "../Utils/Result"; -import { log } from "../Utils/logger"; import { DEFAULT_GLOBALS, DEFAULT_SHAPES, @@ -320,6 +319,8 @@ const EnvironmentConfigSchema = z.object({ */ throwUnknownException__testonly: z.boolean().default(false), + enableSharedRuntime__testonly: z.boolean().default(false), + /** * Enables deps of a function epxression to be treated as conditional. This * makes sure we don't load a dep when it's a property (to check if it has @@ -514,7 +515,6 @@ export class Environment { getGlobalDeclaration(binding: NonLocalBinding): Global | null { const name = binding.name; - let resolvedName = name; if (this.config.hookPattern != null) { const match = new RegExp(this.config.hookPattern).exec(name); @@ -523,21 +523,45 @@ export class Environment { typeof match[1] === "string" && isHookName(match[1]) ) { - resolvedName = match[1]; + const resolvedName = match[1]; + return this.#globals.get(resolvedName) ?? this.#getCustomHookType(); } } - let resolvedGlobal: Global | null = this.#globals.get(resolvedName) ?? null; - if (resolvedGlobal === null) { - // Hack, since we don't track module level declarations and imports - if (isHookName(resolvedName)) { - return this.#getCustomHookType(); - } else { - log(() => `Undefined global \`${name}\``); + let global: Global | null = null; + switch (binding.kind) { + case "ModuleLocal": { + // don't resolve module locals + break; + } + case "Global": { + global = this.#globals.get(name) ?? null; + break; + } + case "ImportDefault": + case "ImportNamespace": + case "ImportSpecifier": { + if ( + binding.module.toLowerCase() === "react" || + binding.module.toLowerCase() === "react-dom" || + (this.config.enableSharedRuntime__testonly && + binding.module === "shared-runtime") + ) { + // only resolve imports to modules we know about + global = this.#globals.get(name) ?? null; + } + break; } } - return resolvedGlobal; + if (global === null && isHookName(name)) { + /** + * Type inference relies on all hooks being resolved as such, so if we don't have + * a global declaration and its a hook name, return the default custom hook type. + */ + return this.#getCustomHookType(); + } + return global; } getPropertyType( diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 126e78f8002fc..b0a6fb1635b00 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -588,7 +588,38 @@ export function printInstructionValue(instrValue: ReactiveValue): string { break; } case "LoadGlobal": { - value = `LoadGlobal ${instrValue.binding.name}`; + switch (instrValue.binding.kind) { + case "Global": { + value = `LoadGlobal(global) ${instrValue.binding.name}`; + break; + } + case "ModuleLocal": { + value = `LoadGlobal(module) ${instrValue.binding.name}`; + break; + } + case "ImportDefault": { + value = `LoadGlobal import ${instrValue.binding.name} from '${instrValue.binding.module}'`; + break; + } + case "ImportNamespace": { + value = `LoadGlobal import * as ${instrValue.binding.name} from '${instrValue.binding.module}'`; + break; + } + case "ImportSpecifier": { + if (instrValue.binding.imported !== instrValue.binding.name) { + value = `LoadGlobal import { ${instrValue.binding.imported} as ${instrValue.binding.name} } from '${instrValue.binding.module}'`; + } else { + value = `LoadGlobal import { ${instrValue.binding.name} } from '${instrValue.binding.module}'`; + } + break; + } + default: { + assertExhaustive( + instrValue.binding, + `Unexpected binding kind \`${(instrValue.binding as any).kind}\`` + ); + } + } break; } case "StoreGlobal": { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md index 7fb336c467607..e93fbb3b45fcf 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md @@ -56,71 +56,78 @@ function useFragment(_arg1, _arg2) { } function Component(props) { - const $ = _c(14); - const post = useFragment(graphql`...`, props.post); + const $ = _c(15); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = graphql`...`; + $[0] = t0; + } else { + t0 = $[0]; + } + const post = useFragment(t0, props.post); let media; let allUrls; let onClick; - if ($[0] !== post) { + if ($[1] !== post) { allUrls = []; - const { media: t0, comments: t1, urls: t2 } = post; - media = t0 === undefined ? null : t0; - let t3; - if ($[4] !== t1) { - t3 = t1 === undefined ? [] : t1; - $[4] = t1; - $[5] = t3; - } else { - t3 = $[5]; - } - const comments = t3; + const { media: t1, comments: t2, urls: t3 } = post; + media = t1 === undefined ? null : t1; let t4; - if ($[6] !== t2) { + if ($[5] !== t2) { t4 = t2 === undefined ? [] : t2; - $[6] = t2; - $[7] = t4; + $[5] = t2; + $[6] = t4; } else { - t4 = $[7]; + t4 = $[6]; } - const urls = t4; + const comments = t4; let t5; - if ($[8] !== comments.length) { - t5 = (e) => { + if ($[7] !== t3) { + t5 = t3 === undefined ? [] : t3; + $[7] = t3; + $[8] = t5; + } else { + t5 = $[8]; + } + const urls = t5; + let t6; + if ($[9] !== comments.length) { + t6 = (e) => { if (!comments.length) { return; } console.log(comments.length); }; - $[8] = comments.length; - $[9] = t5; + $[9] = comments.length; + $[10] = t6; } else { - t5 = $[9]; + t6 = $[10]; } - onClick = t5; + onClick = t6; allUrls.push(...urls); - $[0] = post; - $[1] = media; - $[2] = allUrls; - $[3] = onClick; + $[1] = post; + $[2] = media; + $[3] = allUrls; + $[4] = onClick; } else { - media = $[1]; - allUrls = $[2]; - onClick = $[3]; + media = $[2]; + allUrls = $[3]; + onClick = $[4]; } - let t0; - if ($[10] !== media || $[11] !== allUrls || $[12] !== onClick) { - t0 = ; - $[10] = media; - $[11] = allUrls; - $[12] = onClick; - $[13] = t0; + let t1; + if ($[11] !== media || $[12] !== allUrls || $[13] !== onClick) { + t1 = ; + $[11] = media; + $[12] = allUrls; + $[13] = onClick; + $[14] = t1; } else { - t0 = $[13]; + t1 = $[14]; } - return t0; + return t1; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.expect.md new file mode 100644 index 0000000000000..3038c80dc8e72 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.expect.md @@ -0,0 +1,152 @@ + +## Input + +```javascript +import { useState as _useState, useCallback, useEffect } from "react"; +import { ValidateMemoization } from "shared-runtime"; + +function useState(value) { + "use no memo"; // opt-out because we want to force resetting the setState function + const [state, _setState] = _useState(value); + const setState = useCallback( + (...args) => { + console.log(...args); + return _setState(...args); + }, + // explicitly reset the callback when state changes + [state] + ); + if (setState.state === undefined) { + setState.state = state; + } + return [state, setState]; +} + +function Component() { + const [state, setState] = useState("hello"); + console.log(state, setState.state); + + const callback = useCallback(() => { + setState("goodbye"); + }, [setState]); + + useEffect(() => { + callback(); + }, []); + + return ( + <> + + {state} + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useState as _useState, useCallback, useEffect } from "react"; +import { ValidateMemoization } from "shared-runtime"; + +function useState(value) { + "use no memo"; // opt-out because we want to force resetting the setState function + const [state, _setState] = _useState(value); + const setState = useCallback( + (...args) => { + console.log(...args); + return _setState(...args); + }, + // explicitly reset the callback when state changes + [state] + ); + if (setState.state === undefined) { + setState.state = state; + } + return [state, setState]; +} + +function Component() { + const $ = _c(13); + const [state, setState] = useState("hello"); + console.log(state, setState.state); + let t0; + if ($[0] !== setState) { + t0 = () => { + setState("goodbye"); + }; + $[0] = setState; + $[1] = t0; + } else { + t0 = $[1]; + } + const callback = t0; + let t1; + if ($[2] !== callback) { + t1 = () => { + callback(); + }; + $[2] = callback; + $[3] = t1; + } else { + t1 = $[3]; + } + let t2; + if ($[4] === Symbol.for("react.memo_cache_sentinel")) { + t2 = []; + $[4] = t2; + } else { + t2 = $[4]; + } + useEffect(t1, t2); + let t3; + if ($[5] !== setState) { + t3 = [setState]; + $[5] = setState; + $[6] = t3; + } else { + t3 = $[6]; + } + let t4; + if ($[7] !== t3 || $[8] !== callback) { + t4 = ; + $[7] = t3; + $[8] = callback; + $[9] = t4; + } else { + t4 = $[9]; + } + let t5; + if ($[10] !== t4 || $[11] !== state) { + t5 = ( + <> + {t4} + {state} + + ); + $[10] = t4; + $[11] = state; + $[12] = t5; + } else { + t5 = $[12]; + } + return t5; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: exception) Output identity changed but inputs did not +logs: ['hello','hello','goodbye','hello','hello','goodbye'] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.js new file mode 100644 index 0000000000000..cdb55bb7799f5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.js @@ -0,0 +1,44 @@ +import { useState as _useState, useCallback, useEffect } from "react"; +import { ValidateMemoization } from "shared-runtime"; + +function useState(value) { + "use no memo"; // opt-out because we want to force resetting the setState function + const [state, _setState] = _useState(value); + const setState = useCallback( + (...args) => { + console.log(...args); + return _setState(...args); + }, + // explicitly reset the callback when state changes + [state] + ); + if (setState.state === undefined) { + setState.state = state; + } + return [state, setState]; +} + +function Component() { + const [state, setState] = useState("hello"); + console.log(state, setState.state); + + const callback = useCallback(() => { + setState("goodbye"); + }, [setState]); + + useEffect(() => { + callback(); + }, []); + + return ( + <> + + {state} + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.expect.md new file mode 100644 index 0000000000000..930302faec2ab --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +import { useEffect, useState } from "react"; + +function Component() { + const [state, setState] = useState("hello"); + useEffect(() => { + setState("goodbye"); + }, []); + + return
{state}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useEffect, useState } from "react"; + +function Component() { + const $ = _c(4); + const [state, setState] = useState("hello"); + let t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + setState("goodbye"); + }; + t1 = []; + $[0] = t0; + $[1] = t1; + } else { + t0 = $[0]; + t1 = $[1]; + } + useEffect(t0, t1); + let t2; + if ($[2] !== state) { + t2 =
{state}
; + $[2] = state; + $[3] = t2; + } else { + t2 = $[3]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
goodbye
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.js new file mode 100644 index 0000000000000..fdc695eba5b0b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.js @@ -0,0 +1,15 @@ +import { useEffect, useState } from "react"; + +function Component() { + const [state, setState] = useState("hello"); + useEffect(() => { + setState("goodbye"); + }, []); + + return
{state}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; diff --git a/compiler/packages/snap/src/compiler.ts b/compiler/packages/snap/src/compiler.ts index 786fc7278a2bf..69b909645ae6a 100644 --- a/compiler/packages/snap/src/compiler.ts +++ b/compiler/packages/snap/src/compiler.ts @@ -170,6 +170,7 @@ function makePluginOptions( enableEmitInstrumentForget, enableEmitHookGuards, assertValidMutableRanges: true, + enableSharedRuntime__testonly: true, hookPattern, validatePreserveExistingMemoizationGuarantees, },