Open-source React Compiler #29061
Merged
Open-source React Compiler #29061
+358,424
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reduces a dep needed to be installed by users of the polyfill
Based on implementation of a similar case in D54776832.
Fixes T180509722. What happened is that the logic in LeaveSSA to find declarations within for initializers wasn't working with try/catch because the initializer block gets broken up with a maybe-throw after every instruction that can throw. These maybe-throws can then get turned into gotos by PruneMaybeThrows, so LeaveSSA has to handle both cases. The new logic scans from the start of the init block until reaching the end, and creates declarations for all StoreLocals. Note that we don't yet support maybe-throw in value blocks — that's already a todo — so the change here simply allows us to compile farther until reaching that other todo. But i've double-checked the HIR and it looks correct for this case, so it should just work once we fix that todo. I've also added a comment to help us remember (and of course, we'd have to add a snap fixture too)
Fixes T175282980. InferReactiveScopeVariables had logic to force assigning a scope to MethodCall property lookups with the idea of forcing the method call lookup to be in the same scope as the method call itself. But this doesn't work if we never assign a scope to the method call! That can happen if we're able to infer that the method call produces a primitive and doesn't need memoization. This PR changes things so that: * InferReactiveScopeVariables no longer assumes that MethodCall property values need a scope * We run a separate pass that ensures that _if_ a MethodCall has a scope, that it's property is in the scope, and that otherwise its property doesn't get a scope. This is similar to the existing passes that force a single scope for related instructions like ObjectMethod+ObjectExpression and fbt operands/calls.
Fixes T180504437. In MergeOverlappingReactiveScopes we track the active scopes and mark them as "ended" when reaching the first instruction after their mutable range. However, in cases of interleaving that will be merged, we could previously mark a scope as complete when it's original range was completed, even though the range would get extended post-merge. The fix here detects interleaving earlier, and eagerly updates the mutable ranges of the merged scopes to ensure that neither is "ended" earlier than it should. The repro here fails without this change.
Fixes T180504437. We expected `<fbt:param>` to always have no surrounding whitespace or have both leading and trailing whitespace, it can have one but not the other, though such cases are rare in practice.
This was an oversight in codegen. The entire pipeline supports multiple values in a for initializer, but codegen was dropping all but the first initializer.
I addressed some of the cases that lead to this invariant but there were still more. In this case, we have scopes like this: ``` scope @1 declarations=[t$0] { let t$0 = ArrayExpression [] if (...) { return null; } } scope @2 deps=[t$0] declarations=[t$1] { let t$1 = Jsx children=[t$0] ... } ``` Because scope 1 has an early return, PropagateEarlyReturns wraps its contents in a label and converts the returns to breaks: ``` scope @1 declarations=[t$0] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] if (...) { t$2 = null; break bb0; } } } scope @2 deps=[t$0] declarations=[t$1] { let t$1 = Jsx children=[t$0] ... } ``` But then MergeReactiveScopesThatInvalidateTogether smushes them together: ``` scope @1 declarations=[t$1] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] // <--- Oops! We're inside a block now if (...) { t$2 = null; break bb0; } } let t$1 = Jsx children=[t$0] ... } ``` Note that the `t$0` binding is now created inside the labeled block, so it's no longer accessible to the Jsx instruction which follows the labeled block. This isn't an issue with promoting temporaries or propagating outputs, but a simple issue of the labeled block (used for early return) introducing a new block scope. The solution here is to simply reorder the passes so that we transform for early returns after other optimizations. This means the jsx element will basically move inside the labeled block, solving the scoping issue: ``` scope @1 declarations=[t$1] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] // ok, same block scope as its use if (...) { t$2 = null; break bb0; } let t$1 = Jsx children=[t$0] // note this moved inside the labeled block } } ```
"Support" in the sense of dropping these on the floor and compiling, rather than bailing out with a todo. We already don't make any guarantees about which type annotations we'll preserve through to the output, so it seems fine for now to just drop type aliases.
This was one of the last invariants firing internally, this PR adds a minimal repro and the next PR makes it a todo.
We need to revisit the conversion from value blocks into ReactiveFunction. Or just revisit ReactiveFunction altogether (see my post about what this would look like). For now, makes this case a todo.
While i'm here, we know that there are a variety of cases that are not supported yet around combining value blocks with other syntax constructs. Since we're aware of these cases and detect them, we can make this a todo instead of an invariant.
[linter] rename ReactForgetDiagnostics to ReactCompilerRule
I addressed some of the cases that lead to this invariant but there were still more. In this case, we have scopes like this: ``` scope @1 declarations=[t$0] { let t$0 = ArrayExpression [] if (...) { return null; } } scope @2 deps=[t$0] declarations=[t$1] { let t$1 = Jsx children=[t$0] ... } ``` Because scope 1 has an early return, PropagateEarlyReturns wraps its contents in a label and converts the returns to breaks: ``` scope @1 declarations=[t$0] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] if (...) { t$2 = null; break bb0; } } } scope @2 deps=[t$0] declarations=[t$1] { let t$1 = Jsx children=[t$0] ... } ``` But then MergeReactiveScopesThatInvalidateTogether smushes them together: ``` scope @1 declarations=[t$1] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] // <--- Oops! We're inside a block now if (...) { t$2 = null; break bb0; } } let t$1 = Jsx children=[t$0] ... } ``` Note that the `t$0` binding is now created inside the labeled block, so it's no longer accessible to the Jsx instruction which follows the labeled block. This isn't an issue with promoting temporaries or propagating outputs, but a simple issue of the labeled block (used for early return) introducing a new block scope. The solution (in the next PR) is to simply reorder the passes so that we transform for early returns after other optimizations. This means the jsx element will basically move inside the labeled block, solving the scoping issue: ``` scope @1 declarations=[t$1] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] // ok, same block scope as its use if (...) { t$2 = null; break bb0; } let t$1 = Jsx children=[t$0] // note this moved inside the labeled block } } ```
Repro from T180504728 which reproduced internally and on playground, neither of which have #2687 yet. That PR (earlier in this stack) already fixes the issue, so i'm just adding the repro to help prevent regressions.
---
Previously, we always emitted `Memoize dep` instructions after the function
expression literal and depslist instructions
```js
// source
useManualMemo(() => {...}, [arg])
// lowered
$0 = FunctionExpression(...)
$1 = LoadLocal (arg)
$2 = ArrayExpression [$1]
$3 = Memoize (arg)
$4 = Call / LoadLocal
$5 = Memoize $4
```
Now, we insert `Memoize dep` before the corresponding function expression
literal:
```js
// lowered
$0 = StartMemoize (arg) <---- this moved up!
$1 = FunctionExpression(...)
$2 = LoadLocal (arg)
$3 = ArrayExpression [$2]
$4 = Call / LoadLocal
$5 = FinishMemoize $4
```
Design considerations:
- #2663 needs to understand which lowered instructions belong to a manual
memoization block, so we need to emit `StartMemoize` instructions before the
`useMemo/useCallback` function argument, which contains relevant memoized
instructions
- we choose to insert StartMemoize instructions to (1) avoid unsafe instruction
reordering of source and (2) to ensure that Forget output does not change when
enabling validation
This PR only renames `Memoize` -> `Start/FinishMemoize` and hoists
`StartMemoize` as described. The latter may help with stricter validation for
`useCallback`s, although testing is left to the next PR.
#2663 contains all validation changes
…ves subset
of dependencies from source
---
`validatePreserveExistingMemoizationGuarantees` previously checked
- manual memoization dependencies and declarations (the returned value) do not
"lose" memoization due to inferred mutations
```
function useFoo() {
const y = {};
// bail out because we infer that y cannot be a dependency of x as its
mutableRange
// extends beyond
const x = useMemo(() => maybeMutate(y), [y]);
// similarly, bail out if we find that x or y are mutated here
return x;
}
```
- manual memoization deps and decls do not get deopted due to hook calls
```
function useBar() {
const x = getArray();
useHook();
mutate(x);
return useCallback(() => [x], [x]);
}
```
This PR updates `validatePreserveExistingMemoizationGuarantees` with the
following correctness conditions:
*major change* All inferred dependencies of reactive scopes between
`StartMemoize` and `StopMemoize` instructions (e.g. scopes containing manual
memoization code) must either:
1. be produced from earlier within the same manual memoization block
2. exactly match an element of depslist from source
This assumes that the source codebase mostly follows the `exhaustive-deps` lint
rule, which ensures that deps lists are (1) simple expressions composing of
reads from named identifiers + property loads and (2) exactly match deps usages
in the useMemo/useCallback itself.
---
Validated that this does not change source by running internally on ~50k files
(no validation on `main`, no validation on this PR, and validation on this PR).
Remove private header from playground Before we miss removing this from the public release, I think we can remove this header now already. We're still behind a secret URL + password.
--- Many compiler errors have neither descriptions nor suggestions (e.g. most `todo` or `invariant` errors), so let's make those optional
This case is specific to early return inside an inlined IIFE (which can often occur as a result of dropping manual memoization). When we inline IIFEs, as a reminder we wrap the body in a labeled block and convert returns to assignment of a temporary + break out of the label. Those reassignments themselves are getting a reactive scope assigned since the reassigned value has a mutable range. They don't really need a mutable range or scope, though. And then the presence of the `break` statements means that we can sometimes exit out of the scope before reaching the end - leading to unreachable code. This can only occur though where _all the values are already memoized_. So the code works just fine and even memoizes just fine - it's just that we have some extraneous scopes and there is technically unreachable code. I'll fix in a follow-up, adding a repro here.
Extracts a helper from the repro earlier in the stack into a helper in shared-runtime. This makes it easy to verify that memoization is actually working.
The example earlier in the stack had unreachable code in the output because there was an unnecessary memoization block around an assignment. This was a holdover from before we moved the logic to expand mutable ranges for phis from LeaveSSA to InferMutableRanges. We were conservatively assigning a mutable range to all variables with a phi, even those that didn't strictly need one. Removing the range extension logic in LeaveSSA fixed the issue, but uncovered the fact that AlignReactiveScopesToBlockScopes was missing a case to handle optionals. ## Test Plan Synced internally and ran a snapshot/comparison of compilation before/after (P1197734337 for those curious). The majority of components get fewer memo slots thanks to not needing to memoize non-allocating value block expressions like ternaries/optionals. In a few cases, the fact that we're no longer assigning a mutable range for value blocks (unless there is actually a mutation!) means we get more fine-grained memoization and increase the number of memoization blocks. So overall this appears to be correct, improve memoization, and reduce code size.
Fbt violates the JSX spec by using a lowercase function as a tagname, even though lowercase names are reserved for builtins. Here we detect cases where there is an `<fbt>` tag where `fbt` is a local identifier and throw a todo.
Fbt enums appear to rely on source locations and something that we're doing (maybe destructuring?) isn't preserving locations such that the fbt plugin breaks.
|
congrats everyone! fantastic work, it's surreal the amount of work that went into this PR |
|
Hell yeah! |
|
Finally! |
|
Congrats!! Thank you for your hard work ❤️ |
|
Wow! Congrats |
|
Congrats 🎉 |
|
This is unreal! |
|
go react! |
|
I just feel the vibe that I need to be here!! 🚀 |
|
Yay! |
|
that is beyond rad! congrats |
|
Congratulations 🎊 |
|
+1 |
|
congrats!! |
|
Wow.. |
|
yes |
|
Congratulations, looking forward to future improvements!) |
|
You guys are making history 👏 |
|
🔥👏🗿 |
|
Lets go!! 🚀 |
|
Fix the workflow please |
|
Way to go Idaho! 👏 ❤️ |
|
Congrats 🎉 |
|
LGTM |
|
🥇 |
|
Congrats! |
|
Well done 😎 |
|
Congrats 🧑💻 |
|
Lets go |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
React Compiler is open source!