The existing errors thrown were marked as InvalidInput, which is now considered
critical. This was causing an error in the sync since we had 2 occurrences of
the errors being thrown. These are really todos and not invalid code.
Turning off this flag makes only critical errors throw, so TODO errors will no
longer be surfaced by the plugin. The previously failing test for unsupported
syntax is now valid.
We were incorrectly calculating the dependencies of nested lambdas, because the
"component" scope was incorrectly set to the next closest parent rather than
outermost React function (component/hook) being compiled.
This PR adds a new feature which enables additional validation/optimization of
function expressions, gated by the `enableOptimizeFunctionExpressions` feature
flag. When disabled, we actually revert the changes earlier in this stack, and
do all our lowering of function expressions in AnalyzeFunctions.
When the feature is enabled, we incrementally process function expressions in
the various compilation stages, eg InferTypes infers into function expressions,
ConstantPropagation propagates constants into function expressions, etc. Because
this stage optimizes function expressions, in this mode codegen uses the HIR as
the source rather than the original babel node.
The feature is disabled by default so it has no impact on generated code. For
now i've enabled the feature on just one test to demonstrate constant
propagation into a function expression.
Updates InferTypes to perform type inference across function boundaries.
Specifically InferTypes is now responsible for driving type inference of
function expressions (rather than deferring to AnalyzeFunctions to infer
functions), and type inference now traverses into function expressions and
infers types of free variables taking into account information from the outer
context. This relies on the fact that identifier ids are consistent across
function expression boundaries and that all free variables in functions are
guaranteed to be effectively `const`, since we promote non-const variables used
in function expressions to context variables.
Currently the process of lowering function expression bodies is deferred until
AnalyzeFunctions. However, per the motivation of the previous PR, we'd like to
be able to perform inference across function expression boundaries. To do that
we need to use consistent identifier ids across function expression boundaries.
This requires SSA conversion of function expressions at the same time as we
enter SSA.
To start, this PR moves MergeConsecutiveBlocks for function expressions into
that phase.
Generally we should reserve the `packages` directory for packages that
are needed to run the compiler, while `apps` is for frontends that might
make use of the compiler (and optionally, related packages)
This PR updates the conditions for which variables are promoted to context
variables.
The previous rule was to promote any variable where the variable was reassigned
in some function expression other than the function that declared the variable.
Notably, this meant that we did not use context variables for variables which
were captured in a function expression, but reassigned _outside_ a function
expression.
The new rule is more consistent: we promote any variable which is a) reassigned
somewhere and b) referenced in some function expression outside of their
declaring function. The implementation builds two sets of identifiers, one for
each criteria, then takes the union of these two sets.
## Motivation
The motivation for this change is to unblock additional validations and
optimizations of function expressions. It's currently difficult to translate
metadata that we infer about identifiers outside of a function expression into
metadata about the identifiers within a function expression — for example to
infer types within function expression bodies based on type information outside,
propagate constants into functions, infer reference effects, etc.
After this change, the only free variables inside function expressions will be
variables that are effectively `const` - never reassigned anywhere. Thus it will
be safe to renumber those identifiers to match the outer context (during
EnterSSA), making it trivial to map metadata from outside the function into the
function.
This change also more closely models the runtime representation — any variable
referenced in a function, and reassigned somewhere, would have to be compiled
(ie in a JS engine) to use a context variable.
The goal of this PR is to improve ValidateNoRefAccessInRender to find function
expressions which a) access refs and b) may be called during render. Currently
we always allow ref access in any function expression, but that's obviously
optimistic.
For the approach, the observation is that we already have a system that tells us
whether a function may get called — mutable range inference. So long as we
consider a function "mutable", we'll infer a range for it, but the problem is
that we don't currently view functions which depend on refs to be mutable. So
here I'm doing ~~sort of~~ a hack to force function deps on refs to be treated
as Effect.Capture. This is enough for the function to be considered mutable, for
a mutable range to be assigned, and for us to detect that during ref validation.
I don't love the hack, i'm open to other ideas!
---
- [patch] Find context variables within FunctionDeclarations (previously
missing)
- [todo] Need to fix non-allocating values / variables being DCE'd
One approach is to label everything referenced by a lambda as context variables.
I couldn't come up with other examples that break without this change, so I
wonder if something lighter / hackier works just as well. (My only hesitation is
that we may end up losing out on potential optimizations for everything aliased
to these variables).
---
(wip, waiting for feedback on workplace post)
- remove calls to `isInROMode`, as we want to log all mutations after 'freezing'
a value (both within and outside of render cycle)
- add `source` parameter -- this is the function name of the parent component /
hook
- Gating checks for debugging / profiling can be moved to within the
instrumentation or makeReadOnly functions. This simplifies codegen and reduces
code bloat.
- Warn if importing conflicting identifiers
- Aggregate imports from the same source
---
Emit calls to makeReadOnly for memoized values.
```js
function MyComponent() {
let x;
if (c_0) {
x = // ... (recompute x)
$[0] = __DEV__ ? makeReadOnly(x, "MyComponent") : x;
} else {
x = $[0]
}
}
```
- import source / specifier should be configurable, as
- we'll likely want to add gk gating to `makeReadOnly` itself to reduce codesize
bloat
- each Forget project needs different logging and filter configurations
- codegen function name as an argument for easier debugging
- only freeze memoized outputs
This is an attempt to scaffold yarn workspaces into our repo with as minimal as
possible changes to our current setup and directory structure. Essentially this
PR moves current `src` into `packages/babel-plugin-react-forget` as a first step
to keep everything working. Later on when we're ready we can split out a
`react-compiler` package that is decoupled from Babel, but it's too early for
that now
Not to get ahead of myself (sorry i had to), but i think this is the last order
of evaluation bug. At least it's the last one we know of[1]. Per the previous
PR, the issue is that constant propagation can copy the last value of a sequence
expression to where the sequence is used, leaving the original sequence
expression out of order after other instructions are moved around. We fix that
here by explicitly skipping constant propagation for the last value of a
sequence block.
[1] There are some places where we _would_ have evaluation order bugs if we
allowed arbitrary expressions, but we explicitly limit the expressions we allow
in those places. For the curious: switch test case values and destructuring
default values.
Changes the lowering for sequence expressions to use the new terminal. When
converting to a ReactiveFunction, we convert these terminals into
ReactiveSequenceValues, which nests the instructions and preserves order of
evaluation in the output.
The only catch is constant propagation — constant propagation breaks
order-of-evaluation because it can effectively copy the final value of a
sequence elsewhere, leaving the original sequence in the wrong place. I'll
address that in a follow-up.
I realized that we can use our value block system to fix _most_ of the remaining
order-of-evaluation issues we had with sequence expressions. This PR adds a new
SequenceTerminal to HIR; there is already a ReactiveFunction equivalent
(ReactiveSequenceValue) that the next PR will convert this terminal into.
Handles three more cases:
* Template literals
* deletion (property/computed)
* type casts
Only the latter has an observable impact, though i added tests for deletion just
in case and found a bug. For type casts, they're reasonably common internally
for fixmes, so this PR will help to ensure we don't drop type information just
because of a cast.
Renames some error fixtures for clarity, "error.invalid-*" are fixtures that are
expected to fail for invalid input, where other "error.*" fixtures are basically
todos. While i was here i clarified the error messages for invalid useMemo
callbacks, and changes the error severity from Invariant to InvalidInput.
Fixes one more category of bug. For assignment expressions, we validating
against redeclaring a global variable when the assignment target was an
identifier, but not when the global was reassigned via destructuring. This PR
adds a `lowerIdentifierForAssignment()` helper and uses it for assignment of all
identifier variants, including destructuring.
I reviewed the test cases we have marked as bugs ("_bug.*") and realized that
several of them are already fixed — woohoo! Then one wasn't fixed _yet_: our
type inference loses track of refs if you stash them inside an object/array. But
that's why I added the ValidateNoRefAccessInRender pass, which i've updated to
detect and reject these invalid cases. There are now only a few bugs left (more
fixes coming).
Lauren Tan
Joe Savona
Mofei Zhang