Changes the lowering for sequence expressions to use the new terminal. When
converting to a ReactiveFunction, we convert these terminals into
ReactiveSequenceValues, which nests the instructions and preserves order of
evaluation in the output.
The only catch is constant propagation — constant propagation breaks
order-of-evaluation because it can effectively copy the final value of a
sequence elsewhere, leaving the original sequence in the wrong place. I'll
address that in a follow-up.
I realized that we can use our value block system to fix _most_ of the remaining
order-of-evaluation issues we had with sequence expressions. This PR adds a new
SequenceTerminal to HIR; there is already a ReactiveFunction equivalent
(ReactiveSequenceValue) that the next PR will convert this terminal into.
Handles three more cases:
* Template literals
* deletion (property/computed)
* type casts
Only the latter has an observable impact, though i added tests for deletion just
in case and found a bug. For type casts, they're reasonably common internally
for fixmes, so this PR will help to ensure we don't drop type information just
because of a cast.
Renames some error fixtures for clarity, "error.invalid-*" are fixtures that are
expected to fail for invalid input, where other "error.*" fixtures are basically
todos. While i was here i clarified the error messages for invalid useMemo
callbacks, and changes the error severity from Invariant to InvalidInput.
Fixes one more category of bug. For assignment expressions, we validating
against redeclaring a global variable when the assignment target was an
identifier, but not when the global was reassigned via destructuring. This PR
adds a `lowerIdentifierForAssignment()` helper and uses it for assignment of all
identifier variants, including destructuring.
I reviewed the test cases we have marked as bugs ("_bug.*") and realized that
several of them are already fixed — woohoo! Then one wasn't fixed _yet_: our
type inference loses track of refs if you stash them inside an object/array. But
that's why I added the ValidateNoRefAccessInRender pass, which i've updated to
detect and reject these invalid cases. There are now only a few bugs left (more
fixes coming).
These are still quite Babel specific, but the interface is slightly more
generic: CompilerEntrypoint takes a non-Babel specific CompilerPass as an
argument instead of passing Babel's PluginPass directly.
In the future we can consider lowering the whole Program into HIR but that
involves a significant lift in our representation and a small amount of new
syntax to support (eg import statements), so this PR is the extent of this stack
for now
Updates PruneNonReactiveDependencies to treat setState functions as
non-reactive, since we know they have a stable identity. This is based on type
inference and our recently added definitions for useState and its return type,
so it's conservative and will only work when our inference can prove that the
scope dependency has the SetState type.
Note that this approach is simple and has limitations, notably the fact that the
setState is non-reactive doesn't propagate. But it's simple, trivially correct,
and already improves codegen somewhat, so i figured it's worth landing for now.
## Test Plan
Tested on internal app
Adds validation to reject freezing mutable lambdas, since lambdas cannot _be_
frozen, they're either frozen or not. Example invalid code:
```javascript
function Component(props) {
const x = {value: ""};
const onChange = (e) => {
// MUTATION!!!!!
x.value = e.target.value;
setX(x);
};
return <input value={x.value} onChange={onChange} />;
}
```
Note that there is a separate issue in which we are not detecting lambdas that
would definitely modify immutable values. We may need to distinguish
ConditionallyCapture (captures if the value is mutable, otherwise readonly) from
Capture (definitely mutates) in order to make that case work. But already this
validation helps prevent some invalid code.
Adds a `removeAllMemoization` flag that runs the entire compiler pipeline but
strips out all memoization. The intent is to be able to compare (in limited
use-cases) the performance of an existing app with all memoization removed, vs
the performance with manual memoization, vs the performance with Forget enabled.
In terms of how this works: we already strip out useMemo/useCallback since
Forget is more accurate. The new option adds an extra pass that strips out all
reactive scopes. Collectively this leaves ~zero memoization within components
(this does leave React.memo, but close enough).
---
Remove jest fixture tests in favor of snap runner. Main reasons:
- maintaining feature flags and compatible behavior required syncing all changes
to 3 files (`generateTestsFromFixtures`, `compiler-test`, and `compiler-worker`)
- jest snapshot test file causes rebase conflicts on most rebases
- speed 🙌
$ time yarn test compiler-test
(the extra test here is `has a consistent extension for input fixtures`)
```
Test Suites: 1 passed, 1 total
Tests: 37 skipped, 480 passed, 517 total
Snapshots: 479 passed, 479 total
Time: 27.668 s
Ran all test suites matching /compiler-test/i.
✨ Done in 43.18s.
yarn test compiler-test 57.05s user 3.85s system 139% cpu 43.546 total
```
$ time yarn snap
```
478 Tests, 478 Passed, 0 Failed
✨ Done in 13.12s.
yarn snap 53.96s user 9.35s system 468% cpu 13.518 total
```
Jest and snap should have the same set of features:
- report test failures via exit status (used by Git Actions)
- watch mode
- breakpoints + `debugger` statements
- note that `--sync` is not required for this
- skip `todo.` prefixed fixtures
- fixtures in nested directories e.g. `rules-of-hooks/testname.js`
- filter mode (via editing `testfilter.txt`)
- filter + debug mode
(1) edit `testfilter.txt` to filter out all but one test
(2) add `@debug` pragma to the first line of the test
testfilter.txt
```js
// @only
testfixture_basename1
testfixture_basename2
```
Turns out I just forgot to forward `process.env` when forking 😅
`debugger` statements and breakpoints should work with both `--sync` and
`--no-sync` (default) modes
This is the example we discussed in our design sync.
```javascript
function Component(props) {
const [x, setX] = useState({ value: "" });
const onChange = (e) => {
// INVALID! should use copy-on-write and pass the new value
x.value = e.target.value;
setX(x);
};
return <input value={x.value} onChange={onChange} />;
}
```
Here `onChange` is a mutable lambda, and it should be invalid to pass a mutable
lambda where a frozen value is expected. This is because unlike other value
types, you cannot freeze a lambda — the only choice is to not call it at all.
Note that there is a harder case to catch:
```js
function Component(props) {
const [x, setX] = useState({ value: "" });
const onChange = (e) => {
// INVALID! should use copy-on-write and pass the new value
x.value = e.target.value;
setX(x);
};
const x = constructAValueThatMaybeAliasesItsInput(onChange);
return <input value={x.value} onChange={x.maybeGetTheLambdaBack()} />;
}
```
This case demonstrates how mutable lambdas can be captured and then accessed
later — the analysis to catch this case is more sophisticated bc it involves
inferring that `x` aliases a mutable lambda. But we also can't be sure that `x`
does alias the lambda, so disallowing this code could prevent a lot of valid
code from compiling. My hypothesis is that we should start with at least
validating the example at the top, while allowing the second case for now.
We can now type `Array.prototype.{map,filter}`:
* The callee is ConditionallyMutable because, although the array itself is not
modified, its items flow into the lambda and may be modified there.
* The argument is ConditionallyMutable because it accepts both mutable and
immutable lambdas. Mutate would disallow immutable lambdas (wrong), while Read
would be incorrect for mutable lambdas since calling them triggers mutation.
Adds test cases to ensure we're correctly inferring mutative builtin operations
— property store, computed property store, property deletion, and computed
property deletion — as definite mutation and that we're rejecting inputs where
these operations are used on immutable/frozen values.
Adds back `Effect.Mutate`, and changes so that `Effect.ConditionallyMutate`
never rejects frozen/immutable values, while `Effect.Mutate` _always_ rejects
frozen/immutable values.
We currently use `Effect.Mutate` both for places that _may_ mutate (ie untyped
function calls) and for places that have known mutation (typed function calls,
or operations like `delete x.y`). We then use a separate mechanism to decide
whether to reject the input, with some call paths checking the effect and others
not.
This stack refactors this logic in InferReferenceEffects per our discussion, so
that `Effect.ConditionallyMutate` is for "may or may not mutate" either because
we're not 100% sure (untyped function) or because the mutation depends on the
operand (ie, a callback arg that will be invoked and thus will mutate if the
lambda is mutable, not mutate if the lambda is immutable). Later diffs add back
`Effect.Mutate` as "definitely 100% mutating".
Defines 4 new types:
* Return type of `useState()`, which has properties "0" and "1" to allow us to
infer the types when destructuring
* Type of useState() set state function
* Return type of `useRef()` so we know what is a ref
* Type of ref.current, so we know what is a ref *value*
Example:
<img width="1670" alt="Screenshot 2023-05-24 at 9 59 37 AM"
src="https://github.com/facebook/react-forget/assets/6425824/3ee7d04a-fda3-4b7b-89b7-d205d9a6fd0d">
---
Toggle default to true, since this should be a no-op refactor.
Tests:
- test fixtures
- ran on Store + - and saw no difference in compiled output
- [diff](P744101621) with
`enableTreatHooksAsFunctions=false`
- [diff](P744105565) with
`enableTreatHooksAsFunctions=true`
Adds a new feature flag which tells the compiler to assume that hooks follow the
Rules of React. Specifically, the idea that since any hook could be wrapped in a
giant `useMemo()` call, all arguments to hooks have to be treated as if they're
owned by React — and therefore become immutable — and that the return value of
the hook is immutable.
Our default is to assume that hooks break the rules, but in practice nearly
every component follows them.
Updates the InferReferenceEffects logic for CallExpression to work similarly to
MethodCall, where we take into account the function signature (if present) when
inferring the effects and return kind.
Defines the `Boolean`, `String`, and `Number` global functions. This will be
useful for allowing developers to wrap statements that produce a primitive in a
way that Forget knows about in order to optimize better.
Joe Savona
Lauren Tan
Sathya Gunasekaran
Mofei Zhang