[compiler] Patch ValidatePreserveMemo to bailout correctly for refs #30603

mofeiZ · 2024-08-06T02:20:51Z

Stack from ghstack (oldest at bottom):

Background
ValidatePreserveExistingMemo checks that useMemos are preserved by checking that the returned result of the existing useMemo/useCallbacks either:

is never assigned a reactive scope (e.g. inferred to be a global or primitive)
has an unpruned reactive scope
We prune scopes for a few reasons. This is a bit overly conservative as there are valid reasons for pruning (see special-casing for PruneAlwaysInvalidatingScopes). Note that this bugfix actually exposes some of these. We can either special-case more pruning passes to be valid (potentially introducing PrunedScope.reason) or continue to be conservative to retain source semantics.

The bug
When inlining useMemo, we convert the useMemo to an iife and inline it, synthesizing a temporary variable for recording the iife return value.

// source
useMemo(() => foo(), ...);

// hir
$0 = Decl t$0
$1 = LoadGlobal "foo"
$2 = CallExpression $1
$3 = Reassign t$0 = $2
$4 = FinishMemoize decl=t$0  // validation marker

$2 gets assigned a reactive scope. But t$0 is never mutated after the reassignment (note that this is impossible by construction -- validatePreserveExistingMemo would fail earlier in inferReferenceEffects in that case), so it would never receive a scope. We incorrectly check t$0 (instead of $2) for scope pruning

This PR
This PR fixes a bug around ValidatePreserveUseMemo + iife inlining. For useMemos that are successfully inlined, we previously always hit case 1: assuming that useMemo return values don't need reactive scopes due to type inference.

This PR patches validatePreserveExistingMemo to check through reassignments to the FinishMemoize value. Since FinishMemoize only ever records rvalues (the result of the useMemo / useCallback call itself), this should only affect inlined useMemos.

Following the same example, we previously would check that t$0 preserves existing memo semantics. Now, we check $2 instead.

[ghstack-poisoned]

vercel · 2024-08-06T02:20:53Z

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	Aug 7, 2024 11:23pm

ghstack-source-id: 3f5d5c15bae9f6523a572fff879274559a490c38 Pull Request resolved: #30603

… for refs" --- **Background** ValidatePreserveExistingMemo checks that useMemos are preserved by checking that the returned result of the existing `useMemo`/`useCallbacks` either: 1. is never assigned a reactive scope (e.g. inferred to be a global or primitive) 2. has an *unpruned* reactive scope We prune scopes for a few reasons. This is a bit overly conservative as there are valid reasons for pruning (see special-casing for `PruneAlwaysInvalidatingScopes`). Note that this bugfix actually exposes some of these. We can either special-case more pruning passes to be valid (potentially introducing `PrunedScope.reason`) or continue to be conservative to retain source semantics. **This PR** This PR fixes a bug around ValidatePreserveUseMemo + iife inlining. For useMemos that are successfully inlined, we previously always hit case 1: assuming that useMemo return values don't need reactive scopes due to type inference. When inlining useMemo, we convert the useMemo to an iife and inline it, synthesizing a temporary variable for recording the iife return value. ```js // source useMemo(() => foo(), ...); // hir $0 = Decl t$0 $1 = LoadGlobal "foo" $2 = CallExpression $1 $3 = Reassign t$0 = $2 $4 = FinishMemoize decl=t$0 // validation marker ``` `$2` gets assigned a reactive scope. But `t$0` is never mutated after the reassignment (note that this is impossible by construction -- `validatePreserveExistingMemo` would fail earlier in inferReferenceEffects in that case), so it does not get a scope. This PR patches `validatePreserveExistingMemo` to check through reassignments to the `FinishMemoize` value. Since FinishMemoize only ever records rvalues (the result of the useMemo / useCallback call itself), this should only affect inlined useMemos. Following the same example, we previously would check that `t$0` preserves existing memo semantics. Now, we check `$2` instead. TODO: test by syncing internally [ghstack-poisoned]

ghstack-source-id: 7013d50a4eae1fdbf85a3fd996f4daa4235374c0 Pull Request resolved: #30603

mofeiZ · 2024-08-06T03:06:53Z

...rve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md

+  useHook();
+  x.push(props);
+
+  return useMemo(() => [x], [x]);


Note this bails out because we don't have the same special-casing for pruning always-invalidating scopes as we do for non-escaping scope decls.

Hopefully this is a rare case, although it makes sense to add special handling for it (more so than the logic we already have for PruneNonEscapingScopes, which arguably changes program semantics)

I think it makes sense to add special-casing for pruning of always invalidating scopes to avoid this false positive

