Skip to content

Commit

Permalink
[compiler] Fix invalid Array.map type (#32095)
Browse files Browse the repository at this point in the history
See test fixture
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32095).
* #32099
* #32104
* #32098
* #32097
* #32096
* __->__ #32095
* #32094
* #32093
  • Loading branch information
mofeiZ authored Jan 22, 2025
1 parent deba48a commit b83090f
Show file tree
Hide file tree
Showing 3 changed files with 224 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -549,8 +549,16 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [
[
'map',
addFunction(BUILTIN_SHAPES, [], {
/**
* Note `map`'s arguments are annotated as Effect.ConditionallyMutate as
* calling `<array>.map(fn)` might invoke `fn`, which means replaying its
* effects.
*
* (Note that Effect.Read / Effect.Capture on a function type means
* potential data dependency or aliasing respectively.)
*/
positionalParams: [],
restParam: Effect.Read,
restParam: Effect.ConditionallyMutate,
returnType: {kind: 'Object', shapeId: BuiltInArrayId},
calleeEffect: Effect.ConditionallyMutate,
returnValueKind: ValueKind.Mutable,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@

## Input

```javascript
import {
arrayPush,
identity,
makeArray,
Stringify,
useFragment,
} from 'shared-runtime';

/**
* Bug repro showing why it's invalid for function references to be annotated
* with a `Read` effect when that reference might lead to the function being
* invoked.
*
* Note that currently, `Array.map` is annotated to have `Read` effects on its
* operands. This is incorrect as function effects must be replayed when `map`
* is called
* - Read: non-aliasing data dependency
* - Capture: maybe-aliasing data dependency
* - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
* but only if the value is mutable
*
* Invalid evaluator result: Found differences in evaluator results Non-forget
* (expected): (kind: ok)
* <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
* <div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
* Forget:
* (kind: ok)
* <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
* <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
*/

function Component({extraJsx}) {
const x = makeArray();
const items = useFragment();
// This closure has the following effects that must be replayed:
// - MaybeFreeze / Capture of `items`
// - ConditionalMutate of x
const jsx = items.a.map((item, i) => {
arrayPush(x, 2);
return <Stringify item={item} key={i} />;
});
const offset = jsx.length;
for (let i = 0; i < extraJsx; i++) {
jsx.push(<Stringify item={0} key={i + offset} />);
}
const count = jsx.length;
identity(count);
return (
<>
<Stringify x={x} count={count} />
{jsx[0]}
</>
);
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{extraJsx: 0}],
sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}],
};

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime";
import {
arrayPush,
identity,
makeArray,
Stringify,
useFragment,
} from "shared-runtime";

/**
* Bug repro showing why it's invalid for function references to be annotated
* with a `Read` effect when that reference might lead to the function being
* invoked.
*
* Note that currently, `Array.map` is annotated to have `Read` effects on its
* operands. This is incorrect as function effects must be replayed when `map`
* is called
* - Read: non-aliasing data dependency
* - Capture: maybe-aliasing data dependency
* - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
* but only if the value is mutable
*
* Invalid evaluator result: Found differences in evaluator results Non-forget
* (expected): (kind: ok)
* <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
* <div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
* Forget:
* (kind: ok)
* <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
* <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
*/

function Component(t0) {
const $ = _c(6);
const { extraJsx } = t0;
const x = makeArray();
const items = useFragment();

const jsx = items.a.map((item, i) => {
arrayPush(x, 2);
return <Stringify item={item} key={i} />;
});
const offset = jsx.length;
for (let i_0 = 0; i_0 < extraJsx; i_0++) {
jsx.push(<Stringify item={0} key={i_0 + offset} />);
}

const count = jsx.length;
identity(count);
let t1;
if ($[0] !== count || $[1] !== x) {
t1 = <Stringify x={x} count={count} />;
$[0] = count;
$[1] = x;
$[2] = t1;
} else {
t1 = $[2];
}
const t2 = jsx[0];
let t3;
if ($[3] !== t1 || $[4] !== t2) {
t3 = (
<>
{t1}
{t2}
</>
);
$[3] = t1;
$[4] = t2;
$[5] = t3;
} else {
t3 = $[5];
}
return t3;
}

export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{ extraJsx: 0 }],
sequentialRenders: [{ extraJsx: 0 }, { extraJsx: 1 }],
};

```
### Eval output
(kind: ok) <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
<div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import {
arrayPush,
identity,
makeArray,
Stringify,
useFragment,
} from 'shared-runtime';

/**
* Bug repro showing why it's invalid for function references to be annotated
* with a `Read` effect when that reference might lead to the function being
* invoked.
*
* Note that currently, `Array.map` is annotated to have `Read` effects on its
* operands. This is incorrect as function effects must be replayed when `map`
* is called
* - Read: non-aliasing data dependency
* - Capture: maybe-aliasing data dependency
* - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
* but only if the value is mutable
*
* Invalid evaluator result: Found differences in evaluator results Non-forget
* (expected): (kind: ok)
* <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
* <div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
* Forget:
* (kind: ok)
* <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
* <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
*/

function Component({extraJsx}) {
const x = makeArray();
const items = useFragment();
// This closure has the following effects that must be replayed:
// - MaybeFreeze / Capture of `items`
// - ConditionalMutate of x
const jsx = items.a.map((item, i) => {
arrayPush(x, 2);
return <Stringify item={item} key={i} />;
});
const offset = jsx.length;
for (let i = 0; i < extraJsx; i++) {
jsx.push(<Stringify item={0} key={i + offset} />);
}
const count = jsx.length;
identity(count);
return (
<>
<Stringify x={x} count={count} />
{jsx[0]}
</>
);
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{extraJsx: 0}],
sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}],
};

0 comments on commit b83090f

Please sign in to comment.