Compare commits

...

1 Commits

Author SHA1 Message Date
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
4 changed files with 173 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

@@ -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: [{}],
};