The previous error for hoisting violations pointed only to the variable declaration, but didn't show where the value was accessed before that declaration. We now track where each hoisted variable is first accessed and report two errors, one for the reference and one for the declaration. When we improve our diagnostic infra to support reporting errors at multiple locations we can merge these into a single conceptual error.
The previous error message was generic, because the old style function
signature didn't support a way to specify a reason alongside a freeze
effect. This meant we could only say why a value was frozen for
instructions, but not hooks which use function signatures. By defining a
new aliasing signature for custom hooks we can specify a reason and
provide a better error message.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33513).
* #33571
* #33558
* #33547
* #33543
* #33533
* #33532
* #33530
* #33526
* #33522
* #33518
* #33514
* __->__ #33513
AnalyzeFunctions had logic to reset the mutable ranges of context
variables after visiting inner function expressions. However, there was
a bug in that logic: InferReactiveScopeVariables makes all the
identifiers in a scope point to the same mutable range instance. That
meant that it was possible for a later function expression to indirectly
cause an earlier function expressions' context variables to get a
non-zero mutable range.
The fix is to not just reset start/end of context var ranges, but assign
a new range instance. Thanks for the help on debugging, @mofeiz!
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33500).
* #33571
* #33558
* #33547
* #33543
* #33533
* #33532
* #33530
* #33526
* #33522
* #33518
* #33514
* #33513
* #33512
* #33504
* __->__ #33500
* #33497
* #33496
Squashed, review-friendly version of the stack from
https://github.com/facebook/react/pull/33488.
This is new version of our mutability and inference model, designed to
replace the core algorithm for determining the sets of instructions
involved in constructing a given value or set of values. The new model
replaces InferReferenceEffects, InferMutableRanges (and all of its
subcomponents), and parts of AnalyzeFunctions. The new model does not
use per-Place effect values, but in order to make this drop-in the end
_result_ of the inference adds these per-Place effects.
I'll write up a larger document on the model, first i'm doing some
housekeeping to rebase the PR.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33494).
* #33571
* #33558
* #33547
* #33543
* #33533
* #33532
* #33530
* #33526
* #33522
* #33518
* #33514
* #33513
* #33512
* #33504
* #33500
* #33497
* #33496
* #33495
* __->__ #33494
* #33572
This bug was reported via our wg and appears to only affect values
created as a ref.
Currently, postfix operators used in a callback gets compiled to:
```js
modalId.current = modalId.current + 1; // 1
const id = modalId.current; // 1
return id;
```
which is semantically incorrect. The postfix increment operator should
return the value before incrementing. In other words something like this
should have been compiled instead:
```js
const id = modalId.current; // 0
modalId.current = modalId.current + 1; // 1
return id;
```
This bug does not trigger when the incremented value is a plain
primitive, instead there is a TODO bailout.
This is a babel bug + edge case.
Babel compact mode produces invalid JavaScript (i.e. parse error) when
given a `NumericLiteral` with a negative value.
See https://codesandbox.io/p/devbox/5d47fr for repro.
## Summary
While investigating the root cause of #33208, I noticed a clear typo for
one of the validation files.
## How did you test this change?
Inside `/react/compiler/packages/babel-plugin-react-compiler` I ran the
test script successfully:
<img width="415" alt="Screenshot at May 22 16-43-06"
src="https://github.com/user-attachments/assets/3fe8c5e1-37ce-4a31-b35e-7e323e57cd9d"
/>
Inferred effect dependencies now include optional chains.
This is a temporary solution while
https://github.com/facebook/react/pull/32099 and its followups are
worked on. Ideally, we should model reactive scope dependencies in the
IR similarly to `ComputeIR` -- dependencies should be hoisted and all
references rewritten to use the hoisted dependencies.
`
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33326).
* __->__ #33326
* #33325
* #32286
React Compiler's program traversal logic is pretty lengthy and complex
as we've added a lot of features piecemeal. `compileProgram` is 300+
lines long and has confusing control flow (defining helpers inline,
invoking visitors, mutating-asts-while-iterating, mutating global
`ALREADY_COMPILED` state).
- Moved more stuff to `ProgramContext`
- Separated `compileProgram` into a bunch of helpers
Tested by syncing this stack to a Meta codebase and observing no
compilation output changes (D74487851, P1806855669, P1806855379)
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33147).
* #33149
* #33148
* __->__ #33147
New take on #29716
## Summary
Template literals consisting entirely of constant values will be inlined
to a string literal, effectively replacing the backticks with a double
quote.
This is done primarily to make the resulting instruction a string
literal, so it can be processed further in constant propatation. So this
is now correctly simplified to `true`:
```js
`` === "" // now true
`a${1}` === "a1" // now true
```
If a template string literal can only partially be comptime-evaluated,
it is not that useful for dead code elimination or further constant
folding steps and thus, is left as-is in that case. Same is true if the
literal contains an array, object, symbol or function.
## How did you test this change?
See added tests.
(Almost) all pragmas are now one of the following:
- `@...TestOnly`: custom pragma for test fixtures
- `@<configName>` | `@<configName>:true`: enables with either true or a
default enabled value
- `@<configName>:<json value>`
## Summary
`-constant` is represented as a `UnaryExpression` node that is currently
not part of constant folding. If the operand is a constant number, the
node is folded to `constant * -1`. This also coerces `-0` to `0`,
resulting in `0 === -0` being folded to `true`.
## How did you test this change?
See attached tests
Note that bailing out adds false positives for hoisted functions whose
only references are within other functions. For example, this rewrite
would be safe.
```js
// source program
function foo() {
return bar();
}
function bar() {
return 42;
}
// compiler output
let bar;
if (/* deps changed */) {
function foo() {
return bar();
}
bar = function bar() {
return 42;
}
}
```
These false positives are difficult to detect because any maybe-call of
foo before the definition of bar would be invalid.
Instead of bailing out, we should rewrite hoisted function declarations
to the following form.
```js
let bar$0;
if (/* deps changed */) {
// All references within the declaring memo block
// or before the function declaration should use
// the original identifier `bar`
function foo() {
return bar();
}
function bar() {
return 42;
}
bar$0 = bar;
}
// All references after the declaring memo block
// or after the function declaration should use
// the rewritten declaration `bar$0`
```
This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal.
We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error.
This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default.
ghstack-source-id: 075a731444
Pull Request resolved: https://github.com/facebook/react/pull/33079
If a function captures a mutable value but never gets called, we don't infer a mutable range for that function. This means that we also don't alias the function with its mutable captures.
This case is tricky, because we don't generally know for sure what is a mutation and what may just be a normal function call. For example:
```js
hook useFoo() {
const x = makeObject();
return () => {
return readObject(x); // could be a mutation!
}
}
```
If we pessimistically assume that all such cases are mutations, we'd have to group lots of memo scopes together unnecessarily. However, if there is definitely a mutation:
```js
hook useFoo(createEntryForKey) {
const cache = new WeakMap();
return (key) => {
let entry = cache.get(key);
if (entry == null) {
entry = createEntryForKey(key);
cache.set(key, entry); // known mutation!
}
return entry;
}
}
```
Then we have to ensure that the function and its mutable captures alias together and end up in the same scope. However, aliasing together isn't enough if the function and operands all have empty mutable ranges (end = start + 1).
This pass finds function expressions and object methods that have an empty mutable range and known-mutable operands which also don't have a mutable range, and ensures that the function and those operands are aliased together *and* that their ranges are updated to end after the function expression. This is sufficient to ensure that a reactive scope is created for the alias set.
NOTE: The alternative is to reject these cases. If we do that we'd also want to similarly disallow cases like passing a mutable function to a hook.
ghstack-source-id: 5d8158246a
Pull Request resolved: https://github.com/facebook/react/pull/33078
Building on mofeiz's recent work to type constructors. Also, types for reanimated values which are useful in the next PR.
ghstack-source-id: 1c81e213a1
Pull Request resolved: https://github.com/facebook/react/pull/33077
This pass didn't previously report the precise difference btw inferred/manual dependencies unless a debug flag was set. But the error message is really good (nice job mofeiz): the only catch is that in theory the inferred dep could be a temporary that can't trivially be reported to the user.
But the messages are really useful for quickly verifying why the compiler couldn't preserve memoization. So here we switch to outputting a detailed message about the discrepancy btw inferred/manual deps so long as the inferred dep root is a named variable. I also slightly adjusted the message to handle the case where there is no diagnostic, which can occur if there were no manual deps but the compiler inferred a dependency.
ghstack-source-id: 534f6f1fec
Pull Request resolved: https://github.com/facebook/react/pull/33095
If a JSX attribute value is a string that contains unicode or other characters that need special escaping, we wrap the attribute value in an expression container. However, our unicode to detect this only handled the basic unicode character plane, not the "astral" plane which includes emojis.
This PR updates the regex to detect such extended characters and also use an expression container.
ghstack-source-id: 6d9c8e4dd2
Pull Request resolved: https://github.com/facebook/react/pull/33096
We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions.
Still WIP, this needs to handle terminals other than for..of.
ghstack-source-id: 09a2923051
Pull Request resolved: https://github.com/facebook/react/pull/33062
```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
When effect dependencies cannot be inferred due to memoization-related
bailouts or unexpected mutable ranges (which currently often have to do
with writes to refs), fall back to traversing the effect lambda itself.
This fallback uses the same logic as PropagateScopeDependencies:
1. Collect a sidemap of loads and property loads
2. Find hoistable accesses from the control flow graph. Note that here,
we currently take into account the mutable ranges of instructions (see
`mutate-after-useeffect-granular-access` fixture)
3. Collect the set of property paths accessed by the effect
4. Merge to get the set of minimal dependencies
Inferred effect dependencies and inlined jsx (both experimental
features) rely on `InferReactivePlaces` to determine their dependencies.
Since adding type inference for phi nodes
(https://github.com/facebook/react/pull/30796), we have been incorrectly
inferring stable-typed value blocks (e.g. `props.cond ? setState1 :
setState2`) as non-reactive. This fix patches InferReactivePlaces
instead of adding a new pass since we want non-reactivity propagated
correctly
Previously the CompileSuccess event would emit first before CompileSkip,
so the lsp's codelens would incorrectly flag skipped components/hooks
(via 'use no memo') as being optimized.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33012).
* __->__ #33012
* #33011
* #33010
Adds a new codeaction event in the compiler and handler in forgive. This
allows you to remove a dependency array when you're editing a range that
is within an autodep eligible function.
Co-authored-by: Jordan Brown <jmbrown@meta.com>
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33000).
* #33002
* #33001
* __->__ #33000
Co-authored-by: Jordan Brown <jmbrown@meta.com>
Joe Savona
Joseph SavonaandGitHub
Jordan BrownandGitHub
laurenandGitHub
RickyandGitHub
mofeiZandGitHub
Wesley LeMahieuandGitHub
Jan KassensandGitHub
Niklas MollenhauerandGitHub