The previous PR added name hints for anonymous functions, but didn't
handle the case of outlined functions. Here we do some cleanup around
function `id` and name hints:
* Make `HIRFunction.id` a ValidatedIdentifierName, which involved some
cleanup of the validation helpers
* Add `HIRFunction.nameHint: string` as a place to store the generated
name hints which are not valid identifiers
* Update Codegen to always use the `id` as the actual function name, and
only use nameHint as part of generating the object+property wrapper for
debug purposes.
This ensures we don't conflate synthesized hints with real function
names. Then, we also update OutlineFunctions to use the function name
_or_ the nameHint as the input to generating a unique identifier. This
isn't quite as nice as the object form since we lose our formatting, but
it's a simple step that gives more context to the developer than `_temp`
does.
Switching to output the object+property lookup form for outlined
functions is a bit more involved, let's do that in a follow-up.
Adds a `@enableNameAnonymousFunctions` feature to infer helpful names
for anonymous functions within components and hooks. The logic is
inspired by a custom Next.js transform, flagged to us by @eps1lon, that
does something similar. Implementing this transform within React
Compiler means that all React (Compiler) users can benefit from more
helpful names when debugging.
The idea builds on the fact that JS engines try to infer helpful names
for anonymous functions (in stack traces) when those functions are
accessed through an object property lookup:
```js
({'a[xyz]': () => {
throw new Error('hello!')
} }['a[xyz]'])()
// Stack trace:
Uncaught Error: hello!
at a[xyz] (<anonymous>:1:26) // <-- note the name here
at <anonymous>:1:60
```
The new NameAnonymousFunctions transform is gated by the above flag,
which is off by default. It attemps to infer names for functions as
follows:
First, determine a "local" name:
* Assigning a function to a named variable uses the variable name.
`const f = () => {}` gets the name "f".
* Passing the function as an argument to a function gets the name of the
function, ie `foo(() => ...)` get the name "foo()", `foo.bar(() => ...)`
gets the name "foo.bar()". Note the parenthesis to help understand that
it was part of a call.
* Passing the function to a known hook uses the name of the hook,
`useEffect(() => ...)` uses "useEffect()".
* Passing the function as a JSX prop uses the element and attr name, eg
`<div onClick={() => ...}` uses "<div>.onClick".
Second, the local name is combined with the name of the outer
component/hook, so the final names will be strings like `Component[f]`
or `useMyHook[useEffect()]`.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34410).
* #34434
* __->__ #34410
Small fix to make all descriptions consistently printed with a single
period at the end.
Ran `grep -rn "description:" packages/babel-plugin-react-compiler/src
--include="*.ts" --exclude-dir="__tests__" | grep '\.\s*["\`]'` to find
all descriptions ending in a period and manually fixed them.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34404).
* #34409
* __->__ #34404
With #34176 we now have granular lint rules created for each compiler
ErrorCategory. However, we had remnants of our old error severities
still in use which makes reporting errors quite clunky. Previously you
would need to specify both a category and severity which often ended up
being the same.
This PR moves severity definition into our rules which are generated
from our categories. For now I decided to defer "upgrading" categories
from a simple string to a sum type since we are only using severities to
map errors to eslint severity.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34401).
* #34409
* #34404
* #34403
* #34402
* __->__ #34401
NOTE: this is a merged version of @mofeiZ's original PR along with my
edits per offline discussion. The description is updated to reflect the
latest approach.
The key problem we're trying to solve with this PR is to allow
developers more control over the compiler's various validations. The
idea is to have a number of rules targeting a specific category of
issues, such as enforcing immutability of props/state/etc or disallowing
access to refs during render. We don't want to have to run the compiler
again for every single rule, though, so @mofeiZ added an LRU cache that
caches the full compilation output of N most recent files. The first
rule to run on a given file will cause it to get cached, and then
subsequent rules can pull from the cache, with each rule filtering down
to its specific category of errors.
For the categories, I went through and assigned a category roughly 1:1
to existing validations, and then used my judgement on some places that
felt distinct enough to warrant a separate error. Every error in the
compiler now has to supply both a severity (for legacy reasons) and a
category (for ESLint). Each category corresponds 1:1 to a ESLint rule
definition, so that the set of rules is automatically populated based on
the defined categories.
Categories include a flag for whether they should be in the recommended
set or not.
Note that as with the original version of this PR, only
eslint-plugin-react-compiler is changed. We still have to update the
main lint rule.
## Test Plan
* Created a sample project using ESLint v9 and verified that the plugin
can be configured correctly and detects errors
* Edited `fixtures/eslint-v9` and introduced errors, verified that the w
latest config changes in that fixture it correctly detects the errors
* In the sample project, confirmed that the LRU caching is correctly
caching compiler output, ie compiling files just once.
Co-authored-by: Mofei Zhang <feifei0@meta.com>
Much of the logic in the new validation pass is already implemented in
DropManualMemoization, so let's combine them. I opted to keep the
environment flag so we can more precisely control the rollout.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34001).
* #34022
* #34002
* __->__ #34001
Adds a new validation pass to validate against `useMemo`s that don't
return anything. This usually indicates some kind of "useEffect"-like
code that has side effects that need to be memoized to prevent
overfiring, and is an anti-pattern.
A follow up validation could also look at the return value of `useMemo`s
to see if they are being used.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33990).
* #34022
* #34002
* #34001
* __->__ #33990
* #33989
Work in progress, i'm experimenting with revamping our diagnostic infra.
Starting with a better format for representing errors, with an ability
to point ot multiple locations, along with better printing of errors. Of
course, Babel still controls the printing in the majority case so this
still needs more work.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33751).
* #33981
* #33777
* #33767
* #33765
* #33760
* #33759
* #33758
* __->__ #33751
* #33752
* #33753
Squashed, review-friendly version of the stack from
https://github.com/facebook/react/pull/33488.
This is new version of our mutability and inference model, designed to
replace the core algorithm for determining the sets of instructions
involved in constructing a given value or set of values. The new model
replaces InferReferenceEffects, InferMutableRanges (and all of its
subcomponents), and parts of AnalyzeFunctions. The new model does not
use per-Place effect values, but in order to make this drop-in the end
_result_ of the inference adds these per-Place effects.
I'll write up a larger document on the model, first i'm doing some
housekeeping to rebase the PR.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33494).
* #33571
* #33558
* #33547
* #33543
* #33533
* #33532
* #33530
* #33526
* #33522
* #33518
* #33514
* #33513
* #33512
* #33504
* #33500
* #33497
* #33496
* #33495
* __->__ #33494
* #33572
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
(Almost) all pragmas are now one of the following:
- `@...TestOnly`: custom pragma for test fixtures
- `@<configName>` | `@<configName>:true`: enables with either true or a
default enabled value
- `@<configName>:<json value>`
This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal.
We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error.
This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default.
ghstack-source-id: 075a731444
Pull Request resolved: https://github.com/facebook/react/pull/33079
Previously the CompileSuccess event would emit first before CompileSkip,
so the lsp's codelens would incorrectly flag skipped components/hooks
(via 'use no memo') as being optimized.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33012).
* __->__ #33012
* #33011
* #33010
Adds a new codeaction event in the compiler and handler in forgive. This
allows you to remove a dependency array when you're editing a range that
is within an autodep eligible function.
Co-authored-by: Jordan Brown <jmbrown@meta.com>
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33000).
* #33002
* #33001
* __->__ #33000
Co-authored-by: Jordan Brown <jmbrown@meta.com>
Currently, inferred effect dependencies are considered a
"compiler-required" feature. This means that untransformed callsites
should escalate to a build error.
`ValidateNoUntransformedReferences` iterates 'special effect' callsites
and checks that the compiler was able to successfully transform them.
Prior to this PR, this relied on checking the number of arguments passed
to this special effect.
This obviously doesn't work with `noEmit: true`, which is used for our
eslint plugin (this avoids mutating the babel program as other linters
run with the same ast). This PR adds a set of `babel.SourceLocation`s to
do best effort matching in this mode.
Avoid failing builds when imported function specifiers conflict by using
babel's `generateUid`. Failing a build is very disruptive, as it usually
presents to developers similar to a javascript parse error.
```js
import {logRender as _logRender} from 'instrument-runtime';
const logRender = () => { /* local conflicting implementation */ }
function Component_optimized() {
_logRender(); // inserted by compiler
}
```
Currently, we fail builds (even in `panicThreshold:none` cases) when
import specifiers are detected to conflict with existing local
variables. The reason we destructively throw (instead of bailing out) is
because (1) we first generate identifier references to the conflicting
name in compiled functions, (2) replaced original functions with
compiled functions, and then (3) finally check for conflicts.
When we finally check for conflicts, it's too late to bail out.
```js
// import {logRender} from 'instrument-runtime';
const logRender = () => { /* local conflicting implementation */ }
function Component_optimized() {
logRender(); // inserted by compiler
}
```
Updates ~all of our validations to return a Result, and then updates callers to either unwrap() if they should bailout or else just log.
ghstack-source-id: 418b5f5aa2
Pull Request resolved: https://github.com/facebook/react/pull/32688
React uses function identity to determine whether a given JSX expression represents the same type of component and should reconcile (keep state, update props) or replace (teardown state, create a new instance). This PR adds off-by-default validation to check that developers are not dynamically creating components during render.
The check is local and intentionally conservative. We specifically look for the results of call expressions, new expressions, or function expressions that are then used directly (or aliased) as a JSX tag. This allows common sketchy but fine-in-practice cases like passing a reference to a component from a parent as props, but catches very obvious mistakes such as:
```js
function Example() {
const Component = createComponent();
return <Component />;
}
```
We could expand this to catch more cases, but this seems like a reasonable starting point. Note that I tried enabling the validation by default and the only fixtures that error are the new ones added here. I'll also test this internally. What i'm imagining is that we enable this in the linter but not the compiler.
ghstack-source-id: e7408c0a55
Pull Request resolved: https://github.com/facebook/react/pull/32683
Traverse program after running compiler transform to find untransformed
references to compiler features (e.g. `inferEffectDeps`, `fire`).
Hard error to fail the babel pipeline when the compiler fails to
transform these features to give predictable runtime semantics.
Untransformed calls to functions like `fire` will throw at runtime
anyways, so let's fail the build to catch these earlier.
Note that with this fails the build *regardless of panicThreshold*
Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of
`run` parameters. This is a minimal difference but lets us explicitly
opt out certain compiler passes based on mode parameters, instead of
environment configurations
Retry flags don't really make sense to have in `EnvironmentConfig`
anyways as the config is user-facing API, while retrying is a compiler
implementation detail.
(per @josephsavona's feedback
https://github.com/facebook/react/pull/32164#issuecomment-2608616479)
> Re the "hacky" framing of this in the PR title: I think this is fine.
I can see having something like a compilation or output mode that we use
when running the pipeline. Rather than changing environment settings
when we re-run, various passes could take effect based on the
combination of the mode + env flags. The modes might be:
>
> * Full: transform, validate, memoize. This is the default today.
> * Transform: Along the lines of the backup mode in this PR. Only
applies transforms that do not require following the rules of React,
like `fire()`.
> * Validate: This could be used for ESLint.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32511).
* #32512
* __->__ #32511
This change fixes a coverage hole in rolling out with `gating`. Prior to
this PR, configuring `gating` causes React Compiler to bail out of
optimizing some functions.
This means that it's not entirely safe to cutover from `gating` enabled
for all users (i.e. rolled out 100%) to removing the `gating` config
altogether, as new functions may be opted into compilation when they
stop bailing out due to gating-specific logic.
This is technically slightly slower due to the additional function
indirection. An alternative approach is to recommend running a codemod
to insert `use no memo`s on currently-bailing out functions before
removing the`gating` config.
---
Tested [internally](
https://fburl.com/diff/q982ovua) by enabling on a page that previously
had a few hundred bailouts due to gating + hoisted function declarations
and (1) clicking around locally and (2) running a bunch of e2e tests
Reduce false positive bailouts by using the same
`isReferencedIdentifier` logic that the compiler also uses for
determining context variables and a function's own hoisted declarations.
Details:
Previously, we counted every babel identifier as a reference. This is
problematic because babel counts most string symbols as an identifier.
```js
print(x); // x is an identifier as expected
obj.x // x is.. also an identifier here
{x: 2} // x is also an identifier here
```
This PR adds a check for `isReferencedIdentifier`. Note that only
non-lval
references pass this check. This should be fine as we don't need to
hoist function declarations before writes to the same lvalue (which
should error in strict mode anyways)
```js
print(x); // isReferencedIdentifier(x) -> true
obj.x // isReferencedIdentifier(x) -> false
{x: 2} // isReferencedIdentifier(x) -> false
x = 2 // isReferencedIdentifier(x) -> false
```
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32596).
* __->__ #32596
* #32595
* #32594
* #32593
* #32522
* #32521
Currently in the `compiler` workspace, we invoke esbuild directly to
build most packages (with the exception of `snap`). This has been mostly
fine, but does not allow us to do things like generate type declaration
files.
I would like #32416 to be able to consume the merged
eslint-plugin-react-compiler from source rather than via npm, and one of
the things that has come up from my exploration in that stack using the
compiler from source is that babel-plugin-react-compiler is missing type
declarations. This is primarily because React's build process uses
rollup + rollup-plugin-typescript, which runs tsc. So the merged plugin
needs to typecheck properly in order to build. An alternative might be
to migrate to something like babel with rollup instead to simply strip
types rather than typecheck before building. The minor downside of that
approach is that we would need to manually maintain a d.ts file for
eslint-plugin-react-hooks. For now I would like to see if this PR helps
us make progress rather than go for the slightly worse alternative.
[`tsup`](https://github.com/egoist/tsup) is esbuild based so build
performance is comparable. It is slower when generating d.ts files, but
it's still much faster than rollup which we used prior to esbuild. For
now, I have turned off `dts` by default, and it is only passed when
publishing on npm.
If you want to also generate d.ts files you can run `yarn build --dts`.
```
# BEFORE: build all compiler packages (esbuild)
$ time yarn build
✨ Done in 15.61s.
yarn build 13.82s user 1.54s system 96% cpu 15.842 total
# ---
# AFTER: build all compiler packages (tsup)
$ time yarn build
✨ Done in 12.39s.
yarn build 12.58s user 1.68s system 106% cpu 13.350 total
# ---
# AFTER: build all compiler packages and type declarations (tsup)
$ time yarn build --dts
✨ Done in 30.69s.
yarn build 43.57s user 3.20s system 150% cpu 31.061 total
```
I still need to test if this unblocks #32416 but this stack can be
landed independently though as we could probably just release type
declarations on npm. No one should be using the compiler directly, but
if they really wanted to, lack of type declarations would not stop them
(cf React secret internals).
Note that I still kept esbuild as we still use it directly for forgive.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32550).
* #32551
* __->__ #32550
Adds a new Timing logger event to the compiler which currently only
records the walltime of running the compiler from the time the babel
plugin's Program visitor enters to the time it exits.
To enable, run the compiler with `ENABLE_REACT_COMPILER_TIMINGS=1 ...`
or `export ENABLE_REACT_COMPILER_TIMINGS=1` to set it by default.
Hacky retry pipeline for when transforming `fire(...)` calls encounters
validation, todo, or memoization invariant bailouts. Would love feedback
on how we implement this to be extensible to other compiler
non-memoization features (e.g. inlineJSX)
Some observations:
- Compiler "front-end" passes (e.g. lower, type, effect, and mutability
inferences) should be shared for all compiler features -- memo and
otherwise
- Many passes (anything dealing with reactive scope ranges, scope blocks
/ dependencies, and optimizations such as ReactiveIR #31974) can be left
out of the retry pipeline. This PR hackily skips memoization features by
removing reactive scope creation, but we probably should restructure the
pipeline to skip these entirely on a retry
- We should maintain a canonical set of "validation flags"
Note the newly added fixtures are prefixed with `bailout-...` when the
retry fire pipeline is used. These fixture outputs contain correctly
inserted `useFire` calls and no memoization.
For now we just reject all calls of impure functions, and the validation
is off by default. Going forward we can make this more precise and only
reject impure functions called during render.
Note that I was intentionally imprecise in the return type of these
functions in order to avoid changing output of existing code. We lie to
the compiler and say that Date.now, performance.now, and Math.random
return unknown mutable objects rather than primitives. Once the
validation is complete and vetted we can switch this to be more precise.
Joseph SavonaandGitHub
laurenandGitHub
Jordan BrownandGitHub
mofeiZandGitHub
Joe Savona
Dimitri POSTOLOVandGitHub