Skip to content

Commit 33a1095

Browse files
authored
[compiler] Infer render helpers for additional validation (facebook#33647)
We currently assume that any functions passes as props may be event handlers or effect functions, and thus don't check for side effects such as mutating globals. However, if a prop is a function that returns JSX that is a sure sign that it's actually a render helper and not an event handler or effect function. So we now emit a `Render` effect for any prop that is a JSX-returning function, triggering all of our render validation. This required a small fix to InferTypes: we weren't correctly populating the `return` type of function types during unification. I also improved the printing of types so we can see the inferred return types. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33647). * facebook#33643 * facebook#33650 * facebook#33642 * __->__ facebook#33647
1 parent 2135948 commit 33a1095

File tree

7 files changed

+141
-1
lines changed

7 files changed

+141
-1
lines changed

compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -880,7 +880,8 @@ export function printType(type: Type): string {
880880
if (type.kind === 'Object' && type.shapeId != null) {
881881
return `:T${type.kind}<${type.shapeId}>`;
882882
} else if (type.kind === 'Function' && type.shapeId != null) {
883-
return `:T${type.kind}<${type.shapeId}>`;
883+
const returnType = printType(type.return);
884+
return `:T${type.kind}<${type.shapeId}>()${returnType !== '' ? `: ${returnType}` : ''}`;
884885
} else {
885886
return `:T${type.kind}`;
886887
}

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
InstructionKind,
2828
InstructionValue,
2929
isArrayType,
30+
isJsxType,
3031
isMapType,
3132
isPrimitiveType,
3233
isRefOrRefValue,
@@ -1871,6 +1872,23 @@ function computeSignatureForInstruction(
18711872
});
18721873
}
18731874
}
1875+
for (const prop of value.props) {
1876+
if (
1877+
prop.kind === 'JsxAttribute' &&
1878+
prop.place.identifier.type.kind === 'Function' &&
1879+
(isJsxType(prop.place.identifier.type.return) ||
1880+
(prop.place.identifier.type.return.kind === 'Phi' &&
1881+
prop.place.identifier.type.return.operands.some(operand =>
1882+
isJsxType(operand),
1883+
)))
1884+
) {
1885+
// Any props which return jsx are assumed to be called during render
1886+
effects.push({
1887+
kind: 'Render',
1888+
place: prop.place,
1889+
});
1890+
}
1891+
}
18741892
}
18751893
break;
18761894
}

compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -777,6 +777,15 @@ class Unifier {
777777
return {kind: 'Phi', operands: type.operands.map(o => this.get(o))};
778778
}
779779

780+
if (type.kind === 'Function') {
781+
return {
782+
kind: 'Function',
783+
isConstructor: type.isConstructor,
784+
shapeId: type.shapeId,
785+
return: this.get(type.return),
786+
};
787+
}
788+
780789
return type;
781790
}
782791
}
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
2+
## Input
3+
4+
```javascript
5+
function Component() {
6+
const renderItem = item => {
7+
// Multiple returns so that the return type is a Phi (union)
8+
if (item == null) {
9+
return null;
10+
}
11+
// Normally we assume that it's safe to mutate globals in a function passed
12+
// as a prop, because the prop could be used as an event handler or effect.
13+
// But if the function returns JSX we can assume it's a render helper, ie
14+
// called during render, and thus it's unsafe to mutate globals or call
15+
// other impure code.
16+
global.property = true;
17+
return <Item item={item} value={rand} />;
18+
};
19+
return <ItemList renderItem={renderItem} />;
20+
}
21+
22+
```
23+
24+
25+
## Error
26+
27+
```
28+
Found 1 error:
29+
30+
Error: This value cannot be modified
31+
32+
Modifying a variable defined outside a component or hook is not allowed. Consider using an effect.
33+
34+
error.invalid-mutate-global-in-render-helper-phi-return-prop.ts:12:4
35+
10 | // called during render, and thus it's unsafe to mutate globals or call
36+
11 | // other impure code.
37+
> 12 | global.property = true;
38+
| ^^^^^^ value cannot be modified
39+
13 | return <Item item={item} value={rand} />;
40+
14 | };
41+
15 | return <ItemList renderItem={renderItem} />;
42+
```
43+
44+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
function Component() {
2+
const renderItem = item => {
3+
// Multiple returns so that the return type is a Phi (union)
4+
if (item == null) {
5+
return null;
6+
}
7+
// Normally we assume that it's safe to mutate globals in a function passed
8+
// as a prop, because the prop could be used as an event handler or effect.
9+
// But if the function returns JSX we can assume it's a render helper, ie
10+
// called during render, and thus it's unsafe to mutate globals or call
11+
// other impure code.
12+
global.property = true;
13+
return <Item item={item} value={rand} />;
14+
};
15+
return <ItemList renderItem={renderItem} />;
16+
}
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
2+
## Input
3+
4+
```javascript
5+
function Component() {
6+
const renderItem = item => {
7+
// Normally we assume that it's safe to mutate globals in a function passed
8+
// as a prop, because the prop could be used as an event handler or effect.
9+
// But if the function returns JSX we can assume it's a render helper, ie
10+
// called during render, and thus it's unsafe to mutate globals or call
11+
// other impure code.
12+
global.property = true;
13+
return <Item item={item} value={rand} />;
14+
};
15+
return <ItemList renderItem={renderItem} />;
16+
}
17+
18+
```
19+
20+
21+
## Error
22+
23+
```
24+
Found 1 error:
25+
26+
Error: This value cannot be modified
27+
28+
Modifying a variable defined outside a component or hook is not allowed. Consider using an effect.
29+
30+
error.invalid-mutate-global-in-render-helper-prop.ts:8:4
31+
6 | // called during render, and thus it's unsafe to mutate globals or call
32+
7 | // other impure code.
33+
> 8 | global.property = true;
34+
| ^^^^^^ value cannot be modified
35+
9 | return <Item item={item} value={rand} />;
36+
10 | };
37+
11 | return <ItemList renderItem={renderItem} />;
38+
```
39+
40+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
function Component() {
2+
const renderItem = item => {
3+
// Normally we assume that it's safe to mutate globals in a function passed
4+
// as a prop, because the prop could be used as an event handler or effect.
5+
// But if the function returns JSX we can assume it's a render helper, ie
6+
// called during render, and thus it's unsafe to mutate globals or call
7+
// other impure code.
8+
global.property = true;
9+
return <Item item={item} value={rand} />;
10+
};
11+
return <ItemList renderItem={renderItem} />;
12+
}

0 commit comments

Comments
 (0)