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 e5005d02c4ef7..7f8640122b942 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -103,6 +103,7 @@ import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoF import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects'; import {inferMutationAliasingRanges} from '../Inference/InferMutationAliasingRanges'; import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects'; +import {nameAnonymousFunctions} from '../Transform/NameAnonymousFunctions'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -414,6 +415,15 @@ function runWithEnvironment( }); } + if (env.config.enableNameAnonymousFunctions) { + nameAnonymousFunctions(hir); + log({ + kind: 'hir', + name: 'NameAnonymougFunctions', + value: hir, + }); + } + const reactiveFunction = buildReactiveFunction(hir); log({ kind: 'reactive', diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index 02607f27b4e60..4d0461fe105b0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -3566,6 +3566,8 @@ function lowerFunctionToValue( let name: string | null = null; if (expr.isFunctionExpression()) { name = expr.get('id')?.node?.name ?? null; + } else if (expr.isFunctionDeclaration()) { + name = expr.get('id')?.node?.name ?? null; } const loweredFunc = lowerFunction(builder, expr); if (!loweredFunc) { 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 e38a49cb56a06..8e6816a3d5128 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -261,6 +261,8 @@ export const EnvironmentConfigSchema = z.object({ enableFire: z.boolean().default(false), + enableNameAnonymousFunctions: z.boolean().default(false), + /** * Enables inference and auto-insertion of effect dependencies. Takes in an array of * configurable module and import pairs to allow for user-land experimentation. For example, diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 582ef93cf34f2..fa502f821d0b7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -15,6 +15,7 @@ import {Type, makeType} from './Types'; import {z} from 'zod'; import type {AliasingEffect} from '../Inference/AliasingEffects'; import {isReservedWord} from '../Utils/Keyword'; +import {Err, Ok, Result} from '../Utils/Result'; /* * ******************************************************************************************* @@ -1298,6 +1299,15 @@ export function forkTemporaryIdentifier( }; } +export function validateIdentifierName( + name: string, +): Result { + if (isReservedWord(name) || !t.isValidIdentifier(name)) { + return Err(null); + } + return Ok(makeIdentifierName(name).value); +} + /** * Creates a valid identifier name. This should *not* be used for synthesizing * identifier names: only call this method for identifier names that appear in the 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 9d6f46c2374a7..28d8afd84b4fe 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -43,6 +43,7 @@ import { ValidIdentifierName, getHookKind, makeIdentifierName, + validateIdentifierName, } from '../HIR/HIR'; import {printIdentifier, printInstruction, printPlace} from '../HIR/PrintHIR'; import {eachPatternOperand} from '../HIR/visitors'; @@ -2326,6 +2327,11 @@ function codegenInstructionValue( ), reactiveFunction, ).unwrap(); + + const validatedName = + instrValue.name != null + ? validateIdentifierName(instrValue.name) + : Err(null); if (instrValue.type === 'ArrowFunctionExpression') { let body: t.BlockStatement | t.Expression = fn.body; if (body.body.length === 1 && loweredFunc.directives.length == 0) { @@ -2337,14 +2343,28 @@ function codegenInstructionValue( value = t.arrowFunctionExpression(fn.params, body, fn.async); } else { value = t.functionExpression( - fn.id ?? - (instrValue.name != null ? t.identifier(instrValue.name) : null), + validatedName + .map(name => t.identifier(name)) + .unwrapOr(null), fn.params, fn.body, fn.generator, fn.async, ); } + if ( + cx.env.config.enableNameAnonymousFunctions && + validatedName.isErr() && + instrValue.name != null + ) { + const name = instrValue.name; + value = t.memberExpression( + t.objectExpression([t.objectProperty(t.stringLiteral(name), value)]), + t.stringLiteral(name), + true, + false, + ); + } break; } case 'TaggedTemplateExpression': { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Transform/NameAnonymousFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Transform/NameAnonymousFunctions.ts new file mode 100644 index 0000000000000..5fed3af0f733f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Transform/NameAnonymousFunctions.ts @@ -0,0 +1,173 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import { + FunctionExpression, + getHookKind, + HIRFunction, + IdentifierId, +} from '../HIR'; + +export function nameAnonymousFunctions(fn: HIRFunction): void { + if (fn.id == null) { + return; + } + const parentName = fn.id; + const functions = nameAnonymousFunctionsImpl(fn); + function visit(node: Node, prefix: string): void { + if (node.generatedName != null) { + /** + * Note that we don't generate a name for functions that already had one, + * so we'll only add the prefix to anonymous functions regardless of + * nesting depth. + */ + const name = `${prefix}${node.generatedName}]`; + node.fn.name = name; + } + /** + * Whether or not we generated a name for the function at this node, + * traverse into its nested functions to assign them names + */ + const nextPrefix = `${prefix}${node.generatedName ?? node.fn.name ?? ''} > `; + for (const inner of node.inner) { + visit(inner, nextPrefix); + } + } + for (const node of functions) { + visit(node, `${parentName}[`); + } +} + +type Node = { + fn: FunctionExpression; + generatedName: string | null; + inner: Array; +}; + +function nameAnonymousFunctionsImpl(fn: HIRFunction): Array { + // Functions that we track to generate names for + const functions: Map = new Map(); + // Tracks temporaries that read from variables/globals/properties + const names: Map = new Map(); + // Tracks all function nodes to bubble up for later renaming + const nodes: Array = []; + for (const block of fn.body.blocks.values()) { + for (const instr of block.instructions) { + const {lvalue, value} = instr; + switch (value.kind) { + case 'LoadGlobal': { + names.set(lvalue.identifier.id, value.binding.name); + break; + } + case 'LoadContext': + case 'LoadLocal': { + const name = value.place.identifier.name; + if (name != null && name.kind === 'named') { + names.set(lvalue.identifier.id, name.value); + } + break; + } + case 'PropertyLoad': { + const objectName = names.get(value.object.identifier.id); + if (objectName != null) { + names.set( + lvalue.identifier.id, + `${objectName}.${String(value.property)}`, + ); + } + break; + } + case 'FunctionExpression': { + const inner = nameAnonymousFunctionsImpl(value.loweredFunc.func); + const node: Node = { + fn: value, + generatedName: null, + inner, + }; + /** + * Bubble-up all functions, even if they're named, so that we can + * later generate names for any inner anonymous functions + */ + nodes.push(node); + if (value.name == null) { + // but only generate names for anonymous functions + functions.set(lvalue.identifier.id, node); + } + break; + } + case 'StoreContext': + case 'StoreLocal': { + const node = functions.get(value.value.identifier.id); + const variableName = value.lvalue.place.identifier.name; + if ( + node != null && + variableName != null && + variableName.kind === 'named' + ) { + node.generatedName = variableName.value; + functions.delete(value.value.identifier.id); + } + break; + } + case 'CallExpression': + case 'MethodCall': { + const callee = + value.kind === 'MethodCall' ? value.property : value.callee; + const hookKind = getHookKind(fn.env, callee.identifier); + let calleeName: string | null = null; + if (hookKind != null && hookKind !== 'Custom') { + calleeName = hookKind; + } else { + calleeName = names.get(callee.identifier.id) ?? '(anonymous)'; + } + let fnArgCount = 0; + for (const arg of value.args) { + if (arg.kind === 'Identifier' && functions.has(arg.identifier.id)) { + fnArgCount++; + } + } + for (let i = 0; i < value.args.length; i++) { + const arg = value.args[i]!; + if (arg.kind === 'Spread') { + continue; + } + const node = functions.get(arg.identifier.id); + if (node != null) { + const generatedName = + fnArgCount > 1 ? `${calleeName}(arg${i})` : `${calleeName}()`; + node.generatedName = generatedName; + functions.delete(arg.identifier.id); + } + } + break; + } + case 'JsxExpression': { + for (const attr of value.props) { + if (attr.kind === 'JsxSpreadAttribute') { + continue; + } + const node = functions.get(attr.place.identifier.id); + if (node != null) { + const elementName = + value.tag.kind === 'BuiltinTag' + ? value.tag.name + : (names.get(value.tag.identifier.id) ?? null); + const propName = + elementName == null + ? attr.name + : `<${elementName}>.${attr.name}`; + node.generatedName = `${propName}`; + functions.delete(attr.place.identifier.id); + } + } + break; + } + } + } + } + return nodes; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 49d8a85a96b3c..19af0ed080142 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -18,7 +18,6 @@ import { IdentifierId, InstructionValue, ManualMemoDependency, - Place, PrunedReactiveScopeBlock, ReactiveFunction, ReactiveInstruction, @@ -29,7 +28,10 @@ import { SourceLocation, } from '../HIR'; import {printIdentifier, printManualMemoDependency} from '../HIR/PrintHIR'; -import {eachInstructionValueOperand} from '../HIR/visitors'; +import { + eachInstructionValueLValue, + eachInstructionValueOperand, +} from '../HIR/visitors'; import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization'; import { ReactiveFunctionVisitor, @@ -337,56 +339,53 @@ class Visitor extends ReactiveFunctionVisitor { * @returns a @{ManualMemoDependency} representing the variable + * property reads represented by @value */ - recordDepsInValue( - value: ReactiveValue, - state: VisitorState, - ): ManualMemoDependency | null { + recordDepsInValue(value: ReactiveValue, state: VisitorState): void { switch (value.kind) { case 'SequenceExpression': { for (const instr of value.instructions) { this.visitInstruction(instr, state); } - const result = this.recordDepsInValue(value.value, state); - return result; + this.recordDepsInValue(value.value, state); + break; } case 'OptionalExpression': { - return this.recordDepsInValue(value.value, state); + this.recordDepsInValue(value.value, state); + break; } case 'ConditionalExpression': { this.recordDepsInValue(value.test, state); this.recordDepsInValue(value.consequent, state); this.recordDepsInValue(value.alternate, state); - return null; + break; } case 'LogicalExpression': { this.recordDepsInValue(value.left, state); this.recordDepsInValue(value.right, state); - return null; + break; } default: { - const dep = collectMaybeMemoDependencies( - value, - this.temporaries, - false, - ); - if (value.kind === 'StoreLocal' || value.kind === 'StoreContext') { - const storeTarget = value.lvalue.place; - state.manualMemoState?.decls.add( - storeTarget.identifier.declarationId, - ); - if (storeTarget.identifier.name?.kind === 'named' && dep == null) { - const dep: ManualMemoDependency = { - root: { - kind: 'NamedLocal', - value: storeTarget, - }, - path: [], - }; - this.temporaries.set(storeTarget.identifier.id, dep); - return dep; + collectMaybeMemoDependencies(value, this.temporaries, false); + if ( + value.kind === 'StoreLocal' || + value.kind === 'StoreContext' || + value.kind === 'Destructure' + ) { + for (const storeTarget of eachInstructionValueLValue(value)) { + state.manualMemoState?.decls.add( + storeTarget.identifier.declarationId, + ); + if (storeTarget.identifier.name?.kind === 'named') { + this.temporaries.set(storeTarget.identifier.id, { + root: { + kind: 'NamedLocal', + value: storeTarget, + }, + path: [], + }); + } } } - return dep; + break; } } } @@ -403,19 +402,15 @@ class Visitor extends ReactiveFunctionVisitor { state.manualMemoState.decls.add(lvalue.identifier.declarationId); } - const maybeDep = this.recordDepsInValue(value, state); - if (lvalId != null) { - if (maybeDep != null) { - temporaries.set(lvalId, maybeDep); - } else if (isNamedLocal) { - temporaries.set(lvalId, { - root: { - kind: 'NamedLocal', - value: {...(instr.lvalue as Place)}, - }, - path: [], - }); - } + this.recordDepsInValue(value, state); + if (lvalue != null) { + temporaries.set(lvalue.identifier.id, { + root: { + kind: 'NamedLocal', + value: {...lvalue}, + }, + path: [], + }); } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md new file mode 100644 index 0000000000000..d8223e2949837 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + * + * This fixture adds a later potential mutation, which extends the scope and should + * fail validation. This confirms that even though we allow the dependency to diverge, + * we still check that the output value is memoized. + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = transform(input); + return {value}; + }, [input]); + mutate(object); + return object; +} + +``` + + +## Error + +``` +Found 1 error: + +Compilation Skipped: Existing memoization could not be preserved + +React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. + +error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.ts:19:17 + 17 | */ + 18 | function useInputValue(input) { +> 19 | const object = React.useMemo(() => { + | ^^^^^^^^^^^^^^^^^^^^^ +> 20 | const {value} = transform(input); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 21 | return {value}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 22 | }, [input]); + | ^^^^^^^^^^^^^^ Could not preserve existing memoization + 23 | mutate(object); + 24 | return object; + 25 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js new file mode 100644 index 0000000000000..f07d00854cd85 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js @@ -0,0 +1,25 @@ +// @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + * + * This fixture adds a later potential mutation, which extends the scope and should + * fail validation. This confirms that even though we allow the dependency to diverge, + * we still check that the output value is memoized. + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = transform(input); + return {value}; + }, [input]); + mutate(object); + return object; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md new file mode 100644 index 0000000000000..371348a560643 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md @@ -0,0 +1,64 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify, useHook} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + */ +function useInputValue(input) { + // Conflate the `identity(input, x)` call with something outside the useMemo, + // to try and break memoization of `value`. This gets correctly flagged since + // the dependency is being mutated + let x = {}; + useHook(); + const object = React.useMemo(() => { + const {value} = identity(input, x); + return {value}; + }, [input, x]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Compilation Skipped: Existing memoization could not be preserved + +React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly. + +error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.ts:25:13 + 23 | const {value} = identity(input, x); + 24 | return {value}; +> 25 | }, [input, x]); + | ^ This dependency may be modified later + 26 | return object; + 27 | } + 28 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js new file mode 100644 index 0000000000000..06d2945868dd1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js @@ -0,0 +1,36 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify, useHook} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + */ +function useInputValue(input) { + // Conflate the `identity(input, x)` call with something outside the useMemo, + // to try and break memoization of `value`. This gets correctly flagged since + // the dependency is being mutated + let x = {}; + useHook(); + const object = React.useMemo(() => { + const {value} = identity(input, x); + return {value}; + }, [input, x]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/name-anonymous-functions.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/name-anonymous-functions.expect.md new file mode 100644 index 0000000000000..88e270647d3e6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/name-anonymous-functions.expect.md @@ -0,0 +1,272 @@ + +## Input + +```javascript +// @enableNameAnonymousFunctions + +import {useEffect} from 'react'; +import {identity, Stringify, useIdentity} from 'shared-runtime'; +import * as SharedRuntime from 'shared-runtime'; + +function Component(props) { + function named() { + const inner = () => props.named; + return inner(); + } + const namedVariable = function () { + return props.namedVariable; + }; + const methodCall = SharedRuntime.identity(() => props.methodCall); + const call = identity(() => props.call); + const builtinElementAttr =
props.builtinElementAttr} />; + const namedElementAttr = props.namedElementAttr} />; + const hookArgument = useIdentity(() => props.hookArgument); + useEffect(() => { + console.log(props.useEffect); + JSON.stringify(null, null, () => props.useEffect); + const g = () => props.useEffect; + console.log(g()); + }, [props.useEffect]); + return ( + <> + {named()} + {namedVariable()} + {methodCall()} + {call()} + {builtinElementAttr} + {namedElementAttr} + {hookArgument()} + + ); +} + +export const TODO_FIXTURE_ENTRYPOINT = { + fn: Component, + params: [ + { + named: '', + namedVariable: '', + methodCall: '', + call: '', + builtinElementAttr: '', + namedElementAttr: '', + hookArgument: '', + useEffect: '', + }, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enableNameAnonymousFunctions + +import { useEffect } from "react"; +import { identity, Stringify, useIdentity } from "shared-runtime"; +import * as SharedRuntime from "shared-runtime"; + +function Component(props) { + const $ = _c(31); + let t0; + if ($[0] !== props.named) { + t0 = function named() { + const inner = { "Component[named > inner]": () => props.named }[ + "Component[named > inner]" + ]; + return inner(); + }; + $[0] = props.named; + $[1] = t0; + } else { + t0 = $[1]; + } + const named = t0; + let t1; + if ($[2] !== props.namedVariable) { + t1 = { + "Component[namedVariable]": function () { + return props.namedVariable; + }, + }["Component[namedVariable]"]; + $[2] = props.namedVariable; + $[3] = t1; + } else { + t1 = $[3]; + } + const namedVariable = t1; + let t2; + if ($[4] !== props.methodCall) { + t2 = { "Component[SharedRuntime.identity()]": () => props.methodCall }[ + "Component[SharedRuntime.identity()]" + ]; + $[4] = props.methodCall; + $[5] = t2; + } else { + t2 = $[5]; + } + const methodCall = SharedRuntime.identity(t2); + let t3; + if ($[6] !== props.call) { + t3 = { "Component[identity()]": () => props.call }["Component[identity()]"]; + $[6] = props.call; + $[7] = t3; + } else { + t3 = $[7]; + } + const call = identity(t3); + let t4; + if ($[8] !== props.builtinElementAttr) { + t4 = ( +
.onClick]": () => props.builtinElementAttr }[ + "Component[
.onClick]" + ] + } + /> + ); + $[8] = props.builtinElementAttr; + $[9] = t4; + } else { + t4 = $[9]; + } + const builtinElementAttr = t4; + let t5; + if ($[10] !== props.namedElementAttr) { + t5 = ( + .onClick]": () => props.namedElementAttr }[ + "Component[.onClick]" + ] + } + /> + ); + $[10] = props.namedElementAttr; + $[11] = t5; + } else { + t5 = $[11]; + } + const namedElementAttr = t5; + let t6; + if ($[12] !== props.hookArgument) { + t6 = { "Component[useIdentity()]": () => props.hookArgument }[ + "Component[useIdentity()]" + ]; + $[12] = props.hookArgument; + $[13] = t6; + } else { + t6 = $[13]; + } + const hookArgument = useIdentity(t6); + let t7; + let t8; + if ($[14] !== props.useEffect) { + t7 = { + "Component[useEffect()]": () => { + console.log(props.useEffect); + JSON.stringify( + null, + null, + { + "Component[useEffect() > JSON.stringify()]": () => props.useEffect, + }["Component[useEffect() > JSON.stringify()]"], + ); + const g = { "Component[useEffect() > g]": () => props.useEffect }[ + "Component[useEffect() > g]" + ]; + console.log(g()); + }, + }["Component[useEffect()]"]; + t8 = [props.useEffect]; + $[14] = props.useEffect; + $[15] = t7; + $[16] = t8; + } else { + t7 = $[15]; + t8 = $[16]; + } + useEffect(t7, t8); + let t9; + if ($[17] !== named) { + t9 = named(); + $[17] = named; + $[18] = t9; + } else { + t9 = $[18]; + } + let t10; + if ($[19] !== namedVariable) { + t10 = namedVariable(); + $[19] = namedVariable; + $[20] = t10; + } else { + t10 = $[20]; + } + const t11 = methodCall(); + const t12 = call(); + let t13; + if ($[21] !== hookArgument) { + t13 = hookArgument(); + $[21] = hookArgument; + $[22] = t13; + } else { + t13 = $[22]; + } + let t14; + if ( + $[23] !== builtinElementAttr || + $[24] !== namedElementAttr || + $[25] !== t10 || + $[26] !== t11 || + $[27] !== t12 || + $[28] !== t13 || + $[29] !== t9 + ) { + t14 = ( + <> + {t9} + {t10} + {t11} + {t12} + {builtinElementAttr} + {namedElementAttr} + {t13} + + ); + $[23] = builtinElementAttr; + $[24] = namedElementAttr; + $[25] = t10; + $[26] = t11; + $[27] = t12; + $[28] = t13; + $[29] = t9; + $[30] = t14; + } else { + t14 = $[30]; + } + return t14; +} + +export const TODO_FIXTURE_ENTRYPOINT = { + fn: Component, + params: [ + { + named: "", + namedVariable: "", + methodCall: "", + call: "", + builtinElementAttr: "", + namedElementAttr: "", + hookArgument: "", + useEffect: "", + }, + ], +}; + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/name-anonymous-functions.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/name-anonymous-functions.js new file mode 100644 index 0000000000000..ff4f1f6017d98 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/name-anonymous-functions.js @@ -0,0 +1,53 @@ +// @enableNameAnonymousFunctions + +import {useEffect} from 'react'; +import {identity, Stringify, useIdentity} from 'shared-runtime'; +import * as SharedRuntime from 'shared-runtime'; + +function Component(props) { + function named() { + const inner = () => props.named; + return inner(); + } + const namedVariable = function () { + return props.namedVariable; + }; + const methodCall = SharedRuntime.identity(() => props.methodCall); + const call = identity(() => props.call); + const builtinElementAttr =
props.builtinElementAttr} />; + const namedElementAttr = props.namedElementAttr} />; + const hookArgument = useIdentity(() => props.hookArgument); + useEffect(() => { + console.log(props.useEffect); + JSON.stringify(null, null, () => props.useEffect); + const g = () => props.useEffect; + console.log(g()); + }, [props.useEffect]); + return ( + <> + {named()} + {namedVariable()} + {methodCall()} + {call()} + {builtinElementAttr} + {namedElementAttr} + {hookArgument()} + + ); +} + +export const TODO_FIXTURE_ENTRYPOINT = { + fn: Component, + params: [ + { + named: '', + namedVariable: '', + methodCall: '', + call: '', + builtinElementAttr: '', + namedElementAttr: '', + hookArgument: '', + useEffect: '', + }, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md new file mode 100644 index 0000000000000..eca36e2a7919d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md @@ -0,0 +1,109 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = identity(input); + return {value}; + }, [input]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees + +import { identity, Stringify } from "shared-runtime"; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const $ = _c(4); + let t0; + if ($[0] !== input) { + t0 = identity(input); + $[0] = input; + $[1] = t0; + } else { + t0 = $[1]; + } + const { value } = t0; + let t1; + if ($[2] !== value) { + t1 = { value }; + $[2] = value; + $[3] = t1; + } else { + t1 = $[3]; + } + const object = t1; + return object; +} + +function Component() { + const $ = _c(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = { value: 42 }; + $[0] = t0; + } else { + t0 = $[0]; + } + const t1 = useInputValue(t0); + let t2; + if ($[1] !== t1.value) { + t2 = ; + $[1] = t1.value; + $[2] = t2; + } else { + t2 = $[2]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
{"value":42}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js new file mode 100644 index 0000000000000..a9533ce44bbde --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js @@ -0,0 +1,31 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = identity(input); + return {value}; + }, [input]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; diff --git a/compiler/packages/eslint-plugin-react-compiler/src/index.ts b/compiler/packages/eslint-plugin-react-compiler/src/index.ts index 9d49b16c57e8f..3f0c7bcdcb058 100644 --- a/compiler/packages/eslint-plugin-react-compiler/src/index.ts +++ b/compiler/packages/eslint-plugin-react-compiler/src/index.ts @@ -34,4 +34,8 @@ const configs = { }, }; -export {configs, allRules as rules, meta}; +const rules = Object.fromEntries( + Object.entries(allRules).map(([name, {rule}]) => [name, rule]), +); + +export {configs, rules, meta}; diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index c7cb641cd242b..132747e60a883 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -1794,13 +1794,21 @@ function transferReferencedDebugInfo( existingDebugInfo.push.apply(existingDebugInfo, referencedDebugInfo); } } - // We also add it to the initializing chunk since the resolution of that promise is - // also blocked by these. By adding it to both we can track it even if the array/element + // We also add the debug info to the initializing chunk since the resolution of that promise is + // also blocked by the referenced debug info. By adding it to both we can track it even if the array/element // is extracted, or if the root is rendered as is. if (parentChunk !== null) { const parentDebugInfo = parentChunk._debugInfo; - // $FlowFixMe[method-unbinding] - parentDebugInfo.push.apply(parentDebugInfo, referencedDebugInfo); + for (let i = 0; i < referencedDebugInfo.length; ++i) { + const debugInfoEntry = referencedDebugInfo[i]; + if (debugInfoEntry.name != null) { + (debugInfoEntry: ReactComponentInfo); + // We're not transferring Component info since we use Component info + // in Debug info to fill in gaps between Fibers for the parent stack. + } else { + parentDebugInfo.push(debugInfoEntry); + } + } } } } diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 01f37319bf574..3bcf4988feb86 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -2960,13 +2960,6 @@ describe('ReactFlight', () => { { time: 16, }, - { - env: 'third-party', - key: null, - name: 'ThirdPartyAsyncIterableComponent', - props: {}, - stack: ' in Object. (at **)', - }, { time: 16, }, @@ -2995,13 +2988,6 @@ describe('ReactFlight', () => { { time: 19, }, - { - env: 'third-party', - key: null, - name: 'ThirdPartyAsyncIterableComponent', - props: {}, - stack: ' in Object. (at **)', - }, {time: 19}, ] : undefined, @@ -3847,4 +3833,115 @@ describe('ReactFlight', () => { expect(ReactNoop).toMatchRenderedOutput(
not using props
); }); + + // @gate !__DEV__ || enableComponentPerformanceTrack + it('produces correct parent stacks', async () => { + function Container() { + return ReactServer.createElement('div', null); + } + function ContainerParent() { + return ReactServer.createElement(Container, null); + } + function App() { + return ReactServer.createElement( + 'main', + null, + ReactServer.createElement(ContainerParent, null), + ); + } + + const transport = ReactNoopFlightServer.render({ + root: ReactServer.createElement(App, null), + }); + + await act(async () => { + const {root} = await ReactNoopFlightClient.read(transport); + + ReactNoop.render(root); + + expect(root.type).toBe('main'); + if (__DEV__) { + const div = root.props.children; + expect(getDebugInfo(div)).toEqual([ + { + time: 14, + }, + { + env: 'Server', + key: null, + name: 'ContainerParent', + owner: { + env: 'Server', + key: null, + name: 'App', + props: {}, + stack: ' in Object. (at **)', + }, + props: {}, + stack: ' in App (at **)', + }, + { + time: 15, + }, + { + env: 'Server', + key: null, + name: 'Container', + owner: { + env: 'Server', + key: null, + name: 'ContainerParent', + owner: { + env: 'Server', + key: null, + name: 'App', + props: {}, + stack: ' in Object. (at **)', + }, + props: {}, + stack: ' in App (at **)', + }, + props: {}, + stack: ' in ContainerParent (at **)', + }, + { + time: 16, + }, + ]); + expect(getDebugInfo(root)).toEqual([ + { + time: 12, + }, + { + env: 'Server', + key: null, + name: 'App', + props: {}, + stack: ' in Object. (at **)', + }, + { + time: 13, + }, + { + time: 14, + }, + { + time: 15, + }, + { + time: 16, + }, + ]); + } else { + expect(root._debugInfo).toBe(undefined); + expect(root._owner).toBe(undefined); + } + }); + + expect(ReactNoop).toMatchRenderedOutput( +
+
+
, + ); + }); }); diff --git a/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js b/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js index 6716766b2794c..73df17071bac2 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js @@ -229,7 +229,7 @@ describe('commit tree', () => { ▾ [suspense-root] rects={null} - + `); utils.act(() => modernRender()); expect(store).toMatchInlineSnapshot(` @@ -238,7 +238,7 @@ describe('commit tree', () => { ▾ [suspense-root] rects={null} - + `); utils.act(() => modernRender()); expect(store).toMatchInlineSnapshot(` @@ -304,7 +304,7 @@ describe('commit tree', () => { ▾ [suspense-root] rects={null} - + `); utils.act(() => modernRender()); expect(store).toMatchInlineSnapshot(` diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index e6eddff8402f0..fad4622acc15b 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -493,7 +493,7 @@ describe('Store', () => { ▾ [suspense-root] rects={[{x:1,y:2,width:5,height:1}, {x:1,y:2,width:10,height:1}]} - + `); await act(() => { @@ -506,7 +506,7 @@ describe('Store', () => { ▾ [suspense-root] rects={[{x:1,y:2,width:5,height:1}, {x:1,y:2,width:5,height:1}]} - + `); }); @@ -1054,7 +1054,7 @@ describe('Store', () => { ▾ [suspense-root] rects={[{x:1,y:2,width:5,height:1}, {x:1,y:2,width:10,height:1}]} - + `); await act(() => { @@ -1069,7 +1069,7 @@ describe('Store', () => { [suspense-root] rects={[{x:1,y:2,width:5,height:1}, {x:1,y:2,width:5,height:1}, {x:1,y:2,width:5,height:1}]} - + `); }); @@ -1407,7 +1407,7 @@ describe('Store', () => { [root] ▸ [suspense-root] rects={[{x:1,y:2,width:5,height:1}, {x:1,y:2,width:10,height:1}]} - + `); // This test isn't meaningful unless we expand the suspended tree @@ -1424,7 +1424,7 @@ describe('Store', () => { ▾ [suspense-root] rects={[{x:1,y:2,width:5,height:1}, {x:1,y:2,width:10,height:1}]} - + `); await act(() => { @@ -1437,7 +1437,7 @@ describe('Store', () => { ▾ [suspense-root] rects={[{x:1,y:2,width:5,height:1}, {x:1,y:2,width:5,height:1}]} - + `); }); @@ -1663,7 +1663,7 @@ describe('Store', () => { [root] ▸ [suspense-root] rects={null} - + `); await act(() => @@ -1678,7 +1678,7 @@ describe('Store', () => { ▾ ▸ [suspense-root] rects={null} - + `); const rendererID = getRendererID(); @@ -1697,7 +1697,7 @@ describe('Store', () => { ▾ [suspense-root] rects={null} - + `); await act(() => @@ -1713,7 +1713,7 @@ describe('Store', () => { ▾ ▸ [suspense-root] rects={null} - + `); }); }); @@ -2018,7 +2018,7 @@ describe('Store', () => { ▾ [suspense-root] rects={null} - + `); await Promise.resolve(); @@ -2032,7 +2032,7 @@ describe('Store', () => { ▾ [suspense-root] rects={null} - + `); // Render again to unmount it @@ -2523,7 +2523,7 @@ describe('Store', () => { ▾ [suspense-root] rects={null} - + `); await actAsync(() => render()); @@ -2534,7 +2534,7 @@ describe('Store', () => { ▾ [suspense-root] rects={null} - + `); }); }); diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 6f382f976cb0a..9b6b7e7ab073d 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -1369,8 +1369,8 @@ describe('TreeListContext', () => { ▾ [suspense-root] rects={null} - - + + `); const outerSuspenseID = ((store.getElementIDAtIndex(1): any): number); @@ -1411,8 +1411,8 @@ describe('TreeListContext', () => { ▾ [suspense-root] rects={null} - - + + `); }); }); diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 02e60a080af3e..310321b5ffcc5 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -1950,14 +1950,13 @@ export default class Store extends EventEmitter<{ throw error; } - _guessSuspenseName(element: Element): string | null { - // TODO: Use key + _guessSuspenseName(element: Element): string { const owner = this._idToElement.get(element.ownerID); - if (owner !== undefined) { - // TODO: This is clowny - return `${owner.displayName || 'Unknown'}>?`; + let name = 'Unknown'; + if (owner !== undefined && owner.displayName !== null) { + name = owner.displayName; } - return null; + return name; } }