[compiler] Stop relying on identifier mutable ranges after constructing scopes

[josephsavona]

Stack from ghstack (oldest at bottom):

• -> [compiler] Stop relying on identifier mutable ranges after constructing scopes #30428
• [compiler] repro of false positive for ValidatePreserveManualMemo #30431
• [compiler] Maintain RPO and unique instruction ids when constructing scope terminals #30399
• [compiler] Flatten scopes based on fallthrough, not scope range #30398
• [compiler] printTerminal always prints instruction id #30397

Addresses discussion at #30399 (comment). 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:

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.

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

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 24, 2024 0:16am

[mofeiZ]

Makes sense, thanks for the detailed writeup + testing!