diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts index 356bc8af08..aac092707d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -276,6 +276,8 @@ class InferenceState { */ #variables: Map>; + #functionEffects: Map> = new Map(); + constructor( env: Environment, values: Map, @@ -302,6 +304,19 @@ class InferenceState { this.#values.set(value, kind); } + setFunctionEffects(value: InstructionValue, effect: Array): void { + CompilerError.invariant(value.kind !== 'LoadLocal', { + reason: + 'Expected all top-level identifiers to be defined as variables, not values', + description: null, + loc: value.loc, + suggestions: null, + }); + const curEffects = this.#functionEffects.get(value) ?? []; + curEffects.push(...effect); + this.#functionEffects.set(value, curEffects); + } + values(place: Place): Array { const values = this.#variables.get(place.identifier.id); CompilerError.invariant(values != null, { @@ -400,50 +415,57 @@ class InferenceState { } // Propagate effects of function expressions to the outer (ie current) effect context + const dependentEffects = []; for (const value of values) { if ( (value.kind === 'FunctionExpression' || - value.kind === 'ObjectMethod') && - value.loweredFunc.func.effects != null + value.kind === 'ObjectMethod') ) { - for (const effect of value.loweredFunc.func.effects) { - if ( - effect.kind === 'GlobalMutation' || - effect.kind === 'ReactMutation' - ) { - // Known effects are always propagated upwards - functionEffects.push(effect); - } else { - /** - * Contextual effects need to be replayed against the current inference - * state, which may know more about the value to which the effect applied. - * The main cases are: - * 1. The mutated context value is _still_ a context value in the current scope, - * so we have to continue propagating the original context mutation. - * 2. The mutated context value is a mutable value in the current scope, - * so the context mutation was fine and we can skip propagating the effect. - * 3. The mutated context value is an immutable value in the current scope, - * resulting in a non-ContextMutation FunctionEffect. We propagate that new, - * more detailed effect to the current function context. - */ - for (const place of effect.places) { - if (this.isDefined(place)) { - const replayedEffect = this.reference( - {...place, loc: effect.loc}, - effect.effect, - reason, - ); - if (replayedEffect != null) { - if (replayedEffect.kind === 'ContextMutation') { - // Case 1, still a context value so propagate the original effect - functionEffects.push(effect); - } else { - // Case 3, immutable value so propagate the more precise effect - functionEffects.push(replayedEffect); - } - } // else case 2, local mutable value so this effect was fine + dependentEffects.push(...value.loweredFunc.func.effects ?? []); + } + const knownEffects = this.#functionEffects.get(value); + if (knownEffects != null) { + dependentEffects.push(...knownEffects) + } + } + + + for (const effect of dependentEffects) { + if ( + effect.kind === 'GlobalMutation' || + effect.kind === 'ReactMutation' + ) { + // Known effects are always propagated upwards + functionEffects.push(effect); + } else { + /** + * Contextual effects need to be replayed against the current inference + * state, which may know more about the value to which the effect applied. + * The main cases are: + * 1. The mutated context value is _still_ a context value in the current scope, + * so we have to continue propagating the original context mutation. + * 2. The mutated context value is a mutable value in the current scope, + * so the context mutation was fine and we can skip propagating the effect. + * 3. The mutated context value is an immutable value in the current scope, + * resulting in a non-ContextMutation FunctionEffect. We propagate that new, + * more detailed effect to the current function context. + */ + for (const place of effect.places) { + if (this.isDefined(place)) { + const replayedEffect = this.reference( + {...place, loc: effect.loc}, + effect.effect, + reason, + ); + if (replayedEffect != null) { + if (replayedEffect.kind === 'ContextMutation') { + // Case 1, still a context value so propagate the original effect + functionEffects.push(effect); + } else { + // Case 3, immutable value so propagate the more precise effect + functionEffects.push(replayedEffect); } - } + } // else case 2, local mutable value so this effect was fine } } } @@ -995,8 +1017,22 @@ function inferBlock( context: new Set(), }; effect = {kind: Effect.Capture, reason: ValueReason.Other}; - lvalueEffect = Effect.Store; - break; + + const arrEffects: Array = []; + for (const operand of eachInstructionOperand(instr)) { + state.referenceAndRecordEffects( + operand, + effect.kind, + effect.reason, + arrEffects, + ); + } + + state.initialize(instrValue, valueKind); + state.define(instr.lvalue, instrValue); + instr.lvalue.effect = Effect.Store; + state.setFunctionEffects(instrValue, arrEffects); + continue; } case 'NewExpression': { /** @@ -1039,6 +1075,7 @@ function inferBlock( continue; } case 'ObjectExpression': { + const objEffects: Array = []; valueKind = hasContextRefOperand(state, instrValue) ? { kind: ValueKind.Context, @@ -1060,7 +1097,7 @@ function inferBlock( property.key.name, Effect.Freeze, ValueReason.Other, - functionEffects, + objEffects, ); } // Object construction captures but does not modify the key/property values @@ -1068,7 +1105,7 @@ function inferBlock( property.place, Effect.Capture, ValueReason.Other, - functionEffects, + objEffects, ); break; } @@ -1078,7 +1115,7 @@ function inferBlock( property.place, Effect.Capture, ValueReason.Other, - functionEffects, + objEffects, ); break; } @@ -1092,6 +1129,7 @@ function inferBlock( } state.initialize(instrValue, valueKind); + state.setFunctionEffects(instrValue, objEffects); state.define(instr.lvalue, instrValue); instr.lvalue.effect = Effect.Store; continue; @@ -1547,14 +1585,16 @@ function inferBlock( break; } case 'PropertyLoad': { + const loadEffects: Array = []; state.referenceAndRecordEffects( instrValue.object, Effect.Read, ValueReason.Other, - functionEffects, + loadEffects, ); const lvalue = instr.lvalue; lvalue.effect = Effect.ConditionallyMutate; + state.setFunctionEffects(instrValue, loadEffects); state.initialize(instrValue, state.kind(instrValue.object)); state.define(lvalue, instrValue); continue; @@ -1611,22 +1651,24 @@ function inferBlock( continue; } case 'ComputedLoad': { + const loadEffects: Array = []; state.referenceAndRecordEffects( instrValue.object, Effect.Read, ValueReason.Other, - functionEffects, + loadEffects, ); state.referenceAndRecordEffects( instrValue.property, Effect.Read, ValueReason.Other, - functionEffects, + loadEffects, ); const lvalue = instr.lvalue; lvalue.effect = Effect.ConditionallyMutate; state.initialize(instrValue, state.kind(instrValue.object)); state.define(lvalue, instrValue); + state.setFunctionEffects(instrValue, loadEffects); continue; } case 'Await': { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.expect.md deleted file mode 100644 index 1ab2a46afe..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.expect.md +++ /dev/null @@ -1,33 +0,0 @@ - -## Input - -```javascript -function Foo() { - const x = () => { - window.href = 'foo'; - }; - const y = {x}; - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [], -}; - -``` - - -## Error - -``` - 1 | function Foo() { - 2 | const x = () => { -> 3 | window.href = 'foo'; - | ^^^^^^ InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect (3:3) - 4 | }; - 5 | const y = {x}; - 6 | return ; -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.reassign-global-obj-call.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.reassign-global-obj-call.expect.md new file mode 100644 index 0000000000..831943364b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.reassign-global-obj-call.expect.md @@ -0,0 +1,35 @@ + +## Input + +```javascript +let b = 1; + +export default function useMyHook() { + const fn = () => { + b = 2; + }; + const obj = { fn }; + obj.fn(); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useMyHook, + params: [], +}; + +``` + + +## Error + +``` + 3 | export default function useMyHook() { + 4 | const fn = () => { +> 5 | b = 2; + | ^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (5:5) + 6 | }; + 7 | const obj = { fn }; + 8 | obj.fn(); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.reassign-global-obj-call.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.reassign-global-obj-call.js new file mode 100644 index 0000000000..bc71da1d9e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.reassign-global-obj-call.js @@ -0,0 +1,15 @@ +let b = 1; + +export default function useMyHook() { + const fn = () => { + b = 2; + }; + const obj = { fn }; + const arr = [obj]; + foo(arr); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useMyHook, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-capture-global-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-capture-global-mutation.expect.md new file mode 100644 index 0000000000..9d970ef9e6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-capture-global-mutation.expect.md @@ -0,0 +1,49 @@ + +## Input + +```javascript +function Foo() { + const x = () => { + window.href = 'foo'; + }; + const y = {x}; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Foo() { + const $ = _c(1); + const x = _temp; + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const y = { x }; + t0 = ; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} +function _temp() { + window.href = "foo"; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; + +``` + +### Eval output +(kind: exception) Bar is not defined \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-capture-global-mutation.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-capture-global-mutation.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reassign-global-return-obj.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reassign-global-return-obj.expect.md new file mode 100644 index 0000000000..52e981d7c5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reassign-global-return-obj.expect.md @@ -0,0 +1,51 @@ + +## Input + +```javascript +let b = 1; + +export default function useMyHook() { + const fn = () => { + b = 2; + }; + return { fn }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useMyHook, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +let b = 1; + +export default function useMyHook() { + const $ = _c(1); + const fn = _temp; + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = { fn }; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} +function _temp() { + b = 2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useMyHook, + params: [], +}; + +``` + +### Eval output +(kind: ok) {"fn":"[[ function params=0 ]]"} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reassign-global-return-obj.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reassign-global-return-obj.js new file mode 100644 index 0000000000..696b3b84c9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reassign-global-return-obj.js @@ -0,0 +1,13 @@ +let b = 1; + +export default function useMyHook() { + const fn = () => { + b = 2; + }; + return { fn }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useMyHook, + params: [], +};