Fixes#1751. Function ids can be plain strings, and we can refer to them as
globals rather than via a local identifier. In addition to the bug from $1751
this also cleans up an existing todo.
This is a fix specifically for the `enableOptimizeFunctionExpression` feature
(disabled by default). I tried running all of our fixtures with that flag on
everywhere, and this was the only issue. It's actually extracted from another
fixture which is more complicated, this is a distilled version. There were two
bugs:
* DCE was running after LeaveSSA when it needs to run before. Fixing the order
means code inside the function expression stops getting removed.
* In the feature flag, context variables share Identifier instances with the
surrounding code, whereas they are distinct instances without the feature. This
meant that mutable ranges from inside the function propagated outside the
function, throwing off our inference. The fix was to reset the ranges of context
variables after inferring the function expression's effects.
inference
Integrates the new inference to detect definitive mutations of context
variables. This allows us to more precisely infer mutable function expressions
and validate that they aren't passed where frozen values are expected.
New pass to infer context variables which are definitively mutated,
distinguishing from context variables which _may_ have been mutated. See the
code comment on the new pass for more details.
Small tweak necessary for the subsequent PRs, refs and ref values take
precedence over other mutations when computing the effect type of context
variables. We always want to record ref access within a function as capture
(since we have later validation) rather than a mutation. For now this has no
impact, either order records a Capture. But it allows later diffs to make
non-ref cases have other effects.
The original version of the code wasn't checking return values. I missed this
since my examples were passing functions _into_ the return value, as opposed to
return the functions directly. This revealed some existing test fixtures that
were technically invalid, but easy to fix by changing the return value.
In InferReferenceEffects, locations that are ConditionallyMutate are either
recorded as a Mutate or Read, which means we lose the distinction btw
conditional/unconditional mutation in later passes. This PR changes to remember
that these places were conditionally mutable, used in later analysis.
Defaults to false, ie it runs the codegen pass. When enabled it will simply run
all passes up to codegen and then skip over it. Naming of this option is copied
from [TypeScript](https://www.typescriptlang.org/tsconfig#noEmit) which has the
same named option that makes the compiler only perform typechecking.
I'm adding this option primarily to get around some issues running the eslint
plugin on Meta code. The plugin would error because Forget would report
duplicate Babel AST nodes, which I presume would only occur during codegen. It
should also make it a tiny bit faster to not run codegen, which is a nice plus.
This was causing some issues in the eslint plugin where the babel `hub` wasn't
defined. afaik the hub is only setup when running the plugin as part of a babel
pipeline, instead of a manual parse/traversal. We're using some of that infra
for printing codeframes
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.
Joe Savona
Lauren Tan
Mofei Zhang