Skip to content

Commit

Permalink
compiler: Add support for ref effects
Browse files Browse the repository at this point in the history
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability.

The fix is to add a new RefMutation variant to FunctionEffect, which we emit whenever a ref is mutated. We disallow this effect type from within a function body, but allow it within function expressions that are passed to useEffect and friends as well as to jsx.

ghstack-source-id: 057e505c16be50d16fc1dc12c1e5ed754ace87ed
Pull Request resolved: #29733
  • Loading branch information
josephsavona committed Jun 3, 2024
1 parent d77dd31 commit 0b6a09d
Show file tree
Hide file tree
Showing 17 changed files with 658 additions and 14 deletions.
4 changes: 4 additions & 0 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,10 @@ export type FunctionEffect =
kind: "GlobalMutation";
error: CompilerErrorDetailOptions;
}
| {
kind: "RefMutation";
error: CompilerErrorDetailOptions;
}
| {
kind: "ReactMutation";
error: CompilerErrorDetailOptions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import {
isArrayType,
isMutableEffect,
isObjectType,
isRefValueType,
isUseRefType,
} from "../HIR/HIR";
import { FunctionSignature } from "../HIR/ObjectShape";
import {
Expand Down Expand Up @@ -239,7 +241,8 @@ export default function inferReferenceEffects(
functionEffects.forEach((eff) => {
switch (eff.kind) {
case "ReactMutation":
case "GlobalMutation": {
case "GlobalMutation":
case "RefMutation": {
CompilerError.throw(eff.error);
}
case "ContextMutation": {
Expand Down Expand Up @@ -407,7 +410,8 @@ class InferenceState {
for (const effect of value.loweredFunc.func.effects) {
if (
effect.kind === "GlobalMutation" ||
effect.kind === "ReactMutation"
effect.kind === "ReactMutation" ||
effect.kind === "RefMutation"
) {
// Known effects are always propagated upwards
functionEffects.push(effect);
Expand Down Expand Up @@ -521,7 +525,28 @@ class InferenceState {
break;
}
case Effect.Mutate: {
if (valueKind.kind === ValueKind.Context) {
if (
isRefValueType(place.identifier) ||
isUseRefType(place.identifier)
) {
if (this.#env.config.validateRefAccessDuringRender) {
functionEffect = {
kind: "RefMutation",
error: {
reason:
"Ref values (the `current` property) may not be modified during render. (https://react.dev/reference/react/useRef)",
description:
place.identifier.name !== null &&
place.identifier.name.kind === "named"
? `Found mutation of \`${place.identifier.name.value}\``
: null,
loc: place.loc,
suggestions: null,
severity: ErrorSeverity.InvalidReact,
},
};
}
} else if (valueKind.kind === ValueKind.Context) {
functionEffect = {
kind: "ContextMutation",
loc: place.loc,
Expand Down Expand Up @@ -560,7 +585,28 @@ class InferenceState {
break;
}
case Effect.Store: {
if (valueKind.kind === ValueKind.Context) {
if (
isRefValueType(place.identifier) ||
isUseRefType(place.identifier)
) {
if (this.#env.config.validateRefAccessDuringRender) {
functionEffect = {
kind: "RefMutation",
error: {
reason:
"Ref values (the `current` property) may not be modified during render. (https://react.dev/reference/react/useRef)",
description:
place.identifier.name !== null &&
place.identifier.name.kind === "named"
? `Found mutation of \`${place.identifier.name.value}\``
: null,
loc: place.loc,
suggestions: null,
severity: ErrorSeverity.InvalidReact,
},
};
}
} else if (valueKind.kind === ValueKind.Context) {
functionEffect = {
kind: "ContextMutation",
loc: place.loc,
Expand Down Expand Up @@ -1139,7 +1185,9 @@ function inferBlock(
);
functionEffects.push(
...propEffects.filter(
(propEffect) => propEffect.kind !== "GlobalMutation"
(propEffect) =>
propEffect.kind !== "GlobalMutation" &&
propEffect.kind !== "RefMutation"
)
);
}
Expand Down Expand Up @@ -1333,7 +1381,10 @@ function inferBlock(
functionEffects.push(
...argumentEffects.filter(
(argEffect) =>
!isUseEffect || i !== 0 || argEffect.kind !== "GlobalMutation"
!isUseEffect ||
i !== 0 ||
(argEffect.kind !== "GlobalMutation" &&
argEffect.kind !== "RefMutation")
)
);
hasCaptureArgument ||= place.effect === Effect.Capture;
Expand Down Expand Up @@ -1462,7 +1513,10 @@ function inferBlock(
functionEffects.push(
...argumentEffects.filter(
(argEffect) =>
!isUseEffect || i !== 0 || argEffect.kind !== "GlobalMutation"
!isUseEffect ||
i !== 0 ||
(argEffect.kind !== "GlobalMutation" &&
argEffect.kind !== "RefMutation")
)
);
hasCaptureArgument ||= place.effect === Effect.Capture;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@

## Input

```javascript
// @validateRefAccessDuringRender
import { useRef } from "react";

function Component() {
const ref = useRef(null);

const setRef = () => {
if (ref.current !== null) {
ref.current = "";
}
};

const onClick = () => {
setRef();
};

return (
<>
<input ref={ref} />
<button onClick={onClick} />
</>
);
}

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

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @validateRefAccessDuringRender
import { useRef } from "react";

function Component() {
const $ = _c(10);
const ref = useRef(null);
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = () => {
if (ref.current !== null) {
ref.current = "";
}
};
$[0] = t0;
} else {
t0 = $[0];
}
const setRef = t0;
let t1;
if ($[1] !== setRef) {
t1 = () => {
setRef();
};
$[1] = setRef;
$[2] = t1;
} else {
t1 = $[2];
}
const onClick = t1;
let t2;
if ($[3] !== ref) {
t2 = <input ref={ref} />;
$[3] = ref;
$[4] = t2;
} else {
t2 = $[4];
}
let t3;
if ($[5] !== onClick) {
t3 = <button onClick={onClick} />;
$[5] = onClick;
$[6] = t3;
} else {
t3 = $[6];
}
let t4;
if ($[7] !== t2 || $[8] !== t3) {
t4 = (
<>
{t2}
{t3}
</>
);
$[7] = t2;
$[8] = t3;
$[9] = t4;
} else {
t4 = $[9];
}
return t4;
}

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

```
### Eval output
(kind: ok) <input><button></button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// @validateRefAccessDuringRender
import { useRef } from "react";

function Component() {
const ref = useRef(null);

const setRef = () => {
if (ref.current !== null) {
ref.current = "";
}
};

const onClick = () => {
setRef();
};

return (
<>
<input ref={ref} />
<button onClick={onClick} />
</>
);
}

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

## Input

```javascript
// @validateRefAccessDuringRender
import { useRef } from "react";

function Component() {
const ref = useRef(null);

const onClick = () => {
if (ref.current !== null) {
ref.current = "";
}
};

return (
<>
<input ref={ref} />
<button onClick={onClick} />
</>
);
}

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

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @validateRefAccessDuringRender
import { useRef } from "react";

function Component() {
const $ = _c(8);
const ref = useRef(null);
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = () => {
if (ref.current !== null) {
ref.current = "";
}
};
$[0] = t0;
} else {
t0 = $[0];
}
const onClick = t0;
let t1;
if ($[1] !== ref) {
t1 = <input ref={ref} />;
$[1] = ref;
$[2] = t1;
} else {
t1 = $[2];
}
let t2;
if ($[3] !== onClick) {
t2 = <button onClick={onClick} />;
$[3] = onClick;
$[4] = t2;
} else {
t2 = $[4];
}
let t3;
if ($[5] !== t1 || $[6] !== t2) {
t3 = (
<>
{t1}
{t2}
</>
);
$[5] = t1;
$[6] = t2;
$[7] = t3;
} else {
t3 = $[7];
}
return t3;
}

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

```
### Eval output
(kind: ok) <input><button></button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// @validateRefAccessDuringRender
import { useRef } from "react";

function Component() {
const ref = useRef(null);

const onClick = () => {
if (ref.current !== null) {
ref.current = "";
}
};

return (
<>
<input ref={ref} />
<button onClick={onClick} />
</>
);
}

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

0 comments on commit 0b6a09d

Please sign in to comment.