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] Exclude refs and ref values from having mutable ranges #30713

Merged
merged 5 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -11,6 +11,8 @@ import {
Identifier,
InstructionId,
InstructionKind,
isRefValueType,
isUseRefType,
makeInstructionId,
Place,
} from '../HIR/HIR';
Expand Down Expand Up @@ -66,7 +68,9 @@ import {assertExhaustive} from '../Utils/utils';
*/

function infer(place: Place, instrId: InstructionId): void {
place.identifier.mutableRange.end = makeInstructionId(instrId + 1);
if (!isUseRefType(place.identifier) && !isRefValueType(place.identifier)) {
place.identifier.mutableRange.end = makeInstructionId(instrId + 1);
}
}

function inferPlace(
Expand Down Expand Up @@ -171,7 +175,11 @@ export function inferMutableLifetimes(
const declaration = contextVariableDeclarationInstructions.get(
instr.value.lvalue.place.identifier,
);
if (declaration != null) {
if (
declaration != null &&
!isRefValueType(instr.value.lvalue.place.identifier) &&
!isUseRefType(instr.value.lvalue.place.identifier)
) {
const range = instr.value.lvalue.place.identifier.mutableRange;
if (range.start === 0) {
range.start = declaration;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@
* LICENSE file in the root directory of this source tree.
*/

import {HIRFunction, Identifier, InstructionId} from '../HIR/HIR';
import {
HIRFunction,
Identifier,
InstructionId,
isRefValueType,
isUseRefType,
} from '../HIR/HIR';
import DisjointSet from '../Utils/DisjointSet';

export function inferMutableRangesForAlias(
Expand All @@ -19,7 +25,10 @@ export function inferMutableRangesForAlias(
* mutated.
*/
const mutatingIdentifiers = [...aliasSet].filter(
id => id.mutableRange.end - id.mutableRange.start > 1,
id =>
id.mutableRange.end - id.mutableRange.start > 1 &&
!isUseRefType(id) &&
!isRefValueType(id),
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a helper for this combination?

);

if (mutatingIdentifiers.length > 0) {
Expand All @@ -36,7 +45,11 @@ export function inferMutableRangesForAlias(
* last mutation.
*/
for (const alias of aliasSet) {
if (alias.mutableRange.end < lastMutatingInstructionId) {
if (
alias.mutableRange.end < lastMutatingInstructionId &&
!isUseRefType(alias) &&
!isRefValueType(alias)
) {
alias.mutableRange.end = lastMutatingInstructionId as InstructionId;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,27 +36,26 @@ import { useRef } from "react";
import { addOne } from "shared-runtime";

function useKeyCommand() {
const $ = _c(2);
const $ = _c(1);
const currentPosition = useRef(0);
const handleKey = (direction) => () => {
const position = currentPosition.current;
const nextPosition = direction === "left" ? addOne(position) : position;
currentPosition.current = nextPosition;
};
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
const handleKey = (direction) => () => {
const position = currentPosition.current;
const nextPosition = direction === "left" ? addOne(position) : position;
currentPosition.current = nextPosition;
};

const moveLeft = { handler: handleKey("left") };
const moveLeft = { handler: handleKey("left") };

const t0 = handleKey("right");
let t1;
if ($[0] !== t0) {
t1 = { handler: t0 };
const moveRight = { handler: handleKey("right") };

t0 = [moveLeft, moveRight];
$[0] = t0;
$[1] = t1;
} else {
t1 = $[1];
t0 = $[0];
}
const moveRight = t1;
return [moveLeft, moveRight];
return t0;
}

export const FIXTURE_ENTRYPOINT = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
## Input

```javascript
// @enablePreserveExistingMemoizationGuarantees
// @enablePreserveExistingMemoizationGuarantees @validateRefAccessDuringRender
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to need to enable @validateRefAccessDuringRender by default to make this change safe. Obviously if you're using React Compiler with Flow, then Flow would catch the ref accesses, but we need to ensure this is just as safe without Flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see you did this later in the stack, sweet

import {useCallback, useRef} from 'react';

function Component(props) {
Expand Down Expand Up @@ -42,7 +42,9 @@ export const FIXTURE_ENTRYPOINT = {
> 10 | ref.current.inner = event.target.value;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 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. (7:11)
| ^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $44:TObject<BuiltInFunction> (7:11)

InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (14:14)
12 |
13 | // The ref is modified later, extending its range and preventing memoization of onChange
14 | ref.current.inner = null;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @enablePreserveExistingMemoizationGuarantees
// @enablePreserveExistingMemoizationGuarantees @validateRefAccessDuringRender
import {useCallback, useRef} from 'react';

function Component(props) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
## Input

```javascript
// @enablePreserveExistingMemoizationGuarantees
// @enablePreserveExistingMemoizationGuarantees @validateRefAccessDuringRender
import {useCallback, useRef} from 'react';

function Component(props) {
Expand Down Expand Up @@ -45,7 +45,11 @@ export const FIXTURE_ENTRYPOINT = {
> 10 | ref.current.inner = event.target.value;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 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. (7:11)
| ^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $53:TObject<BuiltInFunction> (7:11)

InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef). Function mutate? $77[20:22]:TObject<BuiltInFunction> accesses a ref (17:17)

InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (17:17)
12 |
13 | // The ref is modified later, extending its range and preventing memoization of onChange
14 | const reset = () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @enablePreserveExistingMemoizationGuarantees
// @enablePreserveExistingMemoizationGuarantees @validateRefAccessDuringRender
import {useCallback, useRef} from 'react';

function Component(props) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,42 +37,26 @@ import { useRef } from "react";
import { addOne } from "shared-runtime";

function useKeyCommand() {
const $ = _c(6);
const $ = _c(1);
const currentPosition = useRef(0);
const handleKey = (direction) => () => {
const position = currentPosition.current;
const nextPosition = direction === "left" ? addOne(position) : position;
currentPosition.current = nextPosition;
};
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = { handler: handleKey("left") };
const handleKey = (direction) => () => {
const position = currentPosition.current;
const nextPosition = direction === "left" ? addOne(position) : position;
currentPosition.current = nextPosition;
};

const moveLeft = { handler: handleKey("left") };

const moveRight = { handler: handleKey("right") };

t0 = [moveLeft, moveRight];
$[0] = t0;
} else {
t0 = $[0];
}
const moveLeft = t0;

const t1 = handleKey("right");
let t2;
if ($[1] !== t1) {
t2 = { handler: t1 };
$[1] = t1;
$[2] = t2;
} else {
t2 = $[2];
}
const moveRight = t2;
let t3;
if ($[3] !== moveLeft || $[4] !== moveRight) {
t3 = [moveLeft, moveRight];
$[3] = moveLeft;
$[4] = moveRight;
$[5] = t3;
} else {
t3 = $[5];
}
return t3;
return t0;
}

export const FIXTURE_ENTRYPOINT = {
Expand Down

This file was deleted.

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

## Input

```javascript
// @validatePreserveExistingMemoizationGuarantees:true

import {useRef, useMemo} from 'react';
import {makeArray} from 'shared-runtime';

function useFoo() {
const r = useRef();
return useMemo(() => makeArray(r), []);
}

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

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees:true

import { useRef, useMemo } from "react";
import { makeArray } from "shared-runtime";

function useFoo() {
const $ = _c(1);
const r = useRef();
let t0;
let t1;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t1 = makeArray(r);
$[0] = t1;
} else {
t1 = $[0];
}
t0 = t1;
return t0;
}

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

```

### Eval output
(kind: ok) [{}]
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import { c as _c } from "react/compiler-runtime";
import { Stringify, identity, mutate, CONST_TRUE } from "shared-runtime";

function Foo(props, ref) {
const $ = _c(5);
const $ = _c(7);
let value;
let t0;
if ($[0] !== ref) {
Expand All @@ -45,19 +45,6 @@ function Foo(props, ref) {
}

mutate(value);
if (CONST_TRUE) {
const t1 = identity(ref);
let t2;
if ($[3] !== t1) {
t2 = <Stringify ref={t1} />;
$[3] = t1;
$[4] = t2;
} else {
t2 = $[4];
}
t0 = t2;
break bb0;
}
}
$[0] = ref;
$[1] = value;
Expand All @@ -69,6 +56,25 @@ function Foo(props, ref) {
if (t0 !== Symbol.for("react.early_return_sentinel")) {
return t0;
}
if (CONST_TRUE) {
let t1;
if ($[3] !== ref) {
t1 = identity(ref);
$[3] = ref;
$[4] = t1;
} else {
t1 = $[4];
}
let t2;
if ($[5] !== t1) {
t2 = <Stringify ref={t1} />;
$[5] = t1;
$[6] = t2;
} else {
t2 = $[6];
}
return t2;
}
return value;
}

Expand Down
Loading