Compare commits

...

2 Commits

Author SHA1 Message Date
Joe Savona
5eb09a72c4 [compiler] More flexible/helpful lazy ref initialization
Two small QoL improvements inspired by feedback:
* `if (ref.current == undefined) { ref.current = ... }` is now allowed.
* `if (!ref.current) { ref.current = ... }` is still disallowed, but we emit an extra hint suggesting the `if (!ref.current == null)` pattern.

I was on the fence about the latter. We got feedback asking to allow `if (!ref.current)` but if your ref stores a boolean value then this would allow reading the ref in render. The unary form is also less precise in general due to sketchy truthiness conversions. I figured a hint is a good compromise.
2025-09-09 17:52:16 -07:00
Joe Savona
8219b34b60 [compiler] Fix false positive hook return mutation error
This was fun. We previously added the MaybeAlias effect in #33984 in order to describe the semantic that an unknown function call _may_ alias its return value in its result, but that we don't know this for sure. We record mutations through MaybeAlias edges when walking backward in the data flow graph, but downgrade them to conditional mutations. See the original PR for full context.

That change was sufficient for the original case like

```js
const frozen = useContext();
useEffect(() => {
  frozen.method().property = true;
}, [...]);
```

But it wasn't sufficient for cases where the aliasing occured between operands:

```js
const dispatch = useDispatch();
<div onClick={(e) => {
  dispatch(...e.target.value)
  e.target.value = ...;
}} />
```

Here we would record a `Capture dispatch <- e.target` effect. Then during processing of the `event.target.value = ...` assignment we'd eventually  _forward_ from `event` to `dispatch` (along a MaybeAlias edge). But in #33984 I missed that this forward walk also has to downgrade to conditional.

In addition to that change, we also have to be a bit more precise about which set of effects we create for alias/capture/maybe-alias. The new logic is a bit clearer, I think:

* If the value is frozen, it's an ImmutableCapture edge
* If the values are mutable, it's a Capture
* If it's a context->context, context->mutable, or mutable->context, count it as MaybeAlias.
2025-09-09 13:18:05 -07:00
11 changed files with 420 additions and 28 deletions

View File

