Skip to content

Commit

Permalink
Fix false positive on preserving memo of non-escaping values
Browse files Browse the repository at this point in the history
Fixes the false positive in the previous PR. When we prune a scope because it's 
values are non-escaping, we now also remove any `Memoize` instructions for that 
scope. The intuition being that we're actively removing unnecessary memoization, 
so we don't need to check that the memoization occurred anymore.
  • Loading branch information
josephsavona committed Jan 12, 2024
1 parent 8e4d2fb commit 0c86667
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,8 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor<State> {
class PruneScopesTransform extends ReactiveFunctionTransform<
Set<IdentifierId>
> {
prunedScopes: Set<ScopeId> = new Set();

override transformScope(
scopeBlock: ReactiveScopeBlock,
state: Set<IdentifierId>
Expand Down Expand Up @@ -900,7 +902,32 @@ class PruneScopesTransform extends ReactiveFunctionTransform<
if (hasMemoizedOutput) {
return { kind: "keep" };
} else {
this.prunedScopes.add(scopeBlock.scope.id);
return { kind: "replace-many", value: scopeBlock.instructions };
}
}

override transformInstruction(
instruction: ReactiveInstruction,
state: Set<IdentifierId>
): Transformed<ReactiveStatement> {
this.traverseInstruction(instruction, state);

/**
* If we pruned the scope for a non-escaping value, we know it doesn't
* need to be memoized. Remove associated `Memoize` instructions so that
* we don't report false positives on "missing" memoization of these values.
*/
if (instruction.value.kind === "Memoize") {
const identifier = instruction.value.value.identifier;
if (
identifier.scope !== null &&
this.prunedScopes.has(identifier.scope.id)
) {
return { kind: "remove" };
}
}

return { kind: "keep" };
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@

## Input

```javascript
// @validatePreserveExistingMemoizationGuarantees @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
import { useCallback } from "react";

function Component({ entity, children }) {
// showMessage doesn't escape so we don't memoize it.
// However, validatePreserveExistingMemoizationGuarantees only sees that the scope
// doesn't exist, and thinks the memoization was missed instead of being intentionally dropped.
const showMessage = useCallback(() => entity != null);

if (!showMessage()) {
return children;
}

return <div>{children}</div>;
}

export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [
{
entity: { name: "Sathya" },
children: [<div key="gsathya">Hi Sathya!</div>],
},
],
};

```

## Code

```javascript
// @validatePreserveExistingMemoizationGuarantees @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
import { useCallback, unstable_useMemoCache as useMemoCache } from "react";

function Component(t23) {
const $ = useMemoCache(2);
const { entity, children } = t23;

const showMessage = () => entity != null;
if (!showMessage()) {
return children;
}
let t0;
if ($[0] !== children) {
t0 = <div>{children}</div>;
$[0] = children;
$[1] = t0;
} else {
t0 = $[1];
}
return t0;
}

export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [
{
entity: { name: "Sathya" },
children: [<div key="gsathya">Hi Sathya!</div>],
},
],
};

```
### Eval output
(kind: ok) <div><div>Hi Sathya!</div></div>
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
// @validatePreserveExistingMemoizationGuarantees @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
import { useCallback } from "react";

function Component({ entity, children }) {
// showMessage doesn't escape so we don't memoize it.
// However, validatePreserveExistingMemoizationGuarantees only sees that the scope
// doesn't exist, and thinks the memoization was missed instead of being intentionally dropped.
const showMessage = useCallback(() => entity != null);

if (!showMessage) {
if (!showMessage()) {
return children;
}

return <Message>{children}</Message>;
return <div>{children}</div>;
}

export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [
{
entity: { name: "Sathya" },
children: [<div key="gsathya">Hi Sathya!</div>],
},
],
};

0 comments on commit 0c86667

Please sign in to comment.