From c8d53fcde4b2f6c0c5c81ea24487fd8054d6b512 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Sun, 15 Dec 2024 20:55:10 -0500 Subject: [PATCH] [compile] Error on fire outside of effects and ensure correct compilation Traverse the compiled functions to ensure there are no lingering fires and that all fire calls are inside an effect lambda. -- --- .../src/HIR/PrintHIR.ts | 8 ++ .../src/Transform/TransformFire.ts | 100 ++++++++++++++++-- .../error.mix-fire-and-no-fire.expect.md | 39 +++++++ .../error.mix-fire-and-no-fire.js | 18 ++++ .../error.outside-effect.expect.md | 38 +++++++ .../transform-fire/error.outside-effect.js | 15 +++ 6 files changed, 210 insertions(+), 8 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.mix-fire-and-no-fire.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.mix-fire-and-no-fire.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.outside-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.outside-effect.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 526ab7c7e52bb..a6f6c606e118d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -897,6 +897,14 @@ export function printSourceLocation(loc: SourceLocation): string { } } +export function printSourceLocationLine(loc: SourceLocation): string { + if (typeof loc === 'symbol') { + return 'generated'; + } else { + return `${loc.start.line}:${loc.end.line}`; + } +} + export function printAliases(aliases: DisjointSet): string { const aliasSets = aliases.buildSets(); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts index 5ae21377255ad..1f1514c8abafa 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts @@ -5,7 +5,12 @@ * LICENSE file in the root directory of this source tree. */ -import {CompilerError, CompilerErrorDetailOptions, ErrorSeverity} from '..'; +import { + CompilerError, + CompilerErrorDetailOptions, + ErrorSeverity, + SourceLocation, +} from '..'; import { CallExpression, Effect, @@ -28,14 +33,11 @@ import { import {createTemporaryPlace, markInstructionIds} from '../HIR/HIRBuilder'; import {getOrInsertWith} from '../Utils/utils'; import {BuiltInFireId, DefaultNonmutatingHook} from '../HIR/ObjectShape'; +import {eachInstructionOperand} from '../HIR/visitors'; +import {printSourceLocationLine} from '../HIR/PrintHIR'; /* * TODO(jmbrown): - * In this stack: - * - Assert no lingering fire calls - * - Ensure a fired function is not called regularly elsewhere in the same effect - * - * Future: * - rewrite dep arrays * - traverse object methods * - method calls @@ -187,6 +189,7 @@ type FireCalleesToFireFunctionBinding = Map< { fireFunctionBinding: Place; capturedCalleeIdentifier: Identifier; + fireLoc: SourceLocation; } >; @@ -295,7 +298,11 @@ class Context { return this.#loadLocals.get(id); } - getOrGenerateFireFunctionBinding(callee: Place, env: Environment): Place { + getOrGenerateFireFunctionBinding( + callee: Place, + env: Environment, + fireLoc: SourceLocation, + ): Place { const fireFunctionBinding = getOrInsertWith( this.#fireCalleesToFireFunctions, callee.identifier.id, @@ -305,6 +312,7 @@ class Context { this.#capturedCalleeIdentifierIds.set(callee.identifier.id, { fireFunctionBinding, capturedCalleeIdentifier: callee.identifier, + fireLoc, }); return fireFunctionBinding; @@ -346,8 +354,12 @@ class Context { return this.#loadGlobalInstructionIds.get(id); } + hasErrors(): boolean { + return this.#errors.hasErrors(); + } + throwIfErrorsFound(): void { - if (this.#errors.hasErrors()) throw this.#errors; + if (this.hasErrors()) throw this.#errors; } } @@ -510,6 +522,11 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void { rewriteInstrs.set(loadUseEffectInstrId, newInstrs); } } + ensureNoRemainingCalleeCaptures( + lambda.loweredFunc.func, + context, + capturedCallees, + ); } } else if ( value.kind === 'CallExpression' && @@ -551,6 +568,7 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void { context.getOrGenerateFireFunctionBinding( {...loadLocal.place}, fn.env, + value.loc, ); loadLocal.place = {...fireFunctionBinding}; @@ -626,8 +644,74 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void { } } +/* + * eachInstructionOperand is not sufficient for our cases because: + * 1. fire is a global, which will not appear + * 2. The HIR may be malformed, so can't rely on function deps and must + * traverse the whole function. + */ +function* eachReachablePlace(fn: HIRFunction): Iterable { + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + if ( + instr.value.kind === 'FunctionExpression' || + instr.value.kind === 'ObjectMethod' + ) { + yield* eachReachablePlace(instr.value.loweredFunc.func); + } else { + yield* eachInstructionOperand(instr); + } + } + } +} + +function ensureNoRemainingCalleeCaptures( + fn: HIRFunction, + context: Context, + capturedCallees: FireCalleesToFireFunctionBinding, +): void { + for (const place of eachReachablePlace(fn)) { + const calleeInfo = capturedCallees.get(place.identifier.id); + if (calleeInfo != null) { + const calleeName = + calleeInfo.capturedCalleeIdentifier.name?.kind === 'named' + ? calleeInfo.capturedCalleeIdentifier.name.value + : ''; + context.pushError({ + loc: place.loc, + description: `All uses of ${calleeName} must be either used with a fire() call in \ +this effect or not used with a fire() call at all. ${calleeName} was used with fire() on line \ +${printSourceLocationLine(calleeInfo.fireLoc)} in this effect`, + severity: ErrorSeverity.InvalidReact, + reason: CANNOT_COMPILE_FIRE, + suggestions: null, + }); + } + } +} + +function ensureNoMoreFireUses(fn: HIRFunction, context: Context): void { + for (const place of eachReachablePlace(fn)) { + if ( + place.identifier.type.kind === 'Function' && + place.identifier.type.shapeId === BuiltInFireId + ) { + context.pushError({ + loc: place.identifier.loc, + description: 'Cannot use `fire` outside of a useEffect function', + severity: ErrorSeverity.Invariant, + reason: CANNOT_COMPILE_FIRE, + suggestions: null, + }); + } + } +} + export function transformFire(fn: HIRFunction): void { const context = new Context(); replaceFireFunctions(fn, context); + if (!context.hasErrors()) { + ensureNoMoreFireUses(fn, context); + } context.throwIfErrorsFound(); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.mix-fire-and-no-fire.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.mix-fire-and-no-fire.expect.md new file mode 100644 index 0000000000000..e73451a896ee4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.mix-fire-and-no-fire.expect.md @@ -0,0 +1,39 @@ + +## Input + +```javascript +// @enableFire +import {fire} from 'react'; + +function Component(props) { + const foo = props => { + console.log(props); + }; + useEffect(() => { + function nested() { + fire(foo(props)); + foo(props); + } + + nested(); + }); + + return null; +} + +``` + + +## Error + +``` + 9 | function nested() { + 10 | fire(foo(props)); +> 11 | foo(props); + | ^^^ InvalidReact: Cannot compile `fire`. All uses of foo must be either used with a fire() call in this effect or not used with a fire() call at all. foo was used with fire() on line 10:10 in this effect (11:11) + 12 | } + 13 | + 14 | nested(); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.mix-fire-and-no-fire.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.mix-fire-and-no-fire.js new file mode 100644 index 0000000000000..ee2f915a34ed5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.mix-fire-and-no-fire.js @@ -0,0 +1,18 @@ +// @enableFire +import {fire} from 'react'; + +function Component(props) { + const foo = props => { + console.log(props); + }; + useEffect(() => { + function nested() { + fire(foo(props)); + foo(props); + } + + nested(); + }); + + return null; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.outside-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.outside-effect.expect.md new file mode 100644 index 0000000000000..687a21f98cdb4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.outside-effect.expect.md @@ -0,0 +1,38 @@ + +## Input + +```javascript +// @enableFire +import {fire, useCallback} from 'react'; + +function Component({props, bar}) { + const foo = () => { + console.log(props); + }; + fire(foo(props)); + + useCallback(() => { + fire(foo(props)); + }, [foo, props]); + + return null; +} + +``` + + +## Error + +``` + 6 | console.log(props); + 7 | }; +> 8 | fire(foo(props)); + | ^^^^ Invariant: Cannot compile `fire`. Cannot use `fire` outside of a useEffect function (8:8) + +Invariant: Cannot compile `fire`. Cannot use `fire` outside of a useEffect function (11:11) + 9 | + 10 | useCallback(() => { + 11 | fire(foo(props)); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.outside-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.outside-effect.js new file mode 100644 index 0000000000000..8ac9be6d7648f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.outside-effect.js @@ -0,0 +1,15 @@ +// @enableFire +import {fire, useCallback} from 'react'; + +function Component({props, bar}) { + const foo = () => { + console.log(props); + }; + fire(foo(props)); + + useCallback(() => { + fire(foo(props)); + }, [foo, props]); + + return null; +}