From 2a30856ab3be8104cfe0f9ac2e300397de0ef35a Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Wed, 29 May 2024 12:30:05 -0700 Subject: [PATCH] [compiler] Debug tool to emit change detection code rather than memoization Summary: The essential assumption of the compiler is that if the inputs to a computation have not changed, then the output should not change either--computation that the compiler optimizes is idempotent. This is, of course, known to be false in practice, because this property rests on requirements (the Rules of React) that are loosely enforced at best. When rolling out the compiler to a codebase that might have rules of react violations, how should developers debug any issues that arise? This diff attempts one approach to that: when the option is set, rather than simply skipping computation when dependencies haven't changed, we will *still perform the computation*, but will then use a runtime function to compare the original value and the resultant value. The runtime function can be customized, but the idea is that it will perform a structural equality check on the values, and if the values aren't structurally equal, we can report an error, including information about what file and what variable was to blame. This assists in debugging by narrowing down what specific computation is responsible for a difference in behavior between the uncompiled code and the program after compilation. [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 3 +- .../src/Entrypoint/Program.ts | 7 + .../src/HIR/Environment.ts | 8 + .../ReactiveScopes/CodegenReactiveFunction.ts | 62 ++++++- ...-pruned-dependency-change-detect.expect.md | 48 +++++ ...seState-pruned-dependency-change-detect.js | 7 + .../react-compiler-runtime/src/index.ts | 169 +++++++++++++++++- .../packages/snap/src/SproutTodoFilter.ts | 1 + compiler/packages/snap/src/compiler.ts | 8 + 9 files changed, 304 insertions(+), 9 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 303397f6116..a5adc15ee6b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -150,7 +150,8 @@ function* runWithEnvironment( if ( !env.config.enablePreserveExistingManualUseMemo && - !env.config.disableMemoizationForDebugging + !env.config.disableMemoizationForDebugging && + !env.config.enableChangeDetectionForDebugging ) { dropManualMemoization(hir); yield log({ kind: "hir", name: "DropManualMemoization", value: hir }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index dc74077b63a..3d6612afd44 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -421,6 +421,13 @@ export function compileProgram( ); externalFunctions.push(enableEmitHookGuards); } + + if (options.environment?.enableChangeDetectionForDebugging != null) { + const enableChangeDetectionForDebugging = tryParseExternalFunction( + options.environment.enableChangeDetectionForDebugging + ); + externalFunctions.push(enableChangeDetectionForDebugging); + } } catch (err) { handleError(err, pass, null); return; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 6809742d80a..655e9baeeec 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -359,6 +359,14 @@ const EnvironmentConfigSchema = z.object({ */ disableMemoizationForDebugging: z.boolean().default(false), + /** + * When true, rather using memoized values, the compiler will always re-compute + * values, and then use a heuristic to compare the memoized value to the newly + * computed one. This detects cases where rules of react violations may cause the + * compiled code to behave differently than the original. + */ + enableChangeDetectionForDebugging: ExternalFunctionSchema.nullish(), + /** * The react native re-animated library uses custom Babel transforms that * requires the calls to library API remain unmodified. diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index 16ee4fed563..9f1b5edbe32 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -621,16 +621,64 @@ function codegenReactiveScope( t.booleanLiteral(true) ); } - let computationBlock = codegenBlock(cx, block); - computationBlock.body.push(...cacheStoreStatements); + let memoStatement; const memoBlock = t.blockStatement(cacheLoadStatements); + if ( + cx.env.config.enableChangeDetectionForDebugging != null && + changeExpressions.length > 0 + ) { + const detectionFunction = + cx.env.config.enableChangeDetectionForDebugging.importSpecifierName; + const changeDetectionStatements: Array = []; + memoBlock.body.forEach((stmt) => { + if ( + stmt.type === "ExpressionStatement" && + stmt.expression.type === "AssignmentExpression" && + stmt.expression.left.type === "Identifier" + ) { + const name = stmt.expression.left.name; + const loadName = cx.synthesizeName(`old$${name}`); + statements.push( + t.variableDeclaration("let", [ + t.variableDeclarator(t.identifier(loadName)), + ]) + ); + stmt.expression.left = t.identifier(loadName); + changeDetectionStatements.push( + t.expressionStatement( + t.callExpression(t.identifier(detectionFunction), [ + t.identifier(loadName), + t.identifier(name), + t.stringLiteral(name), + t.stringLiteral(cx.fnName), + ]) + ) + ); + changeDetectionStatements.push( + t.expressionStatement( + t.assignmentExpression( + "=", + t.identifier(name), + t.identifier(loadName) + ) + ) + ); + } + }); + memoStatement = t.blockStatement([ + ...computationBlock.body, + t.ifStatement( + t.unaryExpression("!", testCondition), + t.blockStatement([...memoBlock.body, ...changeDetectionStatements]) + ), + ...cacheStoreStatements, + ]); + } else { + computationBlock.body.push(...cacheStoreStatements); - const memoStatement = t.ifStatement( - testCondition, - computationBlock, - memoBlock - ); + memoStatement = t.ifStatement(testCondition, computationBlock, memoBlock); + } if (cx.env.config.enableMemoizationComments) { if (changeExpressionComments.length) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md new file mode 100644 index 00000000000..2aefa2173a1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md @@ -0,0 +1,48 @@ + +## Input + +```javascript +// @enableChangeDetectionForDebugging +import { useState } from "react"; + +function Component(props) { + const [x, _] = useState(f(props.x)); + return
{x}
; +} + +``` + +## Code + +```javascript +import { $structuralCheck } from "react-compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enableChangeDetectionForDebugging +import { useState } from "react"; + +function Component(props) { + const $ = _c(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = f(props.x); + $[0] = t0; + } else { + t0 = $[0]; + } + const [x] = useState(t0); + let t1; + let old$t1; + { + t1 =
{x}
; + if (!($[1] !== x)) { + old$t1 = $[2]; + $structuralCheck(old$t1, t1, "t1", "Component"); + t1 = old$t1; + } + $[1] = x; + $[2] = t1; + } + return t1; +} + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.js new file mode 100644 index 00000000000..46a9c23fe94 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.js @@ -0,0 +1,7 @@ +// @enableChangeDetectionForDebugging +import { useState } from "react"; + +function Component(props) { + const [x, _] = useState(f(props.x)); + return
{x}
; +} diff --git a/compiler/packages/react-compiler-runtime/src/index.ts b/compiler/packages/react-compiler-runtime/src/index.ts index 743b7633aa1..aca78194ef2 100644 --- a/compiler/packages/react-compiler-runtime/src/index.ts +++ b/compiler/packages/react-compiler-runtime/src/index.ts @@ -9,7 +9,7 @@ import * as React from "react"; -const { useRef, useEffect } = React; +const { useRef, useEffect, isValidElement } = React; const ReactSecretInternals = //@ts-ignore React.__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE ?? @@ -251,3 +251,170 @@ export function useRenderCounter(name: string): void { }; }); } + +const seenErrors = new Set(); + +export function $structuralCheck( + oldValue: any, + newValue: any, + variableName: string, + fnName: string +): void { + function error(l: string, r: string, path: string, depth: number) { + const str = `${fnName}: ${variableName}${path} changed from ${l} to ${r} at depth ${depth}`; + if (seenErrors.has(str)) { + return; + } + seenErrors.add(str); + console.error(str); + } + const depthLimit = 2; + function recur(oldValue: any, newValue: any, path: string, depth: number) { + if (depth > depthLimit) { + return; + } else if (oldValue === newValue) { + return; + } else if (typeof oldValue !== typeof newValue) { + error(`type ${typeof oldValue}`, `type ${typeof newValue}`, path, depth); + } else if (typeof oldValue === "object") { + const oldArray = Array.isArray(oldValue); + const newArray = Array.isArray(newValue); + if (oldValue === null && newValue !== null) { + error("null", `type ${typeof newValue}`, path, depth); + } else if (newValue === null) { + error(`type ${typeof oldValue}`, null, path, depth); + } else if (oldValue instanceof Map) { + if (!(newValue instanceof Map)) { + error(`Map instance`, `other value`, path, depth); + } else if (oldValue.size !== newValue.size) { + error( + `Map instance with size ${oldValue.size}`, + `Map instance with size ${newValue.size}`, + path, + depth + ); + } else { + for (const [k, v] of oldValue) { + if (!newValue.has(k)) { + error( + `Map instance with key ${k}`, + `Map instance without key ${k}`, + path, + depth + ); + } else { + recur(v, newValue.get(k), `${path}.get(${k})`, depth + 1); + } + } + } + } else if (newValue instanceof Map) { + error("other value", `Map instance`, path, depth); + } else if (oldValue instanceof Set) { + if (!(newValue instanceof Set)) { + error(`Set instance`, `other value`, path, depth); + } else if (oldValue.size !== newValue.size) { + error( + `Set instance with size ${oldValue.size}`, + `Set instance with size ${newValue.size}`, + path, + depth + ); + } else { + for (const v of newValue) { + if (!oldValue.has(v)) { + error( + `Set instance without element ${v}`, + `Set instance with element ${v}`, + path, + depth + ); + } + } + } + } else if (newValue instanceof Set) { + error("other value", `Set instance`, path, depth); + } else if (oldArray || newArray) { + if (oldArray !== newArray) { + error( + `type ${oldArray ? "array" : "object"}`, + `type ${newArray ? "array" : "object"}`, + path, + depth + ); + } else if (oldValue.length !== newValue.length) { + error( + `array with length ${oldValue.length}`, + `array with length ${newValue.length}`, + path, + depth + ); + } else { + for (let ii = 0; ii < oldValue.length; ii++) { + recur(oldValue[ii], newValue[ii], `${path}[${ii}]`, depth + 1); + } + } + } else if (isValidElement(oldValue) || isValidElement(newValue)) { + if (isValidElement(oldValue) !== isValidElement(newValue)) { + error( + `type ${isValidElement(oldValue) ? "React element" : "object"}`, + `type ${isValidElement(newValue) ? "React element" : "object"}`, + path, + depth + ); + } else if (oldValue.type !== newValue.type) { + error( + `React element of type ${oldValue.type}`, + `React element of type ${newValue.type}`, + path, + depth + ); + } else { + recur( + oldValue.props, + newValue.props, + `[props of ${path}]`, + depth + 1 + ); + } + } else { + for (const key in newValue) { + if (!(key in oldValue)) { + error( + `object without key ${key}`, + `object with key ${key}`, + path, + depth + ); + } + } + for (const key in oldValue) { + if (!(key in newValue)) { + error( + `object with key ${key}`, + `object without key ${key}`, + path, + depth + ); + } else { + recur(oldValue[key], newValue[key], `${path}.${key}`, depth + 1); + } + } + } + } else if (typeof oldValue === "function") { + // Bail on functions for now + return; + } else if (isNaN(oldValue) || isNaN(newValue)) { + if (isNaN(oldValue) !== isNaN(newValue)) { + error( + `${isNaN(oldValue) ? "NaN" : "non-NaN value"}`, + `${isNaN(newValue) ? "NaN" : "non-NaN value"}`, + path, + depth + ); + } + } else if (oldValue !== newValue) { + error(oldValue, newValue, path, depth); + } + } + recur(oldValue, newValue, "", 0); +} diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 14ed51b2cc0..c6fdc12b727 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -495,6 +495,7 @@ const skipFilter = new Set([ "flag-enable-emit-hook-guards", "fast-refresh-refresh-on-const-changes-dev", + "useState-pruned-dependency-change-detect", ]); export default skipFilter; diff --git a/compiler/packages/snap/src/compiler.ts b/compiler/packages/snap/src/compiler.ts index 29c85a053dd..ab7a50d12ae 100644 --- a/compiler/packages/snap/src/compiler.ts +++ b/compiler/packages/snap/src/compiler.ts @@ -45,6 +45,7 @@ function makePluginOptions( let validatePreserveExistingMemoizationGuarantees = false; let enablePreserveExistingManualUseMemo = false; let disableMemoizationForDebugging = false; + let enableChangeDetectionForDebugging = null; if (firstLine.indexOf("@compilationMode(annotation)") !== -1) { assert( @@ -128,6 +129,12 @@ function makePluginOptions( if (firstLine.includes("@disableMemoizationForDebugging")) { disableMemoizationForDebugging = true; } + if (firstLine.includes("@enableChangeDetectionForDebugging")) { + enableChangeDetectionForDebugging = { + source: "react-compiler-runtime", + importSpecifierName: "$structuralCheck", + }; + } const hookPatternMatch = /@hookPattern:"([^"]+)"/.exec(firstLine); if ( hookPatternMatch && @@ -183,6 +190,7 @@ function makePluginOptions( validatePreserveExistingMemoizationGuarantees, enablePreserveExistingManualUseMemo, disableMemoizationForDebugging, + enableChangeDetectionForDebugging, }, compilationMode, logger: null,