Repository: facebook/react
Pull request: [compiler] Stop relying on identifier mutable ranges after constructing scopes

Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom):
* __->__ #30428
* #30431
* #30399
* #30398
* #30397

Addresses discussion at https://github.com/facebook/react/pull/30399#discussion_r1684693021. Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter.

The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged.

We already have fixtures that test for these edge cases:

## preserve-memo-validation/useMemo-dropped-infer-always-invalidating.ts

This case passes the validation because the way Forget constructs the scopes matches what the user had: there is an unmemoized dependency (x, whose scope is pruned bc it has a hook call) whose range ends before the useMemo, and the useMemo is preserved:

```js
const x = [];
useHook();
x.push(props);

return useMemo(() => [x], [x]);
```

Concretely: there is a `StartMemoize deps=x@0`, but scope `@0` is pruned so it's allowed.

## preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.ts

This case fails the validation because Forget constructs the scopes differently than the user code: the construction of `val` is part of the scope for `identity(val)` since Forget infers that `val` may be mutated there. So rather than `val` appearing as an unmemoized dependency of the memo block, it becomes part of the memo block. So we correctly reject it.

```js
const val = [1, 2, 3];

return useMemo(() => {
  return identity(val);
}, [val]);
```

Concretely, there is a `StartMemoize deps=val@0`, but the StartMemoize instruction is within scope `@0`, which hasn't completed yet, so it can't be a valid dependency and we reject it.

# Sync Results

I synced this stack:
* Minimal changes to compilation output: P1491221074. The pattern is consistent: we previously had false positives where we reported that values that came from pruned scopes (due to hooks) were "unmemoized", whereas the new logic accounts for this with the pruned scope check. See the fixture added in the previous PR in the stack, and how it becomes compiled here.
* No changes to lint output


Changed files:
filename: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.expect.md
patch: @@ -54,6 +54,4 @@ function Component_withRef(t0, ref) {
 }
 
 ```
-      
-### Eval output
-(kind: exception) Fixture not implemented
\ No newline at end of file
+      
\ No newline at end of file

filename: compiler/packages/snap/src/SproutTodoFilter.ts
patch: @@ -398,7 +398,7 @@ const skipFilter = new Set([
   'deeply-nested-function-expressions-with-params',
   'readonly-object-method-calls',
   'readonly-object-method-calls-mutable-lambda',
-  'useMemo-with-refs.flow',
+  'preserve-memo-validation/useMemo-with-refs.flow',
 
   // TODO: we probably want to always skip these
   'rules-of-hooks/rules-of-hooks-0592bd574811',