Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@

## Input

```javascript
// @validatePreserveExistingMemoizationGuarantees:true

import {useCallback} from 'react';
import {Stringify, useIdentity} from 'shared-runtime';

/**
* Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`.
* - t1 does not have a scope as it captures `x` after x's mutable range
* - `x` is a context variable, which means its mutable range extends to all
* references / aliases.
* - `a`, `b`, and `x` get the same mutable range due to potential aliasing.
*
* We currently bail out because `a` has a scope and is not transitively memoized
* (as its scope is pruned due to a hook call)
*/
function useBar({a, b}, cond) {
let x = useIdentity({val: 3});
if (cond) {
x = b;
}

const cb = useCallback(() => {
return [a, x];
}, [a, x]);

return <Stringify cb={cb} shouldInvoke={true} />;
}

export const FIXTURE_ENTRYPOINT = {
fn: useBar,
params: [{a: 1, b: 2}, true],
};

```


## Error

```
20 | }
21 |
> 22 | const cb = useCallback(() => {
| ^^^^^^^
> 23 | return [a, x];
| ^^^^^^^^^^^^^^^^^^
> 24 | }, [a, x]);
| ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. (22:24)
25 |
26 | return <Stringify cb={cb} shouldInvoke={true} />;
27 | }
```


Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// @validatePreserveExistingMemoizationGuarantees:true

import {useCallback} from 'react';
import {Stringify, useIdentity} from 'shared-runtime';

/**
* Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`.
* - t1 does not have a scope as it captures `x` after x's mutable range
* - `x` is a context variable, which means its mutable range extends to all
* references / aliases.
* - `a`, `b`, and `x` get the same mutable range due to potential aliasing.
*
* We currently bail out because `a` has a scope and is not transitively memoized
* (as its scope is pruned due to a hook call)
Comment on lines +13 to +14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting - looks like we are grouping the argument destructuring into a scope? that seems wrong, a and b aren't mutable so they shouldn't be getting added into any scopes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this is because x is a context variable, and since our understanding of mutable context variables is imprecise, x and all potential aliases get marked as mutable. I might be incorrect though -- can look into this more

*/
function useBar({a, b}, cond) {
let x = useIdentity({val: 3});
if (cond) {
x = b;
}

const cb = useCallback(() => {
return [a, x];
}, [a, x]);

return <Stringify cb={cb} shouldInvoke={true} />;
}

export const FIXTURE_ENTRYPOINT = {
fn: useBar,
params: [{a: 1, b: 2}, true],
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@

## Input

```javascript
// @validatePreserveExistingMemoizationGuarantees
import {useCallback} from 'react';
import {identity, useIdentity} from 'shared-runtime';

/**
* Repro showing a manual memo whose declaration (useCallback's 1st argument)
* is memoized, but not its dependency (x). In this case, `x`'s scope is pruned
* due to hook-call flattening.
*/
function useFoo(a) {
const x = identity(a);
useIdentity(2);
mutate(x);

return useCallback(() => [x, []], [x]);
}

export const FIXTURE_ENTRYPOINT = {
fn: useFoo,
params: [3],
};

```


## Error

```
13 | mutate(x);
14 |
> 15 | return useCallback(() => [x, []], [x]);
| ^^^^^^^^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. (15:15)
16 | }
17 |
18 | export const FIXTURE_ENTRYPOINT = {
```


Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// @validatePreserveExistingMemoizationGuarantees
import {useCallback} from 'react';
import {identity, useIdentity} from 'shared-runtime';

/**
* Repro showing a manual memo whose declaration (useCallback's 1st argument)
* is memoized, but not its dependency (x). In this case, `x`'s scope is pruned
* due to hook-call flattening.
*/
function useFoo(a) {
const x = identity(a);
useIdentity(2);
mutate(x);

return useCallback(() => [x, []], [x]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a false positive because the input wasn't memoized in source, right? Just making sure i'm interpreting this the same way you are.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep exactly! I added this fixture to show changes from #30630

}

export const FIXTURE_ENTRYPOINT = {
fn: useFoo,
params: [3],
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@

## Input

```javascript
// @validatePreserveExistingMemoizationGuarantees:true
import {useMemo} from 'react';
import {arrayPush} from 'shared-runtime';

/**
* Repro showing differences between mutable ranges and scope ranges.
*
* For useMemo dependency `x`:
* - mutable range ends after the `arrayPush(x, b)` instruction
* - scope range is extended due to MergeOverlappingScopes
*
* Since manual memo deps are guaranteed to be named (guaranteeing valid
* codegen), it's correct to take a dependency on a dep *before* the end
* of its scope (but after its mutable range ends).
*/

function useFoo(a, b) {
const x = [];
const y = [];
arrayPush(x, b);
const result = useMemo(() => {
return [Math.max(x[1], a)];
}, [a, x]);
arrayPush(y, 3);
return {result, y};
}

export const FIXTURE_ENTRYPOINT = {
fn: useFoo,
params: [1, 2],
};

```


## Error

```
21 | const result = useMemo(() => {
22 | return [Math.max(x[1], a)];
> 23 | }, [a, x]);
| ^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly (23:23)
24 | arrayPush(y, 3);
25 | return {result, y};
26 | }
```


Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// @validatePreserveExistingMemoizationGuarantees:true
import {useMemo} from 'react';
import {arrayPush} from 'shared-runtime';

/**
* Repro showing differences between mutable ranges and scope ranges.
*
* For useMemo dependency `x`:
* - mutable range ends after the `arrayPush(x, b)` instruction
* - scope range is extended due to MergeOverlappingScopes
*
* Since manual memo deps are guaranteed to be named (guaranteeing valid
* codegen), it's correct to take a dependency on a dep *before* the end
* of its scope (but after its mutable range ends).
*/

function useFoo(a, b) {
Copy link
Contributor Author

@mofeiZ mofeiZ Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only false positive not fixed by #30630 and unrelated to #30603. Included since it shows a use-case for an identifier's original mutable range (instead of the currently used scope range).

Currently, forget (incorrectly) infers that x is potentially mutated after StartMemoize deps=[x, ...]

const x = [];
const y = [];
arrayPush(x, b);
const result = useMemo(() => {
return [Math.max(x[1], a)];
}, [a, x]);
arrayPush(y, 3);
return {result, y};
}

export const FIXTURE_ENTRYPOINT = {
fn: useFoo,
params: [1, 2],
};