… for refs" --- **Background** ValidatePreserveExistingMemo checks that useMemos are preserved by checking that the returned result of the existing `useMemo`/`useCallbacks` either: 1. is never assigned a reactive scope (e.g. inferred to be a global or primitive) 2. has an *unpruned* reactive scope We prune scopes for a few reasons. This is a bit overly conservative as there are valid reasons for pruning (see special-casing for `PruneAlwaysInvalidatingScopes`). Note that this bugfix actually exposes some of these. We can either special-case more pruning passes to be valid (potentially introducing `PrunedScope.reason`) or continue to be conservative to retain source semantics. **This PR** This PR fixes a bug around ValidatePreserveUseMemo + iife inlining. For useMemos that are successfully inlined, we previously always hit case 1: assuming that useMemo return values don't need reactive scopes due to type inference. When inlining useMemo, we convert the useMemo to an iife and inline it, synthesizing a temporary variable for recording the iife return value. ```js // source useMemo(() => foo(), ...); // hir $0 = Decl t$0 $1 = LoadGlobal "foo" $2 = CallExpression $1 $3 = Reassign t$0 = $2 $4 = FinishMemoize decl=t$0 // validation marker ``` `$2` gets assigned a reactive scope. But `t$0` is never mutated after the reassignment (note that this is impossible by construction -- `validatePreserveExistingMemo` would fail earlier in inferReferenceEffects in that case), so it does not get a scope. This PR patches `validatePreserveExistingMemo` to check through reassignments to the `FinishMemoize` value. Since FinishMemoize only ever records rvalues (the result of the useMemo / useCallback call itself), this should only affect inlined useMemos. Following the same example, we previously would check that `t$0` preserves existing memo semantics. Now, we check `$2` instead. TODO: test by syncing internally [ghstack-poisoned]

ghstack-source-id: 73fc47f23488b392644ffa27f124c309dc82b69c Pull Request resolved: #30603

mofeiZ · 2024-08-06T03:18:21Z

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts

+  /**
+   * Track reassignments so we can correctly set `pruned` flags for
+   * inlined useMemos.
+   */
+  reassignments: Map<DeclarationId, Set<Identifier>> = new Map();


This is the same logic (and comments) as in ValidatePreserveManualMemo. This was needed to avoid a regression of 0c86667

mofeiZ · 2024-08-06T03:21:21Z

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts

