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.
Handles some edge-cases where we previously flattened away some of the structure
of a labeled block, instead ensuring that we retain the original shape. See the
output.
## Test Plan
Tested on the internal app we're focused on (w useMemo inlining enabled), it
works fine.
I realized a wayyyy simpler approach to inlining a lambda: wrap it in a labeled
block. The transformation is roughly as follows:
```javascript
// Before
const x = useMemo(() => {
if (a) {
return b;
}
return c;
}, [a, b, c]);
return x;
// After
let x;
label: {
if (a) {
x = b;
break label;
}
x = c;
break label;
}
return x;
```
The key to making this work is fixing up some edge cases in labeled blocks,
hence the previous PRs.
This is part of a stack to fix some edge cases in inlining of useMemo closures.
In this first step, I'm disabling `shrink()` in order to retain more information
about the data flow. For example,
```
label: if (cond) {
break label;
}
return foo;
```
Would previously have shrunk away the if body, making the IfTerminal.consequent
point directly to the fallthrough block (w the return). Now we retain a separate
block.
- delete output files when we detect input files are deleted
- enable test fixtures in nested directories
- exit with error code when we detect failures
Note that the test failure on this PR is expected and will be fixed by #1608 (or
happy to abandon that PR and fold the changes)
---
Looks like we delete `.js` files but missed `.expect` files. Jest probably
didn't catch this because the basename of the fixtures had duplicates (in
rule-of-hooks)
---
(`react-forget-runtime` package seems to be synced to -.)
RFC: useRenderCounter hook:
- tracks # renders (increments on render path)
- exposes a global renderCounterRegistry (counting # renders in alive / mounted
components)
Next PR will modify BabelPlugin to add codegen
```js
// Similar to how we're currently importing `isForgetEnabled`
import {isInstrumentForgetEnabled_Secret} from "ReactForgetFeatureFlag";
// ...
function Component_uncompiled(props) {
if (isInstrumentForgetEnabled_Secret) {
useRenderCounter();
}
// ...
}
function Component_forget(props) {
if (isInstrumentForgetEnabled_Secret) {
useRenderCounter();
}
// ...
}
```
I originally created a separate test for the mode with JSX memoization disabled,
but we can merge this into the main compiler-test and enable the feature with a
pragma.
Reverts #1502, but flips test flags (e.g. `inlineUseMemo` by default, unless a
test specifies `@inlineUseMemo false`. I figured this add less thrash for test
fixtures, but happy to just do a clean revert (or remove the pragma altogether
and always pass `inlineUseMemo: true`)
Joe Savona
Sathya Gunasekaran
Mofei Zhang
mofeiZ