Skip to content

[compiler] Fix mutable ranges for StoreContext #33386

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

Closed
wants to merge 15 commits into from
Closed
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
Expand Up @@ -15,6 +15,7 @@ import {
Hole,
IdentifierId,
Instruction,
InstructionKind,
InstructionValue,
isArrayType,
isMapType,
Expand Down Expand Up @@ -768,7 +769,7 @@ function applyEffect(
}
}

const DEBUG = true;
const DEBUG = false;

class InferenceState {
env: Environment;
Expand Down Expand Up @@ -1452,7 +1453,21 @@ function computeSignatureForInstruction(
}
break;
}
case 'DeclareContext':
case 'DeclareContext': {
// Context variables are conceptually like mutable boxes
effects.push({
kind: 'Create',
into: value.lvalue.place,
value: ValueKind.Mutable,
});
effects.push({
kind: 'Create',
into: lvalue,
// The result can't be referenced so this value doesn't matter
value: ValueKind.Primitive,
});
break;
}
case 'DeclareLocal': {
// TODO check this
effects.push({
Expand Down Expand Up @@ -1481,13 +1496,25 @@ function computeSignatureForInstruction(
break;
}
case 'LoadContext': {
// We load from the context
effects.push({kind: 'Assign', from: value.place, into: lvalue});
break;
}
case 'StoreContext': {
effects.push({kind: 'Mutate', value: value.lvalue.place});
// We're storing into the conceptual box
if (value.lvalue.kind === InstructionKind.Reassign) {
effects.push({kind: 'Mutate', value: value.lvalue.place});
} else {
// Context variables are conceptually like mutable boxes
effects.push({
kind: 'Create',
into: value.lvalue.place,
value: ValueKind.Mutable,
});
}
// Which aliases the value
effects.push({
kind: 'Assign',
kind: 'Alias',
from: value.value,
into: value.lvalue.place,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
} from '../HIR/visitors';
import DisjointSet from '../Utils/DisjointSet';
import {assertExhaustive} from '../Utils/utils';
import {debugAliases} from './InferMutableRanges';

Check failure on line 23 in compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts

View workflow job for this annotation

GitHub Actions / Lint babel-plugin-react-compiler

'debugAliases' is defined but never used. Allowed unused vars must match /^_/u
import {inferMutableRangesForAlias} from './InferMutableRangesForAlias';

/**
Expand All @@ -44,7 +45,9 @@

for (const instr of block.instructions) {
for (const lvalue of eachInstructionLValue(instr)) {
lvalue.identifier.mutableRange.start = instr.id;
if (lvalue.identifier.mutableRange.start === 0) {
lvalue.identifier.mutableRange.start = instr.id;
}
lvalue.identifier.mutableRange.end = makeInstructionId(
Math.max(instr.id + 1, lvalue.identifier.mutableRange.end),
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@

## Input

```javascript
// @validatePreserveExistingMemoizationGuarantees @enableNewMutationAliasingModel
import {useCallback} from 'react';
import {makeArray} from 'shared-runtime';

// This case is already unsound in source, so we can safely bailout
function Foo(props) {
let x = [];
x.push(props);

// makeArray() is captured, but depsList contains [props]
const cb = useCallback(() => [x], [x]);

x = makeArray();

return cb;
}
export const FIXTURE_ENTRYPOINT = {
fn: Foo,
params: [{}],
};

```


## Error

```
9 |
10 | // makeArray() is captured, but depsList contains [props]
> 11 | const cb = useCallback(() => [x], [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 (11:11)

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:11)
12 |
13 | x = makeArray();
14 |
```


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

// This case is already unsound in source, so we can safely bailout
function Foo(props) {
let x = [];
x.push(props);

// makeArray() is captured, but depsList contains [props]
const cb = useCallback(() => [x], [x]);

x = makeArray();

return cb;
}
export const FIXTURE_ENTRYPOINT = {
fn: Foo,
params: [{}],
};
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,28 @@ function ReactiveRefInEffect(props) {
```javascript
import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel
function ReactiveRefInEffect(props) {
const $ = _c(2);
const $ = _c(4);
const ref1 = useRef("initial value");
const ref2 = useRef("initial value");
let ref;
if (props.foo) {
ref = ref1;
if ($[0] !== props.foo) {
if (props.foo) {
ref = ref1;
} else {
ref = ref2;
}
$[0] = props.foo;
$[1] = ref;
} else {
ref = ref2;
ref = $[1];
}
let t0;
if ($[0] !== ref) {
if ($[2] !== ref) {
t0 = () => print(ref);
$[0] = ref;
$[1] = t0;
$[2] = ref;
$[3] = t0;
} else {
t0 = $[1];
t0 = $[3];
}
useEffect(t0);
}
Expand Down
Loading