diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts index 8f198d8367..1107b2f9a6 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -63,6 +63,7 @@ import { pruneUnusedScopes, renameVariables, } from "../ReactiveScopes"; +import { alignMethodCallScopes } from "../ReactiveScopes/AlignMethodCallScopes"; import { pruneAlwaysInvalidatingScopes } from "../ReactiveScopes/PruneAlwaysInvalidatingScopes"; import { eliminateRedundantPhi, enterSSA, leaveSSA } from "../SSA"; import { inferTypes } from "../TypeInference"; @@ -204,6 +205,13 @@ function* runWithEnvironment( inferReactiveScopeVariables(hir); yield log({ kind: "hir", name: "InferReactiveScopeVariables", value: hir }); + alignMethodCallScopes(hir); + yield log({ + kind: "hir", + name: "AlignMethodCallScopes", + value: hir, + }); + alignObjectMethodScopes(hir); yield log({ kind: "hir", diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignMethodCallScopes.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignMethodCallScopes.ts new file mode 100644 index 0000000000..1d52e6d679 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignMethodCallScopes.ts @@ -0,0 +1,80 @@ +/* + * 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 { + HIRFunction, + IdentifierId, + ReactiveScope, + makeInstructionId, +} from "../HIR"; +import DisjointSet from "../Utils/DisjointSet"; + +/** + * Ensures that method call instructions have scopes such that either: + * - Both the MethodCall and its property have the same scope + * - OR neither has a scope + */ +export function alignMethodCallScopes(fn: HIRFunction): void { + const scopeMapping = new Map(); + const mergedScopes = new DisjointSet(); + + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + const { lvalue, value } = instr; + if (value.kind === "MethodCall") { + const lvalueScope = lvalue.identifier.scope; + const propertyScope = value.property.identifier.scope; + if (lvalueScope !== null) { + if (propertyScope !== null) { + // Both have a scope: merge the scopes + mergedScopes.union([lvalueScope, propertyScope]); + } else { + /* + * Else the call itself has a scope but not the property, + * record that this property should be in this scope + */ + scopeMapping.set(value.property.identifier.id, lvalueScope); + } + } else if (propertyScope !== null) { + // else this property does not need a scope + scopeMapping.set(value.property.identifier.id, null); + } + } else if ( + value.kind === "FunctionExpression" || + value.kind === "ObjectMethod" + ) { + alignMethodCallScopes(value.loweredFunc.func); + } + } + } + + mergedScopes.forEach((scope, root) => { + if (scope === root) { + return; + } + root.range.start = makeInstructionId( + Math.min(scope.range.start, root.range.start) + ); + root.range.end = makeInstructionId( + Math.max(scope.range.end, root.range.end) + ); + }); + + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + const mappedScope = scopeMapping.get(instr.lvalue.identifier.id); + if (mappedScope !== undefined) { + instr.lvalue.identifier.scope = mappedScope; + } else if (instr.lvalue.identifier.scope !== null) { + const mergedScope = mergedScopes.find(instr.lvalue.identifier.scope); + if (mergedScope != null) { + instr.lvalue.identifier.scope = mergedScope; + } + } + } + } +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.expect.md new file mode 100644 index 0000000000..4ff0234428 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.expect.md @@ -0,0 +1,132 @@ + +## Input + +```javascript +// @flow @enableAssumeHooksFollowRulesOfReact +function Component({ label, highlightedItem }) { + const serverTime = useServerTime(); + const highlight = new Highlight(highlightedItem); + + const time = serverTime.get(); + // subtle bit here: the binary expression infers the result of the call + // as a primitive and not needing memoization. the logical is necessary + // because without it there are no intermediate scopes which observe + // the result of the binary expression, so its memoization can be pruned + const timestampLabel = time / 1000 || label; + + return ( + <> + {highlight.render()} + {timestampLabel} + + ); +} + +function useServerTime() { + "use no forget"; + + return { + get() { + return 42000; // would be a constant value from the server + }, + }; +} + +class Highlight { + constructor(value) { + this.value = value; + } + + render() { + return this.value; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ label: "", highlightedItem: "Seconds passed: " }], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(t0) { + const $ = useMemoCache(11); + const { label, highlightedItem } = t0; + const serverTime = useServerTime(); + let t1; + let timestampLabel; + if ($[0] !== highlightedItem || $[1] !== serverTime || $[2] !== label) { + const highlight = new Highlight(highlightedItem); + + const time = serverTime.get(); + let t2; + if ($[5] !== time || $[6] !== label) { + t2 = time / 1000 || label; + $[5] = time; + $[6] = label; + $[7] = t2; + } else { + t2 = $[7]; + } + timestampLabel = t2; + + t1 = highlight.render(); + $[0] = highlightedItem; + $[1] = serverTime; + $[2] = label; + $[3] = t1; + $[4] = timestampLabel; + } else { + t1 = $[3]; + timestampLabel = $[4]; + } + let t2; + if ($[8] !== t1 || $[9] !== timestampLabel) { + t2 = ( + <> + {t1} + {timestampLabel} + + ); + $[8] = t1; + $[9] = timestampLabel; + $[10] = t2; + } else { + t2 = $[10]; + } + return t2; +} + +function useServerTime() { + "use no forget"; + + return { + get() { + return 42000; + }, + }; +} + +class Highlight { + constructor(value) { + this.value = value; + } + + render() { + return this.value; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ label: "", highlightedItem: "Seconds passed: " }], +}; + +``` + +### Eval output +(kind: ok) Seconds passed: 42 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.js new file mode 100644 index 0000000000..1b5b12c54a --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.js @@ -0,0 +1,44 @@ +// @flow @enableAssumeHooksFollowRulesOfReact +function Component({ label, highlightedItem }) { + const serverTime = useServerTime(); + const highlight = new Highlight(highlightedItem); + + const time = serverTime.get(); + // subtle bit here: the binary expression infers the result of the call + // as a primitive and not needing memoization. the logical is necessary + // because without it there are no intermediate scopes which observe + // the result of the binary expression, so its memoization can be pruned + const timestampLabel = time / 1000 || label; + + return ( + <> + {highlight.render()} + {timestampLabel} + + ); +} + +function useServerTime() { + "use no forget"; + + return { + get() { + return 42000; // would be a constant value from the server + }, + }; +} + +class Highlight { + constructor(value) { + this.value = value; + } + + render() { + return this.value; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ label: "", highlightedItem: "Seconds passed: " }], +};