From 79e3fc0acb3c4d3b8cd1caf86880b764e657ec42 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 13 Mar 2024 13:52:29 -0700 Subject: [PATCH] Fix for method call not memoizing in same scope as outer call Fixes T175282980. InferReactiveScopeVariables had logic to force assigning a scope to MethodCall property lookups with the idea of forcing the method call lookup to be in the same scope as the method call itself. But this doesn't work if we never assign a scope to the method call! That can happen if we're able to infer that the method call produces a primitive and doesn't need memoization. This PR changes things so that: * InferReactiveScopeVariables no longer assumes that MethodCall property values need a scope * We run a separate pass that ensures that _if_ a MethodCall has a scope, that it's property is in the scope, and that otherwise its property doesn't get a scope. This is similar to the existing passes that force a single scope for related instructions like ObjectMethod+ObjectExpression and fbt operands/calls. --- .../src/Entrypoint/Pipeline.ts | 8 ++ .../ReactiveScopes/AlignMethodCallScopes.ts | 80 +++++++++++ ...ed-property-load-for-method-call.expect.md | 132 ++++++++++++++++++ ...-memoized-property-load-for-method-call.js | 44 ++++++ 4 files changed, 264 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignMethodCallScopes.ts create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.js 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: " }], +};