Alternate take at a new mutability and alising model, aiming to replace `InferReferenceEffects` and `InferMutableRanges`. My initial passes at this were more complicated than necessary, and I've iterated to refine and distill this down to the core concepts. There are two effects that track information flow: `capture` and `alias`:
* Given distinct values A and B. After capture A -> B, mutate(B) does *not* modify A. This more precisely captures the semantic of the previous `Store` effect. As an example, `array.push(item)` has the effect `capture item -> array` and `mutate(array)`. The array is modified, not the item.
* Given distinct values A and B. After alias A -> B, mutate(B) *does* modify A. This is because B now refers to the same value as A.
* Given distinct values A and B, after *either* capture A -> B *or* alias A -> B, transitiveMutate(B) counts as a mutation of A.
Conceptually "capture A -> B" means that a reference to A was "captured" (or stored) within A, but there is not a directly aliasing. Whereas "alias A -> B" means literal value aliasing.
The idea is that our previous sequential fixpoint loops in InferMutableRanges can instead work by first looking at transitive mutations, then look at non-transitive mutations. And aliasing groups can be built purely based on the `alias` effect.
Lots more to do here but the structure is coming together.
[ghstack-poisoned]
Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following:
```
const x = {y: {z: {}}};
x.y.z.key = value;
```
That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z).
But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object.
[ghstack-poisoned]
We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't.
The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root.
[ghstack-poisoned]
This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
This is a stab at addressing a pattern that @mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example @mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value.
With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable.
[ghstack-poisoned]
```js
function Component() {
useEffect(() => {
let hasCleanedUp = false;
document.addEventListener(..., () => hasCleanedUp ? foo() : bar());
// effect return values shouldn't be typed as frozen
return () => {
hasCleanedUp = true;
}
};
}
```
### Problem
`PruneHoistedContexts` currently strips hoisted declarations and
rewrites the first `StoreContext` reassignment to a declaration. For
example, in the following example, instruction 0 is removed while a
synthetic `DeclareContext let` is inserted before instruction 1.
```js
// source
const cb = () => x; // reference that causes x to be hoisted
let x = 4;
x = 5;
// React Compiler IR
[0] DeclareContext HoistedLet 'x'
...
[1] StoreContext reassign 'x' = 4
[2] StoreContext reassign 'x' = 5
```
Currently, we don't account for `DeclareContext let`. As a result, we're
rewriting to insert duplicate declarations.
```js
// source
const cb = () => x; // reference that causes x to be hoisted
let x;
x = 5;
// React Compiler IR
[0] DeclareContext HoistedLet 'x'
...
[1] DeclareContext Let 'x'
[2] StoreContext reassign 'x' = 5
```
### Solution
Instead of always lowering context variables to a DeclareContext
followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let'
| 'Reassign' | etc` on StoreContext.
Pros:
- retain more information in HIR, so we can codegen easily `const` and
`let` context variable declarations back
- pruning hoisted `DeclareContext` instructions is simple.
Cons:
- passes are more verbose as we need to check for both `DeclareContext`
and `StoreContext` declarations
~(note: also see alternative implementation in
https://github.com/facebook/react/pull/32745)~
### Testing
Context variables are tricky. I synced and diffed changes in a large
meta codebase and feel pretty confident about landing this. About 0.01%
of compiled files changed. Among these changes, ~25% were [direct
bugfixes](https://www.internalfb.com/phabricator/paste/view/P1800029094).
The [other
changes](https://www.internalfb.com/phabricator/paste/view/P1800028575)
were primarily due to changed (corrected) mutable ranges from
https://github.com/facebook/react/pull/33047. I tried to represent most
interesting changes in new test fixtures
`
Fixes an edge case in React Compiler's effects inference model.
Returned values should only be typed as 'frozen' if they are (1) local
and (2) not a function expression which may capture and mutate this
function's outer context. See test fixtures for details
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33047).
* #32765
* #32747
* __->__ #33047
Adds Effect.ConditionallyMutateIterator, which has the following
effects:
- capture for known array, map, and sets
- mutate for all other values
An alternative to this approach could be to add polymorphic shape
definitions
* Adds `isConstructor: boolean` to `FunctionType`. With this PR, each
typed function can either be a constructor (currently only known
globals) or non constructor. Alternatively, we prefer to encode
polymorphic types / effects (and match the closest subtype)
* Add Map and Set globals + built-ins
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32697).
* #32698
* __->__ #32697
Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of
`run` parameters. This is a minimal difference but lets us explicitly
opt out certain compiler passes based on mode parameters, instead of
environment configurations
Retry flags don't really make sense to have in `EnvironmentConfig`
anyways as the config is user-facing API, while retrying is a compiler
implementation detail.
(per @josephsavona's feedback
https://github.com/facebook/react/pull/32164#issuecomment-2608616479)
> Re the "hacky" framing of this in the PR title: I think this is fine.
I can see having something like a compilation or output mode that we use
when running the pipeline. Rather than changing environment settings
when we re-run, various passes could take effect based on the
combination of the mode + env flags. The modes might be:
>
> * Full: transform, validate, memoize. This is the default today.
> * Transform: Along the lines of the backup mode in this PR. Only
applies transforms that do not require following the rules of React,
like `fire()`.
> * Validate: This could be used for ESLint.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32511).
* #32512
* __->__ #32511
LoweredFunction dependencies were exclusively used for dependency
extraction (in `propagateScopeDeps`). Now that we have a
`propagateScopeDepsHIR` that recursively traverses into nested
functions, we can delete `dependencies` and their associated synthetic
`LoadLocal`/`PropertyLoad` instructions.
[Internal snapshot
diff](https://www.internalfb.com/phabricator/paste/view/P1716950202) for
this change shows ~.2% of files changed. I [read through ~60 of the
changed
files](https://www.internalfb.com/phabricator/paste/view/P1733074307)
- most changes are due to better outlining (due to better DCE)
- a few changes in memo inference are due to changed ordering
```
// source
arr.map(() => contextVar.inner);
// previous instructions
$0 = LoadLocal arr
$1 = $0.map
// Below instructions are synthetic
$2 = LoadLocal contextVar
$3 = $2.inner
$4 = Function deps=$3 context=contextVar {
...
}
```
- a few changes are effectively bugfixes (see
`aliased-nested-scope-fn-expr`)
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32096).
* #32099
* #32286
* #32104
* #32098
* #32097
* __->__ #32096
Hacky retry pipeline for when transforming `fire(...)` calls encounters
validation, todo, or memoization invariant bailouts. Would love feedback
on how we implement this to be extensible to other compiler
non-memoization features (e.g. inlineJSX)
Some observations:
- Compiler "front-end" passes (e.g. lower, type, effect, and mutability
inferences) should be shared for all compiler features -- memo and
otherwise
- Many passes (anything dealing with reactive scope ranges, scope blocks
/ dependencies, and optimizations such as ReactiveIR #31974) can be left
out of the retry pipeline. This PR hackily skips memoization features by
removing reactive scope creation, but we probably should restructure the
pipeline to skip these entirely on a retry
- We should maintain a canonical set of "validation flags"
Note the newly added fixtures are prefixed with `bailout-...` when the
retry fire pipeline is used. These fixture outputs contain correctly
inserted `useFire` calls and no memoization.
Summary:
The fact that phis are identifiers rather than places is unfortunate in a few cases. In some later analyses, we might wish to know whether a phi is reactive, but we don't have an easy way to do that currently.
Most of the changes here is just replacing phi.id with phi.place.identifier and such. Interesting bits are EnterSSA (several functions now take places rather than identifiers, and InferReactivePlaces now needs to mark places as reactive explicitly.
ghstack-source-id: 5f4fb396cd
Pull Request resolved: https://github.com/facebook/react/pull/31171
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: 21cb50c140
Pull Request resolved: https://github.com/facebook/react/pull/30920
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: 3cd6511b7f
Pull Request resolved: https://github.com/facebook/react/pull/30869
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: c05e716f37
Pull Request resolved: https://github.com/facebook/react/pull/30766
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: e8f36ac25e
Pull Request resolved: https://github.com/facebook/react/pull/30713
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`.
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
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.
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
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.
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: 4aba4e6d21
Pull Request resolved: https://github.com/facebook/react/pull/29591
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: 9f6b8dbc50
Pull Request resolved: https://github.com/facebook/react/pull/29154
Joe Savona
mofeiZ
Mike Vitousek
Jan Kassens
Niklas Mollenhauer
XiaoPi