{"commit":{"authorName":"Joe Savona","authorEmail":"joesavona@fb.com","authoredAt":"2023-02-08T14:07:01-08:00","subject":"Dont memoize scopes with hook calls","shortRevision":"7f60f32118","revision":"7f60f321186ae64001adff905fdc1403aee327f2"},"body":"","parents":["c7e3bc4d413fd5a17597bd345dc8766ceed983ea"],"files":[{"status":"M","path":"compiler\/forget\/src\/CompilerPipeline.ts"},{"status":"M","path":"compiler\/forget\/src\/Inference\/InferReferenceEffects.ts"},{"status":"A","path":"compiler\/forget\/src\/ReactiveScopes\/FlattenScopesWithHooks.ts"},{"status":"M","path":"compiler\/forget\/src\/ReactiveScopes\/InferReactiveIdentifiers.ts"},{"status":"M","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/concise-arrow-expr.expect.md"},{"status":"A","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/controlled-input.expect.md"},{"status":"A","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/controlled-input.js"},{"status":"M","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/hook-call.expect.md"},{"status":"A","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/nested-scopes-hook-call.expect.md"},{"status":"A","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/nested-scopes-hook-call.js"},{"status":"M","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/optional-member-expression.expect.md"},{"status":"M","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/template-literal.expect.md"}],"patch":"diff --git a\/compiler\/forget\/src\/CompilerPipeline.ts b\/compiler\/forget\/src\/CompilerPipeline.ts\nindex a517d3f73f..9807f6c212 100644\n--- a\/compiler\/forget\/src\/CompilerPipeline.ts\n+++ b\/compiler\/forget\/src\/CompilerPipeline.ts\n@@ -33,6 +33,7 @@ import {\n pruneUnusedScopes,\n renameVariables,\n } from \".\/ReactiveScopes\";\n+import { flattenScopesWithHooks } from \".\/ReactiveScopes\/FlattenScopesWithHooks\";\n import { eliminateRedundantPhi, enterSSA, leaveSSA } from \".\/SSA\";\n import { inferTypes } from \".\/TypeInference\";\n import { logHIRFunction, logReactiveFunction } from \".\/Utils\/logger\";\n@@ -118,6 +119,13 @@ export function* run(\n value: reactiveFunction,\n });\n \n+ flattenScopesWithHooks(reactiveFunction);\n+ yield log({\n+ kind: \"reactive\",\n+ name: \"FlattenScopesWithHooks\",\n+ value: reactiveFunction,\n+ });\n+\n propagateScopeDependencies(reactiveFunction);\n yield log({\n kind: \"reactive\",\ndiff --git a\/compiler\/forget\/src\/Inference\/InferReferenceEffects.ts b\/compiler\/forget\/src\/Inference\/InferReferenceEffects.ts\nindex 79400f07dc..f00c4559bd 100644\n--- a\/compiler\/forget\/src\/Inference\/InferReferenceEffects.ts\n+++ b\/compiler\/forget\/src\/Inference\/InferReferenceEffects.ts\n@@ -801,7 +801,7 @@ const HOOKS: Map = new Map([\n type HookKind = { kind: \"State\" } | { kind: \"Ref\" } | { kind: \"Custom\" };\n type Hook = HookKind & { effectKind: Effect; valueKind: ValueKind };\n \n-function parseHookCall(place: Place): Hook | null {\n+export function parseHookCall(place: Place): Hook | null {\n const name = place.identifier.name;\n if (name === null || !name.match(\/^_?use\/)) {\n return null;\ndiff --git a\/compiler\/forget\/src\/ReactiveScopes\/FlattenScopesWithHooks.ts b\/compiler\/forget\/src\/ReactiveScopes\/FlattenScopesWithHooks.ts\nnew file mode 100644\nindex 0000000000..c737b25f88\n--- \/dev\/null\n+++ b\/compiler\/forget\/src\/ReactiveScopes\/FlattenScopesWithHooks.ts\n@@ -0,0 +1,66 @@\n+\/**\n+ * Copyright (c) Facebook, Inc. and its affiliates.\n+ *\n+ * This source code is licensed under the MIT license found in the\n+ * LICENSE file in the root directory of this source tree.\n+ *\/\n+\n+import {\n+ InstructionId,\n+ ReactiveFunction,\n+ ReactiveScopeBlock,\n+ ReactiveStatement,\n+ ReactiveValue,\n+} from \"..\/HIR\";\n+import { parseHookCall } from \"..\/Inference\/InferReferenceEffects\";\n+import {\n+ ReactiveFunctionTransform,\n+ Transformed,\n+ visitReactiveFunction,\n+} from \".\/visitors\";\n+\n+\/**\n+ * Most parts of compilation do not treat hooks specially, because there is no guarantee that custom\n+ * hooks obey any particular contract. For example, we can't assume that custom hooks won't modify\n+ * their arguments, and we can't assume that hooks return immutable or memoized values. Therefore\n+ * earlier passes largely ignore hooks, and may end up creating reactive scopes that contain hook calls.\n+ *\n+ * This pass then finds and removes any scopes that transitively contain a hook call. By running all\n+ * the reactive scope inference first, agnostic of hooks, we know that the reactive scopes accurately\n+ * describe the set of values which \"construct together\", and remove _all_ that memoization in order\n+ * to ensure the hook call does not inadvertently become conditional.\n+ *\/\n+export function flattenScopesWithHooks(fn: ReactiveFunction): void {\n+ visitReactiveFunction(fn, new Transform(), { hasHook: false });\n+}\n+\n+type State = { hasHook: boolean };\n+\n+class Transform extends ReactiveFunctionTransform {\n+ override transformScope(\n+ scope: ReactiveScopeBlock,\n+ outerState: State\n+ ): Transformed {\n+ const innerState: State = { hasHook: false };\n+ this.visitScope(scope, innerState);\n+ outerState.hasHook ||= innerState.hasHook;\n+ if (innerState.hasHook) {\n+ return { kind: \"replace-many\", value: scope.instructions };\n+ } else {\n+ return { kind: \"keep\" };\n+ }\n+ }\n+\n+ override visitValue(\n+ id: InstructionId,\n+ value: ReactiveValue,\n+ state: State\n+ ): void {\n+ if (value.kind === \"CallExpression\") {\n+ const hook = parseHookCall(value.callee);\n+ if (hook !== null) {\n+ state.hasHook = true;\n+ }\n+ }\n+ }\n+}\ndiff --git a\/compiler\/forget\/src\/ReactiveScopes\/InferReactiveIdentifiers.ts b\/compiler\/forget\/src\/ReactiveScopes\/InferReactiveIdentifiers.ts\nindex 3c4378c323..f080fba8f0 100644\n--- a\/compiler\/forget\/src\/ReactiveScopes\/InferReactiveIdentifiers.ts\n+++ b\/compiler\/forget\/src\/ReactiveScopes\/InferReactiveIdentifiers.ts\n@@ -12,6 +12,7 @@ import {\n ReactiveInstruction,\n ReactiveScope,\n } from \"..\/HIR\/HIR\";\n+import { parseHookCall } from \"..\/Inference\/InferReferenceEffects\";\n import {\n eachReactiveValueOperand,\n ReactiveFunctionVisitor,\n@@ -40,6 +41,17 @@ class Environment extends ReactiveFunctionVisitor {\n break;\n }\n }\n+ if (!hasReactiveInput && instr.value.kind === \"CallExpression\") {\n+ \/\/ Hooks cannot be memoized. Even if they do not accept any reactive inputs,\n+ \/\/ they are not guaranteed to memoize their return value, and their result\n+ \/\/ must be assumed to be reactive.\n+ \/\/ TODO: use types or an opt-in registry of custom hook information to\n+ \/\/ allow treating safe hooks as non-reactive.\n+ const hook = parseHookCall(instr.value.callee);\n+ if (hook !== null) {\n+ hasReactiveInput = true;\n+ }\n+ }\n reactivityMap.set(lval.place.identifier, hasReactiveInput);\n \n if (hasReactiveInput) {\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/concise-arrow-expr.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/concise-arrow-expr.expect.md\nindex 18b6421dad..ae0e1f74b0 100644\n--- a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/concise-arrow-expr.expect.md\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/concise-arrow-expr.expect.md\n@@ -15,29 +15,26 @@ function component() {\n ```javascript\n function component() {\n const $ = React.unstable_useMemoCache();\n- let t0;\n- if ($[0] === Symbol.for(\"react.memo_cache_sentinel\")) {\n- t0 = useState(0);\n- $[0] = t0;\n- } else {\n- t0 = $[0];\n- }\n- const setX = t0[1];\n+ const setX = useState(0)[1];\n+ const c_0 = $[0] !== setX;\n let handler;\n- if ($[1] === Symbol.for(\"react.memo_cache_sentinel\")) {\n+ if (c_0) {\n handler = (v) => setX(v);\n+ $[0] = setX;\n $[1] = handler;\n } else {\n handler = $[1];\n }\n- let t1;\n- if ($[2] === Symbol.for(\"react.memo_cache_sentinel\")) {\n- t1 = <\/Foo>;\n- $[2] = t1;\n+ const c_2 = $[2] !== handler;\n+ let t0;\n+ if (c_2) {\n+ t0 = <\/Foo>;\n+ $[2] = handler;\n+ $[3] = t0;\n } else {\n- t1 = $[2];\n+ t0 = $[3];\n }\n- return t1;\n+ return t0;\n }\n \n ```\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/controlled-input.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/controlled-input.expect.md\nnew file mode 100644\nindex 0000000000..0e309538c2\n--- \/dev\/null\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/controlled-input.expect.md\n@@ -0,0 +1,44 @@\n+\n+## Input\n+\n+```javascript\n+function component() {\n+ let [x, setX] = useState(0);\n+ const handler = (event) => setX(event.target.value);\n+ return ;\n+}\n+\n+```\n+\n+## Code\n+\n+```javascript\n+function component() {\n+ const $ = React.unstable_useMemoCache();\n+ const x = useState(0)[0];\n+ const setX = useState(0)[1];\n+ const c_0 = $[0] !== setX;\n+ let handler;\n+ if (c_0) {\n+ handler = (event) => setX(event.target.value);\n+ $[0] = setX;\n+ $[1] = handler;\n+ } else {\n+ handler = $[1];\n+ }\n+ const c_2 = $[2] !== handler;\n+ const c_3 = $[3] !== x;\n+ let t0;\n+ if (c_2 || c_3) {\n+ t0 = <\/input>;\n+ $[2] = handler;\n+ $[3] = x;\n+ $[4] = t0;\n+ } else {\n+ t0 = $[4];\n+ }\n+ return t0;\n+}\n+\n+```\n+ \n\\ No newline at end of file\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/controlled-input.js b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/controlled-input.js\nnew file mode 100644\nindex 0000000000..6a18b2d0ea\n--- \/dev\/null\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/controlled-input.js\n@@ -0,0 +1,5 @@\n+function component() {\n+ let [x, setX] = useState(0);\n+ const handler = (event) => setX(event.target.value);\n+ return ;\n+}\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/hook-call.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/hook-call.expect.md\nindex 77510f1cde..f2a4f22dcd 100644\n--- a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/hook-call.expect.md\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/hook-call.expect.md\n@@ -34,22 +34,18 @@ function Component(props) {\n } else {\n x = $[0];\n }\n- let y;\n- if ($[1] === Symbol.for(\"react.memo_cache_sentinel\")) {\n- y = useFreeze(x);\n- $[1] = y;\n- } else {\n- y = $[1];\n- }\n+ const y = useFreeze(x);\n foo(y, x);\n+ const c_1 = $[1] !== y;\n let t0;\n- if ($[2] === Symbol.for(\"react.memo_cache_sentinel\")) {\n+ if (c_1) {\n t0 = (\n \n {x}\n {y}\n <\/Component>\n );\n+ $[1] = y;\n $[2] = t0;\n } else {\n t0 = $[2];\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/nested-scopes-hook-call.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/nested-scopes-hook-call.expect.md\nnew file mode 100644\nindex 0000000000..600ff19f0f\n--- \/dev\/null\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/nested-scopes-hook-call.expect.md\n@@ -0,0 +1,27 @@\n+\n+## Input\n+\n+```javascript\n+function component(props) {\n+ let x = [];\n+ let y = [];\n+ y.push(useHook(props.foo));\n+ x.push(y);\n+ return x;\n+}\n+\n+```\n+\n+## Code\n+\n+```javascript\n+function component(props) {\n+ const x = [];\n+ const y = [];\n+ y.push(useHook(props.foo));\n+ x.push(y);\n+ return x;\n+}\n+\n+```\n+ \n\\ No newline at end of file\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/nested-scopes-hook-call.js b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/nested-scopes-hook-call.js\nnew file mode 100644\nindex 0000000000..a1b0858fb3\n--- \/dev\/null\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/nested-scopes-hook-call.js\n@@ -0,0 +1,7 @@\n+function component(props) {\n+ let x = [];\n+ let y = [];\n+ y.push(useHook(props.foo));\n+ x.push(y);\n+ return x;\n+}\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/optional-member-expression.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/optional-member-expression.expect.md\nindex 339c36c7f2..cbc15b72b1 100644\n--- a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/optional-member-expression.expect.md\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/optional-member-expression.expect.md\n@@ -27,15 +27,8 @@ function Foo(props) {\n x = $[1];\n }\n const y = x?.b;\n- const c_2 = $[2] !== y;\n- let z;\n- if (c_2) {\n- z = useBar(y);\n- $[2] = y;\n- $[3] = z;\n- } else {\n- z = $[3];\n- }\n+\n+ const z = useBar(y);\n return z;\n }\n \ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/template-literal.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/template-literal.expect.md\nindex 97ee8ca1f4..9d68dce234 100644\n--- a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/template-literal.expect.md\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/template-literal.expect.md\n@@ -25,17 +25,7 @@ function componentA(props) {\n }\n \n function componentB(props) {\n- const $ = React.unstable_useMemoCache();\n- const t0 = `hello ${props.a}`;\n- const c_0 = $[0] !== t0;\n- let x;\n- if (c_0) {\n- x = useFoo(t0);\n- $[0] = t0;\n- $[1] = x;\n- } else {\n- x = $[1];\n- }\n+ const x = useFoo(`hello ${props.a}`);\n return x;\n }\n \n"}

dillon1000/react

Files changed