@@ -748,10 +748,14 @@ function applyEffect(
case 'Alias':
case 'Capture': {
CompilerError.invariant(
effect.kind === 'Capture' || initialized.has(effect.into.identifier.id),
effect.kind === 'Capture' ||
effect.kind === 'MaybeAlias' ||
initialized.has(effect.into.identifier.id),
{
reason: `Expected destination value to already be initialized within this instruction for Alias effect`,
description: `Destination ${printPlace(effect.into)} is not initialized in this instruction`,
reason: `Expected destination to already be initialized within this instruction`,
description:
`Destination ${printPlace(effect.into)} is not initialized in this ` +
`instruction for effect ${printAliasingEffect(effect)}`,
details: [
{
kind: 'error',
@@ -767,49 +771,67 @@ function applyEffect(
* copy-on-write semantics, then we can prune the effect
*/
const intoKind = state.kind(effect.into).kind;
let isMutableDesination: boolean;
let destinationType: 'context' | 'mutable' | null = null;
switch (intoKind) {
case ValueKind.Context:
case ValueKind.Mutable:
case ValueKind.MaybeFrozen: {
isMutableDesination = true;
case ValueKind.Context: {
destinationType = 'context';
break;
}
default: {
isMutableDesination = false;
case ValueKind.Mutable:
case ValueKind.MaybeFrozen: {
destinationType = 'mutable';
break;
}
}
const fromKind = state.kind(effect.from).kind;
let isMutableReferenceType: boolean;
let sourceType: 'context' | 'mutable' | 'frozen' | null = null;
switch (fromKind) {
case ValueKind.Context: {
sourceType = 'context';
break;
}
case ValueKind.Global:
case ValueKind.Primitive: {
isMutableReferenceType = false;
break;
}
case ValueKind.Frozen: {
isMutableReferenceType = false;
applyEffect(
context,
state,
{
kind: 'ImmutableCapture',
from: effect.from,
into: effect.into,
},
initialized,
effects,
);
sourceType = 'frozen';
break;
}
default: {
isMutableReferenceType = true;
sourceType = 'mutable';
break;
}
}
if (isMutableDesination && isMutableReferenceType) {
if (sourceType === 'frozen') {
applyEffect(
context,
state,
{
kind: 'ImmutableCapture',
from: effect.from,
into: effect.into,
},
initialized,
effects,
);
} else if (
(sourceType === 'mutable' && destinationType === 'mutable') ||
effect.kind === 'MaybeAlias'
) {
effects.push(effect);
} else if (
(sourceType === 'context' && destinationType != null) ||
(sourceType === 'mutable' && destinationType === 'context')
) {
applyEffect(
context,
state,
{kind: 'MaybeAlias', from: effect.from, into: effect.into},
initialized,
effects,
);
}
break;
}

View File

@@ -779,7 +779,13 @@ class AliasingState {
if (edge.index >= index) {
break;
}
queue.push({place: edge.node, transitive, direction: 'forwards', kind});
queue.push({
place: edge.node,
transitive,
direction: 'forwards',
// Traversing a maybeAlias edge always downgrades to conditional mutation
kind: edge.kind === 'maybeAlias' ? MutationKind.Conditional : kind,
});
}
for (const [alias, when] of node.createdFrom) {
if (when >= index) {
@@ -807,7 +813,12 @@ class AliasingState {
if (when >= index) {
continue;
}
queue.push({place: alias, transitive, direction: 'backwards', kind});
queue.push({
place: alias,
transitive,
direction: 'backwards',
kind,
});
}
/**
* MaybeAlias indicates potential data flow from unknown function calls,

View File

@@ -639,12 +639,55 @@ function validateNoRefAccessInRenderImpl(
case 'StartMemoize':
case 'FinishMemoize':
break;
case 'LoadGlobal': {
if (instr.value.binding.name === 'undefined') {
env.set(instr.lvalue.identifier.id, {kind: 'Nullable'});
}
break;
}
case 'Primitive': {
if (instr.value.value == null) {
env.set(instr.lvalue.identifier.id, {kind: 'Nullable'});
}
break;
}
case 'UnaryExpression': {
if (instr.value.operator === '!') {
const value = env.get(instr.value.value.identifier.id);
const refId =
value?.kind === 'RefValue' && value.refId != null
? value.refId
: null;
if (refId !== null) {
/*
* Record an error suggesting the `if (ref.current == null)` pattern,
* but also record the lvalue as a guard so that we don't emit a second
* error for the write to the ref
*/
env.set(instr.lvalue.identifier.id, {kind: 'Guard', refId});
errors.pushDiagnostic(
CompilerDiagnostic.create({
category: ErrorCategory.Refs,
reason: 'Cannot access refs during render',
description: ERROR_DESCRIPTION,
})
.withDetails({
kind: 'error',
loc: instr.value.value.loc,
message: `Cannot access ref value during render`,
})
.withDetails({
kind: 'hint',
message:
'To initialize a ref only once, check that the ref is null with the pattern `if (ref.current == null) { ref.current = ... }`',
}),
);
break;
}
}
validateNoRefValueAccess(errors, env, instr.value.value);
break;
}
case 'BinaryExpression': {
const left = env.get(instr.value.left.identifier.id);
const right = env.get(instr.value.right.identifier.id);

View File

@@ -0,0 +1,42 @@
## Input
```javascript
//@flow
import {useRef} from 'react';
component C() {
const r = useRef(null);
if (r.current == undefined) {
r.current = 1;
}
}
export const FIXTURE_ENTRYPOINT = {
fn: C,
params: [{}],
};
```
## Code
```javascript
import { useRef } from "react";
function C() {
const r = useRef(null);
if (r.current == undefined) {
r.current = 1;
}
}
export const FIXTURE_ENTRYPOINT = {
fn: C,
params: [{}],
};
```
### Eval output
(kind: ok)

View File

@@ -0,0 +1,14 @@
//@flow
import {useRef} from 'react';
component C() {
const r = useRef(null);
if (r.current == undefined) {
r.current = 1;
}
}
export const FIXTURE_ENTRYPOINT = {
fn: C,
params: [{}],
};

View File

@@ -0,0 +1,78 @@
## Input
```javascript
//@flow
import {useRef} from 'react';
component C() {
const r = useRef(null);
const current = !r.current;
return <div>{current}</div>;
}
export const FIXTURE_ENTRYPOINT = {
fn: C,
params: [{}],
};
```
## Error
```
Found 4 errors:
Error: Cannot access refs during render
React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef).
4 | component C() {
5 | const r = useRef(null);
> 6 | const current = !r.current;
| ^^^^^^^^^ Cannot access ref value during render
7 | return <div>{current}</div>;
8 | }
9 |
To initialize a ref only once, check that the ref is null with the pattern `if (ref.current == null) { ref.current = ... }`
Error: Cannot access refs during render
React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef).
4 | component C() {
5 | const r = useRef(null);
> 6 | const current = !r.current;
| ^^^^^^^^^^ Cannot access ref value during render
7 | return <div>{current}</div>;
8 | }
9 |
Error: Cannot access refs during render
React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef).
5 | const r = useRef(null);
6 | const current = !r.current;
> 7 | return <div>{current}</div>;
| ^^^^^^^ Cannot access ref value during render
8 | }
9 |
10 | export const FIXTURE_ENTRYPOINT = {
Error: Cannot access refs during render
React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef).
5 | const r = useRef(null);
6 | const current = !r.current;
> 7 | return <div>{current}</div>;
| ^^^^^^^ Cannot access ref value during render
8 | }
9 |
10 | export const FIXTURE_ENTRYPOINT = {
```

View File

@@ -0,0 +1,13 @@
//@flow
import {useRef} from 'react';
component C() {
const r = useRef(null);
const current = !r.current;
return <div>{current}</div>;
}
export const FIXTURE_ENTRYPOINT = {
fn: C,
params: [{}],
};

View File

@@ -0,0 +1,43 @@
## Input
```javascript
//@flow
import {useRef} from 'react';
component C() {
const r = useRef(null);
if (!r.current) {
r.current = 1;
}
}
export const FIXTURE_ENTRYPOINT = {
fn: C,
params: [{}],
};
```
## Error
```
Found 1 error:
Error: Cannot access refs during render
React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef).
4 | component C() {
5 | const r = useRef(null);
> 6 | if (!r.current) {
| ^^^^^^^^^ Cannot access ref value during render
7 | r.current = 1;
8 | }
9 | }
To initialize a ref only once, check that the ref is null with the pattern `if (ref.current == null) { ref.current = ... }`
```

View File

@@ -0,0 +1,14 @@
//@flow
import {useRef} from 'react';
component C() {
const r = useRef(null);
if (!r.current) {
r.current = 1;
}
}
export const FIXTURE_ENTRYPOINT = {
fn: C,
params: [{}],
};

View File

@@ -0,0 +1,82 @@
## Input
```javascript
// @compilationMode:"infer"
function Component() {
const dispatch = useDispatch();
// const [state, setState] = useState(0);
return (
<div>
<input
type="file"
onChange={event => {
dispatch(...event.target);
event.target.value = '';
}}
/>
</div>
);
}
function useDispatch() {
'use no memo';
// skip compilation to make it easier to debug the above function
return (...values) => {
console.log(...values);
};
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{}],
};
```
## Code
```javascript
import { c as _c } from "react/compiler-runtime"; // @compilationMode:"infer"
function Component() {
const $ = _c(2);
const dispatch = useDispatch();
let t0;
if ($[0] !== dispatch) {
t0 = (
<div>
<input
type="file"
onChange={(event) => {
dispatch(...event.target);
event.target.value = "";
}}
/>
</div>
);
$[0] = dispatch;
$[1] = t0;
} else {
t0 = $[1];
}
return t0;
}
function useDispatch() {
"use no memo";
// skip compilation to make it easier to debug the above function
return (...values) => {
console.log(...values);
};
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{}],
};
```
### Eval output
(kind: ok) <div><input type="file"></div>

View File

@@ -0,0 +1,30 @@
// @compilationMode:"infer"
function Component() {
const dispatch = useDispatch();
// const [state, setState] = useState(0);
return (
<div>
<input
type="file"
onChange={event => {
dispatch(...event.target);
event.target.value = '';
}}
/>
</div>
);
}
function useDispatch() {
'use no memo';
// skip compilation to make it easier to debug the above function
return (...values) => {
console.log(...values);
};
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{}],
};