Followup from #30894 , not sure how these got missed. Note that this PR just copies the fixtures without adding `@enablePropagateDepsInHIR`. #31032 follows and actually enables the HIR-version of propagateScopeDeps to run. I split this out into two PRs to make snapshot differences easier to review, but also happy to merge
Fixtures found from locally setting snap test runner to default to `enablePropagateDepsInHIR: 'enabled_baseline'` and forking fixtures files with different output.
ghstack-source-id: 7d7cf41aa923d83ad49f89079171b0411923ce6b
Pull Request resolved: https://github.com/facebook/react/pull/31030
Currently the playground is setup as a linked workspace for the
compiler which complicates our yarn workspace setup and means that snap
can sometimes pull in a different version of react than was otherwise
specified.
There's no real reason to have these workspaces combined so let's split
them up.
ghstack-source-id: 56ab064b2f
Pull Request resolved: https://github.com/facebook/react/pull/31081
Based on https://github.com/facebook/react/pull/30995 ([rendered
diff](https://github.com/jackpope/react/compare/inline-jsx-2...jackpope:react:inline-jsx-3?expand=1))
____
Some apps still use `react.element` symbols. Not only do we want to test
there but we also want to be able to upgrade those sites to
`react.transitional.element` without blocking on the compiler (we can
change the symbol feature flag and compiler config at the same time).
The compiler runtime uses `react.transitional.element`, so the snap
fixture will fail if we change the default here. However I confirmed
that commenting out the fixture entrypoint and running snap with
`react.element` will update the fixture symbols as expected.
If JSX receives a props spread without additional attributes (besides
`ref` and `key`), we can pass the spread object as a property directly
to avoid the extra object copy.
```
<Test {...propsToSpread} />
// {props: propsToSpread}
<Test {...propsToSpread} a="z" />
// {props: {...propsToSpread, a: "z"}}
```
This adds an `InlineJsxTransform` optimization pass, toggled by the
`enableInlineJsxTransform` flag. When enabled, JSX will be transformed
into React Element object literals, preventing runtime overhead during
element creation.
TODO:
- [ ] Add conditionals to make transform PROD-only
- [ ] Make the React element symbol configurable so this works with
runtimes that support `react.element` or `react.transitional.element`
- [ ] Look into additional optimization to pass props spread through
directly if none of the properties are mutated
Summary:
1. Minor refactor to provide a stable API for calling the compiler from the playground
2. Allows spaces in pass names without breaking the appearance of the playground by replacing spaces with in pass tabs
ghstack-source-id: 12a43ad86c16c0e21f3e6b4086d531cdefd893eb
Pull Request resolved: https://github.com/facebook/react/pull/30988
Compiler bailout diagnostics should now highlight only the first line of
the source location span.
(Resubmission of #30423 which was reverted due to invalid column
number.)
Summary:
Introduces a new binding kind for functions that allows them to be hoisted. Also has the result of causing all nested function declarations to be outputted as function declarations, not as let bindings.
ghstack-source-id: fa40d4909fb3d30c23691e36510ebb3c3cc41053
Pull Request resolved: https://github.com/facebook/react/pull/30922
Summary:
This brings the behavior of ref mutation within hook callbacks into alignment with the behavior of global mutations--that is, we allow all hooks to take callbacks that may mutate a ref. This is potentially unsafe if the hook eagerly calls its callback, but the alternative is excessively limiting (and inconsistent with other enforcement).
This also bans *directly* passing a ref.current value to a hook, which was previously allowed.
ghstack-source-id: e66ce7123e
Pull Request resolved: https://github.com/facebook/react/pull/30917
Summary:
This change expands our handling of refs to build an understanding of nested refs within objects and functions that may return refs. It builds a special-purpose type system within the ref analysis that gives a very lightweight structural type to objects and array expressions (merging the types of all their members), and then propagating those types throughout the analysis (e.g., if `ref` has type `Ref`, then `{ x: ref }` and `[ref]` have type `Structural(value=Ref)` and `{x: ref}.anything` and `[ref][anything]` have type `Ref`).
This allows us to support structures that contain refs, and functions that operate over them, being created and passed around during rendering without at runtime accessing a ref value.
The analysis here uses a fixpoint to allow types to be fully propagated through the system, and we defend against diverging by widening the type of a variable if it could grow infinitely: so, in something like
```
let x = ref;
while (condition) {
x = [x]
}
```
we end up giving `x` the type `Structural(value=Ref)`.
ghstack-source-id: afb0b0cb01
Pull Request resolved: https://github.com/facebook/react/pull/30902
Summary:
This PR performs a major refactor of InferReferenceEffects to separate out the work on marking places with Effects from inferring FunctionEffects. The behavior should be identical after this change (see [internal sync](https://www.internalfb.com/intern/everpaste/?handle=GN74VxscnUaztTYDAL8q0CRWBIxibsIXAAAB)) but the FunctionEffect logic should be easier to work with.
These analyses are unfortunately still deeply linked--the FunctionEffect analysis needs to reason about the "current" value kind for each point in the program, while the InferReferenceEffects algorithm performs global updates on the state of the program (e.g. freezing). In the future, it might be possible to make these entirely separate passes if we store the ValueKind directly on places.
For the most part, the logic of reference effects and function effects can be cleanly separated: for each instruction and terminal, we visit its places and infer their effects, and then we visit its places and infer any function effects that they cause. The biggest wrinkle here is that when a transitive function freeze operation occurs, it has to happen *after* inferring the function effects on the place, because otherwise we may convert a value from Context to Frozen, which will cause the ContextualMutation function effect to be converted to a ReactMutation effect too early. This can be observed in a case like this:
```
export default component C() {
foo(() => {
const p = {};
return () => {
p['a'] = 1
};
});
}
```
Here when the outer function returns the inner function, it freezes the inner function which transitively freezes `p`. But before that freeze happens, we need to replay the ContextualMutation on the inner function to determine that the value is mutable in the outer context. If we froze `p` first, we would instead convert the ContextualMutation to a ReactMutation and error.
To handle this, InferReferenceEffects now delays the exection of the freezeValue action until after it's called the helper functions that generate function effects. So the order of operations on a given place is now
set effect --> generate function effects --> transitively freeze dependencies, if applicable
ghstack-source-id: 21cb50c14054e7e7a307acb595ef30b54c2f2a52
Pull Request resolved: https://github.com/facebook/react/pull/30920
Resubmission of #30079 -- core logic unchanged, but needed to rebase past #30573
### Quick background
#### Temporaries
The compiler currently treats temporaries and named variables (e.g. `x`) differently in this pass.
- named variables may be reassigned (in fact, since we're running after LeaveSSA, a single named identifier's IdentifierId may map to multiple `Identifier` instances -- each with its own scope and mutable range)
- temporaries are replaced with their represented expressions during codegen. This is correct (mostly correct, see #29878) as we're careful to always lower the correct evaluation semantics. However, since we rewrite reactive scopes entirely (to if/else blocks), we need to track temporaries that a scope produces in `ReactiveScope.declarations` and later promote them to named variables.
In the same example, $4, $5, and $6 need to be promoted: $2 ->`t0`, $5 ->`t1`, and $6 ->`t2`.
```js
[1] $2 = LoadGlobal(global) foo
[2] $3 = LoadLocal bar$1
scope 0:
[3] $4 = Call $2(<unknown> $3)
scope 1:
[4] $5 = Object { }
scope 2:
[5] $6 = Object { a: $4, b: $5 }
[6] $8 = StoreLocal Const x$7 = $6
```
#### Dependencies
`ReactiveScope.dependencies` records the set of (read-only) values that a reactive scope is dependent on. This is currently limited to just variables (named variables from source and promoted temporaries) and property-loads.
All dependencies we record need to be hoistable -- i.e. reordered to just before the ReactiveScope begins. Not all PropertyLoads are hoistable.
In this example, we should not evaluate `obj.a.b` without before creating x and checking `objIsNull`.
```js
// reduce-reactive-deps/no-uncond.js
function useFoo({ obj, objIsNull }) {
const x = [];
if (isFalse(objIsNull)) {
x.push(obj.a.b);
}
return x;
}
```
While other memoization strategies with different constraints exist, the current compiler requires that `ReactiveScope.dependencies` be re-orderable to the beginning of the reactive scope. But.. `PropertyLoad`s from null values will throw `TypeError`. This means that evaluating hoisted dependencies should throw if and only if the source program throws. (It is also a bug if source throws and compiler output does not throw. See https://github.com/facebook/react-forget/pull/2709)
---
### Rough high level overview
1. Pass 1
Walk over instructions to gather every temporary used outside of its defining scope (same as ReactiveFunction version). These determine the sidemaps we produce, as temporaries used outside of their declaring scopes get promoted to named variables later (and are not considered hoistable rvals).
2. Pass 2 (collectTemporariesSidemap)
Walk over instructions to generate a sidemap of temporary identifier -> named variable and property path (e.g. `$3 -> {obj: props, path: ["a", "b"]}`)
2. Pass 2 (collectHoistablePropertyLoads)
a. Build a sidemap of block -> accessed variables and properties (e.g. `bb0 -> [ {obj: props, path: ["a", "b"]} ]`)
b. Propagate "non-nullness" i.e. variables and properties for which we can safely evaluate `PropertyLoad`.
A basic block can unconditionally read from identifier X if any of the following applies:
- the block itself reads from identifier X
- all predecessors of the block read from identifier X
- all successors of the block read from identifier X
4. Pass 3: (collectDependencies)
Walks over instructions again to record dependencies and declarations, using the previously produced sidemaps. We do not record any control-flow here
5. Merge every scope's recorded dependencies with the set of hoistable PropertyLoads
Tested by syncing internally and (1) checking compilation output differences ([internal link](https://www.internalfb.com/intern/everpaste/?handle=GPCfUBt_HCoy_S4EAJDVFJyJJMR0bsIXAAAB)), running internally e2e tests ([internal link](https://fburl.com/sandcastle/cs5mlkxq))
---
### Followups:
1. Rewrite function expression deps
This change produces much more optimal output as the compiler now uses the function CFG to understand which variables / paths are assumed to be non-null. However, it may exacerbate [this function-expr hoisting bug](https://github.com/facebook/react/blob/main/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.tsx). A short term fix here is to simply call some form of `collectNonNullObjects` on every function expression to find hoistable variable / paths. In the longer term, we should refactor out `FunctionExpression.deps`.
2. Enable optional paths
(a) don't count optional load temporaries as dependencies (e.g. `collectOptionalLoadRValues(...)`).
(b) record optional paths in both collectHoistablePropertyLoads and dependency collection
ghstack-source-id: 2507f6ea751dce09ad1dccd353ae6fc7cf411582
Pull Request resolved: https://github.com/facebook/react/pull/30894
- flip `enablePropagateDepsInHIR` to off by default
- fork fixtures which produce compilation differences in #30894 to separate directory `propagate-scope-deps-hir-fork`, to be cleaned up when we remove this flag
ghstack-source-id: 7d5b8dc29788a65c272c846af9877b09fbf2cd60
Pull Request resolved: https://github.com/facebook/react/pull/30949
Adds evaluator support for a few compiler test fixtures
ghstack-source-id: 202654992a9876cea59885b54a338c908e369ddb
Pull Request resolved: https://github.com/facebook/react/pull/30948
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).
ghstack-source-id: 3e8b5a0a7010d0c484bbb417fb258e76bf4e32bc
Pull Request resolved: https://github.com/facebook/react/pull/30888
Reactive scopes in HIR has been stable for over 3 months now and is the future direction of react compiler, removing this flag to reduce implementation forks.
ghstack-source-id: 65cdf63cf76029fa22d40fd85aba0ac976dcfc08
Pull Request resolved: https://github.com/facebook/react/pull/30891
At Meta we have a pattern of using tagged template literals for features that are compiled away:
```
// Relay:
graphql`...graphql text...`
```
In many cases these tags produce a primitive value, and we can get even more optimal output if we can tell the compiler about these types. The new moduleTypeProvider gives us the ability to declare such types, this PR extends the compiler to use this type information for TaggedTemplateExpression values.
ghstack-source-id: 3cd6511b7f4e708bcb86f3f3fde5773bc51c7197
Pull Request resolved: https://github.com/facebook/react/pull/30869
Errors in an earlier component/hook shouldn't stop later components from compiling.
ghstack-source-id: 6e04a5bb2e2045303cbddad6d6d4bd38d5f7990b
Pull Request resolved: https://github.com/facebook/react/pull/30844
To prevent any difference in behavior, we check that the optionality of the inferred deps exactly matches the optionality of the manual dependencies. This required a fix, I was incorrectly inferring optionality of manual deps (they're only optional if OptionalTerminal.optional is true) - for nested cases of mixed optional/non-optional.
ghstack-source-id: afd49e89cc
Pull Request resolved: https://github.com/facebook/react/pull/30840
Per title. This gives us much more granular memoization when the source used optional member expressions. Note that we only infer optional deps when the source used optionals: we don't (yet) infer optional dependencies from conditionals.
ghstack-source-id: 104d0b712d
Pull Request resolved: https://github.com/facebook/react/pull/30838
Handles an additional case as part of testing combinations of the same path being accessed in different places with different segments as optional/unconditional.
ghstack-source-id: ace777fcbb
Pull Request resolved: https://github.com/facebook/react/pull/30836
Updates PropagateScopeDeps and DeriveMinimalDeps to understand optional dependency paths (`a?.b`). There a few key pieces to this:
In PropagateScopeDeps we jump through some hoops to work around the awkward structure of nested OptionalExpressions. This is much easier in HIR form, but I managed to get this pretty close and i think it will be landable with further cleanup. A good chunk of this is avoiding prematurely registering a value as a dependency - there are a bunch of indirections in the ReactiveFunction structure:
```
t0 = OptionalExpression
SequenceExpression
t0 = Sequence
...
LoadLocal t0
```
Where if at any point we call `visitOperand()` we'll prematurely register a dependency instead of declareProperty(). The other bit is that optionals can be optional=false for nested member expressions where not all the parts are actually optional (`foo.bar?.bar.call()`). And of course, parts of an optional chain can still be conditional even when optional=true (for example the `x` in `foo.bar?.[x]?.baz`). Not all of this is tested yet so there are likely bugs still.
The other bit is DeriveMinimalDeps, which is thankfully easier. We add OptionalAccess and OptionalDep and update the merge and reducing logic for these cases. There is probably still more to update though, for things like merging subtrees. There are a lot of ternaries that assume a result can be exactly one of two states (conditional/unconditional, dependency/access) and these assumptions don't hold anymore. I'd like to refactor to dependency/access separate from conditional/optional/unconditional. Also, the reducing logic isn't quite right: once a child is optional we keep inferring all the parents as optional too, losing some precision. I need to adjust the reducing logic to let children decide whether their path token is optional or not.
ghstack-source-id: 207842ac64
Pull Request resolved: https://github.com/facebook/react/pull/30819
If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation.
The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule.
ghstack-source-id: 9f7a99ee5f
Pull Request resolved: https://github.com/facebook/react/pull/30816
Branch terminals didn't have a fallthrough because they correspond to an outer terminal (optional, logical, etc) that has the "real" fallthrough. But understanding how branch terminals correspond to these outer terminals requires knowing the branch fallthrough. For example, `foo?.bar?.baz` creates terminals along the lines of:
```
bb0:
optional fallthrough=bb4
bb1:
optional fallthrough=bb3
bb2:
...
branch ... (fallthrough=bb3)
...
bb3:
...
branch ... (fallthrough=bb4)
...
bb4:
...
```
Without a fallthrough on `branch` terminals, it's unclear that the optional from bb0 has its branch node in bb3. With the fallthroughs, we can see look for a branch with the same fallthrough as the outer optional terminal to match them up.
ghstack-source-id: d48c623289
Pull Request resolved: https://github.com/facebook/react/pull/30814
Adds an `optional: boolean` property to each token in a DependencyPath, currently always set to false. Also updates the equality and printing logic for paths to account for this field.
Subsequent PRs will update our logic to determine which manual dependencies were optional, then we can start inferring optional deps as well.
ghstack-source-id: 66c2da2cfa
Pull Request resolved: https://github.com/facebook/react/pull/30813
Previously the path of a ReactiveScopeDependency was `Array<string>`. We need to track whether each property access is optional or not, so as a first step we change this to `Array<{property: string}>`, making space for an additional property in a subsequent PR.
ghstack-source-id: c5d38d72f6
Pull Request resolved: https://github.com/facebook/react/pull/30812
AnalyzeFunctions was reusing the `ReactiveScopeDependency` type since it happened to have a convenient shape, but we need to change this type to represent optionality. We now use a locally defined type instead.
ghstack-source-id: e305c6ede4
Pull Request resolved: https://github.com/facebook/react/pull/30811
Summary:
This addresses the issue of the compiler being overly restrictive about refs escaping into object expressions. Rather than erroring whenever a ref flows into an object, we will now treat the object itself as a ref, and apply the same escape rules to it. Whenever we look up a property from a ref value, we now don't know whether that value is itself a ref or a ref value, so we assume it's both.
The same logic applies to ref-accessing functions--if such a function is stored in an object, we'll propagate that property to the object itself and any properties looked up from it.
ghstack-source-id: 5c6fcb895d4a1658ce9dddec286aad3a57a4c9f1
Pull Request resolved: https://github.com/facebook/react/pull/30821
Summary:
We currently can return a ref from a hook but not an object containing a ref.
ghstack-source-id: 8b1de4991eb2731b7f758e685ba62d9f07d584b2
Pull Request resolved: https://github.com/facebook/react/pull/30820
This allows us to handle common operations such as `useFragment(...).edges.nodes ?? []` where we have a `Phi(MixedReadonly, Array)`. The underlying pattern remains general-purpose and not Relay-specific, and any API that returns transitively "mixed" data (primitives, arrays, plain objects) can benefit from the same type refinement.
ghstack-source-id: 51283108942002a14d032613a9d0b8b665ee3a94
Pull Request resolved: https://github.com/facebook/react/pull/30797
Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this:
1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`.
2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind.
3. Handle circular types by removing the cycle.
However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop.
ghstack-source-id: 2e1b02844d3a814dce094b7e3812df799e54343f
Pull Request resolved: https://github.com/facebook/react/pull/30796
This is a complex case: we not only need phi type inference but also need to be able infer the union of `MixedReadonly | Array`.
ghstack-source-id: 935088910dd8c210b3253cf8ff1f4b935f5081b7
Pull Request resolved: https://github.com/facebook/react/pull/30793
This reverts commit 44a2e62a2e.
This hack doesn't play well internally so I'm reverting this for now
(but keeping the compilationMode override). I'll audit the locations we
report later and try to make them more accurate so we won't need this
workaround.
ghstack-source-id: b6be29c11d
Pull Request resolved: https://github.com/facebook/react/pull/30792
We don't a full Identifier object for the return type, we can just store the type.
ghstack-source-id: 4594d64ce3900ced3e461945697926489898318e
Pull Request resolved: https://github.com/facebook/react/pull/30790
Rename this field so we can use it for the actual return type.
ghstack-source-id: 118d7dcfbbcc40911bf6d13f14e70053e436738d
Pull Request resolved: https://github.com/facebook/react/pull/30789
Uses the returnIdentifier added in the previous PR to provide a stable identifier for which we can infer a return type for functions, then wires up the equations in InferTypes to infer the type.
ghstack-source-id: 22c0a9ea096daa5f72821fca2a5ff5b199f65c8b
Pull Request resolved: https://github.com/facebook/react/pull/30785
This gives us a place to store type information, used in follow-up PRs.
ghstack-source-id: ee0bfa253f63c30ccaac083b9f1f72b76617f19c
Pull Request resolved: https://github.com/facebook/react/pull/30784
If you have a function expression which _captures_ a mutable value (but does not mutate it), and that function is invoked during render, we infer the invocation as a mutation of the captured value. But in some circumstances we can prove that the captured value cannot have been mutated, and could in theory avoid inferring a mutation.
ghstack-source-id: 47664e48ce8c51a6edf4d714d1acd1ec4781df80
Pull Request resolved: https://github.com/facebook/react/pull/30783
Adds a new Environment config option which allows specifying a function that is called to resolve types of imported modules. The function is passed the name of the imported module (the RHS of the import stmt) and can return a TypeConfig, which is a recursive type of the following form:
* Object of valid identifier keys (or "*" for wildcard) and values that are TypeConfigs
* Function with various properties, whose return type is a TypeConfig
* or a reference to a builtin type using one of a small list (currently Ref, Array, MixedReadonly, Primitive)
Rather than have to eagerly supply all known types (most of which may not be used) when creating the config, this function can do so lazily. During InferTypes we call `getGlobalDeclaration()` to resolve global types. Originally this was just for known react modules, but if the new config option is passed we also call it to see if it can resolve a type. For `import {name} from 'module'` syntax, we first resolve the module type and then call `getPropertyType(moduleType, 'name')` to attempt to retrieve the property of the module (the module would obviously have to be typed as an object type for this to have a chance of yielding a result). If the module type is returned as null, or the property doesn't exist, we fall through to the original checking of whether the name was hook-like.
TODO:
* testing
* cache the results of modules so we don't have to re-parse/install their types on each LoadGlobal of the same module
* decide what to do if the module types are invalid. probably better to fatal rather than bail out, since this would indicate an invalid configuration.
ghstack-source-id: bfdbf67e3dd0cbfd511bed0bd6ba92266cf99ab8
Pull Request resolved: https://github.com/facebook/react/pull/30771
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
Mofei Zhang