Promote enableOptimizeFunctionExpressions to default/only option
We’ve had this feature turned on internally for a while with only a couple minor bugs and no fundamental issues. It’s a necessary change for simplifying function expression dependencies and context variables, so let’s remove the feature flag and fix forward for any issues.
Joe Savona committed
Aug 18, 2023 at 12:06 UTC
bcd692fca43543f765c222077ea9fef275d49389
11 files changed
+55
-109
compiler/forget/apps/playground/components/Editor/index.tsx
-5
@@ -115,7 +115,6 @@ function parsePragma(pragma: string) {
115
let disableAllMemoization = false;
116
let validateRefAccessDuringRender = true;
117
let enableEmitFreeze = null;
118
- let enableOptimizeFunctionExpressions = true;
118
let inlineUseMemo = true;
119
let validateHooksUsage = true;
120
let enableFunctionCallSignatureOptimizations = true;
@@ -137,9 +136,6 @@ function parsePragma(pragma: string) {
136
if (pragma.includes("@validateRefAccessDuringRender false")) {
137
validateRefAccessDuringRender = false;
138
}
140
- if (pragma.includes("@enableOptimizeFunctionExpressions false")) {
141
- enableOptimizeFunctionExpressions = false;
142
- }
139
if (pragma.includes("@enableEmitFreeze")) {
140
enableEmitFreeze = {
141
source: "react-forget-runtime",
@@ -173,7 +169,6 @@ function parsePragma(pragma: string) {
169
validateRefAccessDuringRender,
170
validateFrozenLambdas,
171
enableEmitFreeze,
176
- enableOptimizeFunctionExpressions,
172
assertValidMutableRanges,
173
};
174
}
compiler/forget/packages/babel-plugin-react-forget/src/HIR/Environment.ts
-12
@@ -159,15 +159,6 @@ export type EnvironmentConfig = Partial<{
159
*/
160
enableEmitFreeze: ExternalFunction | null;
161
162
- /**
163
- * When enabled, function expression codegen uses a subset of the compiler pipeline
164
- * to transform and optimize their contents. When disabled, function expression
165
- * codegen uses the original, un-transformed function body.
166
- *
167
- * Defaults to false (use the un-transformed function body).
168
- */
169
- enableOptimizeFunctionExpressions: boolean;
170
-
162
/**
163
* Enable validation of mutable ranges
164
*
@@ -190,7 +181,6 @@ export class Environment {
181
enableTreatHooksAsFunctions: boolean;
182
disableAllMemoization: boolean;
183
enableEmitFreeze: ExternalFunction | null;
193
- enableOptimizeFunctionExpressions: boolean;
184
assertValidMutableRanges: boolean;
185
186
#contextIdentifiers: Set<t.Identifier>;
@@ -237,8 +227,6 @@ export class Environment {
227
config?.enableTreatHooksAsFunctions ?? true;
228
this.disableAllMemoization = config?.disableAllMemoization ?? false;
229
this.enableEmitFreeze = config?.enableEmitFreeze ?? null;
240
- this.enableOptimizeFunctionExpressions =
241
- config?.enableOptimizeFunctionExpressions ?? true;
230
this.assertValidMutableRanges = config?.assertValidMutableRanges ?? false;
231
this.validateNoSetStateInRender =
232
config?.validateNoSetStateInRender ?? false;
compiler/forget/packages/babel-plugin-react-forget/src/HIR/MergeConsecutiveBlocks.ts
+3
-5
@@ -31,11 +31,9 @@ import { mapOptionalFallthroughs } from "./visitors";
31
export function mergeConsecutiveBlocks(fn: HIRFunction): void {
32
const merged = new MergedBlocks();
33
for (const [, block] of fn.body.blocks) {
34
- if (fn.env.enableOptimizeFunctionExpressions) {
35
- for (const instr of block.instructions) {
36
- if (instr.value.kind === "FunctionExpression") {
37
- mergeConsecutiveBlocks(instr.value.loweredFunc);
38
- }
34
+ for (const instr of block.instructions) {
35
+ if (instr.value.kind === "FunctionExpression") {
36
+ mergeConsecutiveBlocks(instr.value.loweredFunc);
37
}
38
}
39
compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts
+2
-12
@@ -14,14 +14,12 @@ import {
14
isRefValueType,
15
isSetStateType,
16
isUseRefType,
17
- mergeConsecutiveBlocks,
17
Place,
18
ReactiveScopeDependency,
19
} from "../HIR";
21
-import { constantPropagation, deadCodeElimination } from "../Optimization";
20
+import { deadCodeElimination } from "../Optimization";
21
import { inferReactiveScopeVariables } from "../ReactiveScopes";
23
-import { eliminateRedundantPhi, enterSSA, leaveSSA } from "../SSA";
24
-import { inferTypes } from "../TypeInference";
22
+import { leaveSSA } from "../SSA";
23
import { logHIRFunction } from "../Utils/logger";
24
import { inferMutableContextVariables } from "./InferMutableContextVariables";
25
import { inferMutableRanges } from "./InferMutableRanges";
@@ -104,14 +102,6 @@ export default function analyseFunctions(func: HIRFunction): void {
102
}
103
104
function lower(func: HIRFunction): void {
107
- if (!func.env.enableOptimizeFunctionExpressions) {
108
- mergeConsecutiveBlocks(func);
109
- enterSSA(func);
110
- eliminateRedundantPhi(func);
111
- constantPropagation(func);
112
- inferTypes(func);
113
- }
114
-
105
analyseFunctions(func);
106
inferReferenceEffects(func, { isFunctionExpression: true });
107
deadCodeElimination(func);
compiler/forget/packages/babel-plugin-react-forget/src/Optimization/ConstantPropagation.ts
+1
-9
@@ -131,12 +131,6 @@ function applyConstantPropagation(
131
continue;
132
}
133
const instr = block.instructions[i]!;
134
- if (!fn.env.enableOptimizeFunctionExpressions) {
135
- // Don't propagate constants used as function expression dependencies
136
- if (functionDependencies.has(instr.lvalue.identifier.id)) {
137
- continue;
138
- }
139
- }
134
const value = evaluateInstruction(fn.env, constants, instr);
135
if (value !== null) {
136
constants.set(instr.lvalue.identifier.id, value);
@@ -438,9 +432,7 @@ function evaluateInstruction(
432
return placeValue;
433
}
434
case "FunctionExpression": {
441
- if (env.enableOptimizeFunctionExpressions) {
442
- constantPropagationImpl(value.loweredFunc, constants);
443
- }
435
+ constantPropagationImpl(value.loweredFunc, constants);
436
return null;
437
}
438
default: {
compiler/forget/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts
+21
-25
@@ -993,34 +993,30 @@ function codegenInstructionValue(
993
break;
994
}
995
case "FunctionExpression": {
996
- if (cx.env.enableOptimizeFunctionExpressions) {
997
- const loweredFunc = instrValue.loweredFunc;
998
- const reactiveFunction = buildReactiveFunction(loweredFunc);
999
- pruneUnusedLabels(reactiveFunction);
1000
- pruneUnusedLValues(reactiveFunction);
1001
- renameVariables(reactiveFunction);
1002
- const fn = codegenReactiveFunction(reactiveFunction).unwrap();
1003
- if (instrValue.expr.type === "ArrowFunctionExpression") {
1004
- let body: t.BlockStatement | t.Expression = fn.body;
1005
- if (body.body.length === 1) {
1006
- const stmt = body.body[0]!;
1007
- if (stmt.type === "ReturnStatement" && stmt.argument != null) {
1008
- body = stmt.argument;
1009
- }
996
+ const loweredFunc = instrValue.loweredFunc;
997
+ const reactiveFunction = buildReactiveFunction(loweredFunc);
998
+ pruneUnusedLabels(reactiveFunction);
999
+ pruneUnusedLValues(reactiveFunction);
1000
+ renameVariables(reactiveFunction);
1001
+ const fn = codegenReactiveFunction(reactiveFunction).unwrap();
1002
+ if (instrValue.expr.type === "ArrowFunctionExpression") {
1003
+ let body: t.BlockStatement | t.Expression = fn.body;
1004
+ if (body.body.length === 1) {
1005
+ const stmt = body.body[0]!;
1006
+ if (stmt.type === "ReturnStatement" && stmt.argument != null) {
1007
+ body = stmt.argument;
1008
}
1011
- value = t.arrowFunctionExpression(fn.params, body, fn.async);
1012
- } else {
1013
- value = t.functionExpression(
1014
- fn.id ??
1015
- (instrValue.name != null ? t.identifier(instrValue.name) : null),
1016
- fn.params,
1017
- fn.body,
1018
- fn.generator,
1019
- fn.async
1020
- );
1009
}
1010
+ value = t.arrowFunctionExpression(fn.params, body, fn.async);
1011
} else {
1023
- value = t.cloneNode(instrValue.expr, true, false);
1012
+ value = t.functionExpression(
1013
+ fn.id ??
1014
+ (instrValue.name != null ? t.identifier(instrValue.name) : null),
1015
+ fn.params,
1016
+ fn.body,
1017
+ fn.generator,
1018
+ fn.async
1019
+ );
1020
}
1021
break;
1022
}
compiler/forget/packages/babel-plugin-react-forget/src/SSA/EliminateRedundantPhi.ts
+1
-4
@@ -106,10 +106,7 @@ export function eliminateRedundantPhi(
106
rewritePlace(place, rewrites);
107
}
108
109
- if (
110
- instr.value.kind === "FunctionExpression" &&
111
- fn.env.enableOptimizeFunctionExpressions
112
- ) {
109
+ if (instr.value.kind === "FunctionExpression") {
110
const { context } = instr.value.loweredFunc;
111
for (const place of context) {
112
rewritePlace(place, rewrites);
compiler/forget/packages/babel-plugin-react-forget/src/SSA/EnterSSA.ts
+7
-14
@@ -250,16 +250,12 @@ function enterSSAImpl(
250
251
if (blockId === rootEntry) {
252
// NOTE: func.context should be empty for the root function
253
- if (func.env.enableOptimizeFunctionExpressions) {
254
- CompilerError.invariant(func.context.length === 0, {
255
- reason: `Expected function context to be empty for outer function declarations`,
256
- description: null,
257
- loc: func.loc,
258
- suggestions: null,
259
- });
260
- } else {
261
- func.context = func.context.map((p) => builder.defineContext(p));
262
- }
253
+ CompilerError.invariant(func.context.length === 0, {
254
+ reason: `Expected function context to be empty for outer function declarations`,
255
+ description: null,
256
+ loc: func.loc,
257
+ suggestions: null,
258
+ });
259
func.params = func.params.map((p) => builder.definePlace(p));
260
}
261
@@ -267,10 +263,7 @@ function enterSSAImpl(
263
mapInstructionOperands(instr, (place) => builder.getPlace(place));
264
mapInstructionLValues(instr, (lvalue) => builder.definePlace(lvalue));
265
270
- if (
271
- instr.value.kind === "FunctionExpression" &&
272
- func.env.enableOptimizeFunctionExpressions
273
- ) {
266
+ if (instr.value.kind === "FunctionExpression") {
267
const loweredFunc = instr.value.loweredFunc;
268
const entry = loweredFunc.body.blocks.get(loweredFunc.body.entry)!;
269
CompilerError.invariant(entry.preds.size === 0, {
compiler/forget/packages/babel-plugin-react-forget/src/TypeInference/InferTypes.ts
+2
-7
@@ -68,10 +68,7 @@ function apply(func: HIRFunction, unifier: Unifier): void {
68
const { lvalue, value } = instr;
69
lvalue.identifier.type = unifier.get(lvalue.identifier.type);
70
71
- if (
72
- value.kind === "FunctionExpression" &&
73
- func.env.enableOptimizeFunctionExpressions
74
- ) {
71
+ if (value.kind === "FunctionExpression") {
72
apply(value.loweredFunc, unifier);
73
}
74
}
@@ -259,9 +256,7 @@ function* generateInstructionTypes(
256
}
257
258
case "FunctionExpression": {
262
- if (env.enableOptimizeFunctionExpressions) {
263
- yield* generate(value.loweredFunc);
264
- }
259
+ yield* generate(value.loweredFunc);
260
break;
261
}
262
compiler/forget/packages/fixture-test-utils/src/compiler-utils.ts
-5
@@ -32,7 +32,6 @@ export function transformFixtureInput(
32
let validateRefAccessDuringRender = true;
33
let validateNoSetStateInRender = true;
34
let enableEmitFreeze = null;
35
- let enableOptimizeFunctionExpressions = true;
35
let enableOnlyOnReactScript = false;
36
37
if (firstLine.indexOf("@forgetDirective") !== -1) {
@@ -71,9 +70,6 @@ export function transformFixtureInput(
70
if (firstLine.includes("@validateNoSetStateInRender false")) {
71
validateNoSetStateInRender = false;
72
}
74
- if (firstLine.includes("@enableOptimizeFunctionExpressions false")) {
75
- enableOptimizeFunctionExpressions = false;
76
- }
73
if (firstLine.includes("@enableEmitFreeze")) {
74
enableEmitFreeze = {
75
source: "react-forget-runtime",
@@ -111,7 +107,6 @@ export function transformFixtureInput(
107
validateFrozenLambdas: true,
108
validateNoSetStateInRender,
109
enableEmitFreeze,
114
- enableOptimizeFunctionExpressions,
110
assertValidMutableRanges: true,
111
},
112
enableOnlyOnUseForgetDirective,
compiler/forget/packages/snap/src/compiler-worker.ts
+18
-11
@@ -5,8 +5,12 @@
5
* LICENSE file in the root directory of this source tree.
6
*/
7
8
-import type { runReactForgetBabelPlugin as RunReactForgetBabelPlugin } from 'babel-plugin-react-forget/src/Babel/RunReactForgetBabelPlugin';
9
-import { TestFixture, transformFixtureInput, writeOutputToString } from 'fixture-test-utils';
8
+import type { runReactForgetBabelPlugin as RunReactForgetBabelPlugin } from "babel-plugin-react-forget/src/Babel/RunReactForgetBabelPlugin";
9
+import {
10
+ TestFixture,
11
+ transformFixtureInput,
12
+ writeOutputToString,
13
+} from "fixture-test-utils";
14
import fs from "fs/promises";
15
16
const originalConsoleError = console.error;
@@ -47,13 +51,10 @@ export async function compile(
51
clearRequireCache();
52
}
53
version = compilerVersion;
50
- const { inputPath, inputExists, outputPath, outputExists, basename } = fixture;
51
- const input = inputExists
52
- ? await fs.readFile(inputPath, "utf8")
53
- : null;
54
- const expected = outputExists
55
- ? await fs.readFile(outputPath, "utf8")
56
- : null;
54
+ const { inputPath, inputExists, outputPath, outputExists, basename } =
55
+ fixture;
56
+ const input = inputExists ? await fs.readFile(inputPath, "utf8") : null;
57
+ const expected = outputExists ? await fs.readFile(outputPath, "utf8") : null;
58
59
// Input will be null if the input file did not exist, in which case the output file
60
// is stale
@@ -72,14 +73,20 @@ export async function compile(
73
try {
74
// NOTE: we intentionally require lazily here so that we can clear the require cache
75
// and load fresh versions of the compiler when `compilerVersion` changes.
75
- const { runReactForgetBabelPlugin } = require(compilerPath) as { runReactForgetBabelPlugin: typeof RunReactForgetBabelPlugin };
76
+ const { runReactForgetBabelPlugin } = require(compilerPath) as {
77
+ runReactForgetBabelPlugin: typeof RunReactForgetBabelPlugin;
78
+ };
79
const { toggleLogging } = require(loggerPath);
80
81
// only try logging if we filtered out all but one fixture,
82
// since console log order is non-deterministic
83
const shouldLogPragma = input.split("\n")[0].includes("@debug");
84
toggleLogging(isOnlyFixture && shouldLogPragma);
82
- code = transformFixtureInput(input, basename, runReactForgetBabelPlugin).code;
85
+ code = transformFixtureInput(
86
+ input,
87
+ basename,
88
+ runReactForgetBabelPlugin
89
+ ).code;
90
} catch (e) {
91
error = e;
92
}