From 5302bbb75d323a9f6cbcc375d17574376bf277e5 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Fri, 2 Aug 2024 13:21:29 -0700 Subject: [PATCH] [compiler] Fix issue with macro arguments being outlined Summary: Fixes issue documented by #30435. We change the pipeline order so that outlining comes after tracking macro operands, and any function that is referenced in a macro will now not be outlined. [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 18 +++--- .../src/HIR/Environment.ts | 5 ++ .../src/Optimization/OutlineFunctions.ts | 15 +++-- .../src/ReactiveScopes/index.ts | 2 +- .../compiler/error.idx-outlining.expect.md | 27 -------- .../compiler/idx-no-outlining.expect.md | 64 +++++++++++++++++++ ...r.idx-outlining.js => idx-no-outlining.js} | 1 + 7 files changed, 89 insertions(+), 43 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.idx-outlining.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-no-outlining.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.idx-outlining.js => idx-no-outlining.js} (91%) 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 73713e0ff81..acee9595550 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -56,7 +56,7 @@ import { flattenReactiveLoops, flattenScopesWithHooksOrUse, inferReactiveScopeVariables, - memoizeFbtOperandsInSameScope, + memoizeFbtAndMacroOperandsInSameScope, mergeOverlappingReactiveScopes, mergeReactiveScopesThatInvalidateTogether, promoteUsedTemporaries, @@ -243,8 +243,15 @@ function* runWithEnvironment( inferReactiveScopeVariables(hir); yield log({kind: 'hir', name: 'InferReactiveScopeVariables', value: hir}); + const fbtOperands = memoizeFbtAndMacroOperandsInSameScope(hir); + yield log({ + kind: 'hir', + name: 'MemoizeFbtAndMacroOperandsInSameScope', + value: hir, + }); + if (env.config.enableFunctionOutlining) { - outlineFunctions(hir); + outlineFunctions(hir, fbtOperands); yield log({kind: 'hir', name: 'OutlineFunctions', value: hir}); } @@ -262,13 +269,6 @@ function* runWithEnvironment( value: hir, }); - const fbtOperands = memoizeFbtOperandsInSameScope(hir); - yield log({ - kind: 'hir', - name: 'MemoizeFbtAndMacroOperandsInSameScope', - value: hir, - }); - if (env.config.enableReactiveScopesInHIR) { pruneUnusedLabelsHIR(hir); yield log({ 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 05c90b355cd..dad27965afd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -485,6 +485,11 @@ export function parseConfigPragma(pragma: string): EnvironmentConfig { continue; } + if (key === 'customMacros' && val) { + maybeConfig[key] = [val]; + continue; + } + if (typeof defaultConfig[key as keyof EnvironmentConfig] !== 'boolean') { // skip parsing non-boolean properties continue; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts index 1b90ecfc9a6..7a1473be40c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts @@ -5,27 +5,30 @@ * LICENSE file in the root directory of this source tree. */ -import {HIRFunction} from '../HIR'; +import {HIRFunction, IdentifierId} from '../HIR'; -export function outlineFunctions(fn: HIRFunction): void { +export function outlineFunctions( + fn: HIRFunction, + fbtOperands: Set, +): void { for (const [, block] of fn.body.blocks) { for (const instr of block.instructions) { - const {value} = instr; + const {value, lvalue} = instr; if ( value.kind === 'FunctionExpression' || value.kind === 'ObjectMethod' ) { // Recurse in case there are inner functions which can be outlined - outlineFunctions(value.loweredFunc.func); + outlineFunctions(value.loweredFunc.func, fbtOperands); } - if ( value.kind === 'FunctionExpression' && value.loweredFunc.dependencies.length === 0 && value.loweredFunc.func.context.length === 0 && // TODO: handle outlining named functions - value.loweredFunc.func.id === null + value.loweredFunc.func.id === null && + !fbtOperands.has(lvalue.identifier.id) ) { const loweredFunc = value.loweredFunc.func; diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/index.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/index.ts index 3dd64a26d21..8f6cad8d11f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/index.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/index.ts @@ -16,7 +16,7 @@ export {extractScopeDeclarationsFromDestructuring} from './ExtractScopeDeclarati export {flattenReactiveLoops} from './FlattenReactiveLoops'; export {flattenScopesWithHooksOrUse} from './FlattenScopesWithHooksOrUse'; export {inferReactiveScopeVariables} from './InferReactiveScopeVariables'; -export {memoizeFbtAndMacroOperandsInSameScope as memoizeFbtOperandsInSameScope} from './MemoizeFbtAndMacroOperandsInSameScope'; +export {memoizeFbtAndMacroOperandsInSameScope} from './MemoizeFbtAndMacroOperandsInSameScope'; export {mergeOverlappingReactiveScopes} from './MergeOverlappingReactiveScopes'; export {mergeReactiveScopesThatInvalidateTogether} from './MergeReactiveScopesThatInvalidateTogether'; export {printReactiveFunction} from './PrintReactiveFunction'; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.idx-outlining.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.idx-outlining.expect.md deleted file mode 100644 index 71ffa795c55..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.idx-outlining.expect.md +++ /dev/null @@ -1,27 +0,0 @@ - -## Input - -```javascript -import idx from 'idx'; - -function Component(props) { - // the lambda should not be outlined - const groupName = idx(props, _ => _.group.label); - return
{groupName}
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{}], -}; - -``` - - -## Error - -``` -The second argument supplied to `idx` must be an arrow function. (This is an error on an internal node. Probably an internal error.) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-no-outlining.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-no-outlining.expect.md new file mode 100644 index 00000000000..a0fafc56c80 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-no-outlining.expect.md @@ -0,0 +1,64 @@ + +## Input + +```javascript +// @customMacros(idx) +import idx from 'idx'; + +function Component(props) { + // the lambda should not be outlined + const groupName = idx(props, _ => _.group.label); + return
{groupName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @customMacros(idx) + +function Component(props) { + var _ref2; + const $ = _c(4); + let t0; + if ($[0] !== props) { + var _ref; + + t0 = + (_ref = props) != null + ? (_ref = _ref.group) != null + ? _ref.label + : _ref + : _ref; + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + const groupName = t0; + let t1; + if ($[2] !== groupName) { + t1 =
{groupName}
; + $[2] = groupName; + $[3] = t1; + } else { + t1 = $[3]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.idx-outlining.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-no-outlining.js similarity index 91% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.idx-outlining.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-no-outlining.js index 2b76c60d37b..7d16c8d2b76 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.idx-outlining.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-no-outlining.js @@ -1,3 +1,4 @@ +// @customMacros(idx) import idx from 'idx'; function Component(props) {