Skip to content

Commit 193c9f1

Browse files
committed
Update on "[compiler] Stop relying on identifier mutable ranges after constructing scopes"
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: ```js const x = []; useHook(); x.push(props); return useMemo(() => [x], [x]); ``` Concretely: there is a `StartMemoize deps=x0`, 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=val0`, 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 [ghstack-poisoned]
2 parents ddaa39f + ee33eb1 commit 193c9f1

File tree

2 files changed

+2
-4
lines changed

2 files changed

+2
-4
lines changed

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.expect.md

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,4 @@ function Component_withRef(t0, ref) {
5454
}
5555

5656
```
57-
58-
### Eval output
59-
(kind: exception) Fixture not implemented
57+

compiler/packages/snap/src/SproutTodoFilter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -398,7 +398,7 @@ const skipFilter = new Set([
398398
'deeply-nested-function-expressions-with-params',
399399
'readonly-object-method-calls',
400400
'readonly-object-method-calls-mutable-lambda',
401-
'useMemo-with-refs.flow',
401+
'preserve-memo-validation/useMemo-with-refs.flow',
402402

403403
// TODO: we probably want to always skip these
404404
'rules-of-hooks/rules-of-hooks-0592bd574811',

0 commit comments

Comments
 (0)