Skip to content

[compiler] Transitively freezing functions marks values as frozen, not effects #30766

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.

Sign up for GitHub

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

Merged
josephsavona merged 2 commits into from
Aug 22, 2024
Merged

[compiler] Transitively freezing functions marks values as frozen, not effects #30766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28ee171
[compiler] Transitively freezing functions marks values as frozen, no…
josephsavona Aug 21, 2024
517951a
Update on "[compiler] Transitively freezing functions marks values as…
josephsavona Aug 21, 2024
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
59 changes: 36 additions & 23 deletions compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,37 @@ class InferenceState {
}
}

freezeValues(values: Set<InstructionValue>, reason: Set<ValueReason>): void {
for (const value of values) {
this.#values.set(value, {
kind: ValueKind.Frozen,
reason,
context: new Set(),
});
if (value.kind === 'FunctionExpression') {
if (
this.#env.config.enablePreserveExistingMemoizationGuarantees ||
this.#env.config.enableTransitivelyFreezeFunctionExpressions
) {
if (value.kind === 'FunctionExpression') {
/*
* We want to freeze the captured values, not mark the operands
* themselves as frozen. There could be mutations that occur
* before the freeze we are processing, and it would be invalid
* to overwrite those mutations as a freeze.
*/
for (const operand of eachInstructionValueOperand(value)) {
const operandValues = this.#variables.get(operand.identifier.id);
if (operandValues !== undefined) {
this.freezeValues(operandValues, reason);
}
}
}
}
}
}
}

reference(
place: Place,
effectKind: Effect,
Expand Down Expand Up @@ -482,29 +513,7 @@ class InferenceState {
reason: reasonSet,
context: new Set(),
};
values.forEach(value => {
this.#values.set(value, {
kind: ValueKind.Frozen,
reason: reasonSet,
context: new Set(),
});

if (
this.#env.config.enablePreserveExistingMemoizationGuarantees ||
this.#env.config.enableTransitivelyFreezeFunctionExpressions
) {
if (value.kind === 'FunctionExpression') {
for (const operand of eachInstructionValueOperand(value)) {
this.referenceAndRecordEffects(
operand,
Effect.Freeze,
ValueReason.Other,
[],
);
}
}
}
});
this.freezeValues(values, reasonSet);
} else {
effect = Effect.Read;
}
Expand Down Expand Up @@ -1241,13 +1250,17 @@ function inferBlock(
case 'ObjectMethod':
case 'FunctionExpression': {
let hasMutableOperand = false;
const mutableOperands: Array<Place> = [];
for (const operand of eachInstructionOperand(instr)) {
state.referenceAndRecordEffects(
operand,
operand.effect === Effect.Unknown ? Effect.Read : operand.effect,
ValueReason.Other,
[],
);
if (isMutableEffect(operand.effect, operand.loc)) {
mutableOperands.push(operand);
}
hasMutableOperand ||= isMutableEffect(operand.effect, operand.loc);

/**
Expand Down
53 changes: 28 additions & 25 deletions ...rc/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,54 +46,57 @@ import { useEffect, useState } from "react";
let someGlobal = { value: null };

function Component() {
const $ = _c(6);
const $ = _c(7);
const [state, setState] = useState(someGlobal);

let x = someGlobal;
while (x == null) {
x = someGlobal;
}

const y = x;
let t0;
let t1;
let t2;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = () => {
let x = someGlobal;
while (x == null) {
x = someGlobal;
}

const y = x;
t0 = useEffect;
t1 = () => {
y.value = "hello";
};
t1 = [];
t2 = [];
$[0] = t0;
$[1] = t1;
$[2] = t2;
} else {
t0 = $[0];
t1 = $[1];
t2 = $[2];
}
useEffect(t0, t1);
let t2;
t0(t1, t2);
let t3;
if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
t2 = () => {
let t4;
if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
t3 = () => {
setState(someGlobal.value);
};
t3 = [someGlobal];
$[2] = t2;
t4 = [someGlobal];
$[3] = t3;
$[4] = t4;
} else {
t2 = $[2];
t3 = $[3];
t4 = $[4];
}
useEffect(t2, t3);
useEffect(t3, t4);

const t4 = String(state);
let t5;
if ($[4] !== t4) {
t5 = <div>{t4}</div>;
$[4] = t4;
const t5 = String(state);
let t6;
if ($[5] !== t5) {
t6 = <div>{t5}</div>;
$[5] = t5;
$[6] = t6;
} else {
t5 = $[5];
t6 = $[6];
}
return t5;
return t6;
}

export const FIXTURE_ENTRYPOINT = {
Expand Down
51 changes: 23 additions & 28 deletions .../error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,14 @@ import LogEvent from 'LogEvent';
import {useCallback, useMemo} from 'react';

component Component(id) {
const {data} = useFragment();
const items = data.items.edges;
const items = useFragment();

const [prevId, setPrevId] = useState(id);
const [index, setIndex] = useState(0);

const logData = useMemo(() => {
const item = items[index];
return {
key: item.key ?? '',
key: item.key,
};
}, [index, items]);

Expand All @@ -35,7 +33,6 @@ component Component(id) {
);

if (prevId !== id) {
setPrevId(id);
setCurrentIndex(0);
}

Expand All @@ -55,29 +52,27 @@ component Component(id) {
## Error

```
19 |
20 | const setCurrentIndex = useCallback(
> 21 | (index: number) => {
| ^^^^^^^^^^^^^^^^^^^^
> 22 | const object = {
| ^^^^^^^^^^^^^^^^^^^^^^
> 23 | tracking: logData.key,
| ^^^^^^^^^^^^^^^^^^^^^^
> 24 | };
| ^^^^^^^^^^^^^^^^^^^^^^
> 25 | // We infer that this may mutate `object`, which in turn aliases
| ^^^^^^^^^^^^^^^^^^^^^^
> 26 | // data from `logData`, such that `logData` may be mutated.
| ^^^^^^^^^^^^^^^^^^^^^^
> 27 | LogEvent.log(() => object);
| ^^^^^^^^^^^^^^^^^^^^^^
> 28 | setIndex(index);
| ^^^^^^^^^^^^^^^^^^^^^^
> 29 | },
| ^^^^^^ 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. (21:29)
30 | [index, logData, items]
31 | );
32 |
9 | const [index, setIndex] = useState(0);
10 |
> 11 | const logData = useMemo(() => {
| ^^^^^^^^^^^^^^^
> 12 | const item = items[index];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 13 | return {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 14 | key: item.key,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 15 | };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 16 | }, [index, items]);
| ^^^^^^^^^^^^^^^^^^^^^ 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. (11:16)

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 (28:28)

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. (19:27)
17 |
18 | const setCurrentIndex = useCallback(
19 | (index: number) => {
```


7 changes: 2 additions & 5 deletions ...fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,14 @@ import LogEvent from 'LogEvent';
import {useCallback, useMemo} from 'react';

component Component(id) {
const {data} = useFragment();
const items = data.items.edges;
const items = useFragment();

const [prevId, setPrevId] = useState(id);
const [index, setIndex] = useState(0);

const logData = useMemo(() => {
const item = items[index];
return {
key: item.key ?? '',
key: item.key,
};
}, [index, items]);

Expand All @@ -31,7 +29,6 @@ component Component(id) {
);

if (prevId !== id) {
setPrevId(id);
setCurrentIndex(0);
}

Expand Down