From 7398334948f21f96fc531c3f5ea993ce8f53a7ad Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 6 Aug 2025 23:18:13 -0700 Subject: [PATCH] [compiler] Infer render helpers for additional validation 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. --- .../src/HIR/PrintHIR.ts | 3 +- .../Inference/InferMutationAliasingEffects.ts | 18 ++++++++ .../src/TypeInference/InferTypes.ts | 9 ++++ ...in-render-helper-phi-return-prop.expect.md | 44 +++++++++++++++++++ ...global-in-render-helper-phi-return-prop.js | 16 +++++++ ...ate-global-in-render-helper-prop.expect.md | 40 +++++++++++++++++ ...lid-mutate-global-in-render-helper-prop.js | 12 +++++ 7 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index aa6a7b0c65ce..f12dcd6e4318 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -892,7 +892,8 @@ export function printType(type: Type): string { if (type.kind === 'Object' && type.shapeId != null) { return `:T${type.kind}<${type.shapeId}>`; } else if (type.kind === 'Function' && type.shapeId != null) { - return `:T${type.kind}<${type.shapeId}>`; + const returnType = printType(type.return); + return `:T${type.kind}<${type.shapeId}>()${returnType !== '' ? `: ${returnType}` : ''}`; } else { return `:T${type.kind}`; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts index 2adf78fe058e..ac2d0ab32a05 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -26,6 +26,7 @@ import { InstructionKind, InstructionValue, isArrayType, + isJsxType, isMapType, isPrimitiveType, isRefOrRefValue, @@ -1841,6 +1842,23 @@ function computeSignatureForInstruction( }); } } + for (const prop of value.props) { + if ( + prop.kind === 'JsxAttribute' && + prop.place.identifier.type.kind === 'Function' && + (isJsxType(prop.place.identifier.type.return) || + (prop.place.identifier.type.return.kind === 'Phi' && + prop.place.identifier.type.return.operands.some(operand => + isJsxType(operand), + ))) + ) { + // Any props which return jsx are assumed to be called during render + effects.push({ + kind: 'Render', + place: prop.place, + }); + } + } } break; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts index 488d988b9715..d3a297e2e51c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts @@ -777,6 +777,15 @@ class Unifier { return {kind: 'Phi', operands: type.operands.map(o => this.get(o))}; } + if (type.kind === 'Function') { + return { + kind: 'Function', + isConstructor: type.isConstructor, + shapeId: type.shapeId, + return: this.get(type.return), + }; + } + return type; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.expect.md new file mode 100644 index 000000000000..3e3dcab81868 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +function Component() { + const renderItem = item => { + // Multiple returns so that the return type is a Phi (union) + if (item == null) { + return null; + } + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying a variable defined outside a component or hook is not allowed. Consider using an effect. + +error.invalid-mutate-global-in-render-helper-phi-return-prop.ts:12:4 + 10 | // called during render, and thus it's unsafe to mutate globals or call + 11 | // other impure code. +> 12 | global.property = true; + | ^^^^^^ value cannot be modified + 13 | return ; + 14 | }; + 15 | return ; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.js new file mode 100644 index 000000000000..b6ca0a04aeea --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.js @@ -0,0 +1,16 @@ +function Component() { + const renderItem = item => { + // Multiple returns so that the return type is a Phi (union) + if (item == null) { + return null; + } + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md new file mode 100644 index 000000000000..9e995d5124d4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md @@ -0,0 +1,40 @@ + +## Input + +```javascript +function Component() { + const renderItem = item => { + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying a variable defined outside a component or hook is not allowed. Consider using an effect. + +error.invalid-mutate-global-in-render-helper-prop.ts:8:4 + 6 | // called during render, and thus it's unsafe to mutate globals or call + 7 | // other impure code. +> 8 | global.property = true; + | ^^^^^^ value cannot be modified + 9 | return ; + 10 | }; + 11 | return ; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js new file mode 100644 index 000000000000..9355c482fb61 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js @@ -0,0 +1,12 @@ +function Component() { + const renderItem = item => { + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +}