From 8fb1dc42f6874637dcd4dd87e0170c67395dbb32 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 18 Nov 2024 12:47:39 -0500 Subject: [PATCH 1/3] [compiler][ez] Clean up duplicate code in propagateScopeDeps Clean up duplicate checks for when to skip processing values as dependencies / hoistable temporaries. --- .../src/HIR/PropagateScopeDependenciesHIR.ts | 82 +++++++++++++------ 1 file changed, 58 insertions(+), 24 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts index 8aed17f8ee847..4a85a4ef5cfad 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -358,6 +358,7 @@ class Context { #temporaries: ReadonlyMap; #temporariesUsedOutsideScope: ReadonlySet; + #processedInstrsInOptional: ReadonlySet; /** * Tracks the traversal state. See Context.declare for explanation of why this @@ -368,9 +369,11 @@ class Context { constructor( temporariesUsedOutsideScope: ReadonlySet, temporaries: ReadonlyMap, + processedInstrsInOptional: ReadonlySet, ) { this.#temporariesUsedOutsideScope = temporariesUsedOutsideScope; this.#temporaries = temporaries; + this.#processedInstrsInOptional = processedInstrsInOptional; } enterScope(scope: ReactiveScope): void { @@ -574,22 +577,49 @@ class Context { currentScope.reassignments.add(place.identifier); } } + enterInnerFn(cb: () => T): T { + const wasInInnerFn = this.inInnerFn; + this.inInnerFn = true; + const result = cb(); + this.inInnerFn = wasInInnerFn; + return result; + } + + /** + * Skip dependencies that are subexpressions of other dependencies. e.g. if a + * dependency is tracked in the temporaries sidemap, it can be added at + * site-of-use + */ + isDeferredDependency( + instr: + | {kind: HIRValue.Instruction; value: Instruction} + | {kind: HIRValue.Terminal; value: Terminal}, + ): boolean { + return ( + this.#processedInstrsInOptional.has(instr.value) || + (instr.kind === HIRValue.Instruction && + this.#temporaries.has(instr.value.lvalue.identifier.id)) + ); + } +} +enum HIRValue { + Instruction = 1, + Terminal, } function handleInstruction(instr: Instruction, context: Context): void { const {id, value, lvalue} = instr; - if (value.kind === 'LoadLocal') { - if ( - value.place.identifier.name === null || - lvalue.identifier.name !== null || - context.isUsedOutsideDeclaringScope(lvalue) - ) { - context.visitOperand(value.place); - } - } else if (value.kind === 'PropertyLoad') { - if (context.isUsedOutsideDeclaringScope(lvalue)) { - context.visitProperty(value.object, value.property, false); - } + context.declare(lvalue.identifier, { + id, + scope: context.currentScope, + }); + if ( + context.isDeferredDependency({kind: HIRValue.Instruction, value: instr}) + ) { + return; + } + if (value.kind === 'PropertyLoad') { + context.visitProperty(value.object, value.property, false); } else if (value.kind === 'StoreLocal') { context.visitOperand(value.value); if (value.lvalue.kind === InstructionKind.Reassign) { @@ -632,11 +662,6 @@ function handleInstruction(instr: Instruction, context: Context): void { context.visitOperand(operand); } } - - context.declare(lvalue.identifier, { - id, - scope: context.currentScope, - }); } function collectDependencies( @@ -645,7 +670,11 @@ function collectDependencies( temporaries: ReadonlyMap, processedInstrsInOptional: ReadonlySet, ): Map> { - const context = new Context(usedOutsideDeclaringScope, temporaries); + const context = new Context( + usedOutsideDeclaringScope, + temporaries, + processedInstrsInOptional, + ); for (const param of fn.params) { if (param.kind === 'Identifier') { @@ -694,16 +723,21 @@ function collectDependencies( /** * Recursively visit the inner function to extract dependencies there */ - const wasInInnerFn = context.inInnerFn; - context.inInnerFn = true; - handleFunction(instr.value.loweredFunc.func); - context.inInnerFn = wasInInnerFn; - } else if (!processedInstrsInOptional.has(instr)) { + const innerFn = instr.value.loweredFunc.func; + context.enterInnerFn(() => { + handleFunction(innerFn); + }); + } else { handleInstruction(instr, context); } } - if (!processedInstrsInOptional.has(block.terminal)) { + if ( + !context.isDeferredDependency({ + kind: HIRValue.Terminal, + value: block.terminal, + }) + ) { for (const place of eachTerminalOperand(block.terminal)) { context.visitOperand(place); } From 22902de456698d3be51f247eb84adb7307100c39 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 19 Nov 2024 09:43:04 -0500 Subject: [PATCH 2/3] [compiler] Context variables as dependencies We previously didn't track context variables in the hoistable values sidemap of `propagateScopeDependencies`. This was overly conservative as we *do* track the mutable range of context variables, and it is safe to hoist accesses to context variables after their last direct / aliased maybe-assignment. ```js function Component({value}) { // start of mutable range for `x` let x = DEFAULT; const setX = () => x = value; const aliasedSet = maybeAlias(setX); maybeCall(aliasedSet); // end of mutable range for `x` // here, we should be able to take x (and property reads // off of x) as dependencies return } ``` --- .../src/HIR/HIR.ts | 11 +- .../src/HIR/PropagateScopeDependenciesHIR.ts | 65 ++++++--- .../bug-functiondecl-hoisting.expect.md | 18 ++- ...-array-assignment-to-context-var.expect.md | 15 +- ...array-declaration-to-context-var.expect.md | 15 +- ...object-assignment-to-context-var.expect.md | 15 +- ...bject-declaration-to-context-var.expect.md | 15 +- ...mutated-non-reactive-to-reactive.expect.md | 16 +-- ...ures-reassigned-context-property.expect.md | 53 ------- ...ures-reassigned-context-property.expect.md | 101 ++++++++++++++ ...-captures-reassigned-context-property.tsx} | 0 ...o-reordering-depslist-assignment.expect.md | 15 +- ...epro-scope-missing-mutable-range.expect.md | 16 +-- ...l-dependency-on-context-variable.expect.md | 16 +-- .../context-var-granular-dep.expect.md | 130 ++++++++++++++++++ .../context-var-granular-dep.js | 43 ++++++ ...epro-scope-missing-mutable-range.expect.md | 16 +-- .../use-operator-conditional.expect.md | 42 +++--- compiler/packages/snap/src/sprout/index.ts | 10 +- .../snap/src/sprout/shared-runtime.ts | 29 ++-- 20 files changed, 447 insertions(+), 194 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/{error.todo-useCallback-captures-reassigned-context-property.tsx => useCallback-captures-reassigned-context-property.tsx} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.js 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 954fb6f40053a..8cae007ec0928 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -840,6 +840,11 @@ export type LoadLocal = { place: Place; loc: SourceLocation; }; +export type LoadContext = { + kind: 'LoadContext'; + place: Place; + loc: SourceLocation; +}; /* * The value of a given instruction. Note that values are not recursive: complex @@ -852,11 +857,7 @@ export type LoadLocal = { export type InstructionValue = | LoadLocal - | { - kind: 'LoadContext'; - place: Place; - loc: SourceLocation; - } + | LoadContext | { kind: 'DeclareLocal'; lvalue: LValue; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts index 4a85a4ef5cfad..08856e9143139 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -17,6 +17,11 @@ import { areEqualPaths, IdentifierId, Terminal, + InstructionValue, + LoadContext, + TInstruction, + FunctionExpression, + ObjectMethod, } from './HIR'; import { collectHoistablePropertyLoads, @@ -223,11 +228,25 @@ export function collectTemporariesSidemap( fn, usedOutsideDeclaringScope, temporaries, - false, + null, ); return temporaries; } +function isLoadContextMutable( + instrValue: InstructionValue, + id: InstructionId, +): instrValue is LoadContext { + if (instrValue.kind === 'LoadContext') { + CompilerError.invariant(instrValue.place.identifier.scope != null, { + reason: + '[PropagateScopeDependencies] Expected all context variables to be assigned a scope', + loc: instrValue.loc, + }); + return id >= instrValue.place.identifier.scope.range.end; + } + return false; +} /** * Recursive collect a sidemap of all `LoadLocal` and `PropertyLoads` with a * function and all nested functions. @@ -239,17 +258,21 @@ function collectTemporariesSidemapImpl( fn: HIRFunction, usedOutsideDeclaringScope: ReadonlySet, temporaries: Map, - isInnerFn: boolean, + innerFnContext: {instrId: InstructionId} | null, ): void { for (const [_, block] of fn.body.blocks) { - for (const instr of block.instructions) { - const {value, lvalue} = instr; + for (const {value, lvalue, id: origInstrId} of block.instructions) { + const instrId = + innerFnContext != null ? innerFnContext.instrId : origInstrId; const usedOutside = usedOutsideDeclaringScope.has( lvalue.identifier.declarationId, ); if (value.kind === 'PropertyLoad' && !usedOutside) { - if (!isInnerFn || temporaries.has(value.object.identifier.id)) { + if ( + innerFnContext == null || + temporaries.has(value.object.identifier.id) + ) { /** * All dependencies of a inner / nested function must have a base * identifier from the outermost component / hook. This is because the @@ -265,13 +288,13 @@ function collectTemporariesSidemapImpl( temporaries.set(lvalue.identifier.id, property); } } else if ( - value.kind === 'LoadLocal' && + (value.kind === 'LoadLocal' || isLoadContextMutable(value, instrId)) && lvalue.identifier.name == null && value.place.identifier.name !== null && !usedOutside ) { if ( - !isInnerFn || + innerFnContext == null || fn.context.some( context => context.identifier.id === value.place.identifier.id, ) @@ -289,7 +312,7 @@ function collectTemporariesSidemapImpl( value.loweredFunc.func, usedOutsideDeclaringScope, temporaries, - true, + innerFnContext ?? {instrId}, ); } } @@ -364,7 +387,7 @@ class Context { * Tracks the traversal state. See Context.declare for explanation of why this * is needed. */ - inInnerFn: boolean = false; + #innerFnContext: {outerInstrId: InstructionId} | null = null; constructor( temporariesUsedOutsideScope: ReadonlySet, @@ -434,7 +457,7 @@ class Context { * by root identifier mutable ranges). */ declare(identifier: Identifier, decl: Decl): void { - if (this.inInnerFn) return; + if (this.#innerFnContext != null) return; if (!this.#declarations.has(identifier.declarationId)) { this.#declarations.set(identifier.declarationId, decl); } @@ -577,11 +600,14 @@ class Context { currentScope.reassignments.add(place.identifier); } } - enterInnerFn(cb: () => T): T { - const wasInInnerFn = this.inInnerFn; - this.inInnerFn = true; + enterInnerFn( + innerFn: TInstruction | TInstruction, + cb: () => T, + ): T { + const prevContext = this.#innerFnContext; + this.#innerFnContext = this.#innerFnContext ?? {outerInstrId: innerFn.id}; const result = cb(); - this.inInnerFn = wasInInnerFn; + this.#innerFnContext = prevContext; return result; } @@ -724,9 +750,14 @@ function collectDependencies( * Recursively visit the inner function to extract dependencies there */ const innerFn = instr.value.loweredFunc.func; - context.enterInnerFn(() => { - handleFunction(innerFn); - }); + context.enterInnerFn( + instr as + | TInstruction + | TInstruction, + () => { + handleFunction(innerFn); + }, + ); } else { handleInstruction(instr, context); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md index 2b0031b117be2..f8712ed7289a9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md @@ -58,18 +58,16 @@ function Foo(t0) { bar = $[1]; result = $[2]; } - - const t1 = bar; - let t2; - if ($[3] !== result || $[4] !== t1) { - t2 = ; - $[3] = result; - $[4] = t1; - $[5] = t2; + let t1; + if ($[3] !== bar || $[4] !== result) { + t1 = ; + $[3] = bar; + $[4] = result; + $[5] = t1; } else { - t2 = $[5]; + t1 = $[5]; } - return t2; + return t1; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-array-assignment-to-context-var.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-array-assignment-to-context-var.expect.md index 7febb3fecb6e7..1268cbcfdc3d3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-array-assignment-to-context-var.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-array-assignment-to-context-var.expect.md @@ -43,16 +43,15 @@ function Component(props) { } else { x = $[1]; } - const t0 = x; - let t1; - if ($[2] !== t0) { - t1 = { x: t0 }; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== x) { + t0 = { x }; + $[2] = x; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } - return t1; + return t0; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-array-declaration-to-context-var.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-array-declaration-to-context-var.expect.md index 26b56ea2a4f4d..769e4871f4ad8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-array-declaration-to-context-var.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-array-declaration-to-context-var.expect.md @@ -42,16 +42,15 @@ function Component(props) { } else { x = $[1]; } - const t0 = x; - let t1; - if ($[2] !== t0) { - t1 =
{t0}
; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== x) { + t0 =
{x}
; + $[2] = x; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } - return t1; + return t0; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-object-assignment-to-context-var.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-object-assignment-to-context-var.expect.md index 5ffa73389ffd0..e66ef2df13d5f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-object-assignment-to-context-var.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-object-assignment-to-context-var.expect.md @@ -43,16 +43,15 @@ function Component(props) { } else { x = $[1]; } - const t0 = x; - let t1; - if ($[2] !== t0) { - t1 = { x: t0 }; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== x) { + t0 = { x }; + $[2] = x; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } - return t1; + return t0; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-object-declaration-to-context-var.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-object-declaration-to-context-var.expect.md index 2c495d8223d0b..66799c5c4720b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-object-declaration-to-context-var.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructure-object-declaration-to-context-var.expect.md @@ -42,16 +42,15 @@ function Component(props) { } else { x = $[1]; } - const t0 = x; - let t1; - if ($[2] !== t0) { - t1 = { x: t0 }; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== x) { + t0 = { x }; + $[2] = x; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } - return t1; + return t0; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-non-reactive-to-reactive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-non-reactive-to-reactive.expect.md index dfe941282e2a3..d34db46d6aa28 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-non-reactive-to-reactive.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-non-reactive-to-reactive.expect.md @@ -33,17 +33,15 @@ function f(a) { } else { x = $[1]; } - - const t0 = x; - let t1; - if ($[2] !== t0) { - t1 =
; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== x) { + t0 =
; + $[2] = x; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } - return t1; + return t0; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.expect.md deleted file mode 100644 index ae44f27912293..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.expect.md +++ /dev/null @@ -1,53 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees -import {useCallback} from 'react'; -import {Stringify} from 'shared-runtime'; - -/** - * TODO: we're currently bailing out because `contextVar` is a context variable - * and not recorded into the PropagateScopeDeps LoadLocal / PropertyLoad - * sidemap. Previously, we were able to avoid this as `BuildHIR` hoisted - * `LoadContext` and `PropertyLoad` instructions into the outer function, which - * we took as eligible dependencies. - * - * One solution is to simply record `LoadContext` identifiers into the - * temporaries sidemap when the instruction occurs *after* the context - * variable's mutable range. - */ -function Foo(props) { - let contextVar; - if (props.cond) { - contextVar = {val: 2}; - } else { - contextVar = {}; - } - - const cb = useCallback(() => [contextVar.val], [contextVar.val]); - - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{cond: true}], -}; - -``` - - -## Error - -``` - 22 | } - 23 | -> 24 | const cb = useCallback(() => [contextVar.val], [contextVar.val]); - | ^^^^^^^^^^^^^^^^^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (24:24) - 25 | - 26 | return ; - 27 | } -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.expect.md new file mode 100644 index 0000000000000..a1cbe89a88340 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.expect.md @@ -0,0 +1,101 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +import {useCallback} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * TODO: we're currently bailing out because `contextVar` is a context variable + * and not recorded into the PropagateScopeDeps LoadLocal / PropertyLoad + * sidemap. Previously, we were able to avoid this as `BuildHIR` hoisted + * `LoadContext` and `PropertyLoad` instructions into the outer function, which + * we took as eligible dependencies. + * + * One solution is to simply record `LoadContext` identifiers into the + * temporaries sidemap when the instruction occurs *after* the context + * variable's mutable range. + */ +function Foo(props) { + let contextVar; + if (props.cond) { + contextVar = {val: 2}; + } else { + contextVar = {}; + } + + const cb = useCallback(() => [contextVar.val], [contextVar.val]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{cond: true}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees +import { useCallback } from "react"; +import { Stringify } from "shared-runtime"; + +/** + * TODO: we're currently bailing out because `contextVar` is a context variable + * and not recorded into the PropagateScopeDeps LoadLocal / PropertyLoad + * sidemap. Previously, we were able to avoid this as `BuildHIR` hoisted + * `LoadContext` and `PropertyLoad` instructions into the outer function, which + * we took as eligible dependencies. + * + * One solution is to simply record `LoadContext` identifiers into the + * temporaries sidemap when the instruction occurs *after* the context + * variable's mutable range. + */ +function Foo(props) { + const $ = _c(6); + let contextVar; + if ($[0] !== props.cond) { + if (props.cond) { + contextVar = { val: 2 }; + } else { + contextVar = {}; + } + $[0] = props.cond; + $[1] = contextVar; + } else { + contextVar = $[1]; + } + let t0; + if ($[2] !== contextVar.val) { + t0 = () => [contextVar.val]; + $[2] = contextVar.val; + $[3] = t0; + } else { + t0 = $[3]; + } + contextVar; + const cb = t0; + let t1; + if ($[4] !== cb) { + t1 = ; + $[4] = cb; + $[5] = t1; + } else { + t1 = $[5]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ cond: true }], +}; + +``` + +### Eval output +(kind: ok)
{"cb":{"kind":"Function","result":[2]},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.tsx similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.tsx rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-captures-reassigned-context-property.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md index dc1a87fe5113c..e8a3e2d627c59 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md @@ -44,16 +44,15 @@ function useFoo(arr1, arr2) { y = $[2]; } let t0; - const t1 = y; - let t2; - if ($[3] !== t1) { - t2 = { y: t1 }; - $[3] = t1; - $[4] = t2; + let t1; + if ($[3] !== y) { + t1 = { y }; + $[3] = y; + $[4] = t1; } else { - t2 = $[4]; + t1 = $[4]; } - t0 = t2; + t0 = t1; return t0; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md index 39f301432e51f..9d232d8e78169 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md @@ -36,17 +36,15 @@ function HomeDiscoStoreItemTileRating(props) { } else { count = $[1]; } - - const t0 = count; - let t1; - if ($[2] !== t0) { - t1 = {t0}; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== count) { + t0 = {count}; + $[2] = count; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } - return t1; + return t0; } ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-on-context-variable.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-on-context-variable.expect.md index 23cc7ee84607b..ceaa350012258 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-on-context-variable.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-on-context-variable.expect.md @@ -67,17 +67,15 @@ function Component(props) { } else { x = $[1]; } - - const t0 = x; - let t1; - if ($[2] !== t0) { - t1 = [t0]; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== x) { + t0 = [x]; + $[2] = x; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } - return t1; + return t0; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.expect.md new file mode 100644 index 0000000000000..d72f34b4fd8a9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.expect.md @@ -0,0 +1,130 @@ + +## Input + +```javascript +import {throwErrorWithMessage, ValidateMemoization} from 'shared-runtime'; + +/** + * Context variables are local variables that (1) have at least one reassignment + * and (2) are captured into a function expression. These have a known mutable + * range: from first declaration / assignment to the last direct or aliased, + * mutable reference. + * + * This fixture validates that forget can take granular dependencies on context + * variables when the reference to a context var happens *after* the end of its + * mutable range. + */ +function Component({cond, a}) { + let contextVar; + if (cond) { + contextVar = {val: a}; + } else { + contextVar = {}; + throwErrorWithMessage(''); + } + const cb = {cb: () => contextVar.val * 4}; + + /** + * manually specify input to avoid adding a `PropertyLoad` from contextVar, + * which might affect hoistable-objects analysis. + */ + return ( + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: false, a: undefined}], + sequentialRenders: [ + {cond: true, a: 2}, + {cond: true, a: 2}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { throwErrorWithMessage, ValidateMemoization } from "shared-runtime"; + +/** + * Context variables are local variables that (1) have at least one reassignment + * and (2) are captured into a function expression. These have a known mutable + * range: from first declaration / assignment to the last direct or aliased, + * mutable reference. + * + * This fixture validates that forget can take granular dependencies on context + * variables when the reference to a context var happens *after* the end of its + * mutable range. + */ +function Component(t0) { + const $ = _c(10); + const { cond, a } = t0; + let contextVar; + if ($[0] !== a || $[1] !== cond) { + if (cond) { + contextVar = { val: a }; + } else { + contextVar = {}; + throwErrorWithMessage(""); + } + $[0] = a; + $[1] = cond; + $[2] = contextVar; + } else { + contextVar = $[2]; + } + let t1; + if ($[3] !== contextVar.val) { + t1 = { cb: () => contextVar.val * 4 }; + $[3] = contextVar.val; + $[4] = t1; + } else { + t1 = $[4]; + } + const cb = t1; + + const t2 = cond ? a : undefined; + let t3; + if ($[5] !== t2) { + t3 = [t2]; + $[5] = t2; + $[6] = t3; + } else { + t3 = $[6]; + } + let t4; + if ($[7] !== cb || $[8] !== t3) { + t4 = ( + + ); + $[7] = cb; + $[8] = t3; + $[9] = t4; + } else { + t4 = $[9]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: false, a: undefined }], + sequentialRenders: [ + { cond: true, a: 2 }, + { cond: true, a: 2 }, + ], +}; + +``` + +### Eval output +(kind: ok)
{"inputs":[2],"output":{"cb":"[[ function params=0 ]]"}}
+
{"inputs":[2],"output":{"cb":"[[ function params=0 ]]"}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.js new file mode 100644 index 0000000000000..b9bdd67e2f504 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.js @@ -0,0 +1,43 @@ +import {throwErrorWithMessage, ValidateMemoization} from 'shared-runtime'; + +/** + * Context variables are local variables that (1) have at least one reassignment + * and (2) are captured into a function expression. These have a known mutable + * range: from first declaration / assignment to the last direct or aliased, + * mutable reference. + * + * This fixture validates that forget can take granular dependencies on context + * variables when the reference to a context var happens *after* the end of its + * mutable range. + */ +function Component({cond, a}) { + let contextVar; + if (cond) { + contextVar = {val: a}; + } else { + contextVar = {}; + throwErrorWithMessage(''); + } + const cb = {cb: () => contextVar.val * 4}; + + /** + * manually specify input to avoid adding a `PropertyLoad` from contextVar, + * which might affect hoistable-objects analysis. + */ + return ( + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: false, a: undefined}], + sequentialRenders: [ + {cond: true, a: 2}, + {cond: true, a: 2}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md index d8e59c486a55b..b7c425ba5c027 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md @@ -35,17 +35,15 @@ function HomeDiscoStoreItemTileRating(props) { } else { count = $[1]; } - - const t0 = count; - let t1; - if ($[2] !== t0) { - t1 = {t0}; - $[2] = t0; - $[3] = t1; + let t0; + if ($[2] !== count) { + t0 = {count}; + $[2] = count; + $[3] = t0; } else { - t1 = $[3]; + t0 = $[3]; } - return t1; + return t0; } ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-operator-conditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-operator-conditional.expect.md index d94a5e7e375b3..e335273026791 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-operator-conditional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-operator-conditional.expect.md @@ -88,36 +88,34 @@ function Inner(props) { input; input; let t0; - const t1 = input; - let t2; - if ($[0] !== t1) { - t2 = [t1]; - $[0] = t1; - $[1] = t2; + let t1; + if ($[0] !== input) { + t1 = [input]; + $[0] = input; + $[1] = t1; } else { - t2 = $[1]; + t1 = $[1]; } - t0 = t2; + t0 = t1; const output = t0; - const t3 = input; - let t4; - if ($[2] !== t3) { - t4 = [t3]; - $[2] = t3; - $[3] = t4; + let t2; + if ($[2] !== input) { + t2 = [input]; + $[2] = input; + $[3] = t2; } else { - t4 = $[3]; + t2 = $[3]; } - let t5; - if ($[4] !== output || $[5] !== t4) { - t5 = ; + let t3; + if ($[4] !== output || $[5] !== t2) { + t3 = ; $[4] = output; - $[5] = t4; - $[6] = t5; + $[5] = t2; + $[6] = t3; } else { - t5 = $[6]; + t3 = $[6]; } - return t5; + return t3; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/snap/src/sprout/index.ts b/compiler/packages/snap/src/sprout/index.ts index 733be561c08a8..04748bed28f4f 100644 --- a/compiler/packages/snap/src/sprout/index.ts +++ b/compiler/packages/snap/src/sprout/index.ts @@ -32,7 +32,15 @@ export function runSprout( originalCode: string, forgetCode: string, ): SproutResult { - const forgetResult = doEval(forgetCode); + let forgetResult; + try { + (globalThis as any).__SNAP_EVALUATOR_MODE = 'forget'; + forgetResult = doEval(forgetCode); + } catch (e) { + throw e; + } finally { + (globalThis as any).__SNAP_EVALUATOR_MODE = undefined; + } if (forgetResult.kind === 'UnexpectedError') { return makeError('Unexpected error in Forget runner', forgetResult.value); } diff --git a/compiler/packages/snap/src/sprout/shared-runtime.ts b/compiler/packages/snap/src/sprout/shared-runtime.ts index e6e82d6b77e05..5687f26c45d0a 100644 --- a/compiler/packages/snap/src/sprout/shared-runtime.ts +++ b/compiler/packages/snap/src/sprout/shared-runtime.ts @@ -258,26 +258,35 @@ export function Throw() { export function ValidateMemoization({ inputs, - output, + output: rawOutput, + onlyCheckCompiled = false, }: { inputs: Array; output: any; + onlyCheckCompiled: boolean; }): React.ReactElement { 'use no forget'; + // Wrap rawOutput as it might be a function, which useState would invoke. + const output = {value: rawOutput}; const [previousInputs, setPreviousInputs] = React.useState(inputs); const [previousOutput, setPreviousOutput] = React.useState(output); if ( - inputs.length !== previousInputs.length || - inputs.some((item, i) => item !== previousInputs[i]) + onlyCheckCompiled && + (globalThis as any).__SNAP_EVALUATOR_MODE === 'forget' ) { - // Some input changed, we expect the output to change - setPreviousInputs(inputs); - setPreviousOutput(output); - } else if (output !== previousOutput) { - // Else output should be stable - throw new Error('Output identity changed but inputs did not'); + if ( + inputs.length !== previousInputs.length || + inputs.some((item, i) => item !== previousInputs[i]) + ) { + // Some input changed, we expect the output to change + setPreviousInputs(inputs); + setPreviousOutput(output); + } else if (output.value !== previousOutput.value) { + // Else output should be stable + throw new Error('Output identity changed but inputs did not'); + } } - return React.createElement(Stringify, {inputs, output}); + return React.createElement(Stringify, {inputs, output: rawOutput}); } export function createHookWrapper( From ffe8e0079a925a08145496234d91e1bb4334226a Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 19 Nov 2024 09:43:04 -0500 Subject: [PATCH 3/3] [compiler][ez] Add shape for global Object.keys Add shape / type for global Object.keys. This is useful because - it has an Effect.Read (not an Effect.Capture) as it cannot alias its argument. - Object.keys return an array --- .../src/HIR/Globals.ts | 15 ++++++ .../compiler/shapes-object-key.expect.md | 49 +++++++++++++++++++ .../fixtures/compiler/shapes-object-key.ts | 11 +++++ 3 files changed, 75 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/shapes-object-key.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/shapes-object-key.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts index 2525b87bd86bc..c85de65f06949 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts @@ -87,6 +87,21 @@ const UNTYPED_GLOBALS: Set = new Set([ ]); const TYPED_GLOBALS: Array<[string, BuiltInType]> = [ + [ + 'Object', + addObject(DEFAULT_SHAPES, 'Object', [ + [ + 'keys', + addFunction(DEFAULT_SHAPES, [], { + positionalParams: [Effect.Read], + restParam: null, + returnType: {kind: 'Object', shapeId: BuiltInArrayId}, + calleeEffect: Effect.Read, + returnValueKind: ValueKind.Mutable, + }), + ], + ]), + ], [ 'Array', addObject(DEFAULT_SHAPES, 'Array', [ diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/shapes-object-key.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/shapes-object-key.expect.md new file mode 100644 index 0000000000000..e491eb6c69d3c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/shapes-object-key.expect.md @@ -0,0 +1,49 @@ + +## Input + +```javascript +import {arrayPush} from 'shared-runtime'; + +function useFoo({a, b}) { + const obj = {a}; + arrayPush(Object.keys(obj), b); + return obj; +} +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: 2, b: 3}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { arrayPush } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(2); + const { a, b } = t0; + let t1; + if ($[0] !== a) { + t1 = { a }; + $[0] = a; + $[1] = t1; + } else { + t1 = $[1]; + } + const obj = t1; + arrayPush(Object.keys(obj), b); + return obj; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: 2, b: 3 }], +}; + +``` + +### Eval output +(kind: ok) {"a":2} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/shapes-object-key.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/shapes-object-key.ts new file mode 100644 index 0000000000000..9dbaac79c6b5e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/shapes-object-key.ts @@ -0,0 +1,11 @@ +import {arrayPush} from 'shared-runtime'; + +function useFoo({a, b}) { + const obj = {a}; + arrayPush(Object.keys(obj), b); + return obj; +} +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: 2, b: 3}], +};