diff --git a/compiler/packages/babel-plugin-react-forget/src/CompilerError.ts b/compiler/packages/babel-plugin-react-forget/src/CompilerError.ts index c515991dd4..a1b6050b55 100644 --- a/compiler/packages/babel-plugin-react-forget/src/CompilerError.ts +++ b/compiler/packages/babel-plugin-react-forget/src/CompilerError.ts @@ -165,6 +165,12 @@ export class CompilerError extends Error { throw errors; } + static throw(options: CompilerErrorDetailOptions): never { + const errors = new CompilerError(); + errors.pushErrorDetail(new CompilerErrorDetail(options)); + throw errors; + } + constructor(...args: any[]) { super(...args); this.name = "ReactForgetCompilerError"; diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts index e1d3b88619..325162d427 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts @@ -216,6 +216,7 @@ export function lower( async: func.node.async === true, loc: func.node.loc ?? GeneratedSource, env, + effects: null, }); } diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts index 9c2ff5ef58..3d92a54572 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts @@ -6,7 +6,7 @@ */ import * as t from "@babel/types"; -import { CompilerError } from "../CompilerError"; +import { CompilerError, CompilerErrorDetailOptions } from "../CompilerError"; import { assertExhaustive } from "../Utils/utils"; import { Environment } from "./Environment"; import { HookKind } from "./ObjectShape"; @@ -244,11 +244,17 @@ export type HIRFunction = { params: Array; returnType: t.FlowType | t.TSType | null; context: Array; + effects: Array | null; body: HIR; generator: boolean; async: boolean; }; +export type FunctionEffect = { + kind: "GlobalMutation"; + error: CompilerErrorDetailOptions; +}; + /* * Each reactive scope may have its own control-flow, so the instructions form * a control-flow graph. The graph comprises a set of basic blocks which reference diff --git a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts index a565700d80..518ec1b504 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. */ -import { CompilerError } from "../CompilerError"; +import { CompilerError, ErrorSeverity } from "../CompilerError"; import { Environment } from "../HIR"; import { AbstractValue, @@ -13,6 +13,7 @@ import { BlockId, CallExpression, Effect, + FunctionEffect, GeneratedSource, HIRFunction, IdentifierId, @@ -43,6 +44,7 @@ import { eachTerminalSuccessor, } from "../HIR/visitors"; import { assertExhaustive } from "../Utils/utils"; +import { isEffectHook } from "../Validation/ValidateMemoizedEffectDependencies"; const UndefinedValue: InstructionValue = { kind: "Primitive", @@ -206,6 +208,8 @@ export default function inferReferenceEffects( } queue(fn.body.entry, initialState); + const functionEffects: Array = fn.effects ?? []; + while (queuedStates.size !== 0) { for (const [blockId, block] of fn.body.blocks) { const incomingState = queuedStates.get(blockId); @@ -216,13 +220,29 @@ export default function inferReferenceEffects( statesByBlock.set(blockId, incomingState); const state = incomingState.clone(); - inferBlock(fn.env, state, block); + inferBlock(fn.env, functionEffects, state, block); for (const nextBlockId of eachTerminalSuccessor(block.terminal)) { queue(nextBlockId, state); } } } + + if (!options.isFunctionExpression) { + functionEffects.forEach((eff) => { + switch (eff.kind) { + case "GlobalMutation": + CompilerError.throw(eff.error); + default: + assertExhaustive( + eff.kind, + `Unexpected function effect kind '${eff.kind}'` + ); + } + }); + } else { + fn.effects = functionEffects; + } } // Maintains a mapping of top-level variables to the kind of value they hold @@ -340,7 +360,12 @@ class InferenceState { * Similarly, a freeze reference is converted to readonly if the * value is already frozen or is immutable. */ - reference(place: Place, effectKind: Effect, reason: ValueReason): void { + reference( + place: Place, + functionEffects: Array, + effectKind: Effect, + reason: ValueReason + ): void { const values = this.#variables.get(place.identifier.id); if (values === undefined) { CompilerError.invariant(effectKind !== Effect.Store, { @@ -355,6 +380,16 @@ class InferenceState { : Effect.Read; return; } + + for (const value of values) { + if ( + (value.kind === "FunctionExpression" || + value.kind === "ObjectMethod") && + value.loweredFunc.func.effects != null + ) { + functionEffects.push(...value.loweredFunc.func.effects); + } + } let valueKind: AbstractValue | null = this.kind(place); let effect: Effect | null = null; switch (effectKind) { @@ -382,7 +417,12 @@ class InferenceState { ) { if (value.kind === "FunctionExpression") { for (const operand of eachInstructionValueOperand(value)) { - this.reference(operand, Effect.Freeze, ValueReason.Other); + this.reference( + operand, + functionEffects, + Effect.Freeze, + ValueReason.Other + ); } } } @@ -405,22 +445,25 @@ class InferenceState { } case Effect.Mutate: { if ( - valueKind.kind === ValueKind.Mutable || - valueKind.kind === ValueKind.Context + valueKind.kind !== ValueKind.Mutable && + valueKind.kind !== ValueKind.Context ) { - effect = Effect.Mutate; - } else { let reason = getWriteErrorReason(valueKind); - CompilerError.throwInvalidReact({ - reason, - description: - place.identifier.name !== null - ? `Found mutation of ${place.identifier.name}` - : null, - loc: place.loc, - suggestions: null, + functionEffects.push({ + kind: "GlobalMutation", + error: { + reason, + description: + place.identifier.name !== null + ? `Found mutation of ${place.identifier.name}` + : null, + loc: place.loc, + suggestions: null, + severity: ErrorSeverity.InvalidReact, + }, }); } + effect = Effect.Mutate; break; } case Effect.Store: { @@ -429,15 +472,18 @@ class InferenceState { valueKind.kind !== ValueKind.Context ) { let reason = getWriteErrorReason(valueKind); - - CompilerError.throwInvalidReact({ - reason, - description: - place.identifier.name !== null - ? `Found mutation of ${place.identifier.name}` - : null, - loc: place.loc, - suggestions: null, + functionEffects.push({ + kind: "GlobalMutation", + error: { + reason, + description: + place.identifier.name !== null + ? `Found mutation of ${place.identifier.name}` + : null, + loc: place.loc, + suggestions: null, + severity: ErrorSeverity.InvalidReact, + }, }); } @@ -767,6 +813,7 @@ function mergeAbstractValues( */ function inferBlock( env: Environment, + functionEffects: Array, state: InferenceState, block: BasicBlock ): void { @@ -828,6 +875,7 @@ function inferBlock( // Object keys must be primitives, so we know they're frozen at this point state.reference( property.key.name, + functionEffects, Effect.Freeze, ValueReason.Other ); @@ -835,6 +883,7 @@ function inferBlock( // Object construction captures but does not modify the key/property values state.reference( property.place, + functionEffects, Effect.Capture, ValueReason.Other ); @@ -844,6 +893,7 @@ function inferBlock( // Object construction captures but does not modify the key/property values state.reference( property.place, + functionEffects, Effect.Capture, ValueReason.Other ); @@ -954,6 +1004,7 @@ function inferBlock( for (const operand of eachInstructionOperand(instr)) { state.reference( operand, + functionEffects, operand.effect === Effect.Unknown ? Effect.Read : operand.effect, ValueReason.Other ); @@ -990,29 +1041,49 @@ function inferBlock( } : { kind: ValueKind.Mutable, reason: new Set([ValueReason.Other]) }; let hasCaptureArgument = false; + let isUseEffect = isEffectHook(instrValue.callee.identifier); for (let i = 0; i < instrValue.args.length; i++) { + const argumentEffects: Array = []; const arg = instrValue.args[i]; const place = arg.kind === "Identifier" ? arg : arg.place; if (effects !== null) { - state.reference(place, effects[i], ValueReason.Other); + state.reference( + place, + argumentEffects, + effects[i], + ValueReason.Other + ); } else { state.reference( place, + argumentEffects, Effect.ConditionallyMutate, ValueReason.Other ); } + /* + * Join the effects of the argument with the effects of the enclosing function, + * unless the we're detecting a global mutation inside a useEffect hook + */ + functionEffects.push( + ...argumentEffects.filter( + (argEffect) => + !isUseEffect || i !== 0 || argEffect.kind !== "GlobalMutation" + ) + ); hasCaptureArgument ||= place.effect === Effect.Capture; } if (signature !== null) { state.reference( instrValue.callee, + functionEffects, signature.calleeEffect, ValueReason.Other ); } else { state.reference( instrValue.callee, + functionEffects, Effect.ConditionallyMutate, ValueReason.Other ); @@ -1034,7 +1105,12 @@ function inferBlock( loc: instrValue.loc, suggestions: null, }); - state.reference(instrValue.property, Effect.Read, ValueReason.Other); + state.reference( + instrValue.property, + functionEffects, + Effect.Read, + ValueReason.Other + ); const signature = getFunctionCallSignature( env, @@ -1060,10 +1136,16 @@ function inferBlock( */ for (const arg of instrValue.args) { const place = arg.kind === "Identifier" ? arg : arg.place; - state.reference(place, Effect.Read, ValueReason.Other); + state.reference( + place, + functionEffects, + Effect.Read, + ValueReason.Other + ); } state.reference( instrValue.receiver, + functionEffects, Effect.Capture, ValueReason.Other ); @@ -1087,10 +1169,16 @@ function inferBlock( * If effects are inferred for an argument, we should fail invalid * mutating effects */ - state.reference(place, effects[i], ValueReason.Other); + state.reference( + place, + functionEffects, + effects[i], + ValueReason.Other + ); } else { state.reference( place, + functionEffects, Effect.ConditionallyMutate, ValueReason.Other ); @@ -1100,12 +1188,14 @@ function inferBlock( if (signature !== null) { state.reference( instrValue.receiver, + functionEffects, signature.calleeEffect, ValueReason.Other ); } else { state.reference( instrValue.receiver, + functionEffects, Effect.ConditionallyMutate, ValueReason.Other ); @@ -1124,8 +1214,18 @@ function inferBlock( state.kind(instrValue.object).kind === ValueKind.Context ? Effect.ConditionallyMutate : Effect.Capture; - state.reference(instrValue.value, effect, ValueReason.Other); - state.reference(instrValue.object, Effect.Store, ValueReason.Other); + state.reference( + instrValue.value, + functionEffects, + effect, + ValueReason.Other + ); + state.reference( + instrValue.object, + functionEffects, + Effect.Store, + ValueReason.Other + ); const lvalue = instr.lvalue; state.alias(lvalue, instrValue.value); @@ -1142,7 +1242,12 @@ function inferBlock( break; } case "PropertyLoad": { - state.reference(instrValue.object, Effect.Read, ValueReason.Other); + state.reference( + instrValue.object, + functionEffects, + Effect.Read, + ValueReason.Other + ); const lvalue = instr.lvalue; lvalue.effect = Effect.ConditionallyMutate; state.initialize(instrValue, state.kind(instrValue.object)); @@ -1154,9 +1259,24 @@ function inferBlock( state.kind(instrValue.object).kind === ValueKind.Context ? Effect.ConditionallyMutate : Effect.Capture; - state.reference(instrValue.value, effect, ValueReason.Other); - state.reference(instrValue.property, Effect.Capture, ValueReason.Other); - state.reference(instrValue.object, Effect.Store, ValueReason.Other); + state.reference( + instrValue.value, + functionEffects, + effect, + ValueReason.Other + ); + state.reference( + instrValue.property, + functionEffects, + Effect.Capture, + ValueReason.Other + ); + state.reference( + instrValue.object, + functionEffects, + Effect.Store, + ValueReason.Other + ); const lvalue = instr.lvalue; state.alias(lvalue, instrValue.value); @@ -1164,8 +1284,18 @@ function inferBlock( continue; } case "ComputedDelete": { - state.reference(instrValue.object, Effect.Mutate, ValueReason.Other); - state.reference(instrValue.property, Effect.Read, ValueReason.Other); + state.reference( + instrValue.object, + functionEffects, + Effect.Mutate, + ValueReason.Other + ); + state.reference( + instrValue.property, + functionEffects, + Effect.Read, + ValueReason.Other + ); state.initialize(instrValue, { kind: ValueKind.Immutable, reason: new Set([ValueReason.Other]), @@ -1175,8 +1305,18 @@ function inferBlock( continue; } case "ComputedLoad": { - state.reference(instrValue.object, Effect.Read, ValueReason.Other); - state.reference(instrValue.property, Effect.Read, ValueReason.Other); + state.reference( + instrValue.object, + functionEffects, + Effect.Read, + ValueReason.Other + ); + state.reference( + instrValue.property, + functionEffects, + Effect.Read, + ValueReason.Other + ); const lvalue = instr.lvalue; lvalue.effect = Effect.ConditionallyMutate; state.initialize(instrValue, state.kind(instrValue.object)); @@ -1192,6 +1332,7 @@ function inferBlock( */ state.reference( instrValue.value, + functionEffects, Effect.ConditionallyMutate, ValueReason.Other ); @@ -1210,7 +1351,12 @@ function inferBlock( * ``` */ state.initialize(instrValue, state.kind(instrValue.value)); - state.reference(instrValue.value, Effect.Read, ValueReason.Other); + state.reference( + instrValue.value, + functionEffects, + Effect.Read, + ValueReason.Other + ); const lvalue = instr.lvalue; lvalue.effect = Effect.ConditionallyMutate; state.alias(lvalue, instrValue.value); @@ -1218,9 +1364,19 @@ function inferBlock( } case "Memoize": { if (env.config.enablePreserveExistingMemoizationGuarantees) { - state.reference(instrValue.value, Effect.Freeze, ValueReason.Other); + state.reference( + instrValue.value, + functionEffects, + Effect.Freeze, + ValueReason.Other + ); } else { - state.reference(instrValue.value, Effect.Read, ValueReason.Other); + state.reference( + instrValue.value, + functionEffects, + Effect.Read, + ValueReason.Other + ); } const lvalue = instr.lvalue; lvalue.effect = Effect.ConditionallyMutate; @@ -1238,14 +1394,24 @@ function inferBlock( state.kind(lvalue).kind === ValueKind.Context ? Effect.ConditionallyMutate : Effect.Capture; - state.reference(instrValue.place, effect, ValueReason.Other); + state.reference( + instrValue.place, + functionEffects, + effect, + ValueReason.Other + ); lvalue.effect = Effect.ConditionallyMutate; // direct aliasing: `a = b`; state.alias(lvalue, instrValue.place); continue; } case "LoadContext": { - state.reference(instrValue.place, Effect.Capture, ValueReason.Other); + state.reference( + instrValue.place, + functionEffects, + Effect.Capture, + ValueReason.Other + ); const lvalue = instr.lvalue; lvalue.effect = Effect.ConditionallyMutate; const valueKind = state.kind(instrValue.place); @@ -1297,7 +1463,12 @@ function inferBlock( state.kind(instrValue.lvalue).kind === ValueKind.Context ? Effect.ConditionallyMutate : Effect.Capture; - state.reference(instrValue.value, effect, ValueReason.Other); + state.reference( + instrValue.value, + functionEffects, + effect, + ValueReason.Other + ); const lvalue = instr.lvalue; state.alias(lvalue, instrValue.value); @@ -1318,7 +1489,12 @@ function inferBlock( state.kind(instrValue.lvalue.place).kind === ValueKind.Context ? Effect.ConditionallyMutate : Effect.Capture; - state.reference(instrValue.value, effect, ValueReason.Other); + state.reference( + instrValue.value, + functionEffects, + effect, + ValueReason.Other + ); const lvalue = instr.lvalue; state.alias(lvalue, instrValue.value); @@ -1336,11 +1512,13 @@ function inferBlock( case "StoreContext": { state.reference( instrValue.value, + functionEffects, Effect.ConditionallyMutate, ValueReason.Other ); state.reference( instrValue.lvalue.place, + functionEffects, Effect.Mutate, ValueReason.Other ); @@ -1361,7 +1539,12 @@ function inferBlock( break; } } - state.reference(instrValue.value, effect, ValueReason.Other); + state.reference( + instrValue.value, + functionEffects, + effect, + ValueReason.Other + ); const lvalue = instr.lvalue; state.alias(lvalue, instrValue.value); @@ -1408,7 +1591,7 @@ function inferBlock( loc: instrValue.loc, suggestions: null, }); - state.reference(operand, effect.kind, effect.reason); + state.reference(operand, functionEffects, effect.kind, effect.reason); } state.initialize(instrValue, valueKind); @@ -1430,7 +1613,7 @@ function inferBlock( } else { effect = Effect.Read; } - state.reference(operand, effect, ValueReason.Other); + state.reference(operand, functionEffects, effect, ValueReason.Other); } } diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts index ea92f97ff2..e0d18cc51e 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts @@ -119,7 +119,7 @@ function isUnmemoized(operand: Identifier, scopes: Set): boolean { return operand.scope != null && !scopes.has(operand.scope.id); } -function isEffectHook(identifier: Identifier): boolean { +export function isEffectHook(identifier: Identifier): boolean { return ( isUseEffectHookType(identifier) || isUseLayoutEffectHookType(identifier) || diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.not-useEffect-external-mutate.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.not-useEffect-external-mutate.expect.md new file mode 100644 index 0000000000..ae047130ec --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.not-useEffect-external-mutate.expect.md @@ -0,0 +1,29 @@ + +## Input + +```javascript +let x = { a: 42 }; + +function Component(props) { + foo(() => { + x.a = 10; + x.a = 20; + }); +} + +``` + + +## Error + +``` + 3 | function Component(props) { + 4 | foo(() => { +> 5 | x.a = 10; + | ^ [ReactForget] InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect. (5:5) + 6 | x.a = 20; + 7 | }); + 8 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.not-useEffect-external-mutate.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.not-useEffect-external-mutate.js new file mode 100644 index 0000000000..56e375d430 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.not-useEffect-external-mutate.js @@ -0,0 +1,8 @@ +let x = { a: 42 }; + +function Component(props) { + foo(() => { + x.a = 10; + x.a = 20; + }); +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useEffect-external-mutate.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useEffect-external-mutate.expect.md new file mode 100644 index 0000000000..af9cc829da --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useEffect-external-mutate.expect.md @@ -0,0 +1,51 @@ + +## Input + +```javascript +import { useEffect } from "react"; + +let x = { a: 42 }; + +function Component(props) { + useEffect(() => { + x.a = 10; + }); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + +## Code + +```javascript +import { useEffect, unstable_useMemoCache as useMemoCache } from "react"; + +let x = { a: 42 }; + +function Component(props) { + const $ = useMemoCache(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + x.a = 10; + }; + $[0] = t0; + } else { + t0 = $[0]; + } + useEffect(t0); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + +### Eval output +(kind: ok) \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useEffect-external-mutate.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useEffect-external-mutate.js new file mode 100644 index 0000000000..5e5b7de3b3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useEffect-external-mutate.js @@ -0,0 +1,14 @@ +import { useEffect } from "react"; + +let x = { a: 42 }; + +function Component(props) { + useEffect(() => { + x.a = 10; + }); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +};