The current error message "This mutates a global or a variable after it
was passed to React" no longer makes sense since we now have more
specific error messages for different kinds of Effect.Mutate or
Effect.Stores. This replaces the fallthrough "Other" case with a
more generic message. It's not perfect, but it's a little more accurate
than what is currently emitted
The proper fix might be to treat functions as mutable objects and allow
the mutation, or special case `Function.displayName`. For now though
this PR just updates the message in the meantime so it's less
confusing.
We're doing some internal benchmarking using a lightweight bundler that @pieterv
wrote for experimentation purposes. It's designed to fully preserve Flow type
annotations so we can experiment with type-driven compilation and test out what
benefits we might get from "cross-module" compilation more easily (ie by just
bundling together a few modules so we can see them all as one).
However, the bundler renames local variables and imports, so that a reference to
`useMemo()` might end up as `React$useMemo()` or similar. This PR adds a flag to
tell the compiler that builtin hooks might be prefixed and resolve them
appropriately.
---
Currently, we error on non-hoisted identifiers in EnterSSA with a somewhat
cryptic message. This PR changes `BuildHIR` hoisting logic to find ALL hoistable
bindings, then error when we try to lower hoisting for unsupported declaration
types.
Two benefits to this refactoring:
- Dedups "unhandled identifier declaration" logic (previous to #2552 and this
PR, we did this check in three places).
- More explicit todo diagnostic messages when we cannot hoist a declaration
---
Three functional changes:
- Instead of visiting all identifier references, explicitly traverse only
function decls/exprs. This avoids bugs like accidentally hoisting inline
references
```js
// input
const x = identity(y);
const y = 2;
// lowered HIR before this PR (simplified)
[0] DeclareContext HoistedConst y$0
[1] LoadContext y$0
[2] StoreLocal Const x$5 = identity([1])
```
- Rely on `isReferencedIdentifier()` instead of manually checking member
properties / assignments, which is error prone
```js
// added fixture hoisting-repro-variable-used-in-assignment
const callbk = () => {
// before this PR, we skip hoisting x because it's part of a declaration
const copy = x;
return copy;
};
const x = 2;
return callbk();
```
- Visit lvalues after rvalues. This allows for recursive self-references (e.g.
factorial)
From the Babel side, this change relies heavily on babel's scope binding
resolution logic. My understanding is:
- Babel guarantees node objects are uniqued (`node1 === node2` <--> node1 and
node2 are the same node in the ast)
- Each binding has exactly one `bindingIdentifier` (`binding.identifier`,
`getBindingIdentifier`, etc) which is identifier node @ its declaration site
```js
// x is a binding identifier
const x = 2;
// foo is a binding identifier
function foo() {
}
// param is a binding identifier
(param) => {...}
// this bar is a binding identifier
let bar;
// but not this bar
bar = 2;
```
---
This is likely a rare edge case, but it does produce a parse error.
RenameVariables visits all identifier references to ensure we don't end up
producing conflicting variable declarations, using a stack of block scopes to
check "in scope variables".
This pass is currently built to be conservative -- we explicitly rename shadowed
variables, and visit all rvalue references. The issue is for this IR:
```
{
1. decl t0;
2. scope 0 {
3. reassign t0 = ...
4. read(t0)
5. }
6. let t0 = ...
7. read(t0);
}
```
We currently visit t0 only on line 4 and 7 (and never rename t0). Instead we
should visit lvalues (declaration sites) which occur earlier than rvalues
(visiting lines 1 and 6 will show conflicting declarations)
The compiler bails out of compiling code that contains suppressions of the
official React ESLint rules. However, some apps may use additional rules that
they want to trigger bailouts for, or use the official rules under a different
name (we do this at Meta). This PR adds a compiler flag to specify a custom set
of line rule names, suppression of which should trigger a bailout.
Fixtures from T173102122 and T173101739 demonstrating cases where
MergeConsecutiveBlocks can move code out of its correct block scope, changing
behavior or breaking the program, in cases where a control flow structure (such
as switch) only has one non-returning control flow path. In these cases, the
non-returning path gets merged with the fallthrough, effectively lifting that
code out of the control flow structure and moving it into the outer scope. This
can create dead code or just invalid code (with references to variables that are
not in scope).
Sprout fails on both of these fixtures:
<img width="812" alt="Screenshot 2024-01-23 at 11 25 36 AM"
src="https://github.com/facebook/react-forget/assets/6425824/d397ea22-3fa3-436e-b655-09a45781274b">
See the previous PR, interleaved mutation can cause values that were not
reactive to become reactive. I swear I had a case where this was observable, but
I came up with it before reordering the PRs in this stack. I think my repro
relied on an immutable reference to a mutable value, which is now handled in
InferReactivePlaces. So here i'm just adding fixtures, and allowing this case
since it's unobservable.
During PruneNonReactiveDependencies, we sometimes need to promote a value from
non-reactive to reactive if it ended up being grouped in the same reactive scope
as some other reactive value. This generally happens due to interleaving
mutations.
In this case all downstream usage of the promoted value need to also be
considered reactive. Fully propagating the reactivity requires re-running
InferReactivePlaces, to account for things like control reactivity. We can't yet
reuse that pass here though, because we haven't unified the pipeline on HIR yet.
For now, we propagate the reactivity through local variables and downstream
reactive scopes. See test fixtures for some examples that now correctly
propagate reactivity and some that need the full reactivity inference to run
correctly. The latter cases are handled in the next PR.
I found this by adding logic to reject inputs where reactivity gets newly
propagated in PruneNonReactiveDependencies. It's possible to create a readonly
alias to a mutable value such that we don't know the value is reactive yet when
the alias is created. Thus we need to do a fixpoint iteration even if there are
no loops in order to be able to revisit such aliases and reflow the reactivity
forward. Example:
```javascript
const x = [];
const y = x;
const z = [y]; // y isn't reactive yet when we first visit this, so z is
initially non-reactive
y.push(props.value); // then we realize y is reactive. we need a fixpoint to
propagate this back to z
const a = [z]; // need an indirection to get past the partial propagation in
PruneNonReactiveDependencies
let b = 0;
if (a[0][0]) {
b = 1;
}
return [b];
```
Existing fixtures don't change because the basic reactivity propagation in
PruneNonReactiveDependencies is enough to make common cases work. I confirmed
that the new fixture does not work on previous PR in the stack.
Fixes T175227223. When inferring reactivity, mutation of a value with a reactive
input marks the mutable value as reactive. However, we also need to account for
aliases:
```javascript
const x = [];
const y = x;
y.push(props.value);
```
Previously we would have only considered `y` reactive here, but `x` also becomes
reactive.
The implementation extracts out a helper from InferReactiveScopeVariables that
builds a `DisjointSet<Identifier>` of disjoint sets of mutably aliased values.
InferReactivePlaces then treats all instances of each mutable alias group as
equivalent for reactivity purposes.
In InferReactivePlaces, we already account for reactively controlled values:
where a value is never assigned a non-reactive value, but _which_ value is
assigned is based on a reactive condition (the test conditions of an if, switch,
loop, etc).
This PR extends that reactively-controlled inference to mutation that is
conditioned upon a reactive value. From the test case:
```javascript
let x = [];
if (props.cond) {
// This mutation has no reactive inputs.
// *But* the mutation conditionally occurs based on props.cond which is reactive
x.push(1);
}
let y = false;
if (x[0]) { // therefore the value observed here is reactive
y = true;
}
// so the value of y here is reactive via the reactive control dependency x[0]
return [y];
```
---
Previously, our logic was something like:
```js
fixed-point-loop {
foreach instruction {
mark referenced identifiers
// assume that usages are always visited before declarations
if (instruction is decl) {
prune(instruction);
}
}
foreach instruction {
if not referenced {
delete(instruction);
}
}
```
This contained a bug, as not all usages of a variable are guaranteed to be
visited before its declaration.
```js
// input
let x = 0;
while(x < 10) {
x += 2;
}
return x;
// hir
entry:
x$0 = 0
goto loop-test
loop-test:
x$1 = phi(x$0, x$2)
if ... goto loop-body else goto fallthrough
loop-body:
x$2 = x$1 ...
goto loop-test
fallthrough:
return x$1
```
In this example,`x$2` is defined by `loop-body` and used by `loop-test`.
Similarly, `x$1` is defined by `loop-test` and used by `loop-body`.
---
TODO: trying to come up with more test fixtures
Same babel identifier issue as #2510 but for HoistedConst
Not sure how we should best test this -- one possibility is using constant prop.
Currently, we have false positives for HoistedConst that prevent constant
propagation. I don't want to over-rotate on babel apis tests in our fixtures
(instead of semantically interesting ones)
```js
// input
function Component() {
{ x: 4 };
const x = 2;
return x;
}
// output
function Component() {
const $ = useMemoCache(1);
let x;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
x = 2;
$[0] = x;
} else {
x = $[0];
}
return x;
}
```
identifiers
---
A few fixes for finding context identifiers:
Previously, we counted every babel identifier as a reference. This is
problematic because babel counts every string symbol as an identifier.
```js
print(x); // x is an identifier as expected
obj.x // x is.. also an identifier here
{x: 2} // x is also an identifier here
```
This PR adds a check for `isReferencedIdentifier`. Note that only non-lval
references pass this check
```js
print(x); // isReferencedIdentifier(x) -> true
obj.x // isReferencedIdentifier(x) -> false
{x: 2} // isReferencedIdentifier(x) -> false
x = 2 // isReferencedIdentifier(x) -> false
```
Which brings us to change #2.
Previously, we counted assignments as references due to the identifier visiting
+ checking logic. The logic was roughly the following (from #1691)
```js
contextVars = intersection(reassigned, referencedByInnerFn);
```
Now that assignments (lvals) and references (rvals) are tracked separately, the
equivalent logic is this. Note that assignment to a context variable does not
need to be modeled as a read (`console.log(x = 5)` always will evaluates and
prints 5, regardless of the previous value of x).
```
contextVars = union(reassignedByInnerFn, intersection(reassigned,
referencedByInnerFn))
```
---
Note that variables that are never read do not need to be modeled as context
variables, but this is unlikely to be a common pattern.
```js
function fn() {
let x = 2;
const inner = () => {
x = 3;
}
}
```
I sincerely appreciate the effort to get test262 up and running. This was my
idea, it seemed like a really good way to test our correctness on edge cases of
JS. Unfortunately test262 relies heavily on a few specific features that we
don't support, like classes and `var`, which has meant that we never actually
use this as a test suite.
In the meantime we've created a pretty extensive test suite and have tools like
Sprout to test actual memoization behavior at runtime, which is the right place
to invest our energy. Let's remove?
Do we still use these? I'm happy to close this PR if we still want this but it
feels like these may have served their purpose and no longer be necessary.
Fixes the false positive in the previous PR. When we prune a scope because it's
values are non-escaping, we now also remove any `Memoize` instructions for that
scope. The intuition being that we're actively removing unnecessary memoization,
so we don't need to check that the memoization occurred anymore.
This demonstrates a false positive in validatePreserveExistingManualMemoization.
We prune memoization of non-escaping values, but the validation pass just sees
that the value "should" have a scope and that scope doesn't exist, and thinks we
failed to preserve memoization.
Interpolating values into the `reason` field of an error breaks our error
aggregation. This PR moves the offending function name into the `description`
field which isn't used for aggregation.
I had trouble checking out the repo using Sapling because the submodule couldn't
clone properly. I got
> Error: Permission denied (publickey)
In my branch I tested that the https URL format seems to work okay with Sapling.
Add frozen reason for props and hook arguments
Improves the error message when mutating props or hook arguments.
Previously, this would print a generic error about mutating global variables.
This was an oversight in the original definition of useContext (oops my bad).
Context values are owned by React and should not be modified. I found this
because some cases of existing useMemo were not preserved (tested via the
validatePreserveExistingManualMemo flag) due to function calls referencing
context being assumed to mutate.
This change will allow more memoization, it's also just more correct for the
rules of React. Note the new ValueReason variant so that we can provide a
precise error message about mutating context values.
It's starting to get complex just with a couple of extra
passes — we either need to substantially extend the HIR or (as i've done so far)
pass information from early passes to later ones. This PR changes things so that
very early in the babel plugin we fork into a separate mode. Forest has
its own `compileProgram()` equivalent, its own pipeline, its own codegen, etc.
Joe Savona
Sathya Gunasekaran
Lauren Tan
Mofei Zhang
Jan Kassens