From 909ed63e0adc162a95a4704d3ed07a956dcf9cd1 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Mon, 16 Dec 2024 12:32:07 -0500 Subject: [PATCH 1/7] Clean up context access profiling experiment (#31806) We introduced the `unstable_useContextWithBailout` API to run compiler based experiments. This API was designed to be an experiment proxy for alternative approaches which would be heavier to implement. The experiment turned out to be inconclusive. Since most of our performance critical usage is already optimized, we weren't able to find a clear win with this approach. Since we don't have further plans for this API, let's clean it up. --- .../react-debug-tools/src/ReactDebugHooks.js | 30 +-- .../react-reconciler/src/ReactFiberHooks.js | 115 +--------- .../src/ReactFiberNewContext.js | 137 +---------- .../src/ReactInternalTypes.js | 27 +-- .../__tests__/ReactContextWithBailout-test.js | 217 ------------------ packages/react/index.fb.js | 1 - packages/react/src/ReactClient.js | 2 - packages/react/src/ReactHooks.js | 25 -- packages/shared/ReactFeatureFlags.js | 3 - .../forks/ReactFeatureFlags.native-fb.js | 1 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../shared/forks/ReactFeatureFlags.www.js | 2 - scripts/error-codes/codes.json | 1 - 15 files changed, 16 insertions(+), 548 deletions(-) delete mode 100644 packages/react-reconciler/src/__tests__/ReactContextWithBailout-test.js diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index eccaa6a67b..d2cea81f57 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -20,7 +20,6 @@ import type { Dependencies, Fiber, Dispatcher as DispatcherType, - ContextDependencyWithSelect, } from 'react-reconciler/src/ReactInternalTypes'; import type {TransitionStatus} from 'react-reconciler/src/ReactFiberConfig'; @@ -76,13 +75,6 @@ function getPrimitiveStackCache(): Map> { try { // Use all hooks here to add them to the hook log. Dispatcher.useContext(({_currentValue: null}: any)); - if (typeof Dispatcher.unstable_useContextWithBailout === 'function') { - // This type check is for Flow only. - Dispatcher.unstable_useContextWithBailout( - ({_currentValue: null}: any), - null, - ); - } Dispatcher.useState(null); Dispatcher.useReducer((s: mixed, a: mixed) => s, null); Dispatcher.useRef(null); @@ -150,10 +142,7 @@ function getPrimitiveStackCache(): Map> { let currentFiber: null | Fiber = null; let currentHook: null | Hook = null; -let currentContextDependency: - | null - | ContextDependency - | ContextDependencyWithSelect = null; +let currentContextDependency: null | ContextDependency = null; function nextHook(): null | Hook { const hook = currentHook; @@ -274,22 +263,6 @@ function useContext(context: ReactContext): T { return value; } -function unstable_useContextWithBailout( - context: ReactContext, - select: (T => Array) | null, -): T { - const value = readContext(context); - hookLog.push({ - displayName: context.displayName || null, - primitive: 'ContextWithBailout', - stackError: new Error(), - value: value, - debugInfo: null, - dispatcherHookName: 'ContextWithBailout', - }); - return value; -} - function useState( initialState: (() => S) | S, ): [S, Dispatch>] { @@ -764,7 +737,6 @@ const Dispatcher: DispatcherType = { useCacheRefresh, useCallback, useContext, - unstable_useContextWithBailout, useEffect, useImperativeHandle, useDebugValue, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 8affe4804b..771b60cbb0 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -39,12 +39,11 @@ import { enableLazyContextPropagation, enableTransitionTracing, enableUseEffectEventHook, + enableUseResourceEffectHook, enableLegacyCache, debugRenderPhaseSideEffectsForStrictMode, disableLegacyMode, enableNoCloningMemoCache, - enableContextProfiling, - enableUseResourceEffectHook, } from 'shared/ReactFeatureFlags'; import { REACT_CONTEXT_TYPE, @@ -78,11 +77,7 @@ import { ContinuousEventPriority, higherEventPriority, } from './ReactEventPriorities'; -import { - readContext, - readContextAndCompare, - checkIfContextChanged, -} from './ReactFiberNewContext'; +import {readContext, checkIfContextChanged} from './ReactFiberNewContext'; import {HostRoot, CacheComponent, HostComponent} from './ReactWorkTags'; import { LayoutStatic as LayoutStaticEffect, @@ -1111,16 +1106,6 @@ function updateWorkInProgressHook(): Hook { return workInProgressHook; } -function unstable_useContextWithBailout( - context: ReactContext, - select: (T => Array) | null, -): T { - if (select === null) { - return readContext(context); - } - return readContextAndCompare(context, select); -} - function createFunctionComponentUpdateQueue(): FunctionComponentUpdateQueue { return { lastEffect: null, @@ -3958,10 +3943,6 @@ if (enableUseEffectEventHook) { if (enableUseResourceEffectHook) { (ContextOnlyDispatcher: Dispatcher).useResourceEffect = throwInvalidHookError; } -if (enableContextProfiling) { - (ContextOnlyDispatcher: Dispatcher).unstable_useContextWithBailout = - throwInvalidHookError; -} const HooksDispatcherOnMount: Dispatcher = { readContext, @@ -3995,10 +3976,6 @@ if (enableUseEffectEventHook) { if (enableUseResourceEffectHook) { (HooksDispatcherOnMount: Dispatcher).useResourceEffect = mountResourceEffect; } -if (enableContextProfiling) { - (HooksDispatcherOnMount: Dispatcher).unstable_useContextWithBailout = - unstable_useContextWithBailout; -} const HooksDispatcherOnUpdate: Dispatcher = { readContext, @@ -4033,10 +4010,6 @@ if (enableUseResourceEffectHook) { (HooksDispatcherOnUpdate: Dispatcher).useResourceEffect = updateResourceEffect; } -if (enableContextProfiling) { - (HooksDispatcherOnUpdate: Dispatcher).unstable_useContextWithBailout = - unstable_useContextWithBailout; -} const HooksDispatcherOnRerender: Dispatcher = { readContext, @@ -4071,10 +4044,6 @@ if (enableUseResourceEffectHook) { (HooksDispatcherOnRerender: Dispatcher).useResourceEffect = updateResourceEffect; } -if (enableContextProfiling) { - (HooksDispatcherOnRerender: Dispatcher).unstable_useContextWithBailout = - unstable_useContextWithBailout; -} let HooksDispatcherOnMountInDEV: Dispatcher | null = null; let HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher | null = null; @@ -4296,17 +4265,6 @@ if (__DEV__) { ); }; } - if (enableContextProfiling) { - (HooksDispatcherOnMountInDEV: Dispatcher).unstable_useContextWithBailout = - function ( - context: ReactContext, - select: (T => Array) | null, - ): T { - currentHookNameInDev = 'useContext'; - mountHookTypesDev(); - return unstable_useContextWithBailout(context, select); - }; - } HooksDispatcherOnMountWithHookTypesInDEV = { readContext(context: ReactContext): T { @@ -4494,17 +4452,6 @@ if (__DEV__) { ); }; } - if (enableContextProfiling) { - (HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher).unstable_useContextWithBailout = - function ( - context: ReactContext, - select: (T => Array) | null, - ): T { - currentHookNameInDev = 'useContext'; - updateHookTypesDev(); - return unstable_useContextWithBailout(context, select); - }; - } HooksDispatcherOnUpdateInDEV = { readContext(context: ReactContext): T { @@ -4692,17 +4639,6 @@ if (__DEV__) { ); }; } - if (enableContextProfiling) { - (HooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout = - function ( - context: ReactContext, - select: (T => Array) | null, - ): T { - currentHookNameInDev = 'useContext'; - updateHookTypesDev(); - return unstable_useContextWithBailout(context, select); - }; - } HooksDispatcherOnRerenderInDEV = { readContext(context: ReactContext): T { @@ -4890,17 +4826,6 @@ if (__DEV__) { ); }; } - if (enableContextProfiling) { - (HooksDispatcherOnRerenderInDEV: Dispatcher).unstable_useContextWithBailout = - function ( - context: ReactContext, - select: (T => Array) | null, - ): T { - currentHookNameInDev = 'useContext'; - updateHookTypesDev(); - return unstable_useContextWithBailout(context, select); - }; - } InvalidNestedHooksDispatcherOnMountInDEV = { readContext(context: ReactContext): T { @@ -5114,18 +5039,6 @@ if (__DEV__) { ); }; } - if (enableContextProfiling) { - (InvalidNestedHooksDispatcherOnMountInDEV: Dispatcher).unstable_useContextWithBailout = - function ( - context: ReactContext, - select: (T => Array) | null, - ): T { - currentHookNameInDev = 'useContext'; - warnInvalidHookAccess(); - mountHookTypesDev(); - return unstable_useContextWithBailout(context, select); - }; - } InvalidNestedHooksDispatcherOnUpdateInDEV = { readContext(context: ReactContext): T { @@ -5339,18 +5252,6 @@ if (__DEV__) { ); }; } - if (enableContextProfiling) { - (InvalidNestedHooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout = - function ( - context: ReactContext, - select: (T => Array) | null, - ): T { - currentHookNameInDev = 'useContext'; - warnInvalidHookAccess(); - updateHookTypesDev(); - return unstable_useContextWithBailout(context, select); - }; - } InvalidNestedHooksDispatcherOnRerenderInDEV = { readContext(context: ReactContext): T { @@ -5564,16 +5465,4 @@ if (__DEV__) { ); }; } - if (enableContextProfiling) { - (InvalidNestedHooksDispatcherOnRerenderInDEV: Dispatcher).unstable_useContextWithBailout = - function ( - context: ReactContext, - select: (T => Array) | null, - ): T { - currentHookNameInDev = 'useContext'; - warnInvalidHookAccess(); - updateHookTypesDev(); - return unstable_useContextWithBailout(context, select); - }; - } } diff --git a/packages/react-reconciler/src/ReactFiberNewContext.js b/packages/react-reconciler/src/ReactFiberNewContext.js index 190475519e..6a779cfd4a 100644 --- a/packages/react-reconciler/src/ReactFiberNewContext.js +++ b/packages/react-reconciler/src/ReactFiberNewContext.js @@ -12,7 +12,6 @@ import type { Fiber, ContextDependency, Dependencies, - ContextDependencyWithSelect, } from './ReactInternalTypes'; import type {StackCursor} from './ReactFiberStack'; import type {Lanes} from './ReactFiberLane'; @@ -48,8 +47,6 @@ import { enableRenderableContext, } from 'shared/ReactFeatureFlags'; import {getHostTransitionProvider} from './ReactFiberHostContext'; -import isArray from '../../shared/isArray'; -import {enableContextProfiling} from '../../shared/ReactFeatureFlags'; const valueCursor: StackCursor = createCursor(null); @@ -69,10 +66,7 @@ if (__DEV__) { } let currentlyRenderingFiber: Fiber | null = null; -let lastContextDependency: - | ContextDependency - | ContextDependencyWithSelect - | null = null; +let lastContextDependency: ContextDependency | null = null; let isDisallowedContextReadInDEV: boolean = false; @@ -401,23 +395,6 @@ function propagateContextChanges( const context: ReactContext = contexts[i]; // Check if the context matches. if (dependency.context === context) { - if (enableContextProfiling) { - const select = dependency.select; - if (select != null && dependency.lastSelectedValue != null) { - const newValue = isPrimaryRenderer - ? dependency.context._currentValue - : dependency.context._currentValue2; - if ( - !checkIfSelectedContextValuesChanged( - dependency.lastSelectedValue, - select(newValue), - ) - ) { - // Compared value hasn't changed. Bail out early. - continue findContext; - } - } - } // Match! Schedule an update on this fiber. // In the lazy implementation, don't mark a dirty flag on the @@ -657,29 +634,6 @@ function propagateParentContextChanges( workInProgress.flags |= DidPropagateContext; } -function checkIfSelectedContextValuesChanged( - oldComparedValue: Array, - newComparedValue: Array, -): boolean { - // We have an implicit contract that compare functions must return arrays. - // This allows us to compare multiple values in the same context access - // since compiling to additional hook calls regresses perf. - if (isArray(oldComparedValue) && isArray(newComparedValue)) { - if (oldComparedValue.length !== newComparedValue.length) { - return true; - } - - for (let i = 0; i < oldComparedValue.length; i++) { - if (!is(newComparedValue[i], oldComparedValue[i])) { - return true; - } - } - } else { - throw new Error('Compared context values must be arrays'); - } - return false; -} - export function checkIfContextChanged( currentDependencies: Dependencies, ): boolean { @@ -698,23 +652,8 @@ export function checkIfContextChanged( ? context._currentValue : context._currentValue2; const oldValue = dependency.memoizedValue; - if ( - enableContextProfiling && - dependency.select != null && - dependency.lastSelectedValue != null - ) { - if ( - checkIfSelectedContextValuesChanged( - dependency.lastSelectedValue, - dependency.select(newValue), - ) - ) { - return true; - } - } else { - if (!is(newValue, oldValue)) { - return true; - } + if (!is(newValue, oldValue)) { + return true; } dependency = dependency.next; } @@ -747,21 +686,6 @@ export function prepareToReadContext( } } -export function readContextAndCompare( - context: ReactContext, - select: C => Array, -): C { - if (!(enableLazyContextPropagation && enableContextProfiling)) { - throw new Error('Not implemented.'); - } - - return readContextForConsumer_withSelect( - currentlyRenderingFiber, - context, - select, - ); -} - export function readContext(context: ReactContext): T { if (__DEV__) { // This warning would fire if you read context inside a Hook like useMemo. @@ -789,59 +713,10 @@ export function readContextDuringReconciliation( return readContextForConsumer(consumer, context); } -function readContextForConsumer_withSelect( +function readContextForConsumer( consumer: Fiber | null, - context: ReactContext, - select: C => Array, -): C { - const value = isPrimaryRenderer - ? context._currentValue - : context._currentValue2; - - const contextItem = { - context: ((context: any): ReactContext), - memoizedValue: value, - next: null, - select: ((select: any): (context: mixed) => Array), - lastSelectedValue: select(value), - }; - - if (lastContextDependency === null) { - if (consumer === null) { - throw new Error( - 'Context can only be read while React is rendering. ' + - 'In classes, you can read it in the render method or getDerivedStateFromProps. ' + - 'In function components, you can read it directly in the function body, but not ' + - 'inside Hooks like useReducer() or useMemo().', - ); - } - - // This is the first dependency for this component. Create a new list. - lastContextDependency = contextItem; - consumer.dependencies = __DEV__ - ? { - lanes: NoLanes, - firstContext: contextItem, - _debugThenableState: null, - } - : { - lanes: NoLanes, - firstContext: contextItem, - }; - if (enableLazyContextPropagation) { - consumer.flags |= NeedsPropagation; - } - } else { - // Append a new context item. - lastContextDependency = lastContextDependency.next = contextItem; - } - return value; -} - -function readContextForConsumer( - consumer: Fiber | null, - context: ReactContext, -): C { + context: ReactContext, +): T { const value = isPrimaryRenderer ? context._currentValue : context._currentValue2; diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 2ef3a2da4f..afe853202f 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -63,27 +63,18 @@ export type HookType = | 'useFormState' | 'useActionState'; -export type ContextDependency = { - context: ReactContext, - next: ContextDependency | ContextDependencyWithSelect | null, - memoizedValue: C, -}; - -export type ContextDependencyWithSelect = { - context: ReactContext, - next: ContextDependency | ContextDependencyWithSelect | null, - memoizedValue: C, - select: C => Array, - lastSelectedValue: ?Array, +export type ContextDependency = { + context: ReactContext, + next: ContextDependency | null, + memoizedValue: T, + ... }; export type Dependencies = { lanes: Lanes, - firstContext: - | ContextDependency - | ContextDependencyWithSelect - | null, + firstContext: ContextDependency | null, _debugThenableState?: null | ThenableState, // DEV-only + ... }; export type MemoCache = { @@ -401,10 +392,6 @@ export type Dispatcher = { initialArg: I, init?: (I) => S, ): [S, Dispatch], - unstable_useContextWithBailout?: ( - context: ReactContext, - select: (T => Array) | null, - ) => T, useContext(context: ReactContext): T, useRef(initialValue: T): {current: T}, useEffect( diff --git a/packages/react-reconciler/src/__tests__/ReactContextWithBailout-test.js b/packages/react-reconciler/src/__tests__/ReactContextWithBailout-test.js deleted file mode 100644 index c510206148..0000000000 --- a/packages/react-reconciler/src/__tests__/ReactContextWithBailout-test.js +++ /dev/null @@ -1,217 +0,0 @@ -let React; -let ReactNoop; -let Scheduler; -let act; -let assertLog; -let useState; -let useContext; -let unstable_useContextWithBailout; - -describe('ReactContextWithBailout', () => { - beforeEach(() => { - jest.resetModules(); - - React = require('react'); - ReactNoop = require('react-noop-renderer'); - Scheduler = require('scheduler'); - const testUtils = require('internal-test-utils'); - act = testUtils.act; - assertLog = testUtils.assertLog; - useState = React.useState; - useContext = React.useContext; - unstable_useContextWithBailout = React.unstable_useContextWithBailout; - }); - - function Text({text}) { - Scheduler.log(text); - return text; - } - - // @gate enableLazyContextPropagation && enableContextProfiling - test('unstable_useContextWithBailout basic usage', async () => { - const Context = React.createContext(); - - let setContext; - function App() { - const [context, _setContext] = useState({a: 'A0', b: 'B0', c: 'C0'}); - setContext = _setContext; - return ( - - - - ); - } - - // Intermediate parent that bails out. Children will only re-render when the - // context changes. - const Indirection = React.memo(() => { - return ( - <> - A: , B: , C: , AB: - - ); - }); - - function A() { - const {a} = unstable_useContextWithBailout(Context, context => [ - context.a, - ]); - return ; - } - - function B() { - const {b} = unstable_useContextWithBailout(Context, context => [ - context.b, - ]); - return ; - } - - function C() { - const {c} = unstable_useContextWithBailout(Context, context => [ - context.c, - ]); - return ; - } - - function AB() { - const {a, b} = unstable_useContextWithBailout(Context, context => [ - context.a, - context.b, - ]); - return ; - } - - const root = ReactNoop.createRoot(); - await act(async () => { - root.render(); - }); - assertLog(['A0', 'B0', 'C0', 'A0B0']); - expect(root).toMatchRenderedOutput('A: A0, B: B0, C: C0, AB: A0B0'); - - // Update a. Only the A and AB consumer should re-render. - await act(async () => { - setContext({a: 'A1', c: 'C0', b: 'B0'}); - }); - assertLog(['A1', 'A1B0']); - expect(root).toMatchRenderedOutput('A: A1, B: B0, C: C0, AB: A1B0'); - - // Update b. Only the B and AB consumer should re-render. - await act(async () => { - setContext({a: 'A1', b: 'B1', c: 'C0'}); - }); - assertLog(['B1', 'A1B1']); - expect(root).toMatchRenderedOutput('A: A1, B: B1, C: C0, AB: A1B1'); - - // Update c. Only the C consumer should re-render. - await act(async () => { - setContext({a: 'A1', b: 'B1', c: 'C1'}); - }); - assertLog(['C1']); - expect(root).toMatchRenderedOutput('A: A1, B: B1, C: C1, AB: A1B1'); - }); - - // @gate enableLazyContextPropagation && enableContextProfiling - test('unstable_useContextWithBailout and useContext subscribing to same context in same component', async () => { - const Context = React.createContext(); - - let setContext; - function App() { - const [context, _setContext] = useState({a: 0, b: 0, unrelated: 0}); - setContext = _setContext; - return ( - - - - ); - } - - // Intermediate parent that bails out. Children will only re-render when the - // context changes. - const Indirection = React.memo(() => { - return ; - }); - - function Child() { - const {a} = unstable_useContextWithBailout(Context, context => [ - context.a, - ]); - const context = useContext(Context); - return ; - } - - const root = ReactNoop.createRoot(); - await act(async () => { - root.render(); - }); - assertLog(['A: 0, B: 0']); - expect(root).toMatchRenderedOutput('A: 0, B: 0'); - - // Update an unrelated field that isn't used by the component. The context - // attempts to bail out, but the normal context forces an update. - await act(async () => { - setContext({a: 0, b: 0, unrelated: 1}); - }); - assertLog(['A: 0, B: 0']); - expect(root).toMatchRenderedOutput('A: 0, B: 0'); - }); - - // @gate enableLazyContextPropagation && enableContextProfiling - test('unstable_useContextWithBailout and useContext subscribing to different contexts in same component', async () => { - const ContextA = React.createContext(); - const ContextB = React.createContext(); - - let setContextA; - let setContextB; - function App() { - const [a, _setContextA] = useState({a: 0, unrelated: 0}); - const [b, _setContextB] = useState(0); - setContextA = _setContextA; - setContextB = _setContextB; - return ( - - - - - - ); - } - - // Intermediate parent that bails out. Children will only re-render when the - // context changes. - const Indirection = React.memo(() => { - return ; - }); - - function Child() { - const {a} = unstable_useContextWithBailout(ContextA, context => [ - context.a, - ]); - const b = useContext(ContextB); - return ; - } - - const root = ReactNoop.createRoot(); - await act(async () => { - root.render(); - }); - assertLog(['A: 0, B: 0']); - expect(root).toMatchRenderedOutput('A: 0, B: 0'); - - // Update a field in A that isn't part of the compared context. It should - // bail out. - await act(async () => { - setContextA({a: 0, unrelated: 1}); - }); - assertLog([]); - expect(root).toMatchRenderedOutput('A: 0, B: 0'); - - // Now update the same a field again, but this time, also update a different - // context in the same batch. The other context prevents a bail out. - await act(async () => { - setContextA({a: 0, unrelated: 1}); - setContextB(1); - }); - assertLog(['A: 0, B: 1']); - expect(root).toMatchRenderedOutput('A: 0, B: 1'); - }); -}); diff --git a/packages/react/index.fb.js b/packages/react/index.fb.js index 8cded91b98..84128cf0ea 100644 --- a/packages/react/index.fb.js +++ b/packages/react/index.fb.js @@ -42,7 +42,6 @@ export { use, useActionState, useCallback, - unstable_useContextWithBailout, useContext, useDebugValue, useDeferredValue, diff --git a/packages/react/src/ReactClient.js b/packages/react/src/ReactClient.js index 90ce18b133..02625e3af9 100644 --- a/packages/react/src/ReactClient.js +++ b/packages/react/src/ReactClient.js @@ -37,7 +37,6 @@ import {postpone} from './ReactPostpone'; import { getCacheForType, useCallback, - unstable_useContextWithBailout, useContext, useEffect, useEffectEvent, @@ -86,7 +85,6 @@ export { cache, postpone as unstable_postpone, useCallback, - unstable_useContextWithBailout, useContext, useEffect, useEffectEvent as experimental_useEffectEvent, diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index 40c8a23d37..32f4926888 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -19,10 +19,6 @@ import {REACT_CONSUMER_TYPE} from 'shared/ReactSymbols'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import {enableUseResourceEffectHook} from 'shared/ReactFeatureFlags'; -import { - enableContextProfiling, - enableLazyContextPropagation, -} from '../../shared/ReactFeatureFlags'; type BasicStateAction = (S => S) | S; type Dispatch = A => void; @@ -69,27 +65,6 @@ export function useContext(Context: ReactContext): T { return dispatcher.useContext(Context); } -export function unstable_useContextWithBailout( - context: ReactContext, - select: (T => Array) | null, -): T { - if (!(enableLazyContextPropagation && enableContextProfiling)) { - throw new Error('Not implemented.'); - } - - const dispatcher = resolveDispatcher(); - if (__DEV__) { - if (context.$$typeof === REACT_CONSUMER_TYPE) { - console.error( - 'Calling useContext(Context.Consumer) is not supported and will cause bugs. ' + - 'Did you mean to call useContext(Context) instead?', - ); - } - } - // $FlowFixMe[not-a-function] This is unstable, thus optional - return dispatcher.unstable_useContextWithBailout(context, select); -} - export function useState( initialState: (() => S) | S, ): [S, Dispatch>] { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index d1dc6cb7f7..8caf79fbf2 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -101,9 +101,6 @@ export const enableTransitionTracing = false; export const enableLazyContextPropagation = true; -// Expose unstable useContext for performance testing -export const enableContextProfiling = false; - // FB-only usage. The new API has different semantics. export const enableLegacyHidden = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 498af214b0..6c14a9ece3 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -53,7 +53,6 @@ export const enableFizzExternalRuntime = true; export const enableGetInspectorDataForInstanceInProduction = true; export const enableHalt = false; export const enableInfiniteRenderLoopDetection = false; -export const enableContextProfiling = false; export const enableLazyContextPropagation = true; export const enableLegacyCache = false; export const enableLegacyFBSupport = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index fb996a43ad..6dce24e57f 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -43,7 +43,6 @@ export const enableHalt = false; export const enableHiddenSubtreeInsertionEffectCleanup = false; export const enableInfiniteRenderLoopDetection = false; export const enableLazyContextPropagation = true; -export const enableContextProfiling = false; export const enableLegacyCache = false; export const enableLegacyFBSupport = false; export const enableLegacyHidden = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 6646a126a0..b332c485a0 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -49,7 +49,6 @@ export const transitionLaneExpirationMs = 5000; export const disableSchedulerTimeoutInWorkLoop = false; export const enableLazyContextPropagation = true; -export const enableContextProfiling = false; export const enableLegacyHidden = false; export const enableTransitionTracing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index dae38936f8..20ecdb44b4 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -51,7 +51,6 @@ export const transitionLaneExpirationMs = 5000; export const disableSchedulerTimeoutInWorkLoop = false; export const enableLazyContextPropagation = true; -export const enableContextProfiling = false; export const enableLegacyHidden = false; export const enableTransitionTracing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 060600286e..8054a14bbc 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -79,8 +79,6 @@ export const enablePostpone = false; export const enableHalt = false; -export const enableContextProfiling = true; - // TODO: www currently relies on this feature. It's disabled in open source. // Need to remove it. export const disableCommentsAsDOMContainers = false; diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 65961e313b..3ad0aa6f60 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -530,4 +530,3 @@ "542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.", "543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React." } - From 07facb52d3cad10ae9888bf3b645d5a0cb760016 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 16 Dec 2024 12:39:15 -0500 Subject: [PATCH 2/7] [Flight] Sort Server Components Track Group ahead of Client Scheduler/Components Tracks (#31736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #31735. This ensures that Server Components Track comes first. Since it's typically rendered first on the server for initial load and then flows into scheduler and client components work. Also puts it closer to the Network and further away from "Main" JS. Screenshot 2024-12-11 at 5 31 41 PM Same trick as in #31615. --- .../react-client/src/ReactFlightClient.js | 6 +++++- .../src/ReactFlightPerformanceTrack.js | 20 +++++++++++++++++++ .../src/ReactFiberPerformanceTrack.js | 8 ++++---- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 0456136a7e..ccb8632289 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -69,7 +69,10 @@ import {createBoundServerReference} from './ReactFlightReplyClient'; import {readTemporaryReference} from './ReactFlightTemporaryReferences'; -import {logComponentRender} from './ReactFlightPerformanceTrack'; +import { + markAllTracksInOrder, + logComponentRender, +} from './ReactFlightPerformanceTrack'; import { REACT_LAZY_TYPE, @@ -643,6 +646,7 @@ export function reportGlobalError(response: Response, error: Error): void { } }); if (enableProfilerTimer && enableComponentPerformanceTrack) { + markAllTracksInOrder(); flushComponentPerformance(getChunk(response, 0), 0, -Infinity); } } diff --git a/packages/react-client/src/ReactFlightPerformanceTrack.js b/packages/react-client/src/ReactFlightPerformanceTrack.js index b8bd9ca2be..cf78f7d7de 100644 --- a/packages/react-client/src/ReactFlightPerformanceTrack.js +++ b/packages/react-client/src/ReactFlightPerformanceTrack.js @@ -19,6 +19,26 @@ const supportsUserTiming = const COMPONENTS_TRACK = 'Server Components ⚛'; +const componentsTrackMarker = { + startTime: 0.001, + detail: { + devtools: { + color: 'primary-light', + track: 'Primary', + trackGroup: COMPONENTS_TRACK, + }, + }, +}; + +export function markAllTracksInOrder() { + if (supportsUserTiming) { + // Ensure we create the Server Component track groups earlier than the Client Scheduler + // and Client Components. We can always add the 0 time slot even if it's in the past. + // That's still considered for ordering. + performance.mark('Server Components Track', componentsTrackMarker); + } +} + // Reused to avoid thrashing the GC. const reusableComponentDevToolDetails = { color: 'primary', diff --git a/packages/react-reconciler/src/ReactFiberPerformanceTrack.js b/packages/react-reconciler/src/ReactFiberPerformanceTrack.js index 7e9e246d8b..d94fd5ee48 100644 --- a/packages/react-reconciler/src/ReactFiberPerformanceTrack.js +++ b/packages/react-reconciler/src/ReactFiberPerformanceTrack.js @@ -63,7 +63,7 @@ export function setCurrentTrackFromLanes(lanes: Lanes): void { } const blockingLaneMarker = { - startTime: 0, + startTime: 0.003, detail: { devtools: { color: 'primary-light', @@ -74,7 +74,7 @@ const blockingLaneMarker = { }; const transitionLaneMarker = { - startTime: 0, + startTime: 0.003, detail: { devtools: { color: 'primary-light', @@ -85,7 +85,7 @@ const transitionLaneMarker = { }; const suspenseLaneMarker = { - startTime: 0, + startTime: 0.003, detail: { devtools: { color: 'primary-light', @@ -96,7 +96,7 @@ const suspenseLaneMarker = { }; const idleLaneMarker = { - startTime: 0, + startTime: 0.003, detail: { devtools: { color: 'primary-light', From bdf187174d610eb6471bfe39f0d80ab33b68cb3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 16 Dec 2024 13:16:53 -0500 Subject: [PATCH 3/7] [Flight] Emit Deduped Server Components Marker (#31737) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #31736. Screenshot 2024-12-11 at 8 21 12 PM This emits a placeholder when we're deduping a component. This starts when the parent's self time ends, where we would've started rendering this component if it wasn't already started. The end time is when the actual render ends since the parent is also blocked by it. --- fixtures/flight/src/App.js | 5 ++- .../react-client/src/ReactFlightClient.js | 45 +++++++++++++++++-- .../src/ReactFlightPerformanceTrack.js | 16 +++++++ 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/fixtures/flight/src/App.js b/fixtures/flight/src/App.js index 49bfc9e051..69fbb5e0af 100644 --- a/fixtures/flight/src/App.js +++ b/fixtures/flight/src/App.js @@ -27,7 +27,8 @@ function Foo({children}) { return
{children}
; } -function Bar({children}) { +async function Bar({children}) { + await new Promise(resolve => setTimeout(() => resolve('deferred text'), 10)); return
{children}
; } @@ -81,7 +82,7 @@ export default async function App({prerender}) { {dedupedChild} - {dedupedChild} + {Promise.resolve([dedupedChild])} diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index ccb8632289..52c7ec73a4 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -72,6 +72,7 @@ import {readTemporaryReference} from './ReactFlightTemporaryReferences'; import { markAllTracksInOrder, logComponentRender, + logDedupedComponentRender, } from './ReactFlightPerformanceTrack'; import { @@ -130,6 +131,7 @@ export type JSONValue = type ProfilingResult = { track: number, endTime: number, + component: null | ReactComponentInfo, }; const ROW_ID = 0; @@ -647,7 +649,7 @@ export function reportGlobalError(response: Response, error: Error): void { }); if (enableProfilerTimer && enableComponentPerformanceTrack) { markAllTracksInOrder(); - flushComponentPerformance(getChunk(response, 0), 0, -Infinity); + flushComponentPerformance(getChunk(response, 0), 0, -Infinity, -Infinity); } } @@ -2748,7 +2750,8 @@ function resolveTypedArray( function flushComponentPerformance( root: SomeChunk, trackIdx: number, // Next available track - trackTime: number, // The time after which it is available + trackTime: number, // The time after which it is available, + parentEndTime: number, ): ProfilingResult { if (!enableProfilerTimer || !enableComponentPerformanceTrack) { // eslint-disable-next-line react-internal/prod-error-codes @@ -2765,6 +2768,22 @@ function flushComponentPerformance( // chunk in two places. We should extend the current end time as if it was // rendered as part of this tree. const previousResult: ProfilingResult = root._children; + const previousEndTime = previousResult.endTime; + if ( + parentEndTime > -Infinity && + parentEndTime < previousEndTime && + previousResult.component !== null + ) { + // Log a placeholder for the deduped value under this child starting + // from the end of the self time of the parent and spanning until the + // the deduped end. + logDedupedComponentRender( + previousResult.component, + trackIdx, + parentEndTime, + previousEndTime, + ); + } // Since we didn't bump the track this time, we just return the same track. previousResult.track = trackIdx; return previousResult; @@ -2792,15 +2811,27 @@ function flushComponentPerformance( // The start time of this component is before the end time of the previous // component on this track so we need to bump the next one to a parallel track. trackIdx++; - trackTime = startTime; } + trackTime = startTime; break; } } } + for (let i = debugInfo.length - 1; i >= 0; i--) { + const info = debugInfo[i]; + if (typeof info.time === 'number') { + if (info.time > parentEndTime) { + parentEndTime = info.time; + } + } + } } - const result: ProfilingResult = {track: trackIdx, endTime: -Infinity}; + const result: ProfilingResult = { + track: trackIdx, + endTime: -Infinity, + component: null, + }; root._children = result; let childrenEndTime = -Infinity; let childTrackIdx = trackIdx; @@ -2810,7 +2841,11 @@ function flushComponentPerformance( children[i], childTrackIdx, childTrackTime, + parentEndTime, ); + if (childResult.component !== null) { + result.component = childResult.component; + } childTrackIdx = childResult.track; const childEndTime = childResult.endTime; childTrackTime = childEndTime; @@ -2842,6 +2877,8 @@ function flushComponentPerformance( endTime, childrenEndTime, ); + // Track the root most component of the result for deduping logging. + result.component = componentInfo; } } } diff --git a/packages/react-client/src/ReactFlightPerformanceTrack.js b/packages/react-client/src/ReactFlightPerformanceTrack.js index cf78f7d7de..cfd8ca96b3 100644 --- a/packages/react-client/src/ReactFlightPerformanceTrack.js +++ b/packages/react-client/src/ReactFlightPerformanceTrack.js @@ -90,3 +90,19 @@ export function logComponentRender( performance.measure(name, reusableComponentOptions); } } + +export function logDedupedComponentRender( + componentInfo: ReactComponentInfo, + trackIdx: number, + startTime: number, + endTime: number, +): void { + if (supportsUserTiming && endTime >= 0 && trackIdx < 10) { + const name = componentInfo.name; + reusableComponentDevToolDetails.color = 'tertiary-light'; + reusableComponentDevToolDetails.track = trackNames[trackIdx]; + reusableComponentOptions.start = startTime < 0 ? 0 : startTime; + reusableComponentOptions.end = endTime; + performance.measure(name + ' [deduped]', reusableComponentOptions); + } +} From 54e86bd0d0eac76320c8810090810e7a858125d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 16 Dec 2024 13:39:19 -0500 Subject: [PATCH 4/7] [Flight] Color and badge non-primary environments (#31738) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #31737. Screenshot 2024-12-11 at 8 41 15 PM When mixing environments (like "use cache" or third party RSC) it's useful to color and badge those components differently to differentiate. I'm not putting them in separate tracks because when they do actually execute, like cache misses or third party RSCs, they behave like they're part of the same tree. --- .../react-client/src/ReactFlightClient.js | 11 +++++++++- .../src/ReactFlightPerformanceTrack.js | 22 ++++++++++++++----- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 52c7ec73a4..8e00df7c1c 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -649,7 +649,13 @@ export function reportGlobalError(response: Response, error: Error): void { }); if (enableProfilerTimer && enableComponentPerformanceTrack) { markAllTracksInOrder(); - flushComponentPerformance(getChunk(response, 0), 0, -Infinity, -Infinity); + flushComponentPerformance( + response, + getChunk(response, 0), + 0, + -Infinity, + -Infinity, + ); } } @@ -2748,6 +2754,7 @@ function resolveTypedArray( } function flushComponentPerformance( + response: Response, root: SomeChunk, trackIdx: number, // Next available track trackTime: number, // The time after which it is available, @@ -2838,6 +2845,7 @@ function flushComponentPerformance( let childTrackTime = trackTime; for (let i = 0; i < children.length; i++) { const childResult = flushComponentPerformance( + response, children[i], childTrackIdx, childTrackTime, @@ -2876,6 +2884,7 @@ function flushComponentPerformance( startTime, endTime, childrenEndTime, + response._rootEnvironmentName, ); // Track the root most component of the result for deduping logging. result.component = componentInfo; diff --git a/packages/react-client/src/ReactFlightPerformanceTrack.js b/packages/react-client/src/ReactFlightPerformanceTrack.js index cfd8ca96b3..d2860e407c 100644 --- a/packages/react-client/src/ReactFlightPerformanceTrack.js +++ b/packages/react-client/src/ReactFlightPerformanceTrack.js @@ -72,22 +72,33 @@ export function logComponentRender( startTime: number, endTime: number, childrenEndTime: number, + rootEnv: string, ): void { if (supportsUserTiming && childrenEndTime >= 0 && trackIdx < 10) { + const env = componentInfo.env; const name = componentInfo.name; + const isPrimaryEnv = env === rootEnv; const selfTime = endTime - startTime; reusableComponentDevToolDetails.color = selfTime < 0.5 - ? 'primary-light' + ? isPrimaryEnv + ? 'primary-light' + : 'secondary-light' : selfTime < 50 - ? 'primary' + ? isPrimaryEnv + ? 'primary' + : 'secondary' : selfTime < 500 - ? 'primary-dark' + ? isPrimaryEnv + ? 'primary-dark' + : 'secondary-dark' : 'error'; reusableComponentDevToolDetails.track = trackNames[trackIdx]; reusableComponentOptions.start = startTime < 0 ? 0 : startTime; reusableComponentOptions.end = childrenEndTime; - performance.measure(name, reusableComponentOptions); + const entryName = + isPrimaryEnv || env === undefined ? name : name + ' [' + env + ']'; + performance.measure(entryName, reusableComponentOptions); } } @@ -103,6 +114,7 @@ export function logDedupedComponentRender( reusableComponentDevToolDetails.track = trackNames[trackIdx]; reusableComponentOptions.start = startTime < 0 ? 0 : startTime; reusableComponentOptions.end = endTime; - performance.measure(name + ' [deduped]', reusableComponentOptions); + const entryName = name + ' [deduped]'; + performance.measure(entryName, reusableComponentOptions); } } From e30872a4e01bdc0cf185a818156ae7741c815e21 Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Mon, 16 Dec 2024 14:43:21 -0500 Subject: [PATCH 5/7] [compiler][be] Playground now compiles entire program (#31774) Compiler playground now runs the entire program through `babel-plugin-react-compiler` instead of a custom pipeline which previously duplicated function inference logic from `Program.ts`. In addition, the playground output reflects the tranformed file (instead of a "virtual file" of manually concatenated functions). This helps with the following: - Reduce potential discrepencies between playground and babel plugin behavior. See attached fixture output for an example where we previously diverged. - Let playground users see compiler-inserted imports (e.g. `_c` or `useFire`) This also helps us repurpose playground into a more general tool for compiler-users instead of just for compiler engineers. - imports and other functions are preserved. We differentiate between imports and globals in many cases (e.g. `inferEffectDeps`), so it may be misleading to omit imports in printed output - playground now shows other program-changing behavior like position of outlined functions and hoisted declarations - emitted compiled functions do not need synthetic names --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31774). * #31809 * __->__ #31774 --- .../page.spec.ts/01-user-output.txt | 3 +- .../page.spec.ts/02-default-output.txt | 3 +- .../module-scope-use-memo-output.txt | 4 +- .../module-scope-use-no-memo-output.txt | 3 +- .../page.spec.ts/parse-flow-output.txt | 14 + .../page.spec.ts/parse-typescript-output.txt | 20 ++ ...cope-does-not-beat-module-scope-output.txt | 5 + .../page.spec.ts/use-memo-output.txt | 5 +- .../page.spec.ts/use-no-memo-output.txt | 8 +- .../playground/__tests__/e2e/page.spec.ts | 41 ++- .../components/Editor/EditorImpl.tsx | 260 ++++-------------- .../playground/components/Editor/Output.tsx | 63 ++--- .../src/Babel/BabelPlugin.ts | 5 +- .../src/Entrypoint/Options.ts | 2 + .../src/Entrypoint/Pipeline.ts | 141 +++++----- .../src/HIR/Environment.ts | 29 +- ...ule-scope-usememo-function-scope.expect.md | 29 ++ ...emo-module-scope-usememo-function-scope.js | 7 + .../src/__tests__/parseConfigPragma-test.ts | 1 + .../babel-plugin-react-compiler/src/index.ts | 2 - 20 files changed, 302 insertions(+), 343 deletions(-) create mode 100644 compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/parse-flow-output.txt create mode 100644 compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/parse-typescript-output.txt create mode 100644 compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/todo-function-scope-does-not-beat-module-scope-output.txt create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-no-memo-module-scope-usememo-function-scope.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-no-memo-module-scope-usememo-function-scope.js diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/01-user-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/01-user-output.txt index ba680bbb57..1600f35107 100644 --- a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/01-user-output.txt +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/01-user-output.txt @@ -1,4 +1,5 @@ -function TestComponent(t0) { +import { c as _c } from "react/compiler-runtime"; +export default function TestComponent(t0) { const $ = _c(2); const { x } = t0; let t1; diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/02-default-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/02-default-output.txt index 2cbd09bba6..1d59a120f9 100644 --- a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/02-default-output.txt +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/02-default-output.txt @@ -1,4 +1,5 @@ -function MyApp() { +import { c as _c } from "react/compiler-runtime"; +export default function MyApp() { const $ = _c(1); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/module-scope-use-memo-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/module-scope-use-memo-output.txt index ba680bbb57..638a2bcd22 100644 --- a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/module-scope-use-memo-output.txt +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/module-scope-use-memo-output.txt @@ -1,4 +1,6 @@ -function TestComponent(t0) { +"use memo"; +import { c as _c } from "react/compiler-runtime"; +export default function TestComponent(t0) { const $ = _c(2); const { x } = t0; let t1; diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/module-scope-use-no-memo-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/module-scope-use-no-memo-output.txt index 2c69ddc1d6..ebd2d2b046 100644 --- a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/module-scope-use-no-memo-output.txt +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/module-scope-use-no-memo-output.txt @@ -1,3 +1,4 @@ -function TestComponent({ x }) { +"use no memo"; +export default function TestComponent({ x }) { return ; } diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/parse-flow-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/parse-flow-output.txt new file mode 100644 index 0000000000..c0907856ba --- /dev/null +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/parse-flow-output.txt @@ -0,0 +1,14 @@ +import { c as _c } from "react/compiler-runtime"; +function useFoo(propVal) { +  const $ = _c(2); +  const t0 = (propVal.baz: number); +  let t1; +  if ($[0] !== t0) { +    t1 = 
{t0}
; +    $[0] = t0; +    $[1] = t1; +  } else { +    t1 = $[1]; +  } +  return t1; +} \ No newline at end of file diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/parse-typescript-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/parse-typescript-output.txt new file mode 100644 index 0000000000..c936c5ae2e --- /dev/null +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/parse-typescript-output.txt @@ -0,0 +1,20 @@ +import { c as _c } from "react/compiler-runtime"; +function Foo() { +  const $ = _c(2); +  let t0; +  if ($[0] === Symbol.for("react.memo_cache_sentinel")) { +    t0 = foo(); +    $[0] = t0; +  } else { +    t0 = $[0]; +  } +  const x = t0 as number; +  let t1; +  if ($[1] === Symbol.for("react.memo_cache_sentinel")) { +    t1 = 
{x}
; +    $[1] = t1; +  } else { +    t1 = $[1]; +  } +  return t1; +} \ No newline at end of file diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/todo-function-scope-does-not-beat-module-scope-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/todo-function-scope-does-not-beat-module-scope-output.txt new file mode 100644 index 0000000000..325e6972e1 --- /dev/null +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/todo-function-scope-does-not-beat-module-scope-output.txt @@ -0,0 +1,5 @@ +"use no memo"; +function TestComponent({ x }) { + "use memo"; + return ; +} diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/use-memo-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/use-memo-output.txt index 804bacab97..de6dd52680 100644 --- a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/use-memo-output.txt +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/use-memo-output.txt @@ -1,3 +1,4 @@ +import { c as _c } from "react/compiler-runtime"; function TestComponent(t0) { "use memo"; const $ = _c(2); @@ -12,7 +13,7 @@ function TestComponent(t0) { } return t1; } -function anonymous_1(t0) { +const TestComponent2 = (t0) => { "use memo"; const $ = _c(2); const { x } = t0; @@ -25,4 +26,4 @@ function anonymous_1(t0) { t1 = $[1]; } return t1; -} +}; diff --git a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/use-no-memo-output.txt b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/use-no-memo-output.txt index 5fb66309fc..02c1367622 100644 --- a/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/use-no-memo-output.txt +++ b/compiler/apps/playground/__tests__/e2e/__snapshots__/page.spec.ts/use-no-memo-output.txt @@ -1,8 +1,8 @@ -function anonymous_1() { +const TestComponent = function () { "use no memo"; return ; -} -function anonymous_3({ x }) { +}; +const TestComponent2 = ({ x }) => { "use no memo"; return ; -} +}; diff --git a/compiler/apps/playground/__tests__/e2e/page.spec.ts b/compiler/apps/playground/__tests__/e2e/page.spec.ts index 846e6227bd..05fe96d4b9 100644 --- a/compiler/apps/playground/__tests__/e2e/page.spec.ts +++ b/compiler/apps/playground/__tests__/e2e/page.spec.ts @@ -9,11 +9,11 @@ import {expect, test} from '@playwright/test'; import {encodeStore, type Store} from '../../lib/stores'; import {format} from 'prettier'; -function print(data: Array): Promise { +function formatPrint(data: Array): Promise { return format(data.join(''), {parser: 'babel'}); } -const DIRECTIVE_TEST_CASES = [ +const TEST_CASE_INPUTS = [ { name: 'module-scope-use-memo', input: ` @@ -55,7 +55,7 @@ const TestComponent2 = ({ x }) => { };`, }, { - name: 'function-scope-beats-module-scope', + name: 'todo-function-scope-does-not-beat-module-scope', input: ` 'use no memo'; function TestComponent({ x }) { @@ -63,6 +63,26 @@ function TestComponent({ x }) { return ; }`, }, + { + name: 'parse-typescript', + input: ` +function Foo() { + const x = foo() as number; + return
{x}
; +} +`, + noFormat: true, + }, + { + name: 'parse-flow', + input: ` +// @flow +function useFoo(propVal: {+baz: number}) { + return
{(propVal.baz as number)}
; +} + `, + noFormat: true, + }, ]; test('editor should open successfully', async ({page}) => { @@ -90,7 +110,7 @@ test('editor should compile from hash successfully', async ({page}) => { }); const text = (await page.locator('.monaco-editor').nth(1).allInnerTexts()) ?? []; - const output = await print(text); + const output = await formatPrint(text); expect(output).not.toEqual(''); expect(output).toMatchSnapshot('01-user-output.txt'); @@ -115,14 +135,14 @@ test('reset button works', async ({page}) => { }); const text = (await page.locator('.monaco-editor').nth(1).allInnerTexts()) ?? []; - const output = await print(text); + const output = await formatPrint(text); expect(output).not.toEqual(''); expect(output).toMatchSnapshot('02-default-output.txt'); }); -DIRECTIVE_TEST_CASES.forEach((t, idx) => - test(`directives work: ${t.name}`, async ({page}) => { +TEST_CASE_INPUTS.forEach((t, idx) => + test(`playground compiles: ${t.name}`, async ({page}) => { const store: Store = { source: t.input, }; @@ -135,7 +155,12 @@ DIRECTIVE_TEST_CASES.forEach((t, idx) => const text = (await page.locator('.monaco-editor').nth(1).allInnerTexts()) ?? []; - const output = await print(text); + let output: string; + if (t.noFormat) { + output = text.join(''); + } else { + output = await formatPrint(text); + } expect(output).not.toEqual(''); expect(output).toMatchSnapshot(`${t.name}-output.txt`); diff --git a/compiler/apps/playground/components/Editor/EditorImpl.tsx b/compiler/apps/playground/components/Editor/EditorImpl.tsx index 82a40272bd..785b9fd075 100644 --- a/compiler/apps/playground/components/Editor/EditorImpl.tsx +++ b/compiler/apps/playground/components/Editor/EditorImpl.tsx @@ -5,23 +5,22 @@ * LICENSE file in the root directory of this source tree. */ -import {parse as babelParse} from '@babel/parser'; +import {parse as babelParse, ParseResult} from '@babel/parser'; import * as HermesParser from 'hermes-parser'; -import traverse, {NodePath} from '@babel/traverse'; import * as t from '@babel/types'; -import { +import BabelPluginReactCompiler, { CompilerError, CompilerErrorDetail, Effect, ErrorSeverity, parseConfigPragmaForTests, ValueKind, - runPlayground, type Hook, - findDirectiveDisablingMemoization, - findDirectiveEnablingMemoization, + PluginOptions, + CompilerPipelineValue, + parsePluginOptions, } from 'babel-plugin-react-compiler/src'; -import {type ReactFunctionType} from 'babel-plugin-react-compiler/src/HIR/Environment'; +import {type EnvironmentConfig} from 'babel-plugin-react-compiler/src/HIR/Environment'; import clsx from 'clsx'; import invariant from 'invariant'; import {useSnackbar} from 'notistack'; @@ -39,32 +38,18 @@ import {useStore, useStoreDispatch} from '../StoreContext'; import Input from './Input'; import { CompilerOutput, + CompilerTransformOutput, default as Output, PrintedCompilerPipelineValue, } from './Output'; import {printFunctionWithOutlined} from 'babel-plugin-react-compiler/src/HIR/PrintHIR'; import {printReactiveFunctionWithOutlined} from 'babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction'; +import {transformFromAstSync} from '@babel/core'; -type FunctionLike = - | NodePath - | NodePath - | NodePath; -enum MemoizeDirectiveState { - Enabled = 'Enabled', - Disabled = 'Disabled', - Undefined = 'Undefined', -} - -const MEMOIZE_ENABLED_OR_UNDEFINED_STATES = new Set([ - MemoizeDirectiveState.Enabled, - MemoizeDirectiveState.Undefined, -]); - -const MEMOIZE_ENABLED_OR_DISABLED_STATES = new Set([ - MemoizeDirectiveState.Enabled, - MemoizeDirectiveState.Disabled, -]); -function parseInput(input: string, language: 'flow' | 'typescript'): any { +function parseInput( + input: string, + language: 'flow' | 'typescript', +): ParseResult { // Extract the first line to quickly check for custom test directives if (language === 'flow') { return HermesParser.parse(input, { @@ -77,95 +62,45 @@ function parseInput(input: string, language: 'flow' | 'typescript'): any { return babelParse(input, { plugins: ['typescript', 'jsx'], sourceType: 'module', - }); + }) as ParseResult; } } -function parseFunctions( +function invokeCompiler( source: string, language: 'flow' | 'typescript', -): Array<{ - compilationEnabled: boolean; - fn: FunctionLike; -}> { - const items: Array<{ - compilationEnabled: boolean; - fn: FunctionLike; - }> = []; - try { - const ast = parseInput(source, language); - traverse(ast, { - FunctionDeclaration(nodePath) { - items.push({ - compilationEnabled: shouldCompile(nodePath), - fn: nodePath, - }); - nodePath.skip(); - }, - ArrowFunctionExpression(nodePath) { - items.push({ - compilationEnabled: shouldCompile(nodePath), - fn: nodePath, - }); - nodePath.skip(); - }, - FunctionExpression(nodePath) { - items.push({ - compilationEnabled: shouldCompile(nodePath), - fn: nodePath, - }); - nodePath.skip(); - }, - }); - } catch (e) { - console.error(e); - CompilerError.throwInvalidJS({ - reason: String(e), - description: null, - loc: null, - suggestions: null, - }); + environment: EnvironmentConfig, + logIR: (pipelineValue: CompilerPipelineValue) => void, +): CompilerTransformOutput { + const opts: PluginOptions = parsePluginOptions({ + logger: { + debugLogIRs: logIR, + logEvent: () => {}, + }, + environment, + compilationMode: 'all', + panicThreshold: 'all_errors', + }); + const ast = parseInput(source, language); + let result = transformFromAstSync(ast, source, { + filename: '_playgroundFile.js', + highlightCode: false, + retainLines: true, + plugins: [[BabelPluginReactCompiler, opts]], + ast: true, + sourceType: 'module', + configFile: false, + sourceMaps: true, + babelrc: false, + }); + if (result?.ast == null || result?.code == null || result?.map == null) { + throw new Error('Expected successful compilation'); } - - return items; -} - -function shouldCompile(fn: FunctionLike): boolean { - const {body} = fn.node; - if (t.isBlockStatement(body)) { - const selfCheck = checkExplicitMemoizeDirectives(body.directives); - if (selfCheck === MemoizeDirectiveState.Enabled) return true; - if (selfCheck === MemoizeDirectiveState.Disabled) return false; - - const parentWithDirective = fn.findParent(parentPath => { - if (parentPath.isBlockStatement() || parentPath.isProgram()) { - const directiveCheck = checkExplicitMemoizeDirectives( - parentPath.node.directives, - ); - return MEMOIZE_ENABLED_OR_DISABLED_STATES.has(directiveCheck); - } - return false; - }); - - if (!parentWithDirective) return true; - const parentDirectiveCheck = checkExplicitMemoizeDirectives( - (parentWithDirective.node as t.Program | t.BlockStatement).directives, - ); - return MEMOIZE_ENABLED_OR_UNDEFINED_STATES.has(parentDirectiveCheck); - } - return false; -} - -function checkExplicitMemoizeDirectives( - directives: Array, -): MemoizeDirectiveState { - if (findDirectiveEnablingMemoization(directives).length) { - return MemoizeDirectiveState.Enabled; - } - if (findDirectiveDisablingMemoization(directives).length) { - return MemoizeDirectiveState.Disabled; - } - return MemoizeDirectiveState.Undefined; + return { + code: result.code, + sourceMaps: result.map, + language, + }; } const COMMON_HOOKS: Array<[string, Hook]> = [ @@ -216,37 +151,6 @@ const COMMON_HOOKS: Array<[string, Hook]> = [ ], ]; -function isHookName(s: string): boolean { - return /^use[A-Z0-9]/.test(s); -} - -function getReactFunctionType(id: t.Identifier | null): ReactFunctionType { - if (id != null) { - if (isHookName(id.name)) { - return 'Hook'; - } - - const isPascalCaseNameSpace = /^[A-Z].*/; - if (isPascalCaseNameSpace.test(id.name)) { - return 'Component'; - } - } - return 'Other'; -} - -function getFunctionIdentifier( - fn: - | NodePath - | NodePath - | NodePath, -): t.Identifier | null { - if (fn.isArrowFunctionExpression()) { - return null; - } - const id = fn.get('id'); - return Array.isArray(id) === false && id.isIdentifier() ? id.node : null; -} - function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] { const results = new Map>(); const error = new CompilerError(); @@ -264,71 +168,25 @@ function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] { } else { language = 'typescript'; } - let count = 0; - const withIdentifier = (id: t.Identifier | null): t.Identifier => { - if (id != null && id.name != null) { - return id; - } else { - return t.identifier(`anonymous_${count++}`); - } - }; + let transformOutput; try { // Extract the first line to quickly check for custom test directives const pragma = source.substring(0, source.indexOf('\n')); const config = parseConfigPragmaForTests(pragma); - const parsedFunctions = parseFunctions(source, language); - for (const func of parsedFunctions) { - const id = withIdentifier(getFunctionIdentifier(func.fn)); - const fnName = id.name; - if (!func.compilationEnabled) { - upsert({ - kind: 'ast', - fnName, - name: 'CodeGen', - value: { - type: 'FunctionDeclaration', - id: - func.fn.isArrowFunctionExpression() || - func.fn.isFunctionExpression() - ? withIdentifier(null) - : func.fn.node.id, - async: func.fn.node.async, - generator: !!func.fn.node.generator, - body: func.fn.node.body as t.BlockStatement, - params: func.fn.node.params, - }, - }); - continue; - } - for (const result of runPlayground( - func.fn, - { - ...config, - customHooks: new Map([...COMMON_HOOKS]), - }, - getReactFunctionType(id), - )) { + + transformOutput = invokeCompiler( + source, + language, + {...config, customHooks: new Map([...COMMON_HOOKS])}, + result => { switch (result.kind) { case 'ast': { - upsert({ - kind: 'ast', - fnName, - name: result.name, - value: { - type: 'FunctionDeclaration', - id: withIdentifier(result.value.id), - async: result.value.async, - generator: result.value.generator, - body: result.value.body, - params: result.value.params, - }, - }); break; } case 'hir': { upsert({ kind: 'hir', - fnName, + fnName: result.value.id, name: result.name, value: printFunctionWithOutlined(result.value), }); @@ -337,7 +195,7 @@ function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] { case 'reactive': { upsert({ kind: 'reactive', - fnName, + fnName: result.value.id, name: result.name, value: printReactiveFunctionWithOutlined(result.value), }); @@ -346,7 +204,7 @@ function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] { case 'debug': { upsert({ kind: 'debug', - fnName, + fnName: null, name: result.name, value: result.value, }); @@ -357,8 +215,8 @@ function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] { throw new Error(`Unhandled result ${result}`); } } - } - } + }, + ); } catch (err) { /** * error might be an invariant violation or other runtime error @@ -385,7 +243,7 @@ function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] { if (error.hasErrors()) { return [{kind: 'err', results, error: error}, language]; } - return [{kind: 'ok', results}, language]; + return [{kind: 'ok', results, transformOutput}, language]; } export default function Editor(): JSX.Element { @@ -405,7 +263,7 @@ export default function Editor(): JSX.Element { } catch (e) { invariant(e instanceof Error, 'Only Error may be caught.'); enqueueSnackbar(e.message, { - variant: 'message', + variant: 'warning', ...createMessage( 'Bad URL - fell back to the default Playground.', MessageLevel.Info, diff --git a/compiler/apps/playground/components/Editor/Output.tsx b/compiler/apps/playground/components/Editor/Output.tsx index 97a63400d2..d4127c63cf 100644 --- a/compiler/apps/playground/components/Editor/Output.tsx +++ b/compiler/apps/playground/components/Editor/Output.tsx @@ -5,8 +5,6 @@ * LICENSE file in the root directory of this source tree. */ -import generate from '@babel/generator'; -import * as t from '@babel/types'; import { CodeIcon, DocumentAddIcon, @@ -21,17 +19,12 @@ import {memo, ReactNode, useEffect, useState} from 'react'; import {type Store} from '../../lib/stores'; import TabbedWindow from '../TabbedWindow'; import {monacoOptions} from './monacoOptions'; +import {BabelFileResult} from '@babel/core'; const MemoizedOutput = memo(Output); export default MemoizedOutput; export type PrintedCompilerPipelineValue = - | { - kind: 'ast'; - name: string; - fnName: string | null; - value: t.FunctionDeclaration; - } | { kind: 'hir'; name: string; @@ -41,8 +34,17 @@ export type PrintedCompilerPipelineValue = | {kind: 'reactive'; name: string; fnName: string | null; value: string} | {kind: 'debug'; name: string; fnName: string | null; value: string}; +export type CompilerTransformOutput = { + code: string; + sourceMaps: BabelFileResult['map']; + language: 'flow' | 'typescript'; +}; export type CompilerOutput = - | {kind: 'ok'; results: Map>} + | { + kind: 'ok'; + transformOutput: CompilerTransformOutput; + results: Map>; + } | { kind: 'err'; results: Map>; @@ -61,7 +63,6 @@ async function tabify( const tabs = new Map(); const reorderedTabs = new Map(); const concattedResults = new Map(); - let topLevelFnDecls: Array = []; // Concat all top level function declaration results into a single tab for each pass for (const [passName, results] of compilerOutput.results) { for (const result of results) { @@ -87,9 +88,6 @@ async function tabify( } break; } - case 'ast': - topLevelFnDecls.push(result.value); - break; case 'debug': { concattedResults.set(passName, result.value); break; @@ -114,13 +112,17 @@ async function tabify( lastPassOutput = text; } // Ensure that JS and the JS source map come first - if (topLevelFnDecls.length > 0) { - /** - * Make a synthetic Program so we can have a single AST with all the top level - * FunctionDeclarations - */ - const ast = t.program(topLevelFnDecls); - const {code, sourceMapUrl} = await codegen(ast, source); + if (compilerOutput.kind === 'ok') { + const {transformOutput} = compilerOutput; + const sourceMapUrl = getSourceMapUrl( + transformOutput.code, + JSON.stringify(transformOutput.sourceMaps), + ); + const code = await prettier.format(transformOutput.code, { + semi: true, + parser: transformOutput.language === 'flow' ? 'babel-flow' : 'babel-ts', + plugins: [parserBabel, prettierPluginEstree], + }); reorderedTabs.set( 'JS', { - const generated = generate( - ast, - {sourceMaps: true, sourceFileName: 'input.js'}, - source, - ); - const sourceMapUrl = getSourceMapUrl( - generated.code, - JSON.stringify(generated.map), - ); - const codegenOutput = await prettier.format(generated.code, { - semi: true, - parser: 'babel', - plugins: [parserBabel, prettierPluginEstree], - }); - return {code: codegenOutput, sourceMapUrl}; -} - function utf16ToUTF8(s: string): string { return unescape(encodeURIComponent(s)); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts b/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts index 401cbd4bdf..c648c66043 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts @@ -39,7 +39,10 @@ export default function BabelPluginReactCompiler( ) { opts = injectReanimatedFlag(opts); } - if (isDev) { + if ( + opts.environment.enableResetCacheOnSourceFileChanges !== false && + isDev + ) { opts = { ...opts, environment: { 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 72ed9e7c86..fb951d25c5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts @@ -15,6 +15,7 @@ import { } from '../HIR/Environment'; import {hasOwnProperty} from '../Utils/utils'; import {fromZodError} from 'zod-validation-error'; +import {CompilerPipelineValue} from './Pipeline'; const PanicThresholdOptionsSchema = z.enum([ /* @@ -209,6 +210,7 @@ export type LoggerEvent = export type Logger = { logEvent: (filename: string | null, event: LoggerEvent) => void; + debugLogIRs?: (value: CompilerPipelineValue) => void; }; export const defaultOptions: PluginOptions = { 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 8a97eea217..6995aec7f1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -112,7 +112,7 @@ export type CompilerPipelineValue = | {kind: 'reactive'; name: string; value: ReactiveFunction} | {kind: 'debug'; name: string; value: string}; -export function* run( +function run( func: NodePath< t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression >, @@ -122,7 +122,7 @@ export function* run( logger: Logger | null, filename: string | null, code: string | null, -): Generator { +): CodegenFunction { const contextIdentifiers = findContextIdentifiers(func); const env = new Environment( func.scope, @@ -134,12 +134,17 @@ export function* run( code, useMemoCacheIdentifier, ); - yield log({ + env.logger?.debugLogIRs?.({ kind: 'debug', name: 'EnvironmentConfig', value: prettyFormat(env.config), }); - const ast = yield* runWithEnvironment(func, env); + printLog({ + kind: 'debug', + name: 'EnvironmentConfig', + value: prettyFormat(env.config), + }); + const ast = runWithEnvironment(func, env); return ast; } @@ -147,17 +152,22 @@ export function* run( * Note: this is split from run() to make `config` out of scope, so that all * access to feature flags has to be through the Environment for consistency. */ -function* runWithEnvironment( +function runWithEnvironment( func: NodePath< t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression >, env: Environment, -): Generator { +): CodegenFunction { + const log = (value: CompilerPipelineValue): CompilerPipelineValue => { + printLog(value); + env.logger?.debugLogIRs?.(value); + return value; + }; const hir = lower(func, env).unwrap(); - yield log({kind: 'hir', name: 'HIR', value: hir}); + log({kind: 'hir', name: 'HIR', value: hir}); pruneMaybeThrows(hir); - yield log({kind: 'hir', name: 'PruneMaybeThrows', value: hir}); + log({kind: 'hir', name: 'PruneMaybeThrows', value: hir}); validateContextVariableLValues(hir); validateUseMemo(hir); @@ -168,35 +178,35 @@ function* runWithEnvironment( !env.config.enableChangeDetectionForDebugging ) { dropManualMemoization(hir); - yield log({kind: 'hir', name: 'DropManualMemoization', value: hir}); + log({kind: 'hir', name: 'DropManualMemoization', value: hir}); } inlineImmediatelyInvokedFunctionExpressions(hir); - yield log({ + log({ kind: 'hir', name: 'InlineImmediatelyInvokedFunctionExpressions', value: hir, }); mergeConsecutiveBlocks(hir); - yield log({kind: 'hir', name: 'MergeConsecutiveBlocks', value: hir}); + log({kind: 'hir', name: 'MergeConsecutiveBlocks', value: hir}); assertConsistentIdentifiers(hir); assertTerminalSuccessorsExist(hir); enterSSA(hir); - yield log({kind: 'hir', name: 'SSA', value: hir}); + log({kind: 'hir', name: 'SSA', value: hir}); eliminateRedundantPhi(hir); - yield log({kind: 'hir', name: 'EliminateRedundantPhi', value: hir}); + log({kind: 'hir', name: 'EliminateRedundantPhi', value: hir}); assertConsistentIdentifiers(hir); constantPropagation(hir); - yield log({kind: 'hir', name: 'ConstantPropagation', value: hir}); + log({kind: 'hir', name: 'ConstantPropagation', value: hir}); inferTypes(hir); - yield log({kind: 'hir', name: 'InferTypes', value: hir}); + log({kind: 'hir', name: 'InferTypes', value: hir}); if (env.config.validateHooksUsage) { validateHooksUsage(hir); @@ -211,30 +221,30 @@ function* runWithEnvironment( } optimizePropsMethodCalls(hir); - yield log({kind: 'hir', name: 'OptimizePropsMethodCalls', value: hir}); + log({kind: 'hir', name: 'OptimizePropsMethodCalls', value: hir}); analyseFunctions(hir); - yield log({kind: 'hir', name: 'AnalyseFunctions', value: hir}); + log({kind: 'hir', name: 'AnalyseFunctions', value: hir}); inferReferenceEffects(hir); - yield log({kind: 'hir', name: 'InferReferenceEffects', value: hir}); + log({kind: 'hir', name: 'InferReferenceEffects', value: hir}); validateLocalsNotReassignedAfterRender(hir); // Note: Has to come after infer reference effects because "dead" code may still affect inference deadCodeElimination(hir); - yield log({kind: 'hir', name: 'DeadCodeElimination', value: hir}); + log({kind: 'hir', name: 'DeadCodeElimination', value: hir}); if (env.config.enableInstructionReordering) { instructionReordering(hir); - yield log({kind: 'hir', name: 'InstructionReordering', value: hir}); + log({kind: 'hir', name: 'InstructionReordering', value: hir}); } pruneMaybeThrows(hir); - yield log({kind: 'hir', name: 'PruneMaybeThrows', value: hir}); + log({kind: 'hir', name: 'PruneMaybeThrows', value: hir}); inferMutableRanges(hir); - yield log({kind: 'hir', name: 'InferMutableRanges', value: hir}); + log({kind: 'hir', name: 'InferMutableRanges', value: hir}); if (env.config.assertValidMutableRanges) { assertValidMutableRanges(hir); @@ -257,27 +267,27 @@ function* runWithEnvironment( } inferReactivePlaces(hir); - yield log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); + log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); rewriteInstructionKindsBasedOnReassignment(hir); - yield log({ + log({ kind: 'hir', name: 'RewriteInstructionKindsBasedOnReassignment', value: hir, }); propagatePhiTypes(hir); - yield log({ + log({ kind: 'hir', name: 'PropagatePhiTypes', value: hir, }); inferReactiveScopeVariables(hir); - yield log({kind: 'hir', name: 'InferReactiveScopeVariables', value: hir}); + log({kind: 'hir', name: 'InferReactiveScopeVariables', value: hir}); const fbtOperands = memoizeFbtAndMacroOperandsInSameScope(hir); - yield log({ + log({ kind: 'hir', name: 'MemoizeFbtAndMacroOperandsInSameScope', value: hir, @@ -289,39 +299,39 @@ function* runWithEnvironment( if (env.config.enableFunctionOutlining) { outlineFunctions(hir, fbtOperands); - yield log({kind: 'hir', name: 'OutlineFunctions', value: hir}); + log({kind: 'hir', name: 'OutlineFunctions', value: hir}); } alignMethodCallScopes(hir); - yield log({ + log({ kind: 'hir', name: 'AlignMethodCallScopes', value: hir, }); alignObjectMethodScopes(hir); - yield log({ + log({ kind: 'hir', name: 'AlignObjectMethodScopes', value: hir, }); pruneUnusedLabelsHIR(hir); - yield log({ + log({ kind: 'hir', name: 'PruneUnusedLabelsHIR', value: hir, }); alignReactiveScopesToBlockScopesHIR(hir); - yield log({ + log({ kind: 'hir', name: 'AlignReactiveScopesToBlockScopesHIR', value: hir, }); mergeOverlappingReactiveScopesHIR(hir); - yield log({ + log({ kind: 'hir', name: 'MergeOverlappingReactiveScopesHIR', value: hir, @@ -329,7 +339,7 @@ function* runWithEnvironment( assertValidBlockNesting(hir); buildReactiveScopeTerminalsHIR(hir); - yield log({ + log({ kind: 'hir', name: 'BuildReactiveScopeTerminalsHIR', value: hir, @@ -338,14 +348,14 @@ function* runWithEnvironment( assertValidBlockNesting(hir); flattenReactiveLoopsHIR(hir); - yield log({ + log({ kind: 'hir', name: 'FlattenReactiveLoopsHIR', value: hir, }); flattenScopesWithHooksOrUseHIR(hir); - yield log({ + log({ kind: 'hir', name: 'FlattenScopesWithHooksOrUseHIR', value: hir, @@ -353,7 +363,7 @@ function* runWithEnvironment( assertTerminalSuccessorsExist(hir); assertTerminalPredsExist(hir); propagateScopeDependenciesHIR(hir); - yield log({ + log({ kind: 'hir', name: 'PropagateScopeDependenciesHIR', value: hir, @@ -365,7 +375,7 @@ function* runWithEnvironment( if (env.config.inlineJsxTransform) { inlineJsxTransform(hir, env.config.inlineJsxTransform); - yield log({ + log({ kind: 'hir', name: 'inlineJsxTransform', value: hir, @@ -373,7 +383,7 @@ function* runWithEnvironment( } const reactiveFunction = buildReactiveFunction(hir); - yield log({ + log({ kind: 'reactive', name: 'BuildReactiveFunction', value: reactiveFunction, @@ -382,7 +392,7 @@ function* runWithEnvironment( assertWellFormedBreakTargets(reactiveFunction); pruneUnusedLabels(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PruneUnusedLabels', value: reactiveFunction, @@ -390,35 +400,35 @@ function* runWithEnvironment( assertScopeInstructionsWithinScopes(reactiveFunction); pruneNonEscapingScopes(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PruneNonEscapingScopes', value: reactiveFunction, }); pruneNonReactiveDependencies(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PruneNonReactiveDependencies', value: reactiveFunction, }); pruneUnusedScopes(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PruneUnusedScopes', value: reactiveFunction, }); mergeReactiveScopesThatInvalidateTogether(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'MergeReactiveScopesThatInvalidateTogether', value: reactiveFunction, }); pruneAlwaysInvalidatingScopes(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PruneAlwaysInvalidatingScopes', value: reactiveFunction, @@ -426,7 +436,7 @@ function* runWithEnvironment( if (env.config.enableChangeDetectionForDebugging != null) { pruneInitializationDependencies(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PruneInitializationDependencies', value: reactiveFunction, @@ -434,49 +444,49 @@ function* runWithEnvironment( } propagateEarlyReturns(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PropagateEarlyReturns', value: reactiveFunction, }); pruneUnusedLValues(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PruneUnusedLValues', value: reactiveFunction, }); promoteUsedTemporaries(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PromoteUsedTemporaries', value: reactiveFunction, }); extractScopeDeclarationsFromDestructuring(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'ExtractScopeDeclarationsFromDestructuring', value: reactiveFunction, }); stabilizeBlockIds(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'StabilizeBlockIds', value: reactiveFunction, }); const uniqueIdentifiers = renameVariables(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'RenameVariables', value: reactiveFunction, }); pruneHoistedContexts(reactiveFunction); - yield log({ + log({ kind: 'reactive', name: 'PruneHoistedContexts', value: reactiveFunction, @@ -497,9 +507,9 @@ function* runWithEnvironment( uniqueIdentifiers, fbtOperands, }).unwrap(); - yield log({kind: 'ast', name: 'Codegen', value: ast}); + log({kind: 'ast', name: 'Codegen', value: ast}); for (const outlined of ast.outlined) { - yield log({kind: 'ast', name: 'Codegen (outlined)', value: outlined.fn}); + log({kind: 'ast', name: 'Codegen (outlined)', value: outlined.fn}); } /** @@ -525,7 +535,7 @@ export function compileFn( filename: string | null, code: string | null, ): CodegenFunction { - let generator = run( + return run( func, config, fnType, @@ -534,15 +544,9 @@ export function compileFn( filename, code, ); - while (true) { - const next = generator.next(); - if (next.done) { - return next.value; - } - } } -export function log(value: CompilerPipelineValue): CompilerPipelineValue { +function printLog(value: CompilerPipelineValue): CompilerPipelineValue { switch (value.kind) { case 'ast': { logCodegenFunction(value.name, value.value); @@ -566,14 +570,3 @@ export function log(value: CompilerPipelineValue): CompilerPipelineValue { } return value; } - -export function* runPlayground( - func: NodePath< - t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression - >, - config: EnvironmentConfig, - fnType: ReactFunctionType, -): Generator { - const ast = yield* run(func, config, fnType, '_c', null, null, null); - return ast; -} 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 fa581d8ed8..48589b8be9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -168,11 +168,19 @@ const EnvironmentConfigSchema = z.object({ customMacros: z.nullable(z.array(MacroSchema)).default(null), /** - * Enable a check that resets the memoization cache when the source code of the file changes. - * This is intended to support hot module reloading (HMR), where the same runtime component - * instance will be reused across different versions of the component source. + * Enable a check that resets the memoization cache when the source code of + * the file changes. This is intended to support hot module reloading (HMR), + * where the same runtime component instance will be reused across different + * versions of the component source. + * + * When set to + * - true: code for HMR support is always generated, regardless of NODE_ENV + * or `globalThis.__DEV__` + * - false: code for HMR support is not generated + * - null: (default) code for HMR support is conditionally generated dependent + * on `NODE_ENV` and `globalThis.__DEV__` at the time of compilation. */ - enableResetCacheOnSourceFileChanges: z.boolean().default(false), + enableResetCacheOnSourceFileChanges: z.nullable(z.boolean()).default(null), /** * Enable using information from existing useMemo/useCallback to understand when a value is done @@ -708,7 +716,10 @@ export function parseConfigPragmaForTests(pragma: string): EnvironmentConfig { continue; } - if (typeof defaultConfig[key as keyof EnvironmentConfig] !== 'boolean') { + if ( + key !== 'enableResetCacheOnSourceFileChanges' && + typeof defaultConfig[key as keyof EnvironmentConfig] !== 'boolean' + ) { // skip parsing non-boolean properties continue; } @@ -718,9 +729,15 @@ export function parseConfigPragmaForTests(pragma: string): EnvironmentConfig { maybeConfig[key] = false; } } - const config = EnvironmentConfigSchema.safeParse(maybeConfig); if (config.success) { + /** + * Unless explicitly enabled, do not insert HMR handling code + * in test fixtures or playground to reduce visual noise. + */ + if (config.data.enableResetCacheOnSourceFileChanges == null) { + config.data.enableResetCacheOnSourceFileChanges = false; + } return config.data; } CompilerError.invariant(false, { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-no-memo-module-scope-usememo-function-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-no-memo-module-scope-usememo-function-scope.expect.md new file mode 100644 index 0000000000..69c1b9bbbb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-no-memo-module-scope-usememo-function-scope.expect.md @@ -0,0 +1,29 @@ + +## Input + +```javascript +// @compilationMode(all) +'use no memo'; + +function TestComponent({x}) { + 'use memo'; + return ; +} + +``` + +## Code + +```javascript +// @compilationMode(all) +"use no memo"; + +function TestComponent({ x }) { + "use memo"; + return ; +} + +``` + +### 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/use-no-memo-module-scope-usememo-function-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-no-memo-module-scope-usememo-function-scope.js new file mode 100644 index 0000000000..9b314e1f99 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-no-memo-module-scope-usememo-function-scope.js @@ -0,0 +1,7 @@ +// @compilationMode(all) +'use no memo'; + +function TestComponent({x}) { + 'use memo'; + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/parseConfigPragma-test.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/parseConfigPragma-test.ts index d634bd235f..dc4d5d25a4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/parseConfigPragma-test.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/parseConfigPragma-test.ts @@ -25,6 +25,7 @@ describe('parseConfigPragmaForTests()', () => { enableUseTypeAnnotations: true, validateNoSetStateInPassiveEffects: true, validateNoSetStateInRender: false, + enableResetCacheOnSourceFileChanges: false, }); }); }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/index.ts b/compiler/packages/babel-plugin-react-compiler/src/index.ts index 150df26e45..188c244d9e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/index.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/index.ts @@ -17,8 +17,6 @@ export { compileFn as compile, compileProgram, parsePluginOptions, - run, - runPlayground, OPT_OUT_DIRECTIVES, OPT_IN_DIRECTIVES, findDirectiveEnablingMemoization, From 80b81fe56353f7419bd07ecfc3534d274a413fca Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Mon, 16 Dec 2024 14:43:34 -0500 Subject: [PATCH 6/7] [compiler] Repro for aliased captures within inner function expressions (#31770) see fixture --- ...-func-maybealias-captured-mutate.expect.md | 129 ++++++++++++++++++ ...pturing-func-maybealias-captured-mutate.ts | 48 +++++++ .../packages/snap/src/SproutTodoFilter.ts | 1 + 3 files changed, 178 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-capturing-func-maybealias-captured-mutate.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-capturing-func-maybealias-captured-mutate.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-capturing-func-maybealias-captured-mutate.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-capturing-func-maybealias-captured-mutate.expect.md new file mode 100644 index 0000000000..b8c7f8d422 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-capturing-func-maybealias-captured-mutate.expect.md @@ -0,0 +1,129 @@ + +## Input + +```javascript +import {makeArray, mutate} from 'shared-runtime'; + +/** + * Bug repro: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe"}} + * Forget: + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe","wat1":"joe"}} + * + * Fork of `capturing-func-alias-captured-mutate`, but instead of directly + * aliasing `y` via `[y]`, we make an opaque call. + * + * Note that the bug here is that we don't infer that `a = makeArray(y)` + * potentially captures a context variable into a local variable. As a result, + * we don't understand that `a[0].x = b` captures `x` into `y` -- instead, we're + * currently inferring that this lambda captures `y` (for a potential later + * mutation) and simply reads `x`. + * + * Concretely `InferReferenceEffects.hasContextRefOperand` is incorrectly not + * used when we analyze CallExpressions. + */ +function Component({foo, bar}: {foo: number; bar: number}) { + let x = {foo}; + let y: {bar: number; x?: {foo: number}} = {bar}; + const f0 = function () { + let a = makeArray(y); // a = [y] + let b = x; + // this writes y.x = x + a[0].x = b; + }; + f0(); + mutate(y.x); + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{foo: 3, bar: 4}], + sequentialRenders: [ + {foo: 3, bar: 4}, + {foo: 3, bar: 5}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray, mutate } from "shared-runtime"; + +/** + * Bug repro: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe"}} + * Forget: + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe","wat1":"joe"}} + * + * Fork of `capturing-func-alias-captured-mutate`, but instead of directly + * aliasing `y` via `[y]`, we make an opaque call. + * + * Note that the bug here is that we don't infer that `a = makeArray(y)` + * potentially captures a context variable into a local variable. As a result, + * we don't understand that `a[0].x = b` captures `x` into `y` -- instead, we're + * currently inferring that this lambda captures `y` (for a potential later + * mutation) and simply reads `x`. + * + * Concretely `InferReferenceEffects.hasContextRefOperand` is incorrectly not + * used when we analyze CallExpressions. + */ +function Component(t0) { + const $ = _c(5); + const { foo, bar } = t0; + let t1; + if ($[0] !== foo) { + t1 = { foo }; + $[0] = foo; + $[1] = t1; + } else { + t1 = $[1]; + } + const x = t1; + let y; + if ($[2] !== bar || $[3] !== x) { + y = { bar }; + const f0 = function () { + const a = makeArray(y); + const b = x; + + a[0].x = b; + }; + + f0(); + mutate(y.x); + $[2] = bar; + $[3] = x; + $[4] = y; + } else { + y = $[4]; + } + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ foo: 3, bar: 4 }], + sequentialRenders: [ + { foo: 3, bar: 4 }, + { foo: 3, bar: 5 }, + ], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-capturing-func-maybealias-captured-mutate.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-capturing-func-maybealias-captured-mutate.ts new file mode 100644 index 0000000000..ca7076fda4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-capturing-func-maybealias-captured-mutate.ts @@ -0,0 +1,48 @@ +import {makeArray, mutate} from 'shared-runtime'; + +/** + * Bug repro: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe"}} + * Forget: + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe","wat1":"joe"}} + * + * Fork of `capturing-func-alias-captured-mutate`, but instead of directly + * aliasing `y` via `[y]`, we make an opaque call. + * + * Note that the bug here is that we don't infer that `a = makeArray(y)` + * potentially captures a context variable into a local variable. As a result, + * we don't understand that `a[0].x = b` captures `x` into `y` -- instead, we're + * currently inferring that this lambda captures `y` (for a potential later + * mutation) and simply reads `x`. + * + * Concretely `InferReferenceEffects.hasContextRefOperand` is incorrectly not + * used when we analyze CallExpressions. + */ +function Component({foo, bar}: {foo: number; bar: number}) { + let x = {foo}; + let y: {bar: number; x?: {foo: number}} = {bar}; + const f0 = function () { + let a = makeArray(y); // a = [y] + let b = x; + // this writes y.x = x + a[0].x = b; + }; + f0(); + mutate(y.x); + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{foo: 3, bar: 4}], + sequentialRenders: [ + {foo: 3, bar: 4}, + {foo: 3, bar: 5}, + ], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 36b3e92f96..3610707396 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -479,6 +479,7 @@ const skipFilter = new Set([ // bugs 'fbt/bug-fbt-plural-multiple-function-calls', 'fbt/bug-fbt-plural-multiple-mixed-call-tag', + `bug-capturing-func-maybealias-captured-mutate`, 'bug-object-expression-computed-key-modified-during-after-construction-hoisted-sequence-expr', 'bug-invalid-hoisting-functionexpr', 'bug-aliased-capture-aliased-mutate', From 2764403f05f50f10efcce63227167056c949d646 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 16 Dec 2024 15:10:32 -0500 Subject: [PATCH 7/7] [compiler][be] Logger based debug printing in test runner Avoid mutable logging enabled state and writing to `process.stdout` within our babel transform. --- .../babel-plugin-react-compiler/package.json | 2 - .../src/Entrypoint/Pipeline.ts | 44 +------ .../src/Inference/AnalyseFunctions.ts | 7 +- .../InferReactiveScopeVariables.ts | 7 +- .../src/Utils/logger.ts | 110 ------------------ compiler/packages/snap/src/compiler.ts | 22 ++-- compiler/packages/snap/src/constants.ts | 12 +- compiler/packages/snap/src/runner-worker.ts | 47 +++++++- 8 files changed, 78 insertions(+), 173 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/Utils/logger.ts diff --git a/compiler/packages/babel-plugin-react-compiler/package.json b/compiler/packages/babel-plugin-react-compiler/package.json index 7d55e7b27f..158b800dba 100644 --- a/compiler/packages/babel-plugin-react-compiler/package.json +++ b/compiler/packages/babel-plugin-react-compiler/package.json @@ -42,9 +42,7 @@ "babel-jest": "^29.0.3", "babel-plugin-fbt": "^1.0.0", "babel-plugin-fbt-runtime": "^1.0.0", - "chalk": "4", "eslint": "^8.57.1", - "glob": "^7.1.6", "invariant": "^2.2.4", "jest": "^29.0.3", "jest-environment-jsdom": "^29.0.3", 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 6995aec7f1..c48cba32b2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -79,13 +79,6 @@ import { rewriteInstructionKindsBasedOnReassignment, } from '../SSA'; import {inferTypes} from '../TypeInference'; -import { - logCodegenFunction, - logDebug, - logHIRFunction, - logReactiveFunction, -} from '../Utils/logger'; -import {assertExhaustive} from '../Utils/utils'; import { validateContextVariableLValues, validateHooksUsage, @@ -139,13 +132,7 @@ function run( name: 'EnvironmentConfig', value: prettyFormat(env.config), }); - printLog({ - kind: 'debug', - name: 'EnvironmentConfig', - value: prettyFormat(env.config), - }); - const ast = runWithEnvironment(func, env); - return ast; + return runWithEnvironment(func, env); } /* @@ -158,10 +145,8 @@ function runWithEnvironment( >, env: Environment, ): CodegenFunction { - const log = (value: CompilerPipelineValue): CompilerPipelineValue => { - printLog(value); + const log = (value: CompilerPipelineValue): void => { env.logger?.debugLogIRs?.(value); - return value; }; const hir = lower(func, env).unwrap(); log({kind: 'hir', name: 'HIR', value: hir}); @@ -545,28 +530,3 @@ export function compileFn( code, ); } - -function printLog(value: CompilerPipelineValue): CompilerPipelineValue { - switch (value.kind) { - case 'ast': { - logCodegenFunction(value.name, value.value); - break; - } - case 'hir': { - logHIRFunction(value.name, value.value); - break; - } - case 'reactive': { - logReactiveFunction(value.name, value.value); - break; - } - case 'debug': { - logDebug(value.name, value.value); - break; - } - default: { - assertExhaustive(value, 'Unexpected compilation kind'); - } - } - return value; -} 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 684acaf298..75bd8f6811 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -19,7 +19,6 @@ import { import {deadCodeElimination} from '../Optimization'; import {inferReactiveScopeVariables} from '../ReactiveScopes'; import {rewriteInstructionKindsBasedOnReassignment} from '../SSA'; -import {logHIRFunction} from '../Utils/logger'; import {inferMutableContextVariables} from './InferMutableContextVariables'; import {inferMutableRanges} from './InferMutableRanges'; import inferReferenceEffects from './InferReferenceEffects'; @@ -112,7 +111,11 @@ function lower(func: HIRFunction): void { rewriteInstructionKindsBasedOnReassignment(func); inferReactiveScopeVariables(func); inferMutableContextVariables(func); - logHIRFunction('AnalyseFunction (inner)', func); + func.env.logger?.debugLogIRs?.({ + kind: 'hir', + name: 'AnalyseFunction (inner)', + value: func, + }); } function infer( 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 098139b150..1108422f07 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -25,7 +25,6 @@ import { eachPatternOperand, } from '../HIR/visitors'; import DisjointSet from '../Utils/DisjointSet'; -import {logHIRFunction} from '../Utils/logger'; import {assertExhaustive} from '../Utils/utils'; /* @@ -156,7 +155,11 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void { scope.range.end > maxInstruction + 1 ) { // Make it easier to debug why the error occurred - logHIRFunction('InferReactiveScopeVariables (invalid scope)', fn); + fn.env.logger?.debugLogIRs?.({ + kind: 'hir', + name: 'InferReactiveScopeVariables (invalid scope)', + value: fn, + }); CompilerError.invariant(false, { reason: `Invalid mutable range for scope`, loc: GeneratedSource, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Utils/logger.ts b/compiler/packages/babel-plugin-react-compiler/src/Utils/logger.ts deleted file mode 100644 index fa43a8befe..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/Utils/logger.ts +++ /dev/null @@ -1,110 +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 generate from '@babel/generator'; -import * as t from '@babel/types'; -import chalk from 'chalk'; -import {HIR, HIRFunction, ReactiveFunction} from '../HIR/HIR'; -import {printFunctionWithOutlined, printHIR} from '../HIR/PrintHIR'; -import {CodegenFunction} from '../ReactiveScopes'; -import {printReactiveFunctionWithOutlined} from '../ReactiveScopes/PrintReactiveFunction'; - -let ENABLED: boolean = false; - -let lastLogged: string; - -export function toggleLogging(enabled: boolean): void { - ENABLED = enabled; -} - -export function logDebug(step: string, value: string): void { - if (ENABLED) { - process.stdout.write(`${chalk.green(step)}:\n${value}\n\n`); - } -} - -export function logHIR(step: string, ir: HIR): void { - if (ENABLED) { - const printed = printHIR(ir); - if (printed !== lastLogged) { - lastLogged = printed; - process.stdout.write(`${chalk.green(step)}:\n${printed}\n\n`); - } else { - process.stdout.write(`${chalk.blue(step)}: (no change)\n\n`); - } - } -} - -export function logCodegenFunction(step: string, fn: CodegenFunction): void { - if (ENABLED) { - let printed: string | null = null; - try { - const node = t.functionDeclaration( - fn.id, - fn.params, - fn.body, - fn.generator, - fn.async, - ); - const ast = generate(node); - printed = ast.code; - } catch (e) { - let errMsg: string; - if ( - typeof e === 'object' && - e != null && - 'message' in e && - typeof e.message === 'string' - ) { - errMsg = e.message.toString(); - } else { - errMsg = '[empty]'; - } - console.log('Error formatting AST: ' + errMsg); - } - if (printed === null) { - return; - } - if (printed !== lastLogged) { - lastLogged = printed; - process.stdout.write(`${chalk.green(step)}:\n${printed}\n\n`); - } else { - process.stdout.write(`${chalk.blue(step)}: (no change)\n\n`); - } - } -} - -export function logHIRFunction(step: string, fn: HIRFunction): void { - if (ENABLED) { - const printed = printFunctionWithOutlined(fn); - if (printed !== lastLogged) { - lastLogged = printed; - process.stdout.write(`${chalk.green(step)}:\n${printed}\n\n`); - } else { - process.stdout.write(`${chalk.blue(step)}: (no change)\n\n`); - } - } -} - -export function logReactiveFunction(step: string, fn: ReactiveFunction): void { - if (ENABLED) { - const printed = printReactiveFunctionWithOutlined(fn); - if (printed !== lastLogged) { - lastLogged = printed; - process.stdout.write(`${chalk.green(step)}:\n${printed}\n\n`); - } else { - process.stdout.write(`${chalk.blue(step)}: (no change)\n\n`); - } - } -} - -export function log(fn: () => string): void { - if (ENABLED) { - const message = fn(); - process.stdout.write(message.trim() + '\n\n'); - } -} diff --git a/compiler/packages/snap/src/compiler.ts b/compiler/packages/snap/src/compiler.ts index 1cb8fe48b9..5866445d2e 100644 --- a/compiler/packages/snap/src/compiler.ts +++ b/compiler/packages/snap/src/compiler.ts @@ -19,6 +19,7 @@ import type { PanicThresholdOptions, PluginOptions, CompilerReactTarget, + CompilerPipelineValue, } from 'babel-plugin-react-compiler/src/Entrypoint'; import type {Effect, ValueKind} from 'babel-plugin-react-compiler/src/HIR'; import type { @@ -45,6 +46,7 @@ export function parseLanguage(source: string): 'flow' | 'typescript' { function makePluginOptions( firstLine: string, parseConfigPragmaFn: typeof ParseConfigPragma, + debugIRLogger: (value: CompilerPipelineValue) => void, EffectEnum: typeof Effect, ValueKindEnum: typeof ValueKind, ): [PluginOptions, Array<{filename: string | null; event: LoggerEvent}>] { @@ -182,15 +184,15 @@ function makePluginOptions( .filter(s => s.length > 0); } - let logs: Array<{filename: string | null; event: LoggerEvent}> = []; - let logger: Logger | null = null; - if (firstLine.includes('@logger')) { - logger = { - logEvent(filename: string | null, event: LoggerEvent): void { - logs.push({filename, event}); - }, - }; - } + const logs: Array<{filename: string | null; event: LoggerEvent}> = []; + const logger: Logger = { + logEvent: firstLine.includes('@logger') + ? (filename, event) => { + logs.push({filename, event}); + } + : () => {}, + debugLogIRs: debugIRLogger, + }; const config = parseConfigPragmaFn(firstLine); const options = { @@ -338,6 +340,7 @@ export async function transformFixtureInput( parseConfigPragmaFn: typeof ParseConfigPragma, plugin: BabelCore.PluginObj, includeEvaluator: boolean, + debugIRLogger: (value: CompilerPipelineValue) => void, EffectEnum: typeof Effect, ValueKindEnum: typeof ValueKind, ): Promise<{kind: 'ok'; value: TransformResult} | {kind: 'err'; msg: string}> { @@ -365,6 +368,7 @@ export async function transformFixtureInput( const [options, logs] = makePluginOptions( firstLine, parseConfigPragmaFn, + debugIRLogger, EffectEnum, ValueKindEnum, ); diff --git a/compiler/packages/snap/src/constants.ts b/compiler/packages/snap/src/constants.ts index abee06c55b..ad77441b53 100644 --- a/compiler/packages/snap/src/constants.ts +++ b/compiler/packages/snap/src/constants.ts @@ -18,11 +18,17 @@ export const COMPILER_PATH = path.join( 'BabelPlugin.js', ); export const COMPILER_INDEX_PATH = path.join(process.cwd(), 'dist', 'index'); -export const LOGGER_PATH = path.join( +export const PRINT_HIR_PATH = path.join( process.cwd(), 'dist', - 'Utils', - 'logger.js', + 'HIR', + 'PrintHIR.js', +); +export const PRINT_REACTIVE_IR_PATH = path.join( + process.cwd(), + 'dist', + 'ReactiveScopes', + 'PrintReactiveFunction.js', ); export const PARSE_CONFIG_PRAGMA_PATH = path.join( process.cwd(), diff --git a/compiler/packages/snap/src/runner-worker.ts b/compiler/packages/snap/src/runner-worker.ts index f05757d3df..ea87cd1e91 100644 --- a/compiler/packages/snap/src/runner-worker.ts +++ b/compiler/packages/snap/src/runner-worker.ts @@ -8,16 +8,21 @@ import {codeFrameColumns} from '@babel/code-frame'; import type {PluginObj} from '@babel/core'; import type {parseConfigPragmaForTests as ParseConfigPragma} from 'babel-plugin-react-compiler/src/HIR/Environment'; +import type {printFunctionWithOutlined as PrintFunctionWithOutlined} from 'babel-plugin-react-compiler/src/HIR/PrintHIR'; +import type {printReactiveFunctionWithOutlined as PrintReactiveFunctionWithOutlined} from 'babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction'; import {TransformResult, transformFixtureInput} from './compiler'; import { COMPILER_PATH, COMPILER_INDEX_PATH, - LOGGER_PATH, PARSE_CONFIG_PRAGMA_PATH, + PRINT_HIR_PATH, + PRINT_REACTIVE_IR_PATH, } from './constants'; import {TestFixture, getBasename, isExpectError} from './fixture-utils'; import {TestResult, writeOutputToString} from './reporter'; import {runSprout} from './sprout'; +import {CompilerPipelineValue} from 'babel-plugin-react-compiler/src'; +import chalk from 'chalk'; const originalConsoleError = console.error; @@ -64,20 +69,56 @@ async function compile( const {Effect: EffectEnum, ValueKind: ValueKindEnum} = require( COMPILER_INDEX_PATH, ); - const {toggleLogging} = require(LOGGER_PATH); + const {printFunctionWithOutlined} = require(PRINT_HIR_PATH) as { + printFunctionWithOutlined: typeof PrintFunctionWithOutlined; + }; + const {printReactiveFunctionWithOutlined} = require( + PRINT_REACTIVE_IR_PATH, + ) as { + printReactiveFunctionWithOutlined: typeof PrintReactiveFunctionWithOutlined; + }; + + let lastLogged: string | null = null; + const debugIRLogger = shouldLog + ? (value: CompilerPipelineValue) => { + let printed: string; + switch (value.kind) { + case 'hir': + printed = printFunctionWithOutlined(value.value); + break; + case 'reactive': + printed = printReactiveFunctionWithOutlined(value.value); + break; + case 'debug': + printed = value.value; + break; + case 'ast': + // skip printing ast as we already write fixture output JS + printed = '(ast)'; + break; + } + + if (printed !== lastLogged) { + lastLogged = printed; + console.log(`${chalk.green(value.name)}:\n ${printed}\n`); + } else { + console.log(`${chalk.blue(value.name)}: (no change)\n`); + } + } + : () => {}; const {parseConfigPragmaForTests} = require(PARSE_CONFIG_PRAGMA_PATH) as { parseConfigPragmaForTests: typeof ParseConfigPragma; }; // only try logging if we filtered out all but one fixture, // since console log order is non-deterministic - toggleLogging(shouldLog); const result = await transformFixtureInput( input, fixturePath, parseConfigPragmaForTests, BabelPluginReactCompiler, includeEvaluator, + debugIRLogger, EffectEnum, ValueKindEnum, );