Minimal repro extracted from our internal codebase. Our inference mode sees that
this arrow function is component-like and attempts to compile it, which then
fails because the function accesses `this` which we bailout on.
---
quality of life improvement as this seemed to be confusing (oops, and thanks for
the feedback!)
We don't *really* need static annotations for whether a function returns jsx
(e.g. should be rendered as a React element) or not (e.g. should be wrapped in a
wrapper component. This PR adds check for returned jsx objects at runtime
---
Tested by running diffing the output of `yarn sprout --verbose` between this PR
and base.
I ran into the same issue that @poteto and @gsathya (and probably @mofeiZ) have
run into: "Duplicate declaration of '$'" caused by Babel visiting a function
twice despite our calling `skip()`. This PR keeps a set of nodes that we have
already visited to avoid visiting them again, as a workaround for skip not
working.
# Test Plan
Synced to www and confirmed that the previous bug no longer reproduces, and the
compiled output looks sane.
Completes a todo (ie fixes a silly mistake) from a PR earlier in the stack, so
we now correctly recognize and compile arguments to `React.forwardRef()` and
`React.memo()`.
This PR changes the way we compile ArrowFunctionExpression to allow compiling
more cases, such as within `React.forwardRef()`. We no longer convert arrow
functions to function declarations. Instead, CodeGenerator emits a generic
`CodegenFunction` type, and `Program.ts` is responsible for converting that to
the appropriate type. The rule is basically:
* Retain the original node type by default. Function declaration in, function
declaration out. Arrow function in, arrow function out.
* When gating is enabled, we emit a ConditionalExpression instead of creating a
temporary variable. If the original (and hence compiled) functions are function
declarations, we force them into FunctionExpressions only here, since we need an
expression for each branch of the conditional. Then the rules are:
* If this is a `export function Foo` ie a named export, replace it with a
variable declaration with the conditional expression as the initializer, and the
function name as the variable name.
* Else, just replace the original function node with the conditional. This works
for all other cases.
I'm open to feedback but this seems like a pretty robust approach and will allow
us to support a lot of real-world cases that we didn't yet, so i think we need
_something_ in this direction.
We currently have multiple flags for targeting which functions to compile, but
they are actually mutually exclusive. This PR consolidates to a single
`compilationMode: 'annotation' | 'infer' | 'all'` flag:
* Annotation compiles only functions that explicitly opt-in with "use forget"
* Infer compiles explicitly opted-in functions (via "use forget") as well as any
known/inferred components or hooks:
* Component declarations
* Component or hook-like functions (same rules as the ESLint plugin but with an
extra check for whether it uses JSX or calls a hook)
* All compiles all top-level functions. We should get rid of this in a follow-up
and make tests use infer mode by default, and add explicit opt-ins where
necessary.
In all modes, "use no forget" always takes precedence and can be used to
opt-out. The default mode is now "infer".
Adds a new option to infer which functions to compile, based on React's ESLint
rule. The main difference is that in addition to checking the function name we
also check that it creates JSX or calls a hook. This should cover a significant
majority of components and reduce the chance of accidentally targeting
non-components, but it will leave some false negatives.
Note that some cases that the ESLint plugin infers as React functions don't work
yet: we don't compile FunctionExpressions, only ArrowFunctionExpressions, and
the way we handle ArrowFunctionExpression doesn't work with things like
forwardRef or variable declarations. We'll need more updates to fully handle all
these cases, which I'll do later in the stack.
Updates the parser-benchmark script to use a benchmarking framework, and
also brings in yargs to make it easier to handle parsing arguments.
Non-CI usage:
```
$ cd bench/parser-benchmark
$ yarn bench --help
Options:
--help Show help [boolean]
--ci [boolean] [default: false]
--sampleSize [number] [default: 1]
$ yarn bench --sampleSize=3
[ISOBENCH ENDED] Parser Benchmark (3 times)
OXC - 32 op/s. 3 samples in 9518 ms. 5.288x (BEST)
SWC - 20 op/s. 3 samples in 9487 ms. 3.256x
HermesParser - 6 op/s. 3 samples in 9124 ms. 1.000x (WORST)
Forget (Rust) - 15 op/s. 3 samples in 9749 ms. 2.499x
```
In CI this outputs JSON instead of logging to stdout. The results are
stored in github's action cache and used as a point of comparison when
new commits are pushed to main. The comparison should be shown on
workflow [summary
pages](https://github.com/facebook/react-forget/actions/runs/5956421081), but
nothing will be populated until we merge this PR.
We intentionally only run this workflow on main as any run would
override the last cached result, so a PR with multiple pushes would then
start having comparisons with itself.
Sorry about the thrash in advance! This removes the top level `forget` directory
which adds unnecessary nesting to our repo
Hopefully everything still works
ESTree expects the `range` value to be an array of `[start, end]`, we support
deserializing from that format but serialized as an object of `{start,end}`, now
we serialize to the array form.
We’ve had this feature turned on internally for a while with only a couple minor
bugs and no fundamental issues. It’s a necessary change for simplifying function
expression dependencies and context variables, so let’s remove the feature flag
and fix forward for any issues.
Adds SAFETY comments for the two instances of unsafe blocks that are *not* just
FFI. It seems like overkill to annotate all the FFI calls so i'm skipping that,
let me know if anyone has strong preferences otherwise.
This is mostly to test that the new configuration skips the Rust CI step if
there are no rust changes - yay, that worked! But also worth mentioning in the
readme so lets land.
Adds name resolution support for class declarations and expressions. Mostly this
involves _not_ visiting some `Identifier` nodes that don't actually represent
variables. For example, method names don't introduce new variables (but if they
are computed, they may _refer_ to variables).
Adds an option to pass a list of known globals into the semantic analyzer so
that references to globals can be checked. As a follow-up we'll need to
distinguish between different types of semantic analysis errors, so that callers
which don't want to validate globals can ignore "unknown variable" reference
errors while still handling definite errors such as duplicate declarations.
JavaScript has ~sane~ fun rules around where variables can be redeclared or not,
and which kinds of variables this applies to. Actually the rules are pretty
straightforward:
* `var` can be redeclared any number of times.
* In strict mode, other declarations cannot be redeclared within the same scope.
This implies that a `var` declaration cannot conflict with these other forms,
which must take into account hoisting. So you can't have a `var a; let a` in the
same scope, but you also can't have a `let a` at a scope and then a `var a`
which will hoist to that same scope.
The [temporal dead
zone](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#temporal_dead_zone_tdz),
often abbreviated TDZ, is the period between the start of its declaring block
and the line that contains the let/const/class declaration. Not all instances of
TDZ can be detected statically, because whether or not a TDZ error will occur at
runtime is a property of which control flow path is taken and whether some other
code has initialized the value. However, a subset of cases can be detected, and
that's what we implement here.
When we encounter a variable reference we record the next declaration id at that
point in time. Then when resolving references after visiting the program, we can
check: did the reference end up referring to a let/const/class binding whose id
is equal or greater to that "next declaration"? If so, it means the reference
refers to a variable that is provably declared later and is a known TDZ
violation. The catch is that when resolving references, we reset the "next
declaration" limit value when we bubble up out of a function scope. That's
because references to let/const within a function may occur after the
declaration, and we can't statically validate them.
Previously we attempted to resolve each reference at the close of its defining
scope, and if it couldn't be resolved yet we bubbled the unresolved reference up
to the parent scope. That approach isn't ideal for two reasons:
* First, it's inefficient since we may have to make multiple attempts to resolve
the same reference.
* Second, it's incorrect. There can be cases where we think we can resolve a
reference to a value defined in an outer scope, but there is a hoisted
declaration from an intermediate scope that we haven't seen yet.
The safest and most optimal thing is to just queue all references and resolve
them at the end.
Rather than always create a Global scope and then immediately add a Module child
node, we now set the kind of the root scope to global (for scripts) or module
(for modules) based on the program node.
Teaches the semantic analyzer about import statements. We now treat imports as
declarations, so that subsequent references to the imported value can be
resolved.
Teaches the hermes->estree conversion to convert source ranges. This means our
diagnostics now point to the actual source of the error:
<img width="903" alt="Screenshot 2023-08-14 at 5 39 50 PM"
src="https://github.com/facebook/react-forget/assets/6425824/2960d114-0cc6-4531-9e7d-00ff3bfb1eb3">
Of course I still need to teach our semantic analysis about imports, but that's
a separate issue!
---
I added ~20 more tests to Sprout to get more of a feel for what the test
framework would need to support all fixtures. I'm relatively confident that the
approach outlined in [the original workplace
post](https://fburl.com/workplace/ftu8woch) works to migrate almost all existing
fixture tests to sprout (TLDR: use shared functions when possible, otherwise
write helper functions in-file).
Add `shared-runtime` file to reduce the amount of overhead needed to set up a
fixture test.
- All generalizable functions and values should be added here (e.g.
`shallowCopy`, `deepCopy`, `sum`).
- Editor integration is set up through a custom tsconfig, so importing from
`shared-runtime` should just work (with hover annotations, click-to-definition,
etc).
<img width="682" alt="Screenshot 2023-08-17 at 11 09 47 AM"
src="https://github.com/facebook/react-forget/assets/34200447/78c3dff9-ba10-4057-b3f6-2fa842d19b1d">
---
Tested new test fixtures added to sprout by adding them to `testfilter.txt` and
running with `--verbose --filter`.
Note that there are no unexpected exceptions, and all logs + returned values
match.
```
feifei0@feifei0-mbp babel-plugin-react-forget % yarn sprout:build && yarn sprout
--verbose --filter
yarn run v1.22.19
$ yarn workspace sprout run build
$ rimraf dist && tsc --build
✨ Done in 2.07s.
yarn run v1.22.19
$ node ../sprout/dist/main.js --verbose --filter
PASS console-readonly
ok {"a":1,"b":2} [
"{ a: 1, b: 2 }",
"{ a: 1, b: 2 }",
"{ a: 1, b: 2 }",
"{ a: 1, b: 2 }",
"{ a: 1, b: 2 }"
]
PASS constant-propagate-global-phis-constant
ok <div>global string 0</div>
PASS constant-propagate-global-phis
ok <div>global string 1</div>
PASS dce-loop
ok 10
PASS destructure-capture-global
ok {"a":"value 1","someGlobal":{}}
PASS destructuring-mixed-scope-and-local-variables-with-default
ok {"media":null,"allUrls":["url1","url2","url3"],"onClick":"[[ function
params=1 ]]"}
PASS holey-array-expr
ok [null,"global string 0",{"a":1,"b":2}]
PASS infer-global-object
ok {"primitiveVal1":2,"primitiveVal2":null,"primitiveVal3":null}
PASS infer-phi-primitive
ok 1
PASS infer-types-through-type-cast.flow
ok 4
PASS issue933-disjoint-set-infinite-loop
ok [2]
PASS jsx-tag-evaluation-order-non-global
ok <div>StaticText1<div>StaticText2</div></div>
PASS jsx-tag-evaluation-order
ok <div>StaticText1string value 1<div>StaticText2</div></div>
PASS method-call
ok 4
PASS mutable-lifetime-loops
ok {"a":{"value":6},"b":{"value":5},"c":{"value":4},"d":{"value":6}}
PASS mutable-lifetime-with-aliasing
ok {"b":[{}],"value":[{"c":{},"value":"[[ cyclic ref *0 ]]"},null]}
PASS update-expression-in-sequence
ok [4,2,3,4]
PASS update-expression-on-function-parameter
ok [4,1,4,2,4,3,1,4,4]
PASS update-expression
ok {"x":1,"y":1,"z":2}
PASS useMemo-multiple-if-else
ok 2
20 Tests, 20 Passed, 0 Failed
✨ Done in 4.11s.
```
Sathya Gunasekaran
Joe Savona
Mofei Zhang
Lauren Tan