+        [...decls].every(
+          decl => decl.scope == null || this.prunedScopes.has(decl.scope.id),
+        )
      ) {
-        instruction.value.pruned = true;
+        value.pruned = true;


See newly added test fixtures for why this is needed

… for refs" --- **Background** ValidatePreserveExistingMemo checks that useMemos are preserved by checking that the returned result of the existing `useMemo`/`useCallbacks` either: 1. is never assigned a reactive scope (e.g. inferred to be a global or primitive) 2. has an *unpruned* reactive scope We prune scopes for a few reasons. This is a bit overly conservative as there are valid reasons for pruning (see special-casing for `PruneAlwaysInvalidatingScopes`). Note that this bugfix actually exposes some of these. We can either special-case more pruning passes to be valid (potentially introducing `PrunedScope.reason`) or continue to be conservative to retain source semantics. **The bug** When inlining useMemo, we convert the useMemo to an iife and inline it, synthesizing a temporary variable for recording the iife return value. ```js // source useMemo(() => foo(), ...); // hir $0 = Decl t$0 $1 = LoadGlobal "foo" $2 = CallExpression $1 $3 = Reassign t$0 = $2 $4 = FinishMemoize decl=t$0 // validation marker ``` `$2` gets assigned a reactive scope. But `t$0` is never mutated after the reassignment (note that this is impossible by construction -- `validatePreserveExistingMemo` would fail earlier in inferReferenceEffects in that case), so it would never receive a scope. We incorrectly check `t$0` (instead of `$2`) for scope pruning **This PR** This PR fixes a bug around ValidatePreserveUseMemo + iife inlining. For useMemos that are successfully inlined, we previously always hit case 1: assuming that useMemo return values don't need reactive scopes due to type inference. This PR patches `validatePreserveExistingMemo` to check through reassignments to the `FinishMemoize` value. Since FinishMemoize only ever records rvalues (the result of the useMemo / useCallback call itself), this should only affect inlined useMemos. Following the same example, we previously would check that `t$0` preserves existing memo semantics. Now, we check `$2` instead. TODO: test by syncing internally [ghstack-poisoned]

ghstack-source-id: b9c13bf5f858123b68c9e89ca8c7629cf2b90a15 Pull Request resolved: #30603

josephsavona · 2024-08-08T01:02:55Z

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts

-    if (instruction.value.kind === 'FinishMemoize') {
-      const identifier = instruction.value.decl.identifier;
+    const value = instruction.value;
+    if (value.kind === 'StoreLocal' && value.lvalue.kind === 'Reassign') {


There are other forms of reassignment, should we handle Destructure, PostfixUpdate, and PrefixUpdate too?

Ah, this is intended to only map inlined iife return values, which we replace with a reassignment to a generated + promoted temporary.

This is why the affected test cases are all useMemo related

josephsavona · 2024-08-08T01:05:16Z

...rve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md

+  useHook();
+  x.push(props);
+
+  return useMemo(() => [x], [x]);


I think it makes sense to add special-casing for pruning of always invalidating scopes to avoid this false positive

ghstack-source-id: b9c13bf5f858123b68c9e89ca8c7629cf2b90a15 Pull Request resolved: #30603

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs

2e123c2

[ghstack-poisoned]

mofeiZ added a commit that referenced this pull request Aug 6, 2024

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs

7b6be39

ghstack-source-id: 3f5d5c15bae9f6523a572fff879274559a490c38 Pull Request resolved: #30603

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Aug 6, 2024

vercel bot deployed to Preview August 6, 2024 02:25 View deployment

mofeiZ marked this pull request as ready for review August 6, 2024 03:03

mofeiZ added a commit that referenced this pull request Aug 6, 2024

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs

8174f44

ghstack-source-id: 7013d50a4eae1fdbf85a3fd996f4daa4235374c0 Pull Request resolved: #30603

vercel bot had a problem deploying to Preview August 6, 2024 03:05 Failure

mofeiZ commented Aug 6, 2024

View reviewed changes

mofeiZ added a commit that referenced this pull request Aug 6, 2024

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs

405e79e

ghstack-source-id: 73fc47f23488b392644ffa27f124c309dc82b69c Pull Request resolved: #30603

mofeiZ commented Aug 6, 2024

View reviewed changes

vercel bot deployed to Preview August 6, 2024 03:19 View deployment

mofeiZ commented Aug 6, 2024

View reviewed changes

This was referenced Aug 7, 2024

[compiler][repro] False positives for ValidatePreserveMemo #30629

Merged

[compiler] Remove transitive memo check in validatePreserveMemo #30630

Merged

mofeiZ added a commit that referenced this pull request Aug 7, 2024

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs

c454c3b

ghstack-source-id: b9c13bf5f858123b68c9e89ca8c7629cf2b90a15 Pull Request resolved: #30603

vercel bot deployed to Preview August 7, 2024 23:23 View deployment

mvitousek approved these changes Aug 8, 2024

View reviewed changes

josephsavona reviewed Aug 8, 2024

View reviewed changes

josephsavona approved these changes Aug 8, 2024

View reviewed changes

mofeiZ merged commit 9a259b2 into gh/mofeiZ/16/base Aug 8, 2024
19 checks passed

mofeiZ added a commit that referenced this pull request Aug 8, 2024

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs

a037dab

ghstack-source-id: b9c13bf5f858123b68c9e89ca8c7629cf2b90a15 Pull Request resolved: #30603

mofeiZ deleted the gh/mofeiZ/16/head branch August 8, 2024 01:58

eps1lon mentioned this pull request Aug 14, 2024

Update React from 187dd6a7-20240806 to 49496d49-20240814 vercel/next.js#68898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs #30603

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs #30603

mofeiZ commented Aug 6, 2024 •

edited

Loading

vercel bot commented Aug 6, 2024 •

edited

Loading

mofeiZ Aug 6, 2024 •

edited

Loading

josephsavona Aug 8, 2024

mofeiZ Aug 6, 2024 •

edited

Loading

mofeiZ Aug 6, 2024

josephsavona Aug 8, 2024

mofeiZ Aug 8, 2024

josephsavona Aug 8, 2024

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs #30603

[compiler] Patch ValidatePreserveMemo to bailout correctly for refs #30603

Conversation

mofeiZ commented Aug 6, 2024 • edited Loading

vercel bot commented Aug 6, 2024 • edited Loading

mofeiZ Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

josephsavona Aug 8, 2024

Choose a reason for hiding this comment

mofeiZ Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

mofeiZ Aug 6, 2024

Choose a reason for hiding this comment

josephsavona Aug 8, 2024

Choose a reason for hiding this comment

mofeiZ Aug 8, 2024

Choose a reason for hiding this comment

josephsavona Aug 8, 2024

Choose a reason for hiding this comment

mofeiZ commented Aug 6, 2024 •

edited

Loading

vercel bot commented Aug 6, 2024 •

edited

Loading

mofeiZ Aug 6, 2024 •

edited

Loading

mofeiZ Aug 6, 2024 •

edited

Loading