From a06ded902a9d8da050cbbd1ee162ed2c7d83f828 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 27 Feb 2024 22:06:36 -0800 Subject: [PATCH] [other] Change instrumentation to use an optional gating identifier; record filepath Internal rollout currently has a good number of test failures. `enableEmitInstrumentForget` can help developers understand which functions / files they should look at: ``` // input function Foo() { userCode(); // ... } // output function Foo() { if (__DEV__ && inE2eTestMode) { logRender("Foo", "/path/to/filename.js"); } const $ = useMemoCache(...); userCode(); } ``` --- .../components/Editor/EditorImpl.tsx | 2 +- .../scripts/jest/makeTransform.ts | 2 +- .../src/Entrypoint/Pipeline.ts | 15 ++++++----- .../src/Entrypoint/Program.ts | 25 ++++++++++++++----- .../src/HIR/Environment.ts | 18 +++++++++---- .../ReactiveScopes/CodegenReactiveFunction.ts | 21 ++++++++++++---- ...codegen-emit-imports-same-source.expect.md | 9 +++++-- ...en-instrument-forget-gating-test.expect.md | 8 +++--- .../codegen-instrument-forget-test.expect.md | 8 +++--- compiler/packages/snap/src/compiler.ts | 19 ++++++++++---- 10 files changed, 90 insertions(+), 37 deletions(-) diff --git a/compiler/apps/playground/components/Editor/EditorImpl.tsx b/compiler/apps/playground/components/Editor/EditorImpl.tsx index 3e99032434..efeab8773f 100644 --- a/compiler/apps/playground/components/Editor/EditorImpl.tsx +++ b/compiler/apps/playground/components/Editor/EditorImpl.tsx @@ -176,7 +176,7 @@ function compile(source: string): CompilerOutput { for (const result of run(fn, { ...config, customHooks: new Map([...COMMON_HOOKS]), - })) { + }, null)) { const fnName = fn.node.id?.name ?? null; switch (result.kind) { case "ast": { diff --git a/compiler/packages/babel-plugin-react-forget/scripts/jest/makeTransform.ts b/compiler/packages/babel-plugin-react-forget/scripts/jest/makeTransform.ts index 30fde76298..f8c6107d34 100644 --- a/compiler/packages/babel-plugin-react-forget/scripts/jest/makeTransform.ts +++ b/compiler/packages/babel-plugin-react-forget/scripts/jest/makeTransform.ts @@ -168,7 +168,7 @@ function ReactForgetFunctionTransform() { } } - const compiled = compile(fn, forgetOptions); + const compiled = compile(fn, forgetOptions, null); compiledFns.add(compiled); const fun = t.functionDeclaration( diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts index 6225a7adbe..52ecf1c6ce 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -89,7 +89,8 @@ export function* run( func: NodePath< t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression >, - config: EnvironmentConfig + config: EnvironmentConfig, + filename: string | null ): Generator { const contextIdentifiers = findContextIdentifiers(func); const env = new Environment(config, contextIdentifiers); @@ -98,7 +99,7 @@ export function* run( name: "EnvironmentConfig", value: prettyFormat(env.config), }; - const ast = yield* runWithEnvironment(func, env); + const ast = yield* runWithEnvironment(func, env, filename); return ast; } @@ -110,7 +111,8 @@ function* runWithEnvironment( func: NodePath< t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression >, - env: Environment + env: Environment, + filename: string | null ): Generator { const hir = lower(func, env).unwrap(); yield log({ kind: "hir", name: "HIR", value: hir }); @@ -358,7 +360,7 @@ function* runWithEnvironment( validatePreservedManualMemoization(reactiveFunction); } - const ast = codegenFunction(reactiveFunction).unwrap(); + const ast = codegenFunction(reactiveFunction, filename).unwrap(); yield log({ kind: "ast", name: "Codegen", value: ast }); /** @@ -377,9 +379,10 @@ export function compileFn( func: NodePath< t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression >, - config: EnvironmentConfig + config: EnvironmentConfig, + filename: string | null ): CodegenFunction { - let generator = run(func, config); + let generator = run(func, config, filename); while (true) { const next = generator.next(); if (next.done) { diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts index 48794a07b5..5ce2b5f86d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts @@ -252,9 +252,17 @@ export function compileProgram( * TODO(lauren): Remove pass.opts.environment nullcheck once PluginOptions * is validated */ + if (environment.isErr()) { + CompilerError.throwInvalidConfig({ + reason: "Error in validating environment config", + description: environment.unwrapErr().toString(), + suggestions: null, + loc: null, + }); + } const config = environment.unwrap(); - compiledFn = compileFn(fn, config); + compiledFn = compileFn(fn, config, pass.filename); pass.opts.logger?.logEvent(pass.filename, { kind: "CompileSuccess", fnLoc: fn.node.loc ?? null, @@ -319,7 +327,6 @@ export function compileProgram( } const externalFunctions: ExternalFunction[] = []; - let instrumentForget: null | ExternalFunction = null; let gating: null | ExternalFunction = null; try { // TODO: check for duplicate import specifiers @@ -328,11 +335,17 @@ export function compileProgram( externalFunctions.push(gating); } - if (options.environment?.enableEmitInstrumentForget != null) { - instrumentForget = tryParseExternalFunction( - options.environment.enableEmitInstrumentForget + const enableEmitInstrumentForget = + options.environment?.enableEmitInstrumentForget; + if (enableEmitInstrumentForget != null) { + externalFunctions.push( + tryParseExternalFunction(enableEmitInstrumentForget.fn) ); - externalFunctions.push(instrumentForget); + if (enableEmitInstrumentForget.gating != null) { + externalFunctions.push( + tryParseExternalFunction(enableEmitInstrumentForget.gating) + ); + } } if (options.environment?.enableEmitFreeze != null) { diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts index 06f940a029..bed0c5be76 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts @@ -45,6 +45,12 @@ export const ExternalFunctionSchema = z.object({ // Unique name for the feature flag test condition, eg `isForgetEnabled_ProjectName` importSpecifierName: z.string(), }); + +export const InstrumentationSchema = z.object({ + fn: ExternalFunctionSchema, + gating: ExternalFunctionSchema.nullish(), +}); + export type ExternalFunction = z.infer; const HookSchema = z.object({ @@ -254,22 +260,24 @@ const EnvironmentConfigSchema = z.object({ * instrumentation function, for components and hooks that Forget compiles. * For example: * instrumentForget: { - * source: 'react-forget-runtime', - * importSpecifierName: 'useRenderCounter', + * import: { + * source: 'react-forget-runtime', + * importSpecifierName: 'useRenderCounter', + * } * } * * produces: - * import {useRenderCounter} from 'react-forget-runtime-pokes'; + * import {useRenderCounter} from 'react-forget-runtime'; * * function Component(props) { * if (__DEV__) { - * useRenderCounter(); + * useRenderCounter("Component", "/filepath/filename.js"); * } * // ... * } * */ - enableEmitInstrumentForget: ExternalFunctionSchema.nullish(), + enableEmitInstrumentForget: InstrumentationSchema.nullish(), /** * Enable support for reactive scopes that contain an early return. diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts index d9c7ae1c44..38e1e5592a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -72,7 +72,8 @@ export type CodegenFunction = { }; export function codegenFunction( - fn: ReactiveFunction + fn: ReactiveFunction, + filename: string | null ): Result { const cx = new Context(fn.env, fn.id ?? "[[ anonymous ]]", null); const compileResult = codegenReactiveFunction(cx, fn); @@ -112,14 +113,24 @@ export function codegenFunction( if (emitInstrumentForget != null && fn.id != null) { /* * Technically, this is a conditional hook call. However, we expect - * __DEV__ and gatingIdentifier to be runtime constants + * __DEV__ and gating identifier to be runtime constants */ + let gating: t.Expression; + if (emitInstrumentForget.gating != null) { + gating = t.logicalExpression( + "&&", + t.identifier("__DEV__"), + t.identifier(emitInstrumentForget.gating.importSpecifierName) + ); + } else { + gating = t.identifier("__DEV__"); + } const test: t.IfStatement = t.ifStatement( - t.identifier("__DEV__"), + gating, t.expressionStatement( t.callExpression( - t.identifier(emitInstrumentForget.importSpecifierName), - [t.stringLiteral(fn.id)] + t.identifier(emitInstrumentForget.fn.importSpecifierName), + [t.stringLiteral(fn.id), t.stringLiteral(filename ?? "")] ) ) ); diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-emit-imports-same-source.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-emit-imports-same-source.expect.md index 1ee3b6a5a6..7959cf2764 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-emit-imports-same-source.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-emit-imports-same-source.expect.md @@ -13,11 +13,16 @@ function useFoo(props) { ## Code ```javascript -import { useRenderCounter, makeReadOnly } from "react-forget-runtime"; +import { + useRenderCounter, + shouldInstrument, + makeReadOnly, +} from "react-forget-runtime"; import { unstable_useMemoCache as useMemoCache } from "react"; // @enableEmitFreeze @instrumentForget function useFoo(props) { - if (__DEV__) useRenderCounter("useFoo"); + if (__DEV__ && shouldInstrument) + useRenderCounter("useFoo", "/codegen-emit-imports-same-source.ts"); const $ = useMemoCache(2); let t0; if ($[0] !== props.x) { diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-instrument-forget-gating-test.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-instrument-forget-gating-test.expect.md index 107433a589..05fc4caddb 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-instrument-forget-gating-test.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-instrument-forget-gating-test.expect.md @@ -24,11 +24,12 @@ function Foo(props) { ```javascript import { isForgetEnabled_Fixtures } from "ReactForgetFeatureFlag"; -import { useRenderCounter } from "react-forget-runtime"; +import { useRenderCounter, shouldInstrument } from "react-forget-runtime"; import { unstable_useMemoCache as useMemoCache } from "react"; // @instrumentForget @compilationMode(annotation) @gating const Bar = isForgetEnabled_Fixtures() ? function Bar(props) { - if (__DEV__) useRenderCounter("Bar"); + if (__DEV__ && shouldInstrument) + useRenderCounter("Bar", "/codegen-instrument-forget-gating-test.ts"); const $ = useMemoCache(2); let t0; if ($[0] !== props.bar) { @@ -50,7 +51,8 @@ function NoForget(props) { } const Foo = isForgetEnabled_Fixtures() ? function Foo(props) { - if (__DEV__) useRenderCounter("Foo"); + if (__DEV__ && shouldInstrument) + useRenderCounter("Foo", "/codegen-instrument-forget-gating-test.ts"); const $ = useMemoCache(2); let t0; if ($[0] !== props.bar) { diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-instrument-forget-test.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-instrument-forget-test.expect.md index d03a97a01e..ddc79408f7 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-instrument-forget-test.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/codegen-instrument-forget-test.expect.md @@ -23,11 +23,12 @@ function Foo(props) { ## Code ```javascript -import { useRenderCounter } from "react-forget-runtime"; +import { useRenderCounter, shouldInstrument } from "react-forget-runtime"; import { unstable_useMemoCache as useMemoCache } from "react"; // @instrumentForget @compilationMode(annotation) function Bar(props) { - if (__DEV__) useRenderCounter("Bar"); + if (__DEV__ && shouldInstrument) + useRenderCounter("Bar", "/codegen-instrument-forget-test.ts"); const $ = useMemoCache(2); let t0; if ($[0] !== props.bar) { @@ -45,7 +46,8 @@ function NoForget(props) { } function Foo(props) { - if (__DEV__) useRenderCounter("Foo"); + if (__DEV__ && shouldInstrument) + useRenderCounter("Foo", "/codegen-instrument-forget-test.ts"); const $ = useMemoCache(2); let t0; if ($[0] !== props.bar) { diff --git a/compiler/packages/snap/src/compiler.ts b/compiler/packages/snap/src/compiler.ts index e9b58b9824..d8821e6cba 100644 --- a/compiler/packages/snap/src/compiler.ts +++ b/compiler/packages/snap/src/compiler.ts @@ -65,8 +65,14 @@ function makePluginOptions( } if (firstLine.includes("@instrumentForget")) { enableEmitInstrumentForget = { - source: "react-forget-runtime", - importSpecifierName: "useRenderCounter", + fn: { + source: "react-forget-runtime", + importSpecifierName: "useRenderCounter", + }, + gating: { + source: "react-forget-runtime", + importSpecifierName: "shouldInstrument", + }, }; } if (firstLine.includes("@enableEmitFreeze")) { @@ -282,6 +288,9 @@ export function transformFixtureInput( const filename = path.basename(fixturePath) + (language === "typescript" ? ".ts" : ""); const inputAst = parseInput(input, filename, language); + // Give babel transforms an absolute path as relative paths get prefixed + // with `cwd`, which is different across machines + const virtualFilepath = "/" + filename; const presets = language === "typescript" @@ -292,7 +301,7 @@ export function transformFixtureInput( * Get Forget compiled code */ const forgetResult = transformFromAstSync(inputAst, input, { - filename, + filename: virtualFilepath, highlightCode: false, retainLines: true, plugins: [ @@ -324,7 +333,7 @@ export function transformFixtureInput( ); const result = transformFromAstSync(forgetResult.ast, forgetOutput, { presets, - filename, + filename: virtualFilepath, }); if (result?.code == null) { return { @@ -348,7 +357,7 @@ export function transformFixtureInput( try { const result = transformFromAstSync(inputAst, input, { presets, - filename, + filename: virtualFilepath, }); if (result?.code == null) {