From b4c38015d0f8b58e9d0890dd7fd00ad763d3f965 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 12 Aug 2024 12:32:55 -0400 Subject: [PATCH 1/6] [DevTools] Remove lodash.throttle (#30657) Same principle as #30555. We shouldn't be throttling the UI to make it feel less snappy. Instead, we should use back-pressure to handle it. Normally the browser handles it automatically with frame aligned events. E.g. if the thread can't keep up with sync updates it doesn't send each event but the next one. E.g. pointermove or resize. However, it is possible that we end up queuing too many events if the frontend can't keep up but the solution to this is the same as mentioned in #30555. I.e. to track the last message and only send after we get a response. I still keep the throttle to persist the selection since that affects the disk usage and doesn't have direct UX effects. The main motivation for this change though is that lodash throttle doesn't rely on timers but Date.now which makes it incompatible with most jest helpers which means I can't write tests against these functions properly. --- packages/react-devtools-shared/package.json | 2 - .../src/backend/agent.js | 48 +++++++++++-------- .../src/backend/views/Highlighter/index.js | 20 +++----- .../src/devtools/views/hooks.js | 7 +-- 4 files changed, 36 insertions(+), 41 deletions(-) diff --git a/packages/react-devtools-shared/package.json b/packages/react-devtools-shared/package.json index e503f017fe71e..94f3f9411b0db 100644 --- a/packages/react-devtools-shared/package.json +++ b/packages/react-devtools-shared/package.json @@ -22,8 +22,6 @@ "jsc-safe-url": "^0.2.4", "json5": "^2.1.3", "local-storage-fallback": "^4.1.1", - "lodash.throttle": "^4.1.1", - "memoize-one": "^3.1.1", "react-virtualized-auto-sizer": "^1.0.23", "react-window": "^1.8.10" } diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 886435c289f61..6900e4510d3f9 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -8,7 +8,6 @@ */ import EventEmitter from '../events'; -import throttle from 'lodash.throttle'; import { SESSION_STORAGE_LAST_SELECTION_KEY, SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, @@ -483,7 +482,13 @@ export default class Agent extends EventEmitter<{ this._persistedSelection = null; this._persistedSelectionMatch = null; renderer.setTrackedPath(null); - this._throttledPersistSelection(rendererID, id); + // Throttle persisting the selection. + this._lastSelectedElementID = id; + this._lastSelectedRendererID = rendererID; + if (!this._persistSelectionTimerScheduled) { + this._persistSelectionTimerScheduled = true; + setTimeout(this._persistSelection, 1000); + } } // TODO: If there was a way to change the selected DOM element @@ -893,22 +898,25 @@ export default class Agent extends EventEmitter<{ this._bridge.send('unsupportedRendererVersion', rendererID); } - _throttledPersistSelection: any = throttle( - (rendererID: number, id: number) => { - // This is throttled, so both renderer and selected ID - // might not be available by the time we read them. - // This is why we need the defensive checks here. - const renderer = this._rendererInterfaces[rendererID]; - const path = renderer != null ? renderer.getPathForElement(id) : null; - if (path !== null) { - sessionStorageSetItem( - SESSION_STORAGE_LAST_SELECTION_KEY, - JSON.stringify(({rendererID, path}: PersistedSelection)), - ); - } else { - sessionStorageRemoveItem(SESSION_STORAGE_LAST_SELECTION_KEY); - } - }, - 1000, - ); + _persistSelectionTimerScheduled: boolean = false; + _lastSelectedRendererID: number = -1; + _lastSelectedElementID: number = -1; + _persistSelection: any = () => { + this._persistSelectionTimerScheduled = false; + const rendererID = this._lastSelectedRendererID; + const id = this._lastSelectedElementID; + // This is throttled, so both renderer and selected ID + // might not be available by the time we read them. + // This is why we need the defensive checks here. + const renderer = this._rendererInterfaces[rendererID]; + const path = renderer != null ? renderer.getPathForElement(id) : null; + if (path !== null) { + sessionStorageSetItem( + SESSION_STORAGE_LAST_SELECTION_KEY, + JSON.stringify(({rendererID, path}: PersistedSelection)), + ); + } else { + sessionStorageRemoveItem(SESSION_STORAGE_LAST_SELECTION_KEY); + } + }; } diff --git a/packages/react-devtools-shared/src/backend/views/Highlighter/index.js b/packages/react-devtools-shared/src/backend/views/Highlighter/index.js index 7fefa837e2fc5..419fefc4ffcf8 100644 --- a/packages/react-devtools-shared/src/backend/views/Highlighter/index.js +++ b/packages/react-devtools-shared/src/backend/views/Highlighter/index.js @@ -7,8 +7,6 @@ * @flow */ -import memoize from 'memoize-one'; -import throttle from 'lodash.throttle'; import Agent from 'react-devtools-shared/src/backend/agent'; import {hideOverlay, showOverlay} from './Highlighter'; @@ -189,18 +187,12 @@ export default function setupHighlighter( event.stopPropagation(); } - const selectElementForNode = throttle( - memoize((node: HTMLElement) => { - const id = agent.getIDForHostInstance(node); - if (id !== null) { - bridge.send('selectElement', id); - } - }), - 200, - // Don't change the selection in the very first 200ms - // because those are usually unintentional as you lift the cursor. - {leading: false}, - ); + const selectElementForNode = (node: HTMLElement) => { + const id = agent.getIDForHostInstance(node); + if (id !== null) { + bridge.send('selectElement', id); + } + }; function getEventTarget(event: MouseEvent): HTMLElement { if (event.composed) { diff --git a/packages/react-devtools-shared/src/devtools/views/hooks.js b/packages/react-devtools-shared/src/devtools/views/hooks.js index 6d0afb5c0a0ca..f2debdfa8f580 100644 --- a/packages/react-devtools-shared/src/devtools/views/hooks.js +++ b/packages/react-devtools-shared/src/devtools/views/hooks.js @@ -7,7 +7,6 @@ * @flow */ -import throttle from 'lodash.throttle'; import { useCallback, useEffect, @@ -125,10 +124,8 @@ export function useIsOverflowing( const container = ((containerRef.current: any): HTMLDivElement); - const handleResize = throttle( - () => setIsOverflowing(container.clientWidth <= totalChildWidth), - 100, - ); + const handleResize = () => + setIsOverflowing(container.clientWidth <= totalChildWidth); handleResize(); From 25c584f5672d90ba67d72ea9ba9cc06b12311806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 12 Aug 2024 12:41:29 -0400 Subject: [PATCH 2/6] [DevTools] Further Refactoring of Unmounts (#30658) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #30625 and #30657. This ensures that we only create instances during the commit reconciliation and that we don't create unnecessary instances for things that are filtered or not mounted. This ensures that we also can rely on the reconciliation to do all the clean up. Now everything is created and deleted as a pair in the same pass. Previously we were including unfiltered components in the owner stack which probably doesn't make sense since you're intending to filter them everywhere presumably. However, it also means that those links were broken since you can't link into owners that don't exist in the parent tree. The main complication is the component filters. It relied on not unmounting the old instances. I had to update some tests that asserted on ids that are now shifted. For warnings/errors tracking I now restore them back into the pending set when they unmount. Basically it puts them back into their "pre-commit" state. That way when they remount they’re still there. For restoring the current selection I use the tracked path mechanism instead of relying on the id being unchanged. This is better anyway because if you filter out the currently selected item it's better to select the nearest match instead of just losing the selection. --- .../src/__tests__/inspectedElement-test.js | 140 ++++++++- .../src/__tests__/ownersListContext-test.js | 1 - .../src/__tests__/profilingCache-test.js | 8 +- .../src/__tests__/treeContext-test.js | 85 ------ .../src/backend/agent.js | 15 +- .../src/backend/fiber/renderer.js | 272 +++++++++--------- 6 files changed, 295 insertions(+), 226 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index ee6c4431b9526..a4cc492c83efc 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -34,6 +34,8 @@ describe('InspectedElement', () => { let SettingsContextController; let StoreContext; let TreeContextController; + let TreeStateContext; + let TreeDispatcherContext; let TestUtilsAct; let TestRendererAct; @@ -73,6 +75,10 @@ describe('InspectedElement', () => { require('react-devtools-shared/src/devtools/views/context').StoreContext; TreeContextController = require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeContextController; + TreeStateContext = + require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeStateContext; + TreeDispatcherContext = + require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeDispatcherContext; // Used by inspectElementAtIndex() helper function utils.act(() => { @@ -2142,7 +2148,7 @@ describe('InspectedElement', () => { "context": null, "events": undefined, "hooks": null, - "id": 2, + "id": 4, "owners": null, "props": {}, "rootType": "createRoot()", @@ -2893,7 +2899,7 @@ describe('InspectedElement', () => { "compiledWithForget": false, "displayName": "Child", "hocDisplayNames": null, - "id": 5, + "id": 8, "key": null, "type": 5, }, @@ -2901,7 +2907,7 @@ describe('InspectedElement', () => { "compiledWithForget": false, "displayName": "App", "hocDisplayNames": null, - "id": 4, + "id": 7, "key": null, "type": 5, }, @@ -3016,4 +3022,132 @@ describe('InspectedElement', () => { ); }); }); + + it('should properly handle when components filters are updated', async () => { + const Wrapper = ({children}) => children; + + let state; + let dispatch; + const Capture = () => { + dispatch = React.useContext(TreeDispatcherContext); + state = React.useContext(TreeStateContext); + return null; + }; + + function Child({logError = false, logWarning = false}) { + if (logError === true) { + console.error('test-only: error'); + } + if (logWarning === true) { + console.warn('test-only: warning'); + } + return null; + } + + async function selectNextErrorOrWarning() { + await utils.actAsync( + () => + dispatch({type: 'SELECT_NEXT_ELEMENT_WITH_ERROR_OR_WARNING_IN_TREE'}), + false, + ); + } + + async function selectPreviousErrorOrWarning() { + await utils.actAsync( + () => + dispatch({ + type: 'SELECT_PREVIOUS_ELEMENT_WITH_ERROR_OR_WARNING_IN_TREE', + }), + false, + ); + } + + withErrorsOrWarningsIgnored(['test-only:'], () => + utils.act(() => + render( + + + + + + + + + + , + ), + ), + ); + + utils.act(() => + TestRenderer.create( + + + , + ), + ); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ▾ + ⚠ + ▾ + ▾ + ⚠ + `); + + await selectNextErrorOrWarning(); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ▾ + → ⚠ + ▾ + ▾ + ⚠ + `); + + await utils.actAsync(() => { + store.componentFilters = [utils.createDisplayNameFilter('Wrapper')]; + }, false); + + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + → ⚠ + ⚠ + `); + + await selectNextErrorOrWarning(); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ⚠ + → ⚠ + `); + + await utils.actAsync(() => { + store.componentFilters = []; + }, false); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ▾ + ⚠ + ▾ + ▾ + → ⚠ + `); + + await selectPreviousErrorOrWarning(); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ▾ + → ⚠ + ▾ + ▾ + ⚠ + `); + }); }); diff --git a/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js b/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js index 599492bed54a1..2792541eace13 100644 --- a/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js @@ -156,7 +156,6 @@ describe('OwnersListContext', () => { expect(await getOwnersListForOwner(firstChild)).toMatchInlineSnapshot(` [ "Grandparent", - "Parent", "Child", ] `); diff --git a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js index 90075ec7ea066..4778daffcec2b 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js @@ -1251,8 +1251,8 @@ describe('ProfilingCache', () => { Map { 1 => 16, 2 => 16, + 3 => 1, 4 => 1, - 6 => 1, } `); @@ -1260,8 +1260,8 @@ describe('ProfilingCache', () => { Map { 1 => 0, 2 => 10, + 3 => 1, 4 => 1, - 6 => 1, } `); }); @@ -1322,13 +1322,13 @@ describe('ProfilingCache', () => { `); expect(commitData[1].fiberActualDurations).toMatchInlineSnapshot(` Map { - 7 => 3, + 5 => 3, 3 => 3, } `); expect(commitData[1].fiberSelfDurations).toMatchInlineSnapshot(` Map { - 7 => 3, + 5 => 3, 3 => 0, } `); diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 8bd5a8e17fa0b..fa2031c6b5c8d 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -2286,91 +2286,6 @@ describe('TreeListContext', () => { `); }); - it('should properly handle when components filters are updated', () => { - const Wrapper = ({children}) => children; - - withErrorsOrWarningsIgnored(['test-only:'], () => - utils.act(() => - render( - - - - - - - - - - , - ), - ), - ); - - utils.act(() => TestRenderer.create()); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ▾ - ⚠ - ▾ - ▾ - ⚠ - `); - - selectNextErrorOrWarning(); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ▾ - → ⚠ - ▾ - ▾ - ⚠ - `); - - utils.act(() => { - store.componentFilters = [utils.createDisplayNameFilter('Wrapper')]; - }); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - → ⚠ - ⚠ - `); - - selectNextErrorOrWarning(); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ⚠ - → ⚠ - `); - - utils.act(() => { - store.componentFilters = []; - }); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ▾ - ⚠ - ▾ - ▾ - → ⚠ - `); - - selectPreviousErrorOrWarning(); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ▾ - → ⚠ - ▾ - ▾ - ⚠ - `); - }); - it('should preserve errors for fibers even if they are filtered out of the tree initially', () => { const Wrapper = ({children}) => children; diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 6900e4510d3f9..e81af56ebdf76 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -795,10 +795,23 @@ export default class Agent extends EventEmitter<{ updateComponentFilters: (componentFilters: Array) => void = componentFilters => { - for (const rendererID in this._rendererInterfaces) { + for (const rendererIDString in this._rendererInterfaces) { + const rendererID = +rendererIDString; const renderer = ((this._rendererInterfaces[ (rendererID: any) ]: any): RendererInterface); + if (this._lastSelectedRendererID === rendererID) { + // Changing component filters will unmount and remount the DevTools tree. + // Track the last selection's path so we can restore the selection. + const path = renderer.getPathForElement(this._lastSelectedElementID); + if (path !== null) { + renderer.setTrackedPath(path); + this._persistedSelection = { + rendererID, + path, + }; + } + } renderer.updateComponentFilters(componentFilters); } }; diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index d5b016d387358..d1bb0429e7c48 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -63,7 +63,6 @@ import { SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, TREE_OPERATION_ADD, TREE_OPERATION_REMOVE, - TREE_OPERATION_REMOVE_ROOT, TREE_OPERATION_REORDER_CHILDREN, TREE_OPERATION_SET_SUBTREE_MODE, TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS, @@ -856,8 +855,14 @@ export function attach( // (due to e.g. Suspense or error boundaries). // onErrorOrWarning() adds Fibers and recordPendingErrorsAndWarnings() later clears them. const fibersWithChangedErrorOrWarningCounts: Set = new Set(); - const pendingFiberToErrorsMap: Map> = new Map(); - const pendingFiberToWarningsMap: Map> = new Map(); + const pendingFiberToErrorsMap: WeakMap< + Fiber, + Map, + > = new WeakMap(); + const pendingFiberToWarningsMap: WeakMap< + Fiber, + Map, + > = new WeakMap(); function clearErrorsAndWarnings() { // eslint-disable-next-line no-for-of-loops/no-for-of-loops @@ -876,7 +881,7 @@ export function attach( function clearMessageCountHelper( instanceID: number, - pendingFiberToMessageCountMap: Map>, + pendingFiberToMessageCountMap: WeakMap>, forError: boolean, ) { const devtoolsInstance = idToDevToolsInstanceMap.get(instanceID); @@ -902,7 +907,7 @@ export function attach( if (devtoolsInstance.kind === FIBER_INSTANCE) { const fiber = devtoolsInstance.data; // Throw out any pending changes. - pendingFiberToErrorsMap.delete(fiber); + pendingFiberToMessageCountMap.delete(fiber); if (changed) { // If previous flushed counts have changed, schedule an update too. @@ -1143,11 +1148,8 @@ export function attach( // Recursively unmount all roots. hook.getFiberRoots(rendererID).forEach(root => { - currentRootID = getOrGenerateFiberInstance(root.current).id; - // The TREE_OPERATION_REMOVE_ROOT operation serves two purposes: - // 1. It avoids sending unnecessary bridge traffic to clear a root. - // 2. It preserves Fiber IDs when remounting (below) which in turn ID to error/warning mapping. - pushOperation(TREE_OPERATION_REMOVE_ROOT); + currentRootID = getFiberInstanceThrows(root.current).id; + unmountFiberRecursively(root.current); flushPendingEvents(root); currentRootID = -1; }); @@ -1159,7 +1161,22 @@ export function attach( // Recursively re-mount all roots with new filter criteria applied. hook.getFiberRoots(rendererID).forEach(root => { - currentRootID = getOrGenerateFiberInstance(root.current).id; + const current = root.current; + const alternate = current.alternate; + const newRoot = createFiberInstance(current); + idToDevToolsInstanceMap.set(newRoot.id, newRoot); + fiberToFiberInstanceMap.set(current, newRoot); + if (alternate) { + fiberToFiberInstanceMap.set(alternate, newRoot); + } + + // Before the traversals, remember to start tracking + // our path in case we have selection to restore. + if (trackedPath !== null) { + mightBeOnTrackedPath = true; + } + + currentRootID = newRoot.id; setRootPseudoKey(currentRootID, root.current); mountFiberRecursively(root.current, false); flushPendingEvents(root); @@ -1322,43 +1339,6 @@ export function attach( // When a mount or update is in progress, this value tracks the root that is being operated on. let currentRootID: number = -1; - // Returns the unique ID for a Fiber or generates and caches a new one if the Fiber hasn't been seen before. - // Once this method has been called for a Fiber, untrackFiberID() should always be called later to avoid leaking. - function getOrGenerateFiberInstance(fiber: Fiber): FiberInstance { - let fiberInstance = fiberToFiberInstanceMap.get(fiber); - if (fiberInstance === undefined) { - const {alternate} = fiber; - if (alternate !== null) { - fiberInstance = fiberToFiberInstanceMap.get(alternate); - if (fiberInstance !== undefined) { - // We found the other pair, so we need to make sure we track the other side. - fiberToFiberInstanceMap.set(fiber, fiberInstance); - } - } - } - - let didGenerateID = false; - if (fiberInstance === undefined) { - didGenerateID = true; - fiberInstance = createFiberInstance(fiber); - fiberToFiberInstanceMap.set(fiber, fiberInstance); - idToDevToolsInstanceMap.set(fiberInstance.id, fiberInstance); - } - - if (__DEBUG__) { - if (didGenerateID) { - debug( - 'getOrGenerateFiberInstance()', - fiber, - fiberInstance.parent, - 'Generated a new UID', - ); - } - } - - return fiberInstance; - } - // Returns a FiberInstance if one has already been generated for the Fiber or throws. function getFiberInstanceThrows(fiber: Fiber): FiberInstance { const fiberInstance = getFiberInstanceUnsafe(fiber); @@ -1412,9 +1392,21 @@ export function attach( idToDevToolsInstanceMap.delete(fiberInstance.id); - // Also clear any errors/warnings associated with this fiber. - clearErrorsForElementID(fiberInstance.id); - clearWarningsForElementID(fiberInstance.id); + const fiber = fiberInstance.data; + + // Restore any errors/warnings associated with this fiber to the pending + // map. I.e. treat it as before we tracked the instances. This lets us + // restore them if we remount the same Fibers later. Otherwise we rely + // on the GC of the Fibers to clean them up. + if (fiberInstance.errors !== null) { + pendingFiberToErrorsMap.set(fiber, fiberInstance.errors); + fiberInstance.errors = null; + } + if (fiberInstance.warnings !== null) { + pendingFiberToWarningsMap.set(fiber, fiberInstance.warnings); + fiberInstance.warnings = null; + } + if (fiberInstance.flags & FORCE_ERROR) { fiberInstance.flags &= ~FORCE_ERROR; forceErrorCount--; @@ -1430,7 +1422,6 @@ export function attach( } } - const fiber = fiberInstance.data; fiberToFiberInstanceMap.delete(fiber); const {alternate} = fiber; if (alternate !== null) { @@ -1745,7 +1736,6 @@ export function attach( const pendingOperations: OperationsArray = []; const pendingRealUnmountedIDs: Array = []; - const pendingSimulatedUnmountedIDs: Array = []; let pendingOperationsQueue: Array | null = []; const pendingStringTable: Map = new Map(); let pendingStringTableLength: number = 0; @@ -1776,7 +1766,6 @@ export function attach( return ( pendingOperations.length === 0 && pendingRealUnmountedIDs.length === 0 && - pendingSimulatedUnmountedIDs.length === 0 && pendingUnmountedRootID === null ); } @@ -1856,7 +1845,7 @@ export function attach( function mergeMapsAndGetCountHelper( fiber: Fiber, fiberID: number, - pendingFiberToMessageCountMap: Map>, + pendingFiberToMessageCountMap: WeakMap>, forError: boolean, ): number { let newCount = 0; @@ -1932,18 +1921,18 @@ export function attach( pushOperation(fiberID); pushOperation(errorCount); pushOperation(warningCount); - } - // Always clean up so that we don't leak. - pendingFiberToErrorsMap.delete(fiber); - pendingFiberToWarningsMap.delete(fiber); + // Only clear the ones that we've already shown. Leave others in case + // they mount later. + pendingFiberToErrorsMap.delete(fiber); + pendingFiberToWarningsMap.delete(fiber); + } }); fibersWithChangedErrorOrWarningCounts.clear(); } function flushPendingEvents(root: Object): void { // Add any pending errors and warnings to the operations array. - // We do this just before flushing, so we can ignore errors for no-longer-mounted Fibers. recordPendingErrorsAndWarnings(); if (shouldBailoutWithPendingOperations()) { @@ -1960,7 +1949,6 @@ export function attach( const numUnmountIDs = pendingRealUnmountedIDs.length + - pendingSimulatedUnmountedIDs.length + (pendingUnmountedRootID === null ? 0 : 1); const operations = new Array( @@ -2013,15 +2001,6 @@ export function attach( for (let j = 0; j < pendingRealUnmountedIDs.length; j++) { operations[i++] = pendingRealUnmountedIDs[j]; } - // Fill in the simulated unmounts (hidden Suspense subtrees) in their order. - // (We want children to go before parents.) - // They go *after* the real unmounts because we know for sure they won't be - // children of already pushed "real" IDs. If they were, we wouldn't be able - // to discover them during the traversal, as they would have been deleted. - for (let j = 0; j < pendingSimulatedUnmountedIDs.length; j++) { - operations[i + j] = pendingSimulatedUnmountedIDs[j]; - } - i += pendingSimulatedUnmountedIDs.length; // The root ID should always be unmounted last. if (pendingUnmountedRootID !== null) { operations[i] = pendingUnmountedRootID; @@ -2040,7 +2019,6 @@ export function attach( // Reset all of the pending state now that we've told the frontend about it. pendingOperations.length = 0; pendingRealUnmountedIDs.length = 0; - pendingSimulatedUnmountedIDs.length = 0; pendingUnmountedRootID = null; pendingStringTable.clear(); pendingStringTableLength = 0; @@ -2078,7 +2056,24 @@ export function attach( parentInstance: DevToolsInstance | null, ): FiberInstance { const isRoot = fiber.tag === HostRoot; - const fiberInstance = getOrGenerateFiberInstance(fiber); + let fiberInstance; + if (isRoot) { + const entry = fiberToFiberInstanceMap.get(fiber); + if (entry === undefined) { + throw new Error('The root should have been registered at this point'); + } + fiberInstance = entry; + } else if ( + fiberToFiberInstanceMap.has(fiber) || + (fiber.alternate !== null && fiberToFiberInstanceMap.has(fiber.alternate)) + ) { + throw new Error('Did not expect to see this fiber being mounted twice.'); + } else { + fiberInstance = createFiberInstance(fiber); + } + fiberToFiberInstanceMap.set(fiber, fiberInstance); + idToDevToolsInstanceMap.set(fiberInstance.id, fiberInstance); + const id = fiberInstance.id; if (__DEBUG__) { @@ -2131,7 +2126,12 @@ export function attach( let ownerID: number; if (debugOwner != null) { if (typeof debugOwner.tag === 'number') { - ownerID = getOrGenerateFiberInstance((debugOwner: any)).id; + const ownerFiberInstance = getFiberInstanceUnsafe((debugOwner: any)); + if (ownerFiberInstance !== null) { + ownerID = ownerFiberInstance.id; + } else { + ownerID = 0; + } } else { // TODO: Track Server Component Owners. ownerID = 0; @@ -2183,17 +2183,9 @@ export function attach( return fiberInstance; } - function recordUnmount( - fiber: Fiber, - isSimulated: boolean, - ): null | FiberInstance { + function recordUnmount(fiber: Fiber): null | FiberInstance { if (__DEBUG__) { - debug( - 'recordUnmount()', - fiber, - null, - isSimulated ? 'unmount is simulated' : '', - ); + debug('recordUnmount()', fiber, null); } if (trackedPathMatchFiber !== null) { @@ -2223,28 +2215,22 @@ export function attach( const id = fiberInstance.id; const isRoot = fiber.tag === HostRoot; if (isRoot) { - // Roots must be removed only after all children (pending and simulated) have been removed. + // Roots must be removed only after all children have been removed. // So we track it separately. pendingUnmountedRootID = id; } else if (!shouldFilterFiber(fiber)) { // To maintain child-first ordering, // we'll push it into one of these queues, // and later arrange them in the correct order. - if (isSimulated) { - pendingSimulatedUnmountedIDs.push(id); - } else { - pendingRealUnmountedIDs.push(id); - } + pendingRealUnmountedIDs.push(id); } - if (!fiber._debugNeedsRemount) { - untrackFiber(fiberInstance); + untrackFiber(fiberInstance); - const isProfilingSupported = fiber.hasOwnProperty('treeBaseDuration'); - if (isProfilingSupported) { - idToRootMap.delete(id); - idToTreeBaseDurationMap.delete(id); - } + const isProfilingSupported = fiber.hasOwnProperty('treeBaseDuration'); + if (isProfilingSupported) { + idToRootMap.delete(id); + idToTreeBaseDurationMap.delete(id); } return fiberInstance; } @@ -2323,7 +2309,7 @@ export function attach( let child = remainingReconcilingChildren; while (child !== null) { if (child.kind === FIBER_INSTANCE) { - unmountFiberRecursively(child.data, false); + unmountFiberRecursively(child.data); } removeChild(child); child = remainingReconcilingChildren; @@ -2347,10 +2333,6 @@ export function attach( fiber: Fiber, traceNearestHostComponentUpdate: boolean, ): void { - // Generate an ID even for filtered Fibers, in case it's needed later (e.g. for Profiling). - // TODO: Do we really need to do this eagerly? - getOrGenerateFiberInstance(fiber); - if (__DEBUG__) { debug('mountFiberRecursively()', fiber, reconcilingParent); } @@ -2452,7 +2434,7 @@ export function attach( // We use this to simulate unmounting for Suspense trees // when we switch from primary to fallback, or deleting a subtree. - function unmountFiberRecursively(fiber: Fiber, isSimulated: boolean) { + function unmountFiberRecursively(fiber: Fiber) { if (__DEBUG__) { debug('unmountFiberRecursively()', fiber, null); } @@ -2493,7 +2475,7 @@ export function attach( child = fallbackChildFragment ? fallbackChildFragment.child : null; } - unmountChildrenRecursively(child, isSimulated); + unmountChildrenRecursively(child); } finally { if (shouldIncludeInTree) { reconcilingParent = stashedParent; @@ -2502,20 +2484,17 @@ export function attach( } } if (fiberInstance !== null) { - recordUnmount(fiber, isSimulated); + recordUnmount(fiber); removeChild(fiberInstance); } } - function unmountChildrenRecursively( - firstChild: null | Fiber, - isSimulated: boolean, - ) { + function unmountChildrenRecursively(firstChild: null | Fiber) { let child: null | Fiber = firstChild; while (child !== null) { // Record simulated unmounts children-first. // We skip nodes without return because those are real unmounts. - unmountFiberRecursively(child, isSimulated); + unmountFiberRecursively(child); child = child.sibling; } } @@ -2725,10 +2704,6 @@ export function attach( prevFiber: Fiber, traceNearestHostComponentUpdate: boolean, ): boolean { - // TODO: Do we really need to give this an instance eagerly if it's filtered? - const fiberInstance = getOrGenerateFiberInstance(nextFiber); - const id = fiberInstance.id; - if (__DEBUG__) { debug('updateFiberRecursively()', nextFiber, reconcilingParent); } @@ -2758,26 +2733,38 @@ export function attach( } } - if ( - mostRecentlyInspectedElement !== null && - mostRecentlyInspectedElement.id === id && - didFiberRender(prevFiber, nextFiber) - ) { - // If this Fiber has updated, clear cached inspected data. - // If it is inspected again, it may need to be re-run to obtain updated hooks values. - hasElementUpdatedSinceLastInspected = true; - } - + let fiberInstance: null | FiberInstance = null; const shouldIncludeInTree = !shouldFilterFiber(nextFiber); if (shouldIncludeInTree) { + const entry = fiberToFiberInstanceMap.get(prevFiber); + if (entry === undefined) { + throw new Error( + 'The previous version of the fiber should have already been registered.', + ); + } + fiberInstance = entry; + // Register the new alternate in case it's not already in. + fiberToFiberInstanceMap.set(nextFiber, fiberInstance); + // Update the Fiber so we that we always keep the current Fiber on the data. fiberInstance.data = nextFiber; moveChild(fiberInstance); + + if ( + mostRecentlyInspectedElement !== null && + mostRecentlyInspectedElement.id === fiberInstance.id && + didFiberRender(prevFiber, nextFiber) + ) { + // If this Fiber has updated, clear cached inspected data. + // If it is inspected again, it may need to be re-run to obtain updated hooks values. + hasElementUpdatedSinceLastInspected = true; + } } + const stashedParent = reconcilingParent; const stashedPrevious = previouslyReconciledSibling; const stashedRemaining = remainingReconcilingChildren; - if (shouldIncludeInTree) { + if (fiberInstance !== null) { // Push a new DevTools instance parent while reconciling this subtree. reconcilingParent = fiberInstance; previouslyReconciledSibling = null; @@ -2856,9 +2843,7 @@ export function attach( } else if (!prevDidTimeout && nextDidTimeOut) { // Primary -> Fallback: // 1. Hide primary set - // This is not a real unmount, so it won't get reported by React. - // We need to manually walk the previous tree and record unmounts. - unmountChildrenRecursively(prevFiber.child, true); + unmountChildrenRecursively(prevFiber.child); // 2. Mount fallback set const nextFiberChild = nextFiber.child; const nextFallbackChildSet = nextFiberChild @@ -2886,7 +2871,7 @@ export function attach( } } else { // Children are unchanged. - if (shouldIncludeInTree) { + if (fiberInstance !== null) { // All the remaining children will be children of this same fiber so we can just reuse them. // I.e. we just restore them by undoing what we did above. fiberInstance.firstChild = remainingReconcilingChildren; @@ -3003,7 +2988,15 @@ export function attach( } // If we have not been profiling, then we can just walk the tree and build up its current state as-is. hook.getFiberRoots(rendererID).forEach(root => { - currentRootID = getOrGenerateFiberInstance(root.current).id; + const current = root.current; + const alternate = current.alternate; + const newRoot = createFiberInstance(current); + idToDevToolsInstanceMap.set(newRoot.id, newRoot); + fiberToFiberInstanceMap.set(current, newRoot); + if (alternate) { + fiberToFiberInstanceMap.set(alternate, newRoot); + } + currentRootID = newRoot.id; setRootPseudoKey(currentRootID, root.current); // Handle multi-renderer edge-case where only some v16 renderers support profiling. @@ -3060,7 +3053,20 @@ export function attach( const current = root.current; const alternate = current.alternate; - currentRootID = getOrGenerateFiberInstance(current).id; + const existingRoot = + fiberToFiberInstanceMap.get(current) || + (alternate && fiberToFiberInstanceMap.get(alternate)); + if (!existingRoot) { + const newRoot = createFiberInstance(current); + idToDevToolsInstanceMap.set(newRoot.id, newRoot); + fiberToFiberInstanceMap.set(current, newRoot); + if (alternate) { + fiberToFiberInstanceMap.set(alternate, newRoot); + } + currentRootID = newRoot.id; + } else { + currentRootID = existingRoot.id; + } // Before the traversals, remember to start tracking // our path in case we have selection to restore. @@ -3117,7 +3123,7 @@ export function attach( } else if (wasMounted && !isMounted) { // Unmount an existing root. removeRootPseudoKey(currentRootID); - unmountFiberRecursively(alternate, false); + unmountFiberRecursively(alternate); } } else { // Mount a new root. @@ -3617,7 +3623,9 @@ export function attach( while (owner != null) { if (typeof owner.tag === 'number') { const ownerFiber: Fiber = (owner: any); // Refined - owners.unshift(fiberToSerializedElement(ownerFiber)); + if (!shouldFilterFiber(ownerFiber)) { + owners.unshift(fiberToSerializedElement(ownerFiber)); + } owner = ownerFiber._debugOwner; } else { // TODO: Track Server Component Owners. From fbe81b214a474f0c7b6512dbfe8a9e3948e5620a Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Mon, 12 Aug 2024 14:03:27 -0400 Subject: [PATCH 3/6] [compiler] Publish to latest tag > [!NOTE] > The `latest` tag is published by default if no tag is specified, which > is what we had done since the first release of the compiler In my last PR to auto publish compiler releases I had added the experimental tag to be used in publishing. However because we had already previously published to the latest tag (which is non-removable) this means that the `latest` tag is pinned to an old version. That makes untagged installs of the compiler default to that old version instead of whatever is the latest. This changes the behavior back to what it was before. Since we are still in the experimental release of the compiler anyway it seems fine to use the latest tag. When we reach stable, we can update this to only push to latest for stable releases. ghstack-source-id: 1809481b452150d6a452d4a77ea5482069bc7c83 Pull Request resolved: https://github.com/facebook/react/pull/30666 --- compiler/scripts/release/publish.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/scripts/release/publish.js b/compiler/scripts/release/publish.js index 3262cbabd1509..928994e1e4892 100755 --- a/compiler/scripts/release/publish.js +++ b/compiler/scripts/release/publish.js @@ -162,7 +162,9 @@ async function main() { 'publish', ...opts, '--registry=https://registry.npmjs.org', - '--tag=experimental', + // For now, since the compiler is experimental only, to simplify installation we push + // to the `latest` tag + '--tag=latest', ], { cwd: pkgDir, From d50e024fd49cbd701e7e286441ef2b6b0b59ba62 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Mon, 12 Aug 2024 12:36:49 -0700 Subject: [PATCH 4/6] [compiler] Promote temporaries when necessary to prevent codegen reordering over side-effectful operations ghstack-source-id: 639191e63a0d2b4290d1265a2da12fb17de750d9 Pull Request resolved: https://github.com/facebook/react/pull/30554 --- .../src/Entrypoint/Pipeline.ts | 8 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 8 +- .../ReactiveScopes/PromoteUsedTemporaries.ts | 203 ++++++++++++++++++ .../bug-codegen-inline-iife.expect.md | 92 -------- .../compiler/bug-codegen-inline-iife.ts | 36 ---- .../codegen-inline-iife-reassign.expect.md | 62 ++++++ .../compiler/codegen-inline-iife-reassign.ts | 18 ++ .../codegen-inline-iife-storeprop.expect.md | 60 ++++++ .../compiler/codegen-inline-iife-storeprop.ts | 18 ++ .../compiler/codegen-inline-iife.expect.md | 56 +++++ .../fixtures/compiler/codegen-inline-iife.ts | 16 ++ 11 files changed, 441 insertions(+), 136 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.ts 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 8711b251ea168..1c96e65c71ef5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -452,17 +452,17 @@ function* runWithEnvironment( value: reactiveFunction, }); - promoteUsedTemporaries(reactiveFunction); + pruneUnusedLValues(reactiveFunction); yield log({ kind: 'reactive', - name: 'PromoteUsedTemporaries', + name: 'PruneUnusedLValues', value: reactiveFunction, }); - pruneUnusedLValues(reactiveFunction); + promoteUsedTemporaries(reactiveFunction); yield log({ kind: 'reactive', - name: 'PruneUnusedLValues', + name: 'PromoteUsedTemporaries', value: reactiveFunction, }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index 978596dc67dfd..bd3e97f23aba7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -1209,7 +1209,7 @@ function codegenInstructionNullable( value = null; } else { lvalue = instr.value.lvalue.pattern; - let hasReasign = false; + let hasReassign = false; let hasDeclaration = false; for (const place of eachPatternOperand(lvalue)) { if ( @@ -1219,10 +1219,10 @@ function codegenInstructionNullable( cx.temp.set(place.identifier.declarationId, null); } const isDeclared = cx.hasDeclared(place.identifier); - hasReasign ||= isDeclared; + hasReassign ||= isDeclared; hasDeclaration ||= !isDeclared; } - if (hasReasign && hasDeclaration) { + if (hasReassign && hasDeclaration) { CompilerError.invariant(false, { reason: 'Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations)', @@ -1230,7 +1230,7 @@ function codegenInstructionNullable( loc: instr.loc, suggestions: null, }); - } else if (hasReasign) { + } else if (hasReassign) { kind = InstructionKind.Reassign; } value = codegenPlaceToExpression(cx, instr.value.value); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts index d897b924ce68c..12cbde01b20ff 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts @@ -15,13 +15,17 @@ import { PrunedReactiveScopeBlock, ReactiveFunction, ReactiveScope, + ReactiveInstruction, ReactiveScopeBlock, ReactiveValue, ScopeId, + SpreadPattern, promoteTemporary, promoteTemporaryJsxTag, + IdentifierId, } from '../HIR/HIR'; import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors'; +import {eachInstructionValueLValue, eachPatternOperand} from '../HIR/visitors'; /** * Phase 2: Promote identifiers which are used in a place that requires a named variable. @@ -225,6 +229,199 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor { } } +type InterState = Map; +class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { + #promotable: State; + #consts: Set = new Set(); + #globals: Set = new Set(); + + /* + * Unpromoted temporaries will be emitted at their use sites rather than as separate + * declarations. However, this causes errors if an interposing temporary has been + * promoted, or if an interposing instruction has had its lvalues deleted, because such + * temporaries will be emitted as separate statements, which can effectively cause + * code to be reordered, and when that code has side effects that changes program behavior. + * This visitor promotes temporarties that have such interposing instructions to preserve + * source ordering. + */ + constructor(promotable: State, params: Array) { + super(); + params.forEach(param => { + switch (param.kind) { + case 'Identifier': + this.#consts.add(param.identifier.id); + break; + case 'Spread': + this.#consts.add(param.place.identifier.id); + break; + } + }); + this.#promotable = promotable; + } + + override visitPlace( + _id: InstructionId, + place: Place, + state: InterState, + ): void { + const promo = state.get(place.identifier.id); + if (promo) { + const [identifier, needsPromotion] = promo; + if ( + needsPromotion && + identifier.name === null && + !this.#consts.has(identifier.id) + ) { + /* + * If the identifier hasn't been promoted but is marked as needing + * promotion by the logic in `visitInstruction`, and we've seen a + * use of it after said marking, promote it + */ + promoteIdentifier(identifier, this.#promotable); + } + } + } + + override visitInstruction( + instruction: ReactiveInstruction, + state: InterState, + ): void { + for (const lval of eachInstructionValueLValue(instruction.value)) { + CompilerError.invariant(lval.identifier.name != null, { + reason: + 'PromoteInterposedTemporaries: Assignment targets not expected to be temporaries', + loc: instruction.loc, + }); + } + + switch (instruction.value.kind) { + case 'CallExpression': + case 'MethodCall': + case 'Await': + case 'PropertyStore': + case 'PropertyDelete': + case 'ComputedStore': + case 'ComputedDelete': + case 'PostfixUpdate': + case 'PrefixUpdate': + case 'StoreLocal': + case 'StoreContext': + case 'StoreGlobal': + case 'Destructure': { + let constStore = false; + + if ( + (instruction.value.kind === 'StoreContext' || + instruction.value.kind === 'StoreLocal') && + (instruction.value.lvalue.kind === 'Const' || + instruction.value.lvalue.kind === 'HoistedConst') + ) { + /* + * If an identifier is const, we don't need to worry about it + * being mutated between being loaded and being used + */ + this.#consts.add(instruction.value.lvalue.place.identifier.id); + constStore = true; + } + if ( + instruction.value.kind === 'Destructure' && + (instruction.value.lvalue.kind === 'Const' || + instruction.value.lvalue.kind === 'HoistedConst') + ) { + [...eachPatternOperand(instruction.value.lvalue.pattern)].forEach( + ident => this.#consts.add(ident.identifier.id), + ); + constStore = true; + } + if (instruction.value.kind === 'MethodCall') { + // Treat property of method call as constlike so we don't promote it. + this.#consts.add(instruction.value.property.identifier.id); + } + + super.visitInstruction(instruction, state); + if ( + !constStore && + (instruction.lvalue == null || + instruction.lvalue.identifier.name != null) + ) { + /* + * If we've stripped the lvalue or promoted the lvalue, then we will emit this instruction + * as a statement in codegen. + * + * If this instruction will be emitted directly as a statement rather than as a temporary + * during codegen, then it can interpose between the defs and the uses of other temporaries. + * Since this instruction could potentially mutate those defs, it's not safe to relocate + * the definition of those temporaries to after this instruction. Mark all those temporaries + * as needing promotion, but don't promote them until we actually see them being used. + */ + for (const [key, [ident, _]] of state.entries()) { + state.set(key, [ident, true]); + } + } + if (instruction.lvalue && instruction.lvalue.identifier.name === null) { + // Add this instruction's lvalue to the state, initially not marked as needing promotion + state.set(instruction.lvalue.identifier.id, [ + instruction.lvalue.identifier, + false, + ]); + } + break; + } + case 'DeclareContext': + case 'DeclareLocal': { + if ( + instruction.value.lvalue.kind === 'Const' || + instruction.value.lvalue.kind === 'HoistedConst' + ) { + this.#consts.add(instruction.value.lvalue.place.identifier.id); + } + super.visitInstruction(instruction, state); + break; + } + case 'LoadContext': + case 'LoadLocal': { + if (instruction.lvalue && instruction.lvalue.identifier.name === null) { + if (this.#consts.has(instruction.value.place.identifier.id)) { + this.#consts.add(instruction.lvalue.identifier.id); + } + state.set(instruction.lvalue.identifier.id, [ + instruction.lvalue.identifier, + false, + ]); + } + super.visitInstruction(instruction, state); + break; + } + case 'PropertyLoad': + case 'ComputedLoad': { + if (instruction.lvalue) { + if (this.#globals.has(instruction.value.object.identifier.id)) { + this.#globals.add(instruction.lvalue.identifier.id); + this.#consts.add(instruction.lvalue.identifier.id); + } + if (instruction.lvalue.identifier.name === null) { + state.set(instruction.lvalue.identifier.id, [ + instruction.lvalue.identifier, + false, + ]); + } + } + super.visitInstruction(instruction, state); + break; + } + case 'LoadGlobal': { + instruction.lvalue && + this.#globals.add(instruction.lvalue.identifier.id); + super.visitInstruction(instruction, state); + break; + } + default: { + super.visitInstruction(instruction, state); + } + } + } +} + export function promoteUsedTemporaries(fn: ReactiveFunction): void { const state: State = { tags: new Set(), @@ -239,6 +436,12 @@ export function promoteUsedTemporaries(fn: ReactiveFunction): void { } } visitReactiveFunction(fn, new PromoteTemporaries(), state); + + visitReactiveFunction( + fn, + new PromoteInterposedTemporaries(state, fn.params), + new Map(), + ); visitReactiveFunction( fn, new PromoteAllInstancedOfPromotedTemporaries(), diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md deleted file mode 100644 index 4d36117ec7281..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md +++ /dev/null @@ -1,92 +0,0 @@ - -## Input - -```javascript -import {makeArray, print} from 'shared-runtime'; - -/** - * Exposes bug involving iife inlining + codegen. - * We currently inline iifes to labeled blocks (not value-blocks). - * - * Here, print(1) and the evaluation of makeArray(...) get the same scope - * as the compiler infers that the makeArray call may mutate its arguments. - * Since print(1) does not get its own scope (and is thus not a declaration - * or dependency), it does not get promoted. - * As a result, print(1) gets reordered across the labeled-block instructions - * to be inlined at the makeArray callsite. - * - * Current evaluator results: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) [null,2] - * logs: [1,2] - * Forget: - * (kind: ok) [null,2] - * logs: [2,1] - */ -function useTest() { - return makeArray( - print(1), - (function foo() { - print(2); - return 2; - })(), - ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useTest, - params: [], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import { makeArray, print } from "shared-runtime"; - -/** - * Exposes bug involving iife inlining + codegen. - * We currently inline iifes to labeled blocks (not value-blocks). - * - * Here, print(1) and the evaluation of makeArray(...) get the same scope - * as the compiler infers that the makeArray call may mutate its arguments. - * Since print(1) does not get its own scope (and is thus not a declaration - * or dependency), it does not get promoted. - * As a result, print(1) gets reordered across the labeled-block instructions - * to be inlined at the makeArray callsite. - * - * Current evaluator results: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) [null,2] - * logs: [1,2] - * Forget: - * (kind: ok) [null,2] - * logs: [2,1] - */ -function useTest() { - const $ = _c(1); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - let t1; - - print(2); - t1 = 2; - t0 = makeArray(print(1), t1); - $[0] = t0; - } else { - t0 = $[0]; - } - return t0; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useTest, - params: [], -}; - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts deleted file mode 100644 index 0aa4a95bd5241..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts +++ /dev/null @@ -1,36 +0,0 @@ -import {makeArray, print} from 'shared-runtime'; - -/** - * Exposes bug involving iife inlining + codegen. - * We currently inline iifes to labeled blocks (not value-blocks). - * - * Here, print(1) and the evaluation of makeArray(...) get the same scope - * as the compiler infers that the makeArray call may mutate its arguments. - * Since print(1) does not get its own scope (and is thus not a declaration - * or dependency), it does not get promoted. - * As a result, print(1) gets reordered across the labeled-block instructions - * to be inlined at the makeArray callsite. - * - * Current evaluator results: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) [null,2] - * logs: [1,2] - * Forget: - * (kind: ok) [null,2] - * logs: [2,1] - */ -function useTest() { - return makeArray( - print(1), - (function foo() { - print(2); - return 2; - })(), - ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useTest, - params: [], -}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md new file mode 100644 index 0000000000000..d17c934b3b42c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md @@ -0,0 +1,62 @@ + +## Input + +```javascript +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + let w = {}; + return makeArray( + (w = 42), + w, + (function foo() { + w = 999; + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray, print } from "shared-runtime"; + +function useTest() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + let w; + w = {}; + + const t1 = (w = 42); + const t2 = w; + + w; + let t3; + w = 999; + t3 = 2; + t0 = makeArray(t1, t2, t3); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +### Eval output +(kind: ok) [42,42,2] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.ts new file mode 100644 index 0000000000000..8dd93a8482b92 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.ts @@ -0,0 +1,18 @@ +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + let w = {}; + return makeArray( + (w = 42), + w, + (function foo() { + w = 999; + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.expect.md new file mode 100644 index 0000000000000..58c54ddaab891 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.expect.md @@ -0,0 +1,60 @@ + +## Input + +```javascript +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + let w = {}; + return makeArray( + (w.x = 42), + w.x, + (function foo() { + w.x = 999; + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray, print } from "shared-runtime"; + +function useTest() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const w = {}; + + const t1 = (w.x = 42); + const t2 = w.x; + let t3; + + w.x = 999; + t3 = 2; + t0 = makeArray(t1, t2, t3); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +### Eval output +(kind: ok) [42,42,2] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.ts new file mode 100644 index 0000000000000..1b9f10856e48f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.ts @@ -0,0 +1,18 @@ +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + let w = {}; + return makeArray( + (w.x = 42), + w.x, + (function foo() { + w.x = 999; + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.expect.md new file mode 100644 index 0000000000000..25a08bc3329cb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.expect.md @@ -0,0 +1,56 @@ + +## Input + +```javascript +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + return makeArray( + print(1), + (function foo() { + print(2); + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray, print } from "shared-runtime"; + +function useTest() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const t1 = print(1); + let t2; + + print(2); + t2 = 2; + t0 = makeArray(t1, t2); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +### Eval output +(kind: ok) [null,2] +logs: [1,2] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.ts new file mode 100644 index 0000000000000..52b1e8a9278e1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.ts @@ -0,0 +1,16 @@ +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + return makeArray( + print(1), + (function foo() { + print(2); + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; From 6532c412566f7223f82b1460023180c25e15fca3 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Mon, 12 Aug 2024 12:55:55 -0700 Subject: [PATCH 5/6] [compiler] Allow macro methods Summary: Builds support for macros that are invoked as methods rather than just function calls or jsx. We now record macros as a schema that represents arbitrary member expressions including wildcards (so we can support, e.g., myMacro.*.foo.bar). When examining PropertyLoads in the macro memoization stage, we build up a map of partially-satisfied macro patterns until we determine that the pattern has been fully satisfied, at which point we treat the result of the PropertyLoad as a macro value. ghstack-source-id: d78d9ba7041968c861ffa110fb7882b339a0e257 Pull Request resolved: https://github.com/facebook/react/pull/30589 --- .../src/HIR/Environment.ts | 30 ++++- .../MemoizeFbtAndMacroOperandsInSameScope.ts | 86 ++++++++++-- ...idx-method-no-outlining-wildcard.expect.md | 122 ++++++++++++++++++ .../idx-method-no-outlining-wildcard.js | 23 ++++ .../idx-method-no-outlining.expect.md | 85 ++++++++++++ .../compiler/idx-method-no-outlining.js | 17 +++ .../packages/snap/src/SproutTodoFilter.ts | 5 +- compiler/packages/snap/src/compiler.ts | 27 +++- 8 files changed, 378 insertions(+), 17 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 73645a3c953bc..7a83f7e3a0b76 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -67,6 +67,20 @@ export const InstrumentationSchema = z export type ExternalFunction = z.infer; +export const MacroMethodSchema = z.union([ + z.object({type: z.literal('wildcard')}), + z.object({type: z.literal('name'), name: z.string()}), +]); + +// Would like to change this to drop the string option, but breaks compatibility with existing configs +export const MacroSchema = z.union([ + z.string(), + z.tuple([z.string(), z.array(MacroMethodSchema)]), +]); + +export type Macro = z.infer; +export type MacroMethod = z.infer; + const HookSchema = z.object({ /* * The effect of arguments to this hook. Describes whether the hook may or may @@ -133,7 +147,7 @@ const EnvironmentConfigSchema = z.object({ * plugin since it looks specifically for the name of the function being invoked, not * following aliases. */ - customMacros: z.nullable(z.array(z.string())).default(null), + customMacros: z.nullable(z.array(MacroSchema)).default(null), /** * Enable a check that resets the memoization cache when the source code of the file changes. @@ -490,7 +504,19 @@ export function parseConfigPragma(pragma: string): EnvironmentConfig { } if (key === 'customMacros' && val) { - maybeConfig[key] = [val]; + const valSplit = val.split('.'); + if (valSplit.length > 0) { + const props = []; + for (const elt of valSplit.slice(1)) { + if (elt === '*') { + props.push({type: 'wildcard'}); + } else if (elt.length > 0) { + props.push({type: 'name', name: elt}); + } + } + console.log([valSplit[0], props.map(x => x.name ?? '*').join('.')]); + maybeConfig[key] = [[valSplit[0], props]]; + } continue; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts index 1f3f8b1271814..022890c1f25c2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts @@ -13,6 +13,7 @@ import { Place, ReactiveValue, } from '../HIR'; +import {Macro, MacroMethod} from '../HIR/Environment'; import {eachReactiveValueOperand} from './visitors'; /** @@ -43,15 +44,17 @@ import {eachReactiveValueOperand} from './visitors'; export function memoizeFbtAndMacroOperandsInSameScope( fn: HIRFunction, ): Set { - const fbtMacroTags = new Set([ - ...FBT_TAGS, + const fbtMacroTags = new Set([ + ...Array.from(FBT_TAGS).map((tag): Macro => [tag, []]), ...(fn.env.config.customMacros ?? []), ]); const fbtValues: Set = new Set(); + const macroMethods = new Map>>(); while (true) { - let size = fbtValues.size; - visit(fn, fbtMacroTags, fbtValues); - if (size === fbtValues.size) { + let vsize = fbtValues.size; + let msize = macroMethods.size; + visit(fn, fbtMacroTags, fbtValues, macroMethods); + if (vsize === fbtValues.size && msize === macroMethods.size) { break; } } @@ -71,8 +74,9 @@ export const SINGLE_CHILD_FBT_TAGS: Set = new Set([ function visit( fn: HIRFunction, - fbtMacroTags: Set, + fbtMacroTags: Set, fbtValues: Set, + macroMethods: Map>>, ): void { for (const [, block] of fn.body.blocks) { for (const instruction of block.instructions) { @@ -83,7 +87,7 @@ function visit( if ( value.kind === 'Primitive' && typeof value.value === 'string' && - fbtMacroTags.has(value.value) + matchesExactTag(value.value, fbtMacroTags) ) { /* * We don't distinguish between tag names and strings, so record @@ -92,10 +96,38 @@ function visit( fbtValues.add(lvalue.identifier.id); } else if ( value.kind === 'LoadGlobal' && - fbtMacroTags.has(value.binding.name) + matchesExactTag(value.binding.name, fbtMacroTags) ) { // Record references to `fbt` as a global fbtValues.add(lvalue.identifier.id); + } else if ( + value.kind === 'LoadGlobal' && + matchTagRoot(value.binding.name, fbtMacroTags) !== null + ) { + const methods = matchTagRoot(value.binding.name, fbtMacroTags)!; + macroMethods.set(lvalue.identifier.id, methods); + } else if ( + value.kind === 'PropertyLoad' && + macroMethods.has(value.object.identifier.id) + ) { + const methods = macroMethods.get(value.object.identifier.id)!; + const newMethods = []; + for (const method of methods) { + if ( + method.length > 0 && + (method[0].type === 'wildcard' || + (method[0].type === 'name' && method[0].name === value.property)) + ) { + if (method.length > 1) { + newMethods.push(method.slice(1)); + } else { + fbtValues.add(lvalue.identifier.id); + } + } + } + if (newMethods.length > 0) { + macroMethods.set(lvalue.identifier.id, newMethods); + } } else if (isFbtCallExpression(fbtValues, value)) { const fbtScope = lvalue.identifier.scope; if (fbtScope === null) { @@ -167,17 +199,48 @@ function visit( } } +function matchesExactTag(s: string, tags: Set): boolean { + return Array.from(tags).some(macro => + typeof macro === 'string' + ? s === macro + : macro[1].length === 0 && macro[0] === s, + ); +} + +function matchTagRoot( + s: string, + tags: Set, +): Array> | null { + const methods: Array> = []; + for (const macro of tags) { + if (typeof macro === 'string') { + continue; + } + const [tag, rest] = macro; + if (tag === s && rest.length > 0) { + methods.push(rest); + } + } + if (methods.length > 0) { + return methods; + } else { + return null; + } +} + function isFbtCallExpression( fbtValues: Set, value: ReactiveValue, ): boolean { return ( - value.kind === 'CallExpression' && fbtValues.has(value.callee.identifier.id) + (value.kind === 'CallExpression' && + fbtValues.has(value.callee.identifier.id)) || + (value.kind === 'MethodCall' && fbtValues.has(value.property.identifier.id)) ); } function isFbtJsxExpression( - fbtMacroTags: Set, + fbtMacroTags: Set, fbtValues: Set, value: ReactiveValue, ): boolean { @@ -185,7 +248,8 @@ function isFbtJsxExpression( value.kind === 'JsxExpression' && ((value.tag.kind === 'Identifier' && fbtValues.has(value.tag.identifier.id)) || - (value.tag.kind === 'BuiltinTag' && fbtMacroTags.has(value.tag.name))) + (value.tag.kind === 'BuiltinTag' && + matchesExactTag(value.tag.name, fbtMacroTags))) ); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.expect.md new file mode 100644 index 0000000000000..dbc90978d5ee0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.expect.md @@ -0,0 +1,122 @@ + +## Input + +```javascript +// @customMacros(idx.*.b) + +function Component(props) { + // outlined + const groupName1 = idx(props, _ => _.group.label); + // outlined + const groupName2 = idx.a(props, _ => _.group.label); + // not outlined + const groupName3 = idx.a.b(props, _ => _.group.label); + // not outlined + const groupName4 = idx.hello_world.b(props, _ => _.group.label); + // outlined + const groupName5 = idx.hello_world.b.c(props, _ => _.group.label); + return ( +
+ {groupName1} + {groupName2} + {groupName3} + {groupName4} + {groupName5} +
+ ); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @customMacros(idx.*.b) + +function Component(props) { + const $ = _c(16); + let t0; + if ($[0] !== props) { + t0 = idx(props, _temp); + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + const groupName1 = t0; + let t1; + if ($[2] !== props) { + t1 = idx.a(props, _temp2); + $[2] = props; + $[3] = t1; + } else { + t1 = $[3]; + } + const groupName2 = t1; + let t2; + if ($[4] !== props) { + t2 = idx.a.b(props, (__1) => __1.group.label); + $[4] = props; + $[5] = t2; + } else { + t2 = $[5]; + } + const groupName3 = t2; + let t3; + if ($[6] !== props) { + t3 = idx.hello_world.b(props, (__2) => __2.group.label); + $[6] = props; + $[7] = t3; + } else { + t3 = $[7]; + } + const groupName4 = t3; + let t4; + if ($[8] !== props) { + t4 = idx.hello_world.b.c(props, _temp3); + $[8] = props; + $[9] = t4; + } else { + t4 = $[9]; + } + const groupName5 = t4; + let t5; + if ( + $[10] !== groupName1 || + $[11] !== groupName2 || + $[12] !== groupName3 || + $[13] !== groupName4 || + $[14] !== groupName5 + ) { + t5 = ( +
+ {groupName1} + {groupName2} + {groupName3} + {groupName4} + {groupName5} +
+ ); + $[10] = groupName1; + $[11] = groupName2; + $[12] = groupName3; + $[13] = groupName4; + $[14] = groupName5; + $[15] = t5; + } else { + t5 = $[15]; + } + return t5; +} +function _temp3(__3) { + return __3.group.label; +} +function _temp2(__0) { + return __0.group.label; +} +function _temp(_) { + return _.group.label; +} + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.js new file mode 100644 index 0000000000000..4b944ddccac5a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.js @@ -0,0 +1,23 @@ +// @customMacros(idx.*.b) + +function Component(props) { + // outlined + const groupName1 = idx(props, _ => _.group.label); + // outlined + const groupName2 = idx.a(props, _ => _.group.label); + // not outlined + const groupName3 = idx.a.b(props, _ => _.group.label); + // not outlined + const groupName4 = idx.hello_world.b(props, _ => _.group.label); + // outlined + const groupName5 = idx.hello_world.b.c(props, _ => _.group.label); + return ( +
+ {groupName1} + {groupName2} + {groupName3} + {groupName4} + {groupName5} +
+ ); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.expect.md new file mode 100644 index 0000000000000..f1bced727b718 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.expect.md @@ -0,0 +1,85 @@ + +## Input + +```javascript +// @customMacros(idx.a) + +function Component(props) { + // outlined + const groupName1 = idx(props, _ => _.group.label); + // not outlined + const groupName2 = idx.a(props, _ => _.group.label); + // outlined + const groupName3 = idx.a.b(props, _ => _.group.label); + return ( +
+ {groupName1} + {groupName2} + {groupName3} +
+ ); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @customMacros(idx.a) + +function Component(props) { + const $ = _c(10); + let t0; + if ($[0] !== props) { + t0 = idx(props, _temp); + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + const groupName1 = t0; + let t1; + if ($[2] !== props) { + t1 = idx.a(props, (__0) => __0.group.label); + $[2] = props; + $[3] = t1; + } else { + t1 = $[3]; + } + const groupName2 = t1; + let t2; + if ($[4] !== props) { + t2 = idx.a.b(props, _temp2); + $[4] = props; + $[5] = t2; + } else { + t2 = $[5]; + } + const groupName3 = t2; + let t3; + if ($[6] !== groupName1 || $[7] !== groupName2 || $[8] !== groupName3) { + t3 = ( +
+ {groupName1} + {groupName2} + {groupName3} +
+ ); + $[6] = groupName1; + $[7] = groupName2; + $[8] = groupName3; + $[9] = t3; + } else { + t3 = $[9]; + } + return t3; +} +function _temp2(__1) { + return __1.group.label; +} +function _temp(_) { + return _.group.label; +} + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.js new file mode 100644 index 0000000000000..f5b034b91d67f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.js @@ -0,0 +1,17 @@ +// @customMacros(idx.a) + +function Component(props) { + // outlined + const groupName1 = idx(props, _ => _.group.label); + // not outlined + const groupName2 = idx.a(props, _ => _.group.label); + // outlined + const groupName3 = idx.a.b(props, _ => _.group.label); + return ( +
+ {groupName1} + {groupName2} + {groupName3} +
+ ); +} diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index c13cc007ea05f..f36c69e0260e0 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -493,13 +493,16 @@ const skipFilter = new Set([ // 'react-compiler-runtime' not yet supported 'flag-enable-emit-hook-guards', - 'fast-refresh-refresh-on-const-changes-dev', 'useState-pruned-dependency-change-detect', 'useState-unpruned-dependency', 'useState-and-other-hook-unpruned-dependency', 'change-detect-reassign', + // Depends on external functions + 'idx-method-no-outlining-wildcard', + 'idx-method-no-outlining', + // needs to be executed as a module 'meta-property', diff --git a/compiler/packages/snap/src/compiler.ts b/compiler/packages/snap/src/compiler.ts index a9e705ea3b9cf..de0184f0bddb1 100644 --- a/compiler/packages/snap/src/compiler.ts +++ b/compiler/packages/snap/src/compiler.ts @@ -20,7 +20,11 @@ import type { PluginOptions, } from 'babel-plugin-react-compiler/src/Entrypoint'; import type {Effect, ValueKind} from 'babel-plugin-react-compiler/src/HIR'; -import type {parseConfigPragma as ParseConfigPragma} from 'babel-plugin-react-compiler/src/HIR/Environment'; +import type { + Macro, + MacroMethod, + parseConfigPragma as ParseConfigPragma, +} from 'babel-plugin-react-compiler/src/HIR/Environment'; import * as HermesParser from 'hermes-parser'; import invariant from 'invariant'; import path from 'path'; @@ -46,7 +50,7 @@ function makePluginOptions( // TODO(@mofeiZ) rewrite snap fixtures to @validatePreserveExistingMemo:false let validatePreserveExistingMemoizationGuarantees = false; let enableChangeDetectionForDebugging = null; - let customMacros = null; + let customMacros: null | Array = null; let validateBlocklistedImports = null; if (firstLine.indexOf('@compilationMode(annotation)') !== -1) { @@ -150,10 +154,27 @@ function makePluginOptions( customMacrosMatch.length > 1 && customMacrosMatch[1].trim().length > 0 ) { - customMacros = customMacrosMatch[1] + const customMacrosStrs = customMacrosMatch[1] .split(' ') .map(s => s.trim()) .filter(s => s.length > 0); + if (customMacrosStrs.length > 0) { + customMacros = []; + for (const customMacroStr of customMacrosStrs) { + const props: Array = []; + const customMacroSplit = customMacroStr.split('.'); + if (customMacroSplit.length > 0) { + for (const elt of customMacroSplit.slice(1)) { + if (elt === '*') { + props.push({type: 'wildcard'}); + } else if (elt.length > 0) { + props.push({type: 'name', name: elt}); + } + } + customMacros.push([customMacroSplit[0], props]); + } + } + } } const validateBlocklistedImportsMatch = From 0d7c12c7790c4a7315af80f8c73ac951f024f4fe Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Mon, 12 Aug 2024 12:55:55 -0700 Subject: [PATCH 6/6] [compiler][ez] Enable some sprout tests that no longer need to be disabled Summary: As title. Better support for flow typing, bugfixes, etc fixes these ghstack-source-id: 6326653ce42b33b6c1c76a494434d133382ca80a Pull Request resolved: https://github.com/facebook/react/pull/30591 --- ...component-declaration-basic.flow.expect.md | 32 ++++++++++++++++++- .../component-declaration-basic.flow.js | 9 ++++++ .../hook-declaration-basic.flow.expect.md | 14 +++++++- .../compiler/hook-declaration-basic.flow.js | 5 +++ .../packages/snap/src/SproutTodoFilter.ts | 8 ++--- 5 files changed, 62 insertions(+), 6 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/component-declaration-basic.flow.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/component-declaration-basic.flow.expect.md index 42dc6ad88a367..12184dafbbecc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/component-declaration-basic.flow.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/component-declaration-basic.flow.expect.md @@ -7,8 +7,17 @@ export default component Foo(bar: number) { return ; } +component Bar(bar: number) { + return
{bar}
; +} + function shouldNotCompile() {} +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{bar: 42}], +}; + ``` ## Code @@ -29,7 +38,28 @@ export default function Foo(t0) { return t1; } +function Bar(t0) { + const $ = _c(2); + const { bar } = t0; + let t1; + if ($[0] !== bar) { + t1 =
{bar}
; + $[0] = bar; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + function shouldNotCompile() {} +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ bar: 42 }], +}; + ``` - \ No newline at end of file + +### Eval output +(kind: ok)
42
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/component-declaration-basic.flow.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/component-declaration-basic.flow.js index ed69217d58b5d..d29777d4f08d6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/component-declaration-basic.flow.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/component-declaration-basic.flow.js @@ -3,4 +3,13 @@ export default component Foo(bar: number) { return ; } +component Bar(bar: number) { + return
{bar}
; +} + function shouldNotCompile() {} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{bar: 42}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-declaration-basic.flow.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-declaration-basic.flow.expect.md index dcaa443f26ddb..f75514e8a87cc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-declaration-basic.flow.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-declaration-basic.flow.expect.md @@ -7,6 +7,11 @@ export default hook useFoo(bar: number) { return [bar]; } +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [42], +}; + ``` ## Code @@ -26,5 +31,12 @@ export default function useFoo(bar) { return t0; } +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [42], +}; + ``` - \ No newline at end of file + +### Eval output +(kind: ok) [42] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-declaration-basic.flow.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-declaration-basic.flow.js index b726e1218d3be..217fbe16365d9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-declaration-basic.flow.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-declaration-basic.flow.js @@ -2,3 +2,8 @@ export default hook useFoo(bar: number) { return [bar]; } + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [42], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index f36c69e0260e0..2d875430263c2 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -390,11 +390,12 @@ const skipFilter = new Set([ 'template-literal', 'multi-arrow-expr-export-default-gating-test', - // TODO: we should be able to support these - 'component-declaration-basic.flow', - 'hook-declaration-basic.flow', + // works, but appears differently when printing + // due to optional function argument 'nested-function-with-param-as-captured-dep', 'deeply-nested-function-expressions-with-params', + + // TODO: we should be able to support these 'readonly-object-method-calls', 'readonly-object-method-calls-mutable-lambda', 'preserve-memo-validation/useMemo-with-refs.flow', @@ -483,7 +484,6 @@ const skipFilter = new Set([ 'rules-of-hooks/rules-of-hooks-69521d94fa03', // bugs - 'bug-renaming-jsx-tag-lowercase', 'fbt/bug-fbt-plural-multiple-function-calls', 'fbt/bug-fbt-plural-multiple-mixed-call-tag', 'bug-invalid-hoisting-functionexpr',