The fixture from the previous PR was getting inconsistent behavior because of the following:
1. Create an object in a useMemo
2. Create a callback in a useCallback, where the callback captures the object from (1) into a local object, then passes that local object into a logging method. We have to assume the logging method could modify the local object, and transitively, the object from (1).
3. Call the callback during render.
4. Pass the callback to JSX.
We correctly infer that the object from (1) is captured and modified in (2). However, in (4) we transitively freeze the callback. When transitively freezing functions we were previously doing two things: updating our internal abstract model of the program values to reflect the values as being frozen *and* also updating function operands to change their effects to freeze.
As the case above demonstrates, that can clobber over information about real potential mutability. The potential fix here is to only walk our abstract value model to mark values as frozen, but _not_ override operand effects. Conceptually, this is a forward data flow propagation — but walking backward to update effects is pushing information backwards in the algorithm. An alternative would be to mark that data was propagated backwards, and trigger another loop over the CFG to propagate information forward again given the updated effects. But the fix in this PR is more correct.
ghstack-source-id: c05e716f37827cb5515a059a1f0e8e8ff94b91df
Pull Request resolved: https://github.com/facebook/react/pull/30766
This fixture bails out on ValidatePreserveExistingMemo but would ideally memoize since the original memoization is safe. It's trivial to make it pass by commenting out the commented line (`LogEvent.log(() => object)`). I would expect the compiler to infer this as possible mutation of `logData`, since `object` captures a reference to `logData`. But somehow `logData` is getting memoized successfully, but we still infer the callback, `setCurrentIndex`, as having a mutable range that extends to the `setCurrentIndex()` call after the useCallback.
ghstack-source-id: 4f82e345102f82f6da74de3f9014af263d016762
Pull Request resolved: https://github.com/facebook/react/pull/30764
Per comments on the new validation pass, this disallows creating JSX (expression/fragment) within a try statement. Developers sometimes use this pattern thinking that they can catch errors during the rendering of the element, without realizing that rendering is lazy. The validation allows us to teach developers about the error boundary pattern.
ghstack-source-id: 0bc722aeaed426ddd40e075c008f0ff2576e0c33
Pull Request resolved: https://github.com/facebook/react/pull/30725
Addresses a todo from a while back. We now validate environment options when parsing the plugin options, which means we can stop re-parsing/validating in later phases.
ghstack-source-id: b19806e843e1254716705b33dcf86afb7223f6c7
Pull Request resolved: https://github.com/facebook/react/pull/30726
This PR updates the babel plugin to continue the compilation pipeline as
normal on components/hooks that have been opted out using a directive.
Instead, we no longer emit the compiled function when the directive is
present.
Previously, we would skip over the entire pipeline. By continuing to
enter the pipeline, we'll be able to detect if there are unused
directives.
The end result is:
- (no change) 'use forget' will always opt into compilation
- (new) 'use no forget' will opt out of compilation but continue to log
errors without throwing them. This means that a Program containing
multiple functions (some of which are opted out) will continue to
compile correctly
ghstack-source-id: 5bd85df2f8
Pull Request resolved: https://github.com/facebook/react/pull/30720
Summary:
The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default.
ghstack-source-id: d3ab5f1b28b7aed0f0d6d69547bb638a1e326b66
Pull Request resolved: https://github.com/facebook/react/pull/30716
Summary:
We previously were excessively strict about preventing functions that access refs from being returned--doing so is potentially valid for hooks, because the return value may only be used in an event or effect.
ghstack-source-id: cfa8bb1b54e8eb365f2de50d051bd09e09162d7b
Pull Request resolved: https://github.com/facebook/react/pull/30724
Summary:
Since we want to make ref-in-render errors enabled by default, we should position those errors at the location of the read. Not only will this be a better experience, but it also aligns the behavior of Forget and Flow.
This PR also cleans up the resulting error messages to not emit implementation details about place values.
ghstack-source-id: 1d1131706867a6fc88efddd631c4d16d2181e592
Pull Request resolved: https://github.com/facebook/react/pull/30723
Test Plan:
Documents that useCallback calls interfere with it being ok for refs to escape as part of functions into jsx
ghstack-source-id: a5df427981ca32406fb2325e583b64bbe26b1cdd
Pull Request resolved: https://github.com/facebook/react/pull/30714
Summary:
Refs, as stable values that the rules of react around mutability do not apply to, currently are treated as having mutable ranges, and through aliasing, this can extend the mutable range for other values and disrupt good memoization for those values. This PR excludes refs and their .current values from having mutable ranges.
Note that this is unsafe if ref access is allowed in render: if a mutable value is assigned to ref.current and then ref.current is mutated later, we won't realize that the original mutable value's range extends.
ghstack-source-id: e8f36ac25e2c9aadb0bf13bd8142e4593ee9f984
Pull Request resolved: https://github.com/facebook/react/pull/30713
Test Plan:
Builds support for a.x++ and friends. Similar to a.x += y, emits it as an assignment expression.
ghstack-source-id: 8f3979913aad561cdba70464c3cc5f0ee95887b5
Pull Request resolved: https://github.com/facebook/react/pull/30697
Summary:
It doesn't seem as though this invariant was necessary
ghstack-source-id: b27e76525911d5cfc1991b5cfdb7b2074c039e21
Pull Request resolved: https://github.com/facebook/react/pull/30699
Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render.
This validation is off by default, i'm putting this up for discussion and to experiment with it internally.
ghstack-source-id: 5f385ddab59561ec3939ae5ece265dfee4f2cb56
Pull Request resolved: https://github.com/facebook/react/pull/30685
Summary:
UseTransition is a builtin hook that returns a stable value, like useState. This PR represents that in Forget, and marks the startTransition function as stable.
ghstack-source-id: 0e76a64f2d0c86a4eb55c620922b4698250bb5c3
Pull Request resolved: https://github.com/facebook/react/pull/30681
Summary:
In theory, as I understand it, the result of a useRef will never change between renders, because we'll always provide the same ref value consistently. That means that memoization that depends on a ref value will never re-compute, so I think we could not infer it as a dependency in Forget. This diff, however, doesn't do that: it instead allows the validatePreserveExistingMemoizationGuarantees analysis to admit mismatches between explicit dependencies and implicit ones when the implicit dependency is a ref that doesn't exist in source.
ghstack-source-id: 685d859d1eed5d1e19dbbbfadc75be3875ddb6ea
Pull Request resolved: https://github.com/facebook/react/pull/30679
Summary:
As title. Better support for flow typing, bugfixes, etc fixes these
ghstack-source-id: 6326653ce42b33b6c1c76a494434d133382ca80a
Pull Request resolved: https://github.com/facebook/react/pull/30591
Summary:
Builds support for macros that are invoked as methods rather than just function calls or jsx.
We now record macros as a schema that represents arbitrary member expressions including wildcards (so we can support, e.g., myMacro.*.foo.bar). When examining PropertyLoads in the macro memoization stage, we build up a map of partially-satisfied macro patterns until we determine that the pattern has been fully satisfied, at which point we treat the result of the PropertyLoad as a macro value.
ghstack-source-id: d78d9ba7041968c861ffa110fb7882b339a0e257
Pull Request resolved: https://github.com/facebook/react/pull/30589
We made block types explicit a long time ago, this comment is super stale
ghstack-source-id: 810a34bb4c14a3f4541003db23ffb7ad91aecc8c
Pull Request resolved: https://github.com/facebook/react/pull/30633
Previously the compiler would add an import for the specified context
callee even if the context access was not lowered, leading to unused
imports.
This PR tracks if lowering has happened and adds the import only when
necessary.
ghstack-source-id: 6ad794da41116e1034783b6c4a58fbfe7790343e
Pull Request resolved: https://github.com/facebook/react/pull/30628
If a value is specified for the LowerContextAccess environment config,
we rewrite the callee from 'useContext' to the specificed value.
This will allow us run an experiment internally.
ghstack-source-id: 00e161b988c8f8a1cf96efff8095f050cb534cc1
Pull Request resolved: https://github.com/facebook/react/pull/30612
*This is only for internal profiling, not intended to ship.*
This pass is intended to be used with https://github.com/facebook/react/pull/30407.
This pass synthesizes selector functions by collecting immediately
destructured context acesses. We bailout for other types of context
access.
This pass lowers context access to use a selector function by passing
the synthesized selector function as the second argument.
ghstack-source-id: 92d0f6ff2f
Pull Request resolved: https://github.com/facebook/react/pull/30548
This PR updates to use SSA form through the entire compilation pipeline. This means that in both HIR form and ReactiveFunction form, `Identifier` instances map 1:1 to `IdentifierId` values. If two identifiers have the same IdentifierId, they are the same instance. What this means is that all our passes can use this more precise information to determine if two particular identifiers are not just the same variable, but the same SSA "version" of that variable.
However, some parts of our analysis really care about program variables as opposed to SSA versions, and were relying on LeaveSSA to reset identifiers such that all Identifier instances for a particular program variable would have the same IdentifierId (though not necessarily the same Identifier instance). With LeaveSSA removed, those analysis passes can now use DeclarationId instead to uniquely identify a program variable.
Note that this PR surfaces some opportunties to improve edge-cases around reassigned values being declared/reassigned/depended-upon across multiple scopes. Several passes could/should use IdentifierId to more precisely identify exactly which values are accessed - for example, a scope that reassigns `x` but doesn't use `x` prior to reassignment doesn't have to take a dependency on `x`. But today we take a dependnecy.
My approach for these cases was to add a "TODO LeaveSSA" comment with notes and the name of the fixture demonstrating the difference, but to intentionally preserve the existing behavior (generally, switching to use DeclarationId when IdentifierId would have been more precise).
Beyond updating passes to use DeclarationId instead of Identifier/IdentifierId, the other change here is to extract out the remaining necessary bits of LeaveSSA into a new pass that rewrites InstructionKind (const/let/reassign/etc) based on whether a value is actually const or has reassignments and should be let.
ghstack-source-id: 69afdaee5fadf3fdc98ce97549da805f288218b4
Pull Request resolved: https://github.com/facebook/react/pull/30573
Adds `Identifier.declarationId` and the new `DeclarationId` (simulated) opaque type. DeclarationId allows uniquely identifying a variable in the original source, ie regardless of reassignments. This allows us to stay in SSA form throughout compilation (see next diff) while still being able to distinguish SSA versions (via IdentifierId) and non-SSA versions (DeclarationId).
ghstack-source-id: f2547a58aa7b30cea29fcfe23d5cb45583858a4e
Pull Request resolved: https://github.com/facebook/react/pull/30569
Update createTemporaryPlace to use makeTemporary and also rename
makeTemporary to makeTemporaryIdentifier to make it less ambiguous.
ghstack-source-id: b5955d3d667064f2ccf7e633ab63df2269dc56fa
Pull Request resolved: https://github.com/facebook/react/pull/30585
Summary:
Fixes issue documented by #30435. We change the pipeline order so that outlining comes after tracking macro operands, and any function that is referenced in a macro will now not be outlined.
ghstack-source-id: f731ad65c8b84db3fc5f3a2ff3a6986112765963
Pull Request resolved: https://github.com/facebook/react/pull/30587
*This is only for internal profiling, not intended to ship.*
ghstack-source-id: e48998b7be4272199c8a6ff9cc2ec0975add5030
Pull Request resolved: https://github.com/facebook/react/pull/30547
In the future, we can use this to identify useContext calls.
ghstack-source-id: 01d7b0941ccd09f65346eb5431aa53fe361ce5ed
Pull Request resolved: https://github.com/facebook/react/pull/30546
This is a useful utility function similar to the existing
`makeInstructionId` and `makeIdentifierId` functions.
This PR moves it outside the HIRBuilder so we can use this in passes
that don't have access to the builder instance.
ghstack-source-id: 1ac0839e6cb417aedcdf8cdd159af7069af7172a
Pull Request resolved: https://github.com/facebook/react/pull/30545
Rather than storing the entire babel node, store only the required
information which is the node type.
This will be useful for when we synthesize new functions that don't have
a corresponding babel node.
ghstack-source-id: 9098cbdbc4b1e9a6e7dafa2e7645f6f4854e1eac
Pull Request resolved: https://github.com/facebook/react/pull/30544
ghstack failed to land #30552 properly, resubmitting
Developers sometimes use `useMemo()` as a way to conditionally execute code, including conditionally calling setState. However, the compiler may remove existing useMemo calls if they are not necessary, which _should_ always be a safe optimization. If the useMemo has side effects (eg sets state), then this isn't safe.
This PR improves ValidateNoSetStateInRender to disallow any setState in useMemo (even if it's conditional), expanding on the previous check for unconditional setState in render. Note that the approach uses the StartMemoize/FinishMemoize instructions added in DropManualMemo to know whether a particular setState call is within a useMemo or not. This means enabling the validation in DropManualMemo when the setState validation is enabled, but that's fine since that validation is on everywhere by default (_except_ for in fixtures, which we have a todo for)
ghstack-source-id: 65bb3289c3
Pull Request resolved: https://github.com/facebook/react/pull/30583
Summary:
This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns.
This causes a number of new bailouts in test cases, all of which appear to me to be legit.
ghstack-source-id: 770bf02d079ea2480be243a49caa6f69573d8092
Pull Request resolved: https://github.com/facebook/react/pull/30540
While debugging #30536 I happened to notice that the bug only reproduced
when there was interleaving scopes, and observed that an unpruned scope
nested inside of a pruned one was not being visited by
CollectPromotableTemporaries, which keeps track of which identifiers
should be promoted later. Therefore when actually promoting temporaries
we were skipping over the identifiers in children of pruned scopes
ghstack-source-id: d805f62f22
Pull Request resolved: https://github.com/facebook/react/pull/30537
The invalid GraphQL in these fixtures somehow causes an unhandled
promise rejection error when running `yarn prettier-all`. This fixes
that issue by making the GraphQL valid.
To handle more cases, always append the synthetic outlined function as a
new child of the module rather than make assumptions about the original
function. This should handle whatever case where the original function
expression may be a child of a variety of parents
ghstack-source-id: 8581edb8be
Pull Request resolved: https://github.com/facebook/react/pull/30466
Addresses follow up feedback from #30446. Since the outlined function is
guaranteed to have a module-scoped unique identifier name, we can
simplify the insertion logic for the outlined function to always emit a
function declaration rather than switch based on the original function
type. This is fine because the outlined function is synthetic anyway.
ghstack-source-id: 0a4d1f7b0a
Pull Request resolved: https://github.com/facebook/react/pull/30464
If a function expression that mutates a global is passed as a prop,
we don't throw an error as we assume it's not called in render.
But if this function expression is captured in an object and passed down
as prop, we throw an error.
ghstack-source-id: 74cacee09f565550007b2e01fa8877ad64ccfbe9
Pull Request resolved: https://github.com/facebook/react/pull/30456
Previously we would insert new (Arrow)FunctionExpressions as a sibling
of the original function. However this would break in the outlining case
as it would cause the original function expression's parent to become a
SequenceExpression, breaking a bunch of assumptions in the babel plugin.
To get around this, we synthesize a new VariableDeclaration to contain
the newly inserted function expression and therefore insert it as a true
sibling to the original function.
Yeah, it's kinda gross
ghstack-source-id: df13e3b439962b95af4bbd82ef4302624668faf7
Pull Request resolved: https://github.com/facebook/react/pull/30446
I discovered this compiler crash while trying to do an internal sync of
the compiler. Any kind of outlining appears to crash the babel plugin
when the component is a function expression.
ghstack-source-id: 4f717674af91d4d4b730e64cbd7a144b9faab13e
Pull Request resolved: https://github.com/facebook/react/pull/30443
Addresses discussion at https://github.com/facebook/react/pull/30399#discussion_r1684693021. Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter.
The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error.
For declarations (FinishMemo) the existing logic applies unchanged.
ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997
Pull Request resolved: https://github.com/facebook/react/pull/30428
Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this.
This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids.
ghstack-source-id: 2a99df02ac9d125b202cae369e2dc4dccefb0625
Pull Request resolved: https://github.com/facebook/react/pull/30399
Once we create scopes, we should prefer to use the block structure to identify active scope ranges rather than the scope range. They _should_ always be in sync, but ultimately the block structure determine the active range (ie the id of the 'scope' terminal and the terminal's fallthrough block).
ghstack-source-id: 730b6d1cfaf0eb689d71057c78a48045ac4fb11c
Pull Request resolved: https://github.com/facebook/react/pull/30398
Doing some debugging I noticed that a few of the newer terminals kinds weren't printing the instruction id.
ghstack-source-id: e0e4c96aeefdfe09d3be1527fd7103b4e506eb8e
Pull Request resolved: https://github.com/facebook/react/pull/30397
babel-plugin-idx enforces that its 2nd argument is an arrow function, so
outlining needs to skip over lambdas that are args to idx. This PR adds
a small repro highlighting the issue.
ghstack-source-id: b4627ec552056f33090e2f7bc0536a6006d79d18
Pull Request resolved: https://github.com/facebook/react/pull/30435
This will allow us to parse new flow syntax since the `flow` parser is
no longer updated.
I had to exclude some files and have them fall back to `flow` parser
since they contain invalid graphql syntax that makes the plugin crash.
Updates the prettier config to format all `.ts` and `.tsx` files in the
repo using the existing defaults and removing overrides.
The first commit in this PR contains the config changes, the second is
just the result of running `yarn prettier-all`.
Addresses a follow-up from the previous PR. Destructured function params are currently not eagerly promoted to temporaries: we wait until PromotedUsedTemporaries. But params _always_ have to be named, so we can promote when constructing HIR.
ghstack-source-id: a6f665762ebcb7b06b118fcaf7515b8021645eae
Pull Request resolved: https://github.com/facebook/react/pull/30332
Implements general-purpose function outlining. Specifically, anonymous function expressions which have no dependencies/context variables are extracted into named top-level functions. The original function expression is replaced with a `LoadGlobal` of the generated name.
Note that the architecture is designed to allow very general purpose forms of outlining, though we currently are very conservative in what we outline. Specifically, the outlining allows annotating functions with an optional ReactiveFunctionType, which if set will cause the outlined function to get compiled as that type. So we could for example outline a helper hook or helper component, set the type, and then have the hook/component get memoized as well. For now though we just outline with no type set, and generate the function as-is without running it through compilation.
ghstack-source-id: 2a7da6c8e85c3f8becb22d3869d9b6200f7db126
Pull Request resolved: https://github.com/facebook/react/pull/30331
Refactors Program.ts to first traverse the `Program` node and build up a queue of functions to visit, then iterate that queue and compile the functions. This doesn't change behavior, but allows the next diff to add additional items to the queue during compilation (for function outlining).
ghstack-source-id: 858527c30ccc26b3aa6fe75a4746fce0820b316f
Pull Request resolved: https://github.com/facebook/react/pull/30330
Fixture demonstrating a case where we can "outline" a function expression.
ghstack-source-id: 836471518f0ff14d16f7b7bbf2e8900660896e97
Pull Request resolved: https://github.com/facebook/react/pull/30329
Stores the Babel `Scope` object for the current function on the Environment, allowing access later for generating new globally unique names. The idea is to expose a small subset of the capabilities of the Scope API via Environment, so that the rest of the compiler remains decoupled from Babel. Ideally we'd use our own Scope implementation too, but we can punt on that for now since the parts we're using (global id generation) seem pretty reliable.
ghstack-source-id: 37f7113b11fe980688dae423883cf6b8890e77be
Pull Request resolved: https://github.com/facebook/react/pull/30328
I'm experimenting with a new pass that sometimes creates scopes with early returns earlier in the pipeline, but there are a few passes that assume that can't happen. This PR is updating those passes just to be more resilient to help unblock experimentation.
ghstack-source-id: a9e348181ddad1a1e936ef023b5d5ee44aaf3d8c
Pull Request resolved: https://github.com/facebook/react/pull/30333
We have an experimental mode where we generate scopes for simple phi values, even if they aren't subsequently mutated. This mode was incorrectly generating scope ranges, leaving the start at 0 which is invalid. The fix is to allow non-zero identifier ranges to overwrite the scope start (rather than taking the min) if the scope start is still zero.
ghstack-source-id: ecbb04c96ed4de62f781e48cda46309c42aa07e0
Pull Request resolved: https://github.com/facebook/react/pull/30321
Summary: In change-detection mode, we previously were spreading the contents of the computation block into the result twice. Other babel passes that cause in-place mutations of the AST would then be causing action at a distance and breaking the overall transform result. This pr creates clones of the nodes instead, so that mutations aren't reflected in both places where the block is used.
ghstack-source-id: b78def8d8d1b8f9978df0a231f64fdeda786a3a3
Pull Request resolved: https://github.com/facebook/react/pull/30148
Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.
Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.
ghstack-source-id: c2b22e3d62a1ab490a6a2150e28b934b9dc8676b
Pull Request resolved: https://github.com/facebook/react/pull/30107
Currently, the `hookKind` for `useInsertionEffect` is set to
`useLayoutEffect`. This pull request fixes it by adding a new `hookKind`
for `useInsertionEffect`.
Double checked by syncing internally and verifying the # of `visitInstruction` calls with unique `InstructionId`s.
This is a bit of an awkward pattern though. A cleaner alternative might be to override `visitValue` and store its results in a sidemap (instead of returning)
ghstack-source-id: f6797d7652
Pull Request resolved: https://github.com/facebook/react/pull/30077
Adds Array.prototype methods that return primitives or other arrays -- naive type inference can be really helpful in reducing mutable ranges -> achieving higher quality memoization.
Also copies Array.prototype methods to our mixed read-only JSON-like object shape.
(Inspired after going through some suboptimal internal compilation outputs.)
ghstack-source-id: 0bfad11180
Pull Request resolved: https://github.com/facebook/react/pull/30075
Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g.
```js
// source
a();
if (...) {
b();
}
c();
// HIR
bb0:
a()
if test=... consequent=bb1 fallthrough=bb2
bb1:
b()
goto bb2
bb2:
c()
// AlignReactiveScopesToBlockScopesHIR nodes
Root node (maps to both bb0 and bb2)
|- bb1
|- ...
```
There are two issues with the existing implementation:
1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range.
```
\# This case gets handled correctly
┌──────────────┐
│ │
block start block end
scope start scope end
│ │
└───────────────┘
\# But not this one!
┌──────────────┐
│ │
block start block end
scope start scope end
│ │
└───────────────┘
```
2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details.
ghstack-source-id: 327dec5019
Pull Request resolved: https://github.com/facebook/react/pull/29891
ghstack-source-id: 04b1526c85
Pull Request resolved: https://github.com/facebook/react/pull/29878
The AlignReactiveScope bug should be simplest to fix, but it's also caught by an invariant assertion. I think a fix could be either keeping track of "active" block-fallthrough pairs (`retainWhere(pair => pair.range.end > current.instr[0].id)`) or following the approach in `assertValidBlockNesting`.
I'm tempted to pull the value-block aligning logic out into its own pass (using the current `node` tree traversal), then align to non-value blocks with the `assertValidBlockNesting` approach. Happy to hear feedback on this though!
The other two are likely bigger issues, as they're not caught by static invariants.
Update:
- removed bug-phi-reference-effect as it's been patched by @josephsavona
- added bug-array-concat-should-capture
When converting value blocks from HIR to ReactiveFunction, we have to drop StoreLocal assignments that represent the assignment of the phi, since ReactiveFunction supports compound expressions. These StoreLocals are only present to represent the conditional assignment of the value itself - but it's also possible for the expression to have contained an assignment expression. Before, in trying to strip the first category of StoreLocal we also accidentally stripped the second category. Now we check that the assignment is for a temporary, and don't strip otherwise.
ghstack-source-id: e7759c963bbc1bbff2d3230534b049199e3262ad
Pull Request resolved: https://github.com/facebook/react/pull/30067
Adds a fixture based on internal case where our current output is quite a bit more verbose than the original memoization. See the comment in the fixture for more about the heuristic we can apply.
ghstack-source-id: e637a38140
Pull Request resolved: https://github.com/facebook/react/pull/29998
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain.
This PR includes two changes:
* First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once
* Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks.
In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization.
This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this.
So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes.
I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures.
ghstack-source-id: ad3e516fa4
Pull Request resolved: https://github.com/facebook/react/pull/29882
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.
This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.
Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.
Partially addresses #29648.
ghstack-source-id: ecc61c9f0bece90d18623b3c570fea05fbcd811a
Pull Request resolved: https://github.com/facebook/react/pull/29997
If a component uses the `useRef` hook directly then we type it's return
value as a ref. But if it's wrapped in a custom hook then we lose out on
this type information as the compiler doesn't look at the hook
definition. This has resulted in some false positives in our analysis
like the ones reported in #29160 and #29196.
This PR will treat objects named as `ref` or if their names end with the
substring `Ref`, and contain a property named `current`, as React refs.
```
const ref = useMyRef();
const myRef = useMyRef2();
useEffect(() => {
ref.current = ...;
myRef.current = ...;
})
```
In the above example, `ref` and `myRef` will be treated as React refs.
Updated version of #29758 removing `useFormState` since that was the
previous name for `useActionState`.
---------
Co-authored-by: Hieu Do <hieudn.uh@gmail.com>
Adds fixtures for `macro.namespace(...)` style invocations which we use internally in some cases instead of just `macro(...)`. I tried every example i could think of that could possibly break it (including basing one off of another fixture where we hit an invariant related due to a temporary being emitted for a method call), and they all worked. I just had to fix an existing bug where we early return in some cases instead of continuing, which is a holdover from when this pass was originally written as a ReactiveFunction visitor.
ghstack-source-id: c01f45b3ef6f42b6d1f1ff0508aea258000e0fce
Pull Request resolved: https://github.com/facebook/react/pull/29899
Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures.
The algorithm itself is described in the docblock.
note: This is a cleaned up version of #29579 that is ready for review.
ghstack-source-id: c54a806cad7aefba4ac1876c9fd9b25f9177e95a
Pull Request resolved: https://github.com/facebook/react/pull/29863
Updates our scope merging pass to allow more types of instructions to intervene btw scopes. This includes all the non-allocating kinds of nodes that are considered reorderable in #29863. It's already safe to merge scopes with these instructions — we only merge if the lvalue is not used past the next scope. Additionally, without changing this pass reordering isn't very effective, since we would reorder to add these types of intervening instructions and then not be able to merge scopes.
Sequencing this first helps to see the win just from reordering alone.
ghstack-source-id: 79263576d8eaeb45ef4d1ec4951478459853a287
Pull Request resolved: https://github.com/facebook/react/pull/29881
Summary: The change detection mode was unavailable in the playground because the pragma was not a boolean. This fixes that by special casing it in pragma parsing, similar to validateNoCapitalizedCalls
ghstack-source-id: 4a8c17d21ab8b7936ca61c9dd1f7fdf8322614c9
Pull Request resolved: https://github.com/facebook/react/pull/29889
Our passes aren't sequenced such that we could observe this bug, but this retains the proper terminal kind for pruned-scopes in mapTerminalSuccessors.
ghstack-source-id: 1a03b40e45649bbef7d6db968fb2dbd6261a246a
Pull Request resolved: https://github.com/facebook/react/pull/29884
Summary: Minor change inspired by #29863: the BuildHIR pass ensures that Binary and UnaryOperator nodes only use a limited set of the operators that babel's operator types represent, which that pr relies on for safe reorderability, but the type of those HIR nodes admits the other operators. For example, even though you can't build an HIR UnaryOperator with `delete` as the operator, it is a valid HIR node--and if we made a mistaken change that let you build such a node, it would be unsafe to reorder.
This pr makes the typing of operators stricter to prevent that.
ghstack-source-id: 9bf3b1a37eae3f14c0e9fb42bb3ece522b317d98
Pull Request resolved: https://github.com/facebook/react/pull/29880
Fixes a bug found by mofeiZ in #29878. When we merge queued states, if the new state does not introduce changes relative to the queued state we should use the queued state, not the new state.
ghstack-source-id: c59f69de15
Pull Request resolved: https://github.com/facebook/react/pull/29879
Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.
ghstack-source-id: e2c23348265b7ef651232b962ed7be7f6fed1930
Pull Request resolved: https://github.com/facebook/react/pull/29866
Summary: We can tighten our criteria for what is a component by requiring that a component or hook contain JSX or hook calls directly within its body, excluding nested functions . Currently, if we see them within the body anywhere -- including nested functions -- we treat it as a component if the other requirements are met. This change makes this stricter.
We also now expect components (but not necessarily hooks) to have return statements, and those returns must be potential React nodes (we can reject functions that return function or object literals, for example).
ghstack-source-id: 4507cc3955216c564bf257c0b81bfb551ae6ae55
Pull Request resolved: https://github.com/facebook/react/pull/29865
Summary: Projects which have heavily adopted Flow component syntax may wish to enable the compiler only for components and hooks that use the syntax, rather than trying to guess which functions are components and hooks. This provides that option.
ghstack-source-id: 579ac9f0fa01d8cdb6a0b8f9923906a0b37662f3
Pull Request resolved: https://github.com/facebook/react/pull/29864
Per title, implements an HIR-based version of FlattenScopesWithHooksOrUse as part of our push to use HIR everywhere. This is the last pass to migrate before PropagateScopeDeps, which is blocking the fix for `bug.invalid-hoisting-functionexpr`, ie where we can infer incorrect dependencies for function expressions if the dependencies are accessed conditionally.
ghstack-source-id: 05c6e26b3b7a3b1c3e106a37053f88ac3c72caf5
Pull Request resolved: https://github.com/facebook/react/pull/29840
Pre the title, this implements an HIR-based version of FlattenReactiveLoops. Another step on the way to HIR-everywhere.
ghstack-source-id: e1d166352df6b0725e4c4915a19445437916251f
Pull Request resolved: https://github.com/facebook/react/pull/29838
Adds the HIR equivalent of a pruned-scope, allowing us to start porting the scope-pruning passes to operate on HIR.
ghstack-source-id: dbbdc43219123467acc1a531d8276e8b9cc91e14
Pull Request resolved: https://github.com/facebook/react/pull/29837
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.
Before submitting a pull request, please make sure the following is
done:
1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
10. If you haven't already, complete the CLA.
Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->
## Summary
<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
The parsePluginOptions seemed to be duplicated within
[BabelPlugin.ts](f5af92d2c4/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts (L32))
and
[Program.ts](f5af92d2c4/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts (L220)).
Since the options already parsed in BabelPlugin.ts should have been
passed to compileProgram, in this PR we deleted parsePluginOptions in
compileProgram and used the options passed as arguments as they are.
I've done that.
## How did you test this change?
<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
If you leave this empty, your PR will very likely be closed.
-->
<img width="516" alt="image"
src="https://github.com/facebook/react/assets/87469023/2a70c6ea-0330-42a2-adff-48ae3e905790">
Adds a shape type for component props, which has one defined property: "ref". This means that if the ref property exists, we can type usage of `props.ref` (or via destructuring) the same as the result of `useRef()` and infer downstream usage similarly.
ghstack-source-id: 76cd07c5dfeea2a4aafe141912663b097308fd73
Pull Request resolved: https://github.com/facebook/react/pull/29834
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope:
* Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them.
* Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case.
I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement.
ghstack-source-id: 80610ddafad65eb837d0037e2692dd74bc548088
Pull Request resolved: https://github.com/facebook/react/pull/29820
Adds additional information to the CompileSuccess LoggerEvent:
* `prunedMemoBlocks` is the number of reactive scopes that were pruned for some reason.
* `prunedMemoValues` is the number of unique _values_ produced by those scopes.
Both numbers exclude blocks that are just a hook call - ie although we create and prune a scope for eg `useState()`, that's just an artifact of the sequencing of our pipeline. So what this metric is counting is cases of _other_ values that go unmemoized. See the new fixture, which takes advantage of improvements in the snap runner to optionally emit the logger events in the .expect.md file if you include the "logger" pragma in a fixture.
ghstack-source-id: c2015bb5565746d07427587526b71e23685279c2
Pull Request resolved: https://github.com/facebook/react/pull/29810
Mostly addresses the issue with non-reactive pruned scopes. Before, values from pruned scopes would not be memoized, but could still be depended upon by downstream scopes. However, those downstream scopes would assume the value could never change. This could allow the developer to observe two different versions of a value - the freshly created one (if observed outside a scope) or a cached one (if observed inside, or through) a scope which used the value but didn't depend on it.
The fix here is to consider the outputs of pruned reactive scopes as reactive. Note that this is a partial fix because of things like control variables — the full solution would be to mark these values as reactive, and then re-run InferReactivePlaces. We can do this once we've fully converted our pipeline to use HIR everywhere. For now, this should fix most issues in practice because PruneNonReactiveDependencies already does basic alias tracking (see new fixture).
ghstack-source-id: 364430bbeca4cfca2fbf9df4d92b2e61b3352311
Pull Request resolved: https://github.com/facebook/react/pull/29790
There's a category of bug currently where pruned reactive scopes whose outputs are non-reactive can have their code end up inlining into another scope, moving the location of the instruction. Any value that had a scope assigned has to have its order of evaluation preserved, despite the fact that it got pruned, so naively we could just force every pruned scope to have its declarations promoted to named variables.
However, that ends up assigning names to _tons_ of scope declarations that don't really need to be promoted. For example, a scope with just a hook call ends up with:
```
const x = useFoo();
=>
scope {
$t0 = Call read useFoo$ (...);
}
$t1 = StoreLocal 'x' = read $t0;
```
Where t0 doesn't need to be promoted since it's used immediately to assign to another value which is a non-temporary.
So the idea of this PR is that we can track outputs of pruned scopes which are directly referenced from inside a later scope. This fixes one of the two cases of the above pattern. We'll also likely have to consider values from pruned scopes as always reactive, i'll do that in the next PR.
ghstack-source-id: b37fb9a7cb1430b7c35ec5946269ce5a886a486a
Pull Request resolved: https://github.com/facebook/react/pull/29789
There are a few places where we want to check whether a value actually got memoized, and we currently have to infer this based on values that "should" have a scope and whether a corresponding scope actually exists. This PR adds a new ReactiveStatement variant to model a reactive scope block that was pruned for some reason, and updates all the passes that prune scopes to instead produce this new variant.
ghstack-source-id: aea6dab469acb1f20058b85cb6f9aafab5d167cd
Pull Request resolved: https://github.com/facebook/react/pull/29781
## Summary
See #29737
## How did you test this change?
As the feature requires module support and the test runner does
currently not support running tests as modules, I could only test it via
playground.
This PR makes it so we always emit a const VariableDeclaration for
compiled functions in gating mode. If the original declaration's parent
was an ExportDefaultDeclaration we'll also append a new
ExportDefaultDeclaration pointing to the new identifier. This allows
code that adds optional properties to the function declaration to still
work in gating mode
ghstack-source-id: 5705479135baa268eeb3c85bfbf1883964e84916
Pull Request resolved: https://github.com/facebook/react/pull/29806
When gating is enabled, any function declaration properties that were
previously set (typically `Function.displayName`) would cause a crash
after compilation as the original identifier is no longer present.
ghstack-source-id: beb7e258561ea598d306fa67706d34a8788d9322
Pull Request resolved: https://github.com/facebook/react/pull/29802
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives.
ghstack-source-id: 1a30609f5f
Pull Request resolved: https://github.com/facebook/react/pull/29733
We don't always have the NODE_ENV set, so additionally check for the
__DEV__ global if it has one set.
ghstack-source-id: 3719a4710a5fb1b4abf511f469c815917b7dfdf4
Pull Request resolved: https://github.com/facebook/react/pull/29785
Summary
The dispatch function from useReducer is stable, so it is also non-reactive.
the related PR: #29665
the related comment: #29674 (comment)
I am not sure if the location of the new test file is appropriate😅.
How did you test this change?
Added the specific test compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useReducer-returned-dispatcher-is-non-reactive.expect.md.
Summary: Using the change detection code to debug codebases that violate the rules of react is a lot easier when we have a source location corresponding to the value that has changed inappropriately. I didn't see an easy way to track that information in the existing data structures at the point of codegen, so this PR adds locations to identifiers and reactive scopes (the location of a reactive scope is the range of the locations of its included identifiers).
I'm interested if there's a better way to do this that I missed!
ghstack-source-id: aed5f7edda
Pull Request resolved: https://github.com/facebook/react/pull/29658
Summary: This PR expands the analysis from the previous in the stack in order to also capture when a value can incorrectly change within a single render, rather than just changing between two renders. In the case where dependencies have changed and so a new value is being computed, we now compute the value twice and compare the results. This would, for example, catch when we call Math.random() in render.
The generated code is a little convoluted, because we don't want to have to traverse the generated code and substitute variable names with new ones. Instead, we save the initial value to the cache as normal, then run the computation block again and compare the resulting values to the cached ones. Then, to make sure that the cached values are identical to the computed ones, we reassign the cached values into the output variables.
ghstack-source-id: d0f11a4cb2
Pull Request resolved: https://github.com/facebook/react/pull/29657
Summary: jmbrown215 recently had an observation that the arguments to useState/useRef are only used when a component renders for the first time, and never afterwards. We can skip more computation that we previously could, with reactive blocks that previously recomputed values when inputs changed now only ever computing them on the first render.
ghstack-source-id: 5d044ef787
Pull Request resolved: https://github.com/facebook/react/pull/29653
Summary: The essential assumption of the compiler is that if the inputs to a computation have not changed, then the output should not change either--computation that the compiler optimizes is idempotent.
This is, of course, known to be false in practice, because this property rests on requirements (the Rules of React) that are loosely enforced at best. When rolling out the compiler to a codebase that might have rules of react violations, how should developers debug any issues that arise?
This diff attempts one approach to that: when the option is set, rather than simply skipping computation when dependencies haven't changed, we will *still perform the computation*, but will then use a runtime function to compare the original value and the resultant value. The runtime function can be customized, but the idea is that it will perform a structural equality check on the values, and if the values aren't structurally equal, we can report an error, including information about what file and what variable was to blame.
This assists in debugging by narrowing down what specific computation is responsible for a difference in behavior between the uncompiled code and the program after compilation.
ghstack-source-id: 50dad3dacf
Pull Request resolved: https://github.com/facebook/react/pull/29656
Summary: This adds a debugging mode to the compiler that simply adds a `|| true` to the guard on all memoization blocks, which results in the generated code never using memoized values and always recomputing them. This is designed as a validation tool for the compiler's correctness--every program *should* behave exactly the same with this option enabled as it would with it disabled, and so any difference in behavior should be investigated as either a compiler bug or a pipeline issue.
(We add `|| true` rather than dropping the conditional block entirely because we still want to exercise the guard tests, in case the guards themselves are the source of an error, like reading a property from undefined in a guard.)
ghstack-source-id: 955a47ec16
Pull Request resolved: https://github.com/facebook/react/pull/29655
Summary: This adds a compiler option to not drop existing manual memoization and leaving useMemo/useCallback in the generated source. Why do we need this, given that we also have options to validate or ensure that existing memoization is preserved? It's because later diffs on this stack are designed to alter the behavior of the memoization that the compiler emits, in order to detect rules of react violations and debug issues. We don't want to change the behavior of user-level memoization, however, since doing so would be altering the semantics of the user's program in an unacceptable way.
ghstack-source-id: 89dccdec9ccb4306b16e849e9fa2170bb5dd021f
Pull Request resolved: https://github.com/facebook/react/pull/29654
## Summary
Resolves#29622
## How did you test this change?
I verified the implementation using the test.
Note:
This PR was done without waiting for approval in #29622, so feel free to
just close it.
We were missing a check that ObjectMethods are not getters or setters. In our experience this is pretty rare within React components and hooks themselves, so let's start with a todo.
Closes#29586
ghstack-source-id: 03c6cce9a9368a4a4f4ba98bcdff3fa4729ceaf9
Pull Request resolved: https://github.com/facebook/react/pull/29592
Fixes https://x.com/raibima/status/1794395807216738792
The issue is that if you pass a global-modifying function as prop to JSX, we currently report that it's invalid to modify a global during rendering. The problem is that we don't really know when/if the child component will actually call that function prop. It would be against the rules to call the function during render, but it's totally fine to call it during an event handler or from a useEffect.
Since we don't know at the call-site how the child will use the function, we should allow such calls. In the future we could improve this in a few ways:
* For all functions that modify globals, codegen an assertion or warning into the function that fires if it's called "during render". We'd have to precisely define what "during render" is, but this would at least help developers catch this dynamically.
* Use the type system to distinguish "event/effect" and "render" functions to help developers avoid accidentally mutating globals during render.
ghstack-source-id: 4aba4e6d214fd6c062e4029294efe9b8fe25cd83
Pull Request resolved: https://github.com/facebook/react/pull/29591
user's pipeline
When the user app has a babel.config file that is missing the compiler,
strange things happen as babel does some strange merging of options from
the user's config and in various callsites like in our eslint rule and
healthcheck script. To minimize odd behavior, we default to not reading
the user's babel.config
Fixes#29135
ghstack-source-id: d6fdc43c5c
Pull Request resolved: https://github.com/facebook/react/pull/29211
After this is merged, I'll add it to .git-blame-ignore-revs. I can't do
it now as the hash will change after ghstack lands this stack.
ghstack-source-id: 054ca869b7
Pull Request resolved: https://github.com/facebook/react/pull/29214
When I added new builtin types for jsx and functions, i forget to add a shape definition. This meant that attempting to accesss a property or method on these types would cause an internal error with an unresolved shape. That wasn't obvious because we rarely call methods on these types.
I confirmed that the new fixtures here fail without the fix.
ghstack-source-id: aa8f8d75a3
Pull Request resolved: https://github.com/facebook/react/pull/29624
We currently don't report an error if the code attempts to reassign a const. Our thinking has been that we're not trying to catch all possible mistakes you could make in JavaScript — that's what ESLint, TypeScript, and Flow are for — and that we want to focus on React errors. However, accidentally reassigning a const is easy to catch and doesn't get in the way of other analysis so let's implement it.
Note that React Compiler's ESLint plugin won't report these errors by default, but they will show up in playground.
Fixes#29598
ghstack-source-id: a0af8b9a486d74a8991413322efddc3e3028c755
Pull Request resolved: https://github.com/facebook/react/pull/29619
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc.
I tried to write a proper fixture test to test that we react to changes to a custom setState setter function, but I think there may be an issue with snap and how it handles re-renders from effects. I think the tests are good here but open to feedback if we want to go down the rabbit hole of figuring out a proper snap test for this.
ghstack-source-id: 5e9a8f6e0d23659c72a9d041e8d394b83d6e526d
Pull Request resolved: https://github.com/facebook/react/pull/29190
No-op refactor to make Environment#getGlobalDeclaration() take a NonLocalBinding instead of just a name. The idea is that in subsequent PRs we can use information about the binding to resolve a type more accurately. For example, we can resolve `Array` differently if its an import or local and not the global Array. Similar for resolving local `useState` differently than the one from React.
ghstack-source-id: c8063e6fb8acdd347a56477d6b06238dd54979b1
Pull Request resolved: https://github.com/facebook/react/pull/29189
We currently use `LoadGlobal` and `StoreGlobal` to represent any read (or write) of a variable defined outside the component or hook that is being compiled. This is mostly fine, but for a lot of things we want to do going forward (resolving types across modules, for example) it helps to understand the actual source of a variable.
This PR is an incremental step in that direction. We continue to use LoadGlobal/StoreGlobal, but LoadGlobal now has a `binding:NonLocalBinding` instead of just the name of the global. The NonLocalBinding type tells us whether it was an import (and which kind, the source module name etc), a module-local binding, or a true global. By keeping the LoadGlobal/StoreGlobal instructions, most code that deals with "anything not declared locally" doesn't have to care about the difference. However, code that _does_ want to know the source of the value can figure it out.
ghstack-source-id: e701d4ebc0fb5681a0197198ac2c2a03b3e8aae9
Pull Request resolved: https://github.com/facebook/react/pull/29188
Repro of a case where we should ideally merge consecutive scopes, but where intermediate temporaries prevent the scopes from merging.
We'd need to reorder instructions in order to merge these.
ghstack-source-id: 4f05672604eeb547fc6c26ef99db6572843ac646
Pull Request resolved: https://github.com/facebook/react/pull/29197
In MergeReactiveScopesThatInvalidateTogether when deciding which scopes were eligible for mergin at all, we looked specifically at the instructions whose lvalue produces the declaration. So if a scope declaration was `t0`, we'd love for the instruction where `t0` was the lvalue and look at the instruction type to decide if it is eligible for merging.
Here, we use the inferred type instead (now that the inferred types support the same set of types of instructions we looked at before). This allows us to find more cases where scopes can be merged.
ghstack-source-id: 0e3e05f24ea0ac6e3c43046bc3e114f906747a04
Pull Request resolved: https://github.com/facebook/react/pull/29157
Improves merging of consecutive scopes so that we now merge two scopes if the dependencies of the second scope are a subset of the previous scope's output *and* that dependency has a type that will always produce a new value (array, object, jsx, function) if it is re-evaluated.
To make this easier, we extend the set of builtin types to include ones for function expressions and JSX and to infer these types in InferTypes. This allows using the already inferred types in MergeReactiveScopesThatInvalidateTogether.
ghstack-source-id: e9119fc4e02b3665848113d71fdff0c5bac3348a
Pull Request resolved: https://github.com/facebook/react/pull/29156
React Compiler attempts to merge consecutive reactive scopes in order to reduce overhead. The basic idea is that if two consecutive scopes would always invalidate together then we should merge them. It gets more complicated, though, because values produced by the earlier scope may not always invalidate when their inputs do. For example, a scope that produces `fn(x)` may not invalidate on all changes to `x` if the function is `Math.max(x, 10)` (changing x from 8 to 9 won't change the output).
Previously we were conservative and only merged if either:
* the two scopes had the same dependencies
* the second scope's deps exactly matched the previous scope's outputs.
You can see this in the new fixture, where the second `<button>` gets its own scope, which happens because the preceding scope has an extra output that isn't a dep of the `<button>`'s scope.
ghstack-source-id: d869c8d4df5aa4105bbdae01b5dd7f106145b351
Pull Request resolved: https://github.com/facebook/react/pull/29155
## Summary
We ran React compiler against part of our codebase and collected
compiler errors. One of the more common non-actionable errors is caused
by usage of the `!` TypeScript non-null assertion operation:
```
(BuildHIR::lowerExpression) Handle TSNonNullExpression expressions
```
It seems like React Compiler _should_ be able to support this by just
ignoring the syntax and using the underlying expression. I'm sure a lot
of our non-null assertion usage should not exist and I understand if
React Compiler does not want to support this syntax. It wasn't obvious
to me if this omission was intentional or if there are future plans to
use `TSNonNullExpression` as part of the compiler's analysis. If there
are no future plans it seems like just ignoring it should be fine.
## How did you test this change?
```sh
❯ yarn snap --filter
yarn run v1.17.3
$ yarn workspace babel-plugin-react-compiler run snap --filter
$ node ../snap/dist/main.js --filter
PASS non-null-assertion
1 Tests, 1 Passed, 0 Failed
```
By default, React Compiler will skip compilation if it cannot preserve existing memoization. Ie, if the code has an existing `useMemo()` or `useCallback()` and the compiler cannot determine that it is safe to keep that memoization — or do even better — then we'll leave the code alone. The actual compilation doesn't use any hints from existing memo calls, this is purely to check and avoid regressing any specific memoization that developers may have already applied.
However, we were accidentally reporting some false-positive _validation_ errors due to the StartMemoize and FinishMemoize instructions that we emit to track where the memoization was in the source code. This is now fixed.
Fixes#29131Fixes#29132
ghstack-source-id: 9f6b8dbc5074ccc96e6073cf11c4920b5375faf6
Pull Request resolved: https://github.com/facebook/react/pull/29154
Improves ValidateNoRefAccessInRender (still disabled by default) to properly ignore ref access within effects. This includes allowing ref access within functions that are only transitively called from an effect.
While I was here I also added some extra test fixtures for allowing global mutation in effects.
ghstack-source-id: fb6352a1788b7bdbebb40d5b844b711ef87d6771
Pull Request resolved: https://github.com/facebook/react/pull/29151
Babel doesn't seem to properly preserve escaping of HTML entities when emitting JSX text children, so this commit works around the issue by emitting a JsxExpressionContainer for JSX children that contain ">", "<", or "&" characters.
Closes#29100
ghstack-source-id: 2d0622397cc067c6336f3635073e07daef854084
Pull Request resolved: https://github.com/facebook/react/pull/29143
Workaround for a bug in older versions of Babel, where strings with unicode are incorrectly escaped when emitted as JSX attributes, causing double-escaping by later processing.
Closes#29120Closes#29124
ghstack-source-id: 065440d4fb97e164beb8a8f15f252f372a59c5a0
Pull Request resolved: https://github.com/facebook/react/pull/29141
@jbonta nerd-sniped me into making this optimization during conference
prep, posting this as a PR now that keynote is over.
Consider these two cases:
```javascript
export default function MyApp1({ count }) {
const cb = () => count;
return <div onclick={cb}>Hello World</div>;
}
export default function MyApp2({ count }) {
return <div onclick={() => count}>Hello World</div>;
}
```
Previously, the former would create two reactive scopes (one for `cb`,
one for the div) while the latter would only have a single scope for the
`div` and its inline callback. The reason we created separate scopes
before is that there's a `StoreLocal 'cb' = t0` instruction in-between,
and i had conservatively implemented the merging pass to not allow
intervening StoreLocal instructions.
The observation is that intervening StoreLocals are fine _if_ the
assigned variable's last usage is the next scope. We already have a
check that the intervening lvalues are last-used at/before the next
scope, so it's trivial to extend this to support StoreLocal.
Note that we already don't merge scopes if there are intervening
terminals, so we don't have to worry about things like conditional
StoreLocal, conditional access of the resulting value, etc.
The [`files` field](https://docs.npmjs.com/cli/v10/commands/npm-publish#files-included-in-package)
controls what files get included in the published package.
This PR specifies the `files` field on our publishable packages to only
include the `dist` directory, since we don't need to ship any types or
sourcemaps with 3 of them.
react-compiler-runtime is a runtime package which has sourcemaps, so we
also include the `src` directory in the published package.
Also fixes an invalid version range for the react peer dependency in
react-compiler-runtime, tested that it works via https://semver.npmjs.com/
ghstack-source-id: 12b36c203fc9fd8d72a1995fb3fba2312de4aa51
Pull Request resolved: https://github.com/facebook/react-forget/pull/2965
We found this issue through enabling the compiler on the React Conf app.
`babel-preset-expo` automatically adds the `react-native-animated`
plugin to apps that use the preset. This means that Expo apps sometimes
omit the react-native-animated plugin from their config, which was
failing our existing check. This PR copies the same detection that Expo
does for adding reanimated as a fallback
ghstack-source-id: 46f7aec0bc
Pull Request resolved: https://github.com/facebook/react-forget/pull/2953
Adds supports for hot module reloading (HMR) by resetting the cache if a hash of the source file changes. This is enabled via a compiler flag, but also enabled automatically via the babel plugin when NODE_ENV=development.
ghstack-source-id: 5cd1ad5c89
Pull Request resolved: https://github.com/facebook/react-forget/pull/2951
Make it clearer how to address this error by allowlisting globals that
are known to be safe
ghstack-source-id: e7fa6464ebb561a7a1366ff70430842007c6552e
Pull Request resolved: https://github.com/facebook/react-forget/pull/2909
- Updated all directly defined dependencies to the latest React 19 Beta
- `package.json`: used `resolutions` to force React 19 for `react-is` transitive dependency
- `package.json`: postinstall script to patch fbt for the React 19 element Symbol
- Match on the message in Snap to exclude a React 19 warning that `act` should be imported from `react` instead (from inside `@testing-library/react`)
- Some updated snapshots, I think due to now recovering behavior of `useMemoCache`, please review.
In a next step, we can do the following. I excluded it since it from here as it made the PR unreviewable on GitHub.
- Snapshots now use `react/compiler-runtime` as in prod, so the different default in Snap is no longer needed.
