[compiler][be] Make program traversal more readable #33147
Conversation
| * @returns the compiled function or null if the function was skipped (due to | ||
| * config settings and/or outputs) | ||
| */ | ||
| function processFn( |
There was a problem hiding this comment.
processFn now has compilation control flow logic. This is mainly:
- retry logic
- determining whether a compiled function should be emitted, according to compiler options and directive opt-ins / opt-outs
| ): Array<t.Directive> { | ||
| return directives.filter(directive => | ||
| OPT_OUT_DIRECTIVES.has(directive.value.value), | ||
| ): t.Directive | null { |
There was a problem hiding this comment.
We were only checking if the returned array had entries. Note that we do use loc from this directive sometimes, which is why this isn't refactored to just return a boolean
| /* | ||
| * This is a hack to work around what seems to be a Babel bug. Babel doesn't | ||
| * consistently respect the `skip()` function to avoid revisiting a node within | ||
| * a pass, so we use this set to track nodes that we have compiled. | ||
| */ | ||
| const ALREADY_COMPILED: WeakSet<object> | Set<object> = new (WeakSet ?? Set)(); | ||
|
|
There was a problem hiding this comment.
This is now moved to ProgramContext
| export function compileProgram( | ||
| program: NodePath<t.Program>, | ||
| pass: CompilerPass, | ||
| ): CompileProgramResult | null { | ||
| ): CompileProgramMetadata | null { |
There was a problem hiding this comment.
This is now much shorter. There are no semantically meaningful changes (most lines were copied and pasted)
mofeiZ
force-pushed
the
pr33147
branch
2 times, most recently
from
23c45ff to
6002392
Compare
| const retryResult = retryCompileFunction(fn, fnType, programContext); | ||
| if (retryResult == null) { | ||
| return null; | ||
| } | ||
| compileResult = { | ||
| kind: 'compile', | ||
| compiledFn: retryResult, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Added a retry-no-emit fixture to test that we're preserving behavior
| const referencedBeforeDeclared = | ||
| pass.opts.gating != null | ||
| ? getFunctionReferencedBeforeDeclarationAtTopLevel(program, compiledFns) | ||
| : null; | ||
| for (const result of compiledFns) { | ||
| const {kind, originalFn, compiledFn} = result; | ||
| const transformedFn = createNewFunctionNode(originalFn, compiledFn); | ||
| programContext.alreadyCompiled.add(transformedFn); |
There was a problem hiding this comment.
This is really a no-op since we only use alreadyCompiled when making an initial pass over the program to collect react functions for compilation
mofeiZ
force-pushed
the
pr33147
branch
2 times, most recently
from
1c521f5 to
d80c6a2
Compare
There was a problem hiding this comment.
Nice! See comments for a couple things but overall the refactoring definitely makes this easier to follow (the git diff is hard to follow bc of how much is just moving and formatting differences, but just looking at the new version in its entirety, the improvement is clear)
| // Avoid modifying the program if we find a program level opt-out | ||
| if (findDirectiveDisablingMemoization(program.node.directives) != null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
hmm, insertNewOutlinedFunctionNode() mutates the program and can have been called already. Shouldn't we check this first before processing the queue?
There was a problem hiding this comment.
I kept this PR as refactoring only, #33148 makes that correction (see PR and its changed test fixtures)
| } | ||
|
|
||
| // Insert React Compiler generated functions into the Babel AST | ||
| applyCompiledFunctions(program, compiledFns, pass, programContext); |
There was a problem hiding this comment.
alternatively we could change insertNewOutlinedFunctionNode() to not mutate and do the mutation portion of it here
There was a problem hiding this comment.
The tricky part is that our pipeline needs to operate on a NodePath (primarily for identifier scope resolution). We can only get a NodePath by mutating the existing program AST or creating a placeholder wrapper program (which isn't ideal since we'd lose variable scope analysis, module imports, etc)
There was a problem hiding this comment.
I'd also greatly prefer to apply all compiled functions after all transformations, as transformations may throw / fail or we may re-introduce program-level bailouts. Would love any ideas on how to achieve that!
| return null; | ||
| } | ||
| let compiledFn: CodegenFunction; | ||
| const compileResult = tryCompileFunction(fn, fnType, programContext); |
There was a problem hiding this comment.
doesn't this mean we'll always compile even if a file is opted out of memoization (even if we're not in ignoreUseNoForget=true mode?
There was a problem hiding this comment.
Yeah. This is just a refactoring PR, happy to change this in a follow up
React Compiler's program traversal logic is pretty lengthy and complex as we've added a lot of features piecemeal. `compileProgram` is 300+ lines long and has confusing control flow (defining helpers inline, invoking visitors, mutating-asts-while-iterating, mutating global `ALREADY_COMPILED` state). - Moved more stuff to `ProgramContext` - Separated `compileProgram` into a bunch of helpers Tested by syncing this stack to a Meta codebase and observing no compilation output changes (D74487851, P1806855669, P1806855379)
React Compiler's program traversal logic is pretty lengthy and complex as we've added a lot of features piecemeal. `compileProgram` is 300+ lines long and has confusing control flow (defining helpers inline, invoking visitors, mutating-asts-while-iterating, mutating global `ALREADY_COMPILED` state). - Moved more stuff to `ProgramContext` - Separated `compileProgram` into a bunch of helpers Tested by syncing this stack to a Meta codebase and observing no compilation output changes (D74487851, P1806855669, P1806855379) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33147). * #33149 * #33148 * __->__ #33147 DiffTrain build for [5069e18](5069e18)
React Compiler's program traversal logic is pretty lengthy and complex as we've added a lot of features piecemeal. `compileProgram` is 300+ lines long and has confusing control flow (defining helpers inline, invoking visitors, mutating-asts-while-iterating, mutating global `ALREADY_COMPILED` state). - Moved more stuff to `ProgramContext` - Separated `compileProgram` into a bunch of helpers Tested by syncing this stack to a Meta codebase and observing no compilation output changes (D74487851, P1806855669, P1806855379) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33147). * #33149 * #33148 * __->__ #33147 DiffTrain build for [5069e18](5069e18)
React Compiler's program traversal logic is pretty lengthy and complex as we've added a lot of features piecemeal.
compileProgramis 300+ lines long and has confusing control flow (defining helpers inline, invoking visitors, mutating-asts-while-iterating, mutating globalALREADY_COMPILEDstate).ProgramContextcompilePrograminto a bunch of helpersTested by syncing this stack to a Meta codebase and observing no compilation output changes (D74487851, P1806855669, P1806855379)
Stack created with Sapling. Best reviewed with ReviewStack.